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

[Zhijian Li (Fujitsu)]

ping



On 11/04/2024 18:18, Jonathan Cameron wrote:
> 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.
>