On Tue, 9 Jun 2026 16:28:33 +0530 Shrihari E S <[email protected]> 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 <[email protected]> > Signed-off-by: Dongjoo Seo <[email protected]> > --- > 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. > + }
