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