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.ji...@intel.com>
> Signed-off-by: Jonathan Cameron <jonathan.came...@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.came...@huawei.com>
> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Michael S. Tsirkin <m...@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
>
