Re: [PULL 33/63] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Thomas Huth <thuth@redhat.com>]

On 04/10/2023 10.44, Michael S. Tsirkin wrote:
> From: Dave Jiang <dave.jiang@intel.com>
> 
> 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>
> 
> --
> 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>
> ---
>   include/hw/acpi/cxl.h |  1 +
>   hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
>   hw/i386/acpi-build.c  |  1 +
>   3 files changed, 59 insertions(+)
> 
> diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> index acf4418886..8f22c71530 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
> diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> index 92b46bc932..5e9039785a 100644
> --- a/hw/acpi/cxl.c
> +++ b/hw/acpi/cxl.c
> @@ -30,6 +30,63 @@
>   #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}} inciate 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[1] = { 0x01 };
> +            uint16_t word_list2[1] = { 0 };
> +            uint8_t *byte_list = (uint8_t *)word_list;
> +            uint8_t *byte_list2 = (uint8_t *)word_list2;
> +            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), byte_list2));
> +            pak = aml_package(2);
> +            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_list));
> +            aml_append(pak, pak1);

This looks fishy ... first declaring a uint16_t array in host endian order, 
then casting it to an uint8_t* byte array for code that likely expects 
little endian byte order in the guest ... I assume that this won't work and 
needs some more love for big endian hosts?

  Thomas


> +            aml_append(ifctx2, aml_return(pak));
> +        }
> +        aml_append(ifctx, ifctx2);
> +    }
> +    aml_append(method, ifctx);
> +    aml_append(dev, method);
> +}