[PATCH v3 0/4] hw/cxl: Support emulating 4 HDM decoders throughout topology

[PATCH v3 0/4] hw/cxl: Support emulating 4 HDM decoders throughout topology

[Jonathan Cameron via]

v3: Thanks to Philippe.
 - Pull the hdm_inc change out as a precursor to the increase in HDM
   decoders (new patch: 3)
 - Fix a bug where cxl-host used the encoded count as if it were the
   decoded version.

For initial CXL emulation / kernel driver bring up a single Host-managed
Device Memory (HDM) decoder instance was sufficient as it let us test the
basic region creation code etc. More complex testing appropriate today
requires a more realistic configuration with multiple decoders.

The Linux kernel will use separate decoders for each memory type (and
shortly per DCD region) and for each interleave set within a memory type
or DCD region. 4 decoders are sufficient for most test cases today but
we may need to grow these further in future.

This patch set already allowed us to identify one kernel bug which is
now fixed.
https://lore.kernel.org/linux-cxl/168696507968.3590522.14484000711718573626.stgit@dwillia2-xfh.jf.intel.com/

Note that, whilst I'm proposing this series for upstream (based on
priorities of what we have out of tree) it hasn't previously been posted
so needs review. (I failed to send it out previously)

Based on: [PATCH 0/4] hw/cxl: Minor CXL emulation fixes and cleanup
Based on: [PATCH v2 0/3] hw/cxl: Add dummy ACPI QTG DSM

Based on: Message ID: 20230904132806.6094-1-Jonathan.Cameron@huawei.com
Based on: Message ID: 20230904161847.18468-1-Jonathan.Cameron@huawei.com

Jonathan Cameron (4):
  hw/cxl: Push cxl_decoder_count_enc() and cxl_decode_ig() into .c
  hw/cxl: Add utility functions decoder interleave ways and target
    count.
  hw/cxl: Fix and use same calculation for HDM decoder block size
    everywhere
  hw/cxl: Support 4 HDM decoders at all levels of topology

 include/hw/cxl/cxl_component.h |  30 +++++-----
 hw/cxl/cxl-component-utils.c   |  91 +++++++++++++++++++++++++----
 hw/cxl/cxl-host.c              |  67 +++++++++++++++-------
 hw/mem/cxl_type3.c             | 102 +++++++++++++++++++++++----------
 4 files changed, 212 insertions(+), 78 deletions(-)

-- 
2.39.2

[PATCH v3 1/4] hw/cxl: Push cxl_decoder_count_enc() and cxl_decode_ig() into .c

[Jonathan Cameron via]

There is no strong justification for keeping these in the header
so push them down into the associated cxl-component-utils.c file.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/hw/cxl/cxl_component.h | 18 ++----------------
 hw/cxl/cxl-component-utils.c   | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 42c7e581a7..bdb3881a6b 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -225,26 +225,12 @@ void cxl_component_create_dvsec(CXLComponentState *cxl_cstate,
                                 enum reg_type cxl_dev_type, uint16_t length,
                                 uint16_t type, uint8_t rev, uint8_t *body);
 
-static inline int cxl_decoder_count_enc(int count)
-{
-    switch (count) {
-    case 1: return 0;
-    case 2: return 1;
-    case 4: return 2;
-    case 6: return 3;
-    case 8: return 4;
-    case 10: return 5;
-    }
-    return 0;
-}
+int cxl_decoder_count_enc(int count);
 
 uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
 uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
 
-static inline hwaddr cxl_decode_ig(int ig)
-{
-    return 1ULL << (ig + 8);
-}
+hwaddr cxl_decode_ig(int ig);
 
 CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
 bool cxl_get_hb_passthrough(PCIHostState *hb);
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 378f1082ce..ea2d4770ec 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -13,6 +13,24 @@
 #include "hw/pci/pci.h"
 #include "hw/cxl/cxl.h"
 
+int cxl_decoder_count_enc(int count)
+{
+    switch (count) {
+    case 1: return 0;
+    case 2: return 1;
+    case 4: return 2;
+    case 6: return 3;
+    case 8: return 4;
+    case 10: return 5;
+    }
+    return 0;
+}
+
+hwaddr cxl_decode_ig(int ig)
+{
+    return 1ULL << (ig + 8);
+}
+
 static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
                                        unsigned size)
 {
-- 
2.39.2

Re: [PATCH v3 1/4] hw/cxl: Push cxl_decoder_count_enc() and cxl_decode_ig() into .c

[Fan Ni]

On Mon, Sep 11, 2023 at 12:43:10PM +0100, Jonathan Cameron wrote:

> There is no strong justification for keeping these in the header
> so push them down into the associated cxl-component-utils.c file.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

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

>  include/hw/cxl/cxl_component.h | 18 ++----------------
>  hw/cxl/cxl-component-utils.c   | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 42c7e581a7..bdb3881a6b 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -225,26 +225,12 @@ void cxl_component_create_dvsec(CXLComponentState *cxl_cstate,
>                                  enum reg_type cxl_dev_type, uint16_t length,
>                                  uint16_t type, uint8_t rev, uint8_t *body);
>  
> -static inline int cxl_decoder_count_enc(int count)
> -{
> -    switch (count) {
> -    case 1: return 0;
> -    case 2: return 1;
> -    case 4: return 2;
> -    case 6: return 3;
> -    case 8: return 4;
> -    case 10: return 5;
> -    }
> -    return 0;
> -}
> +int cxl_decoder_count_enc(int count);
>  
>  uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
>  uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
>  
> -static inline hwaddr cxl_decode_ig(int ig)
> -{
> -    return 1ULL << (ig + 8);
> -}
> +hwaddr cxl_decode_ig(int ig);
>  
>  CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
>  bool cxl_get_hb_passthrough(PCIHostState *hb);
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 378f1082ce..ea2d4770ec 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -13,6 +13,24 @@
>  #include "hw/pci/pci.h"
>  #include "hw/cxl/cxl.h"
>  
> +int cxl_decoder_count_enc(int count)
> +{
> +    switch (count) {
> +    case 1: return 0;
> +    case 2: return 1;
> +    case 4: return 2;
> +    case 6: return 3;
> +    case 8: return 4;
> +    case 10: return 5;
> +    }
> +    return 0;
> +}
> +
> +hwaddr cxl_decode_ig(int ig)
> +{
> +    return 1ULL << (ig + 8);
> +}
> +
>  static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
>                                         unsigned size)
>  {
> -- 
> 2.39.2
> 
> 

[PATCH v3 2/4] hw/cxl: Add utility functions decoder interleave ways and target count.

[Jonathan Cameron via]

As an encoded version of these key configuration parameters is available
in a register, provide functions to extract it again so as to avoid
the need for duplicating the storage.

Whilst here update the _enc() function to include additional values
as defined in the CXL 3.0 specification. Whilst they are not
currently used in the emulation, they may be in future and it is
easier to compare with the specification if all values are covered.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v3: No changes, picked up tags.
v2: Thanks to Philippe Mathieu-Daudé
 - Expand both enc() and dec() functions to include full set of values
   defined in CXL r3.0
 - Pushed implementation down into the .c file.
---
 include/hw/cxl/cxl_component.h |  2 ++
 hw/cxl/cxl-component-utils.c   | 59 ++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index bdb3881a6b..ef9e033919 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -226,8 +226,10 @@ void cxl_component_create_dvsec(CXLComponentState *cxl_cstate,
                                 uint16_t type, uint8_t rev, uint8_t *body);
 
 int cxl_decoder_count_enc(int count);
+int cxl_decoder_count_dec(int enc_cnt);
 
 uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
+int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp);
 uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
 
 hwaddr cxl_decode_ig(int ig);
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index ea2d4770ec..352d0dace2 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -13,15 +13,45 @@
 #include "hw/pci/pci.h"
 #include "hw/cxl/cxl.h"
 
