On Tue, 13 Jun 2023 13:07:17 +0200 (CEST) BALATON Zoltan <bala...@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 <imamm...@redhat.com> wrote: > >> > >> On Sun, 11 Jun 2023 12:33:59 +0200 > >> Bernhard Beschow <shen...@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-shen...@gmail.com/ > >> 20230214131441.101760-3-shen...@gmail.com/# > >> 20230301164339-mutt-send-email-...@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 <shen...@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 */ > >> > >> > > > > > >