Re: [RFC 5/8] hw/cxl: Wire UIO capability into HDM decoder registers

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  9 Jun 2026 16:28:33 +0530
Shrihari E S <shrihari.s@samsung.com> wrote:

> Plumb the 'uio_capable' flag to CXL HDM decoder capability and
> control register interfaces. The UIO bit in the capability register
> is now set for CXL Type3 devices and ports when UIO support is
> advertised via 'x-uio-svc' property.
> 
> Per CXL 4.0 specification Section 8.2.4.20.7, the decoder control
> UIO bit is validated against the advertised capability during
> HDM decoder commit operations.
> 
> Additionally, replace hardcoded HDM decoder control write masks
> with spec-aligned 'WRMASK' definitions.
I think we should be modifying that depending on whether it is uio_capable
rather than saying it is 'writeable' but 'not writetable' if not UIO capable.

A few other comments inline.

> 
> Signed-off-by: Shrihari E S <shrihari.s@samsung.com>
> Signed-off-by: Dongjoo Seo <dongjoo.seo1@samsung.com>
> ---
>  hw/cxl/cxl-component-utils.c        | 29 +++++++++++++++++++++++------
>  hw/mem/cxl_type3.c                  |  9 ++++++++-
>  hw/pci-bridge/cxl_downstream.c      |  2 +-
>  hw/pci-bridge/cxl_root_port.c       |  2 +-
>  hw/pci-bridge/cxl_upstream.c        |  3 ++-
>  hw/pci-bridge/pci_expander_bridge.c |  3 ++-
>  include/hw/cxl/cxl_component.h      |  2 +-
>  include/hw/cxl/cxl_device.h         |  2 ++
>  8 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 31bbedb502..9a5b382272 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -13,6 +13,19 @@
>  #include "hw/pci/pci.h"
>  #include "hw/cxl/cxl.h"
>  
> +/* CXL 4.0 specification 8.4.2.20.7 */
> +#define CXL_HDM_DECODER_CTRL_WRMASK (    \
> +    (0xfu << 0)   | /* IG */             \
> +    (0xfu << 4)   | /* IW */             \
> +    BIT(8)        | /* LOCK_ON_COMMIT */ \
> +    BIT(9)        | /* COMMIT */         \
> +    BIT(12)       | /* TYPE */           \
> +    BIT(13)       | /* BI */             \
> +    BIT(14)       | /* UIO */            \
> +    (0xfu << 16)  | /* UIG */            \
> +    (0xfu << 20)  | /* UIW */            \
> +    (0xfu << 24))   /* ISP */
Given the FIELD() macro defines appropriate masks, can we build this as
	R_CXL_HDM_DECODER_0_CTRL_IG_MASK |
	R_CXL_HDM_DECODER_0_CTRL_IW_MASK |
etc
That will make it both self documenting and ensure it aligns with the field
defines.  If we get a bug at least it will only be in one place :)

> +
>  /* CXL r3.1 Section 8.2.4.20.1 CXL HDM Decoder Capability Register */
>  int cxl_decoder_count_enc(int count)
>  {
> @@ -305,7 +318,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, bool bi)
> +                            enum reg_type type, bool bi, bool uio)
>  {
>      int decoder_count = CXL_HDM_DECODER_COUNT;
>      int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
> @@ -325,9 +338,12 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>          ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 3_6_12_WAY, 0);
>          ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 16_WAY, 0);
>      }
> -    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO, 0);
> +    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO,
> +                     (type == CXL2_TYPE3_DEVICE || CXL2_UPSTREAM_PORT) && uio);
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> -                     UIO_DECODER_COUNT, 0);
> +                     UIO_DECODER_COUNT,
> +                     (type == CXL2_TYPE3_DEVICE || CXL2_UPSTREAM_PORT) && uio ?
> +                     decoder_count : 0);
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, MEMDATA_NXM_CAP, 0);
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
>                       SUPPORTED_COHERENCY_MODEL,
> @@ -341,7 +357,8 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>          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;
> +        write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] =
> +                                             CXL_HDM_DECODER_CTRL_WRMASK;
I think file at least mostly aligns these long line wraps as
        write_msk[R_CXL_HDM_DECODER0_CTRL + i * hdm_inc] =
            CXL_HDM_DECODER_CTRL_WRMASK;

Given i think this needs to be dependent on whether the device is uio_capable
maybe just bring the mask defines suggested above down here, and modify the
UIO bit to fit whether it should be writeable.


>          if (type == CXL2_DEVICE ||
>              type == CXL2_TYPE3_DEVICE ||
>              type == CXL2_LOGICAL_DEVICE) {
> @@ -391,7 +408,7 @@ static void bi_decoder_init_common(uint32_t *reg_state, uint32_t *write_msk,
>  void cxl_component_register_init_common(uint32_t *reg_state,
>                                          uint32_t *write_msk,
>                                          enum reg_type type,
> -                                        bool bi)
> +                                        bool bi, bool uio)
>  {
>      int caps = 0;
>  
> @@ -431,7 +448,7 @@ void cxl_component_register_init_common(uint32_t *reg_state,
>      case CXL2_LOGICAL_DEVICE:
>          /* + HDM */
>          init_cap_reg(HDM, 5, 1);
> -        hdm_init_common(reg_state, write_msk, type, bi);
> +        hdm_init_common(reg_state, write_msk, type, bi, uio);
>          /* fallthrough */
>      case CXL2_DOWNSTREAM_PORT:
>      case CXL2_DEVICE:
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b7ad437cbc..4cdadc3e10 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -590,6 +590,11 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int which)
>      /* 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);
> +    if (ct3d->uio_capable) {
> +        ct3d->uio_enabled = FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, UIO);
> +    } else {
> +        ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, UIO, 0);

If it's not supported, shouldn't the write mask stop it being written?  I think
that would be cleaner than overriding any write to 0 here.


> +    }