[PATCH v1 0/2] A Fixup for "QEMU: CXL mailbox rework and features (Part 1)"

[PATCH v1 0/2] A Fixup for "QEMU: CXL mailbox rework and features (Part 1)"

[Hyeonggon Yoo]

Hi, this is a fixup for the recent patch series "QEMU: CXL mailbox rework and
features (Part 1)" [1].

This fixes two problems:

   1. Media Status in memory device status register not being correctly
      read as "Disabled" while sanitation is in progress.

   2. QEMU assertion failure when it issues an MSI-X interrupt
      (indicating the completion of the sanitize command).

[1] https://lore.kernel.org/linux-cxl/20231023160806.13206-1-Jonathan.Cameron@huawei.com

Hyeonggon Yoo (2):
  hw/cxl/device: read from register values in mdev_reg_read()
  hw/mem/cxl_type3: allocate more vectors for MSI-X

 hw/cxl/cxl-device-utils.c   | 17 +++++++++++------
 hw/mem/cxl_type3.c          |  2 +-
 include/hw/cxl/cxl_device.h |  4 +++-
 3 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.39.1

[PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Hyeonggon Yoo]

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")
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 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 61a3c4dc2e..f1111eb20f 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 befb5f884b..873e6d6ab1 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -353,7 +353,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)                    \
-- 
2.39.1

[PATCH v1 2/2] hw/mem/cxl_type3: allocate more vectors for MSI-X

[Hyeonggon Yoo]

commit 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
completion") enables notifying background command completion via MSI-X
interrupt (vector number 9).