+/* CXL r3.0 Section 8.2.4.19.1 CXL HDM Decoder Capability Register */
 int cxl_decoder_count_enc(int count)
 {
     switch (count) {
-    case 1: return 0;
-    case 2: return 1;
-    case 4: return 2;
-    case 6: return 3;
-    case 8: return 4;
-    case 10: return 5;
+    case 1: return 0x0;
+    case 2: return 0x1;
+    case 4: return 0x2;
+    case 6: return 0x3;
+    case 8: return 0x4;
+    case 10: return 0x5;
+    /* Switches and Host Bridges may have more than 10 decoders */
+    case 12: return 0x6;
+    case 14: return 0x7;
+    case 16: return 0x8;
+    case 20: return 0x9;
+    case 24: return 0xa;
+    case 28: return 0xb;
+    case 32: return 0xc;
+    }
+    return 0;
+}
+
+int cxl_decoder_count_dec(int enc_cnt)
+{
+    switch (enc_cnt) {
+    case 0x0: return 1;
+    case 0x1: return 2;
+    case 0x2: return 4;
+    case 0x3: return 6;
+    case 0x4: return 8;
+    case 0x5: return 10;
+    /* Switches and Host Bridges may have more than 10 decoders */
+    case 0x6: return 12;
+    case 0x7: return 14;
+    case 0x8: return 16;
+    case 0x9: return 20;
+    case 0xa: return 24;
+    case 0xb: return 28;
+    case 0xc: return 32;
     }
     return 0;
 }
@@ -410,6 +440,23 @@ uint8_t cxl_interleave_ways_enc(int iw, Error **errp)
     }
 }
 
+int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp)
+{
+    switch (iw_enc) {
+    case 0x0: return 1;
+    case 0x1: return 2;
+    case 0x2: return 4;
+    case 0x3: return 8;
+    case 0x4: return 16;
+    case 0x8: return 3;
+    case 0x9: return 6;
+    case 0xa: return 12;
+    default:
+        error_setg(errp, "Encoded interleave ways: %d not supported", iw_enc);
+        return 0;
+    }
+}
+
 uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp)
 {
     switch (gran) {
-- 
2.39.2

Re: [PATCH v3 2/4] hw/cxl: Add utility functions decoder interleave ways and target count.

[Fan Ni]

On Mon, Sep 11, 2023 at 12:43:11PM +0100, Jonathan Cameron wrote:

> As an encoded version of these key configuration parameters is available
> in a register, provide functions to extract it again so as to avoid
> the need for duplicating the storage.
> 
> Whilst here update the _enc() function to include additional values
> as defined in the CXL 3.0 specification. Whilst they are not
> currently used in the emulation, they may be in future and it is
> easier to compare with the specification if all values are covered.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

LGTM. Only one minor comment inline.

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


> v3: No changes, picked up tags.
> v2: Thanks to Philippe Mathieu-Daudé
>  - Expand both enc() and dec() functions to include full set of values
>    defined in CXL r3.0
>  - Pushed implementation down into the .c file.
> ---
>  include/hw/cxl/cxl_component.h |  2 ++
>  hw/cxl/cxl-component-utils.c   | 59 ++++++++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index bdb3881a6b..ef9e033919 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -226,8 +226,10 @@ void cxl_component_create_dvsec(CXLComponentState *cxl_cstate,
>                                  uint16_t type, uint8_t rev, uint8_t *body);
>  
>  int cxl_decoder_count_enc(int count);
> +int cxl_decoder_count_dec(int enc_cnt);
>  
>  uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
> +int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp);
>  uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
>  
>  hwaddr cxl_decode_ig(int ig);
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index ea2d4770ec..352d0dace2 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -13,15 +13,45 @@
>  #include "hw/pci/pci.h"
>  #include "hw/cxl/cxl.h"
>  
> +/* CXL r3.0 Section 8.2.4.19.1 CXL HDM Decoder Capability Register */
>  int cxl_decoder_count_enc(int count)
>  {
>      switch (count) {
> -    case 1: return 0;
> -    case 2: return 1;
> -    case 4: return 2;
> -    case 6: return 3;
> -    case 8: return 4;
> -    case 10: return 5;
> +    case 1: return 0x0;
> +    case 2: return 0x1;
> +    case 4: return 0x2;
> +    case 6: return 0x3;
> +    case 8: return 0x4;
> +    case 10: return 0x5;
> +    /* Switches and Host Bridges may have more than 10 decoders */
> +    case 12: return 0x6;
> +    case 14: return 0x7;
> +    case 16: return 0x8;
> +    case 20: return 0x9;
> +    case 24: return 0xa;
> +    case 28: return 0xb;
> +    case 32: return 0xc;
> +    }
> +    return 0;
> +}
> +
> +int cxl_decoder_count_dec(int enc_cnt)
> +{
> +    switch (enc_cnt) {
> +    case 0x0: return 1;
> +    case 0x1: return 2;
> +    case 0x2: return 4;
> +    case 0x3: return 6;
> +    case 0x4: return 8;
> +    case 0x5: return 10;
> +    /* Switches and Host Bridges may have more than 10 decoders */
> +    case 0x6: return 12;
> +    case 0x7: return 14;
> +    case 0x8: return 16;
> +    case 0x9: return 20;
> +    case 0xa: return 24;
> +    case 0xb: return 28;
> +    case 0xc: return 32;
>      }
>      return 0;
>  }
> @@ -410,6 +440,23 @@ uint8_t cxl_interleave_ways_enc(int iw, Error **errp)
>      }
>  }
>  

