[PATCH v2 0/3] hw/cxl: Add dummy ACPI QTG DSM

[PATCH v2 0/3] hw/cxl: Add dummy ACPI QTG DSM

[Jonathan Cameron via]

v2 updates:
- Edit of patch description to not confuse matters by mentioning
  switches.
- Associated ACPI test updates.

CXL platforms may support the concept of QoS Thottling groups (QTG).

Typically you want to associate devices with similar performance
with the same QTG. As there is no standard way of understanding the
relationship between expected performance and appropriate QTG
the platform firmware provides a query mechanism via ACPI Device
Specific Method (DSM) with parameters of the performance numbers
that returns the appropriate QTG ID.

This support is basically stubbing out that function so that it always
returns 0. For now that is sufficient for current CXL emulation usecases.

Based on: [PATCH 0/4] hw/cxl: Minor CXL emulation fixes and cleanup
Based on: Message ID: 20230904132806.6094-1-Jonathan.Cameron@huawei.com

Dave Jiang (1):
  hw/cxl: Add QTG _DSM support for ACPI0017 device

Jonathan Cameron (2):
  tests/acpi: Allow update of DSDT.cxl
  tests/acpi: Update DSDT.cxl with QTG DSM

 include/hw/acpi/cxl.h        |   1 +
 hw/acpi/cxl.c                |  57 +++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c         |   1 +
 tests/data/acpi/q35/DSDT.cxl | Bin 9655 -> 9723 bytes
 4 files changed, 59 insertions(+)

-- 
2.39.2

[PATCH v2 1/3] tests/acpi: Allow update of DSDT.cxl

[Jonathan Cameron via]

Addition of QTG in following patch requires an update to the test
data.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..9ce0f596cc 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT.cxl",
-- 
2.39.2

[PATCH v2 2/3] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Jonathan Cameron via]

From: Dave Jiang <dave.jiang@intel.com>

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>

--
v2: Minor edit to drop reference to switches in patch description.
---
 include/hw/acpi/cxl.h |  1 +
 hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c  |  1 +
 3 files changed, 59 insertions(+)

diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
index acf4418886..8f22c71530 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
diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 92b46bc932..5e9039785a 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -30,6 +30,63 @@
 #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}} inciate 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[1] = { 0x01 };
+            uint16_t word_list2[1] = { 0 };
+            uint8_t *byte_list = (uint8_t *)word_list;
+            uint8_t *byte_list2 = (uint8_t *)word_list2;
+            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), byte_list2));
+            pak = aml_package(2);
+            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_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 bb12b0ad43..d3bc5875eb 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);
-- 
2.39.2

[PATCH v2 3/3] tests/acpi: Update DSDT.cxl with QTG DSM

[Jonathan Cameron via]

Description of change in previous patch.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 tests/data/acpi/q35/DSDT.cxl                | Bin 9655 -> 9723 bytes
 2 files changed, 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 9ce0f596cc..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.cxl",
