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

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

[Jonathan Cameron]

On Thu, 5 Oct 2023 12:32:11 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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.  
> 

WORD does seem to be clearly defined in the ACPI spec as uint16
and as this is describing a DSDT blob I think we can safe that
it means that.  Also lines up with the fixed sizes in CEDT.

> It's not clear buffer is actually word though.
> 
> Jonathan do you have hardware access?

No.  +CC linux-cxl to see if anyone else has hardware + BIOS with
QTG implemented...  There will be lots of implementations soon so I'd make
not guarantee they will all interpret this the same.

Aim here is Linux kernel enablement support, and unfortunately that almost
always means we are ahead of easy availability of hardware. If it exists
its probably prototypes in a lab, in which case no guarantees on the
BIOS tables presented...

> 
> Also, possible to get clarification from the spec committee?

I'm unclear what we are clarifying.  As I read it current implementation
is indeed wrong and I failed to notice this earlier :(

Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding)
should I think be

0x0B 0x00 0x00
WordPrefix then data : note if you try a 0x0001 and feed
it to iasl it will squash it into a byte instead and indeed if you
force the binary to the above it will decode it as 0x0000 but recompile
that and you will be back to just
0x00 (as bytes don't need a prefix..)

Currently it would be.
0x11     0x05 0x0a 0x02 0x00 0x01
BufferOp 

Btw I built a minimal DSDT file to test this and iasl isn't happy with
the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2.
Whilst that's imdef territory as not covered by the CXL spec, we should
return 'something' ;)

Anyhow, to do this as per the CXL spec we need an aml_word()
that just implements the word case from aml_int()

Chance are that it never matters if we get an ecoding that is
only single byte (because the value is small) but who knows what
other AML parsers might do.

Something simple like (copy typed from test machine..)

Aml *aml_word(const uint16_t val)
{
    Aml *var = aml_alloc();
    build_append_byte(var->buf, 0x0B);
    build_append_int_noprefix(var->buf, val, 2);
    return var;
}

and one blob in acpi/cxl.c becomes

        ifctx2 = aml_if(aml_equal(function, aml_int(1)));
        {
             Aml *pak, *pac1;

	     pak1 = aml_package(1)
	     aml_append(pak1, aml_word(0));
             pak = aml_package(2);
             aml_append(pack, aml_word(0x1));
             aml_append(pak, pak1);

             aml_append(ifctx2, aml_return(pak));
        }
        aml_append(ifctx, ifctx2);
...

 
        }

Give something like
If ((Arg2 == One))
{
    Return (Package (0x02)
    {
        0x0001,
        Package (0x01)
        {
            0x0000
        }
    })
}


Binary encoding then clearly uses packages of words.

12      0b        02         0b    01 00     12    05    01        0b    00 00
PkgOp   len       elements   word  0x0001    pkgOp len   elements  word  0x0000

(note I cheated an added a marker in one of the values and didn't decode
the whole thing by hand ;)

> 
> >   
> > > 
> > >   
> > >> +             */
> > >> +            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.

The best we have in the way of description is the multiple QTG example
where it's
Package() {2, 1} combined with it being made up of WORDs

whereas in general that will get squashed to a pair of bytes...
So I'm thinking WORDs is max size rather than only size but
given ambiguity we should encode them as words anyway.

Note this breaks Dave's current kernel proposal which is assuming
a buffer...
https://lore.kernel.org/all/168695172301.3031571.9812118774299137032.stgit@djiang5-mobl3/

Hohum. Dave, can you sanity check with the appropriate SSWG person (MN IIRC)
I can do it you'd prefer - just let me know.

Jonathan

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

[Dan Williams]

Jonathan Cameron wrote:
[..]
> > > > 
> > > > 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.  
> > 
> 
> WORD does seem to be clearly defined in the ACPI spec as uint16
> and as this is describing a DSDT blob I think we can safe that
> it means that.  Also lines up with the fixed sizes in CEDT.

I am not sure it means that, the ACPI specification indicates that packages
can hold "integers" and integers can be any size up to 64-bits.

> > It's not clear buffer is actually word though.
> > 
> > Jonathan do you have hardware access?
> 
> No.  +CC linux-cxl to see if anyone else has hardware + BIOS with
> QTG implemented...  There will be lots of implementations soon so I'd make
> not guarantee they will all interpret this the same.
> 
> Aim here is Linux kernel enablement support, and unfortunately that almost
> always means we are ahead of easy availability of hardware. If it exists
> its probably prototypes in a lab, in which case no guarantees on the
> BIOS tables presented...

