[PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

[PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

[Li Zhijian via]

It helps to figure out where the first dvsec register is located. In
addition, replace offset and size hardcore with existing macros.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 hw/mem/cxl_type3.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b0a7e9f11b64..ad2fe7d463fb 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
     { }
 };
 
+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 ct3_realize(PCIDevice *pci_dev, Error **errp)
 {
     ERRP_GUARD();
@@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     pci_config_set_prog_interface(pci_conf, 0x10);
 
     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;
-    }
+    if (ct3d->sn != UI64_NULL)
+        pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
 
+    cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
     ct3d->cxl_cstate.pdev = pci_dev;
     build_dvsecs(ct3d);
 
-- 
2.29.2

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

[Li Zhijian via]

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 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.

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,
         .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);
+    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.
-- 
2.29.2

Re: [PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

[fan]

On Tue, Apr 02, 2024 at 09:46:46AM +0800, Li Zhijian via wrote:
> It helps to figure out where the first dvsec register is located. In
> addition, replace offset and size hardcore with existing macros.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  hw/mem/cxl_type3.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b64..ad2fe7d463fb 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
>      { }
>  };
>  
> +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 ct3_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      pci_config_set_prog_interface(pci_conf, 0x10);
>  
>      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;
> -    }
> +    if (ct3d->sn != UI64_NULL)
> +        pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
>  
> +    cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
>      ct3d->cxl_cstate.pdev = pci_dev;
>      build_dvsecs(ct3d);
>  
> -- 
> 2.29.2
> 
> 
Hi Zhijian,

Please use Qemu's checkpatch tool to make sure the patches meet the
qemu code format requirement.
Also, please cc linux-cxl@vger.kernel.org if the code is cxl related.

Fan

Re: [PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

[Zhijian Li (Fujitsu) via]

On 02/04/2024 12:09, fan wrote:
> On Tue, Apr 02, 2024 at 09:46:46AM +0800, Li Zhijian via wrote:
>> It helps to figure out where the first dvsec register is located. In
>> addition, replace offset and size hardcore with existing macros.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   hw/mem/cxl_type3.c | 19 +++++++++++++------
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
>> index b0a7e9f11b64..ad2fe7d463fb 100644
>> --- a/hw/mem/cxl_type3.c
>> +++ b/hw/mem/cxl_type3.c
>> @@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
>>       { }
>>   };
>>   
>> +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 ct3_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>>       ERRP_GUARD();
>> @@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>>       pci_config_set_prog_interface(pci_conf, 0x10);
>>   
>>       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;
>> -    }
>> +    if (ct3d->sn != UI64_NULL)
>> +        pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
>>   
>> +    cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
>>       ct3d->cxl_cstate.pdev = pci_dev;
>>       build_dvsecs(ct3d);
>>   
>> -- 
>> 2.29.2
>>
>>
> Hi Zhijian,
> 
> Please use Qemu's checkpatch tool to make sure the patches meet the
> qemu code format requirement.


My mistake, any other input for these 2 patches?



> Also, please cc linux-cxl@vger.kernel.org if the code is cxl related.

Thanks for your remainder, do you mind if I send a patch to add the
"L: linux-cxl@vger.kernel.org' field to the CXL entry


Thanks
Zhijian
> 

> Fan

Re: [PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

[Jonathan Cameron via]

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

> It helps to figure out where the first dvsec register is located. In
> addition, replace offset and size hardcore with existing macros.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

I agree we should be using the macros.

The offset calc is a bit specific to the the chosen memory layout,
so not sure it makes sense to break it out to a separate function.

I'll suggest alternative possible approaches in review of next patch.

Jonathan

> ---
>  hw/mem/cxl_type3.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b64..ad2fe7d463fb 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
>      { }
>  };
>  
> +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 ct3_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      pci_config_set_prog_interface(pci_conf, 0x10);
>  
>      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;
> -    }
> +    if (ct3d->sn != UI64_NULL)
> +        pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
>  
> +    cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
>      ct3d->cxl_cstate.pdev = pci_dev;
>      build_dvsecs(ct3d);
>  

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

[Jonathan Cameron via]

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.

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

[Zhijian Li (Fujitsu) via]

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();
}

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.
> 

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

[Zhijian Li (Fujitsu) via]

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.
>>