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

[Bernhard Beschow]

Am 13. Juni 2023 15:01:40 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>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.

No worries. I'm happy to change it back to LE.

Best regards,
Bernhard