On 02/04/2024 17:17, Jonathan Cameron wrote:
> On Tue, 2 Apr 2024 09:46:47 +0800
> Li Zhijian <lizhij...@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 <lizhij...@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.
>