From a pre-production system the ASL is putting the result of SizeOf
directly into the first element in the return package:

	Local1 = SizeOf (CXQI)
	Local0 [0x00] = Local1

...where CXQI appears to be a fixed table of QTG ids for the platform, and
SizeOf() returns an integer type with no restriction that it be a 16-bit
value.

So I think the specification is misleading by specifying WORD when ACPI
"Package" objects convey "Integers" where the size of the integer can be a
u8 to a u64.

> > Also, possible to get clarification from the spec committee?
> 
> I'm unclear what we are clarifying.  As I read it current implementation
> is indeed wrong and I failed to notice this earlier :(
> 
> Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding)
> should I think be
> 
> 0x0B 0x00 0x00
> WordPrefix then data : note if you try a 0x0001 and feed
> it to iasl it will squash it into a byte instead and indeed if you
> force the binary to the above it will decode it as 0x0000 but recompile
> that and you will be back to just
> 0x00 (as bytes don't need a prefix..)
> 
> Currently it would be.
> 0x11     0x05 0x0a 0x02 0x00 0x01
> BufferOp 
> 
> Btw I built a minimal DSDT file to test this and iasl isn't happy with
> the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2.
> Whilst that's imdef territory as not covered by the CXL spec, we should
> return 'something' ;)
> 
> Anyhow, to do this as per the CXL spec we need an aml_word()
> that just implements the word case from aml_int()

If I understand correctly, aml_int() is sufficient since this is not a
Field() where access size matters.

> Chance are that it never matters if we get an ecoding that is
> only single byte (because the value is small) but who knows what
> other AML parsers might do.

I expect the reason WORD is used in the specification is because of the
size of the QTG ID field in the CFMWS. ACPI could support returning more
than USHRT_MAX in an Integer field in a Package, but those IDs above
USHRT_MAX could not be represented in CFMWS.

[..]
> > 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.
> 
> The best we have in the way of description is the multiple QTG example
> where it's
> Package() {2, 1} combined with it being made up of WORDs
> 
> whereas in general that will get squashed to a pair of bytes...
> So I'm thinking WORDs is max size rather than only size but
> given ambiguity we should encode them as words anyway.

My assertion is that for the Package format the size of the integer does
not matter because the Package.Integer type can convey up to 64-bit values.
For being compatible with the *usage* of that max id, values that do not
fit into 16-bits are out of spec, but nothing prevents the Package from
using any size integer, afaics.

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

[Dave Jiang]

Add a simple _DSM call support for the ACPI0017 device to return fake QTG
ID values of 0 and 1 in all cases. This for _DSM plumbing testing from the OS.

Following edited for readability

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)
                {
                    One,
                    Package (0x02)
                    {
                        Zero,
                        One
                    }
                })
            }
        }
    }

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

--
v4: Change to package of ints rather than buffers. Also moved to 2 QTG IDs
to improve kernel side testing. Tested on x86 qemu guest against kernel
QTG ID _DSM parsing code to be upstreamed.

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         |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c  |    1 +
 include/hw/acpi/cxl.h |    1 +
 3 files changed, 71 insertions(+)

diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 92b46bc9323b..9cd7905ea25a 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -30,6 +30,75 @@
 #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 _DSM Function for Retrieving QTG ID */
+        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 }; /* function 1 only */
+
+            aml_append(ifctx2,
+                       aml_return(aml_buffer(sizeof(byte_list), byte_list)));
+        }
+        aml_append(ifctx, ifctx2);
+
+        /*
+         * Function 1
+         * Creating a package with static values. The max supported QTG ID will
+         * be 1 and recommended QTG IDs are 0 and then 1.
+         * The values here are statically created to simplify emulation. Values
+         * from a real BIOS would be determined by the performance of all the
+         * present CXL memory and then assigned.
+         */
+        ifctx2 = aml_if(aml_equal(function, aml_int(1)));
+        {
+            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}
+             * }
+             *
+             * While the SPEC specified WORD that hints at the value being
+             * 16bit, the ACPI dump of BIOS DSDT table showed that the values
+             * are integers with no specific size specification. aml_int() will
+             * be used for the values.
+             */
+            pak1 = aml_package(2);
+            /* Set QTG ID of 0 */
+            aml_append(pak1, aml_int(0));
+            /* Set QTG ID of 1 */
+            aml_append(pak1, aml_int(1));
+
+            pak = aml_package(2);
+            /* Set Max QTG 1 */
+            aml_append(pak, aml_int(1));
+            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

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

