| Message ID | 20221221-ira-cxl-events-2022-11-17-v2-8-2ce2ecc06219@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QEMU CXL Provide mock CXL events and irq support | expand |
On Wed, 21 Dec 2022 20:24:38 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > To facilitate testing provide a QMP command to inject a general media > event. The event can be added to the log specified. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Hi Ira, Suggestion inline on how to more neatly handle optional arguments using QMPs inbuilt handling. Short version is stick a * in front of the argument name in the json and you get a bonus parameter in the callback bool has_<name> which lets you identify if it is provided or not. Jonathan > > --- > Changes from RFC: > Add all fields for this event > irq happens automatically when log transitions from 0 to 1 > --- > hw/mem/cxl_type3.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > hw/mem/cxl_type3_stubs.c | 8 ++++ > include/hw/cxl/cxl_events.h | 20 ++++++++++ > qapi/cxl.json | 25 ++++++++++++ > 4 files changed, 146 insertions(+) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index a43949cab120..bedd09e500ba 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -916,6 +916,99 @@ static CXLPoisonList *get_poison_list(CXLType3Dev *ct3d) > return &ct3d->poison_list; > } > > +static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr, > + QemuUUID *uuid, uint8_t flags, > + uint8_t length) > +{ > + hdr->flags[0] = flags; > + hdr->length = length; > + memcpy(&hdr->id, uuid, sizeof(hdr->id)); > + hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > +} > + > +QemuUUID gen_media_uuid = { > + .data = UUID(0xfbcd0a77, 0xc260, 0x417f, > + 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6), > +}; > + > +#define CXL_GMER_VALID_CHANNEL BIT(0) > +#define CXL_GMER_VALID_RANK BIT(1) > +#define CXL_GMER_VALID_DEVICE BIT(2) > +#define CXL_GMER_VALID_COMPONENT BIT(3) > + > +/* > + * For channel, rank, and device; any value inside of the fields valid range > + * will flag that field to be valid. IE pass -1 to mark the field invalid. > + * > + * Component ID is device specific. Define this as a string. > + */ > +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > + uint8_t flags, uint64_t physaddr, > + uint8_t descriptor, uint8_t type, > + uint8_t transaction_type, > + int16_t channel, int16_t rank, > + int32_t device, > + const char *component_id, > + Error **errp) > +{ > + Object *obj = object_resolve_path(path, NULL); > + struct cxl_event_gen_media gem; > + struct cxl_event_record_hdr *hdr = &gem.hdr; > + CXLDeviceState *cxlds; > + CXLType3Dev *ct3d; > + uint16_t valid_flags = 0; > + > + if (log >= CXL_EVENT_TYPE_MAX) { > + error_setg(errp, "Invalid log type: %d", log); > + return; > + } > + if (!obj) { > + error_setg(errp, "Unable to resolve path"); > + return; > + } > + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { > + error_setg(errp, "Path does not point to a CXL type 3 device"); > + } > + ct3d = CXL_TYPE3(obj); > + cxlds = &ct3d->cxl_dstate; > + > + memset(&gem, 0, sizeof(gem)); > + cxl_assign_event_header(hdr, &gen_media_uuid, flags, > + sizeof(struct cxl_event_gen_media)); > + > + gem.phys_addr = physaddr; > + gem.descriptor = descriptor; > + gem.type = type; > + gem.transaction_type = transaction_type; > + > + if (0 <= channel && channel <= 0xFF) { > + gem.channel = channel; > + valid_flags |= CXL_GMER_VALID_CHANNEL; > + } > + > + if (0 <= rank && rank <= 0xFF) { > + gem.rank = rank; > + valid_flags |= CXL_GMER_VALID_RANK; > + } > + > + if (0 <= device && device <= 0xFFFFFF) { > + st24_le_p(gem.device, device); > + valid_flags |= CXL_GMER_VALID_DEVICE; > + } > + > + if (component_id && strcmp(component_id, "")) { > + strncpy((char *)gem.component_id, component_id, > + sizeof(gem.component_id) - 1); > + valid_flags |= CXL_GMER_VALID_COMPONENT; > + } > + > + stw_le_p(gem.validity_flags, valid_flags); > + > + if (cxl_event_insert(cxlds, log, (struct cxl_event_record_raw *)&gem)) { > + cxl_event_irq_assert(ct3d); > + } > +} > + > void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, > Error **errp) > { > diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c > index f2c9f48f4010..62f04d487031 100644 > --- a/hw/mem/cxl_type3_stubs.c > +++ b/hw/mem/cxl_type3_stubs.c > @@ -2,6 +2,14 @@ > #include "qemu/osdep.h" > #include "qapi/qapi-commands-cxl.h" > > +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > + uint8_t flags, uint64_t physaddr, > + uint8_t descriptor, uint8_t type, > + uint8_t transaction_type, > + int16_t channel, int16_t rank, > + int32_t device, > + char *component_id, > + Error **errp) {} > void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, > Error **errp) {} > void qmp_cxl_inject_uncorrectable_error(const char *path, > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h > index 2df40720320a..3175e9d9866d 100644 > --- a/include/hw/cxl/cxl_events.h > +++ b/include/hw/cxl/cxl_events.h > @@ -103,4 +103,24 @@ struct cxl_event_interrupt_policy { > /* DCD is optional but other fields are not */ > #define CXL_EVENT_INT_SETTING_MIN_LEN 4 > > +/* > + * General Media Event Record > + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43 > + */ > +#define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10 > +#define CXL_EVENT_GEN_MED_RES_SIZE 0x2e > +struct cxl_event_gen_media { > + struct cxl_event_record_hdr hdr; > + uint64_t phys_addr; > + uint8_t descriptor; > + uint8_t type; > + uint8_t transaction_type; > + uint8_t validity_flags[2]; > + uint8_t channel; > + uint8_t rank; > + uint8_t device[3]; > + uint8_t component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE]; > + uint8_t reserved[CXL_EVENT_GEN_MED_RES_SIZE]; > +} QEMU_PACKED; > + > #endif /* CXL_EVENTS_H */ > diff --git a/qapi/cxl.json b/qapi/cxl.json > index b4836bb87f53..56e85a28d7e0 100644 > --- a/qapi/cxl.json > +++ b/qapi/cxl.json > @@ -5,6 +5,31 @@ > # = CXL devices > ## > > +## > +# @cxl-inject-gen-media-event: > +# > +# @path: CXL type 3 device canonical QOM path > +# > +# @log: Event Log to add the event to > +# @flags: header flags > +# @physaddr: Physical Address > +# @descriptor: Descriptor > +# @type: Type > +# @transactiontype: Transaction Type > +# @channel: Channel > +# @rank: Rank > +# @device: Device > +# @componentid: Device specific string > +# > +## > +{ 'command': 'cxl-inject-gen-media-event', > + 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', > + 'physaddr': 'uint64', 'descriptor': 'uint8', > + 'type': 'uint8', 'transactiontype': 'uint8', > + 'channel': 'int16', 'rank': 'int16', > + 'device': 'int32', 'componentid': 'str' > + }} From an interface cleanliness point of view I'd rather see all the optional fields as optional. That's done by marking them with a * so '*channel': 'int16' Then the signature of the related qmp_cxl_inject_gen_media_event gains a boolean has_channel parameter (before channel) Those booleans can be used to set the flags etc. Very lightly tested: diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 4660a44ef8..cb9bb0b166 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -1203,8 +1203,9 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, uint8_t flags, uint64_t physaddr, uint8_t descriptor, uint8_t type, uint8_t transaction_type, - int16_t channel, int16_t rank, - int32_t device, + bool has_channel, uint8_t channel, + bool has_rank, uint8_t rank, + bool has_device, uint32_t device, const char *component_id, Error **errp) { @@ -1238,22 +1239,22 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, gem.type = type; gem.transaction_type = transaction_type; - if (0 <= channel && channel <= 0xFF) { + if (has_channel) { gem.channel = channel; valid_flags |= CXL_GMER_VALID_CHANNEL; } - if (0 <= rank && rank <= 0xFF) { + if (has_rank) { gem.rank = rank; valid_flags |= CXL_GMER_VALID_RANK; } - if (0 <= device && device <= 0xFFFFFF) { + if (has_device) { st24_le_p(gem.device, device); valid_flags |= CXL_GMER_VALID_DEVICE; } - if (component_id && strcmp(component_id, "")) { + if (component_id) { strncpy((char *)gem.component_id, component_id, sizeof(gem.component_id) - 1); valid_flags |= CXL_GMER_VALID_COMPONENT; diff --git a/qapi/cxl.json b/qapi/cxl.json index 56e85a28d7..085f82e7da 100644 --- a/qapi/cxl.json +++ b/qapi/cxl.json @@ -26,8 +26,8 @@ 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', 'physaddr': 'uint64', 'descriptor': 'uint8', 'type': 'uint8', 'transactiontype': 'uint8', - 'channel': 'int16', 'rank': 'int16', - 'device': 'int32', 'componentid': 'str' + '*channel': 'uint8', '*rank': 'uint8', + '*device': 'uint32', '*componentid': 'str' }} ## > + > ## > # @cxl-inject-poison: > # >
On Tue, Jan 03, 2023 at 06:07:19PM +0000, Jonathan Cameron wrote: > On Wed, 21 Dec 2022 20:24:38 -0800 > Ira Weiny <ira.weiny@intel.com> wrote: > > > To facilitate testing provide a QMP command to inject a general media > > event. The event can be added to the log specified. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > Hi Ira, > > Suggestion inline on how to more neatly handle optional arguments using > QMPs inbuilt handling. Short version is stick a * in front of the > argument name in the json and you get a bonus parameter in the callback > bool has_<name> which lets you identify if it is provided or not. > > Jonathan > > > > > --- > > Changes from RFC: > > Add all fields for this event > > irq happens automatically when log transitions from 0 to 1 > > --- > > hw/mem/cxl_type3.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > > hw/mem/cxl_type3_stubs.c | 8 ++++ > > include/hw/cxl/cxl_events.h | 20 ++++++++++ > > qapi/cxl.json | 25 ++++++++++++ > > 4 files changed, 146 insertions(+) > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index a43949cab120..bedd09e500ba 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -916,6 +916,99 @@ static CXLPoisonList *get_poison_list(CXLType3Dev *ct3d) > > return &ct3d->poison_list; > > } > > > > +static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr, > > + QemuUUID *uuid, uint8_t flags, > > + uint8_t length) > > +{ > > + hdr->flags[0] = flags; > > + hdr->length = length; > > + memcpy(&hdr->id, uuid, sizeof(hdr->id)); > > + hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > +} > > + > > +QemuUUID gen_media_uuid = { > > + .data = UUID(0xfbcd0a77, 0xc260, 0x417f, > > + 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6), > > +}; > > + > > +#define CXL_GMER_VALID_CHANNEL BIT(0) > > +#define CXL_GMER_VALID_RANK BIT(1) > > +#define CXL_GMER_VALID_DEVICE BIT(2) > > +#define CXL_GMER_VALID_COMPONENT BIT(3) > > + > > +/* > > + * For channel, rank, and device; any value inside of the fields valid range > > + * will flag that field to be valid. IE pass -1 to mark the field invalid. > > + * > > + * Component ID is device specific. Define this as a string. > > + */ > > +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > + uint8_t flags, uint64_t physaddr, > > + uint8_t descriptor, uint8_t type, > > + uint8_t transaction_type, > > + int16_t channel, int16_t rank, > > + int32_t device, > > + const char *component_id, > > + Error **errp) > > +{ > > + Object *obj = object_resolve_path(path, NULL); > > + struct cxl_event_gen_media gem; > > + struct cxl_event_record_hdr *hdr = &gem.hdr; > > + CXLDeviceState *cxlds; > > + CXLType3Dev *ct3d; > > + uint16_t valid_flags = 0; > > + > > + if (log >= CXL_EVENT_TYPE_MAX) { > > + error_setg(errp, "Invalid log type: %d", log); > > + return; > > + } > > + if (!obj) { > > + error_setg(errp, "Unable to resolve path"); > > + return; > > + } > > + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { > > + error_setg(errp, "Path does not point to a CXL type 3 device"); > > + } > > + ct3d = CXL_TYPE3(obj); > > + cxlds = &ct3d->cxl_dstate; > > + > > + memset(&gem, 0, sizeof(gem)); > > + cxl_assign_event_header(hdr, &gen_media_uuid, flags, > > + sizeof(struct cxl_event_gen_media)); > > + > > + gem.phys_addr = physaddr; > > + gem.descriptor = descriptor; > > + gem.type = type; > > + gem.transaction_type = transaction_type; > > + > > + if (0 <= channel && channel <= 0xFF) { > > + gem.channel = channel; > > + valid_flags |= CXL_GMER_VALID_CHANNEL; > > + } > > + > > + if (0 <= rank && rank <= 0xFF) { > > + gem.rank = rank; > > + valid_flags |= CXL_GMER_VALID_RANK; > > + } > > + > > + if (0 <= device && device <= 0xFFFFFF) { > > + st24_le_p(gem.device, device); > > + valid_flags |= CXL_GMER_VALID_DEVICE; > > + } > > + > > + if (component_id && strcmp(component_id, "")) { > > + strncpy((char *)gem.component_id, component_id, > > + sizeof(gem.component_id) - 1); > > + valid_flags |= CXL_GMER_VALID_COMPONENT; > > + } > > + > > + stw_le_p(gem.validity_flags, valid_flags); > > + > > + if (cxl_event_insert(cxlds, log, (struct cxl_event_record_raw *)&gem)) { > > + cxl_event_irq_assert(ct3d); > > + } > > +} > > + > > void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, > > Error **errp) > > { > > diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c > > index f2c9f48f4010..62f04d487031 100644 > > --- a/hw/mem/cxl_type3_stubs.c > > +++ b/hw/mem/cxl_type3_stubs.c > > @@ -2,6 +2,14 @@ > > #include "qemu/osdep.h" > > #include "qapi/qapi-commands-cxl.h" > > > > +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > + uint8_t flags, uint64_t physaddr, > > + uint8_t descriptor, uint8_t type, > > + uint8_t transaction_type, > > + int16_t channel, int16_t rank, > > + int32_t device, > > + char *component_id, > > + Error **errp) {} > > void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, > > Error **errp) {} > > void qmp_cxl_inject_uncorrectable_error(const char *path, > > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h > > index 2df40720320a..3175e9d9866d 100644 > > --- a/include/hw/cxl/cxl_events.h > > +++ b/include/hw/cxl/cxl_events.h > > @@ -103,4 +103,24 @@ struct cxl_event_interrupt_policy { > > /* DCD is optional but other fields are not */ > > #define CXL_EVENT_INT_SETTING_MIN_LEN 4 > > > > +/* > > + * General Media Event Record > > + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43 > > + */ > > +#define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10 > > +#define CXL_EVENT_GEN_MED_RES_SIZE 0x2e > > +struct cxl_event_gen_media { > > + struct cxl_event_record_hdr hdr; > > + uint64_t phys_addr; > > + uint8_t descriptor; > > + uint8_t type; > > + uint8_t transaction_type; > > + uint8_t validity_flags[2]; > > + uint8_t channel; > > + uint8_t rank; > > + uint8_t device[3]; > > + uint8_t component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE]; > > + uint8_t reserved[CXL_EVENT_GEN_MED_RES_SIZE]; > > +} QEMU_PACKED; > > + > > #endif /* CXL_EVENTS_H */ > > diff --git a/qapi/cxl.json b/qapi/cxl.json > > index b4836bb87f53..56e85a28d7e0 100644 > > --- a/qapi/cxl.json > > +++ b/qapi/cxl.json > > @@ -5,6 +5,31 @@ > > # = CXL devices > > ## > > > > +## > > +# @cxl-inject-gen-media-event: > > +# > > +# @path: CXL type 3 device canonical QOM path > > +# > > +# @log: Event Log to add the event to > > +# @flags: header flags > > +# @physaddr: Physical Address > > +# @descriptor: Descriptor > > +# @type: Type > > +# @transactiontype: Transaction Type > > +# @channel: Channel > > +# @rank: Rank > > +# @device: Device > > +# @componentid: Device specific string > > +# > > +## > > +{ 'command': 'cxl-inject-gen-media-event', > > + 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', > > + 'physaddr': 'uint64', 'descriptor': 'uint8', > > + 'type': 'uint8', 'transactiontype': 'uint8', > > + 'channel': 'int16', 'rank': 'int16', > > + 'device': 'int32', 'componentid': 'str' > > + }} > > From an interface cleanliness point of view I'd rather see > all the optional fields as optional. That's done by marking them > with a * so > '*channel': 'int16' > > Then the signature of the related qmp_cxl_inject_gen_media_event > gains a boolean has_channel parameter (before channel) > > Those booleans can be used to set the flags etc. Ah! Ok cool. Yes this would make a lot more sense. I did not realize QMP did this optional thing. That makes scripting this easier as well! Did you put all this on a branch or not? I did not see anything new at: https://gitlab.com/jic23/qemu.git I can definitely respin but it sounds like you were going to make some changes. Ira > > Very lightly tested: > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 4660a44ef8..cb9bb0b166 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -1203,8 +1203,9 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > uint8_t flags, uint64_t physaddr, > uint8_t descriptor, uint8_t type, > uint8_t transaction_type, > - int16_t channel, int16_t rank, > - int32_t device, > + bool has_channel, uint8_t channel, > + bool has_rank, uint8_t rank, > + bool has_device, uint32_t device, > const char *component_id, > Error **errp) > { > @@ -1238,22 +1239,22 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > gem.type = type; > gem.transaction_type = transaction_type; > > - if (0 <= channel && channel <= 0xFF) { > + if (has_channel) { > gem.channel = channel; > valid_flags |= CXL_GMER_VALID_CHANNEL; > } > > - if (0 <= rank && rank <= 0xFF) { > + if (has_rank) { > gem.rank = rank; > valid_flags |= CXL_GMER_VALID_RANK; > } > > - if (0 <= device && device <= 0xFFFFFF) { > + if (has_device) { > st24_le_p(gem.device, device); > valid_flags |= CXL_GMER_VALID_DEVICE; > } > > - if (component_id && strcmp(component_id, "")) { > + if (component_id) { > strncpy((char *)gem.component_id, component_id, > sizeof(gem.component_id) - 1); > valid_flags |= CXL_GMER_VALID_COMPONENT; > diff --git a/qapi/cxl.json b/qapi/cxl.json > index 56e85a28d7..085f82e7da 100644 > --- a/qapi/cxl.json > +++ b/qapi/cxl.json > @@ -26,8 +26,8 @@ > 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', > 'physaddr': 'uint64', 'descriptor': 'uint8', > 'type': 'uint8', 'transactiontype': 'uint8', > - 'channel': 'int16', 'rank': 'int16', > - 'device': 'int32', 'componentid': 'str' > + '*channel': 'uint8', '*rank': 'uint8', > + '*device': 'uint32', '*componentid': 'str' > }} > > ## > > > + > > ## > > # @cxl-inject-poison: > > # > > >
On Mon, 9 Jan 2023 11:15:28 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > On Tue, Jan 03, 2023 at 06:07:19PM +0000, Jonathan Cameron wrote: > > On Wed, 21 Dec 2022 20:24:38 -0800 > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > To facilitate testing provide a QMP command to inject a general media > > > event. The event can be added to the log specified. > > > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > Hi Ira, > > > > Suggestion inline on how to more neatly handle optional arguments using > > QMPs inbuilt handling. Short version is stick a * in front of the > > argument name in the json and you get a bonus parameter in the callback > > bool has_<name> which lets you identify if it is provided or not. > > > > Jonathan > > > > > > > > --- > > > Changes from RFC: > > > Add all fields for this event > > > irq happens automatically when log transitions from 0 to 1 > > > --- > > > hw/mem/cxl_type3.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > > > hw/mem/cxl_type3_stubs.c | 8 ++++ > > > include/hw/cxl/cxl_events.h | 20 ++++++++++ > > > qapi/cxl.json | 25 ++++++++++++ > > > 4 files changed, 146 insertions(+) > > > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > > index a43949cab120..bedd09e500ba 100644 > > > --- a/hw/mem/cxl_type3.c > > > +++ b/hw/mem/cxl_type3.c > > > @@ -916,6 +916,99 @@ static CXLPoisonList *get_poison_list(CXLType3Dev *ct3d) > > > return &ct3d->poison_list; > > > } > > > > > > +static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr, > > > + QemuUUID *uuid, uint8_t flags, > > > + uint8_t length) > > > +{ > > > + hdr->flags[0] = flags; > > > + hdr->length = length; > > > + memcpy(&hdr->id, uuid, sizeof(hdr->id)); > > > + hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > > +} > > > + > > > +QemuUUID gen_media_uuid = { > > > + .data = UUID(0xfbcd0a77, 0xc260, 0x417f, > > > + 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6), > > > +}; > > > + > > > +#define CXL_GMER_VALID_CHANNEL BIT(0) > > > +#define CXL_GMER_VALID_RANK BIT(1) > > > +#define CXL_GMER_VALID_DEVICE BIT(2) > > > +#define CXL_GMER_VALID_COMPONENT BIT(3) > > > + > > > +/* > > > + * For channel, rank, and device; any value inside of the fields valid range > > > + * will flag that field to be valid. IE pass -1 to mark the field invalid. > > > + * > > > + * Component ID is device specific. Define this as a string. > > > + */ > > > +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > > + uint8_t flags, uint64_t physaddr, > > > + uint8_t descriptor, uint8_t type, > > > + uint8_t transaction_type, > > > + int16_t channel, int16_t rank, > > > + int32_t device, > > > + const char *component_id, > > > + Error **errp) > > > +{ > > > + Object *obj = object_resolve_path(path, NULL); > > > + struct cxl_event_gen_media gem; > > > + struct cxl_event_record_hdr *hdr = &gem.hdr; > > > + CXLDeviceState *cxlds; > > > + CXLType3Dev *ct3d; > > > + uint16_t valid_flags = 0; > > > + > > > + if (log >= CXL_EVENT_TYPE_MAX) { > > > + error_setg(errp, "Invalid log type: %d", log); > > > + return; > > > + } > > > + if (!obj) { > > > + error_setg(errp, "Unable to resolve path"); > > > + return; > > > + } > > > + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { > > > + error_setg(errp, "Path does not point to a CXL type 3 device"); > > > + } > > > + ct3d = CXL_TYPE3(obj); > > > + cxlds = &ct3d->cxl_dstate; > > > + > > > + memset(&gem, 0, sizeof(gem)); > > > + cxl_assign_event_header(hdr, &gen_media_uuid, flags, > > > + sizeof(struct cxl_event_gen_media)); > > > + > > > + gem.phys_addr = physaddr; > > > + gem.descriptor = descriptor; > > > + gem.type = type; > > > + gem.transaction_type = transaction_type; > > > + > > > + if (0 <= channel && channel <= 0xFF) { > > > + gem.channel = channel; > > > + valid_flags |= CXL_GMER_VALID_CHANNEL; > > > + } > > > + > > > + if (0 <= rank && rank <= 0xFF) { > > > + gem.rank = rank; > > > + valid_flags |= CXL_GMER_VALID_RANK; > > > + } > > > + > > > + if (0 <= device && device <= 0xFFFFFF) { > > > + st24_le_p(gem.device, device); > > > + valid_flags |= CXL_GMER_VALID_DEVICE; > > > + } > > > + > > > + if (component_id && strcmp(component_id, "")) { > > > + strncpy((char *)gem.component_id, component_id, > > > + sizeof(gem.component_id) - 1); > > > + valid_flags |= CXL_GMER_VALID_COMPONENT; > > > + } > > > + > > > + stw_le_p(gem.validity_flags, valid_flags); > > > + > > > + if (cxl_event_insert(cxlds, log, (struct cxl_event_record_raw *)&gem)) { > > > + cxl_event_irq_assert(ct3d); > > > + } > > > +} > > > + > > > void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, > > > Error **errp) > > > { > > > diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c > > > index f2c9f48f4010..62f04d487031 100644 > > > --- a/hw/mem/cxl_type3_stubs.c > > > +++ b/hw/mem/cxl_type3_stubs.c > > > @@ -2,6 +2,14 @@ > > > #include "qemu/osdep.h" > > > #include "qapi/qapi-commands-cxl.h" > > > > > > +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > > + uint8_t flags, uint64_t physaddr, > > > + uint8_t descriptor, uint8_t type, > > > + uint8_t transaction_type, > > > + int16_t channel, int16_t rank, > > > + int32_t device, > > > + char *component_id, > > > + Error **errp) {} > > > void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, > > > Error **errp) {} > > > void qmp_cxl_inject_uncorrectable_error(const char *path, > > > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h > > > index 2df40720320a..3175e9d9866d 100644 > > > --- a/include/hw/cxl/cxl_events.h > > > +++ b/include/hw/cxl/cxl_events.h > > > @@ -103,4 +103,24 @@ struct cxl_event_interrupt_policy { > > > /* DCD is optional but other fields are not */ > > > #define CXL_EVENT_INT_SETTING_MIN_LEN 4 > > > > > > +/* > > > + * General Media Event Record > > > + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43 > > > + */ > > > +#define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10 > > > +#define CXL_EVENT_GEN_MED_RES_SIZE 0x2e > > > +struct cxl_event_gen_media { > > > + struct cxl_event_record_hdr hdr; > > > + uint64_t phys_addr; > > > + uint8_t descriptor; > > > + uint8_t type; > > > + uint8_t transaction_type; > > > + uint8_t validity_flags[2]; > > > + uint8_t channel; > > > + uint8_t rank; > > > + uint8_t device[3]; > > > + uint8_t component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE]; > > > + uint8_t reserved[CXL_EVENT_GEN_MED_RES_SIZE]; > > > +} QEMU_PACKED; > > > + > > > #endif /* CXL_EVENTS_H */ > > > diff --git a/qapi/cxl.json b/qapi/cxl.json > > > index b4836bb87f53..56e85a28d7e0 100644 > > > --- a/qapi/cxl.json > > > +++ b/qapi/cxl.json > > > @@ -5,6 +5,31 @@ > > > # = CXL devices > > > ## > > > > > > +## > > > +# @cxl-inject-gen-media-event: > > > +# > > > +# @path: CXL type 3 device canonical QOM path > > > +# > > > +# @log: Event Log to add the event to > > > +# @flags: header flags > > > +# @physaddr: Physical Address > > > +# @descriptor: Descriptor > > > +# @type: Type > > > +# @transactiontype: Transaction Type > > > +# @channel: Channel > > > +# @rank: Rank > > > +# @device: Device > > > +# @componentid: Device specific string > > > +# > > > +## > > > +{ 'command': 'cxl-inject-gen-media-event', > > > + 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', > > > + 'physaddr': 'uint64', 'descriptor': 'uint8', > > > + 'type': 'uint8', 'transactiontype': 'uint8', > > > + 'channel': 'int16', 'rank': 'int16', > > > + 'device': 'int32', 'componentid': 'str' > > > + }} > > > > From an interface cleanliness point of view I'd rather see > > all the optional fields as optional. That's done by marking them > > with a * so > > '*channel': 'int16' > > > > Then the signature of the related qmp_cxl_inject_gen_media_event > > gains a boolean has_channel parameter (before channel) > > > > Those booleans can be used to set the flags etc. > > Ah! Ok cool. Yes this would make a lot more sense. I did not realize QMP did > this optional thing. That makes scripting this easier as well! > > Did you put all this on a branch or not? I did not see anything new at: > > https://gitlab.com/jic23/qemu.git > > I can definitely respin but it sounds like you were going to make some changes. I got side tracked and reworked a few more things. Hopefully in have a branch up in a day or two. Jonathan > > Ira > > > > > Very lightly tested: > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index 4660a44ef8..cb9bb0b166 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -1203,8 +1203,9 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > uint8_t flags, uint64_t physaddr, > > uint8_t descriptor, uint8_t type, > > uint8_t transaction_type, > > - int16_t channel, int16_t rank, > > - int32_t device, > > + bool has_channel, uint8_t channel, > > + bool has_rank, uint8_t rank, > > + bool has_device, uint32_t device, > > const char *component_id, > > Error **errp) > > { > > @@ -1238,22 +1239,22 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > gem.type = type; > > gem.transaction_type = transaction_type; > > > > - if (0 <= channel && channel <= 0xFF) { > > + if (has_channel) { > > gem.channel = channel; > > valid_flags |= CXL_GMER_VALID_CHANNEL; > > } > > > > - if (0 <= rank && rank <= 0xFF) { > > + if (has_rank) { > > gem.rank = rank; > > valid_flags |= CXL_GMER_VALID_RANK; > > } > > > > - if (0 <= device && device <= 0xFFFFFF) { > > + if (has_device) { > > st24_le_p(gem.device, device); > > valid_flags |= CXL_GMER_VALID_DEVICE; > > } > > > > - if (component_id && strcmp(component_id, "")) { > > + if (component_id) { > > strncpy((char *)gem.component_id, component_id, > > sizeof(gem.component_id) - 1); > > valid_flags |= CXL_GMER_VALID_COMPONENT; > > diff --git a/qapi/cxl.json b/qapi/cxl.json > > index 56e85a28d7..085f82e7da 100644 > > --- a/qapi/cxl.json > > +++ b/qapi/cxl.json > > @@ -26,8 +26,8 @@ > > 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', > > 'physaddr': 'uint64', 'descriptor': 'uint8', > > 'type': 'uint8', 'transactiontype': 'uint8', > > - 'channel': 'int16', 'rank': 'int16', > > - 'device': 'int32', 'componentid': 'str' > > + '*channel': 'uint8', '*rank': 'uint8', > > + '*device': 'uint32', '*componentid': 'str' > > }} > > > > ## > > > > > + > > > ## > > > # @cxl-inject-poison: > > > # > > > > >
> > > From an interface cleanliness point of view I'd rather see > > > all the optional fields as optional. That's done by marking them > > > with a * so > > > '*channel': 'int16' > > > > > > Then the signature of the related qmp_cxl_inject_gen_media_event > > > gains a boolean has_channel parameter (before channel) > > > > > > Those booleans can be used to set the flags etc. > > > > Ah! Ok cool. Yes this would make a lot more sense. I did not realize QMP did > > this optional thing. That makes scripting this easier as well! > > > > Did you put all this on a branch or not? I did not see anything new at: > > > > https://gitlab.com/jic23/qemu.git > > > > I can definitely respin but it sounds like you were going to make some changes. > > I got side tracked and reworked a few more things. Hopefully in have a branch > up in a day or two. Now pushed out as https://gitlab.com/jic23/qemu/-/tree/cxl-2023-01-11 My plan is to send the first 8 patches to Michael targetting upstream which includes your uuid related patches from the start of this series. The changes to RAS error injection since previous tree are substantial to support multiple header logging. I've made a few tweaks to your other patches on that branch including the optional parameters stuff and some reworks I mentioned in this thread. Note there is a dirty hack on top of that tree to deal with a build issue on my arch-linux test box that I foolishly upgraded this morning. Might break things on other distros (version issue with curl). Jonathan > > Jonathan > > > > > Ira > > > > > > > > Very lightly tested: > > > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > > index 4660a44ef8..cb9bb0b166 100644 > > > --- a/hw/mem/cxl_type3.c > > > +++ b/hw/mem/cxl_type3.c > > > @@ -1203,8 +1203,9 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > > uint8_t flags, uint64_t physaddr, > > > uint8_t descriptor, uint8_t type, > > > uint8_t transaction_type, > > > - int16_t channel, int16_t rank, > > > - int32_t device, > > > + bool has_channel, uint8_t channel, > > > + bool has_rank, uint8_t rank, > > > + bool has_device, uint32_t device, > > > const char *component_id, > > > Error **errp) > > > { > > > @@ -1238,22 +1239,22 @@ void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, > > > gem.type = type; > > > gem.transaction_type = transaction_type; > > > > > > - if (0 <= channel && channel <= 0xFF) { > > > + if (has_channel) { > > > gem.channel = channel; > > > valid_flags |= CXL_GMER_VALID_CHANNEL; > > > } > > > > > > - if (0 <= rank && rank <= 0xFF) { > > > + if (has_rank) { > > > gem.rank = rank; > > > valid_flags |= CXL_GMER_VALID_RANK; > > > } > > > > > > - if (0 <= device && device <= 0xFFFFFF) { > > > + if (has_device) { > > > st24_le_p(gem.device, device); > > > valid_flags |= CXL_GMER_VALID_DEVICE; > > > } > > > > > > - if (component_id && strcmp(component_id, "")) { > > > + if (component_id) { > > > strncpy((char *)gem.component_id, component_id, > > > sizeof(gem.component_id) - 1); > > > valid_flags |= CXL_GMER_VALID_COMPONENT; > > > diff --git a/qapi/cxl.json b/qapi/cxl.json > > > index 56e85a28d7..085f82e7da 100644 > > > --- a/qapi/cxl.json > > > +++ b/qapi/cxl.json > > > @@ -26,8 +26,8 @@ > > > 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', > > > 'physaddr': 'uint64', 'descriptor': 'uint8', > > > 'type': 'uint8', 'transactiontype': 'uint8', > > > - 'channel': 'int16', 'rank': 'int16', > > > - 'device': 'int32', 'componentid': 'str' > > > + '*channel': 'uint8', '*rank': 'uint8', > > > + '*device': 'uint32', '*componentid': 'str' > > > }} > > > > > > ## > > > > > > > + > > > > ## > > > > # @cxl-inject-poison: > > > > # > > > > > > > > >
On Wed, 21 Dec 2022 20:24:38 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > To facilitate testing provide a QMP command to inject a general media > event. The event can be added to the log specified. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Hi Ira, One thing inline that kind of came out of Philippe's review of the earlier cleanup patches. > > --- > Changes from RFC: > Add all fields for this event > irq happens automatically when log transitions from 0 to 1 > --- > hw/mem/cxl_type3.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > hw/mem/cxl_type3_stubs.c | 8 ++++ > include/hw/cxl/cxl_events.h | 20 ++++++++++ > qapi/cxl.json | 25 ++++++++++++ > 4 files changed, 146 insertions(+) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index a43949cab120..bedd09e500ba 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -916,6 +916,99 @@ static CXLPoisonList *get_poison_list(CXLType3Dev *ct3d) > return &ct3d->poison_list; > } > > +static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr, > + QemuUUID *uuid, uint8_t flags, make that const QemuUUID *uuid and ... > + uint8_t length) > +{ > + hdr->flags[0] = flags; > + hdr->length = length; > + memcpy(&hdr->id, uuid, sizeof(hdr->id)); > + hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > +} > + static const > +QemuUUID gen_media_uuid = { > + .data = UUID(0xfbcd0a77, 0xc260, 0x417f, > + 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6), > +};
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index a43949cab120..bedd09e500ba 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -916,6 +916,99 @@ static CXLPoisonList *get_poison_list(CXLType3Dev *ct3d) return &ct3d->poison_list; } +static void cxl_assign_event_header(struct cxl_event_record_hdr *hdr, + QemuUUID *uuid, uint8_t flags, + uint8_t length) +{ + hdr->flags[0] = flags; + hdr->length = length; + memcpy(&hdr->id, uuid, sizeof(hdr->id)); + hdr->timestamp = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); +} + +QemuUUID gen_media_uuid = { + .data = UUID(0xfbcd0a77, 0xc260, 0x417f, + 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6), +}; + +#define CXL_GMER_VALID_CHANNEL BIT(0) +#define CXL_GMER_VALID_RANK BIT(1) +#define CXL_GMER_VALID_DEVICE BIT(2) +#define CXL_GMER_VALID_COMPONENT BIT(3) + +/* + * For channel, rank, and device; any value inside of the fields valid range + * will flag that field to be valid. IE pass -1 to mark the field invalid. + * + * Component ID is device specific. Define this as a string. + */ +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, + uint8_t flags, uint64_t physaddr, + uint8_t descriptor, uint8_t type, + uint8_t transaction_type, + int16_t channel, int16_t rank, + int32_t device, + const char *component_id, + Error **errp) +{ + Object *obj = object_resolve_path(path, NULL); + struct cxl_event_gen_media gem; + struct cxl_event_record_hdr *hdr = &gem.hdr; + CXLDeviceState *cxlds; + CXLType3Dev *ct3d; + uint16_t valid_flags = 0; + + if (log >= CXL_EVENT_TYPE_MAX) { + error_setg(errp, "Invalid log type: %d", log); + return; + } + if (!obj) { + error_setg(errp, "Unable to resolve path"); + return; + } + if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) { + error_setg(errp, "Path does not point to a CXL type 3 device"); + } + ct3d = CXL_TYPE3(obj); + cxlds = &ct3d->cxl_dstate; + + memset(&gem, 0, sizeof(gem)); + cxl_assign_event_header(hdr, &gen_media_uuid, flags, + sizeof(struct cxl_event_gen_media)); + + gem.phys_addr = physaddr; + gem.descriptor = descriptor; + gem.type = type; + gem.transaction_type = transaction_type; + + if (0 <= channel && channel <= 0xFF) { + gem.channel = channel; + valid_flags |= CXL_GMER_VALID_CHANNEL; + } + + if (0 <= rank && rank <= 0xFF) { + gem.rank = rank; + valid_flags |= CXL_GMER_VALID_RANK; + } + + if (0 <= device && device <= 0xFFFFFF) { + st24_le_p(gem.device, device); + valid_flags |= CXL_GMER_VALID_DEVICE; + } + + if (component_id && strcmp(component_id, "")) { + strncpy((char *)gem.component_id, component_id, + sizeof(gem.component_id) - 1); + valid_flags |= CXL_GMER_VALID_COMPONENT; + } + + stw_le_p(gem.validity_flags, valid_flags); + + if (cxl_event_insert(cxlds, log, (struct cxl_event_record_raw *)&gem)) { + cxl_event_irq_assert(ct3d); + } +} + void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, Error **errp) { diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c index f2c9f48f4010..62f04d487031 100644 --- a/hw/mem/cxl_type3_stubs.c +++ b/hw/mem/cxl_type3_stubs.c @@ -2,6 +2,14 @@ #include "qemu/osdep.h" #include "qapi/qapi-commands-cxl.h" +void qmp_cxl_inject_gen_media_event(const char *path, uint8_t log, + uint8_t flags, uint64_t physaddr, + uint8_t descriptor, uint8_t type, + uint8_t transaction_type, + int16_t channel, int16_t rank, + int32_t device, + char *component_id, + Error **errp) {} void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length, Error **errp) {} void qmp_cxl_inject_uncorrectable_error(const char *path, diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h index 2df40720320a..3175e9d9866d 100644 --- a/include/hw/cxl/cxl_events.h +++ b/include/hw/cxl/cxl_events.h @@ -103,4 +103,24 @@ struct cxl_event_interrupt_policy { /* DCD is optional but other fields are not */ #define CXL_EVENT_INT_SETTING_MIN_LEN 4 +/* + * General Media Event Record + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43 + */ +#define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10 +#define CXL_EVENT_GEN_MED_RES_SIZE 0x2e +struct cxl_event_gen_media { + struct cxl_event_record_hdr hdr; + uint64_t phys_addr; + uint8_t descriptor; + uint8_t type; + uint8_t transaction_type; + uint8_t validity_flags[2]; + uint8_t channel; + uint8_t rank; + uint8_t device[3]; + uint8_t component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE]; + uint8_t reserved[CXL_EVENT_GEN_MED_RES_SIZE]; +} QEMU_PACKED; + #endif /* CXL_EVENTS_H */ diff --git a/qapi/cxl.json b/qapi/cxl.json index b4836bb87f53..56e85a28d7e0 100644 --- a/qapi/cxl.json +++ b/qapi/cxl.json @@ -5,6 +5,31 @@ # = CXL devices ## +## +# @cxl-inject-gen-media-event: +# +# @path: CXL type 3 device canonical QOM path +# +# @log: Event Log to add the event to +# @flags: header flags +# @physaddr: Physical Address +# @descriptor: Descriptor +# @type: Type +# @transactiontype: Transaction Type +# @channel: Channel +# @rank: Rank +# @device: Device +# @componentid: Device specific string +# +## +{ 'command': 'cxl-inject-gen-media-event', + 'data': { 'path': 'str', 'log': 'uint8', 'flags': 'uint8', + 'physaddr': 'uint64', 'descriptor': 'uint8', + 'type': 'uint8', 'transactiontype': 'uint8', + 'channel': 'int16', 'rank': 'int16', + 'device': 'int32', 'componentid': 'str' + }} + ## # @cxl-inject-poison: #
To facilitate testing provide a QMP command to inject a general media event. The event can be added to the log specified. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- Changes from RFC: Add all fields for this event irq happens automatically when log transitions from 0 to 1 --- hw/mem/cxl_type3.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ hw/mem/cxl_type3_stubs.c | 8 ++++ include/hw/cxl/cxl_events.h | 20 ++++++++++ qapi/cxl.json | 25 ++++++++++++ 4 files changed, 146 insertions(+)