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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 17, 2023 at 02:00:01PM -0700, Stefano Stabellini wrote:
> On Wed, 17 May 2023, Roger Pau Monné wrote:
> > On Tue, May 16, 2023 at 04:34:09PM -0700, Stefano Stabellini wrote:
> > > On Tue, 16 May 2023, Roger Pau Monné wrote:
> > > > On Mon, May 15, 2023 at 05:11:25PM -0700, Stefano Stabellini wrote:
> > > > > On Mon, 15 May 2023, Roger Pau Monné wrote:
> > > > > > On Fri, May 12, 2023 at 06:17:20PM -0700, Stefano Stabellini wrote:
> > > > > > > From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > > > > > > 
> > > > > > > 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.
> > > > > > > 
> > > > > > > This is a workaround.
> > > > > > > 
> > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > > > > > > ---
> > > > > > > As mentioned in the cover letter, this is a RFC workaround as I 
> > > > > > > don't
> > > > > > > know the cause of the underlying problem. I do know that this 
> > > > > > > patch
> > > > > > > solves what would be otherwise a hang at boot when Dom0 PVH 
> > > > > > > attempts to
> > > > > > > parse ACPI tables.
> > > > > > 
> > > > > > I'm unsure how safe this is for native systems, as it's possible for
> > > > > > firmware to modify the data in the tables, so copying them would
> > > > > > break that functionality.
> > > > > > 
> > > > > > I think we need to get to the root cause that triggers this behavior
> > > > > > on QEMU.  Is it the table checksum that fail, or something else?  Is
> > > > > > there an error from Linux you could reference?
> > > > > 
> > > > > I agree with you but so far I haven't managed to find a way to the 
> > > > > root
> > > > > of the issue. Here is what I know. These are the logs of a successful
> > > > > boot using this patch:
> > > > > 
> > > > > [   10.437488] ACPI: Early table checksum verification disabled
> > > > > [   10.439345] ACPI: RSDP 0x000000004005F955 000024 (v02 BOCHS )
> > > > > [   10.441033] ACPI: RSDT 0x000000004005F979 000034 (v01 BOCHS  
> > > > > BXPCRSDT 00000001 BXPC 00000001)
> > > > > [   10.444045] ACPI: APIC 0x0000000040060F76 00008A (v01 BOCHS  
> > > > > BXPCAPIC 00000001 BXPC 00000001)
> > > > > [   10.445984] ACPI: FACP 0x000000004005FA65 000074 (v01 BOCHS  
> > > > > BXPCFACP 00000001 BXPC 00000001)
> > > > > [   10.447170] ACPI BIOS Warning (bug): Incorrect checksum in table 
> > > > > [FACP] - 0x67, should be 0x30 (20220331/tbprint-174)
> > > > > [   10.449522] ACPI: DSDT 0x000000004005FB19 00145D (v01 BOCHS  
> > > > > BXPCDSDT 00000001 BXPC 00000001)
> > > > > [   10.451258] ACPI: FACS 0x000000004005FAD9 000040
> > > > > [   10.452245] ACPI: Reserving APIC table memory at [mem 
> > > > > 0x40060f76-0x40060fff]
> > > > > [   10.452389] ACPI: Reserving FACP table memory at [mem 
> > > > > 0x4005fa65-0x4005fad8]
> > > > > [   10.452497] ACPI: Reserving DSDT table memory at [mem 
> > > > > 0x4005fb19-0x40060f75]
> > > > > [   10.452602] ACPI: Reserving FACS table memory at [mem 
> > > > > 0x4005fad9-0x4005fb18]
> > > > > 
> > > > > 
> > > > > And these are the logs of the same boot (unsuccessful) without this
> > > > > patch:
> > > > > 
> > > > > [   10.516015] ACPI: Early table checksum verification disabled
> > > > > [   10.517732] ACPI: RSDP 0x0000000040060F1E 000024 (v02 BOCHS )
> > > > > [   10.519535] ACPI: RSDT 0x0000000040060F42 000034 (v01 BOCHS  
> > > > > BXPCRSDT 00000001 BXPC 00000001)
> > > > > [   10.522523] ACPI: APIC 0x0000000040060F76 00008A (v01 BOCHS  
> > > > > BXPCAPIC 00000001 BXPC 00000001)
> > > > > [   10.527453] ACPI: ���� 0x000000007FFE149D FFFFFFFF (v255 ������ 
> > > > > �������� FFFFFFFF ���� FFFFFFFF)
> > > > > [   10.528362] ACPI: Reserving APIC table memory at [mem 
> > > > > 0x40060f76-0x40060fff]
> > > > > [   10.528491] ACPI: Reserving ���� table memory at [mem 
> > > > > 0x7ffe149d-0x17ffe149b]
> > > > > 
> > > > > It is clearly a memory corruption around FACS but I couldn't find the
> > > > > reason for it. The mapping code looks correct. I hope you can suggest 
> > > > > a
> > > > > way to narrow down the problem. If I could, I would suggest to apply
> > > > > this patch just for the QEMU PVH tests but we don't have the
> > > > > infrastructure for that in gitlab-ci as there is a single Xen build 
> > > > > for
> > > > > all tests.
> > > > 
> > > > Would be helpful to see the memory map provided to Linux, just in case
> > > > we messed up and there's some overlap.
> > > 
> > > Everything looks correct. Here are some more logs:
> > > 
> > > (XEN) Xen-e820 RAM map:
> > > (XEN)  [0000000000000000, 000000000009fbff] (usable)
> > > (XEN)  [000000000009fc00, 000000000009ffff] (reserved)
> > > (XEN)  [00000000000f0000, 00000000000fffff] (reserved)
> > > (XEN)  [0000000000100000, 000000007ffdffff] (usable)
> > > (XEN)  [000000007ffe0000, 000000007fffffff] (reserved)
> > > (XEN)  [00000000fffc0000, 00000000ffffffff] (reserved)
> > > (XEN)  [000000fd00000000, 000000ffffffffff] (reserved)
> > > (XEN) Microcode loading not available
> > > (XEN) New Xen image base address: 0x7f600000
> > > (XEN) System RAM: 2047MB (2096636kB)
> > > (XEN) ACPI: RSDP 000F58D0, 0014 (r0 BOCHS )
> > > (XEN) ACPI: RSDT 7FFE1B21, 0034 (r1 BOCHS  BXPC            1 BXPC        
> > > 1)
> > > (XEN) ACPI: FACP 7FFE19CD, 0074 (r1 BOCHS  BXPC            1 BXPC        
> > > 1)
> > > (XEN) ACPI: DSDT 7FFE0040, 198D (r1 BOCHS  BXPC            1 BXPC        
> > > 1)
> > > (XEN) ACPI: FACS 7FFE0000, 0040
> > > (XEN) ACPI: APIC 7FFE1A41, 0080 (r1 BOCHS  BXPC            1 BXPC        
> > > 1)
> > > (XEN) ACPI: HPET 7FFE1AC1, 0038 (r1 BOCHS  BXPC            1 BXPC        
> > > 1)
> > > (XEN) ACPI: WAET 7FFE1AF9, 0028 (r1 BOCHS  BXPC            1 BXPC        
> > > 1)
> > > [...]
> > > (XEN) Dom0 memory map:
> > > (XEN)  [0000000000000000, 000000000009efff] (usable)
> > > (XEN)  [000000000009fc00, 000000000009ffff] (reserved)
> > > (XEN)  [00000000000f0000, 00000000000fffff] (reserved)
> > > (XEN)  [0000000000100000, 0000000040060f1d] (usable)
> > > (XEN)  [0000000040060f1e, 0000000040060fa7] (ACPI data)
> > > (XEN)  [0000000040061000, 000000007ffdffff] (unusable)
> > > (XEN)  [000000007ffe0000, 000000007fffffff] (reserved)
> > > (XEN)  [00000000fffc0000, 00000000ffffffff] (reserved)
> > > (XEN)  [000000fd00000000, 000000ffffffffff] (reserved)
> > > [...]
> > > [    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009efff] 
> > > usable
> > > [    0.000000] BIOS-e820: [mem 0x000000000009fc00-0x00000000000fffff] 
> > > reserved
> > > [    0.000000] BIOS-e820: [mem 0x0000000000100000-0x0000000040060f1d] 
> > > usable
> > > [    0.000000] BIOS-e820: [mem 0x0000000040060f1e-0x0000000040060fa7] 
> > > ACPI data
> > > [    0.000000] BIOS-e820: [mem 0x0000000040061000-0x000000007ffdffff] 
> > > unusable
> > > [    0.000000] BIOS-e820: [mem 0x000000007ffe0000-0x000000007fffffff] 
> > > reserved
> > > [    0.000000] BIOS-e820: [mem 0x00000000fffc0000-0x00000000ffffffff] 
> > > reserved
> > > [    0.000000] BIOS-e820: [mem 0x000000fd00000000-0x000000ffffffffff] 
> > > reserved
> > > [...]
> > > [   10.102427] ACPI: Early table checksum verification disabled
> > > [   10.104455] ACPI: RSDP 0x0000000040060F1E 000024 (v02 BOCHS )
> > > [   10.106250] ACPI: RSDT 0x0000000040060F42 000034 (v01 BOCHS  BXPC     
> > > 00000001 BXPC 00000001)
> > > [   10.109549] ACPI: APIC 0x0000000040060F76 00008A (v01 BOCHS  BXPC     
> > > 00000001 BXPC 00000001)
> > > [   10.115173] ACPI: ���� 0x000000007FFE19CD FFFFFFFF (v255 ������ 
> > > �������� FFFFFFFF ���� FFFFFFFF)
> > > [   10.116054] ACPI: Reserving APIC table memory at [mem 
> > > 0x40060f76-0x40060fff]
> > > [   10.116182] ACPI: Reserving ���� table memory at [mem 
> > > 0x7ffe19cd-0x17ffe19cb]
> > > 
> > > 
> > > 
> > > > It seems like some of the XSDT entries (the FADT one) is corrupt?
> > > > 
> > > > Could you maybe add some debug to the Xen-crafted XSDT placement.
> > > 
> > > I added a printk just after:
> > > 
> > >   xsdt->table_offset_entry[j++] = tables[i].address;
> > > 
> > > And it printed only once:
> > > 
> > >   (XEN) DEBUG pvh_setup_acpi_xsdt 1000 name=FACP address=7ffe19cd
> > > 
> > > That actually matches the address read by Linux:
> > > 
> > >   [   10.175448] ACPI: ���� 0x000000007FFE19CD FFFFFFFF (v255 ������ 
> > > �������� FFFFFFFF ���� FFFFFFFF)
> > > 
> > > So the address seems correct. It is the content of the FADT/FACP table
> > > that is corrupted.
> > > 
> > > I wrote the following function in Xen:
> > > 
> > > static void check(void)
> > > {
> > >     unsigned long addr = 0x7ffe19cd;
> > >     struct acpi_table_fadt *fadt;
> > >     fadt = acpi_os_map_memory(addr, sizeof(*fadt));
> > >     printk("DEBUG %s %d s=%s\n",__func__,__LINE__,fadt->header.signature);
> > >     acpi_os_unmap_memory(fadt, sizeof(*fadt));
> > > }
> > > 
> > > It prints the right table signature at the end of pvh_setup_acpi.
> > > I also added a call at the top of xenmem_add_to_physmap_one, and the
> > > signature is still correct. Then I added a call at the beginning of
> > > __update_vcpu_system_time. Here is the surprise: from Xen point of view
> > > the table never gets corrupted. Here are the logs:
> > > 
> > > [...]
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) d0v0: upcall vector f3
> > > [    0.000000] Linux version 6.1.19 (root@124de7fbba7f) (gcc (Debian 
> > > 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40) #1 SMP PREEMPT_3
> > > [    0.000000] Command line: console=hvc0
> > > [...]
> > > [   10.371610] ACPI: Early table checksum verification disabled
> > > [   10.373633] ACPI: RSDP 0x0000000040060F1E 000024 (v02 BOCHS )
> > > [   10.375548] ACPI: RSDT 0x0000000040060F42 000034 (v01 BOCHS  BXPC     
> > > 00000001 BXPC 00000001)
> > > [   10.378732] ACPI: APIC 0x0000000040060F76 00008A (v01 BOCHS  BXPC     
> > > 00000001 BXPC 00000001)
> > > [   10.384188] ACPI: ���� 0x000000007FFE19CD FFFFFFFF (v255 ������ 
> > > �������� FFFFFFFF ���� FFFFFFFF)
> > > [   10.385374] ACPI: Reserving APIC table memory at [mem 
> > > 0x40060f76-0x40060fff]
> > > [   10.385519] ACPI: Reserving ���� table memory at [mem 
> > > 0x7ffe19cd-0x17ffe19cb]
> > > [...]
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > (XEN) DEBUG fadt_check 1551 s=FACPt
> > > 
> > > 
> > > So it looks like it is a problem with the mapping itself? Xen sees the
> > > data correctly and Linux sees it corrupted?
> > 
> > It seems to me like the page is not correctly mapped, and so 1s are
> > returned? (same behavior as a hole).  IOW: would seem to me like MMIO
> > areas are not correctly handled by nested NPT? (I assume you are
> > running this on AMD).
> > 
> > Does it make a difference if you try to boot with dom0=pvh,shadow?
> > 
> > A couple of wild ideas.  Maybe the nested virt support that you are
> > using doesn't handle the UC bit in second stage page table entries?
> > You could to remove this in p2m_type_to_flags() (see the
> > p2m_mmio_direct case).
> > 
> > Another wild idea I have is that the emulated NPT code doesn't like
> > having the bits 63:52 from the PTE set to anything different than 0,
> > and hence only p2m_ram_rw works (p2m_mmio_direct is 5).
> 
> Many thanks to Xenia for figuring out the root cause of the bug. The
> underlying memory region is already added as E820_RESERVED to the guest
> (instead of E820_ACPI). When pvh_add_mem_range is called with E820_ACPI
> as parameter for the interesting table, pvh_add_mem_range returns with
> -EEXIST without doing anything.
> 
> The original fix by Xenia was to carve out the relevant subset of the
> reserved region and mark it as E820_ACPI. Instead, I rewrote it as
> changing the type of the entire region to E820_ACPI because it is
> simpler and doesn't have to deal with the edge cases (partially
> overlapping, overlapping 2 existing regions, etc.)
> 
> What do you think?

Hm, I'm unsure whether wholesale converting reserved regions into ACPI
ones is correct, for example Xen will handle reserved regions specially
when creating the IOMMU mappings, and RMRRs are also expected to live
in reserved regions.

The issue is IMO with the usage of dom0-iommu=none, which leads to
reserved regions not mapped in the dom0 physmap (see
arch_iommu_hwdom_init()).

One option might be to move the mapping of reserved regions from
arch_iommu_hwdom_init() into PVH dom0 build (as part of
pvh_populate_p2m()) and thus render arch_iommu_hwdom_init() PV only.
That would also imply that for PVH dom0 `dom0-iommu=map-reserved` is
fixed cannot be modified (iow: reserved regions are always added to
the PVH dom0 physmap).

Thanks, Roger.