Similar as decoder count dec/enc, maybe we want to add a line of comment below.
/* CXL r3.0 Section 8.2.4.19.7 CXL HDM Decoder n Control Register */

Fan
> +int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp)
> +{
> +    switch (iw_enc) {
> +    case 0x0: return 1;
> +    case 0x1: return 2;
> +    case 0x2: return 4;
> +    case 0x3: return 8;
> +    case 0x4: return 16;
> +    case 0x8: return 3;
> +    case 0x9: return 6;
> +    case 0xa: return 12;
> +    default:
> +        error_setg(errp, "Encoded interleave ways: %d not supported", iw_enc);
> +        return 0;
> +    }
> +}
> +
>  uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp)
>  {
>      switch (gran) {
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 2/4] hw/cxl: Add utility functions decoder interleave ways and target count.

[Jonathan Cameron via]

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

> On Mon, Sep 11, 2023 at 12:43:11PM +0100, Jonathan Cameron wrote:
> 
> > As an encoded version of these key configuration parameters is available
> > in a register, provide functions to extract it again so as to avoid
> > the need for duplicating the storage.
> > 
> > Whilst here update the _enc() function to include additional values
> > as defined in the CXL 3.0 specification. Whilst they are not
> > currently used in the emulation, they may be in future and it is
> > easier to compare with the specification if all values are covered.
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---  
> 
> LGTM. Only one minor comment inline.
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
Thanks!

> > +
> > +int cxl_decoder_count_dec(int enc_cnt)
> > +{
> > +    switch (enc_cnt) {
> > +    case 0x0: return 1;
> > +    case 0x1: return 2;
> > +    case 0x2: return 4;
> > +    case 0x3: return 6;
> > +    case 0x4: return 8;
> > +    case 0x5: return 10;
> > +    /* Switches and Host Bridges may have more than 10 decoders */
> > +    case 0x6: return 12;
> > +    case 0x7: return 14;
> > +    case 0x8: return 16;
> > +    case 0x9: return 20;
> > +    case 0xa: return 24;
> > +    case 0xb: return 28;
> > +    case 0xc: return 32;
> >      }
> >      return 0;
> >  }
> > @@ -410,6 +440,23 @@ uint8_t cxl_interleave_ways_enc(int iw, Error **errp)
> >      }
> >  }
> >    
> 
> Similar as decoder count dec/enc, maybe we want to add a line of comment below.
> /* CXL r3.0 Section 8.2.4.19.7 CXL HDM Decoder n Control Register */

I'll do it before cxl_interleave_ways_enc() - one function up in the file
as applies equally well there.
> 
> Fan
> > +int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp)
> > +{
> > +    switch (iw_enc) {
> > +    case 0x0: return 1;
> > +    case 0x1: return 2;
> > +    case 0x2: return 4;
> > +    case 0x3: return 8;
> > +    case 0x4: return 16;
> > +    case 0x8: return 3;
> > +    case 0x9: return 6;
> > +    case 0xa: return 12;
> > +    default:
> > +        error_setg(errp, "Encoded interleave ways: %d not supported", iw_enc);
> > +        return 0;
> > +    }
> > +}
> > +
> >  uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp)
> >  {
> >      switch (gran) {
> > -- 
> > 2.39.2
> > 
> >  

[PATCH v3 3/4] hw/cxl: Fix and use same calculation for HDM decoder block size everywhere

[Jonathan Cameron via]

In order to avoid having the size of the per HDM decoder register block
repeated in lots of places, create the register definitions for HDM
decoder 1 and use the offset between the first registers in HDM decoder 0 and
HDM decoder 1 to establish the offset.

Calculate in each function as this is more obvious and leads to shorter
line lengths than a single #define which would need a long name
to be specific enough.

Note that the code currently only supports one decoder, so the bugs this
fixes don't actually affect anything. Previously the offset didn't
take into account that the write_msk etc are 4 byte fields.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

--
v3:
New patch to separate this out from the addition of HDM decoders.
---
 include/hw/cxl/cxl_component.h |  2 ++
 hw/cxl/cxl-component-utils.c   | 19 +++++++++++--------
 hw/cxl/cxl-host.c              |  4 +++-
 hw/mem/cxl_type3.c             | 24 +++++++++++++++---------
 4 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index ef9e033919..7c864d2044 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -148,6 +148,8 @@ REG32(CXL_HDM_DECODER_GLOBAL_CONTROL, CXL_HDM_REGISTERS_OFFSET + 4)
     FIELD(CXL_HDM_DECODER_GLOBAL_CONTROL, HDM_DECODER_ENABLE, 1, 1)
 
 HDM_DECODER_INIT(0);
+/* Only used for HDM decoder registers block address increment */
+HDM_DECODER_INIT(1);
 
 /* 8.2.5.13 - CXL Extended Security Capability Structure (Root complex only) */
 #define EXTSEC_ENTRY_MAX        256
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 352d0dace2..aa011a8f34 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -210,6 +210,7 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
                             enum reg_type type)
 {
     int decoder_count = 1;
+    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     int i;
 
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, DECODER_COUNT,
@@ -222,19 +223,21 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
                      HDM_DECODER_ENABLE, 0);
     write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
     for (i = 0; i < decoder_count; i++) {
-        write_msk[R_CXL_HDM_DECODER0_BASE_LO + i * 0x20] = 0xf0000000;
-        write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * 0x20] = 0xffffffff;
-        write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf0000000;
-        write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0xffffffff;
-        write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
+        write_msk[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc] = 0xf0000000;
+        write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] = 0xffffffff;
+        write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc] = 0xf0000000;
+        write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] = 0xffffffff;
+        write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] = 0x13ff;
         if (type == CXL2_DEVICE ||
             type == CXL2_TYPE3_DEVICE ||
             type == CXL2_LOGICAL_DEVICE) {
-            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 0xf0000000;
+            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * hdm_inc] =
+                0xf0000000;
         } else {
-            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 0xffffffff;
+            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * hdm_inc] =
+                0xffffffff;
         }
-        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0xffffffff;
+        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * hdm_inc] = 0xffffffff;
     }
 }
 
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index f0920da956..73c5426476 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -101,12 +101,14 @@ void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
 static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
                                 uint8_t *target)
 {
+    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     uint32_t ctrl;
     uint32_t ig_enc;
     uint32_t iw_enc;
     uint32_t target_idx;
+    int i = 0;
 
-    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
+    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
     if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
         return false;
     }
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 4e314748d3..cd92813436 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -377,34 +377,36 @@ static void build_dvsecs(CXLType3Dev *ct3d)
 
 static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
 {
+    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
     uint32_t *cache_mem = cregs->cache_mem_registers;
     uint32_t ctrl;
 
     assert(which == 0);
 
-    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
+    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
     /* TODO: Sanity checks that the decoder is possible */
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
 
-    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
+    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc, ctrl);
 }
 
 static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
 {
+    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
     uint32_t *cache_mem = cregs->cache_mem_registers;
     uint32_t ctrl;
 
     assert(which == 0);
 
-    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
+    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
 
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
 
-    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
+    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc, ctrl);
 }
 
 static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