[Jonathan Cameron]

On Fri, 06 Oct 2023 15:15:56 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add a simple _DSM call support for the ACPI0017 device to return fake QTG
> ID values of 0 and 1 in all cases. This for _DSM plumbing testing from the OS.
> 
> Following edited for readability
> 
> 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)
>                 {
>                     One,
>                     Package (0x02)
>                     {
>                         Zero,
>                         One
>                     }
>                 })
>             }
>         }
>     }
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> --

You've picked up my bad habit / typo.  Needs to be ---
to stop git picking up this text...

> v4: Change to package of ints rather than buffers. Also moved to 2 QTG IDs
> to improve kernel side testing. Tested on x86 qemu guest against kernel
> QTG ID _DSM parsing code to be upstreamed.
> 
> 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>
Given it's changed in significant way should drop these.

Anyhow, the tests also need updating.  I can do that and push out
a v5 with tests as well once I've tested.

I'm assuming need tweaked kernel code to test?

> ---
>  hw/acpi/cxl.c         |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  |    1 +
>  include/hw/acpi/cxl.h |    1 +
>  3 files changed, 71 insertions(+)
> 
> diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> index 92b46bc9323b..9cd7905ea25a 100644
> --- a/hw/acpi/cxl.c
> +++ b/hw/acpi/cxl.c
> @@ -30,6 +30,75 @@
>  #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 _DSM Function for Retrieving QTG ID */
> +        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 }; /* function 1 only */
> +
> +            aml_append(ifctx2,
> +                       aml_return(aml_buffer(sizeof(byte_list), byte_list)));
> +        }
> +        aml_append(ifctx, ifctx2);
> +
> +        /*
> +         * Function 1
> +         * Creating a package with static values. The max supported QTG ID will
> +         * be 1 and recommended QTG IDs are 0 and then 1.
> +         * The values here are statically created to simplify emulation. Values
> +         * from a real BIOS would be determined by the performance of all the
> +         * present CXL memory and then assigned.
> +         */
> +        ifctx2 = aml_if(aml_equal(function, aml_int(1)));
> +        {
> +            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}
> +             * }
> +             *
> +             * While the SPEC specified WORD that hints at the value being
> +             * 16bit, the ACPI dump of BIOS DSDT table showed that the values
> +             * are integers with no specific size specification. aml_int() will
> +             * be used for the values.
> +             */
> +            pak1 = aml_package(2);
> +            /* Set QTG ID of 0 */
> +            aml_append(pak1, aml_int(0));
> +            /* Set QTG ID of 1 */
> +            aml_append(pak1, aml_int(1));
> +
> +            pak = aml_package(2);
> +            /* Set Max QTG 1 */
> +            aml_append(pak, aml_int(1));
> +            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
> 
> 

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

[Dave Jiang]

On 10/9/23 08:47, Jonathan Cameron wrote:
> On Fri, 06 Oct 2023 15:15:56 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add a simple _DSM call support for the ACPI0017 device to return fake QTG
>> ID values of 0 and 1 in all cases. This for _DSM plumbing testing from the OS.
>>
>> Following edited for readability
>>
>> 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)
>>                 {
>>                     One,
>>                     Package (0x02)
>>                     {
>>                         Zero,
>>                         One
>>                     }
>>                 })
>>             }
>>         }
>>     }
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>
>> --
> 
> You've picked up my bad habit / typo.  Needs to be ---
> to stop git picking up this text...

I preserved what was in the original post. I wasn't sure if Michael had it like that on purpose or if it was a typo. 

> 
>> v4: Change to package of ints rather than buffers. Also moved to 2 QTG IDs
>> to improve kernel side testing. Tested on x86 qemu guest against kernel
>> QTG ID _DSM parsing code to be upstreamed.
>>
>> 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>
> Given it's changed in significant way should drop these.

Sure. I just whole sale preserved what was there as revision history.


> 
> Anyhow, the tests also need updating.  I can do that and push out
> a v5 with tests as well once I've tested.
> 
> I'm assuming need tweaked kernel code to test?

