From: Jonathan Cameron via <qemu-devel@nongnu.org>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Michael Tsirkin <mst@redhat.com>,
Ben Widawsky <bwidawsk@kernel.org>, <qemu-devel@nongnu.org>,
<linux-cxl@vger.kernel.org>,
Peter Maydell <peter.maydell@linaro.org>
Subject: Re: [PATCH v2 8/8] hw/cxl/events: Add in inject general media event
Date: Tue, 10 Jan 2023 15:38:53 +0000 [thread overview]
Message-ID: <20230110153853.00000945@huawei.com> (raw)
In-Reply-To: <Y7xn0NNupucMou4b@iweiny-desk3>
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:
> > > #
> > >
> >
next prev parent reply other threads:[~2023-01-10 15:53 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-22 4:24 [PATCH v2 0/8] QEMU CXL Provide mock CXL events and irq support Ira Weiny
2022-12-22 4:24 ` [PATCH v2 1/8] qemu/bswap: Add const_le64() Ira Weiny
2022-12-22 4:24 ` [PATCH v2 2/8] qemu/uuid: Add UUID static initializer Ira Weiny
2022-12-22 4:24 ` [PATCH v2 3/8] hw/cxl/mailbox: Use new UUID network order define for cel_uuid Ira Weiny
2023-01-03 16:30 ` Jonathan Cameron via
2022-12-22 4:24 ` [PATCH v2 4/8] hw/cxl/events: Add event status register Ira Weiny
2023-01-03 16:36 ` Jonathan Cameron via
2022-12-22 4:24 ` [PATCH v2 5/8] hw/cxl/events: Wire up get/clear event mailbox commands Ira Weiny
2023-01-04 18:07 ` Jonathan Cameron via
2023-01-24 17:04 ` Jonathan Cameron via
2022-12-22 4:24 ` [PATCH v2 6/8] hw/cxl/events: Add event interrupt support Ira Weiny
2022-12-22 4:24 ` [PATCH v2 7/8] bswap: Add the ability to store to an unaligned 24 bit field Ira Weiny
2023-01-03 17:14 ` Jonathan Cameron via
2022-12-22 4:24 ` [PATCH v2 8/8] hw/cxl/events: Add in inject general media event Ira Weiny
2023-01-03 18:07 ` Jonathan Cameron via
2023-01-09 19:15 ` Ira Weiny
2023-01-10 15:38 ` Jonathan Cameron via [this message]
2023-01-11 14:05 ` Jonathan Cameron via
2023-01-12 15:34 ` Jonathan Cameron via
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230110153853.00000945@huawei.com \
--to=qemu-devel@nongnu.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=bwidawsk@kernel.org \
--cc=ira.weiny@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=mst@redhat.com \
--cc=peter.maydell@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).