| Message ID | 169646094762.643966.16021192876985391476.stgit@djiang5-mobl3 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] hw/cxl: Add QTG _DSM support for ACPI0017 device | expand |
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.jiang@intel.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@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 >
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.jiang@intel.com> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@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. > > >> + */ >> + 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. 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. + 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 >> >
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.jiang@intel.com> > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> > >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > >> Signed-off-by: Michael S. Tsirkin <mst@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 > >> > >
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.jiang@intel.com> > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> > >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > >> Signed-off-by: Michael S. Tsirkin <mst@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. Really? That would be a first in a long while for me. Maybe report to git mailing list. > Will fix My point is that name should match spec, not be a shortened version of it. > > > > > > 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. ok then Function 1 return value {1, {0}} indicates .. and QTG ID of 1 -> QTG is 1. these are faked in what sense? why not both 0? I will let Jonathan decide whether it is wise to come up with random stuff and expose it to userspace by default. how will userspace know if we ever start exposing real values? > > > >> + */ > >> + 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. > > > > > > >> + */ > >> + 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. 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. > > + 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 > >> > >
On Thu, 5 Oct 2023 12:32:11 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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.jiang@intel.com> > > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> > > >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > >> Signed-off-by: Michael S. Tsirkin <mst@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. > WORD does seem to be clearly defined in the ACPI spec as uint16 and as this is describing a DSDT blob I think we can safe that it means that. Also lines up with the fixed sizes in CEDT. > It's not clear buffer is actually word though. > > Jonathan do you have hardware access? No. +CC linux-cxl to see if anyone else has hardware + BIOS with QTG implemented... There will be lots of implementations soon so I'd make not guarantee they will all interpret this the same. Aim here is Linux kernel enablement support, and unfortunately that almost always means we are ahead of easy availability of hardware. If it exists its probably prototypes in a lab, in which case no guarantees on the BIOS tables presented... > > Also, possible to get clarification from the spec committee? I'm unclear what we are clarifying. As I read it current implementation is indeed wrong and I failed to notice this earlier :( Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding) should I think be 0x0B 0x00 0x00 WordPrefix then data : note if you try a 0x0001 and feed it to iasl it will squash it into a byte instead and indeed if you force the binary to the above it will decode it as 0x0000 but recompile that and you will be back to just 0x00 (as bytes don't need a prefix..) Currently it would be. 0x11 0x05 0x0a 0x02 0x00 0x01 BufferOp Btw I built a minimal DSDT file to test this and iasl isn't happy with the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2. Whilst that's imdef territory as not covered by the CXL spec, we should return 'something' ;) Anyhow, to do this as per the CXL spec we need an aml_word() that just implements the word case from aml_int() Chance are that it never matters if we get an ecoding that is only single byte (because the value is small) but who knows what other AML parsers might do. Something simple like (copy typed from test machine..) Aml *aml_word(const uint16_t val) { Aml *var = aml_alloc(); build_append_byte(var->buf, 0x0B); build_append_int_noprefix(var->buf, val, 2); return var; } and one blob in acpi/cxl.c becomes ifctx2 = aml_if(aml_equal(function, aml_int(1))); { Aml *pak, *pac1; pak1 = aml_package(1) aml_append(pak1, aml_word(0)); pak = aml_package(2); aml_append(pack, aml_word(0x1)); aml_append(pak, pak1); aml_append(ifctx2, aml_return(pak)); } aml_append(ifctx, ifctx2); ... } Give something like If ((Arg2 == One)) { Return (Package (0x02) { 0x0001, Package (0x01) { 0x0000 } }) } Binary encoding then clearly uses packages of words. 12 0b 02 0b 01 00 12 05 01 0b 00 00 PkgOp len elements word 0x0001 pkgOp len elements word 0x0000 (note I cheated an added a marker in one of the values and didn't decode the whole thing by hand ;) > > > > > > > > > > > >> + */ > > >> + 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. The best we have in the way of description is the multiple QTG example where it's Package() {2, 1} combined with it being made up of WORDs whereas in general that will get squashed to a pair of bytes... So I'm thinking WORDs is max size rather than only size but given ambiguity we should encode them as words anyway. Note this breaks Dave's current kernel proposal which is assuming a buffer... https://lore.kernel.org/all/168695172301.3031571.9812118774299137032.stgit@djiang5-mobl3/ Hohum. Dave, can you sanity check with the appropriate SSWG person (MN IIRC) I can do it you'd prefer - just let me know. Jonathan
Jonathan Cameron wrote: [..] > > > > > > > > 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. > > > > WORD does seem to be clearly defined in the ACPI spec as uint16 > and as this is describing a DSDT blob I think we can safe that > it means that. Also lines up with the fixed sizes in CEDT. I am not sure it means that, the ACPI specification indicates that packages can hold "integers" and integers can be any size up to 64-bits. > > It's not clear buffer is actually word though. > > > > Jonathan do you have hardware access? > > No. +CC linux-cxl to see if anyone else has hardware + BIOS with > QTG implemented... There will be lots of implementations soon so I'd make > not guarantee they will all interpret this the same. > > Aim here is Linux kernel enablement support, and unfortunately that almost > always means we are ahead of easy availability of hardware. If it exists > its probably prototypes in a lab, in which case no guarantees on the > BIOS tables presented... From a pre-production system the ASL is putting the result of SizeOf directly into the first element in the return package: Local1 = SizeOf (CXQI) Local0 [0x00] = Local1 ...where CXQI appears to be a fixed table of QTG ids for the platform, and SizeOf() returns an integer type with no restriction that it be a 16-bit value. So I think the specification is misleading by specifying WORD when ACPI "Package" objects convey "Integers" where the size of the integer can be a u8 to a u64. > > Also, possible to get clarification from the spec committee? > > I'm unclear what we are clarifying. As I read it current implementation > is indeed wrong and I failed to notice this earlier :( > > Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding) > should I think be > > 0x0B 0x00 0x00 > WordPrefix then data : note if you try a 0x0001 and feed > it to iasl it will squash it into a byte instead and indeed if you > force the binary to the above it will decode it as 0x0000 but recompile > that and you will be back to just > 0x00 (as bytes don't need a prefix..) > > Currently it would be. > 0x11 0x05 0x0a 0x02 0x00 0x01 > BufferOp > > Btw I built a minimal DSDT file to test this and iasl isn't happy with > the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2. > Whilst that's imdef territory as not covered by the CXL spec, we should > return 'something' ;) > > Anyhow, to do this as per the CXL spec we need an aml_word() > that just implements the word case from aml_int() If I understand correctly, aml_int() is sufficient since this is not a Field() where access size matters. > Chance are that it never matters if we get an ecoding that is > only single byte (because the value is small) but who knows what > other AML parsers might do. I expect the reason WORD is used in the specification is because of the size of the QTG ID field in the CFMWS. ACPI could support returning more than USHRT_MAX in an Integer field in a Package, but those IDs above USHRT_MAX could not be represented in CFMWS. [..] > > 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. > > The best we have in the way of description is the multiple QTG example > where it's > Package() {2, 1} combined with it being made up of WORDs > > whereas in general that will get squashed to a pair of bytes... > So I'm thinking WORDs is max size rather than only size but > given ambiguity we should encode them as words anyway. My assertion is that for the Package format the size of the integer does not matter because the Package.Integer type can convey up to 64-bit values. For being compatible with the *usage* of that max id, values that do not fit into 16-bits are out of spec, but nothing prevents the Package from using any size integer, afaics.
On Fri, Oct 06, 2023 at 01:09:39PM +0100, Jonathan Cameron wrote: > On Thu, 5 Oct 2023 12:32:11 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > 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.jiang@intel.com> > > > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> > > > >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > >> Signed-off-by: Michael S. Tsirkin <mst@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. > > > > WORD does seem to be clearly defined in the ACPI spec as uint16 > and as this is describing a DSDT blob I think we can safe that > it means that. Also lines up with the fixed sizes in CEDT. Binary blobs are not really legal as return values of AML though. What this patch was doing was a buffer. An alternative interpretation would be an integer. Or something else yet ... > > It's not clear buffer is actually word though. > > > > Jonathan do you have hardware access? > > No. +CC linux-cxl to see if anyone else has hardware + BIOS with > QTG implemented... There will be lots of implementations soon so I'd make > not guarantee they will all interpret this the same. > > Aim here is Linux kernel enablement support, and unfortunately that almost > always means we are ahead of easy availability of hardware. If it exists > its probably prototypes in a lab, in which case no guarantees on the > BIOS tables presented... > > > > > Also, possible to get clarification from the spec committee? > > I'm unclear what we are clarifying. Let me clarify, below. > As I read it current implementation > is indeed wrong and I failed to notice this earlier :( > > Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding) > should I think be > > 0x0B 0x00 0x00 > WordPrefix then data : note if you try a 0x0001 and feed > it to iasl it will squash it into a byte instead and indeed if you > force the binary to the above it will decode it as 0x0000 but recompile > that and you will be back to just > 0x00 (as bytes don't need a prefix..) Exactly. So this is the clarification we seek. ACPI spec does mention WordPrefix however only as one of the ways to encode an integer, as part of ComputationalData, never directly. If CXL requires it to be WordPrefix then qemu can do it but tools such as IASL will need to be taught a way to force using WordPrefix. > Currently it would be. > 0x11 0x05 0x0a 0x02 0x00 0x01 > BufferOp > > Btw I built a minimal DSDT file to test this and iasl isn't happy with > the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2. > Whilst that's imdef territory as not covered by the CXL spec, we should > return 'something' ;) > > Anyhow, to do this as per the CXL spec we need an aml_word() > that just implements the word case from aml_int() > > Chance are that it never matters if we get an ecoding that is > only single byte (because the value is small) but who knows what > other AML parsers might do. > > Something simple like (copy typed from test machine..) > > Aml *aml_word(const uint16_t val) > { > Aml *var = aml_alloc(); > build_append_byte(var->buf, 0x0B); > build_append_int_noprefix(var->buf, val, 2); > return var; > } > > and one blob in acpi/cxl.c becomes > > ifctx2 = aml_if(aml_equal(function, aml_int(1))); > { > Aml *pak, *pac1; > > pak1 = aml_package(1) > aml_append(pak1, aml_word(0)); > pak = aml_package(2); > aml_append(pack, aml_word(0x1)); > aml_append(pak, pak1); > > aml_append(ifctx2, aml_return(pak)); > } > aml_append(ifctx, ifctx2); > ... > > > } > > Give something like > If ((Arg2 == One)) > { > Return (Package (0x02) > { > 0x0001, > Package (0x01) > { > 0x0000 > } > }) > } > > > Binary encoding then clearly uses packages of words. > > 12 0b 02 0b 01 00 12 05 01 0b 00 00 > PkgOp len elements word 0x0001 pkgOp len elements word 0x0000 > > (note I cheated an added a marker in one of the values and didn't decode > the whole thing by hand ;) We could. But I suspect it's a spec bug and they really just meant "an integer in the range 0x0 to 0xffff, encoded in any legal way". > > > > > > > > > > > > > > > > >> + */ > > > >> + 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. > > The best we have in the way of description is the multiple QTG example > where it's > Package() {2, 1} combined with it being made up of WORDs > > whereas in general that will get squashed to a pair of bytes... > So I'm thinking WORDs is max size rather than only size but > given ambiguity we should encode them as words anyway. But why, is it suddenly important to be compatible with lots of drivers? These are just dummy values after all. If the point is for driver development then I would say just use aml_int, this will test support for One and Zero opcodes :) > Note this breaks Dave's current kernel proposal which is assuming > a buffer... > https://lore.kernel.org/all/168695172301.3031571.9812118774299137032.stgit@djiang5-mobl3/ > > Hohum. Dave, can you sanity check with the appropriate SSWG person (MN IIRC) > I can do it you'd prefer - just let me know. > > Jonathan >
On Sat, 7 Oct 2023 17:17:44 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Oct 06, 2023 at 01:09:39PM +0100, Jonathan Cameron wrote: > > On Thu, 5 Oct 2023 12:32:11 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > 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.jiang@intel.com> > > > > >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.Cameron@huawei.com> > > > > >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > >> Signed-off-by: Michael S. Tsirkin <mst@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. > > > > > > > WORD does seem to be clearly defined in the ACPI spec as uint16 > > and as this is describing a DSDT blob I think we can safe that > > it means that. Also lines up with the fixed sizes in CEDT. > > Binary blobs are not really legal as return values of AML though. > What this patch was doing was a buffer. An alternative > interpretation would be an integer. Or something else yet ... Yes. On taking another look, what was here definitely seems wrong. > > > > > It's not clear buffer is actually word though. > > > > > > Jonathan do you have hardware access? > > > > No. +CC linux-cxl to see if anyone else has hardware + BIOS with > > QTG implemented... There will be lots of implementations soon so I'd make > > not guarantee they will all interpret this the same. > > > > Aim here is Linux kernel enablement support, and unfortunately that almost > > always means we are ahead of easy availability of hardware. If it exists > > its probably prototypes in a lab, in which case no guarantees on the > > BIOS tables presented... > > > > > > > > Also, possible to get clarification from the spec committee? > > > > I'm unclear what we are clarifying. > > Let me clarify, below. > > > As I read it current implementation > > is indeed wrong and I failed to notice this earlier :( > > > > Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding) > > should I think be > > > > 0x0B 0x00 0x00 > > WordPrefix then data : note if you try a 0x0001 and feed > > it to iasl it will squash it into a byte instead and indeed if you > > force the binary to the above it will decode it as 0x0000 but recompile > > that and you will be back to just > > 0x00 (as bytes don't need a prefix..) > > Exactly. So this is the clarification we seek. ACPI spec > does mention WordPrefix however only as one of the > ways to encode an integer, as part of ComputationalData, > never directly. If CXL requires it to be WordPrefix then > qemu can do it but tools such as IASL will need to be taught a way > to force using WordPrefix. Agreed. > > > > Currently it would be. > > 0x11 0x05 0x0a 0x02 0x00 0x01 > > BufferOp > > > > Btw I built a minimal DSDT file to test this and iasl isn't happy with > > the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2. > > Whilst that's imdef territory as not covered by the CXL spec, we should > > return 'something' ;) > > > > Anyhow, to do this as per the CXL spec we need an aml_word() > > that just implements the word case from aml_int() > > > > Chance are that it never matters if we get an ecoding that is > > only single byte (because the value is small) but who knows what > > other AML parsers might do. > > > > Something simple like (copy typed from test machine..) > > > > Aml *aml_word(const uint16_t val) > > { > > Aml *var = aml_alloc(); > > build_append_byte(var->buf, 0x0B); > > build_append_int_noprefix(var->buf, val, 2); > > return var; > > } > > > > and one blob in acpi/cxl.c becomes > > > > ifctx2 = aml_if(aml_equal(function, aml_int(1))); > > { > > Aml *pak, *pac1; > > > > pak1 = aml_package(1) > > aml_append(pak1, aml_word(0)); > > pak = aml_package(2); > > aml_append(pack, aml_word(0x1)); > > aml_append(pak, pak1); > > > > aml_append(ifctx2, aml_return(pak)); > > } > > aml_append(ifctx, ifctx2); > > ... > > > > > > } > > > > Give something like > > If ((Arg2 == One)) > > { > > Return (Package (0x02) > > { > > 0x0001, > > Package (0x01) > > { > > 0x0000 > > } > > }) > > } > > > > > > Binary encoding then clearly uses packages of words. > > > > 12 0b 02 0b 01 00 12 05 01 0b 00 00 > > PkgOp len elements word 0x0001 pkgOp len elements word 0x0000 > > > > (note I cheated an added a marker in one of the values and didn't decode > > the whole thing by hand ;) > > We could. But I suspect it's a spec bug and they really just meant > "an integer in the range 0x0 to 0xffff, encoded in any legal way". I suspect you are correct. We all love filing errata docs.. *sigh* Hopefully Dave will do it ;) > > > > > > > > > > > > > > > > > > > > > > > >> + */ > > > > >> + 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. > > > > The best we have in the way of description is the multiple QTG example > > where it's > > Package() {2, 1} combined with it being made up of WORDs > > > > whereas in general that will get squashed to a pair of bytes... > > So I'm thinking WORDs is max size rather than only size but > > given ambiguity we should encode them as words anyway. > > But why, is it suddenly important to be compatible with lots of drivers? > These are just dummy values after all. > If the point is for driver development then I would say just use > aml_int, this will test support for One and Zero opcodes :) Works for me ;) > > > Note this breaks Dave's current kernel proposal which is assuming > > a buffer... > > https://lore.kernel.org/all/168695172301.3031571.9812118774299137032.stgit@djiang5-mobl3/ > > > > Hohum. Dave, can you sanity check with the appropriate SSWG person (MN IIRC) > > I can do it you'd prefer - just let me know. > > > > Jonathan > > > >
On Fri, 6 Oct 2023 10:50:28 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > [..] > > > > > > > > > > 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. > > > > > > > WORD does seem to be clearly defined in the ACPI spec as uint16 > > and as this is describing a DSDT blob I think we can safe that > > it means that. Also lines up with the fixed sizes in CEDT. > > I am not sure it means that, the ACPI specification indicates that packages > can hold "integers" and integers can be any size up to 64-bits. I agree that intent might be more general than an AML WORD but was being paranoid on the basis using one was definitely never wrong ;) > > > > It's not clear buffer is actually word though. > > > > > > Jonathan do you have hardware access? > > > > No. +CC linux-cxl to see if anyone else has hardware + BIOS with > > QTG implemented... There will be lots of implementations soon so I'd make > > not guarantee they will all interpret this the same. > > > > Aim here is Linux kernel enablement support, and unfortunately that almost > > always means we are ahead of easy availability of hardware. If it exists > > its probably prototypes in a lab, in which case no guarantees on the > > BIOS tables presented... > > From a pre-production system the ASL is putting the result of SizeOf > directly into the first element in the return package: > > Local1 = SizeOf (CXQI) > Local0 [0x00] = Local1 > > ...where CXQI appears to be a fixed table of QTG ids for the platform, and > SizeOf() returns an integer type with no restriction that it be a 16-bit > value. > > So I think the specification is misleading by specifying WORD when ACPI > "Package" objects convey "Integers" where the size of the integer can be a > u8 to a u64. Good, that's simpler. > > > > Also, possible to get clarification from the spec committee? > > > > I'm unclear what we are clarifying. As I read it current implementation > > is indeed wrong and I failed to notice this earlier :( > > > > Ultimately data encoding (ACPI 6.5 section 20.2..3 Data Objects Encoding) > > should I think be > > > > 0x0B 0x00 0x00 > > WordPrefix then data : note if you try a 0x0001 and feed > > it to iasl it will squash it into a byte instead and indeed if you > > force the binary to the above it will decode it as 0x0000 but recompile > > that and you will be back to just > > 0x00 (as bytes don't need a prefix..) > > > > Currently it would be. > > 0x11 0x05 0x0a 0x02 0x00 0x01 > > BufferOp > > > > Btw I built a minimal DSDT file to test this and iasl isn't happy with > > the fact the _DSM doesn't return anything at all if ARG2 isn't 1 or 2. > > Whilst that's imdef territory as not covered by the CXL spec, we should > > return 'something' ;) > > > > Anyhow, to do this as per the CXL spec we need an aml_word() > > that just implements the word case from aml_int() > > If I understand correctly, aml_int() is sufficient since this is not a > Field() where access size matters. > > > Chance are that it never matters if we get an ecoding that is > > only single byte (because the value is small) but who knows what > > other AML parsers might do. > > I expect the reason WORD is used in the specification is because of the > size of the QTG ID field in the CFMWS. ACPI could support returning more > than USHRT_MAX in an Integer field in a Package, but those IDs above > USHRT_MAX could not be represented in CFMWS. Agreed that's probably the cause and if anyone does use an AML WORD by forcing the prefix, it does no harm for software stacks that assume INTEGER. > > [..] > > > 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. > > > > The best we have in the way of description is the multiple QTG example > > where it's > > Package() {2, 1} combined with it being made up of WORDs > > > > whereas in general that will get squashed to a pair of bytes... > > So I'm thinking WORDs is max size rather than only size but > > given ambiguity we should encode them as words anyway. > > My assertion is that for the Package format the size of the integer does > not matter because the Package.Integer type can convey up to 64-bit values. > For being compatible with the *usage* of that max id, values that do not > fit into 16-bits are out of spec, but nothing prevents the Package from > using any size integer, afaics. > Works for me, though we should do the leg work to get the spec tweaked to clarify this.. Jonathan
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 *, 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}} indicates 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 = 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. + */ + 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)); + 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
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> -- 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> --- hw/acpi/cxl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/i386/acpi-build.c | 1 + include/hw/acpi/cxl.h | 1 + 3 files changed, 57 insertions(+)