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

[Dave Jiang <dave.jiang@intel.com>]

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