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 <[email protected]>
> >> Signed-off-by: Jonathan Cameron <[email protected]>
> >>
> >> --
> >> 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: <[email protected]>
> >> Reviewed-by: Michael S. Tsirkin <[email protected]>
> >> Signed-off-by: Michael S. Tsirkin <[email protected]>
> >> ---
> >> 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
> >>
> >
