On 17 January 2017 at 09:49, Andrew Jones <drjo...@redhat.com> wrote:
> On Mon, Jan 16, 2017 at 07:31:33PM +0000, Ard Biesheuvel wrote:
>> On 16 January 2017 at 18:20, Peter Maydell <peter.mayd...@linaro.org> wrote:
>> > On 16 January 2017 at 17:30, Ard Biesheuvel <ard.biesheu...@linaro.org>
>> > wrote:
>> >> On 16 January 2017 at 17:25, Peter Maydell <peter.mayd...@linaro.org>
>> >> wrote:
>> >>> On 13 January 2017 at 17:32, Ard Biesheuvel <ard.biesheu...@linaro.org>
>> >>> 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 <ard.biesheu...@linaro.org>
>> >>>> ---
>> >>>> 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
