[PATCH 0/2] QEMU DCD emulation support fix

[PATCH 0/2] QEMU DCD emulation support fix

[nifan.cxl]

From: Fan Ni <fan.ni@samsung.com>

In Ira's latest DCD kernel patchset[1], the More flag support has been
added. While testing it with mainstream Qemu, I identified two issues
with Qemu.

1. For a DC extent add request with more than one extents, the More flag
for the last one is not correctly set.
2. The function cxl_event_insert should only return true for the last event
record in a sequence grouped via More flag so interrupt will only be
triggered once for the sequence not every record.

After the fix of the first issue, the More flag works as expected.
While the test can pass without the second patch which fixes the second issue,
but I think we make sense to notify the OS only when all the records
are put in the event log.

[1] Last DCD kernel patchset: https://lore.kernel.org/linux-cxl/20240816-dcd-type2-upstream-v3-0-7c9b96cba6d7@intel.com/T/#t

Fan Ni (2):
  hw/mem/cxl_type3: Fix More flag setting for dynamic capacity event
    records
  hw/cxl/cxl_event: Fix interrupt triggering for dynamic capacity events
    grouped via More flag

 hw/cxl/cxl-events.c         | 8 ++++++++
 hw/mem/cxl_type3.c          | 2 +-
 include/hw/cxl/cxl_events.h | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/2] hw/mem/cxl_type3: Fix More flag setting for dynamic capacity event records

[nifan.cxl]

From: Fan Ni <fan.ni@samsung.com>

Per cxl spec r3.1, for multiple dynamic capacity event records grouped via
the More flag, the last record in the sequence should clear the More flag.

Before the change, the More flag of the event record is cleared before
the loop of inserting records into the event log, which will leave the flag
always set once it is set in the loop.

