| Message ID | 20230513011720.3978354-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PVH Dom0 on QEMU | expand |
On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > From: Stefano Stabellini <stefano.stabellini@amd.com> > > Xen always generates a XSDT table even if the firmware provided a RSDT > table. Instead of copying the XSDT header from the firmware table (that > might be missing), generate the XSDT header from a preset. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > --- > xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > 1 file changed, 9 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index 307edc6a8c..5fde769863 100644 > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > paddr_t *addr) > { > struct acpi_table_xsdt *xsdt; > - struct acpi_table_header *table; > - struct acpi_table_rsdp *rsdp; > const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > unsigned long size = sizeof(*xsdt); > unsigned int i, j, num_tables = 0; > - paddr_t xsdt_paddr; > int rc; > + struct acpi_table_header header = { > + .signature = "XSDT", > + .length = sizeof(struct acpi_table_header), > + .revision = 0x1, > + .oem_id = "Xen", > + .oem_table_id = "HVM", I think this is wrong, as according to the spec the OEM Table ID must match the OEM Table ID in the FADT. We likely want to copy the OEM ID and OEM Table ID from the RSDP, and possibly also the other OEM related fields. Alternatively we might want to copy and use the RSDT on systems that lack an XSDT, or even just copy the header from the RSDT into Xen's crafted XSDT, since the format of the RSDP and the XSDT headers are exactly the same (the difference is in the size of the description headers that come after). > + .oem_revision = 0, > + }; This wants to be initdata static const if we go down this route. Thanks, Roger.
On 15.05.2023 10:48, Roger Pau Monné wrote: > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: >> From: Stefano Stabellini <stefano.stabellini@amd.com> >> >> Xen always generates a XSDT table even if the firmware provided a RSDT >> table. Instead of copying the XSDT header from the firmware table (that >> might be missing), generate the XSDT header from a preset. >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >> --- >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- >> 1 file changed, 9 insertions(+), 23 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c >> index 307edc6a8c..5fde769863 100644 >> --- a/xen/arch/x86/hvm/dom0_build.c >> +++ b/xen/arch/x86/hvm/dom0_build.c >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, >> paddr_t *addr) >> { >> struct acpi_table_xsdt *xsdt; >> - struct acpi_table_header *table; >> - struct acpi_table_rsdp *rsdp; >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; >> unsigned long size = sizeof(*xsdt); >> unsigned int i, j, num_tables = 0; >> - paddr_t xsdt_paddr; >> int rc; >> + struct acpi_table_header header = { >> + .signature = "XSDT", >> + .length = sizeof(struct acpi_table_header), >> + .revision = 0x1, >> + .oem_id = "Xen", >> + .oem_table_id = "HVM", > > I think this is wrong, as according to the spec the OEM Table ID must > match the OEM Table ID in the FADT. > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > possibly also the other OEM related fields. > > Alternatively we might want to copy and use the RSDT on systems that > lack an XSDT, or even just copy the header from the RSDT into Xen's > crafted XSDT, since the format of the RSDP and the XSDT headers are > exactly the same (the difference is in the size of the description > headers that come after). I guess I'd prefer that last variant. ACPI specifically says "Platforms provide the RSDT to enable compatibility with ACPI 1.0 operating systems." IOW any halfway modern system (including qemu, that is) ought to supply an XSDT anyway. Jan
On Mon, 15 May 2023, Jan Beulich wrote: > On 15.05.2023 10:48, Roger Pau Monné wrote: > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > >> From: Stefano Stabellini <stefano.stabellini@amd.com> > >> > >> Xen always generates a XSDT table even if the firmware provided a RSDT > >> table. Instead of copying the XSDT header from the firmware table (that > >> might be missing), generate the XSDT header from a preset. > >> > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > >> --- > >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > >> 1 file changed, 9 insertions(+), 23 deletions(-) > >> > >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > >> index 307edc6a8c..5fde769863 100644 > >> --- a/xen/arch/x86/hvm/dom0_build.c > >> +++ b/xen/arch/x86/hvm/dom0_build.c > >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > >> paddr_t *addr) > >> { > >> struct acpi_table_xsdt *xsdt; > >> - struct acpi_table_header *table; > >> - struct acpi_table_rsdp *rsdp; > >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > >> unsigned long size = sizeof(*xsdt); > >> unsigned int i, j, num_tables = 0; > >> - paddr_t xsdt_paddr; > >> int rc; > >> + struct acpi_table_header header = { > >> + .signature = "XSDT", > >> + .length = sizeof(struct acpi_table_header), > >> + .revision = 0x1, > >> + .oem_id = "Xen", > >> + .oem_table_id = "HVM", > > > > I think this is wrong, as according to the spec the OEM Table ID must > > match the OEM Table ID in the FADT. > > > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > > possibly also the other OEM related fields. > > > > Alternatively we might want to copy and use the RSDT on systems that > > lack an XSDT, or even just copy the header from the RSDT into Xen's > > crafted XSDT, since the format of the RSDP and the XSDT headers are > > exactly the same (the difference is in the size of the description > > headers that come after). > > I guess I'd prefer that last variant. I tried this approach (together with the second patch for necessity) and it worked. diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index fd2cbf68bc..11d6d1bc23 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, goto out; } xsdt_paddr = rsdp->xsdt_physical_address; + if ( !xsdt_paddr ) + { + xsdt_paddr = rsdp->rsdt_physical_address; + } acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); if ( !table )
On 16.05.2023 02:16, Stefano Stabellini wrote: > On Mon, 15 May 2023, Jan Beulich wrote: >> On 15.05.2023 10:48, Roger Pau Monné wrote: >>> Alternatively we might want to copy and use the RSDT on systems that >>> lack an XSDT, or even just copy the header from the RSDT into Xen's >>> crafted XSDT, since the format of the RSDP and the XSDT headers are >>> exactly the same (the difference is in the size of the description >>> headers that come after). >> >> I guess I'd prefer that last variant. > > I tried this approach (together with the second patch for necessity) and > it worked. Which I find slightly surprising - a fully conforming consumer of the tables may expect an XSDT when xsdt_physical_address is set, and reject RSDT. We may still want to limit ourselves to this less involved approach, but then with a code comment clearly stating the limitation. Jan > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > goto out; > } > xsdt_paddr = rsdp->xsdt_physical_address; > + if ( !xsdt_paddr ) > + { > + xsdt_paddr = rsdp->rsdt_physical_address; > + } > acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > if ( !table )
On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: > On Mon, 15 May 2023, Jan Beulich wrote: > > On 15.05.2023 10:48, Roger Pau Monné wrote: > > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > > >> From: Stefano Stabellini <stefano.stabellini@amd.com> > > >> > > >> Xen always generates a XSDT table even if the firmware provided a RSDT > > >> table. Instead of copying the XSDT header from the firmware table (that > > >> might be missing), generate the XSDT header from a preset. > > >> > > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > >> --- > > >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > > >> 1 file changed, 9 insertions(+), 23 deletions(-) > > >> > > >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > >> index 307edc6a8c..5fde769863 100644 > > >> --- a/xen/arch/x86/hvm/dom0_build.c > > >> +++ b/xen/arch/x86/hvm/dom0_build.c > > >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > >> paddr_t *addr) > > >> { > > >> struct acpi_table_xsdt *xsdt; > > >> - struct acpi_table_header *table; > > >> - struct acpi_table_rsdp *rsdp; > > >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > > >> unsigned long size = sizeof(*xsdt); > > >> unsigned int i, j, num_tables = 0; > > >> - paddr_t xsdt_paddr; > > >> int rc; > > >> + struct acpi_table_header header = { > > >> + .signature = "XSDT", > > >> + .length = sizeof(struct acpi_table_header), > > >> + .revision = 0x1, > > >> + .oem_id = "Xen", > > >> + .oem_table_id = "HVM", > > > > > > I think this is wrong, as according to the spec the OEM Table ID must > > > match the OEM Table ID in the FADT. > > > > > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > > > possibly also the other OEM related fields. > > > > > > Alternatively we might want to copy and use the RSDT on systems that > > > lack an XSDT, or even just copy the header from the RSDT into Xen's > > > crafted XSDT, since the format of the RSDP and the XSDT headers are > > > exactly the same (the difference is in the size of the description > > > headers that come after). > > > > I guess I'd prefer that last variant. > > I tried this approach (together with the second patch for necessity) and > it worked. > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index fd2cbf68bc..11d6d1bc23 100644 > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > goto out; > } > xsdt_paddr = rsdp->xsdt_physical_address; > + if ( !xsdt_paddr ) > + { > + xsdt_paddr = rsdp->rsdt_physical_address; > + } > acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > if ( !table ) To be slightly more consistent, could you use: /* * Note the header is the same for both RSDT and XSDT, so it's fine to * copy the native RSDT header to the Xen crafted XSDT if no native * XSDT is available. */ if (rsdp->revision > 1 && rsdp->xsdt_physical_address) sdt_paddr = rsdp->xsdt_physical_address; else sdt_paddr = rsdp->rsdt_physical_address; It was an oversight of mine to not check for the RSDP revision, as RSDP < 2 will never have an XSDT. Also add: Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') Thanks.
On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote: > On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: > > On Mon, 15 May 2023, Jan Beulich wrote: > > > On 15.05.2023 10:48, Roger Pau Monné wrote: > > > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > > > >> From: Stefano Stabellini <stefano.stabellini@amd.com> > > > >> > > > >> Xen always generates a XSDT table even if the firmware provided a RSDT > > > >> table. Instead of copying the XSDT header from the firmware table (that > > > >> might be missing), generate the XSDT header from a preset. > > > >> > > > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > >> --- > > > >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > > > >> 1 file changed, 9 insertions(+), 23 deletions(-) > > > >> > > > >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > > >> index 307edc6a8c..5fde769863 100644 > > > >> --- a/xen/arch/x86/hvm/dom0_build.c > > > >> +++ b/xen/arch/x86/hvm/dom0_build.c > > > >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > > >> paddr_t *addr) > > > >> { > > > >> struct acpi_table_xsdt *xsdt; > > > >> - struct acpi_table_header *table; > > > >> - struct acpi_table_rsdp *rsdp; > > > >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > > > >> unsigned long size = sizeof(*xsdt); > > > >> unsigned int i, j, num_tables = 0; > > > >> - paddr_t xsdt_paddr; > > > >> int rc; > > > >> + struct acpi_table_header header = { > > > >> + .signature = "XSDT", > > > >> + .length = sizeof(struct acpi_table_header), > > > >> + .revision = 0x1, > > > >> + .oem_id = "Xen", > > > >> + .oem_table_id = "HVM", > > > > > > > > I think this is wrong, as according to the spec the OEM Table ID must > > > > match the OEM Table ID in the FADT. > > > > > > > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > > > > possibly also the other OEM related fields. > > > > > > > > Alternatively we might want to copy and use the RSDT on systems that > > > > lack an XSDT, or even just copy the header from the RSDT into Xen's > > > > crafted XSDT, since the format of the RSDP and the XSDT headers are > > > > exactly the same (the difference is in the size of the description > > > > headers that come after). > > > > > > I guess I'd prefer that last variant. > > > > I tried this approach (together with the second patch for necessity) and > > it worked. > > > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > index fd2cbf68bc..11d6d1bc23 100644 > > --- a/xen/arch/x86/hvm/dom0_build.c > > +++ b/xen/arch/x86/hvm/dom0_build.c > > @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > goto out; > > } > > xsdt_paddr = rsdp->xsdt_physical_address; > > + if ( !xsdt_paddr ) > > + { > > + xsdt_paddr = rsdp->rsdt_physical_address; > > + } > > acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > > table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > > if ( !table ) > > To be slightly more consistent, could you use: > > /* > * Note the header is the same for both RSDT and XSDT, so it's fine to > * copy the native RSDT header to the Xen crafted XSDT if no native > * XSDT is available. > */ > if (rsdp->revision > 1 && rsdp->xsdt_physical_address) > sdt_paddr = rsdp->xsdt_physical_address; > else > sdt_paddr = rsdp->rsdt_physical_address; (please add the missing spaces in the chunk above)
On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote: > On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: > > On Mon, 15 May 2023, Jan Beulich wrote: > > > On 15.05.2023 10:48, Roger Pau Monné wrote: > > > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > > > >> From: Stefano Stabellini <stefano.stabellini@amd.com> > > > >> > > > >> Xen always generates a XSDT table even if the firmware provided a RSDT > > > >> table. Instead of copying the XSDT header from the firmware table (that > > > >> might be missing), generate the XSDT header from a preset. > > > >> > > > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > >> --- > > > >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > > > >> 1 file changed, 9 insertions(+), 23 deletions(-) > > > >> > > > >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > > >> index 307edc6a8c..5fde769863 100644 > > > >> --- a/xen/arch/x86/hvm/dom0_build.c > > > >> +++ b/xen/arch/x86/hvm/dom0_build.c > > > >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > > >> paddr_t *addr) > > > >> { > > > >> struct acpi_table_xsdt *xsdt; > > > >> - struct acpi_table_header *table; > > > >> - struct acpi_table_rsdp *rsdp; > > > >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > > > >> unsigned long size = sizeof(*xsdt); > > > >> unsigned int i, j, num_tables = 0; > > > >> - paddr_t xsdt_paddr; > > > >> int rc; > > > >> + struct acpi_table_header header = { > > > >> + .signature = "XSDT", > > > >> + .length = sizeof(struct acpi_table_header), > > > >> + .revision = 0x1, > > > >> + .oem_id = "Xen", > > > >> + .oem_table_id = "HVM", > > > > > > > > I think this is wrong, as according to the spec the OEM Table ID must > > > > match the OEM Table ID in the FADT. > > > > > > > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > > > > possibly also the other OEM related fields. > > > > > > > > Alternatively we might want to copy and use the RSDT on systems that > > > > lack an XSDT, or even just copy the header from the RSDT into Xen's > > > > crafted XSDT, since the format of the RSDP and the XSDT headers are > > > > exactly the same (the difference is in the size of the description > > > > headers that come after). > > > > > > I guess I'd prefer that last variant. > > > > I tried this approach (together with the second patch for necessity) and > > it worked. > > > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > index fd2cbf68bc..11d6d1bc23 100644 > > --- a/xen/arch/x86/hvm/dom0_build.c > > +++ b/xen/arch/x86/hvm/dom0_build.c > > @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > goto out; > > } > > xsdt_paddr = rsdp->xsdt_physical_address; > > + if ( !xsdt_paddr ) > > + { > > + xsdt_paddr = rsdp->rsdt_physical_address; > > + } > > acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > > table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > > if ( !table ) > > To be slightly more consistent, could you use: > > /* > * Note the header is the same for both RSDT and XSDT, so it's fine to > * copy the native RSDT header to the Xen crafted XSDT if no native > * XSDT is available. > */ > if (rsdp->revision > 1 && rsdp->xsdt_physical_address) > sdt_paddr = rsdp->xsdt_physical_address; > else > sdt_paddr = rsdp->rsdt_physical_address; > > It was an oversight of mine to not check for the RSDP revision, as > RSDP < 2 will never have an XSDT. Also add: > > Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') Just realized this will require some more work so that the guest (dom0) provided RSDP is at least revision 2. You will need to adjust the field and recalculate the checksum if needed. Thanks, Roger.
On 16.05.2023 10:24, Roger Pau Monné wrote: > On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote: >> On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: >>> On Mon, 15 May 2023, Jan Beulich wrote: >>>> On 15.05.2023 10:48, Roger Pau Monné wrote: >>>>> On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: >>>>>> From: Stefano Stabellini <stefano.stabellini@amd.com> >>>>>> >>>>>> Xen always generates a XSDT table even if the firmware provided a RSDT >>>>>> table. Instead of copying the XSDT header from the firmware table (that >>>>>> might be missing), generate the XSDT header from a preset. >>>>>> >>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> >>>>>> --- >>>>>> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- >>>>>> 1 file changed, 9 insertions(+), 23 deletions(-) >>>>>> >>>>>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c >>>>>> index 307edc6a8c..5fde769863 100644 >>>>>> --- a/xen/arch/x86/hvm/dom0_build.c >>>>>> +++ b/xen/arch/x86/hvm/dom0_build.c >>>>>> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, >>>>>> paddr_t *addr) >>>>>> { >>>>>> struct acpi_table_xsdt *xsdt; >>>>>> - struct acpi_table_header *table; >>>>>> - struct acpi_table_rsdp *rsdp; >>>>>> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; >>>>>> unsigned long size = sizeof(*xsdt); >>>>>> unsigned int i, j, num_tables = 0; >>>>>> - paddr_t xsdt_paddr; >>>>>> int rc; >>>>>> + struct acpi_table_header header = { >>>>>> + .signature = "XSDT", >>>>>> + .length = sizeof(struct acpi_table_header), >>>>>> + .revision = 0x1, >>>>>> + .oem_id = "Xen", >>>>>> + .oem_table_id = "HVM", >>>>> >>>>> I think this is wrong, as according to the spec the OEM Table ID must >>>>> match the OEM Table ID in the FADT. >>>>> >>>>> We likely want to copy the OEM ID and OEM Table ID from the RSDP, and >>>>> possibly also the other OEM related fields. >>>>> >>>>> Alternatively we might want to copy and use the RSDT on systems that >>>>> lack an XSDT, or even just copy the header from the RSDT into Xen's >>>>> crafted XSDT, since the format of the RSDP and the XSDT headers are >>>>> exactly the same (the difference is in the size of the description >>>>> headers that come after). >>>> >>>> I guess I'd prefer that last variant. >>> >>> I tried this approach (together with the second patch for necessity) and >>> it worked. >>> >>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c >>> index fd2cbf68bc..11d6d1bc23 100644 >>> --- a/xen/arch/x86/hvm/dom0_build.c >>> +++ b/xen/arch/x86/hvm/dom0_build.c >>> @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, >>> goto out; >>> } >>> xsdt_paddr = rsdp->xsdt_physical_address; >>> + if ( !xsdt_paddr ) >>> + { >>> + xsdt_paddr = rsdp->rsdt_physical_address; >>> + } >>> acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); >>> table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); >>> if ( !table ) >> >> To be slightly more consistent, could you use: >> >> /* >> * Note the header is the same for both RSDT and XSDT, so it's fine to >> * copy the native RSDT header to the Xen crafted XSDT if no native >> * XSDT is available. >> */ >> if (rsdp->revision > 1 && rsdp->xsdt_physical_address) >> sdt_paddr = rsdp->xsdt_physical_address; >> else >> sdt_paddr = rsdp->rsdt_physical_address; >> >> It was an oversight of mine to not check for the RSDP revision, as >> RSDP < 2 will never have an XSDT. Also add: >> >> Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') > > Just realized this will require some more work so that the guest > (dom0) provided RSDP is at least revision 2. You will need to adjust > the field and recalculate the checksum if needed. We could also mandate ACPI version >= 2 for PVH Dom0. Jan
On Tue, May 16, 2023 at 11:13:17AM +0200, Jan Beulich wrote: > On 16.05.2023 10:24, Roger Pau Monné wrote: > > On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote: > >> On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: > >>> On Mon, 15 May 2023, Jan Beulich wrote: > >>>> On 15.05.2023 10:48, Roger Pau Monné wrote: > >>>>> On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > >>>>>> From: Stefano Stabellini <stefano.stabellini@amd.com> > >>>>>> > >>>>>> Xen always generates a XSDT table even if the firmware provided a RSDT > >>>>>> table. Instead of copying the XSDT header from the firmware table (that > >>>>>> might be missing), generate the XSDT header from a preset. > >>>>>> > >>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > >>>>>> --- > >>>>>> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > >>>>>> 1 file changed, 9 insertions(+), 23 deletions(-) > >>>>>> > >>>>>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > >>>>>> index 307edc6a8c..5fde769863 100644 > >>>>>> --- a/xen/arch/x86/hvm/dom0_build.c > >>>>>> +++ b/xen/arch/x86/hvm/dom0_build.c > >>>>>> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > >>>>>> paddr_t *addr) > >>>>>> { > >>>>>> struct acpi_table_xsdt *xsdt; > >>>>>> - struct acpi_table_header *table; > >>>>>> - struct acpi_table_rsdp *rsdp; > >>>>>> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > >>>>>> unsigned long size = sizeof(*xsdt); > >>>>>> unsigned int i, j, num_tables = 0; > >>>>>> - paddr_t xsdt_paddr; > >>>>>> int rc; > >>>>>> + struct acpi_table_header header = { > >>>>>> + .signature = "XSDT", > >>>>>> + .length = sizeof(struct acpi_table_header), > >>>>>> + .revision = 0x1, > >>>>>> + .oem_id = "Xen", > >>>>>> + .oem_table_id = "HVM", > >>>>> > >>>>> I think this is wrong, as according to the spec the OEM Table ID must > >>>>> match the OEM Table ID in the FADT. > >>>>> > >>>>> We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > >>>>> possibly also the other OEM related fields. > >>>>> > >>>>> Alternatively we might want to copy and use the RSDT on systems that > >>>>> lack an XSDT, or even just copy the header from the RSDT into Xen's > >>>>> crafted XSDT, since the format of the RSDP and the XSDT headers are > >>>>> exactly the same (the difference is in the size of the description > >>>>> headers that come after). > >>>> > >>>> I guess I'd prefer that last variant. > >>> > >>> I tried this approach (together with the second patch for necessity) and > >>> it worked. > >>> > >>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > >>> index fd2cbf68bc..11d6d1bc23 100644 > >>> --- a/xen/arch/x86/hvm/dom0_build.c > >>> +++ b/xen/arch/x86/hvm/dom0_build.c > >>> @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > >>> goto out; > >>> } > >>> xsdt_paddr = rsdp->xsdt_physical_address; > >>> + if ( !xsdt_paddr ) > >>> + { > >>> + xsdt_paddr = rsdp->rsdt_physical_address; > >>> + } > >>> acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > >>> table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > >>> if ( !table ) > >> > >> To be slightly more consistent, could you use: > >> > >> /* > >> * Note the header is the same for both RSDT and XSDT, so it's fine to > >> * copy the native RSDT header to the Xen crafted XSDT if no native > >> * XSDT is available. > >> */ > >> if (rsdp->revision > 1 && rsdp->xsdt_physical_address) > >> sdt_paddr = rsdp->xsdt_physical_address; > >> else > >> sdt_paddr = rsdp->rsdt_physical_address; > >> > >> It was an oversight of mine to not check for the RSDP revision, as > >> RSDP < 2 will never have an XSDT. Also add: > >> > >> Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') > > > > Just realized this will require some more work so that the guest > > (dom0) provided RSDP is at least revision 2. You will need to adjust > > the field and recalculate the checksum if needed. > > We could also mandate ACPI version >= 2 for PVH Dom0. Sorry, mentioned on IRC, the above is not required because the RSDP provided to dom0 is already crafted by Xen and unconditionally set to version == 2. There's no need to adjust the RSDP at all. Thanks, Roger.
On Tue, 16 May 2023, Roger Pau Monné wrote: > On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote: > > On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: > > > On Mon, 15 May 2023, Jan Beulich wrote: > > > > On 15.05.2023 10:48, Roger Pau Monné wrote: > > > > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > > > > >> From: Stefano Stabellini <stefano.stabellini@amd.com> > > > > >> > > > > >> Xen always generates a XSDT table even if the firmware provided a RSDT > > > > >> table. Instead of copying the XSDT header from the firmware table (that > > > > >> might be missing), generate the XSDT header from a preset. > > > > >> > > > > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > > >> --- > > > > >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > > > > >> 1 file changed, 9 insertions(+), 23 deletions(-) > > > > >> > > > > >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > > > >> index 307edc6a8c..5fde769863 100644 > > > > >> --- a/xen/arch/x86/hvm/dom0_build.c > > > > >> +++ b/xen/arch/x86/hvm/dom0_build.c > > > > >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > > > >> paddr_t *addr) > > > > >> { > > > > >> struct acpi_table_xsdt *xsdt; > > > > >> - struct acpi_table_header *table; > > > > >> - struct acpi_table_rsdp *rsdp; > > > > >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > > > > >> unsigned long size = sizeof(*xsdt); > > > > >> unsigned int i, j, num_tables = 0; > > > > >> - paddr_t xsdt_paddr; > > > > >> int rc; > > > > >> + struct acpi_table_header header = { > > > > >> + .signature = "XSDT", > > > > >> + .length = sizeof(struct acpi_table_header), > > > > >> + .revision = 0x1, > > > > >> + .oem_id = "Xen", > > > > >> + .oem_table_id = "HVM", > > > > > > > > > > I think this is wrong, as according to the spec the OEM Table ID must > > > > > match the OEM Table ID in the FADT. > > > > > > > > > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > > > > > possibly also the other OEM related fields. > > > > > > > > > > Alternatively we might want to copy and use the RSDT on systems that > > > > > lack an XSDT, or even just copy the header from the RSDT into Xen's > > > > > crafted XSDT, since the format of the RSDP and the XSDT headers are > > > > > exactly the same (the difference is in the size of the description > > > > > headers that come after). > > > > > > > > I guess I'd prefer that last variant. > > > > > > I tried this approach (together with the second patch for necessity) and > > > it worked. > > > > > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > > index fd2cbf68bc..11d6d1bc23 100644 > > > --- a/xen/arch/x86/hvm/dom0_build.c > > > +++ b/xen/arch/x86/hvm/dom0_build.c > > > @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > > goto out; > > > } > > > xsdt_paddr = rsdp->xsdt_physical_address; > > > + if ( !xsdt_paddr ) > > > + { > > > + xsdt_paddr = rsdp->rsdt_physical_address; > > > + } > > > acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > > > table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > > > if ( !table ) > > > > To be slightly more consistent, could you use: > > > > /* > > * Note the header is the same for both RSDT and XSDT, so it's fine to > > * copy the native RSDT header to the Xen crafted XSDT if no native > > * XSDT is available. > > */ > > if (rsdp->revision > 1 && rsdp->xsdt_physical_address) > > sdt_paddr = rsdp->xsdt_physical_address; > > else > > sdt_paddr = rsdp->rsdt_physical_address; > > > > It was an oversight of mine to not check for the RSDP revision, as > > RSDP < 2 will never have an XSDT. Also add: > > > > Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') > > Just realized this will require some more work so that the guest > (dom0) provided RSDP is at least revision 2. You will need to adjust > the field and recalculate the checksum if needed. But we are always providing RSDP version 2 in pvh_setup_acpi, right?
On Tue, May 16, 2023 at 03:11:49PM -0700, Stefano Stabellini wrote: > On Tue, 16 May 2023, Roger Pau Monné wrote: > > On Tue, May 16, 2023 at 10:10:07AM +0200, Roger Pau Monné wrote: > > > On Mon, May 15, 2023 at 05:16:27PM -0700, Stefano Stabellini wrote: > > > > On Mon, 15 May 2023, Jan Beulich wrote: > > > > > On 15.05.2023 10:48, Roger Pau Monné wrote: > > > > > > On Fri, May 12, 2023 at 06:17:19PM -0700, Stefano Stabellini wrote: > > > > > >> From: Stefano Stabellini <stefano.stabellini@amd.com> > > > > > >> > > > > > >> Xen always generates a XSDT table even if the firmware provided a RSDT > > > > > >> table. Instead of copying the XSDT header from the firmware table (that > > > > > >> might be missing), generate the XSDT header from a preset. > > > > > >> > > > > > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > > > > >> --- > > > > > >> xen/arch/x86/hvm/dom0_build.c | 32 +++++++++----------------------- > > > > > >> 1 file changed, 9 insertions(+), 23 deletions(-) > > > > > >> > > > > > >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > > > > >> index 307edc6a8c..5fde769863 100644 > > > > > >> --- a/xen/arch/x86/hvm/dom0_build.c > > > > > >> +++ b/xen/arch/x86/hvm/dom0_build.c > > > > > >> @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > > > > >> paddr_t *addr) > > > > > >> { > > > > > >> struct acpi_table_xsdt *xsdt; > > > > > >> - struct acpi_table_header *table; > > > > > >> - struct acpi_table_rsdp *rsdp; > > > > > >> const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; > > > > > >> unsigned long size = sizeof(*xsdt); > > > > > >> unsigned int i, j, num_tables = 0; > > > > > >> - paddr_t xsdt_paddr; > > > > > >> int rc; > > > > > >> + struct acpi_table_header header = { > > > > > >> + .signature = "XSDT", > > > > > >> + .length = sizeof(struct acpi_table_header), > > > > > >> + .revision = 0x1, > > > > > >> + .oem_id = "Xen", > > > > > >> + .oem_table_id = "HVM", > > > > > > > > > > > > I think this is wrong, as according to the spec the OEM Table ID must > > > > > > match the OEM Table ID in the FADT. > > > > > > > > > > > > We likely want to copy the OEM ID and OEM Table ID from the RSDP, and > > > > > > possibly also the other OEM related fields. > > > > > > > > > > > > Alternatively we might want to copy and use the RSDT on systems that > > > > > > lack an XSDT, or even just copy the header from the RSDT into Xen's > > > > > > crafted XSDT, since the format of the RSDP and the XSDT headers are > > > > > > exactly the same (the difference is in the size of the description > > > > > > headers that come after). > > > > > > > > > > I guess I'd prefer that last variant. > > > > > > > > I tried this approach (together with the second patch for necessity) and > > > > it worked. > > > > > > > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > > > > index fd2cbf68bc..11d6d1bc23 100644 > > > > --- a/xen/arch/x86/hvm/dom0_build.c > > > > +++ b/xen/arch/x86/hvm/dom0_build.c > > > > @@ -967,6 +967,10 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, > > > > goto out; > > > > } > > > > xsdt_paddr = rsdp->xsdt_physical_address; > > > > + if ( !xsdt_paddr ) > > > > + { > > > > + xsdt_paddr = rsdp->rsdt_physical_address; > > > > + } > > > > acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); > > > > table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); > > > > if ( !table ) > > > > > > To be slightly more consistent, could you use: > > > > > > /* > > > * Note the header is the same for both RSDT and XSDT, so it's fine to > > > * copy the native RSDT header to the Xen crafted XSDT if no native > > > * XSDT is available. > > > */ > > > if (rsdp->revision > 1 && rsdp->xsdt_physical_address) > > > sdt_paddr = rsdp->xsdt_physical_address; > > > else > > > sdt_paddr = rsdp->rsdt_physical_address; > > > > > > It was an oversight of mine to not check for the RSDP revision, as > > > RSDP < 2 will never have an XSDT. Also add: > > > > > > Fixes: 1d74282c455f ('x86: setup PVHv2 Dom0 ACPI tables') > > > > Just realized this will require some more work so that the guest > > (dom0) provided RSDP is at least revision 2. You will need to adjust > > the field and recalculate the checksum if needed. > > But we are always providing RSDP version 2 in pvh_setup_acpi, right? Yes, as said in the reply to Jan, just ignore this. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 307edc6a8c..5fde769863 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -963,13 +963,18 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, paddr_t *addr) { struct acpi_table_xsdt *xsdt; - struct acpi_table_header *table; - struct acpi_table_rsdp *rsdp; const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables; unsigned long size = sizeof(*xsdt); unsigned int i, j, num_tables = 0; - paddr_t xsdt_paddr; int rc; + struct acpi_table_header header = { + .signature = "XSDT", + .length = sizeof(struct acpi_table_header), + .revision = 0x1, + .oem_id = "Xen", + .oem_table_id = "HVM", + .oem_revision = 0, + }; /* * Restore original DMAR table signature, we are going to filter it from @@ -1001,26 +1006,7 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr, goto out; } - /* Copy the native XSDT table header. */ - rsdp = acpi_os_map_memory(acpi_os_get_root_pointer(), sizeof(*rsdp)); - if ( !rsdp ) - { - printk("Unable to map RSDP\n"); - rc = -EINVAL; - goto out; - } - xsdt_paddr = rsdp->xsdt_physical_address; - acpi_os_unmap_memory(rsdp, sizeof(*rsdp)); - table = acpi_os_map_memory(xsdt_paddr, sizeof(*table)); - if ( !table ) - { - printk("Unable to map XSDT\n"); - rc = -EINVAL; - goto out; - } - xsdt->header = *table; - acpi_os_unmap_memory(table, sizeof(*table)); - + xsdt->header = header; /* Add the custom MADT. */ xsdt->table_offset_entry[0] = madt_addr;