@@ -761,26 +763,30 @@ static void ct3_exit(PCIDevice *pci_dev)
 /* TODO: Support multiple HDM decoders and DPA skip */
 static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
 {
+    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
     uint64_t decoder_base, decoder_size, hpa_offset;
     uint32_t hdm0_ctrl;
     int ig, iw;
+    int i = 0;
 
-    decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] << 32) |
-                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO]);
+    decoder_base =
+        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
+                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);
     if ((uint64_t)host_addr < decoder_base) {
         return false;
     }
 
     hpa_offset = (uint64_t)host_addr - decoder_base;
 
-    decoder_size = ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI] << 32) |
-        cache_mem[R_CXL_HDM_DECODER0_SIZE_LO];
+    decoder_size =
+        ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] << 32) |
+        cache_mem[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc];
     if (hpa_offset >= decoder_size) {
         return false;
     }
 
-    hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
+    hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
     iw = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IW);
     ig = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IG);
 
-- 
2.39.2

Re: [PATCH v3 3/4] hw/cxl: Fix and use same calculation for HDM decoder block size everywhere

[Fan Ni]

On Mon, Sep 11, 2023 at 12:43:12PM +0100, Jonathan Cameron wrote:

> In order to avoid having the size of the per HDM decoder register block
> repeated in lots of places, create the register definitions for HDM
> decoder 1 and use the offset between the first registers in HDM decoder 0 and
> HDM decoder 1 to establish the offset.
> 
> Calculate in each function as this is more obvious and leads to shorter
> line lengths than a single #define which would need a long name
> to be specific enough.
> 
> Note that the code currently only supports one decoder, so the bugs this
> fixes don't actually affect anything. Previously the offset didn't
> take into account that the write_msk etc are 4 byte fields.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

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

> --
> v3:
> New patch to separate this out from the addition of HDM decoders.
> ---
>  include/hw/cxl/cxl_component.h |  2 ++
>  hw/cxl/cxl-component-utils.c   | 19 +++++++++++--------
>  hw/cxl/cxl-host.c              |  4 +++-
>  hw/mem/cxl_type3.c             | 24 +++++++++++++++---------
>  4 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index ef9e033919..7c864d2044 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -148,6 +148,8 @@ REG32(CXL_HDM_DECODER_GLOBAL_CONTROL, CXL_HDM_REGISTERS_OFFSET + 4)
>      FIELD(CXL_HDM_DECODER_GLOBAL_CONTROL, HDM_DECODER_ENABLE, 1, 1)
>  
>  HDM_DECODER_INIT(0);
> +/* Only used for HDM decoder registers block address increment */
> +HDM_DECODER_INIT(1);
>  
>  /* 8.2.5.13 - CXL Extended Security Capability Structure (Root complex only) */
>  #define EXTSEC_ENTRY_MAX        256
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 352d0dace2..aa011a8f34 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -210,6 +210,7 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>                              enum reg_type type)
>  {
>      int decoder_count = 1;
> +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      int i;
>  
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, DECODER_COUNT,
> @@ -222,19 +223,21 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>                       HDM_DECODER_ENABLE, 0);
>      write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
>      for (i = 0; i < decoder_count; i++) {
> -        write_msk[R_CXL_HDM_DECODER0_BASE_LO + i * 0x20] = 0xf0000000;
> -        write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * 0x20] = 0xffffffff;
> -        write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf0000000;
> -        write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0xffffffff;
> -        write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> +        write_msk[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc] = 0xf0000000;
> +        write_msk[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] = 0xffffffff;
> +        write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc] = 0xf0000000;
> +        write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] = 0xffffffff;
> +        write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] = 0x13ff;
>          if (type == CXL2_DEVICE ||
>              type == CXL2_TYPE3_DEVICE ||
>              type == CXL2_LOGICAL_DEVICE) {
> -            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 0xf0000000;
> +            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * hdm_inc] =
> +                0xf0000000;
>          } else {
> -            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 0xffffffff;
> +            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * hdm_inc] =
> +                0xffffffff;
>          }
> -        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 0xffffffff;
> +        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * hdm_inc] = 0xffffffff;
>      }
>  }
>  
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index f0920da956..73c5426476 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -101,12 +101,14 @@ void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
>  static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
>                                  uint8_t *target)
>  {
> +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      uint32_t ctrl;
>      uint32_t ig_enc;
>      uint32_t iw_enc;
>      uint32_t target_idx;
> +    int i = 0;
>  
> -    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
> +    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
>      if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
>          return false;
>      }
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 4e314748d3..cd92813436 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -377,34 +377,36 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>  
>  static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
>  {
> +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
>      uint32_t *cache_mem = cregs->cache_mem_registers;
>      uint32_t ctrl;
>  
>      assert(which == 0);
>  
> -    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
> +    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
>      /* TODO: Sanity checks that the decoder is possible */
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1);
>  
> -    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
> +    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc, ctrl);
>  }
>  
>  static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
>  {
> +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      ComponentRegisters *cregs = &ct3d->cxl_cstate.crb;
>      uint32_t *cache_mem = cregs->cache_mem_registers;
>      uint32_t ctrl;
>  
>      assert(which == 0);
>  
> -    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL);
> +    ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
>  
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 0);
>  
> -    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL, ctrl);
> +    stl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc, ctrl);
>  }
>  
>  static int ct3d_qmp_uncor_err_to_cxl(CxlUncorErrorType qmp_err)
> @@ -761,26 +763,30 @@ static void ct3_exit(PCIDevice *pci_dev)
>  /* TODO: Support multiple HDM decoders and DPA skip */
>  static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
>  {
> +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
>      uint64_t decoder_base, decoder_size, hpa_offset;
>      uint32_t hdm0_ctrl;
>      int ig, iw;
> +    int i = 0;
>  
> -    decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] << 32) |
> -                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO]);
> +    decoder_base =
> +        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
> +                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);
>      if ((uint64_t)host_addr < decoder_base) {
>          return false;
>      }
>  
>      hpa_offset = (uint64_t)host_addr - decoder_base;
>  
> -    decoder_size = ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI] << 32) |
> -        cache_mem[R_CXL_HDM_DECODER0_SIZE_LO];
> +    decoder_size =
> +        ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] << 32) |
> +        cache_mem[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc];
>      if (hpa_offset >= decoder_size) {
>          return false;
>      }
>  
> -    hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL];
> +    hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
>      iw = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IW);
>      ig = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IG);
>  
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 3/4] hw/cxl: Fix and use same calculation for HDM decoder block size everywhere

