Re: [PATCH v2] hw/i386: changes towards enabling -Wshadow=local
On Tue, Oct 03, 2023 at 11:13:06AM +0530, Ani Sinha wrote:
> Code changes that addresses all compiler complaints coming from enabling
> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
> other local variables or parameters. These makes the code confusing and/or
> adds
> bugs that are difficult to catch.
>
> CC: Markus Armbruster
> CC: Philippe Mathieu-Daude
> CC: m...@redhat.com
> Message-Id: <87r0mqlf9x@pond.sub.org>
> Signed-off-by: Ani Sinha
> Reviewed-by: Daniel P. Berrangé
> Reviewed-by: Peter Xu
Reviewed-by: Michael S. Tsirkin
feel free to move with the rest of these changes.
> ---
> hw/i386/acpi-microvm.c | 4 ++--
> hw/i386/intel_iommu.c | 8
> hw/i386/pc.c | 1 -
> hw/i386/x86.c | 2 --
> 4 files changed, 6 insertions(+), 9 deletions(-)
>
> changelog:
> v2: addressed suggestion from mst.
>
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index a075360d85..6ddcfb0419 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -55,8 +55,8 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>
> bus = sysbus_get_default();
> QTAILQ_FOREACH(kid, >children, sibling) {
> -DeviceState *dev = kid->child;
> -Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MMIO);
> +Object *obj = object_dynamic_cast(OBJECT(kid->child),
> + TYPE_VIRTIO_MMIO);
>
> if (obj) {
> VirtIOMMIOProxy *mmio = VIRTIO_MMIO(obj);
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c0ce896668..2c832ab68b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> PCIBus *bus,
> /* Unmap the whole range in the notifier's scope. */
> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> {
> -hwaddr size, remain;
> +hwaddr total, remain;
> hwaddr start = n->start;
> hwaddr end = n->end;
> IntelIOMMUState *s = as->iommu_state;
> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> }
>
> assert(start <= end);
> -size = remain = end - start + 1;
> +total = remain = end - start + 1;
>
> while (remain >= VTD_PAGE_SIZE) {
> IOMMUTLBEvent event;
> @@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
> VTD_PCI_SLOT(as->devfn),
> VTD_PCI_FUNC(as->devfn),
> - n->start, size);
> + n->start, total);
>
> map.iova = n->start;
> -map.size = size - 1; /* Inclusive */
> +map.size = total - 1; /* Inclusive */
> iova_tree_remove(as->iova_tree, map);
> }
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3db0743f31..e7a233e886 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
>
> if (machine->device_memory) {
> uint64_t *val = g_malloc(sizeof(*val));
> -PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> uint64_t res_mem_end = machine->device_memory->base;
>
> if (!pcmc->broken_reserved_end) {
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index f034df8bf6..b3d054889b 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>
> cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
> if (!cpu_slot) {
> -MachineState *ms = MACHINE(x86ms);
> -
> x86_topo_ids_from_apicid(cpu->apic_id, _info, _ids);
> error_setg(errp,
> "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> --
> 2.42.0
Re: [PATCH v2] hw/i386: changes towards enabling -Wshadow=local
> On 03-Oct-2023, at 11:13 AM, Ani Sinha wrote:
>
> Code changes that addresses all compiler complaints coming from enabling
> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
> other local variables or parameters. These makes the code confusing and/or
> adds
> bugs that are difficult to catch.
>
> CC: Markus Armbruster
> CC: Philippe Mathieu-Daude
> CC: m...@redhat.com
> Message-Id: <87r0mqlf9x@pond.sub.org>
> Signed-off-by: Ani Sinha
> Reviewed-by: Daniel P. Berrangé
> Reviewed-by: Peter Xu
> ---
> hw/i386/acpi-microvm.c | 4 ++--
> hw/i386/intel_iommu.c | 8
> hw/i386/pc.c | 1 -
> hw/i386/x86.c | 2 --
> 4 files changed, 6 insertions(+), 9 deletions(-)
>
> changelog:
> v2: addressed suggestion from mst.
Please ignore this. This was supposed to be v3. I will re-send. Will split-up
intel_iommu into a separate patch.
>
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index a075360d85..6ddcfb0419 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -55,8 +55,8 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>
> bus = sysbus_get_default();
> QTAILQ_FOREACH(kid, >children, sibling) {
> -DeviceState *dev = kid->child;
> -Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MMIO);
> +Object *obj = object_dynamic_cast(OBJECT(kid->child),
> + TYPE_VIRTIO_MMIO);
>
> if (obj) {
> VirtIOMMIOProxy *mmio = VIRTIO_MMIO(obj);
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c0ce896668..2c832ab68b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> PCIBus *bus,
> /* Unmap the whole range in the notifier's scope. */
> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> {
> -hwaddr size, remain;
> +hwaddr total, remain;
> hwaddr start = n->start;
> hwaddr end = n->end;
> IntelIOMMUState *s = as->iommu_state;
> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> }
>
> assert(start <= end);
> -size = remain = end - start + 1;
> +total = remain = end - start + 1;
>
> while (remain >= VTD_PAGE_SIZE) {
> IOMMUTLBEvent event;
> @@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
> VTD_PCI_SLOT(as->devfn),
> VTD_PCI_FUNC(as->devfn),
> - n->start, size);
> + n->start, total);
>
> map.iova = n->start;
> -map.size = size - 1; /* Inclusive */
> +map.size = total - 1; /* Inclusive */
> iova_tree_remove(as->iova_tree, map);
> }
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3db0743f31..e7a233e886 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
>
> if (machine->device_memory) {
> uint64_t *val = g_malloc(sizeof(*val));
> -PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> uint64_t res_mem_end = machine->device_memory->base;
>
> if (!pcmc->broken_reserved_end) {
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index f034df8bf6..b3d054889b 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>
> cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
> if (!cpu_slot) {
> -MachineState *ms = MACHINE(x86ms);
> -
> x86_topo_ids_from_apicid(cpu->apic_id, _info, _ids);
> error_setg(errp,
> "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> --
> 2.42.0
>
Re: [PATCH v2] hw/i386: changes towards enabling -Wshadow=local
On Mon, Oct 2, 2023 at 4:17 PM Michael S. Tsirkin wrote:
>
> On Thu, Sep 28, 2023 at 09:14:07AM +0530, Ani Sinha wrote:
> >
> >
> > > On 27-Sep-2023, at 8:55 PM, Michael S. Tsirkin wrote:
> > >
> > > On Tue, Sep 26, 2023 at 11:22:35AM +0530, Ani Sinha wrote:
> > >> Code changes that addresses all compiler complaints coming from enabling
> > >> -Wshadow flags. Enabling -Wshadow catches cases of local variables
> > >> shadowing
> > >> other local variables or parameters. These makes the code confusing
> > >> and/or adds
> > >
> > > These make
> > >
> > >> bugs that are difficult to catch.
> > >>
> > >> CC: Markus Armbruster
> > >> CC: Philippe Mathieu-Daude
> > >> CC: m...@redhat.com
> > >> Message-Id: <87r0mqlf9x@pond.sub.org>
> > >> Signed-off-by: Ani Sinha
> > >> Reviewed-by: Daniel P. Berrangé
> > >> Reviewed-by: Peter Xu
> > >> ---
> > >
> > >
> > > chunks seem unrelated. why not split them up?
> >
> > ? No idea what you talking about. Here and ...
>
> you patch 4 files in a single patch.
> intel_iommu is part of vtd emulation and
> has separate maintainers. Slightly better to split up
> to have each maintainer get just the patches
> he cares about.
> Not critical, for sure.
this was from the original email that Markus sent that had this group:
X86 Machines
PC
M: Michael S. Tsirkin
M: Marcel Apfelbaum
hw/i386/acpi-build.c(*3*)
hw/i386/acpi-microvm.c(*2*)
hw/i386/intel_iommu.c(*3*)
hw/i386/pc.c(*2*)
hw/i386/x86.c(*2*)
Not sure why it was clubbed with others.
>
>
> > >
> > >> hw/i386/acpi-microvm.c | 12 ++--
> > >> hw/i386/intel_iommu.c | 8
> > >> hw/i386/pc.c | 1 -
> > >> hw/i386/x86.c | 2 --
> > >> 4 files changed, 10 insertions(+), 13 deletions(-)
> > >>
> > >> changelog:
> > >> v2: kept Peter's changes from
> > >> https://lore.kernel.org/r/20230922160410.138786-1-pet...@redhat.com
> > >> and removed mine.
> > >>
> > >> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> > >> index a075360d85..6e4f8061eb 100644
> > >> --- a/hw/i386/acpi-microvm.c
> > >> +++ b/hw/i386/acpi-microvm.c
> > >> @@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> > >> hwaddr base = VIRTIO_MMIO_BASE + index * 512;
> > >> hwaddr size = 512;
> > >>
> > >> -Aml *dev = aml_device("VR%02u", (unsigned)index);
> > >> -aml_append(dev, aml_name_decl("_HID",
> > >> aml_string("LNRO0005")));
> > >> -aml_append(dev, aml_name_decl("_UID", aml_int(index)));
> > >> -aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > >> +Aml *adev = aml_device("VR%02u", (unsigned)index);
> > >> +aml_append(adev, aml_name_decl("_HID",
> > >> aml_string("LNRO0005")));
> > >> +aml_append(adev, aml_name_decl("_UID", aml_int(index)));
> > >> +aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
> > >>
> > >> Aml *crs = aml_resource_template();
> > >> aml_append(crs, aml_memory32_fixed(base, size,
> > >> AML_READ_WRITE));
> > >> aml_append(crs,
> > >>aml_interrupt(AML_CONSUMER, AML_LEVEL,
> > >> AML_ACTIVE_HIGH,
> > >> AML_EXCLUSIVE, , 1));
> > >> -aml_append(dev, aml_name_decl("_CRS", crs));
> > >> -aml_append(scope, dev);
> > >> +aml_append(adev, aml_name_decl("_CRS", crs));
> > >> +aml_append(scope, adev);
> > >> }
> > >> }
> > >> }
> > >
> > > I would prefer to just drop the devicestate dev pointer, use kid->child
> > > inside the
> > > macro.
good idea. addressed in v2.
> >
> > Here …
> >
>
> Well, you renamed dev to adev because there's another dev at
> an outer scope which is set to kid->child, and only used
> once. I suggest just dropping that one instead of removing
> this one.
>
>
> > >
> > >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > >> index c0ce896668..2c832ab68b 100644
> > >> --- a/hw/i386/intel_iommu.c
> > >> +++ b/hw/i386/intel_iommu.c
> > >> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
> > >> *s, PCIBus *bus,
> > >> /* Unmap the whole range in the notifier's scope. */
> > >> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier
> > >> *n)
> > >> {
> > >> -hwaddr size, remain;
> > >> +hwaddr total, remain;
> > >> hwaddr start = n->start;
> > >> hwaddr end = n->end;
> > >> IntelIOMMUState *s = as->iommu_state;
> > >> @@ -3765,7 +3765,7 @@ static void
> > >> vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> > >> }
> > >>
> > >> assert(start <= end);
> > >> -size = remain = end - start + 1;
> > >> +total = remain = end - start + 1;
> > >>
> > >> while (remain >= VTD_PAGE_SIZE) {
> > >> IOMMUTLBEvent event;
> > >> @@ -3793,10 +3793,10 @@ static void
> > >> vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> > >>
[PATCH v2] hw/i386: changes towards enabling -Wshadow=local
Code changes that addresses all compiler complaints coming from enabling
-Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
other local variables or parameters. These makes the code confusing and/or adds
bugs that are difficult to catch.
CC: Markus Armbruster
CC: Philippe Mathieu-Daude
CC: m...@redhat.com
Message-Id: <87r0mqlf9x@pond.sub.org>
Signed-off-by: Ani Sinha
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Peter Xu
---
hw/i386/acpi-microvm.c | 4 ++--
hw/i386/intel_iommu.c | 8
hw/i386/pc.c | 1 -
hw/i386/x86.c | 2 --
4 files changed, 6 insertions(+), 9 deletions(-)
changelog:
v2: addressed suggestion from mst.
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index a075360d85..6ddcfb0419 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -55,8 +55,8 @@ static void acpi_dsdt_add_virtio(Aml *scope,
bus = sysbus_get_default();
QTAILQ_FOREACH(kid, >children, sibling) {
-DeviceState *dev = kid->child;
-Object *obj = object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MMIO);
+Object *obj = object_dynamic_cast(OBJECT(kid->child),
+ TYPE_VIRTIO_MMIO);
if (obj) {
VirtIOMMIOProxy *mmio = VIRTIO_MMIO(obj);
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..2c832ab68b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
PCIBus *bus,
/* Unmap the whole range in the notifier's scope. */
static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
{
-hwaddr size, remain;
+hwaddr total, remain;
hwaddr start = n->start;
hwaddr end = n->end;
IntelIOMMUState *s = as->iommu_state;
@@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace *as,
IOMMUNotifier *n)
}
assert(start <= end);
-size = remain = end - start + 1;
+total = remain = end - start + 1;
while (remain >= VTD_PAGE_SIZE) {
IOMMUTLBEvent event;
@@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
*as, IOMMUNotifier *n)
trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
VTD_PCI_SLOT(as->devfn),
VTD_PCI_FUNC(as->devfn),
- n->start, size);
+ n->start, total);
map.iova = n->start;
-map.size = size - 1; /* Inclusive */
+map.size = total - 1; /* Inclusive */
iova_tree_remove(as->iova_tree, map);
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..e7a233e886 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
if (machine->device_memory) {
uint64_t *val = g_malloc(sizeof(*val));
-PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
uint64_t res_mem_end = machine->device_memory->base;
if (!pcmc->broken_reserved_end) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f034df8bf6..b3d054889b 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
if (!cpu_slot) {
-MachineState *ms = MACHINE(x86ms);
-
x86_topo_ids_from_apicid(cpu->apic_id, _info, _ids);
error_setg(errp,
"Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
--
2.42.0
Re: [PATCH v2] hw/i386: changes towards enabling -Wshadow=local
On Thu, Sep 28, 2023 at 09:14:07AM +0530, Ani Sinha wrote:
>
>
> > On 27-Sep-2023, at 8:55 PM, Michael S. Tsirkin wrote:
> >
> > On Tue, Sep 26, 2023 at 11:22:35AM +0530, Ani Sinha wrote:
> >> Code changes that addresses all compiler complaints coming from enabling
> >> -Wshadow flags. Enabling -Wshadow catches cases of local variables
> >> shadowing
> >> other local variables or parameters. These makes the code confusing and/or
> >> adds
> >
> > These make
> >
> >> bugs that are difficult to catch.
> >>
> >> CC: Markus Armbruster
> >> CC: Philippe Mathieu-Daude
> >> CC: m...@redhat.com
> >> Message-Id: <87r0mqlf9x@pond.sub.org>
> >> Signed-off-by: Ani Sinha
> >> Reviewed-by: Daniel P. Berrangé
> >> Reviewed-by: Peter Xu
> >> ---
> >
> >
> > chunks seem unrelated. why not split them up?
>
> ? No idea what you talking about. Here and ...
you patch 4 files in a single patch.
intel_iommu is part of vtd emulation and
has separate maintainers. Slightly better to split up
to have each maintainer get just the patches
he cares about.
Not critical, for sure.
> >
> >> hw/i386/acpi-microvm.c | 12 ++--
> >> hw/i386/intel_iommu.c | 8
> >> hw/i386/pc.c | 1 -
> >> hw/i386/x86.c | 2 --
> >> 4 files changed, 10 insertions(+), 13 deletions(-)
> >>
> >> changelog:
> >> v2: kept Peter's changes from
> >> https://lore.kernel.org/r/20230922160410.138786-1-pet...@redhat.com
> >> and removed mine.
> >>
> >> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> >> index a075360d85..6e4f8061eb 100644
> >> --- a/hw/i386/acpi-microvm.c
> >> +++ b/hw/i386/acpi-microvm.c
> >> @@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> >> hwaddr base = VIRTIO_MMIO_BASE + index * 512;
> >> hwaddr size = 512;
> >>
> >> -Aml *dev = aml_device("VR%02u", (unsigned)index);
> >> -aml_append(dev, aml_name_decl("_HID",
> >> aml_string("LNRO0005")));
> >> -aml_append(dev, aml_name_decl("_UID", aml_int(index)));
> >> -aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> >> +Aml *adev = aml_device("VR%02u", (unsigned)index);
> >> +aml_append(adev, aml_name_decl("_HID",
> >> aml_string("LNRO0005")));
> >> +aml_append(adev, aml_name_decl("_UID", aml_int(index)));
> >> +aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
> >>
> >> Aml *crs = aml_resource_template();
> >> aml_append(crs, aml_memory32_fixed(base, size,
> >> AML_READ_WRITE));
> >> aml_append(crs,
> >>aml_interrupt(AML_CONSUMER, AML_LEVEL,
> >> AML_ACTIVE_HIGH,
> >> AML_EXCLUSIVE, , 1));
> >> -aml_append(dev, aml_name_decl("_CRS", crs));
> >> -aml_append(scope, dev);
> >> +aml_append(adev, aml_name_decl("_CRS", crs));
> >> +aml_append(scope, adev);
> >> }
> >> }
> >> }
> >
> > I would prefer to just drop the devicestate dev pointer, use kid->child
> > inside the
> > macro.
>
> Here …
>
Well, you renamed dev to adev because there's another dev at
an outer scope which is set to kid->child, and only used
once. I suggest just dropping that one instead of removing
this one.
> >
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index c0ce896668..2c832ab68b 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> >> PCIBus *bus,
> >> /* Unmap the whole range in the notifier's scope. */
> >> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> >> {
> >> -hwaddr size, remain;
> >> +hwaddr total, remain;
> >> hwaddr start = n->start;
> >> hwaddr end = n->end;
> >> IntelIOMMUState *s = as->iommu_state;
> >> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace
> >> *as, IOMMUNotifier *n)
> >> }
> >>
> >> assert(start <= end);
> >> -size = remain = end - start + 1;
> >> +total = remain = end - start + 1;
> >>
> >> while (remain >= VTD_PAGE_SIZE) {
> >> IOMMUTLBEvent event;
> >> @@ -3793,10 +3793,10 @@ static void
> >> vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> >> trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
> >> VTD_PCI_SLOT(as->devfn),
> >> VTD_PCI_FUNC(as->devfn),
> >> - n->start, size);
> >> + n->start, total);
> >>
> >> map.iova = n->start;
> >> -map.size = size - 1; /* Inclusive */
> >> +map.size = total - 1; /* Inclusive */
> >> iova_tree_remove(as->iova_tree, map);
> >> }
> >>
> >
> >
> > arguably an improvement
> >
> >
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 3db0743f31..e7a233e886 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
Re: [PATCH v2] hw/i386: changes towards enabling -Wshadow=local
> On 27-Sep-2023, at 8:55 PM, Michael S. Tsirkin wrote:
>
> On Tue, Sep 26, 2023 at 11:22:35AM +0530, Ani Sinha wrote:
>> Code changes that addresses all compiler complaints coming from enabling
>> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
>> other local variables or parameters. These makes the code confusing and/or
>> adds
>
> These make
>
>> bugs that are difficult to catch.
>>
>> CC: Markus Armbruster
>> CC: Philippe Mathieu-Daude
>> CC: m...@redhat.com
>> Message-Id: <87r0mqlf9x@pond.sub.org>
>> Signed-off-by: Ani Sinha
>> Reviewed-by: Daniel P. Berrangé
>> Reviewed-by: Peter Xu
>> ---
>
>
> chunks seem unrelated. why not split them up?
? No idea what you talking about. Here and ...
>
>> hw/i386/acpi-microvm.c | 12 ++--
>> hw/i386/intel_iommu.c | 8
>> hw/i386/pc.c | 1 -
>> hw/i386/x86.c | 2 --
>> 4 files changed, 10 insertions(+), 13 deletions(-)
>>
>> changelog:
>> v2: kept Peter's changes from
>> https://lore.kernel.org/r/20230922160410.138786-1-pet...@redhat.com
>> and removed mine.
>>
>> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
>> index a075360d85..6e4f8061eb 100644
>> --- a/hw/i386/acpi-microvm.c
>> +++ b/hw/i386/acpi-microvm.c
>> @@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>> hwaddr base = VIRTIO_MMIO_BASE + index * 512;
>> hwaddr size = 512;
>>
>> -Aml *dev = aml_device("VR%02u", (unsigned)index);
>> -aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
>> -aml_append(dev, aml_name_decl("_UID", aml_int(index)));
>> -aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>> +Aml *adev = aml_device("VR%02u", (unsigned)index);
>> +aml_append(adev, aml_name_decl("_HID", aml_string("LNRO0005")));
>> +aml_append(adev, aml_name_decl("_UID", aml_int(index)));
>> +aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
>>
>> Aml *crs = aml_resource_template();
>> aml_append(crs, aml_memory32_fixed(base, size, AML_READ_WRITE));
>> aml_append(crs,
>>aml_interrupt(AML_CONSUMER, AML_LEVEL,
>> AML_ACTIVE_HIGH,
>> AML_EXCLUSIVE, , 1));
>> -aml_append(dev, aml_name_decl("_CRS", crs));
>> -aml_append(scope, dev);
>> +aml_append(adev, aml_name_decl("_CRS", crs));
>> +aml_append(scope, adev);
>> }
>> }
>> }
>
> I would prefer to just drop the devicestate dev pointer, use kid->child
> inside the
> macro.
Here …
>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index c0ce896668..2c832ab68b 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
>> PCIBus *bus,
>> /* Unmap the whole range in the notifier's scope. */
>> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>> {
>> -hwaddr size, remain;
>> +hwaddr total, remain;
>> hwaddr start = n->start;
>> hwaddr end = n->end;
>> IntelIOMMUState *s = as->iommu_state;
>> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace
>> *as, IOMMUNotifier *n)
>> }
>>
>> assert(start <= end);
>> -size = remain = end - start + 1;
>> +total = remain = end - start + 1;
>>
>> while (remain >= VTD_PAGE_SIZE) {
>> IOMMUTLBEvent event;
>> @@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
>> *as, IOMMUNotifier *n)
>> trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>> VTD_PCI_SLOT(as->devfn),
>> VTD_PCI_FUNC(as->devfn),
>> - n->start, size);
>> + n->start, total);
>>
>> map.iova = n->start;
>> -map.size = size - 1; /* Inclusive */
>> +map.size = total - 1; /* Inclusive */
>> iova_tree_remove(as->iova_tree, map);
>> }
>>
>
>
> arguably an improvement
>
>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 3db0743f31..e7a233e886 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
>>
>> if (machine->device_memory) {
>> uint64_t *val = g_malloc(sizeof(*val));
>> -PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> uint64_t res_mem_end = machine->device_memory->base;
>>
>> if (!pcmc->broken_reserved_end) {
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index f034df8bf6..b3d054889b 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>
>> cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
>> if (!cpu_slot) {
>> -MachineState *ms = MACHINE(x86ms);
>> -
>>
Re: [PATCH v2] hw/i386: changes towards enabling -Wshadow=local
On Tue, Sep 26, 2023 at 11:22:35AM +0530, Ani Sinha wrote:
> Code changes that addresses all compiler complaints coming from enabling
> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
> other local variables or parameters. These makes the code confusing and/or
> adds
These make
> bugs that are difficult to catch.
>
> CC: Markus Armbruster
> CC: Philippe Mathieu-Daude
> CC: m...@redhat.com
> Message-Id: <87r0mqlf9x@pond.sub.org>
> Signed-off-by: Ani Sinha
> Reviewed-by: Daniel P. Berrangé
> Reviewed-by: Peter Xu
> ---
chunks seem unrelated. why not split them up?
> hw/i386/acpi-microvm.c | 12 ++--
> hw/i386/intel_iommu.c | 8
> hw/i386/pc.c | 1 -
> hw/i386/x86.c | 2 --
> 4 files changed, 10 insertions(+), 13 deletions(-)
>
> changelog:
> v2: kept Peter's changes from
> https://lore.kernel.org/r/20230922160410.138786-1-pet...@redhat.com
> and removed mine.
>
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index a075360d85..6e4f8061eb 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> hwaddr base = VIRTIO_MMIO_BASE + index * 512;
> hwaddr size = 512;
>
> -Aml *dev = aml_device("VR%02u", (unsigned)index);
> -aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
> -aml_append(dev, aml_name_decl("_UID", aml_int(index)));
> -aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> +Aml *adev = aml_device("VR%02u", (unsigned)index);
> +aml_append(adev, aml_name_decl("_HID", aml_string("LNRO0005")));
> +aml_append(adev, aml_name_decl("_UID", aml_int(index)));
> +aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
>
> Aml *crs = aml_resource_template();
> aml_append(crs, aml_memory32_fixed(base, size, AML_READ_WRITE));
> aml_append(crs,
> aml_interrupt(AML_CONSUMER, AML_LEVEL,
> AML_ACTIVE_HIGH,
> AML_EXCLUSIVE, , 1));
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -aml_append(scope, dev);
> +aml_append(adev, aml_name_decl("_CRS", crs));
> +aml_append(scope, adev);
> }
> }
> }
I would prefer to just drop the devicestate dev pointer, use kid->child inside
the
macro.
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c0ce896668..2c832ab68b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> PCIBus *bus,
> /* Unmap the whole range in the notifier's scope. */
> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> {
> -hwaddr size, remain;
> +hwaddr total, remain;
> hwaddr start = n->start;
> hwaddr end = n->end;
> IntelIOMMUState *s = as->iommu_state;
> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> }
>
> assert(start <= end);
> -size = remain = end - start + 1;
> +total = remain = end - start + 1;
>
> while (remain >= VTD_PAGE_SIZE) {
> IOMMUTLBEvent event;
> @@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
> *as, IOMMUNotifier *n)
> trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
> VTD_PCI_SLOT(as->devfn),
> VTD_PCI_FUNC(as->devfn),
> - n->start, size);
> + n->start, total);
>
> map.iova = n->start;
> -map.size = size - 1; /* Inclusive */
> +map.size = total - 1; /* Inclusive */
> iova_tree_remove(as->iova_tree, map);
> }
>
arguably an improvement
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3db0743f31..e7a233e886 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
>
> if (machine->device_memory) {
> uint64_t *val = g_malloc(sizeof(*val));
> -PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> uint64_t res_mem_end = machine->device_memory->base;
>
> if (!pcmc->broken_reserved_end) {
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index f034df8bf6..b3d054889b 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>
> cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
> if (!cpu_slot) {
> -MachineState *ms = MACHINE(x86ms);
> -
> x86_topo_ids_from_apicid(cpu->apic_id, _info, _ids);
> error_setg(errp,
> "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
killing dead code, nice
> --
> 2.39.3
[PATCH v2] hw/i386: changes towards enabling -Wshadow=local
Code changes that addresses all compiler complaints coming from enabling
-Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
other local variables or parameters. These makes the code confusing and/or adds
bugs that are difficult to catch.
CC: Markus Armbruster
CC: Philippe Mathieu-Daude
CC: m...@redhat.com
Message-Id: <87r0mqlf9x@pond.sub.org>
Signed-off-by: Ani Sinha
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Peter Xu
---
hw/i386/acpi-microvm.c | 12 ++--
hw/i386/intel_iommu.c | 8
hw/i386/pc.c | 1 -
hw/i386/x86.c | 2 --
4 files changed, 10 insertions(+), 13 deletions(-)
changelog:
v2: kept Peter's changes from
https://lore.kernel.org/r/20230922160410.138786-1-pet...@redhat.com
and removed mine.
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index a075360d85..6e4f8061eb 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
hwaddr base = VIRTIO_MMIO_BASE + index * 512;
hwaddr size = 512;
-Aml *dev = aml_device("VR%02u", (unsigned)index);
-aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
-aml_append(dev, aml_name_decl("_UID", aml_int(index)));
-aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
+Aml *adev = aml_device("VR%02u", (unsigned)index);
+aml_append(adev, aml_name_decl("_HID", aml_string("LNRO0005")));
+aml_append(adev, aml_name_decl("_UID", aml_int(index)));
+aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
Aml *crs = aml_resource_template();
aml_append(crs, aml_memory32_fixed(base, size, AML_READ_WRITE));
aml_append(crs,
aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, , 1));
-aml_append(dev, aml_name_decl("_CRS", crs));
-aml_append(scope, dev);
+aml_append(adev, aml_name_decl("_CRS", crs));
+aml_append(scope, adev);
}
}
}
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..2c832ab68b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
PCIBus *bus,
/* Unmap the whole range in the notifier's scope. */
static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
{
-hwaddr size, remain;
+hwaddr total, remain;
hwaddr start = n->start;
hwaddr end = n->end;
IntelIOMMUState *s = as->iommu_state;
@@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace *as,
IOMMUNotifier *n)
}
assert(start <= end);
-size = remain = end - start + 1;
+total = remain = end - start + 1;
while (remain >= VTD_PAGE_SIZE) {
IOMMUTLBEvent event;
@@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace
*as, IOMMUNotifier *n)
trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
VTD_PCI_SLOT(as->devfn),
VTD_PCI_FUNC(as->devfn),
- n->start, size);
+ n->start, total);
map.iova = n->start;
-map.size = size - 1; /* Inclusive */
+map.size = total - 1; /* Inclusive */
iova_tree_remove(as->iova_tree, map);
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..e7a233e886 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
if (machine->device_memory) {
uint64_t *val = g_malloc(sizeof(*val));
-PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
uint64_t res_mem_end = machine->device_memory->base;
if (!pcmc->broken_reserved_end) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f034df8bf6..b3d054889b 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, );
if (!cpu_slot) {
-MachineState *ms = MACHINE(x86ms);
-
x86_topo_ids_from_apicid(cpu->apic_id, _info, _ids);
error_setg(errp,
"Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
--
2.39.3