diff --git a/tests/data/acpi/q35/DSDT.cxl b/tests/data/acpi/q35/DSDT.cxl
index ee16a861c2de7b7caaf11d91c50fcdf308815233..d4272e87c00e010a6805b6a276fcc87d9b6ead17 100644
GIT binary patch
delta 129
zcmdn){o9+%CD<k8w<-ezW5-6WiHaE>Z1KTP@zG5VY|arrz8vu$o-VwO&H<hV28QMg
zA{_C-A&v}77)2ae;$4D$c@|hs&JYyl5?J;#_4B>ug$~QIw(xNK_XREBoSen5M39-0
gae?^cEXE~5f=q&}Tuh7%LL7`B1_Q(9{fa-B0lXk1>i_@%

delta 61
zcmezEz1^G3CD<ioyD9?%qu)lZiHa&J@xe~<(M__>5k9^g@gANoypGNRo(2Yn<_sbn
R@xdXE3`-a{Gb{aI1^_O_5bOW|

-- 
2.39.2

Re: [PATCH v2 1/3] tests/acpi: Allow update of DSDT.cxl

[Fan Ni]

On Mon, Sep 04, 2023 at 05:18:45PM +0100, Jonathan Cameron wrote:

> Addition of QTG in following patch requires an update to the test
> data.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..9ce0f596cc 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT.cxl",
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 3/3] tests/acpi: Update DSDT.cxl with QTG DSM

[Fan Ni]

On Mon, Sep 04, 2023 at 05:18:47PM +0100, Jonathan Cameron wrote:

> Description of change in previous patch.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  tests/qtest/bios-tables-test-allowed-diff.h |   1 -
>  tests/data/acpi/q35/DSDT.cxl                | Bin 9655 -> 9723 bytes
>  2 files changed, 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 9ce0f596cc..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,2 +1 @@
>  /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/q35/DSDT.cxl",
> diff --git a/tests/data/acpi/q35/DSDT.cxl b/tests/data/acpi/q35/DSDT.cxl
> index ee16a861c2de7b7caaf11d91c50fcdf308815233..d4272e87c00e010a6805b6a276fcc87d9b6ead17 100644
> GIT binary patch
> delta 129
> zcmdn){o9+%CD<k8w<-ezW5-6WiHaE>Z1KTP@zG5VY|arrz8vu$o-VwO&H<hV28QMg
> zA{_C-A&v}77)2ae;$4D$c@|hs&JYyl5?J;#_4B>ug$~QIw(xNK_XREBoSen5M39-0
> gae?^cEXE~5f=q&}Tuh7%LL7`B1_Q(9{fa-B0lXk1>i_@%
> 
> delta 61
> zcmezEz1^G3CD<ioyD9?%qu)lZiHa&J@xe~<(M__>5k9^g@gANoypGNRo(2Yn<_sbn
> R@xdXE3`-a{Gb{aI1^_O_5bOW|
> 
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 2/3] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Fan Ni]

On Mon, Sep 04, 2023 at 05:18:46PM +0100, Jonathan Cameron wrote:

> From: Dave Jiang <dave.jiang@intel.com>
> 
> 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>

Looks good to me. One minor comment inline.
> 
> --
> v2: Minor edit to drop reference to switches in patch description.
> ---
>  include/hw/acpi/cxl.h |  1 +
>  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  |  1 +
>  3 files changed, 59 insertions(+)
> 
> diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> index acf4418886..8f22c71530 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
> diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> index 92b46bc932..5e9039785a 100644
> --- a/hw/acpi/cxl.c
> +++ b/hw/acpi/cxl.c
> @@ -30,6 +30,63 @@
>  #include "qapi/error.h"
>  #include "qemu/uuid.h"
>  
> +void build_cxl_dsm_method(Aml *dev)

Not a concern for now, I think, do we need to check the revision
field?

Fan
> +{
> +    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}} inciate 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[1] = { 0x01 };
> +            uint16_t word_list2[1] = { 0 };
> +            uint8_t *byte_list = (uint8_t *)word_list;
> +            uint8_t *byte_list2 = (uint8_t *)word_list2;
> +            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), byte_list2));
> +            pak = aml_package(2);
> +            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_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 bb12b0ad43..d3bc5875eb 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);
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 2/3] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Jonathan Cameron via]

On Tue, 12 Sep 2023 21:12:45 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> On Mon, Sep 04, 2023 at 05:18:46PM +0100, Jonathan Cameron wrote:
> 
> > From: Dave Jiang <dave.jiang@intel.com>
> > 
> > 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>  
> 
> Looks good to me. One minor comment inline.
> > 
> > --
> > v2: Minor edit to drop reference to switches in patch description.
> > ---
> >  include/hw/acpi/cxl.h |  1 +
> >  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c  |  1 +
> >  3 files changed, 59 insertions(+)
> > 
> > diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> > index acf4418886..8f22c71530 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
> > diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> > index 92b46bc932..5e9039785a 100644
> > --- a/hw/acpi/cxl.c
> > +++ b/hw/acpi/cxl.c
> > @@ -30,6 +30,63 @@
> >  #include "qapi/error.h"
> >  #include "qemu/uuid.h"
> >  
> > +void build_cxl_dsm_method(Aml *dev)  
> 
> Not a concern for now, I think, do we need to check the revision
> field?

Any new versions should be defined to be backwards compatible...
So hopefully not (though it has gone wrong a few times in the
past when spec reviewers like me haven't been paying attention :(
If that happens we'll have to deal with it when it becomes public.

Jonathan

> 
> Fan
> > +{
> > +    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}} inciate 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[1] = { 0x01 };
> > +            uint16_t word_list2[1] = { 0 };
> > +            uint8_t *byte_list = (uint8_t *)word_list;
> > +            uint8_t *byte_list2 = (uint8_t *)word_list2;
> > +            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), byte_list2));
> > +            pak = aml_package(2);
> > +            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_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 bb12b0ad43..d3bc5875eb 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);
> > -- 
> > 2.39.2
> > 
> >  