[Philippe Mathieu-Daudé]

On 11/9/23 13:43, Jonathan Cameron wrote:
> In order to avoid having the size of the per HDM decoder register block
> repeated in lots of places, create the register definitions for HDM
> decoder 1 and use the offset between the first registers in HDM decoder 0 and
> HDM decoder 1 to establish the offset.
> 
> Calculate in each function as this is more obvious and leads to shorter
> line lengths than a single #define which would need a long name
> to be specific enough.
> 
> Note that the code currently only supports one decoder, so the bugs this
> fixes don't actually affect anything. Previously the offset didn't
> take into account that the write_msk etc are 4 byte fields.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> --
> v3:
> New patch to separate this out from the addition of HDM decoders.
> ---
>   include/hw/cxl/cxl_component.h |  2 ++
>   hw/cxl/cxl-component-utils.c   | 19 +++++++++++--------
>   hw/cxl/cxl-host.c              |  4 +++-
>   hw/mem/cxl_type3.c             | 24 +++++++++++++++---------
>   4 files changed, 31 insertions(+), 18 deletions(-)


> @@ -761,26 +763,30 @@ static void ct3_exit(PCIDevice *pci_dev)
>   /* TODO: Support multiple HDM decoders and DPA skip */
>   static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
>   {
> +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>       uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
>       uint64_t decoder_base, decoder_size, hpa_offset;
>       uint32_t hdm0_ctrl;
>       int ig, iw;
> +    int i = 0;
>   
> -    decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] << 32) |
> -                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO]);
> +    decoder_base =
> +        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
> +                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);

Alternatively easier to review as (matter of taste ?):

decoder_base = deposit64(cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * 
hdm_inc], 32, 32,
                          cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * 
hdm_inc]);

Regardless:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 3/4] hw/cxl: Fix and use same calculation for HDM decoder block size everywhere

[Jonathan Cameron via]

On Wed, 13 Sep 2023 08:53:55 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 11/9/23 13:43, Jonathan Cameron wrote:
> > In order to avoid having the size of the per HDM decoder register block
> > repeated in lots of places, create the register definitions for HDM
> > decoder 1 and use the offset between the first registers in HDM decoder 0 and
> > HDM decoder 1 to establish the offset.
> > 
> > Calculate in each function as this is more obvious and leads to shorter
> > line lengths than a single #define which would need a long name
> > to be specific enough.
> > 
> > Note that the code currently only supports one decoder, so the bugs this
> > fixes don't actually affect anything. Previously the offset didn't
> > take into account that the write_msk etc are 4 byte fields.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > --
> > v3:
> > New patch to separate this out from the addition of HDM decoders.
> > ---
> >   include/hw/cxl/cxl_component.h |  2 ++
> >   hw/cxl/cxl-component-utils.c   | 19 +++++++++++--------
> >   hw/cxl/cxl-host.c              |  4 +++-
> >   hw/mem/cxl_type3.c             | 24 +++++++++++++++---------
> >   4 files changed, 31 insertions(+), 18 deletions(-)  
> 
> 
> > @@ -761,26 +763,30 @@ static void ct3_exit(PCIDevice *pci_dev)
> >   /* TODO: Support multiple HDM decoders and DPA skip */
> >   static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
> >   {
> > +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
> >       uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
> >       uint64_t decoder_base, decoder_size, hpa_offset;
> >       uint32_t hdm0_ctrl;
> >       int ig, iw;
> > +    int i = 0;
> >   
> > -    decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] << 32) |
> > -                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO]);
> > +    decoder_base =
> > +        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
> > +                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);  
> 
> Alternatively easier to review as (matter of taste ?):
> 
> decoder_base = deposit64(cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * 
> hdm_inc], 32, 32,
>                           cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * 
> hdm_inc]);

I'll leave if for now for consistency in the CXL code.  Might make
sense to consider this as a cross subsystem cleanup at some point though!
Thanks for the suggestion.

> 
> Regardless:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Thanks.

Jonathan

> 
> 

[PATCH v3 4/4] hw/cxl: Support 4 HDM decoders at all levels of topology

[Jonathan Cameron via]

Support these decoders in CXL host bridges (pxb-cxl), CXL Switch USP
and CXL Type 3 end points.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v3: Factor out the hdm_inc changes to previous patch.
    Fix use of encoded hdm count as if it were decoded in cxl-host.
    Minor refactoring to make that path and one in cxl_type3.c
    look more similar.
---
 include/hw/cxl/cxl_component.h | 10 +++-
 hw/cxl/cxl-component-utils.c   |  7 ++-
 hw/cxl/cxl-host.c              | 67 ++++++++++++++++--------
 hw/mem/cxl_type3.c             | 96 +++++++++++++++++++++++-----------
 4 files changed, 124 insertions(+), 56 deletions(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 7c864d2044..3c795a6278 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -135,6 +135,10 @@ REG32(CXL_RAS_ERR_HEADER0, CXL_RAS_REGISTERS_OFFSET + 0x18)
   REG32(CXL_HDM_DECODER##n##_TARGET_LIST_LO,                                   \
         CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x24)                          \
   REG32(CXL_HDM_DECODER##n##_TARGET_LIST_HI,                                   \
+        CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x28)                          \
+  REG32(CXL_HDM_DECODER##n##_DPA_SKIP_LO,                                      \
+        CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x24)                          \
+  REG32(CXL_HDM_DECODER##n##_DPA_SKIP_HI,                                      \
         CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x28)
 
 REG32(CXL_HDM_DECODER_CAPABILITY, CXL_HDM_REGISTERS_OFFSET)
@@ -147,9 +151,13 @@ REG32(CXL_HDM_DECODER_GLOBAL_CONTROL, CXL_HDM_REGISTERS_OFFSET + 4)
     FIELD(CXL_HDM_DECODER_GLOBAL_CONTROL, POISON_ON_ERR_EN, 0, 1)
     FIELD(CXL_HDM_DECODER_GLOBAL_CONTROL, HDM_DECODER_ENABLE, 1, 1)
 
+/* Support 4 decoders at all levels of topology */
+#define CXL_HDM_DECODER_COUNT 4
+
 HDM_DECODER_INIT(0);
-/* Only used for HDM decoder registers block address increment */
 HDM_DECODER_INIT(1);
+HDM_DECODER_INIT(2);
+HDM_DECODER_INIT(3);
 
 /* 8.2.5.13 - CXL Extended Security Capability Structure (Root complex only) */
 #define EXTSEC_ENTRY_MAX        256
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index aa011a8f34..3ecdad4a5e 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -90,6 +90,9 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
 
     switch (offset) {
     case A_CXL_HDM_DECODER0_CTRL:
+    case A_CXL_HDM_DECODER1_CTRL:
+    case A_CXL_HDM_DECODER2_CTRL:
+    case A_CXL_HDM_DECODER3_CTRL:
         should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
         should_uncommit = !should_commit;
         break;
@@ -129,7 +132,7 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
     }
 
     if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
-        offset <= A_CXL_HDM_DECODER0_TARGET_LIST_HI) {
+        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
         dumb_hdm_handler(cxl_cstate, offset, value);
     } else {
         cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
@@ -209,7 +212,7 @@ static void ras_init_common(uint32_t *reg_state, uint32_t *write_msk)
 static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
                             enum reg_type type)
 {
-    int decoder_count = 1;
+    int decoder_count = CXL_HDM_DECODER_COUNT;
     int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     int i;
 
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 73c5426476..2aa776c79c 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -97,35 +97,58 @@ void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
     }
 }
 