https://lore.kernel.org/linux-cxl/168695160531.3031571.4875512229068707023.stgit@djiang5-mobl3/T/#mc45e3d8dd7463f8aeff432d57b834dd4831cc709

There may be some additional changes Dan wanted function input parameter wise. So there will be a v10. But here are the changes to use ints and fixing the input as well. Tested against the changes in qemu and looks to be working as expected. The inputs actually looks like below now:

+               [0].integer = { ACPI_TYPE_INTEGER, input->rd_lat },
+               [1].integer = { ACPI_TYPE_INTEGER, input->wr_lat },
+               [2].integer = { ACPI_TYPE_INTEGER, input->rd_bw },
+               [3].integer = { ACPI_TYPE_INTEGER, input->wr_bw },

> 
>> ---
>>  hw/acpi/cxl.c         |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/i386/acpi-build.c  |    1 +
>>  include/hw/acpi/cxl.h |    1 +
>>  3 files changed, 71 insertions(+)
>>
>> diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
>> index 92b46bc9323b..9cd7905ea25a 100644
>> --- a/hw/acpi/cxl.c
>> +++ b/hw/acpi/cxl.c
>> @@ -30,6 +30,75 @@
>>  #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 _DSM Function for Retrieving QTG ID */
>> +        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 }; /* function 1 only */
>> +
>> +            aml_append(ifctx2,
>> +                       aml_return(aml_buffer(sizeof(byte_list), byte_list)));
>> +        }
>> +        aml_append(ifctx, ifctx2);
>> +
>> +        /*
>> +         * Function 1
>> +         * Creating a package with static values. The max supported QTG ID will
>> +         * be 1 and recommended QTG IDs are 0 and then 1.
>> +         * The values here are statically created to simplify emulation. Values
>> +         * from a real BIOS would be determined by the performance of all the
>> +         * present CXL memory and then assigned.
>> +         */
>> +        ifctx2 = aml_if(aml_equal(function, aml_int(1)));
>> +        {
>> +            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}
>> +             * }
>> +             *
>> +             * While the SPEC specified WORD that hints at the value being
>> +             * 16bit, the ACPI dump of BIOS DSDT table showed that the values
>> +             * are integers with no specific size specification. aml_int() will
>> +             * be used for the values.
>> +             */
>> +            pak1 = aml_package(2);
>> +            /* Set QTG ID of 0 */
>> +            aml_append(pak1, aml_int(0));
>> +            /* Set QTG ID of 1 */
>> +            aml_append(pak1, aml_int(1));
>> +
>> +            pak = aml_package(2);
>> +            /* Set Max QTG 1 */
>> +            aml_append(pak, aml_int(1));
>> +            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
>>
>>
> 

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

[Jonathan Cameron]

On Fri, 6 Oct 2023 10:50:28 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> [..]
> > > > > 
> > > > > 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.    
> > >   
> > 
> > WORD does seem to be clearly defined in the ACPI spec as uint16
> > and as this is describing a DSDT blob I think we can safe that
> > it means that.  Also lines up with the fixed sizes in CEDT.  
> 
> I am not sure it means that, the ACPI specification indicates that packages
> can hold "integers" and integers can be any size up to 64-bits.

I agree that intent might be more general than an AML WORD
but was being paranoid on the basis using one was definitely never
wrong ;)

> 
> > > It's not clear buffer is actually word though.
> > > 
> > > Jonathan do you have hardware access?  
> > 
> > No.  +CC linux-cxl to see if anyone else has hardware + BIOS with
> > QTG implemented...  There will be lots of implementations soon so I'd make
> > not guarantee they will all interpret this the same.
> > 
> > Aim here is Linux kernel enablement support, and unfortunately that almost
> > always means we are ahead of easy availability of hardware. If it exists
> > its probably prototypes in a lab, in which case no guarantees on the
> > BIOS tables presented...  
> 
> From a pre-production system the ASL is putting the result of SizeOf
> directly into the first element in the return package:
> 
> 	Local1 = SizeOf (CXQI)
> 	Local0 [0x00] = Local1
> 
> ...where CXQI appears to be a fixed table of QTG ids for the platform, and
> SizeOf() returns an integer type with no restriction that it be a 16-bit
> value.
> 
> So I think the specification is misleading by specifying WORD when ACPI
> "Package" objects convey "Integers" where the size of the integer can be a
> u8 to a u64.

