| Message ID | 20240402014647.3733839-2-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] CXL/cxl_type3: add first_dvsec_offset() helper | expand |
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.
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. >
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. >>
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.
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(-)