-/* TODO: support, multiple hdm decoders */
 static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
                                 uint8_t *target)
 {
     int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
-    uint32_t ctrl;
-    uint32_t ig_enc;
-    uint32_t iw_enc;
-    uint32_t target_idx;
-    int i = 0;
-
-    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
-    if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
-        return false;
-    }
-
-    ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG);
-    iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
-    target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
+    unsigned int hdm_count;
+    bool found = false;
+    int i;
+    uint32_t cap;
+
+    cap = ldl_le_p(cache_mem + R_CXL_HDM_DECODER_CAPABILITY);
+    hdm_count = cxl_decoder_count_dec(FIELD_EX32(cap,
+                                                 CXL_HDM_DECODER_CAPABILITY,
+                                                 DECODER_COUNT));
+    for (i = 0; i < hdm_count; i++) {
+        uint32_t ctrl, ig_enc, iw_enc, target_idx;
+        uint32_t low, high;
+        uint64_t base, size;
+
+        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc);
+        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc);
+        base = (low & 0xf0000000) | ((uint64_t)high << 32);
+        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc);
+        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc);
+        size = (low & 0xf0000000) | ((uint64_t)high << 32);
+        if (addr < base || addr >= base + size) {
+            continue;
+        }
 
-    if (target_idx < 4) {
-        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO],
-                            target_idx * 8, 8);
-    } else {
-        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_HI],
-                            (target_idx - 4) * 8, 8);
+        ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + i * hdm_inc);
+        if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
+            return false;
+        }
+        found = true;
+        ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG);
+        iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
+        target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
+
+        if (target_idx < 4) {
+            uint32_t val = ldl_le_p(cache_mem +
+                                    R_CXL_HDM_DECODER0_TARGET_LIST_LO +
+                                    i * hdm_inc);
+            *target = extract32(val, target_idx * 8, 8);
+        } else {
+            uint32_t val = ldl_le_p(cache_mem +
+                                    R_CXL_HDM_DECODER0_TARGET_LIST_HI +
+                                    i * hdm_inc);
+            *target = extract32(val, (target_idx - 4) * 8, 8);
+        }
+        break;
     }
 
-    return true;
+    return found;
 }
 
 static PCIDevice *cxl_cfmws_find_device(CXLFixedWindow *fw, hwaddr addr)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index cd92813436..1658e0cc59 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -382,8 +382,6 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
     uint32_t *cache_mem = cregs->cache_mem_registers;
     uint32_t ctrl;
 
-    assert(which == 0);
-
     ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
     /* TODO: Sanity checks that the decoder is possible */
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
@@ -399,8 +397,6 @@ static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
     uint32_t *cache_mem = cregs->cache_mem_registers;
     uint32_t ctrl;
 
-    assert(which == 0);
-
     ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
 
     ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
@@ -489,6 +485,21 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
         should_uncommit = !should_commit;
         which_hdm = 0;
         break;
+    case A_CXL_HDM_DECODER1_CTRL:
+        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
+        which_hdm = 1;
+        break;
+    case A_CXL_HDM_DECODER2_CTRL:
+        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
+        which_hdm = 2;
+        break;
+    case A_CXL_HDM_DECODER3_CTRL:
+        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
+        should_uncommit = !should_commit;
+        which_hdm = 3;
+        break;
     case A_CXL_RAS_UNC_ERR_STATUS:
     {
         uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
@@ -760,40 +771,63 @@ static void ct3_exit(PCIDevice *pci_dev)
     }
 }
 
-/* TODO: Support multiple HDM decoders and DPA skip */
 static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
 {
     int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
     uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
-    uint64_t decoder_base, decoder_size, hpa_offset;
-    uint32_t hdm0_ctrl;
-    int ig, iw;
-    int i = 0;
-
-    decoder_base =
-        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
-                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);
-    if ((uint64_t)host_addr < decoder_base) {
-        return false;
-    }
-
-    hpa_offset = (uint64_t)host_addr - decoder_base;
-
-    decoder_size =
-        ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] << 32) |
-        cache_mem[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc];
-    if (hpa_offset >= decoder_size) {
-        return false;
-    }
+    unsigned int hdm_count;
+    uint32_t cap;
+    uint64_t dpa_base = 0;
+    int i;
 
-    hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
-    iw = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IW);
-    ig = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IG);
+    cap = ldl_le_p(cache_mem + R_CXL_HDM_DECODER_CAPABILITY);
+    hdm_count = cxl_decoder_count_dec(FIELD_EX32(cap,
+                                                 CXL_HDM_DECODER_CAPABILITY,
+                                                 DECODER_COUNT));
+
+    for (i = 0; i < hdm_count; i++) {
+        uint64_t decoder_base, decoder_size, hpa_offset, skip;
+        uint32_t hdm_ctrl, low, high;
+        int ig, iw;
+
+        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc);
+        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc);
+        decoder_base = ((uint64_t)high << 32) | (low & 0xf0000000);
+
+        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc);
+        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc);
+        decoder_size = ((uint64_t)high << 32) | (low & 0xf0000000);
+
+        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_DPA_SKIP_LO +
+                       i * hdm_inc);
+        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_DPA_SKIP_HI +
+                        i * hdm_inc);
+        skip = ((uint64_t)high << 32) | (low & 0xf0000000);
+        dpa_base += skip;
+
+        hpa_offset = (uint64_t)host_addr - decoder_base;
+
+        hdm_ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + i * hdm_inc);
+        iw = FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, IW);
+        ig = FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, IG);
+        if (!FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
+            return false;
+        }
+        if (((uint64_t)host_addr < decoder_base) ||
+            (hpa_offset >= decoder_size)) {
+            dpa_base += decoder_size /
+                cxl_interleave_ways_dec(iw, &error_fatal);
+            continue;
+        }
 
