Message ID | 20240827164304.88876-3-nifan.cxl@gmail.com |
---|---|
State | New |
Headers | show |
Series | QEMU DCD emulation support fix | expand |
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; >
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; > > > >
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;