Re: [Qemu-devel] [PATCH v2] hw/arm/virt-acpi - reserve ECAM space as PNP0C02 device

[Ard Biesheuvel]

On 17 January 2017 at 09:49, Andrew Jones <address@hidden> wrote:
> On Mon, Jan 16, 2017 at 07:31:33PM +0000, Ard Biesheuvel wrote:
>> On 16 January 2017 at 18:20, Peter Maydell <address@hidden> wrote:
>> > On 16 January 2017 at 17:30, Ard Biesheuvel <address@hidden> wrote:
>> >> On 16 January 2017 at 17:25, Peter Maydell <address@hidden> wrote:
>> >>> On 13 January 2017 at 17:32, Ard Biesheuvel <address@hidden> wrote:
>> >>>> Linux for arm64 v4.10 and later will complain if the ECAM config space 
>> >>>> is
>> >>>> not reserved in the ACPI namespace:
>> >>>>
>> >>>>   acpi PNP0A08:00: [Firmware Bug]: ECAM area [mem 
>> >>>> 0x3f000000-0x3fffffff] not reserved in ACPI namespace
>> >>>>
>> >>>> The rationale is that OSes that don't consume the MCFG table should 
>> >>>> still
>> >>>> be able to infer that the PCI config space MMIO region is occupied.
>> >>>>
>> >>>> So update the ACPI table generation routine to add this reservation.
>> >>>>
>> >>>> Signed-off-by: Ard Biesheuvel <address@hidden>
>> >>>> ---
>> >>>>  hw/arm/virt-acpi-build.c | 7 +++++++
>> >>>>  1 file changed, 7 insertions(+)
>> >>>>
>> >>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> >>>> index 085a61117378..50d52f685f68 100644
>> >>>> --- a/hw/arm/virt-acpi-build.c
>> >>>> +++ b/hw/arm/virt-acpi-build.c
>> >>>> @@ -310,6 +310,13 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
>> >>>> MemMapEntry *memmap,
>> >>>>      Aml *dev_rp0 = aml_device("%s", "RP0");
>> >>>>      aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
>> >>>>      aml_append(dev, dev_rp0);
>> >>>> +
>> >>>> +    Aml *dev_res0 = aml_device("%s", "RES0");
>> >>>> +    aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>> >>>> +    crs = aml_resource_template();
>> >>>> +    aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, 
>> >>>> AML_READ_WRITE));
>> >>>> +    aml_append(dev_res0, aml_name_decl("_CRS", crs));
>> >>>> +    aml_append(dev, dev_res0);
>> >>>>      aml_append(scope, dev);
>> >>>>  }
>> >>>
>> >>> This needs to be controlled via the machine class back-compat
>> >>> machinery in hw/arm/virt.c so that it only happens for virt-2.9
>> >>> and later.
>> >>>
>> >>
>> >> Why exactly?
>> >
>> > Because the "virt-2.8" machine has to present to the guest
>> > exactly what "virt" did as of the QEMU 2.8 release, including
>> > any bugs or missing things we happened to have in our ACPI
>> > tables. This allows cross-version compatibility (including
>> > VM migration). Drew will have a more detailed explanation
>> > if you need it.
>> >
>>
>> I suspected as much.
>>
>> But in this case, I am not sure if it is worth the trouble: the
>> generated data is only consumed at boot time by the firmware, and I
>> suppose migration involves freezing a VM, including whatever resident
>> firmware image was used to boot the OS, and so this is unlikely to
>> affect migration.
>>
>> But I will let Drew explain ...
>>
>
> In some cases the problem we're solving with the compat guards is
> a bit hypothetical, but, IMHO, nonetheless a good practice. While
> we may be sure that AAVMF and Linux will be fine with this table
> changing under their feet, we can't be sure there aren't other
> mach-virt users that have more sensitive firmwares/OSes. An ACPI-
> sensitive OS may notice the change on its next reboot after a
> migration, and then simply refuse to continue.
>
> Now, that said, I just spoke with Igor in order to learn the x86
> practice. He says that the policy has been more lax than what I
> suggest above. Hypothetical, low-risk issues are left unguarded,
> and only when a bug is found during testing is it then managed.
> The idea is to try and reduce the amount of compat variables and
> conditions needed in the ACPI generation code, but, of course, at
> some level of risk to users expecting their versioned machine type
> to always appear the same.
>
> So far we've been strict with mach-virt, guarding all hypothetical
> issues. Perhaps this patch is a good example to get a discussion
> started on whether or not we should be so strict though.
>

Yes please. I don't mind respinning the patch, but I agree that it
makes sense to consider whether minimal bug fixes like this one
require this treatment in the first place