Good, that's simpler.

> 
> > > Also, possible to get clarification from the spec committee?  
> > 
> > I'm unclear what we are clarifying.  As I read it current implementation
> > is indeed wrong and I failed to notice this earlier :(
> > 
> > Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding)
> > should I think be
> > 
> > 0x0B 0x00 0x00
> > WordPrefix then data : note if you try a 0x0001 and feed
> > it to iasl it will squash it into a byte instead and indeed if you
> > force the binary to the above it will decode it as 0x0000 but recompile
> > that and you will be back to just
> > 0x00 (as bytes don't need a prefix..)
> > 
> > Currently it would be.
> > 0x11     0x05 0x0a 0x02 0x00 0x01
> > BufferOp 
> > 
> > Btw I built a minimal DSDT file to test this and iasl isn't happy with
> > the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2.
> > Whilst that's imdef territory as not covered by the CXL spec, we should
> > return 'something' ;)
> > 
> > Anyhow, to do this as per the CXL spec we need an aml_word()
> > that just implements the word case from aml_int()  
> 
> If I understand correctly, aml_int() is sufficient since this is not a
> Field() where access size matters.
> 
> > Chance are that it never matters if we get an ecoding that is
> > only single byte (because the value is small) but who knows what
> > other AML parsers might do.  
> 
> I expect the reason WORD is used in the specification is because of the
> size of the QTG ID field in the CFMWS. ACPI could support returning more
> than USHRT_MAX in an Integer field in a Package, but those IDs above
> USHRT_MAX could not be represented in CFMWS.

Agreed that's probably the cause and if anyone does use an AML WORD
by forcing the prefix, it does no harm for software stacks that
assume INTEGER.

> 
> [..]
> > > 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.  
> > 
> > The best we have in the way of description is the multiple QTG example
> > where it's
> > Package() {2, 1} combined with it being made up of WORDs
> > 
> > whereas in general that will get squashed to a pair of bytes...
> > So I'm thinking WORDs is max size rather than only size but
> > given ambiguity we should encode them as words anyway.  
> 
> My assertion is that for the Package format the size of the integer does
> not matter because the Package.Integer type can convey up to 64-bit values.
> For being compatible with the *usage* of that max id, values that do not
> fit into 16-bits are out of spec, but nothing prevents the Package from
> using any size integer, afaics.
> 
Works for me, though we should do the leg work to get the spec tweaked
to clarify this..

Jonathan

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

[Michael S. Tsirkin]

On Fri, Oct 06, 2023 at 01:09:39PM +0100, Jonathan Cameron wrote:
> On Thu, 5 Oct 2023 12:32:11 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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.  
> > 
> 
> WORD does seem to be clearly defined in the ACPI spec as uint16
> and as this is describing a DSDT blob I think we can safe that
> it means that.  Also lines up with the fixed sizes in CEDT.

Binary blobs are not really legal as return values of AML though.
What this patch was doing was a buffer. An alternative
interpretation would be an integer. Or something else yet ...


> > It's not clear buffer is actually word though.
> > 
> > Jonathan do you have hardware access?
> 
> No.  +CC linux-cxl to see if anyone else has hardware + BIOS with
> QTG implemented...  There will be lots of implementations soon so I'd make
> not guarantee they will all interpret this the same.
> 
> Aim here is Linux kernel enablement support, and unfortunately that almost
> always means we are ahead of easy availability of hardware. If it exists
> its probably prototypes in a lab, in which case no guarantees on the
> BIOS tables presented...
> 
> > 
> > Also, possible to get clarification from the spec committee?
> 
> I'm unclear what we are clarifying.

Let me clarify, below.

