On Sat, 7 Oct 2023 17:17:44 -0400
"Michael S. Tsirkin" <m...@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" <m...@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.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.
> > >
> >
> > 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
> >
>
>