Re: [PATCH v2 1/3] tests/acpi: Allow update of DSDT.cxl

[Dave Jiang]

On 9/4/23 09:18, Jonathan Cameron wrote:
> Addition of QTG in following patch requires an update to the test
> data.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..9ce0f596cc 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT.cxl",

Re: [PATCH v2 3/3] tests/acpi: Update DSDT.cxl with QTG DSM

[Dave Jiang]

On 9/4/23 09:18, Jonathan Cameron wrote:
> Description of change in previous patch.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  tests/qtest/bios-tables-test-allowed-diff.h |   1 -
>  tests/data/acpi/q35/DSDT.cxl                | Bin 9655 -> 9723 bytes
>  2 files changed, 1 deletion(-)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index 9ce0f596cc..dfb8523c8b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1,2 +1 @@
>  /* List of comma-separated changed AML files to ignore */
> -"tests/data/acpi/q35/DSDT.cxl",
> diff --git a/tests/data/acpi/q35/DSDT.cxl b/tests/data/acpi/q35/DSDT.cxl
> index ee16a861c2de7b7caaf11d91c50fcdf308815233..d4272e87c00e010a6805b6a276fcc87d9b6ead17 100644
> GIT binary patch
> delta 129
> zcmdn){o9+%CD<k8w<-ezW5-6WiHaE>Z1KTP@zG5VY|arrz8vu$o-VwO&H<hV28QMg
> zA{_C-A&v}77)2ae;$4D$c@|hs&JYyl5?J;#_4B>ug$~QIw(xNK_XREBoSen5M39-0
> gae?^cEXE~5f=q&}Tuh7%LL7`B1_Q(9{fa-B0lXk1>i_@%
> 
> delta 61
> zcmezEz1^G3CD<ioyD9?%qu)lZiHa&J@xe~<(M__>5k9^g@gANoypGNRo(2Yn<_sbn
> R@xdXE3`-a{Gb{aI1^_O_5bOW|
> 

Re: [PATCH v2 2/3] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Michael S. Tsirkin]

On Mon, Sep 04, 2023 at 05:18:46PM +0100, Jonathan Cameron wrote:
> From: Dave Jiang <dave.jiang@intel.com>
> 
> 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>
> 
> --
> v2: Minor edit to drop reference to switches in patch description.
> ---

>  include/hw/acpi/cxl.h |  1 +
>  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  |  1 +
>  3 files changed, 59 insertions(+)


This is not the right way to format it. The correct way is:

---
v2: Minor edit to drop reference to switches in patch description.

 include/hw/acpi/cxl.h |  1 +
 hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c  |  1 +
 3 files changed, 59 insertions(+)

The way you do it breaks b4 and a bunch of other tools.  signatures must
come last before ---, then versioning info (which generally does not
need to be in git because readers of git have no access to older
versions, though there could be exceptions. If there's anything relevant
in this versioning history, such as some design directions which were
tried and discarded, then put it above ---). Then the diff.


> 
> diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> index acf4418886..8f22c71530 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
> diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> index 92b46bc932..5e9039785a 100644
> --- a/hw/acpi/cxl.c
> +++ b/hw/acpi/cxl.c
> @@ -30,6 +30,63 @@
>  #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}} inciate 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[1] = { 0x01 };
> +            uint16_t word_list2[1] = { 0 };
> +            uint8_t *byte_list = (uint8_t *)word_list;
> +            uint8_t *byte_list2 = (uint8_t *)word_list2;
> +            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), byte_list2));
> +            pak = aml_package(2);
> +            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_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 bb12b0ad43..d3bc5875eb 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);
> -- 
> 2.39.2

Re: [PATCH v2 2/3] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Jonathan Cameron via]

