| Message ID | 20230611103412.12109-3-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Q35 and I440FX host bridge QOM cleanup | expand |
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. > > 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 */
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 > > > > > 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 */ > >
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. */ > > > > > 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 */ > >
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. 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 */ >> >> > > >
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 */ > >> > >> > > > > > >
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).
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.
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
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 */
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. Fixes: bafc90bdc594 ("q35: implement TSEG") Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/pci-host/q35.c | 1 - 1 file changed, 1 deletion(-)