However, the commit uses vector number 9 but the maximum number of
entries is less thus resulting in error below. Fix it by passing
nentries = 10 when calling msix_init_exclusive_bar().

 # echo 1 > sanitize
 Background command 4400h finished: success
 qemu-system-x86_64: ../hw/pci/msix.c:529: msix_notify: Assertion `vector < dev->msix_entries_nr' failed.

Fixes: 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background completion")
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 hw/mem/cxl_type3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 52647b4ac7..72d9371347 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -685,7 +685,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     ComponentRegisters *regs = &cxl_cstate->crb;
     MemoryRegion *mr = &regs->component_registers;
     uint8_t *pci_conf = pci_dev->config;
-    unsigned short msix_num = 6;
+    unsigned short msix_num = 10;
     int i, rc;
 
     QTAILQ_INIT(&ct3d->error_list);
-- 
2.39.1

Re: [PATCH v1 2/2] hw/mem/cxl_type3: allocate more vectors for MSI-X

[Davidlohr Bueso]

On Mon, 27 Nov 2023, Hyeonggon Yoo wrote:

>commit 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
>completion") enables notifying background command completion via MSI-X
>interrupt (vector number 9).
>
>However, the commit uses vector number 9 but the maximum number of
>entries is less thus resulting in error below. Fix it by passing
>nentries = 10 when calling msix_init_exclusive_bar().

Hmm yeah this was already set to 10 in Jonathan's tree, thanks for reporting.

Re: [PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Davidlohr Bueso]

On Mon, 27 Nov 2023, Hyeonggon Yoo 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")
>Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Looks good, thanks.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

In addition how about the following to further robustify?
   - disallow certain incoming cci cmd when media is disabled
   - deal with memory reads/writes when media is disabled
   - make __toggle_media() a nop when passed value is already set
   - play nice with arm64 uses little endian reads and writes (this
     should be extended to all of mbox/cci of course).

----8<-----------------------------
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 6eff56fb1b34..9bc5121215c9 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1314,6 +1314,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
      int ret;
      const struct cxl_cmd *cxl_cmd;
      opcode_handler h;
+    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
  
      *len_out = 0;
      cxl_cmd = &cci->cxl_cmd_set[set][cmd];
@@ -1334,8 +1335,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
          return CXL_MBOX_BUSY;
      }
  
-    /* forbid any selected commands while overwriting */
-    if (sanitize_running(cci)) {
+    /* forbid any selected commands when necessary */
+    if (sanitize_running(cci) || cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
          if (h == cmd_events_get_records ||
              h == cmd_ccls_get_partition_info ||
              h == cmd_ccls_set_lsa ||
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 72d93713473d..e0a164fde007 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -899,7 +899,8 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
          return MEMTX_ERROR;
      }
  
-    if (sanitize_running(&ct3d->cci)) {
+    if (sanitize_running(&ct3d->cci) ||
+        cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
          qemu_guest_getrandom_nofail(data, size);
          return MEMTX_OK;
      }
@@ -925,6 +926,11 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
          return MEMTX_OK;
      }
  
+    /* memory writes to the device will have no effect */
+    if (cxl_dev_media_disabled(&ct3d->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 873e6d6ab159..007d4169df7c 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -349,14 +349,26 @@ 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 bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
+{
+    uint64_t dev_status_reg;
+
+    dev_status_reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
+    return FIELD_EX64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == 0x3;
+}
+
  static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
  {
      uint64_t dev_status_reg;
  
-    dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];
+    dev_status_reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
+    if (FIELD_EX64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
+        return;
+    }
+
      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;
+    stq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, dev_status_reg);
  }
  #define cxl_dev_disable_media(cxlds)                    \
          do { __toggle_media((cxlds), 0x3); } while (0)

Re: [PATCH v1 2/2] hw/mem/cxl_type3: allocate more vectors for MSI-X

[Hyeonggon Yoo]

On Tue, Nov 28, 2023 at 2:53 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> On Mon, 27 Nov 2023, Hyeonggon Yoo wrote:
>
> >commit 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
> >completion") enables notifying background command completion via MSI-X
> >interrupt (vector number 9).
> >
> >However, the commit uses vector number 9 but the maximum number of
> >entries is less thus resulting in error below. Fix it by passing
> >nentries = 10 when calling msix_init_exclusive_bar().
>
> Hmm yeah this was already set to 10 in Jonathan's tree, thanks for reporting.

Oh, yeah, it's based on the mainline tree. I should have checked Jonathan's.

hmm it's already 10 there but vector number 9 is already being used by PCIe DOE.
So I think it should change msix_num = 11 and use vector number 10 for
background command completion interrupt instead?

https://gitlab.com/jic23/qemu/-/commit/2823f19188664a6d48a965ea8170c9efa23cddab

Thanks!

--
Hyeonggon

Re: [PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Hyeonggon Yoo]

On Tue, Nov 28, 2023 at 5:27 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> On Mon, 27 Nov 2023, Hyeonggon Yoo 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")
> >Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
>
> Looks good, thanks.
>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>
> In addition how about the following to further robustify?
>    - disallow certain incoming cci cmd when media is disabled
>    - deal with memory reads/writes when media is disabled
>    - make __toggle_media() a nop when passed value is already set
>    - play nice with arm64 uses little endian reads and writes (this
>      should be extended to all of mbox/cci of course).

All of them make sense to me. I will adjust, thanks!

But I'm not confident enough to write a single description for all the
changes so will
split it into a few patches. May I add your Suggested-by (or
Signed-off-by) in v2
as it will contain some part of your idea and code?

> ----8<-----------------------------
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 6eff56fb1b34..9bc5121215c9 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1314,6 +1314,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
>       int ret;
>       const struct cxl_cmd *cxl_cmd;
>       opcode_handler h;
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
>
>       *len_out = 0;
>       cxl_cmd = &cci->cxl_cmd_set[set][cmd];
> @@ -1334,8 +1335,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd,
>           return CXL_MBOX_BUSY;
>       }
>
> -    /* forbid any selected commands while overwriting */
> -    if (sanitize_running(cci)) {
> +    /* forbid any selected commands when necessary */
> +    if (sanitize_running(cci) || cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
>           if (h == cmd_events_get_records ||
>               h == cmd_ccls_get_partition_info ||
>               h == cmd_ccls_set_lsa ||
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 72d93713473d..e0a164fde007 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -899,7 +899,8 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
>           return MEMTX_ERROR;
>       }
>
> -    if (sanitize_running(&ct3d->cci)) {
> +    if (sanitize_running(&ct3d->cci) ||
> +        cxl_dev_media_disabled(&ct3d->cxl_dstate)) {
>           qemu_guest_getrandom_nofail(data, size);
>           return MEMTX_OK;
>       }
> @@ -925,6 +926,11 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
>           return MEMTX_OK;
>       }
>
> +    /* memory writes to the device will have no effect */
> +    if (cxl_dev_media_disabled(&ct3d->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 873e6d6ab159..007d4169df7c 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -349,14 +349,26 @@ 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 bool cxl_dev_media_disabled(CXLDeviceState *cxl_dstate)
> +{
> +    uint64_t dev_status_reg;
> +
> +    dev_status_reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
> +    return FIELD_EX64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == 0x3;
> +}
> +
>   static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
>   {
>       uint64_t dev_status_reg;
>
> -    dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];
> +    dev_status_reg = ldq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS);
> +    if (FIELD_EX64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS) == val) {
> +        return;
> +    }
> +
>       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;
> +    stq_le_p(cxl_dstate->mbox_reg_state64 + R_CXL_MEM_DEV_STS, dev_status_reg);
>   }
>   #define cxl_dev_disable_media(cxlds)                    \
>           do { __toggle_media((cxlds), 0x3); } while (0)

Re: [PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Davidlohr Bueso]

On Tue, 28 Nov 2023, Hyeonggon Yoo wrote:

>All of them make sense to me. I will adjust, thanks!
>
>But I'm not confident enough to write a single description for all the
>changes so will
>split it into a few patches. May I add your Suggested-by (or
>Signed-off-by) in v2
>as it will contain some part of your idea and code?

Sure, feel free to split as you see fit.

Re: [PATCH v1 0/2] A Fixup for "QEMU: CXL mailbox rework and features (Part 1)"

[Michael S. Tsirkin]

On Mon, Nov 27, 2023 at 07:58:28PM +0900, Hyeonggon Yoo wrote:
> Hi, this is a fixup for the recent patch series "QEMU: CXL mailbox rework and
> features (Part 1)" [1].


To clarify do you plan v2 of this?

> This fixes two problems:
> 
>    1. Media Status in memory device status register not being correctly
>       read as "Disabled" while sanitation is in progress.
> 
>    2. QEMU assertion failure when it issues an MSI-X interrupt
>       (indicating the completion of the sanitize command).
> 
> [1] https://lore.kernel.org/linux-cxl/20231023160806.13206-1-Jonathan.Cameron@huawei.com
> 
> Hyeonggon Yoo (2):
>   hw/cxl/device: read from register values in mdev_reg_read()
>   hw/mem/cxl_type3: allocate more vectors for MSI-X
> 
>  hw/cxl/cxl-device-utils.c   | 17 +++++++++++------
>  hw/mem/cxl_type3.c          |  2 +-
>  include/hw/cxl/cxl_device.h |  4 +++-
>  3 files changed, 15 insertions(+), 8 deletions(-)
> 
> -- 
> 2.39.1

Re: [PATCH v1 2/2] hw/mem/cxl_type3: allocate more vectors for MSI-X

[Jonathan Cameron via]

On Tue, 28 Nov 2023 09:27:28 +0900
Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:

> On Tue, Nov 28, 2023 at 2:53 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> > On Mon, 27 Nov 2023, Hyeonggon Yoo wrote:
> >  
> > >commit 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
> > >completion") enables notifying background command completion via MSI-X
> > >interrupt (vector number 9).
> > >
> > >However, the commit uses vector number 9 but the maximum number of
> > >entries is less thus resulting in error below. Fix it by passing
> > >nentries = 10 when calling msix_init_exclusive_bar().  
> >
> > Hmm yeah this was already set to 10 in Jonathan's tree, thanks for reporting.  
> 
> Oh, yeah, it's based on the mainline tree. I should have checked Jonathan's.
> 
> hmm it's already 10 there but vector number 9 is already being used by PCIe DOE.
> So I think it should change msix_num = 11 and use vector number 10 for
> background command completion interrupt instead?
> 
> https://gitlab.com/jic23/qemu/-/commit/2823f19188664a6d48a965ea8170c9efa23cddab

Whilst I clearly messed up a rebase as this wasn't intended, it should be fine
to have multiple things sharing a vector.

On my todo list is making the case of too few vectors being available work for
all the cases in which case everything may end up on one vector.
So we do need to expand the vectors to cover what we are asking for, but
moving this to 11 is a nice to have rather than required.

Jonathan

> 
> Thanks!
> 
> --
> Hyeonggon

Re: [PATCH v1 0/2] A Fixup for "QEMU: CXL mailbox rework and features (Part 1)"

[Hyeonggon Yoo]

On Tue, Nov 28, 2023 at 4:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Nov 27, 2023 at 07:58:28PM +0900, Hyeonggon Yoo wrote:
> > Hi, this is a fixup for the recent patch series "QEMU: CXL mailbox rework and
> > features (Part 1)" [1].
>
>
> To clarify do you plan v2 of this?

Yes, I will send v2. Thanks for asking!

> > This fixes two problems:
> >
> >    1. Media Status in memory device status register not being correctly
> >       read as "Disabled" while sanitation is in progress.
> >
> >    2. QEMU assertion failure when it issues an MSI-X interrupt
> >       (indicating the completion of the sanitize command).
> >
> > [1] https://lore.kernel.org/linux-cxl/20231023160806.13206-1-Jonathan.Cameron@huawei.com
> >
> > Hyeonggon Yoo (2):
> >   hw/cxl/device: read from register values in mdev_reg_read()
> >   hw/mem/cxl_type3: allocate more vectors for MSI-X
> >
> >  hw/cxl/cxl-device-utils.c   | 17 +++++++++++------
> >  hw/mem/cxl_type3.c          |  2 +-
> >  include/hw/cxl/cxl_device.h |  4 +++-
> >  3 files changed, 15 insertions(+), 8 deletions(-)
> >
> > --
> > 2.39.1
>

Re: [PATCH v1 2/2] hw/mem/cxl_type3: allocate more vectors for MSI-X

[Hyeonggon Yoo]

On Tue, Nov 28, 2023 at 9:46 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 28 Nov 2023 09:27:28 +0900
> Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>
> > On Tue, Nov 28, 2023 at 2:53 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
> > >
> > > On Mon, 27 Nov 2023, Hyeonggon Yoo wrote:
> > >
> > > >commit 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
> > > >completion") enables notifying background command completion via MSI-X
> > > >interrupt (vector number 9).
> > > >
> > > >However, the commit uses vector number 9 but the maximum number of
> > > >entries is less thus resulting in error below. Fix it by passing
> > > >nentries = 10 when calling msix_init_exclusive_bar().
> > >
> > > Hmm yeah this was already set to 10 in Jonathan's tree, thanks for reporting.
> >
> > Oh, yeah, it's based on the mainline tree. I should have checked Jonathan's.
> >
> > hmm it's already 10 there but vector number 9 is already being used by PCIe DOE.
> > So I think it should change msix_num = 11 and use vector number 10 for
> > background command completion interrupt instead?
> >
> > https://gitlab.com/jic23/qemu/-/commit/2823f19188664a6d48a965ea8170c9efa23cddab
>
> Whilst I clearly messed up a rebase as this wasn't intended, it should be fine
> to have multiple things sharing a vector.

I wasn't 100% sure of that, thanks for confirming.
I'll drop this patch in v2 (assuming it's going to be fixed in the
next PR of your tree)

For my own learning, I would like to give a few questions
(sorry to bother, no pressures):

> On my todo list is making the case of too few vectors being available work for
> all the cases in which case everything may end up on one vector.

You mean the device needs to support sharing
a vector for different features?

> So we do need to expand the vectors to cover what we are asking for, but
> moving this to 11 is a nice to have rather than required.

May I ask what do you mean by "expand the vectors" to cover what we
are asking for?

Thanks!
--
Hyeonggon

Re: [PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Jonathan Cameron via]

On Mon, 27 Nov 2023 12:27:02 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Mon, 27 Nov 2023, Hyeonggon Yoo 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")
> >Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>  
> 
> Looks good, thanks.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> 
> In addition how about the following to further robustify?
>    - disallow certain incoming cci cmd when media is disabled
>    - deal with memory reads/writes when media is disabled
>    - make __toggle_media() a nop when passed value is already set
>    - play nice with arm64 uses little endian reads and writes (this
>      should be extended to all of mbox/cci of course).
This one you've lost me on.  Arm64 and x86 both little endian.

If you mean generally harden the code we haven't fixed up for
big endian systems then fair enough - that indeed needs doing.
Tricky to be sure we got it all right though unless we have a big
endian arch to test on...

Jonathan

Re: [PATCH v1 1/2] hw/cxl/device: read from register values in mdev_reg_read()

[Davidlohr Bueso]

On Thu, 30 Nov 2023, Jonathan Cameron wrote:

>If you mean generally harden the code we haven't fixed up for
>big endian systems then fair enough - that indeed needs doing.

Yes, that was a braino, sorry for the confusion.

>Tricky to be sure we got it all right though unless we have a big
>endian arch to test on...

Indeed.