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