Signed-off-by: Fan Ni <fan.ni@samsung.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 d648192ab9..e616801c81 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -2060,11 +2060,11 @@ static void qmp_cxl_process_dynamic_capacity_prescriptive(const char *path,
     stw_le_p(&dCap.host_id, hid);
     /* only valid for DC_REGION_CONFIG_UPDATED event */
     dCap.updated_region_id = 0;
-    dCap.flags = 0;
     for (i = 0; i < num_extents; i++) {
         memcpy(&dCap.dynamic_capacity_extent, &extents[i],
                sizeof(CXLDCExtentRaw));
 
+        dCap.flags = 0;
         if (i < num_extents - 1) {
             /* Set "More" flag */
             dCap.flags |= BIT(0);
-- 
2.43.0

Re: [PATCH 1/2] hw/mem/cxl_type3: Fix More flag setting for dynamic capacity event records

[Jonathan Cameron via]

On Tue, 27 Aug 2024 09:40:04 -0700
nifan.cxl@gmail.com wrote:

> From: Fan Ni <fan.ni@samsung.com>
> 
> Per cxl spec r3.1, for multiple dynamic capacity event records grouped via
> the More flag, the last record in the sequence should clear the More flag.
> 
> Before the change, the More flag of the event record is cleared before
> the loop of inserting records into the event log, which will leave the flag
> always set once it is set in the loop.
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Oops.  I'll queue this up, though not sure I'll get a fixes series
out this week so it might only hit after the QEMU release.

Jonathan

> ---
>  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 d648192ab9..e616801c81 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -2060,11 +2060,11 @@ static void qmp_cxl_process_dynamic_capacity_prescriptive(const char *path,
>      stw_le_p(&dCap.host_id, hid);
>      /* only valid for DC_REGION_CONFIG_UPDATED event */
>      dCap.updated_region_id = 0;
> -    dCap.flags = 0;
>      for (i = 0; i < num_extents; i++) {
>          memcpy(&dCap.dynamic_capacity_extent, &extents[i],
>                 sizeof(CXLDCExtentRaw));
>  
> +        dCap.flags = 0;
>          if (i < num_extents - 1) {
>              /* Set "More" flag */
>              dCap.flags |= BIT(0);

[PATCH 2/2] hw/cxl/cxl_event: Fix interrupt triggering for dynamic capacity events grouped via More flag

[nifan.cxl]

From: Fan Ni <fan.ni@samsung.com>

When inserting multiple dynamic capacity event records grouped via More flag,
we should only trigger interrupt after the last record is inserted into the
event log. Achieving the goal by letting cxl_event_insert return true only
for the insertion of the last dynamic capacity event record in the sequence.

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 hw/cxl/cxl-events.c         | 8 ++++++++
 include/hw/cxl/cxl_events.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index 12dee2e467..90536c0e68 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -135,6 +135,14 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
     QSIMPLEQ_INSERT_TAIL(&log->events, entry, node);
     cxl_event_set_status(cxlds, log_type, true);
 
+    /*
+     * For dynamic capacity event records grouped via More flag,
+     * Only raise interrupt after inserting the last record in the log.
+     */
+    if (log_type == CXL_EVENT_TYPE_DYNAMIC_CAP) {
+        CXLEventDynamicCapacity *dCap = (CXLEventDynamicCapacity *)event;
+        return (dCap->flags & MORE_FLAG) ? false : true;
+    }
     /* Count went from 0 to 1 */
     return cxl_event_count(log) == 1;
 }
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 38cadaa0f3..b0e5cc89c0 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -170,6 +170,7 @@ typedef struct CXLEventMemoryModule {
  * CXL r3.1 section Table 8-50: Dynamic Capacity Event Record
  * All fields little endian.
  */
+#define MORE_FLAG BIT_MASK(0)
 typedef struct CXLEventDynamicCapacity {
     CXLEventRecordHdr hdr;
     uint8_t type;
-- 
2.43.0

Re: [PATCH 2/2] hw/cxl/cxl_event: Fix interrupt triggering for dynamic capacity events grouped via More flag

[Jonathan Cameron via]

On Tue, 27 Aug 2024 09:40:05 -0700
nifan.cxl@gmail.com wrote:

> From: Fan Ni <fan.ni@samsung.com>
> 
> When inserting multiple dynamic capacity event records grouped via More flag,
> we should only trigger interrupt after the last record is inserted into the
> event log. Achieving the goal by letting cxl_event_insert return true only
> for the insertion of the last dynamic capacity event record in the sequence.

I'm not sure this one is accurate.  We might well have a slow
system provisioning capacity one extent at time (and interrupting).

The event buffer might also not be large enough to hold all records so
the device might 'wait' before figuring out the next extent for there
to be somewhere to put the record.

Overall I think we can interrupt on each one and it should 'work'
as should interrupt only once there are lots of them or
every (n).

Interrupt only fires on a 0 to >= 1 transition anyway, not
on repeats after that unless the log has been cleared.
It's up to OS to keep clearing records until it at least
momentarily hits 0 if it wants to get any more interrupts.

Jonathan


> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
> ---
>  hw/cxl/cxl-events.c         | 8 ++++++++
>  include/hw/cxl/cxl_events.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index 12dee2e467..90536c0e68 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -135,6 +135,14 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
>      QSIMPLEQ_INSERT_TAIL(&log->events, entry, node);
>      cxl_event_set_status(cxlds, log_type, true);
>  
> +    /*
> +     * For dynamic capacity event records grouped via More flag,
> +     * Only raise interrupt after inserting the last record in the log.
> +     */
> +    if (log_type == CXL_EVENT_TYPE_DYNAMIC_CAP) {
> +        CXLEventDynamicCapacity *dCap = (CXLEventDynamicCapacity *)event;
> +        return (dCap->flags & MORE_FLAG) ? false : true;
> +    }
>      /* Count went from 0 to 1 */
>      return cxl_event_count(log) == 1;

If there are multiple this will fail I think as cxl_event_count(log) will go from 0
to X not 1.

>  }
> diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> index 38cadaa0f3..b0e5cc89c0 100644
> --- a/include/hw/cxl/cxl_events.h
> +++ b/include/hw/cxl/cxl_events.h
> @@ -170,6 +170,7 @@ typedef struct CXLEventMemoryModule {
>   * CXL r3.1 section Table 8-50: Dynamic Capacity Event Record
>   * All fields little endian.
>   */
> +#define MORE_FLAG BIT_MASK(0)
>  typedef struct CXLEventDynamicCapacity {
>      CXLEventRecordHdr hdr;
>      uint8_t type;

Re: [PATCH 2/2] hw/cxl/cxl_event: Fix interrupt triggering for dynamic capacity events grouped via More flag

[Ira Weiny]

Jonathan Cameron wrote:
> On Tue, 27 Aug 2024 09:40:05 -0700
> nifan.cxl@gmail.com wrote:
> 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > When inserting multiple dynamic capacity event records grouped via More flag,
> > we should only trigger interrupt after the last record is inserted into the
> > event log. Achieving the goal by letting cxl_event_insert return true only
> > for the insertion of the last dynamic capacity event record in the sequence.
> 
> I'm not sure this one is accurate.  We might well have a slow
> system provisioning capacity one extent at time (and interrupting).
> 
> The event buffer might also not be large enough to hold all records so
> the device might 'wait' before figuring out the next extent for there
> to be somewhere to put the record.
> 
> Overall I think we can interrupt on each one and it should 'work'
> as should interrupt only once there are lots of them or
> every (n).

Indeed I think it should work.  But you won't see any extents as they will
be pending in the memdev.

Did this fail in some way?  I'm sorry I did not try and use qemu to test
the more bit.  Rather I used cxl_test for that.

Ira

> 
> Interrupt only fires on a 0 to >= 1 transition anyway, not
> on repeats after that unless the log has been cleared.
> It's up to OS to keep clearing records until it at least
> momentarily hits 0 if it wants to get any more interrupts.
> 
> Jonathan
> 
> 
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >  hw/cxl/cxl-events.c         | 8 ++++++++
> >  include/hw/cxl/cxl_events.h | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> > index 12dee2e467..90536c0e68 100644
> > --- a/hw/cxl/cxl-events.c
> > +++ b/hw/cxl/cxl-events.c
> > @@ -135,6 +135,14 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
> >      QSIMPLEQ_INSERT_TAIL(&log->events, entry, node);
> >      cxl_event_set_status(cxlds, log_type, true);
> >  
> > +    /*
> > +     * For dynamic capacity event records grouped via More flag,
> > +     * Only raise interrupt after inserting the last record in the log.
> > +     */
> > +    if (log_type == CXL_EVENT_TYPE_DYNAMIC_CAP) {
> > +        CXLEventDynamicCapacity *dCap = (CXLEventDynamicCapacity *)event;
> > +        return (dCap->flags & MORE_FLAG) ? false : true;
> > +    }
> >      /* Count went from 0 to 1 */
> >      return cxl_event_count(log) == 1;
> 
> If there are multiple this will fail I think as cxl_event_count(log) will go from 0
> to X not 1.
> 
> >  }
> > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> > index 38cadaa0f3..b0e5cc89c0 100644
> > --- a/include/hw/cxl/cxl_events.h
> > +++ b/include/hw/cxl/cxl_events.h
> > @@ -170,6 +170,7 @@ typedef struct CXLEventMemoryModule {
> >   * CXL r3.1 section Table 8-50: Dynamic Capacity Event Record
> >   * All fields little endian.
> >   */
> > +#define MORE_FLAG BIT_MASK(0)
> >  typedef struct CXLEventDynamicCapacity {
> >      CXLEventRecordHdr hdr;
> >      uint8_t type;
> 

Re: [PATCH 2/2] hw/cxl/cxl_event: Fix interrupt triggering for dynamic capacity events grouped via More flag

[Fan Ni]

On Wed, Sep 04, 2024 at 11:37:33AM -0500, Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Tue, 27 Aug 2024 09:40:05 -0700
> > nifan.cxl@gmail.com wrote:
> > 
> > > From: Fan Ni <fan.ni@samsung.com>
> > > 
> > > When inserting multiple dynamic capacity event records grouped via More flag,
> > > we should only trigger interrupt after the last record is inserted into the
> > > event log. Achieving the goal by letting cxl_event_insert return true only
> > > for the insertion of the last dynamic capacity event record in the sequence.
> > 
> > I'm not sure this one is accurate.  We might well have a slow
> > system provisioning capacity one extent at time (and interrupting).
> > 
> > The event buffer might also not be large enough to hold all records so
> > the device might 'wait' before figuring out the next extent for there
> > to be somewhere to put the record.
> > 
> > Overall I think we can interrupt on each one and it should 'work'
> > as should interrupt only once there are lots of them or
> > every (n).
> 
> Indeed I think it should work.  But you won't see any extents as they will
> be pending in the memdev.
> 
> Did this fail in some way?  I'm sorry I did not try and use qemu to test
> the more bit.  Rather I used cxl_test for that.
> 
> Ira

It works with or without this fix in my test. Until the last extent is
notified to the OS, the extents will be pending as you mentioned. 

Fan

> 
> > 
> > Interrupt only fires on a 0 to >= 1 transition anyway, not
> > on repeats after that unless the log has been cleared.
> > It's up to OS to keep clearing records until it at least
> > momentarily hits 0 if it wants to get any more interrupts.
> > 
> > Jonathan
> > 
> > 
> > > 
> > > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > > ---
> > >  hw/cxl/cxl-events.c         | 8 ++++++++
> > >  include/hw/cxl/cxl_events.h | 1 +
> > >  2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> > > index 12dee2e467..90536c0e68 100644
> > > --- a/hw/cxl/cxl-events.c
> > > +++ b/hw/cxl/cxl-events.c
> > > @@ -135,6 +135,14 @@ bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
> > >      QSIMPLEQ_INSERT_TAIL(&log->events, entry, node);
> > >      cxl_event_set_status(cxlds, log_type, true);
> > >  
> > > +    /*
> > > +     * For dynamic capacity event records grouped via More flag,
> > > +     * Only raise interrupt after inserting the last record in the log.
> > > +     */
> > > +    if (log_type == CXL_EVENT_TYPE_DYNAMIC_CAP) {
> > > +        CXLEventDynamicCapacity *dCap = (CXLEventDynamicCapacity *)event;
> > > +        return (dCap->flags & MORE_FLAG) ? false : true;
> > > +    }
> > >      /* Count went from 0 to 1 */
> > >      return cxl_event_count(log) == 1;
> > 
> > If there are multiple this will fail I think as cxl_event_count(log) will go from 0
> > to X not 1.
> > 
> > >  }
> > > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> > > index 38cadaa0f3..b0e5cc89c0 100644
> > > --- a/include/hw/cxl/cxl_events.h
> > > +++ b/include/hw/cxl/cxl_events.h
> > > @@ -170,6 +170,7 @@ typedef struct CXLEventMemoryModule {
> > >   * CXL r3.1 section Table 8-50: Dynamic Capacity Event Record
> > >   * All fields little endian.
> > >   */
> > > +#define MORE_FLAG BIT_MASK(0)
> > >  typedef struct CXLEventDynamicCapacity {
> > >      CXLEventRecordHdr hdr;
> > >      uint8_t type;
> > 
> 
> 

-- 
Fan Ni