> On 27-Sep-2023, at 8:55 PM, Michael S. Tsirkin <m...@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 <arm...@redhat.com>
>> CC: Philippe Mathieu-Daude <phi...@linaro.org>
>> CC: m...@redhat.com
>> Message-Id: <87r0mqlf9x....@pond.sub.org>
>> Signed-off-by: Ani Sinha <anisi...@redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
>> Reviewed-by: Peter Xu <pet...@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-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, &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
