[PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Roger Pau Monne]

The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
from the firmware instead of doing it on the PCI ROM on the physical
device.

As such, this needs to be available for PVH dom0 to access, or else
the GPU won't work.

Reported-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
This is a bugfix, but given PVH dom0 is still experimental I'm not
sure it would qualify for the release.  OTOH being experimental means
there are no guarantees it will work, so the change is unlikely to
make this any worse.
---
 xen/arch/x86/hvm/dom0_build.c | 1 +
 xen/include/acpi/actbl3.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 1864d048a1..307edc6a8c 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -924,6 +924,7 @@ static bool __init pvh_acpi_table_allowed(const char *sig,
         ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_FACS, ACPI_SIG_PSDT,
         ACPI_SIG_SSDT, ACPI_SIG_SBST, ACPI_SIG_MCFG, ACPI_SIG_SLIC,
         ACPI_SIG_MSDM, ACPI_SIG_WDAT, ACPI_SIG_FPDT, ACPI_SIG_S3PT,
+        ACPI_SIG_VFCT,
     };
     unsigned int i;
 
diff --git a/xen/include/acpi/actbl3.h b/xen/include/acpi/actbl3.h
index 0a6778421f..6858d3e60f 100644
--- a/xen/include/acpi/actbl3.h
+++ b/xen/include/acpi/actbl3.h
@@ -79,6 +79,7 @@
 #define ACPI_SIG_MATR           "MATR"	/* Memory Address Translation Table */
 #define ACPI_SIG_MSDM           "MSDM"	/* Microsoft Data Management Table */
 #define ACPI_SIG_WPBT           "WPBT"	/* Windows Platform Binary Table */
