Re: [PATCH 02/15] hw/pci-host/q35: Fix double, contradicting .endianness assignment

[Michael S. Tsirkin]

On Tue, Jun 13, 2023 at 03:40:28PM +0200, Philippe Mathieu-Daudé wrote:
> On 13/6/23 15:05, Igor Mammedov wrote:
> > On Tue, 13 Jun 2023 13:07:17 +0200 (CEST)
> > BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > 
> > > On Tue, 13 Jun 2023, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 13, 2023 at 09:46:53AM +0200, Bernhard Beschow wrote:
> > > > > On Mon, Jun 12, 2023 at 3:01 PM Igor Mammedov <imammedo@redhat.com> 
> > > > > wrote:
> > > > > 
> > > > >      On Sun, 11 Jun 2023 12:33:59 +0200
> > > > >      Bernhard Beschow <shentey@gmail.com> wrote:
> > > > >     > Fixes the following clangd warning (-Winitializer-overrides):
> > > > >     >
> > > > >     >   q35.c:297:19: Initializer overrides prior initialization of 
> > > > > this
> > > > >      subobject
> > > > >     >   q35.c:292:19: previous initialization is here
> > > > >     >
> > > > >     > Settle on native endian which causes the least overhead.
> > > > >      indeed it doesn't matter which way we read all ones, so that 
> > > > > should work.
> > > > >      but does it really matter (I mean the overhead/what workload)?
> > > > >      If not, I'd prefer explicit LE as it's now to be consistent
> > > > >      the the rest of memops on Q35.
> 
> Meaning trying to optimize this single MR on big-endian is irrelevant :)
> 
> > > > > 
> > > > > I got a comment from Michael about this in [1], so I've changed it. I 
> > > > > don't
> > > > > mind changing it either way.
> > > > > 
> > > > > Best regards,
> > > > > Bernhard
> > > > > 
> > > > > [1] 
> > > > > https://patchew.org/QEMU/20230214131441.101760-1-shentey@gmail.com/
> > > > > 20230214131441.101760-3-shentey@gmail.com/#
> > > > > 20230301164339-mutt-send-email-mst@kernel.org
> > > > 
> > > > Hmm it's not terribly important, and the optimization is trivial,
> > > > but yes people tend to copy code, good point. Maybe add a comment?
> > > > /*
> > > > * Note: don't copy this!  normally use DEVICE_LITTLE_ENDIAN. This only
> > > > * works because we don't allow writes and always read all-ones.
> > > > */
> > > 
> > > Why don't you leave DEVICE_LITTLE_ENDIAN and remove DEVICE_NATIVE_ENDIAN
> > > instead? If this only returns all 1s then it does not matter and also
> > > DEVICE_LITTLE_ENDIAN was the last assignment so likely that was effective
> > > so far anyway.
> > 
> > I'm in favor of this as well,
> > the 'optimization' and then piling comments on top to clarify confusion
> > should be justified by usefulness of it (no perf numbers/usecase were 
> > present so far).
> > In absence of above, I'd prefer real hw behavior (LE in this case).


OK I'm fine with this too. Sorry about leading you astray.

-- 
MST