diff mbox series

[v2] hw/i386: changes towards enabling -Wshadow=local

Message ID 20230926055235.9164-1-anisinha@redhat.com
State New
Headers show
Series [v2] hw/i386: changes towards enabling -Wshadow=local | expand

Commit Message

Ani Sinha Sept. 26, 2023, 5:52 a.m. UTC
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 <armbru@redhat.com>
CC: Philippe Mathieu-Daude <philmd@linaro.org>
CC: mst@redhat.com
Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
Signed-off-by: Ani Sinha <anisinha@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 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-peterx@redhat.com
and removed mine.

Comments

Michael S. Tsirkin Sept. 27, 2023, 3:25 p.m. UTC | #1
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 <armbru@redhat.com>
> CC: Philippe Mathieu-Daude <philmd@linaro.org>
> CC: mst@redhat.com
> Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---


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-peterx@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, &irq, 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, &idx);
>      if (!cpu_slot) {
> -        MachineState *ms = MACHINE(x86ms);
> -
>          x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
>          error_setg(errp,
>              "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"


killing dead code, nice

> -- 
> 2.39.3
Ani Sinha Sept. 28, 2023, 3:44 a.m. UTC | #2
> On 27-Sep-2023, at 8:55 PM, Michael S. Tsirkin <mst@redhat.com> 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 <armbru@redhat.com>
>> CC: Philippe Mathieu-Daude <philmd@linaro.org>
>> CC: mst@redhat.com
>> Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> ---
> 
> 
> 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-peterx@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, &irq, 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, &idx);
>>     if (!cpu_slot) {
>> -        MachineState *ms = MACHINE(x86ms);
>> -
>>         x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
>>         error_setg(errp,
>>             "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> 
> 
> killing dead code, nice
> 
>> -- 
>> 2.39.3
Michael S. Tsirkin Oct. 2, 2023, 10:47 a.m. UTC | #3
On Thu, Sep 28, 2023 at 09:14:07AM +0530, Ani Sinha wrote:
> 
> 
> > On 27-Sep-2023, at 8:55 PM, Michael S. Tsirkin <mst@redhat.com> 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 <armbru@redhat.com>
> >> CC: Philippe Mathieu-Daude <philmd@linaro.org>
> >> CC: mst@redhat.com
> >> Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >> Reviewed-by: Peter Xu <peterx@redhat.com>
> >> ---
> > 
> > 
> > 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-peterx@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, &irq, 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
> >> @@ -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, &idx);
> >>     if (!cpu_slot) {
> >> -        MachineState *ms = MACHINE(x86ms);
> >> -
> >>         x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> >>         error_setg(errp,
> >>             "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> > 
> > 
> > killing dead code, nice
> > 
> >> -- 
> >> 2.39.3
Ani Sinha Oct. 3, 2023, 5:51 a.m. UTC | #4
On Mon, Oct 2, 2023 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <armbru@redhat.com>
> > >> CC: Philippe Mathieu-Daude <philmd@linaro.org>
> > >> CC: mst@redhat.com
> > >> Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
> > >> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > >> Reviewed-by: Peter Xu <peterx@redhat.com>
> > >> ---
> > >
> > >
> > > 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 <mst@redhat.com>
M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
   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-peterx@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, &irq, 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)
> > >>     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, &idx);
> > >>     if (!cpu_slot) {
> > >> -        MachineState *ms = MACHINE(x86ms);
> > >> -
> > >>         x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> > >>         error_setg(errp,
> > >>             "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> > >
> > >
> > > killing dead code, nice
> > >
> > >> --
> > >> 2.39.3
>
diff mbox series

Patch

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, &irq, 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, &idx);
     if (!cpu_slot) {
-        MachineState *ms = MACHINE(x86ms);
-
         x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
         error_setg(errp,
             "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"