On Wed, 4 Oct 2023 05:04:31 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Sep 04, 2023 at 05:18:46PM +0100, Jonathan Cameron wrote:
> > From: Dave Jiang <dave.jiang@intel.com>
> > 
> > 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>
> > 
> > --
> > v2: Minor edit to drop reference to switches in patch description.
> > ---  
> 
> >  include/hw/acpi/cxl.h |  1 +
> >  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c  |  1 +
> >  3 files changed, 59 insertions(+)  
> 
> 
> This is not the right way to format it. The correct way is:
> 
> ---
> v2: Minor edit to drop reference to switches in patch description.
> 
>  include/hw/acpi/cxl.h |  1 +
>  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  |  1 +
>  3 files changed, 59 insertions(+)
> 
> The way you do it breaks b4 and a bunch of other tools.  signatures must
> come last before ---, then versioning info (which generally does not
> need to be in git because readers of git have no access to older
> versions, though there could be exceptions. If there's anything relevant
> in this versioning history, such as some design directions which were
> tried and discarded, then put it above ---). Then the diff.

Sorry, typo. I believe b4 and git am are also fine with what was intended
here.

---
v2: xxx
---

stats

which has advantage that you can track the change log easily in a git tree
as things evolve rather than a bunch of hand editing of patches to add
this stuff in at time of send.

I'll be more careful to check for errors like this in future.

Thanks for fixing it up and sorry for the waste of your time!

Jonathan



> 
> 
> > 
> > diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> > index acf4418886..8f22c71530 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
> > diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> > index 92b46bc932..5e9039785a 100644
> > --- a/hw/acpi/cxl.c
> > +++ b/hw/acpi/cxl.c
> > @@ -30,6 +30,63 @@
> >  #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}} inciate 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[1] = { 0x01 };
> > +            uint16_t word_list2[1] = { 0 };
> > +            uint8_t *byte_list = (uint8_t *)word_list;
> > +            uint8_t *byte_list2 = (uint8_t *)word_list2;
> > +            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), byte_list2));
> > +            pak = aml_package(2);
> > +            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_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 bb12b0ad43..d3bc5875eb 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);
> > -- 
> > 2.39.2  
> 

Re: [PATCH v2 2/3] hw/cxl: Add QTG _DSM support for ACPI0017 device

[Jonathan Cameron]

On Wed, 4 Oct 2023 05:04:31 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Sep 04, 2023 at 05:18:46PM +0100, Jonathan Cameron wrote:
> > From: Dave Jiang <dave.jiang@intel.com>
> > 
> > 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>
> > 
> > --
> > v2: Minor edit to drop reference to switches in patch description.
> > ---  
> 
> >  include/hw/acpi/cxl.h |  1 +
> >  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c  |  1 +
> >  3 files changed, 59 insertions(+)  
> 
> 
> This is not the right way to format it. The correct way is:
> 
> ---
> v2: Minor edit to drop reference to switches in patch description.
> 
>  include/hw/acpi/cxl.h |  1 +
>  hw/acpi/cxl.c         | 57 +++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  |  1 +
>  3 files changed, 59 insertions(+)
> 
> The way you do it breaks b4 and a bunch of other tools.  signatures must
> come last before ---, then versioning info (which generally does not
> need to be in git because readers of git have no access to older
> versions, though there could be exceptions. If there's anything relevant
> in this versioning history, such as some design directions which were
> tried and discarded, then put it above ---). Then the diff.

Sorry, typo. I believe b4 and git am are also fine with what was intended
here.

---
v2: xxx
---

stats

which has advantage that you can track the change log easily in a git tree
as things evolve rather than a bunch of hand editing of patches to add
this stuff in at time of send.

I'll be more careful to check for errors like this in future.

Thanks for fixing it up and sorry for the waste of your time!

Jonathan



> 
> 
> > 
> > diff --git a/include/hw/acpi/cxl.h b/include/hw/acpi/cxl.h
> > index acf4418886..8f22c71530 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
> > diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
> > index 92b46bc932..5e9039785a 100644
> > --- a/hw/acpi/cxl.c
> > +++ b/hw/acpi/cxl.c
> > @@ -30,6 +30,63 @@
> >  #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}} inciate 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[1] = { 0x01 };
> > +            uint16_t word_list2[1] = { 0 };
> > +            uint8_t *byte_list = (uint8_t *)word_list;
> > +            uint8_t *byte_list2 = (uint8_t *)word_list2;
> > +            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), byte_list2));
> > +            pak = aml_package(2);
> > +            aml_append(pak, aml_buffer(sizeof(uint16_t), byte_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 bb12b0ad43..d3bc5875eb 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);
> > -- 
> > 2.39.2  
>