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