>  As I read it current implementation
> is indeed wrong and I failed to notice this earlier :(
> 
> Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding)
> should I think be
> 
> 0x0B 0x00 0x00
> WordPrefix then data : note if you try a 0x0001 and feed
> it to iasl it will squash it into a byte instead and indeed if you
> force the binary to the above it will decode it as 0x0000 but recompile
> that and you will be back to just
> 0x00 (as bytes don't need a prefix..)

Exactly. So this is the clarification we seek. ACPI spec
does mention WordPrefix however only as one of the
ways to encode an integer, as part of ComputationalData,
never directly. If CXL requires it to be WordPrefix then
qemu can do it but tools such as IASL will need to be taught a way
to force using WordPrefix.


> Currently it would be.
> 0x11     0x05 0x0a 0x02 0x00 0x01
> BufferOp 
> 
> Btw I built a minimal DSDT file to test this and iasl isn't happy with
> the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2.
> Whilst that's imdef territory as not covered by the CXL spec, we should
> return 'something' ;)
> 
> Anyhow, to do this as per the CXL spec we need an aml_word()
> that just implements the word case from aml_int()
> 
> Chance are that it never matters if we get an ecoding that is
> only single byte (because the value is small) but who knows what
> other AML parsers might do.
> 
> Something simple like (copy typed from test machine..)
> 
> Aml *aml_word(const uint16_t val)
> {
>     Aml *var = aml_alloc();
>     build_append_byte(var->buf, 0x0B);
>     build_append_int_noprefix(var->buf, val, 2);
>     return var;
> }
> 
> and one blob in acpi/cxl.c becomes
> 
>         ifctx2 = aml_if(aml_equal(function, aml_int(1)));
>         {
>              Aml *pak, *pac1;
> 
> 	     pak1 = aml_package(1)
> 	     aml_append(pak1, aml_word(0));
>              pak = aml_package(2);
>              aml_append(pack, aml_word(0x1));
>              aml_append(pak, pak1);
> 
>              aml_append(ifctx2, aml_return(pak));
>         }
>         aml_append(ifctx, ifctx2);
> ...
> 
>  
>         }
> 
> Give something like
> If ((Arg2 == One))
> {
>     Return (Package (0x02)
>     {
>         0x0001,
>         Package (0x01)
>         {
>             0x0000
>         }
>     })
> }
> 
> 
> Binary encoding then clearly uses packages of words.
> 
> 12      0b        02         0b    01 00     12    05    01        0b    00 00
> PkgOp   len       elements   word  0x0001    pkgOp len   elements  word  0x0000
> 
> (note I cheated an added a marker in one of the values and didn't decode
> the whole thing by hand ;)

We could. But I suspect it's a spec bug and they really just meant
"an integer in the range 0x0 to 0xffff, encoded in any legal way".


> > 
> > >   
> > > > 
> > > >   
> > > >> +             */
> > > >> +            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.
> 
> The best we have in the way of description is the multiple QTG example
> where it's
> Package() {2, 1} combined with it being made up of WORDs
> 
> whereas in general that will get squashed to a pair of bytes...
> So I'm thinking WORDs is max size rather than only size but
> given ambiguity we should encode them as words anyway.

But why, is it suddenly important to be compatible with lots of drivers?
These are just dummy values after all.
If the point is for driver development then I would say just use
aml_int, this will test support for One and Zero opcodes :)

> Note this breaks Dave's current kernel proposal which is assuming
> a buffer...
> https://lore.kernel.org/all/168695172301.3031571.9812118774299137032.stgit@djiang5-mobl3/
> 
> Hohum. Dave, can you sanity check with the appropriate SSWG person (MN IIRC)
> I can do it you'd prefer - just let me know.
> 
> Jonathan
> 

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

[Jonathan Cameron]

On Sat, 7 Oct 2023 17:17:44 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Oct 06, 2023 at 01:09:39PM +0100, Jonathan Cameron wrote:
> > On Thu, 5 Oct 2023 12:32:11 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > 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.    
> > >   
> > 
> > WORD does seem to be clearly defined in the ACPI spec as uint16
> > and as this is describing a DSDT blob I think we can safe that
> > it means that.  Also lines up with the fixed sizes in CEDT.  
> 
> Binary blobs are not really legal as return values of AML though.
> What this patch was doing was a buffer. An alternative
> interpretation would be an integer. Or something else yet ...

Yes. On taking another look, what was here definitely seems wrong.