-    *dpa = (MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
-        ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset) >> iw);
+        *dpa = dpa_base +
+            ((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
+             ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
+              >> iw));
 
-    return true;
+        return true;
+    }
+    return false;
 }
 
 static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
-- 
2.39.2

Re: [PATCH v3 4/4] hw/cxl: Support 4 HDM decoders at all levels of topology

[Fan Ni]

On Mon, Sep 11, 2023 at 12:43:13PM +0100, Jonathan Cameron wrote:

> Support these decoders in CXL host bridges (pxb-cxl), CXL Switch USP
> and CXL Type 3 end points.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---

One comment inline, other than that, looks good to me.


> v3: Factor out the hdm_inc changes to previous patch.
>     Fix use of encoded hdm count as if it were decoded in cxl-host.
>     Minor refactoring to make that path and one in cxl_type3.c
>     look more similar.
> ---
>  include/hw/cxl/cxl_component.h | 10 +++-
>  hw/cxl/cxl-component-utils.c   |  7 ++-
>  hw/cxl/cxl-host.c              | 67 ++++++++++++++++--------
>  hw/mem/cxl_type3.c             | 96 +++++++++++++++++++++++-----------
>  4 files changed, 124 insertions(+), 56 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 7c864d2044..3c795a6278 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -135,6 +135,10 @@ REG32(CXL_RAS_ERR_HEADER0, CXL_RAS_REGISTERS_OFFSET + 0x18)
>    REG32(CXL_HDM_DECODER##n##_TARGET_LIST_LO,                                   \
>          CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x24)                          \
>    REG32(CXL_HDM_DECODER##n##_TARGET_LIST_HI,                                   \
> +        CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x28)                          \
> +  REG32(CXL_HDM_DECODER##n##_DPA_SKIP_LO,                                      \
> +        CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x24)                          \
> +  REG32(CXL_HDM_DECODER##n##_DPA_SKIP_HI,                                      \
>          CXL_HDM_REGISTERS_OFFSET + (0x20 * n) + 0x28)
>  
>  REG32(CXL_HDM_DECODER_CAPABILITY, CXL_HDM_REGISTERS_OFFSET)
> @@ -147,9 +151,13 @@ REG32(CXL_HDM_DECODER_GLOBAL_CONTROL, CXL_HDM_REGISTERS_OFFSET + 4)
>      FIELD(CXL_HDM_DECODER_GLOBAL_CONTROL, POISON_ON_ERR_EN, 0, 1)
>      FIELD(CXL_HDM_DECODER_GLOBAL_CONTROL, HDM_DECODER_ENABLE, 1, 1)
>  
> +/* Support 4 decoders at all levels of topology */
> +#define CXL_HDM_DECODER_COUNT 4
> +
>  HDM_DECODER_INIT(0);
> -/* Only used for HDM decoder registers block address increment */
>  HDM_DECODER_INIT(1);
> +HDM_DECODER_INIT(2);
> +HDM_DECODER_INIT(3);
>  
>  /* 8.2.5.13 - CXL Extended Security Capability Structure (Root complex only) */
>  #define EXTSEC_ENTRY_MAX        256
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index aa011a8f34..3ecdad4a5e 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -90,6 +90,9 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
>  
>      switch (offset) {
>      case A_CXL_HDM_DECODER0_CTRL:
> +    case A_CXL_HDM_DECODER1_CTRL:
> +    case A_CXL_HDM_DECODER2_CTRL:
> +    case A_CXL_HDM_DECODER3_CTRL:
>          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
>          should_uncommit = !should_commit;

So for the commit/uncommit flag, we always check decoder 0 control
register? Or i read it wrong? I thought the commit bit is per control register
thing?

Fan

>          break;
> @@ -129,7 +132,7 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
>      }
>  
>      if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> -        offset <= A_CXL_HDM_DECODER0_TARGET_LIST_HI) {
> +        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
>          dumb_hdm_handler(cxl_cstate, offset, value);
>      } else {
>          cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> @@ -209,7 +212,7 @@ static void ras_init_common(uint32_t *reg_state, uint32_t *write_msk)
>  static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>                              enum reg_type type)
>  {
> -    int decoder_count = 1;
> +    int decoder_count = CXL_HDM_DECODER_COUNT;
>      int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      int i;
>  
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 73c5426476..2aa776c79c 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -97,35 +97,58 @@ void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
>      }
>  }
>  
> -/* TODO: support, multiple hdm decoders */
>  static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
>                                  uint8_t *target)
>  {
>      int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
> -    uint32_t ctrl;
> -    uint32_t ig_enc;
> -    uint32_t iw_enc;
> -    uint32_t target_idx;
> -    int i = 0;
> -
> -    ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
> -    if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> -        return false;
> -    }
> -
> -    ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG);
> -    iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
> -    target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
> +    unsigned int hdm_count;
> +    bool found = false;
> +    int i;
> +    uint32_t cap;
> +
> +    cap = ldl_le_p(cache_mem + R_CXL_HDM_DECODER_CAPABILITY);
> +    hdm_count = cxl_decoder_count_dec(FIELD_EX32(cap,
> +                                                 CXL_HDM_DECODER_CAPABILITY,
> +                                                 DECODER_COUNT));
> +    for (i = 0; i < hdm_count; i++) {
> +        uint32_t ctrl, ig_enc, iw_enc, target_idx;
> +        uint32_t low, high;
> +        uint64_t base, size;
> +
> +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc);
> +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc);
> +        base = (low & 0xf0000000) | ((uint64_t)high << 32);
> +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc);
> +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc);
> +        size = (low & 0xf0000000) | ((uint64_t)high << 32);
> +        if (addr < base || addr >= base + size) {
> +            continue;
> +        }
>  
> -    if (target_idx < 4) {
> -        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_LO],
> -                            target_idx * 8, 8);
> -    } else {
> -        *target = extract32(cache_mem[R_CXL_HDM_DECODER0_TARGET_LIST_HI],
> -                            (target_idx - 4) * 8, 8);
> +        ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + i * hdm_inc);
> +        if (!FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> +            return false;
> +        }
> +        found = true;
> +        ig_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IG);
> +        iw_enc = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW);
> +        target_idx = (addr / cxl_decode_ig(ig_enc)) % (1 << iw_enc);
> +
> +        if (target_idx < 4) {
> +            uint32_t val = ldl_le_p(cache_mem +
> +                                    R_CXL_HDM_DECODER0_TARGET_LIST_LO +
> +                                    i * hdm_inc);
> +            *target = extract32(val, target_idx * 8, 8);
> +        } else {
> +            uint32_t val = ldl_le_p(cache_mem +
> +                                    R_CXL_HDM_DECODER0_TARGET_LIST_HI +
> +                                    i * hdm_inc);
> +            *target = extract32(val, (target_idx - 4) * 8, 8);
> +        }
> +        break;
>      }
>  
> -    return true;
> +    return found;
>  }
>  
>  static PCIDevice *cxl_cfmws_find_device(CXLFixedWindow *fw, hwaddr addr)
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index cd92813436..1658e0cc59 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -382,8 +382,6 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
>      uint32_t *cache_mem = cregs->cache_mem_registers;
>      uint32_t ctrl;
>  
> -    assert(which == 0);
> -
>      ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
>      /* TODO: Sanity checks that the decoder is possible */
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> @@ -399,8 +397,6 @@ static void hdm_decoder_uncommit(CXLType3Dev *ct3d, int which)
>      uint32_t *cache_mem = cregs->cache_mem_registers;
>      uint32_t ctrl;
>  
> -    assert(which == 0);
> -
>      ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc);
>  
>      ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0);
> @@ -489,6 +485,21 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>          should_uncommit = !should_commit;
>          which_hdm = 0;
>          break;
> +    case A_CXL_HDM_DECODER1_CTRL:
> +        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
> +        which_hdm = 1;
> +        break;
> +    case A_CXL_HDM_DECODER2_CTRL:
> +        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
> +        which_hdm = 2;
> +        break;
> +    case A_CXL_HDM_DECODER3_CTRL:
> +        should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> +        should_uncommit = !should_commit;
> +        which_hdm = 3;
> +        break;
>      case A_CXL_RAS_UNC_ERR_STATUS:
>      {
>          uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
> @@ -760,40 +771,63 @@ static void ct3_exit(PCIDevice *pci_dev)
>      }
>  }
>  
> -/* TODO: Support multiple HDM decoders and DPA skip */
>  static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
>  {
>      int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
>      uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
> -    uint64_t decoder_base, decoder_size, hpa_offset;
> -    uint32_t hdm0_ctrl;
> -    int ig, iw;
> -    int i = 0;
> -
> -    decoder_base =
> -        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
> -                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);
> -    if ((uint64_t)host_addr < decoder_base) {
> -        return false;
> -    }
> -
> -    hpa_offset = (uint64_t)host_addr - decoder_base;
> -
> -    decoder_size =
> -        ((uint64_t)cache_mem[R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc] << 32) |
> -        cache_mem[R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc];
> -    if (hpa_offset >= decoder_size) {
> -        return false;
> -    }
> +    unsigned int hdm_count;
> +    uint32_t cap;
> +    uint64_t dpa_base = 0;
> +    int i;
>  
> -    hdm0_ctrl = cache_mem[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc];
> -    iw = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IW);
> -    ig = FIELD_EX32(hdm0_ctrl, CXL_HDM_DECODER0_CTRL, IG);
> +    cap = ldl_le_p(cache_mem + R_CXL_HDM_DECODER_CAPABILITY);
> +    hdm_count = cxl_decoder_count_dec(FIELD_EX32(cap,
> +                                                 CXL_HDM_DECODER_CAPABILITY,
> +                                                 DECODER_COUNT));
> +
> +    for (i = 0; i < hdm_count; i++) {
> +        uint64_t decoder_base, decoder_size, hpa_offset, skip;
> +        uint32_t hdm_ctrl, low, high;
> +        int ig, iw;
> +
> +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc);
> +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc);
> +        decoder_base = ((uint64_t)high << 32) | (low & 0xf0000000);
> +
> +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_LO + i * hdm_inc);
> +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_HI + i * hdm_inc);
> +        decoder_size = ((uint64_t)high << 32) | (low & 0xf0000000);
> +
> +        low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_DPA_SKIP_LO +
> +                       i * hdm_inc);
> +        high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_DPA_SKIP_HI +
> +                        i * hdm_inc);
> +        skip = ((uint64_t)high << 32) | (low & 0xf0000000);
> +        dpa_base += skip;
> +
> +        hpa_offset = (uint64_t)host_addr - decoder_base;
> +
> +        hdm_ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + i * hdm_inc);
> +        iw = FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, IW);
> +        ig = FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, IG);
> +        if (!FIELD_EX32(hdm_ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED)) {
> +            return false;
> +        }
> +        if (((uint64_t)host_addr < decoder_base) ||
> +            (hpa_offset >= decoder_size)) {
> +            dpa_base += decoder_size /
> +                cxl_interleave_ways_dec(iw, &error_fatal);
> +            continue;
> +        }
>  
> -    *dpa = (MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> -        ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset) >> iw);
> +        *dpa = dpa_base +
> +            ((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> +             ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
> +              >> iw));
>  
> -    return true;
> +        return true;
> +    }
> +    return false;
>  }
>  
>  static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> -- 
> 2.39.2
> 
> 

