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

[Igor Mammedov]

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.
> >>
> >>
> >> 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).

> 
> Regards,
> BALATON Zoltan
> 
> >>  
> >>    >
> >>    > Fixes: bafc90bdc594 ("q35: implement TSEG")
> >>    > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >>    > ---
> >>    >  hw/pci-host/q35.c | 1 -
> >>    >  1 file changed, 1 deletion(-)
> >>    >
> >>    > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >>    > index fd18920e7f..859c197f25 100644
> >>    > --- a/hw/pci-host/q35.c
> >>    > +++ b/hw/pci-host/q35.c
> >>    > @@ -290,7 +290,6 @@ static const MemoryRegionOps blackhole_ops = {
> >>    >      .valid.max_access_size = 4,
> >>    >      .impl.min_access_size = 4,
> >>    >      .impl.max_access_size = 4,
> >>    > -    .endianness = DEVICE_LITTLE_ENDIAN,
> >>    >  };
> >>    > 
> >>    >  /* PCIe MMCFG */  
> >>
> >>  
> >
> >
> >