Re: [PATCH 3/5] cxl/device: Handle Media Disabled state

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 15 May 2023 09:41:29 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Mon, 15 May 2023, Jonathan Cameron wrote:
> 
> >On Tue, 18 Apr 2023 10:23:35 -0700
> >Davidlohr Bueso <dave@stgolabs.net> wrote:
> >  
> >> Ensure that both memory device read/writes and mailbox commands
> >> can deal with the device media being disabled - per CXL 3.0 specs
> >> 8.2.9.8.5.1 - Sanitize. Disabled semantics here are strictly 11b,
> >> however noting that if the media is not in the ready state (01b),
> >> user data is not accessible.
> >>
> >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> >> ---
> >>  hw/cxl/cxl-mailbox-utils.c  | 15 +++++++++++++++
> >>  hw/mem/cxl_type3.c          | 10 ++++++++++
> >>  include/hw/cxl/cxl_device.h | 33 +++++++++++++++++++++++++++++++++
> >>  3 files changed, 58 insertions(+)
> >>
> >> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> >> index 224bfdb4bfca..e0e20bc3a2bb 100644
> >> --- a/hw/cxl/cxl-mailbox-utils.c
> >> +++ b/hw/cxl/cxl-mailbox-utils.c
> >> @@ -891,6 +891,21 @@ void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> >>          if (len == cxl_cmd->in || cxl_cmd->in == ~0) {
> >>              cxl_cmd->payload = cxl_dstate->mbox_reg_state +
> >>                  A_CXL_DEV_CMD_PAYLOAD;
> >> +
> >> +            if (cxl_dev_media_disabled(cxl_dstate)) {
> >> +                if (h == cmd_events_get_records ||
> >> +                    h == cmd_logs_get_log ||
> >> +                    h == cmd_ccls_get_partition_info ||
> >> +                    h == cmd_ccls_get_lsa ||
> >> +                    h == cmd_ccls_set_lsa ||
> >> +                    h == cmd_media_get_poison_list ||
> >> +                    h == cmd_media_inject_poison ||
> >> +                    h == cmd_media_clear_poison) {
> >> +                        ret = CXL_MBOX_MEDIA_DISABLED;
> >> +                        goto done;
> >> +                }
> >> +            }
> >> +
> >>              /* Only one bg command at a time */
> >>              if ((cxl_cmd->effect & BACKGROUND_OPERATION) &&
> >>                  cxl_dstate->bg.runtime > 0) {
> >> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> >> index 801f72c5fcae..707bdc263f03 100644
> >> --- a/hw/mem/cxl_type3.c
> >> +++ b/hw/mem/cxl_type3.c
> >> @@ -12,6 +12,7 @@
> >>  #include "qemu/pmem.h"
> >>  #include "qemu/range.h"
> >>  #include "qemu/rcu.h"
> >> +#include "qemu/guest-random.h"
> >>  #include "sysemu/hostmem.h"
> >>  #include "sysemu/numa.h"
> >>  #include "hw/cxl/cxl.h"
> >> @@ -989,6 +990,11 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> >>      if (res) {
> >>          return MEMTX_ERROR;
> >>      }
> >> +    /* all memory reads will return random values */  
> >
> >Reference for this would be good.  Random is expensive if we can get
> >away with a constant value.  
> 
> Oh I threw out any performance expectations for qemu out the window, otherwise
> I'd cry. Unless it's problem (this is media disabled, so bleh), I would
> prefer keeping it as is.

Fair enough though marked data (the old DEADBEEF or similar) might be more
useful for verifying it worked as expected.

> 
> >> +    if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
> >> +        qemu_guest_getrandom_nofail(data, size);
> >> +        return MEMTX_OK;
> >> +    }
> >>
> >>      return address_space_read(as, dpa_offset, attrs, data, size);
> >>  }
> >> @@ -1005,6 +1011,10 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> >>      if (res) {
> >>          return MEMTX_ERROR;
> >>      }
> >> +    /* memory writes to the device will have no effect */
> >> +    if (cxl_dev_media_disabled(&CXL_TYPE3(d)->cxl_dstate)) {
> >> +        return MEMTX_OK;
> >> +    }
> >>
> >>      return address_space_write(as, dpa_offset, attrs, &data, size);
> >>  }
> >> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> >> index f986651b6ead..3ef7a7cded95 100644
> >> --- a/include/hw/cxl/cxl_device.h
> >> +++ b/include/hw/cxl/cxl_device.h
> >> @@ -346,6 +346,39 @@ REG64(CXL_MEM_DEV_STS, 0)
> >>      FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
> >>      FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
> >>
> >> +static inline void cxl_dev_media_toggle(CXLDeviceState *cxl_dstate, int val)
> >> +{
> >> +    uint64_t reg;
> >> +
> >> +    reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
> >> +    if (FIELD_EX64(reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
> >> +        return;
> >> +    }
> >> +
> >> +    reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
> >> +    reg = FIELD_DP64(reg, CXL_MEM_DEV_STS, MBOX_READY, 0x1);
> >> +
> >> +    stq_le_p(&cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, reg);  
> >
> >Why & ?  
> 
> braino
> 
> Thanks,
> Davidlohr