Re: [PATCH v2] hw/mem/cxl_type3: reset dvsecs in ct3d_reset()

[Jonathan Cameron]

On Tue,  9 Apr 2024 15:58:46 +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")
> CXL type3 devices cannot be enabled again after the reboot because the
> control register(see 8.1.3.2 in CXL specifiction 2.0 for more details) was
> not reset.
> 
> These registers could be changed by the firmware or OS, let them have
> their initial value in reboot so that the OS can read their clean status.
> 
> Fixes: e1706ea83da0 ("hw/cxl/device: Add a memory device (8.2.8.5)")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Hi,

We need to have a close look at what this is actually doing before
considering applying it.  I don't have time to get that this week, but
hopefully will find some time later this month.

I don't want a partial fix for one particular case that causes
us potential trouble in others.

Jonathan

> ---
> root_port, usp and dsp have the same issue, if this patch get approved,
> I will send another patch to fix them later.
> 
> V2:
>    Add fixes tag.
>    Reset all dvsecs registers instead of CTRL only
> ---
>  hw/mem/cxl_type3.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b64..4f09d0b8fedc 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -30,6 +30,7 @@
>  #include "hw/pci/msix.h"
>  
>  #define DWORD_BYTE 4
> +#define CT3D_CAP_SN_OFFSET PCI_CONFIG_SPACE_SIZE
>  
>  /* Default CDAT entries for a memory region */
>  enum {
> @@ -284,6 +285,10 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>               range2_size_hi = 0, range2_size_lo = 0,
>               range2_base_hi = 0, range2_base_lo = 0;
>  
> +    cxl_cstate->dvsec_offset = CT3D_CAP_SN_OFFSET;
> +    if (ct3d->sn != UI64_NULL) {
> +        cxl_cstate->dvsec_offset += PCI_EXT_CAP_DSN_SIZEOF;
> +    }
>      /*
>       * Volatile memory is mapped as (0x0)
>       * Persistent memory is mapped at (volatile->size)
> @@ -664,10 +669,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>  
>      pcie_endpoint_cap_init(pci_dev, 0x80);
>      if (ct3d->sn != UI64_NULL) {
> -        pcie_dev_ser_num_init(pci_dev, 0x100, ct3d->sn);
> -        cxl_cstate->dvsec_offset = 0x100 + 0x0c;
> -    } else {
> -        cxl_cstate->dvsec_offset = 0x100;
> +        pcie_dev_ser_num_init(pci_dev, CT3D_CAP_SN_OFFSET, ct3d->sn);
>      }
>  
>      ct3d->cxl_cstate.pdev = pci_dev;
> @@ -907,6 +909,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);
> +    build_dvsecs(ct3d);
>  
>      /*
>       * Bring up an endpoint to target with MCTP over VDM.