+#define ACPI_SIG_VFCT           "VFCT"	/* AMD Video BIOS */
 
 /*
  * All tables must be byte-packed to match the ACPI specification, since
-- 
2.37.3

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Huang Rui]

On Tue, Nov 08, 2022 at 09:55:16PM +0800, Roger Pau Monne wrote:
> The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
> from the firmware instead of doing it on the PCI ROM on the physical
> device.
> 
> As such, this needs to be available for PVH dom0 to access, or else
> the GPU won't work.
> 
> Reported-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> This is a bugfix, but given PVH dom0 is still experimental I'm not
> sure it would qualify for the release.  OTOH being experimental means
> there are no guarantees it will work, so the change is unlikely to
> make this any worse.


Yes, thanks Roger! AMD GPU needs a Video BIOS which needs to do the
initialization for hardware. We use ACPI VFCT table to expose the VBIOS rom
image even on the Xen PVH dom0, so we need to add the related signature in
Xen as well.

Acked-and-Tested-by: Huang Rui <ray.huang@amd.com>

> ---
>  xen/arch/x86/hvm/dom0_build.c | 1 +
>  xen/include/acpi/actbl3.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 1864d048a1..307edc6a8c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -924,6 +924,7 @@ static bool __init pvh_acpi_table_allowed(const char *sig,
>          ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_FACS, ACPI_SIG_PSDT,
>          ACPI_SIG_SSDT, ACPI_SIG_SBST, ACPI_SIG_MCFG, ACPI_SIG_SLIC,
>          ACPI_SIG_MSDM, ACPI_SIG_WDAT, ACPI_SIG_FPDT, ACPI_SIG_S3PT,
> +        ACPI_SIG_VFCT,
>      };
>      unsigned int i;
>  
> diff --git a/xen/include/acpi/actbl3.h b/xen/include/acpi/actbl3.h
> index 0a6778421f..6858d3e60f 100644
> --- a/xen/include/acpi/actbl3.h
> +++ b/xen/include/acpi/actbl3.h
> @@ -79,6 +79,7 @@
>  #define ACPI_SIG_MATR           "MATR"	/* Memory Address Translation Table */
>  #define ACPI_SIG_MSDM           "MSDM"	/* Microsoft Data Management Table */
>  #define ACPI_SIG_WPBT           "WPBT"	/* Windows Platform Binary Table */
> +#define ACPI_SIG_VFCT           "VFCT"	/* AMD Video BIOS */
>  
>  /*
>   * All tables must be byte-packed to match the ACPI specification, since
> -- 
> 2.37.3
> 

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Jan Beulich]

On 08.11.2022 14:55, Roger Pau Monne wrote:
> The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
> from the firmware instead of doing it on the PCI ROM on the physical
> device.

I can't find any mention of VFCT in the ACPI 6.3 spec, nor anywhere
under Linux'es include/acpi/. I don't mind the addition that you're
doing, but there needs to be a pointer to a sufficiently "official"
spec.

Jan

> As such, this needs to be available for PVH dom0 to access, or else
> the GPU won't work.
> 
> Reported-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> This is a bugfix, but given PVH dom0 is still experimental I'm not
> sure it would qualify for the release.  OTOH being experimental means
> there are no guarantees it will work, so the change is unlikely to
> make this any worse.
> ---
>  xen/arch/x86/hvm/dom0_build.c | 1 +
>  xen/include/acpi/actbl3.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 1864d048a1..307edc6a8c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -924,6 +924,7 @@ static bool __init pvh_acpi_table_allowed(const char *sig,
>          ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_FACS, ACPI_SIG_PSDT,
>          ACPI_SIG_SSDT, ACPI_SIG_SBST, ACPI_SIG_MCFG, ACPI_SIG_SLIC,
>          ACPI_SIG_MSDM, ACPI_SIG_WDAT, ACPI_SIG_FPDT, ACPI_SIG_S3PT,
> +        ACPI_SIG_VFCT,
>      };
>      unsigned int i;
>  
> diff --git a/xen/include/acpi/actbl3.h b/xen/include/acpi/actbl3.h
> index 0a6778421f..6858d3e60f 100644
> --- a/xen/include/acpi/actbl3.h
> +++ b/xen/include/acpi/actbl3.h
> @@ -79,6 +79,7 @@
>  #define ACPI_SIG_MATR           "MATR"	/* Memory Address Translation Table */
>  #define ACPI_SIG_MSDM           "MSDM"	/* Microsoft Data Management Table */
>  #define ACPI_SIG_WPBT           "WPBT"	/* Windows Platform Binary Table */
> +#define ACPI_SIG_VFCT           "VFCT"	/* AMD Video BIOS */
>  
>  /*
>   * All tables must be byte-packed to match the ACPI specification, since

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Huang Rui]

On Tue, Nov 08, 2022 at 11:56:17PM +0800, Jan Beulich wrote:
> On 08.11.2022 14:55, Roger Pau Monne wrote:
> > The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
> > from the firmware instead of doing it on the PCI ROM on the physical
> > device.
> 
> I can't find any mention of VFCT in the ACPI 6.3 spec, nor anywhere
> under Linux'es include/acpi/. I don't mind the addition that you're
> doing, but there needs to be a pointer to a sufficiently "official"
> spec.

Thanks! In fact, this is defined by AMD atombios (VBIOS), we have up
streamed this in the Linux kenrel. You can see more details here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/amd/include/atombios.h

Thanks,
Ray

> 
> Jan
> 
> > As such, this needs to be available for PVH dom0 to access, or else
> > the GPU won't work.
> > 
> > Reported-by: Huang Rui <ray.huang@amd.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > This is a bugfix, but given PVH dom0 is still experimental I'm not
> > sure it would qualify for the release.  OTOH being experimental means
> > there are no guarantees it will work, so the change is unlikely to
> > make this any worse.
> > ---
> >  xen/arch/x86/hvm/dom0_build.c | 1 +
> >  xen/include/acpi/actbl3.h     | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> > index 1864d048a1..307edc6a8c 100644
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -924,6 +924,7 @@ static bool __init pvh_acpi_table_allowed(const char *sig,
> >          ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_FACS, ACPI_SIG_PSDT,
> >          ACPI_SIG_SSDT, ACPI_SIG_SBST, ACPI_SIG_MCFG, ACPI_SIG_SLIC,
> >          ACPI_SIG_MSDM, ACPI_SIG_WDAT, ACPI_SIG_FPDT, ACPI_SIG_S3PT,
> > +        ACPI_SIG_VFCT,
> >      };
> >      unsigned int i;
> >  
> > diff --git a/xen/include/acpi/actbl3.h b/xen/include/acpi/actbl3.h
> > index 0a6778421f..6858d3e60f 100644
> > --- a/xen/include/acpi/actbl3.h
> > +++ b/xen/include/acpi/actbl3.h
> > @@ -79,6 +79,7 @@
> >  #define ACPI_SIG_MATR           "MATR"	/* Memory Address Translation Table */
> >  #define ACPI_SIG_MSDM           "MSDM"	/* Microsoft Data Management Table */
> >  #define ACPI_SIG_WPBT           "WPBT"	/* Windows Platform Binary Table */
> > +#define ACPI_SIG_VFCT           "VFCT"	/* AMD Video BIOS */
> >  
> >  /*
> >   * All tables must be byte-packed to match the ACPI specification, since
> 

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Roger Pau Monné]

On Tue, Nov 08, 2022 at 04:56:17PM +0100, Jan Beulich wrote:
> On 08.11.2022 14:55, Roger Pau Monne wrote:
> > The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
> > from the firmware instead of doing it on the PCI ROM on the physical
> > device.
> 
> I can't find any mention of VFCT in the ACPI 6.3 spec, nor anywhere
> under Linux'es include/acpi/. I don't mind the addition that you're
> doing, but there needs to be a pointer to a sufficiently "official"
> spec.

I've also asked for some kind of official spec, as I would assume it
should be in:

https://uefi.org/acpi

With the rest of tables not in the spec itself, but there's none.

I could add a link to the Linux header, but that's suboptimal IMO.

Are you fine with this as-is, or would you like me to elaborate about
the lack of an official spec in the commit description?

Thanks, Roger.

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Jan Beulich]

On 08.11.2022 17:16, Huang Rui wrote:
> On Tue, Nov 08, 2022 at 11:56:17PM +0800, Jan Beulich wrote:
>> On 08.11.2022 14:55, Roger Pau Monne wrote:
>>> The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
>>> from the firmware instead of doing it on the PCI ROM on the physical
>>> device.
>>
>> I can't find any mention of VFCT in the ACPI 6.3 spec, nor anywhere
>> under Linux'es include/acpi/. I don't mind the addition that you're
>> doing, but there needs to be a pointer to a sufficiently "official"
>> spec.
> 
> Thanks! In fact, this is defined by AMD atombios (VBIOS), we have up
> streamed this in the Linux kenrel. You can see more details here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/amd/include/atombios.h

I can see about two dozen lines at the bottom of that file, with
effectively no helpful comments at all. I'm afraid I don't view
this as "more details", let alone something replacing a spec.
There at least needs to be enough to be sure that "VFCT" is really
firmly reserved for this and only this purpose within the overall
ACPI table name space.

Jan

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Jan Beulich]

On 08.11.2022 17:31, Roger Pau Monné wrote:
> On Tue, Nov 08, 2022 at 04:56:17PM +0100, Jan Beulich wrote:
>> On 08.11.2022 14:55, Roger Pau Monne wrote:
>>> The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
>>> from the firmware instead of doing it on the PCI ROM on the physical
>>> device.
>>
>> I can't find any mention of VFCT in the ACPI 6.3 spec, nor anywhere
>> under Linux'es include/acpi/. I don't mind the addition that you're
>> doing, but there needs to be a pointer to a sufficiently "official"
>> spec.
> 
> I've also asked for some kind of official spec, as I would assume it
> should be in:
> 
> https://uefi.org/acpi
> 
> With the rest of tables not in the spec itself, but there's none.
> 
> I could add a link to the Linux header, but that's suboptimal IMO.
> 
> Are you fine with this as-is, or would you like me to elaborate about
> the lack of an official spec in the commit description?

A pointer to the Linux bits is better than nothing, but as just said
in reply to Ray there needs to be sufficient guarantee that "VFCT"
really isn't going to be used for something else down the road.

Jan

Re: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Roger Pau Monné]

On Tue, Nov 08, 2022 at 05:35:04PM +0100, Jan Beulich wrote:
> On 08.11.2022 17:31, Roger Pau Monné wrote:
> > On Tue, Nov 08, 2022 at 04:56:17PM +0100, Jan Beulich wrote:
> >> On 08.11.2022 14:55, Roger Pau Monne wrote:
> >>> The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
> >>> from the firmware instead of doing it on the PCI ROM on the physical
> >>> device.
> >>
> >> I can't find any mention of VFCT in the ACPI 6.3 spec, nor anywhere
> >> under Linux'es include/acpi/. I don't mind the addition that you're
> >> doing, but there needs to be a pointer to a sufficiently "official"
> >> spec.
> > 
> > I've also asked for some kind of official spec, as I would assume it
> > should be in:
> > 
> > https://uefi.org/acpi
> > 
> > With the rest of tables not in the spec itself, but there's none.
> > 
> > I could add a link to the Linux header, but that's suboptimal IMO.
> > 
> > Are you fine with this as-is, or would you like me to elaborate about
> > the lack of an official spec in the commit description?
> 
> A pointer to the Linux bits is better than nothing, but as just said
> in reply to Ray there needs to be sufficient guarantee that "VFCT"
> really isn't going to be used for something else down the road.

I will leave this for the AMD folks to clarify.  I would assume AMD
has not picked a random table signature and started using it without
having it reserved with the ACPI folks.  Having that said I don't see
the signature in the table of reserved ones in the ACPI 6.5 spec
from August.

Regards, Roger.

RE: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if present

[Henry Wang]

Hi Roger,

> -----Original Message-----
> Subject: [PATCH for-4.17?] x86/pvh: report ACPI VFCT table to dom0 if
> present
> 
> The VFCT ACPI table is used by AMD GPUs to expose the vbios ROM image
> from the firmware instead of doing it on the PCI ROM on the physical
> device.
> 
> As such, this needs to be available for PVH dom0 to access, or else
> the GPU won't work.
> 
> Reported-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> This is a bugfix, but given PVH dom0 is still experimental I'm not
> sure it would qualify for the release.  OTOH being experimental means
> there are no guarantees it will work, so the change is unlikely to
> make this any worse.

Following the discussion in this thread, I think this is a simple enough
patch with big enough benefit given Ray has already tested the patch.
I wouldn't block this patch for 4.17 release, but we need to make sure
Jan (the ACPI maintainer) is happy with the VFCT table information in
commit message. Then feel free to add my:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry