Re: [PATCH 2/2] CXL/cxl_type3: reset DVSEC CXL Control in ct3d_reset

[Jonathan Cameron]

On Tue,  2 Apr 2024 09:46:47 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:

> After the kernel commit
> 0cab68720598 ("cxl/pci: Fix disabling memory if DVSEC CXL Range does not 
> match a CFMWS window")

Fixes tag seems appropriate.

> CXL type3 devices cannot be enabled again after the reboot because this
> flag was not reset.
> 
> This flag could be changed by the firmware or OS, let it have a
> reset(default) value in reboot so that the OS can read its clean status.

Good find.  I think we should aim for a fix that is less fragile to future
code rearrangement etc.

> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  hw/mem/cxl_type3.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index ad2fe7d463fb..3fe136053390 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -305,7 +305,8 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>  
>      dvsec = (uint8_t *)&(CXLDVSECDevice){
>          .cap = 0x1e,
> -        .ctrl = 0x2,
> +#define CT3D_DEVSEC_CXL_CTRL 0x2
> +        .ctrl = CT3D_DEVSEC_CXL_CTRL,
Naming doesn't make it clear the define is a reset value / default value.
>          .status2 = 0x2,
>          .range1_size_hi = range1_size_hi,
>          .range1_size_lo = range1_size_lo,
> @@ -906,6 +907,16 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr 
> host_addr, uint64_t data,
>      return address_space_write(as, dpa_offset, attrs, &data, size);
>  }
>  
> +/* Reset DVSEC CXL Control */
> +static void ct3d_dvsec_cxl_ctrl_reset(CXLType3Dev *ct3d)
> +{
> +    uint16_t offset = first_dvsec_offset(ct3d);

This relies to much on the current memory layout.  We should doing a search
of config space to find the right entry, or we should cache a pointer to
the relevant structure when we fill it in the first time.

> +    CXLDVSECDevice *dvsec;
> +
> +    dvsec = (CXLDVSECDevice *)(ct3d->cxl_cstate.pdev->config + offset);
> +    dvsec->ctrl = CT3D_DEVSEC_CXL_CTRL;
> +}
> +
>  static void ct3d_reset(DeviceState *dev)
>  {
>      CXLType3Dev *ct3d = CXL_TYPE3(dev);
> @@ -914,6 +925,7 @@ static void ct3d_reset(DeviceState *dev)
>  
>      cxl_component_register_init_common(reg_state, write_msk, 
> CXL2_TYPE3_DEVICE);
>      cxl_device_register_init_t3(ct3d);
> +    ct3d_dvsec_cxl_ctrl_reset(ct3d);
>  
>      /*
>       * Bring up an endpoint to target with MCTP over VDM.