diff mbox series

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

Message ID 169646094762.643966.16021192876985391476.stgit@djiang5-mobl3 (mailing list archive)
State New, archived
Headers show
Series [v3] hw/cxl: Add QTG _DSM support for ACPI0017 device | expand

Commit Message

Dave Jiang Oct. 4, 2023, 11:09 p.m. UTC
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.

Following edited for readbility only

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(+)

Comments

Michael S. Tsirkin Oct. 5, 2023, 3:36 a.m. UTC | #1
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

> 
> Following edited for readbility only

readbility only -> 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)
>                 {
>                     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


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?

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

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

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.


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


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
>
Dave Jiang Oct. 5, 2023, 4:11 p.m. UTC | #2
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.

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

+        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
>>
>
Michael S. Tsirkin Oct. 5, 2023, 4:32 p.m. UTC | #3
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
> >>
> >
Michael S. Tsirkin Oct. 5, 2023, 5 p.m. UTC | #4
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.

Really? That would be a first in a long while for me. Maybe report to git mailing list.

> Will fix

My point is that name should match spec, not be a shortened
version of it.

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

ok then

Function 1 return value {1, {0}} indicates ..

and

 QTG ID of 1 -> QTG is 1.

these are faked in what sense? why not both 0?

I will let Jonathan decide whether it is wise to
come up with random stuff and expose it to userspace
by default. how will userspace know if we ever
start exposing real values?

> > 
> >> +         */
> >> +        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.
> 
> > 
> > 
> >> +             */
> >> +            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. 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. 
> 
> +        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
> >>
> >
Jonathan Cameron Oct. 6, 2023, 12:09 p.m. UTC | #5
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
Dan Williams Oct. 6, 2023, 5:50 p.m. UTC | #6
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.
Michael S. Tsirkin Oct. 7, 2023, 9:17 p.m. UTC | #7
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
>
Jonathan Cameron Oct. 9, 2023, 3:40 p.m. UTC | #8
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
> >   
> 
>
Jonathan Cameron Oct. 9, 2023, 3:44 p.m. UTC | #9
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
diff mbox series

Patch

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 *, QTG ID _DSM */
+        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 */
+
+            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.
+         */
+        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.
+             */
+            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));
+            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