Message ID | 20231222090051.3265307-3-42.hyeyoo@gmail.com |
---|---|
State | New, archived |
Headers | show |
Series | A Followup for "QEMU: CXL mailbox rework and features (Part 1)" | expand |
On Fri, 22 Dec 2023 18:00:49 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > In the current mdev_reg_read() implementation, it consistently returns > that the Media Status is Ready (01b). This was fine until commit > 25a52959f99d ("hw/cxl: Add support for device sanitation") because the > media was presumed to be ready. > > However, as per the CXL 3.0 spec "8.2.9.8.5.1 Sanitize (Opcode 4400h)", > during sanitation, the Media State should be set to Disabled (11b). The > mentioned commit correctly sets it to Disabled, but mdev_reg_read() > still returns Media Status as Ready. > > To address this, update mdev_reg_read() to read register values instead > of returning dummy values. > > Fixes: commit 25a52959f99d ("hw/cxl: Add support for device sanitation") > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> I've applied this one to my tree. (I'll push that out in a day or two after tidying up some other outstanding stuff). Sometime in next week or so I'll send out a set bundling together various fixes and cleanup with the intent for getting it applied. Thanks, Jonathan > --- > hw/cxl/cxl-device-utils.c | 17 +++++++++++------ > include/hw/cxl/cxl_device.h | 4 +++- > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c > index 29de298117..ba3f80e6e7 100644 > --- a/hw/cxl/cxl-device-utils.c > +++ b/hw/cxl/cxl-device-utils.c > @@ -229,12 +229,9 @@ static void mailbox_reg_write(void *opaque, hwaddr offset, uint64_t value, > > static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size) > { > - uint64_t retval = 0; > - > - retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1); > - retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1); > + CXLDeviceState *cxl_dstate = opaque; > > - return retval; > + return cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]; > } > > static void ro_reg_write(void *opaque, hwaddr offset, uint64_t value, > @@ -371,7 +368,15 @@ static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate) > cxl_dstate->mbox_msi_n = msi_n; > } > > -static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { } > +static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) > +{ > + uint64_t memdev_status_reg; > + > + memdev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, 1); > + memdev_status_reg = FIELD_DP64(memdev_status_reg, CXL_MEM_DEV_STS, > + MBOX_READY, 1); > + cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = memdev_status_reg; > +} > > void cxl_device_register_init_t3(CXLType3Dev *ct3d) > { > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > index b2cb280e16..b318d94b36 100644 > --- a/include/hw/cxl/cxl_device.h > +++ b/include/hw/cxl/cxl_device.h > @@ -408,7 +408,9 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val) > { > uint64_t dev_status_reg; > > - dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val); > + dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]; > + dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS, > + val); > cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg; > } > #define cxl_dev_disable_media(cxlds) \
On Tue, 9 Jan 2024 17:45:50 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Fri, 22 Dec 2023 18:00:49 +0900 > Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > > In the current mdev_reg_read() implementation, it consistently returns > > that the Media Status is Ready (01b). This was fine until commit > > 25a52959f99d ("hw/cxl: Add support for device sanitation") because the > > media was presumed to be ready. > > > > However, as per the CXL 3.0 spec "8.2.9.8.5.1 Sanitize (Opcode 4400h)", > > during sanitation, the Media State should be set to Disabled (11b). The > > mentioned commit correctly sets it to Disabled, but mdev_reg_read() > > still returns Media Status as Ready. > > > > To address this, update mdev_reg_read() to read register values instead > > of returning dummy values. > > > > Fixes: commit 25a52959f99d ("hw/cxl: Add support for device sanitation") > > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > I've applied this one to my tree. (I'll push that out in a day or two after > tidying up some other outstanding stuff). > > Sometime in next week or so I'll send out a set bundling together various > fixes and cleanup with the intent for getting it applied. I've changed how this works because what this is doing as presented is overwriting the mailbox capability register. mbox_reg_state64 is as the name should have made obvious to reviewers such as me, the mailbox registers! Anyhow, I'll put an alternative fix in place. Jonathan > > Thanks, > > Jonathan > > > --- > > hw/cxl/cxl-device-utils.c | 17 +++++++++++------ > > include/hw/cxl/cxl_device.h | 4 +++- > > 2 files changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c > > index 29de298117..ba3f80e6e7 100644 > > --- a/hw/cxl/cxl-device-utils.c > > +++ b/hw/cxl/cxl-device-utils.c > > @@ -229,12 +229,9 @@ static void mailbox_reg_write(void *opaque, hwaddr offset, uint64_t value, > > > > static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size) > > { > > - uint64_t retval = 0; > > - > > - retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1); > > - retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1); > > + CXLDeviceState *cxl_dstate = opaque; > > > > - return retval; > > + return cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]; > > } > > > > static void ro_reg_write(void *opaque, hwaddr offset, uint64_t value, > > @@ -371,7 +368,15 @@ static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate) > > cxl_dstate->mbox_msi_n = msi_n; > > } > > > > -static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { } > > +static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) > > +{ > > + uint64_t memdev_status_reg; > > + > > + memdev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, 1); > > + memdev_status_reg = FIELD_DP64(memdev_status_reg, CXL_MEM_DEV_STS, > > + MBOX_READY, 1); > > + cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = memdev_status_reg; > > +} > > > > void cxl_device_register_init_t3(CXLType3Dev *ct3d) > > { > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > index b2cb280e16..b318d94b36 100644 > > --- a/include/hw/cxl/cxl_device.h > > +++ b/include/hw/cxl/cxl_device.h > > @@ -408,7 +408,9 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val) > > { > > uint64_t dev_status_reg; > > > > - dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val); > > + dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]; > > + dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS, > > + val); > > cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg; > > } > > #define cxl_dev_disable_media(cxlds) \ > >
diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c index 29de298117..ba3f80e6e7 100644 --- a/hw/cxl/cxl-device-utils.c +++ b/hw/cxl/cxl-device-utils.c @@ -229,12 +229,9 @@ static void mailbox_reg_write(void *opaque, hwaddr offset, uint64_t value, static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size) { - uint64_t retval = 0; - - retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1); - retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1); + CXLDeviceState *cxl_dstate = opaque; - return retval; + return cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]; } static void ro_reg_write(void *opaque, hwaddr offset, uint64_t value, @@ -371,7 +368,15 @@ static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate) cxl_dstate->mbox_msi_n = msi_n; } -static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { } +static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) +{ + uint64_t memdev_status_reg; + + memdev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, 1); + memdev_status_reg = FIELD_DP64(memdev_status_reg, CXL_MEM_DEV_STS, + MBOX_READY, 1); + cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = memdev_status_reg; +} void cxl_device_register_init_t3(CXLType3Dev *ct3d) { diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h index b2cb280e16..b318d94b36 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -408,7 +408,9 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val) { uint64_t dev_status_reg; - dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val); + dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS]; + dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS, + val); cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg; } #define cxl_dev_disable_media(cxlds) \