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

["Zhijian Li (Fujitsu)" via <qemu-devel@nongnu.org>]

On 03/04/2024 11:42, Li Zhijian wrote:
> 
> 
> On 02/04/2024 17:17, Jonathan Cameron wrote:
>> 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,
> 
> Of course, this option is reliable and portable.
> 
> My thought was that build_dvsecs() and the _reset() are static(internal used),
> their callers should have the responsibility to keep the configure space/DVSECS layout consistent.
> So I'm wondering if is it too heavy to have a *new* _find() subroutine for it.
> 
> 
> Another option could be rebuild the all the DVSECs simply after updated the *offset*, just like:
> 


> void reset_devses()
> {
>       cxl->dvsec_offset = OFFSET_TO_FIRST_DVSEC;
>       build_dvsecs();
> }

In this option, i also propose to move 'cxl->dvsec_offset = OFFSET_TO_FIRST_DVSEC' inside build_dvsecs()
so that build_dvsecs() could maintain its offset completely.

+static uint16_t first_dvsec_offset(CXLType3Dev *ct3d)
+{
+    uint16_t offset = PCI_CONFIG_SPACE_SIZE;
+
+    if (ct3d->sn != UI64_NULL) {
+        offset += PCI_EXT_CAP_DSN_SIZEOF;
+    }
+
+    return offset;
+}
+
  static void build_dvsecs(CXLType3Dev *ct3d)
  {
      CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
@@ -284,6 +295,8 @@ static void build_dvsecs(CXLType3Dev *ct3d)
               range2_size_hi = 0, range2_size_lo = 0,
               range2_base_hi = 0, range2_base_lo = 0;
  
+    /* dvsec_offset should point to the first dvsec */
+    cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
      /*
       * Volatile memory is mapped as (0x0)
       * Persistent memory is mapped at (volatile->size)
@@ -664,10 +677,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, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
      }
  
      ct3d->cxl_cstate.pdev = pci_dev;
@@ -899,6 +909,11 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
      return address_space_write(as, dpa_offset, attrs, &data, size);
  }
  
+static void ct3d_dvsecs_reset(CXLType3Dev *ct3d)
+{
+    build_dvsecs(ct3d);
+}
+
  static void ct3d_reset(DeviceState *dev)
  {
      CXLType3Dev *ct3d = CXL_TYPE3(dev);
@@ -907,6 +922,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_dvsecs_reset(ct3d);



> 
> It's reasonable because we ought to ensure *all* the DVSECs being reset in next boot.
> 
> Let me know your thought.
> 
> Thanks
> Zhijian
> 
> 
>> 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.
>>