Re: [PATCH 2/2] xen/x86/pvh: copy ACPI tables to Dom0 instead of mapping

[Xenia Ragiadakou <xenia.ragiadakou@amd.com>]

On 18/5/23 12:31, Roger Pau Monné wrote:
> On Thu, May 18, 2023 at 10:24:10AM +0300, Xenia Ragiadakou wrote:
>> On 15/5/23 17:17, Jan Beulich wrote:
>>> On 13.05.2023 03:17, Stefano Stabellini wrote:
>>>> From: Stefano Stabellini <stefano.stabellini@amd.com>
>>>>
>>>> Mapping the ACPI tables to Dom0 PVH 1:1 leads to memory corruptions of
>>>> the tables in the guest. Instead, copy the tables to Dom0.
>>> Do you really mean "in the guest" (i.e. from Xen's perspective, i.e.
>>> ignoring that when running on qemu it is kind of a guest itself)?
>>>
>>> I also consider the statement too broad anyway: Various people have
>>> run PVH Dom0 without running into such an issue, so it's clearly not
>>> just "leads to".
>> In my opinion the issue is broader.
>>
>> In pvh_setup_acpi(), the code adding the ACPI tables to dom0 memory map does
>> not check the return value of pvh_add_mem_range(). If there is an overlap
>> and the overlapping region is marked as E820_ACPI, it maps not just the
>> allowed tables but the entire overlapping range ,
> But that's the indented behavior: all ACPI regions will be mapped into
> the dom0 physmap, the filtering of the tables exposed to dom0 is done
> in the XSDT, but not in by filtering the mapped regions.  Note this
> won't be effective anyway, as the minimal granularity of physmap
> entries is 4k, so multiple tables could live in the same 4K region.
> Also Xen cannot parse dynamic tables (SSDT) or execute methods, and
> hence doesn't know exactly which memory will be used.
Thanks a lot for the explanation. I checked more carefully the code and 
it's true that xen does not aim to restrict dom0 access to ACPI tables. 
I got confused by the name of the function pvh_acpi_table_allowed.
>
> Xen relies on the firmware to have the ACPI tables in ACPI, NVS or
> RESERVED regions in order for them to be mapped into the gust physmap.
> The call to pvh_add_mem_range() in pvh_setup_acpi() is just an attempt
> to workaround broken systems that have tables placed in memory map
> holes, and hence ignoring the return value is fine.
In pvh_setup_acpi(), xen identity maps E820_ACPI and E820_NVS ranges to 
dom0. Why it does not do the same for E820_RESERVED, since ACPI tables 
may also lie there and since it does not know which memory will be used?
>> while if the overlapping
>> range is marked as E820_RESERVED, it does not map the tables at all (the
>> issue that Stefano saw with qemu). Since dom0 memory map is initialized
>> based on the native one, the code adding the ACPI table memory ranges will
>> naturally fall into one of the two cases above.
> Xen does map them, but that's done in arch_iommu_hwdom_init() which get
> short-circuited by the usage of dom0-iommu=none in your example.  See
> my reply to Stefano about moving such mappings into pvh_populate_p2m().
Indeed, if dom0-iommu=none is removed from the xen cmdline and qemu is 
configured with an iommu, the issue is not triggered. Because 
arch_iommu_hwdom_init() identity maps to dom0 at least the first 4G, right?
>> So even when not running into this issue, pvh_add_mem_range() still fails
>> and the memory range mapped is wider than the allowed one.
> The intention of that call to pvh_add_mem_range() is not to limit what
> gets mapped into dom0 physmap, but rather to workaround bugs in the
> firmware if ACPI tables are placed in memory map holes.
>
> Thanks, Roger.