Re: [PATCH v3 4/4] hw/cxl: Support 4 HDM decoders at all levels of topology

[Jonathan Cameron via]

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

> On Mon, Sep 11, 2023 at 12:43:13PM +0100, Jonathan Cameron wrote:
> 
> > Support these decoders in CXL host bridges (pxb-cxl), CXL Switch USP
> > and CXL Type 3 end points.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---  
> 
> One comment inline, other than that, looks good to me.

I think we are fine, but also possible I'm missing something :)

> >  
> >  /* 8.2.5.13 - CXL Extended Security Capability Structure (Root complex only) */
> >  #define EXTSEC_ENTRY_MAX        256
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index aa011a8f34..3ecdad4a5e 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -90,6 +90,9 @@ static void dumb_hdm_handler(CXLComponentState *cxl_cstate, hwaddr offset,
> >  
> >      switch (offset) {
> >      case A_CXL_HDM_DECODER0_CTRL:
> > +    case A_CXL_HDM_DECODER1_CTRL:
> > +    case A_CXL_HDM_DECODER2_CTRL:
> > +    case A_CXL_HDM_DECODER3_CTRL:
> >          should_commit = FIELD_EX32(value, CXL_HDM_DECODER0_CTRL, COMMIT);
> >          should_uncommit = !should_commit;  
> 
> So for the commit/uncommit flag, we always check decoder 0 control
> register? Or i read it wrong? I thought the commit bit is per control register
> thing?

This is in the write handler and the value passed in that we are looking at is
for whichever of the _CTRL registers is being written.

I could have coded this as separate entries for each register as
FIELD_EX32(value, CXL_HDM_DECODER[X]_CTRL, COMMIT)
but as this only figures out the field offset and mask, it is the same for X=0,1,2,3

Jonathan