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

[Zhijian Li (Fujitsu)]

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