> 
> 
> > > It's not clear buffer is actually word though.
> > > 
> > > Jonathan do you have hardware access?  
> > 
> > No.  +CC linux-cxl to see if anyone else has hardware + BIOS with
> > QTG implemented...  There will be lots of implementations soon so I'd make
> > not guarantee they will all interpret this the same.
> > 
> > Aim here is Linux kernel enablement support, and unfortunately that almost
> > always means we are ahead of easy availability of hardware. If it exists
> > its probably prototypes in a lab, in which case no guarantees on the
> > BIOS tables presented...
> >   
> > > 
> > > Also, possible to get clarification from the spec committee?  
> > 
> > I'm unclear what we are clarifying.  
> 
> Let me clarify, below.
> 
> >  As I read it current implementation
> > is indeed wrong and I failed to notice this earlier :(
> > 
> > Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding)
> > should I think be
> > 
> > 0x0B 0x00 0x00
> > WordPrefix then data : note if you try a 0x0001 and feed
> > it to iasl it will squash it into a byte instead and indeed if you
> > force the binary to the above it will decode it as 0x0000 but recompile
> > that and you will be back to just
> > 0x00 (as bytes don't need a prefix..)  
> 
> Exactly. So this is the clarification we seek. ACPI spec
> does mention WordPrefix however only as one of the
> ways to encode an integer, as part of ComputationalData,
> never directly. If CXL requires it to be WordPrefix then
> qemu can do it but tools such as IASL will need to be taught a way
> to force using WordPrefix.

Agreed.

> 
> 
> > Currently it would be.
> > 0x11     0x05 0x0a 0x02 0x00 0x01
> > BufferOp 
> > 
> > Btw I built a minimal DSDT file to test this and iasl isn't happy with
> > the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2.
> > Whilst that's imdef territory as not covered by the CXL spec, we should
> > return 'something' ;)
> > 
> > Anyhow, to do this as per the CXL spec we need an aml_word()
> > that just implements the word case from aml_int()
> > 
> > Chance are that it never matters if we get an ecoding that is
> > only single byte (because the value is small) but who knows what
> > other AML parsers might do.
> > 
> > Something simple like (copy typed from test machine..)
> > 
> > Aml *aml_word(const uint16_t val)
> > {
> >     Aml *var = aml_alloc();
> >     build_append_byte(var->buf, 0x0B);
> >     build_append_int_noprefix(var->buf, val, 2);
> >     return var;
> > }
> > 
> > and one blob in acpi/cxl.c becomes
> > 
> >         ifctx2 = aml_if(aml_equal(function, aml_int(1)));
> >         {
> >              Aml *pak, *pac1;
> > 
> > 	     pak1 = aml_package(1)
> > 	     aml_append(pak1, aml_word(0));
> >              pak = aml_package(2);
> >              aml_append(pack, aml_word(0x1));
> >              aml_append(pak, pak1);
> > 
> >              aml_append(ifctx2, aml_return(pak));
> >         }
> >         aml_append(ifctx, ifctx2);
> > ...
> > 
> >  
> >         }
> > 
> > Give something like
> > If ((Arg2 == One))
> > {
> >     Return (Package (0x02)
> >     {
> >         0x0001,
> >         Package (0x01)
> >         {
> >             0x0000
> >         }
> >     })
> > }
> > 
> > 
> > Binary encoding then clearly uses packages of words.
> > 
> > 12      0b        02         0b    01 00     12    05    01        0b    00 00
> > PkgOp   len       elements   word  0x0001    pkgOp len   elements  word  0x0000
> > 
> > (note I cheated an added a marker in one of the values and didn't decode
> > the whole thing by hand ;)  
> 
> We could. But I suspect it's a spec bug and they really just meant
> "an integer in the range 0x0 to 0xffff, encoded in any legal way".

I suspect you are correct.  We all love filing errata docs..
*sigh* Hopefully Dave will do it ;)


> 
> 
> > >   
> > > >     
> > > > > 
> > > > >     
> > > > >> +             */
> > > > >> +            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.  
> > 
> > The best we have in the way of description is the multiple QTG example
> > where it's
> > Package() {2, 1} combined with it being made up of WORDs
> > 
> > whereas in general that will get squashed to a pair of bytes...
> > So I'm thinking WORDs is max size rather than only size but
> > given ambiguity we should encode them as words anyway.  
> 
> But why, is it suddenly important to be compatible with lots of drivers?
> These are just dummy values after all.
> If the point is for driver development then I would say just use
> aml_int, this will test support for One and Zero opcodes :)

Works for me ;)

> 
> > Note this breaks Dave's current kernel proposal which is assuming
> > a buffer...
> > https://lore.kernel.org/all/168695172301.3031571.9812118774299137032.stgit@djiang5-mobl3/
> > 
> > Hohum. Dave, can you sanity check with the appropriate SSWG person (MN IIRC)
> > I can do it you'd prefer - just let me know.
> > 
> > Jonathan
> >   
> 
>