Re: [PATCH v3] hw/cxl: Add QTG _DSM support for ACPI0017 device

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 05, 2023 at 09:11:15AM -0700, Dave Jiang wrote:
> 
> 
> On 10/4/23 20:36, Michael S. Tsirkin wrote:
> > 
> > On Wed, Oct 04, 2023 at 04:09:07PM -0700, Dave Jiang wrote:
> >> Add a simple _DSM call support for the ACPI0017 device to return a fake QTG
> >> ID value of 0 in all cases. The enabling is for _DSM plumbing testing
> >> from the OS.
> > 
> > 
> > the enabling -> this
> 
> will update
> > 
> >>
> >> Following edited for readbility only
> > 
> > readbility only -> readability
> 
> will update
> > 
> > 
> >>
> >> Device (CXLM)
> >> {
> >>     Name (_HID, "ACPI0017")  // _HID: Hardware ID
> >> ...
> >>     Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
> >>     {
> >>         If ((Arg0 == ToUUID ("f365f9a6-a7de-4071-a66a-b40c0b4f8e52")))
> >>         {
> >>             If ((Arg2 == Zero))
> >>             {
> >>                 Return (Buffer (One) { 0x01 })
> >>             }
> >>
> >>             If ((Arg2 == One))
> > 
> >>             {
> >>                 Return (Package (0x02)
> >>                 {
> >>                     Buffer (0x02)
> >>                     { 0x01, 0x00 },
> >>                     Package (0x01)
> >>                     {
> >>                         Buffer (0x02)
> >>                         { 0x00, 0x00 }
> >>                     }
> >>                 })
> >>             }
> >>         }
> >>     }
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>
> >> --
> >> v3: Fix output assignment to be BE host friendly. Fix typo in comment.
> >> According to the CXL spec, the DSM output should be 1 WORD to indicate
> >> the max suppoted QTG ID and a package of 0 or more WORDs for the QTG IDs.
> >> In this dummy impementation, we have first WORD with a 1 to indcate max
> >> supprted QTG ID of 1. And second WORD in a package to indicate the QTG
> >> ID of 0.
> >>
> >> v2: Minor edit to drop reference to switches in patch description.
> >> Message-Id: <20230904161847.18468-3-Jonathan.Cameron@huawei.com>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>  hw/acpi/cxl.c         |   55 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/i386/acpi-build.c  |    1 +
> >>  include/hw/acpi/cxl.h |    1 +
> >>  3 files changed, 57 insertions(+)
> >>
> >> diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> >> index 92b46bc9323b..cce12d5bc81c 100644
> >> --- a/hw/acpi/cxl.c
> >> +++ b/hw/acpi/cxl.c
> >> @@ -30,6 +30,61 @@
> >>  #include "qapi/error.h"
> >>  #include "qemu/uuid.h"
> >>  
> >> +void build_cxl_dsm_method(Aml *dev)
> >> +{
> >> +    Aml *method, *ifctx, *ifctx2;
> >> +
> >> +    method = aml_method("_DSM", 4, AML_SERIALIZED);
> >> +    {
> >> +        Aml *function, *uuid;
> >> +
> >> +        uuid = aml_arg(0);
> >> +        function = aml_arg(2);
> >> +        /* CXL spec v3.0 9.17.3.1 *
> > 
> > 
> > drop this * please
> > 
> >> , QTG ID _DSM
> 
> Ooops. git format-patch mangled this and I didn't catch. Will fix
> 
> > 
> > 
> > this is not the name of this paragraph. pls make it match
> > exactly so people can search
> > 
> >> */
> >> +        ifctx = aml_if(aml_equal(
> >> +            uuid, aml_touuid("F365F9A6-A7DE-4071-A66A-B40C0B4F8E52")));
> >> +
> >> +        /* Function 0, standard DSM query function */
> >> +        ifctx2 = aml_if(aml_equal(function, aml_int(0)));
> >> +        {
> >> +            uint8_t byte_list[1] = { 0x01 }; /* functions 1 only */
> > 
> > function 1?
> 
> Yes, will fix
> 
> > 
> >> +
> >> +            aml_append(ifctx2,
> >> +                       aml_return(aml_buffer(sizeof(byte_list), byte_list)));
> >> +        }
> >> +        aml_append(ifctx, ifctx2);
> >> +
> >> +        /*
> >> +         * Function 1
> >> +         * A return value of {1, {0}} indicates that
> >> +         * max supported QTG ID of 1 and recommended QTG is 0.
> >> +         * The values here are faked to simplify emulation.
> > 
> > again pls quote spec directly do not paraphrase
> 
> Here it's not paraphrasing from the spec. I'm just describing the dummy value that will be provided.
> 
> > 
> >> +         */
> >> +        ifctx2 = aml_if(aml_equal(function, aml_int(1)));
> >> +        {
> >> +            uint16_t word_list = cpu_to_le16(1);
> >> +            uint16_t word_list2 = 0;
> >> +            Aml *pak, *pak1;
> >> +
> >> +            /*
> >> +             * The return package is a package of a WORD
> >> and another package.
> >> +             * The embedded package contains 0 or more WORDs for the
> >> +             * recommended QTG IDs.
> > 
> > 
> > 
> > pls quote the spec directly
> 
> Will do.
> 
> > 
> > what does "a WORD" mean is unclear - do you match what hardware does
> > when you use aml_buffer? pls mention this in commit log, and
> > show actual hardware dump for comparison.
> The CXL spec says WORD without much qualification. It's a 16bit value AFAICT. I'll add additional comment. Currently I do not have access to actual hardware unfortunately. I'm constructing this purely based on spec description.

It's not clear buffer is actually word though.

Jonathan do you have hardware access?

Also, possible to get clarification from the spec committee?

> 
> > 
> > 
> >> +             */
> >> +            pak1 = aml_package(1);
> >> +            aml_append(pak1, aml_buffer(sizeof(uint16_t), word_list2));
> >> +            pak = aml_package(2);
> >> +            aml_append(pak, aml_buffer(sizeof(uint16_t), word_list));
> > 
> > 
> > It does not look like this patch compiles.
> > 
> > So how did you test it?
> > 
> > Please do not post untested patches.
> > 
> > If you do at least minimal testing
> > you would also see failures in bios table test
> > and would follow the procedure described there to
> > post it.
> 
> Sorry about that. I compiled successfully but did not test.

I don't see how it can compile either. In fact I just applied and build
fails.

> The following chunk is tested. However, is it the correct way to do this? The comment is direct spec verbiage. I'm not familiar with constructing ACPI tables in QEMU and tried my best by looking at other ACPI code in QEMU. 

To do what? create a buffer with a two byte word?
For example:
	word = aml_buffer(0, NULL);
	build_append_int_noprefix(word->buf, 2, 0x1);



but again it is not clear at all what does spec mean.
an integer up to 0xfffff? a buffer as you did? just two bytes?

could be any of these.

> 
> +        ifctx2 = aml_if(aml_equal(function, aml_int(1)));
> +        {
> +            uint16_t max_id = cpu_to_le16(1);
> +            uint16_t qtg_id = 0;
> +            Aml *pak, *pak1;
> +
> +            /*
> +            * Return: A package containing two elements - a WORD that returns
> +            * the maximum throttling group that the platform supports, and a
> +            * package containing the QTG ID(s) that the platform recommends.
> +            * Package {
> +            *     Max Supported QTG ID
> +            *     Package {QTG Recommendations}
> +            * }
> +            */
> +            pak1 = aml_package(1);
> +            aml_append(pak1, aml_buffer(sizeof(uint16_t), (uint8_t *)&qtg_id));
> +            pak = aml_package(2);
> +            aml_append(pak, aml_buffer(sizeof(uint16_t), (uint8_t *)&max_id));
> +            aml_append(pak, pak1);
> +
> +            aml_append(ifctx2, aml_return(pak));
> +        }
> 
> 
> > 
> > 
> > When you post next version please also document how the patch
> > was tested: which guests, what tests, what were the results.
> > 
> > thanks!
> > 
> >> +            aml_append(pak, pak1);
> >> +
> >> +            aml_append(ifctx2, aml_return(pak));
> >> +        }
> >> +        aml_append(ifctx, ifctx2);
> >> +    }
> >> +    aml_append(method, ifctx);
> >> +    aml_append(dev, method);
> >> +}
> >> +
> >>  static void cedt_build_chbs(GArray *table_data, PXBCXLDev *cxl)
> >>  {
> >>      PXBDev *pxb = PXB_DEV(cxl);
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 95199c89008a..692af40b1a75 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1422,6 +1422,7 @@ static void build_acpi0017(Aml *table)
> >>      method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> >>      aml_append(method, aml_return(aml_int(0x01)));
> >>      aml_append(dev, method);
> >> +    build_cxl_dsm_method(dev);
> >>  
> >>      aml_append(scope, dev);
> >>      aml_append(table, scope);
> >> diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> >> index acf441888683..8f22c71530d8 100644
> >> --- a/include/hw/acpi/cxl.h
> >> +++ b/include/hw/acpi/cxl.h
> >> @@ -25,5 +25,6 @@ void cxl_build_cedt(GArray *table_offsets, GArray *table_data,
> >>                      BIOSLinker *linker, const char *oem_id,
> >>                      const char *oem_table_id, CXLState *cxl_state);
> >>  void build_cxl_osc_method(Aml *dev);
> >> +void build_cxl_dsm_method(Aml *dev);
> >>  
> >>  #endif
> >>
> >