[v5,5/7] hw/cxl/events: Add injection of General Media Events

Commit Message

Jonathan Cameron April 23, 2023, 4:51 p.m. UTC

From: Ira Weiny <[e-mail]>

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 <[e-mail]>
Signed-off-by: Jonathan Cameron <[e-mail]>

---
v5:
* Rebase
* Update QMP Since entries to 8.1
---
 hw/mem/cxl_type3.c          | 111 ++++++++++++++++++++++++++++++++++++
 hw/mem/cxl_type3_stubs.c    |  10 ++++
 include/hw/cxl/cxl_events.h |  20 +++++++
 qapi/cxl.json               |  50 ++++++++++++++++
 4 files changed, 191 insertions(+)

Comments

Markus Armbruster May 22, 2023, 7:19 a.m. UTC | #1

Jonathan Cameron <[e-mail]> writes:

> From: Ira Weiny <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Jonathan Cameron <[e-mail]>

[...]

> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index ca3af3f0b2..9dcd308a49 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -5,6 +5,56 @@
>  # = CXL devices
>  ##
>  
> +##
> +# @CxlEventLog:
> +#
> +# CXL has a number of separate event logs for different types of event.

types of events

> +# Each such event log is handled and signaled independently.
> +#
> +# @informational: Information Event Log
> +# @warning: Warning Event Log
> +# @failure: Failure Event Log
> +# @fatal: Fatal Event Log

Are these proper nouns?  If not, the words should not be capitalized.

> +#
> +# Since: 8.1
> +##

Please format like

   ##
   # @CxlEventLog:
   #
   # CXL has a number of separate event logs for different types of
   # events.  Each such event log is handled and signaled independently.
   #
   # @informational: Information Event Log
   #
   # @warning: Warning Event Log
   #
   # @failure: Failure Event Log
   #
   # @fatal: Fatal Event Log
   #
   # Since: 8.1
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

The blank lines help with catching certain errors.  rST loves to
surprise...

> +{ 'enum': 'CxlEventLog',
> +  'data': ['informational',
> +           'warning',
> +           'failure',
> +           'fatal'
> +           ]

Make that
              'fatal']

> + }
> +
> +##
> +# @cxl-inject-gen-media-event:

Suggest cxl-inject-general-media-event, because we traditionally avoid
abbreviations in QMP, for better or worse.

> +#
> +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)

What's "CXL r3.0", and where could a reader find it?

Aside: the idea of a document with numbered section nested six levels
deep is kind of horrifying :)

Again, capitalize "General Media Event" only if it's a proper noun.  If
"CXL r3.0" capitalizes it this way, it is.

> +# This event type is reported via one of the event logs specified via
> +# the log parameter.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @log: Event Log to add the event to

event log

> +# @flags: header flags

Either specify the header flags here, or point to specification.

> +# @physaddr: Physical Address

Perhaps "Guest physical address"

Address of what?

We have no consistent naming convention for guest physical addresses.  I
see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
thing without need.

> +# @descriptor: Descriptor

No.

> +# @type: Type

No.

> +# @transaction-type: Transaction Type

No.

> +# @channel: Channel

No.

> +# @rank: Rank

No.

> +# @device: Device

No.

See, I can do too terse, too!

> +# @component-id: Device specific string

Probably no.

> +#
> +# Since: 8.1
> +##

Again, please format like

   ##
   # @cxl-inject-general-media-event:
   #
   # Inject an event record for a General Media Event (CXL r3.0
   # 8.2.9.2.1.1) This event type is reported via one of the event logs
   # specified via the log parameter.
   #
   # @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
   #
   # @transaction-type: Transaction Type
   #
   # @channel: Channel
   #
   # @rank: Rank
   #
   # @device: Device
   #
   # @component-id: Device specific string
   #
   # Since: 8.1
   ##

> +{ 'command': 'cxl-inject-gen-media-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
> +            'physaddr': 'uint64', 'descriptor': 'uint8',
> +            'type': 'uint8', 'transaction-type': 'uint8',
> +            '*channel': 'uint8', '*rank': 'uint8',
> +            '*device': 'uint32', '*component-id': 'str'
> +            }}

Make that

               '*device': 'uint32', '*component-id': 'str' } }

> +
>  ##
>  # @cxl-inject-poison:
>  #

Jonathan Cameron May 22, 2023, 12:57 p.m. UTC | #2

On Mon, 22 May 2023 09:19:57 +0200
Markus Armbruster <[e-mail]> wrote:

> Jonathan Cameron <[e-mail]> writes:
> 
> > From: Ira Weiny <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Jonathan Cameron <[e-mail]>  
> 
> [...]
> 
> > diff --git a/qapi/cxl.json b/qapi/cxl.json
> > index ca3af3f0b2..9dcd308a49 100644
> > --- a/qapi/cxl.json
> > +++ b/qapi/cxl.json
> > @@ -5,6 +5,56 @@
> >  # = CXL devices
> >  ##
> >  
> > +##
> > +# @CxlEventLog:
> > +#
> > +# CXL has a number of separate event logs for different types of event.  
> 
> types of events
> 
> > +# Each such event log is handled and signaled independently.
> > +#
> > +# @informational: Information Event Log
> > +# @warning: Warning Event Log
> > +# @failure: Failure Event Log
> > +# @fatal: Fatal Event Log  
> 
> Are these proper nouns?  If not, the words should not be capitalized.

By your definition below of them being capitalized in the CXL spec then
yes, they are all proper nouns.


...

> > +#
> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)  
> 
> What's "CXL r3.0", and where could a reader find it?

We have docs in docs/system/devices/cxl.rst that include the consortium
website which has download links on the front page.  I'm not sure we want to
have lots of references to the URL spread throughout QEMU.  I can add one
somewhere in cxl.json if you think it is important to have one here as well.

> 
> Aside: the idea of a document with numbered section nested six levels
> deep is kind of horrifying :)

Agreed!

> 
> Again, capitalize "General Media Event" only if it's a proper noun.  If
> "CXL r3.0" capitalizes it this way, it is.

It does capitalize it.

...

> 
> > +# @flags: header flags  
> 
> Either specify the header flags here, or point to specification.

Added a reference - same reason as below, the contents is being added to
with each version and we don't want to bake what is supported in this
interface if we can avoid it.

> 
> > +# @physaddr: Physical Address  
> 
> Perhaps "Guest physical address"
> 
> Address of what?

Changed already based on Phillipe's feedback on v6 to
Physical address (relative to @path device)

In CXL terms it's a Device Physical Address (DPA) which
are independent of the host (or guest) physical addresses with
runtime controllable mappings.
I'll change it to 

@dpa: Device Physical Address (relative to @path device)
(and Device Physical Address is capitalized like that in the CXL spec)

> 
> We have no consistent naming convention for guest physical addresses.  I
> see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
> thing without need.

It's none of the above (except may addr which is so vague)

I'll change to dpa.

Also added a note that some of the lower bits encode flags
Not this is probably why the spec uses a different name - Physical
Address  to distinguish this from DPA - I'll keep that naming in the
implementation of the record, but it's not needed in the injection
interface where I think DPA is less confusing.

> 
> > +# @descriptor: Descriptor  
> 
> No.

Ok this indeed ended up sparse.

It is a tricky balance as I don't think it makes sense to just
duplicate large chunks of the spec. 
I'll have a go at summarizing what sort of things are in each.
As I mention below, we could break, these down fully at the cost
of constant updates as the CXL spec evolves to add new subfields
or values for existing fields.  This one for example currently has
3 bits, Uncorrectable Event, Threshold Event, Poison List Overflow event.
The next one currently has 3 bits defined as well, but there are 3 more
queued up for inclusion.

Realistically no one is going to write a descriptor without
looking at the specification for the field definitions and understanding
the physical geometry of their device (which will be device specific).

I'm fine with tweaking the balance though if you think that makes sense.

Jonathan

Jonathan Cameron May 22, 2023, 1:53 p.m. UTC | #3

On Mon, 22 May 2023 13:57:37 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Mon, 22 May 2023 09:19:57 +0200
> Markus Armbruster <[e-mail]> wrote:
> 
> > Jonathan Cameron <[e-mail]> writes:
> >   
> > > From: Ira Weiny <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Jonathan Cameron <[e-mail]>    
> > 
> > [...]
> >   
> > > diff --git a/qapi/cxl.json b/qapi/cxl.json
> > > index ca3af3f0b2..9dcd308a49 100644
> > > --- a/qapi/cxl.json
> > > +++ b/qapi/cxl.json
> > > @@ -5,6 +5,56 @@
> > >  # = CXL devices
> > >  ##
> > >  
> > > +##
> > > +# @CxlEventLog:
> > > +#
> > > +# CXL has a number of separate event logs for different types of event.    
> > 
> > types of events
> >   
> > > +# Each such event log is handled and signaled independently.
> > > +#
> > > +# @informational: Information Event Log
> > > +# @warning: Warning Event Log
> > > +# @failure: Failure Event Log
> > > +# @fatal: Fatal Event Log    
> > 
> > Are these proper nouns?  If not, the words should not be capitalized.  
> 
> By your definition below of them being capitalized in the CXL spec then
> yes, they are all proper nouns.
> 
> 
> ...
> 
> > > +#
> > > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)    
> > 
> > What's "CXL r3.0", and where could a reader find it?  
> 
> We have docs in docs/system/devices/cxl.rst that include the consortium
> website which has download links on the front page.  I'm not sure we want to
> have lots of references to the URL spread throughout QEMU.  I can add one
> somewhere in cxl.json if you think it is important to have one here as well.

FWIW I tried adding some top level docs by adding stuff directly under the
 = CXL devices 
at the top of the file and building the html docs.

Looks fine in the actual page, but is picked up in the index generation as
a heading we should be able to link to alongside the CxlEventLog and
command definitions.

So, I'll not do that for now.

Jonathan

> 
> > 
> > Aside: the idea of a document with numbered section nested six levels
> > deep is kind of horrifying :)  
> 
> Agreed!
> 
> > 
> > Again, capitalize "General Media Event" only if it's a proper noun.  If
> > "CXL r3.0" capitalizes it this way, it is.  
> 
> It does capitalize it.
> 
> ...
> 
> >   
> > > +# @flags: header flags    
> > 
> > Either specify the header flags here, or point to specification.  
> 
> Added a reference - same reason as below, the contents is being added to
> with each version and we don't want to bake what is supported in this
> interface if we can avoid it.
> 
> >   
> > > +# @physaddr: Physical Address    
> > 
> > Perhaps "Guest physical address"
> > 
> > Address of what?  
> 
> Changed already based on Phillipe's feedback on v6 to
> Physical address (relative to @path device)
> 
> In CXL terms it's a Device Physical Address (DPA) which
> are independent of the host (or guest) physical addresses with
> runtime controllable mappings.
> I'll change it to 
> 
> @dpa: Device Physical Address (relative to @path device)
> (and Device Physical Address is capitalized like that in the CXL spec)
> 
> > 
> > We have no consistent naming convention for guest physical addresses.  I
> > see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
> > thing without need.  
> 
> It's none of the above (except may addr which is so vague)
> 
> I'll change to dpa.
> 
> Also added a note that some of the lower bits encode flags
> Not this is probably why the spec uses a different name - Physical
> Address  to distinguish this from DPA - I'll keep that naming in the
> implementation of the record, but it's not needed in the injection
> interface where I think DPA is less confusing.
> 
> >   
> > > +# @descriptor: Descriptor    
> > 
> > No.  
> 
> Ok this indeed ended up sparse.
> 
> It is a tricky balance as I don't think it makes sense to just
> duplicate large chunks of the spec. 
> I'll have a go at summarizing what sort of things are in each.
> As I mention below, we could break, these down fully at the cost
> of constant updates as the CXL spec evolves to add new subfields
> or values for existing fields.  This one for example currently has
> 3 bits, Uncorrectable Event, Threshold Event, Poison List Overflow event.
> The next one currently has 3 bits defined as well, but there are 3 more
> queued up for inclusion.
> 
> Realistically no one is going to write a descriptor without
> looking at the specification for the field definitions and understanding
> the physical geometry of their device (which will be device specific).
> 
> I'm fine with tweaking the balance though if you think that makes sense.
> 
> Jonathan
> 
> 
> 
>

Markus Armbruster May 23, 2023, 8:10 a.m. UTC | #4

Jonathan Cameron <[e-mail]> writes:

> On Mon, 22 May 2023 09:19:57 +0200
> Markus Armbruster <[e-mail]> wrote:
>
>> Jonathan Cameron <[e-mail]> writes:
>> 
>> > From: Ira Weiny <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > Signed-off-by: Jonathan Cameron <[e-mail]>  
>> 
>> [...]
>> 
>> > diff --git a/qapi/cxl.json b/qapi/cxl.json
>> > index ca3af3f0b2..9dcd308a49 100644
>> > --- a/qapi/cxl.json
>> > +++ b/qapi/cxl.json
>> > @@ -5,6 +5,56 @@
>> >  # = CXL devices
>> >  ##
>> >  
>> > +##
>> > +# @CxlEventLog:
>> > +#
>> > +# CXL has a number of separate event logs for different types of event.  
>> 
>> types of events
>> 
>> > +# Each such event log is handled and signaled independently.
>> > +#
>> > +# @informational: Information Event Log
>> > +# @warning: Warning Event Log
>> > +# @failure: Failure Event Log
>> > +# @fatal: Fatal Event Log  
>> 
>> Are these proper nouns?  If not, the words should not be capitalized.
>
> By your definition below of them being capitalized in the CXL spec then
> yes, they are all proper nouns.

Good.

> ...
>
>> > +#
>> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)  
>> 
>> What's "CXL r3.0", and where could a reader find it?
>
> We have docs in docs/system/devices/cxl.rst that include the consortium
> website which has download links on the front page.

cxl.rst has

    References
    ----------

     - Consortium website for specifications etc:
       http://www.computeexpresslink.org
     - Compute Express link Revision 2 specification, October 2020
     - CEDT CFMWS & QTG _DSM ECN May 2021

Should the second reference be updated to 3.0?  Exact title seems to be
"The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
to bother with the "™" in a reference.

>                                                      I'm not sure we want to
> have lots of references to the URL spread throughout QEMU.  I can add one
> somewhere in cxl.json if you think it is important to have one here as well.

You could add an introduction right under the "# = CXL devices" heading,
and include a full reference to the specification there.  Suitably
abbreviated references like the ones you use in this patch should then
be fine.

Please link to cxl.rst, too: add a label to cxl.rst, :ref: it from
cxl.json.

>> Aside: the idea of a document with numbered section nested six levels
>> deep is kind of horrifying :)
>
> Agreed!
>
>> 
>> Again, capitalize "General Media Event" only if it's a proper noun.  If
>> "CXL r3.0" capitalizes it this way, it is.
>
> It does capitalize it.
>
> ...
>
>> 
>> > +# @flags: header flags  
>> 
>> Either specify the header flags here, or point to specification.
>
> Added a reference - same reason as below, the contents is being added to
> with each version and we don't want to bake what is supported in this
> interface if we can avoid it.

Symbolic flags are a much friendlier interface, but limit the interface
to what QEMU understands.

With a numeric encoding of flags, QEMU can serve as dumb transport
between peers who may understand more flags than QEMU does.  One peer is
the QMP client.  Who is the other peer?  Guest software?

Can flags be useful even though the QEMU device model doesn't understand
them?  Are they safe?  See below for a more general take on this.

>> > +# @physaddr: Physical Address  
>> 
>> Perhaps "Guest physical address"
>> 
>> Address of what?
>
> Changed already based on Phillipe's feedback on v6 to
> Physical address (relative to @path device)
>
> In CXL terms it's a Device Physical Address (DPA) which
> are independent of the host (or guest) physical addresses with
> runtime controllable mappings.
> I'll change it to 
>
> @dpa: Device Physical Address (relative to @path device)
> (and Device Physical Address is capitalized like that in the CXL spec)

Better, because the difference to guest physical address is now clear.

>> We have no consistent naming convention for guest physical addresses.  I
>> see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
>> thing without need.
>
> It's none of the above (except may addr which is so vague)
>
> I'll change to dpa.
>
> Also added a note that some of the lower bits encode flags
> Not this is probably why the spec uses a different name - Physical
> Address  to distinguish this from DPA - I'll keep that naming in the
> implementation of the record, but it's not needed in the injection
> interface where I think DPA is less confusing.
>
>> 
>> > +# @descriptor: Descriptor  
>> 
>> No.
>
> Ok this indeed ended up sparse.
>
> It is a tricky balance as I don't think it makes sense to just
> duplicate large chunks of the spec. 
> I'll have a go at summarizing what sort of things are in each.
> As I mention below, we could break, these down fully at the cost
> of constant updates as the CXL spec evolves to add new subfields
> or values for existing fields.  This one for example currently has
> 3 bits, Uncorrectable Event, Threshold Event, Poison List Overflow event.
> The next one currently has 3 bits defined as well, but there are 3 more
> queued up for inclusion.
>
> Realistically no one is going to write a descriptor without
> looking at the specification for the field definitions and understanding
> the physical geometry of their device (which will be device specific).
>
> I'm fine with tweaking the balance though if you think that makes sense.

This is about picking an appropriate level of abstraction for the QMP
interface.

In your patch, it is basically a few named sequences of bits.  The
interface changes only when new named entities get added to the spec.
Spec revisions may also add new uses of existing entities' bits, but the
interface doesn't care.

The lowest imaginable level is a single sequence of bits.  Basically the
named bit sequences pasted together.  Now the interface changes only
when we run out of bits.  Mind, I'm merely exploring the limits here.

At higher levels we use symbols rather than bits.  This interface needs
to change when symbols get added to the spec.

I figure the deciding question is the QEMU device model's role in all
this.

When something can be used safely only when the device model knows it,
providing a symbolic interface doesn't add to the things QEMU needs to
know.  Moreover, the interface can't be misused.

For things where the device model acts as a dumb transport, i.e. only
management application and guest need to know it, not having to put he
knowledge into QEMU just to enable it to transport bits makes some
sense.  It may enable misuse.

So, can you tell us a bit more about what the device model needs to
know to function?

Markus Armbruster May 23, 2023, 12:46 p.m. UTC | #5

Jonathan Cameron <[e-mail]> writes:

>> >  
>> >> > +#
>> >> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)    
>> >> 
>> >> What's "CXL r3.0", and where could a reader find it?  
>> >
>> > We have docs in docs/system/devices/cxl.rst that include the consortium
>> > website which has download links on the front page.  
>> 
>> cxl.rst has
>> 
>>     References
>>     ----------
>> 
>>      - Consortium website for specifications etc:
>>        http://www.computeexpresslink.org
>>      - Compute Express link Revision 2 specification, October 2020
>>      - CEDT CFMWS & QTG _DSM ECN May 2021
>> 
>> Should the second reference be updated to 3.0?  Exact title seems to be
>> "The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
>> to bother with the "™" in a reference.
>
> Yes. On the todo list is to update all the references to latest released
> specification because old ones are unobtainable to non consortium members
> unless they grabbed a copy in the past.
>
> Annoyingly this will be a repeated requirement as new spec versions are released
> but the cadence should be fairly low.
>
>> 
>> >                                                      I'm not sure we want to
>> > have lots of references to the URL spread throughout QEMU.  I can add one
>> > somewhere in cxl.json if you think it is important to have one here as well.  
>> 
>> You could add an introduction right under the "# = CXL devices" heading,
>> and include a full reference to the specification there.  Suitably
>> abbreviated references like the ones you use in this patch should then
>> be fine.
>
> I tried doing that - it resulted in the index including an entry with all the text.
> So on the webpage, the contents list to the left includes whatever text you put
> in that block.
>  
> I'm not sure why, or how to fix that.

Show me what you tried (as a patch or something I can git-pull), and
I'll have a look.

[...]

Jonathan Cameron May 24, 2023, 9:11 a.m. UTC | #6

On Tue, 23 May 2023 14:46:37 +0200
Markus Armbruster <[e-mail]> wrote:

> Jonathan Cameron <[e-mail]> writes:
> 
> >> >    
> >> >> > +#
> >> >> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)      
> >> >> 
> >> >> What's "CXL r3.0", and where could a reader find it?    
> >> >
> >> > We have docs in docs/system/devices/cxl.rst that include the consortium
> >> > website which has download links on the front page.    
> >> 
> >> cxl.rst has
> >> 
> >>     References
> >>     ----------
> >> 
> >>      - Consortium website for specifications etc:
> >>        http://www.computeexpresslink.org
> >>      - Compute Express link Revision 2 specification, October 2020
> >>      - CEDT CFMWS & QTG _DSM ECN May 2021
> >> 
> >> Should the second reference be updated to 3.0?  Exact title seems to be
> >> "The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
> >> to bother with the "™" in a reference.  
> >
> > Yes. On the todo list is to update all the references to latest released
> > specification because old ones are unobtainable to non consortium members
> > unless they grabbed a copy in the past.
> >
> > Annoyingly this will be a repeated requirement as new spec versions are released
> > but the cadence should be fairly low.
> >  
> >>   
> >> >                                                      I'm not sure we want to
> >> > have lots of references to the URL spread throughout QEMU.  I can add one
> >> > somewhere in cxl.json if you think it is important to have one here as well.    
> >> 
> >> You could add an introduction right under the "# = CXL devices" heading,
> >> and include a full reference to the specification there.  Suitably
> >> abbreviated references like the ones you use in this patch should then
> >> be fine.  
> >
> > I tried doing that - it resulted in the index including an entry with all the text.
> > So on the webpage, the contents list to the left includes whatever text you put
> > in that block.
> >  
> > I'm not sure why, or how to fix that.  
> 
> Show me what you tried (as a patch or something I can git-pull), and
> I'll have a look.
> 
> [...]
> 
Very simple to replicate the issue - see below.

Result of this is an extra
"Test text" entry in the index to the left of the html docs.

In qemu-qmp-ref.html we have

<li class="toctree-l3"><a class="reference internal" href="#qapidoc-3297">CXL devices</a><ul>
<li class="toctree-l4"><a class="reference internal" href="#qapidoc-3298">Test text</a></li>
<li class="toctree-l4"><a class="reference internal" href="#qapidoc-3301"><code class="docutils literal notranslate"><span class="pre">CxlEventLog</span></code> (Enum)</a></li>

diff --git a/qapi/cxl.json b/qapi/cxl.json
index 05c560cfe5..9f7f0485ae 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -3,6 +3,8 @@

 ##
 # = CXL devices
+#
+# Test text
 ##

 ##

Jonathan Cameron May 24, 2023, 9:20 a.m. UTC | #7

On Wed, 24 May 2023 10:11:58 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Tue, 23 May 2023 14:46:37 +0200
> Markus Armbruster <[e-mail]> wrote:
> 
> > Jonathan Cameron <[e-mail]> writes:
> > 
> > >> >    
> > >> >> > +#
> > >> >> > +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)      
> > >> >> 
> > >> >> What's "CXL r3.0", and where could a reader find it?    
> > >> >
> > >> > We have docs in docs/system/devices/cxl.rst that include the consortium
> > >> > website which has download links on the front page.    
> > >> 
> > >> cxl.rst has
> > >> 
> > >>     References
> > >>     ----------
> > >> 
> > >>      - Consortium website for specifications etc:
> > >>        http://www.computeexpresslink.org
> > >>      - Compute Express link Revision 2 specification, October 2020
> > >>      - CEDT CFMWS & QTG _DSM ECN May 2021
> > >> 
> > >> Should the second reference be updated to 3.0?  Exact title seems to be
> > >> "The Compute Express Link™ (CXL™) 3.0 specification".  Not sure we need
> > >> to bother with the "™" in a reference.  
> > >
> > > Yes. On the todo list is to update all the references to latest released
> > > specification because old ones are unobtainable to non consortium members
> > > unless they grabbed a copy in the past.
> > >
> > > Annoyingly this will be a repeated requirement as new spec versions are released
> > > but the cadence should be fairly low.
> > >  
> > >>   
> > >> >                                                      I'm not sure we want to
> > >> > have lots of references to the URL spread throughout QEMU.  I can add one
> > >> > somewhere in cxl.json if you think it is important to have one here as well.    
> > >> 
> > >> You could add an introduction right under the "# = CXL devices" heading,
> > >> and include a full reference to the specification there.  Suitably
> > >> abbreviated references like the ones you use in this patch should then
> > >> be fine.  
> > >
> > > I tried doing that - it resulted in the index including an entry with all the text.
> > > So on the webpage, the contents list to the left includes whatever text you put
> > > in that block.
> > >  
> > > I'm not sure why, or how to fix that.  
> > 
> > Show me what you tried (as a patch or something I can git-pull), and
> > I'll have a look.
> > 
> > [...]
> > 
> Very simple to replicate the issue - see below.
> 
> Result of this is an extra
> "Test text" entry in the index to the left of the html docs.
> 
> In qemu-qmp-ref.html we have
> 
> <li class="toctree-l3"><a class="reference internal" href="#qapidoc-3297">CXL devices</a><ul>
> <li class="toctree-l4"><a class="reference internal" href="#qapidoc-3298">Test text</a></li>
> <li class="toctree-l4"><a class="reference internal" href="#qapidoc-3301"><code class="docutils literal notranslate"><span class="pre">CxlEventLog</span></code> (Enum)</a></li>

Experimented and dived in the code a bit more and got reminded of how bad my python is
(which is why I was avoiding looking at this :)
One option would be to give the extra text a meaningful title so that ends up in the contents list.

> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -3,6 +3,8 @@
> 
>  ##
>  # = CXL devices
> +#
> +# CXL Background
> +#
> +# Actually useful text.
>  ##
> 
>  ##

Can't do
== CXL Background

though to get it nicely formatted as then we loose all the actual entries from the
contents list.  I guess they dropped down one too many levels.

> 
> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index 05c560cfe5..9f7f0485ae 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -3,6 +3,8 @@
> 
>  ##
>  # = CXL devices
> +#
> +# Test text
>  ##
> 
>  ##

Patch

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 7ea38cadf6..f5a604975e 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1181,6 +1181,117 @@  void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
     pcie_aer_inject_error(PCI_DEVICE(obj), &err);
 }
 
+static void cxl_assign_event_header(CXLEventRecordHdr *hdr,
+                                    const QemuUUID *uuid, uint32_t flags,
+                                    uint8_t length, uint64_t timestamp)
+{
+    st24_le_p(&hdr->flags, flags);
+    hdr->length = length;
+    memcpy(&hdr->id, uuid, sizeof(hdr->id));
+    stq_le_p(&hdr->timestamp, timestamp);
+}
+
+static const 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)
+
+static int ct3d_qmp_cxl_event_log_enc(CxlEventLog log)
+{
+    switch (log) {
+    case CXL_EVENT_LOG_INFORMATIONAL:
+        return CXL_EVENT_TYPE_INFO;
+    case CXL_EVENT_LOG_WARNING:
+        return CXL_EVENT_TYPE_WARN;
+    case CXL_EVENT_LOG_FAILURE:
+        return CXL_EVENT_TYPE_FAIL;
+    case CXL_EVENT_LOG_FATAL:
+        return CXL_EVENT_TYPE_FATAL;
+/* DCD not yet supported */
+    default:
+        return -EINVAL;
+    }
+}
+/* Component ID is device specific.  Define this as a string. */
+void qmp_cxl_inject_gen_media_event(const char *path, CxlEventLog log,
+                                    uint8_t flags, uint64_t physaddr,
+                                    uint8_t descriptor, uint8_t type,
+                                    uint8_t transaction_type,
+                                    bool has_channel, uint8_t channel,
+                                    bool has_rank, uint8_t rank,
+                                    bool has_device, uint32_t device,
+                                    const char *component_id,
+                                    Error **errp)
+{
+    Object *obj = object_resolve_path(path, NULL);
+    CXLEventGenMedia gem;
+    CXLEventRecordHdr *hdr = &gem.hdr;
+    CXLDeviceState *cxlds;
+    CXLType3Dev *ct3d;
+    uint16_t valid_flags = 0;
+    uint8_t enc_log;
+    int rc;
+
+    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");
+        return;
+    }
+    ct3d = CXL_TYPE3(obj);
+    cxlds = &ct3d->cxl_dstate;
+
+    rc = ct3d_qmp_cxl_event_log_enc(log);
+    if (rc < 0) {
+        error_setg(errp, "Unhandled error log type");
+        return;
+    }
+    enc_log = rc;
+
+    memset(&gem, 0, sizeof(gem));
+    cxl_assign_event_header(hdr, &gen_media_uuid, flags, sizeof(gem),
+                            cxl_device_get_timestamp(&ct3d->cxl_dstate));
+
+    stq_le_p(&gem.phys_addr, physaddr);
+    gem.descriptor = descriptor;
+    gem.type = type;
+    gem.transaction_type = transaction_type;
+
+    if (has_channel) {
+        gem.channel = channel;
+        valid_flags |= CXL_GMER_VALID_CHANNEL;
+    }
+
+    if (has_rank) {
+        gem.rank = rank;
+        valid_flags |= CXL_GMER_VALID_RANK;
+    }
+
+    if (has_device) {
+        st24_le_p(gem.device, device);
+        valid_flags |= CXL_GMER_VALID_DEVICE;
+    }
+
+    if (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, enc_log, (CXLEventRecordRaw *)&gem)) {
+        cxl_event_irq_assert(ct3d);
+    }
+}
+
 static void ct3_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index fd1166a610..55d19b0e03 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -3,6 +3,16 @@ 
 #include "qapi/error.h"
 #include "qapi/qapi-commands-cxl.h"
 
+void qmp_cxl_inject_gen_media_event(const char *path, CxlEventLog log,
+                                    uint8_t flags, uint64_t physaddr,
+                                    uint8_t descriptor, uint8_t type,
+                                    uint8_t transaction_type,
+                                    bool has_channel, uint8_t channel,
+                                    bool has_rank, uint8_t rank,
+                                    bool has_device, uint32_t device,
+                                    const char *component_id,
+                                    Error **errp) {}
+
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
                            Error **errp)
 {
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 4bf8b7aa08..b189193f4c 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -103,4 +103,24 @@  typedef struct CXLEventInterruptPolicy {
 /* 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
+typedef struct CXLEventGenMedia {
+    CXLEventRecordHdr hdr;
+    uint64_t phys_addr;
+    uint8_t descriptor;
+    uint8_t type;
+    uint8_t transaction_type;
+    uint16_t validity_flags;
+    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 CXLEventGenMedia;
+
 #endif /* CXL_EVENTS_H */
diff --git a/qapi/cxl.json b/qapi/cxl.json
index ca3af3f0b2..9dcd308a49 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -5,6 +5,56 @@ 
 # = CXL devices
 ##
 
+##
+# @CxlEventLog:
+#
+# CXL has a number of separate event logs for different types of event.
+# Each such event log is handled and signaled independently.
+#
+# @informational: Information Event Log
+# @warning: Warning Event Log
+# @failure: Failure Event Log
+# @fatal: Fatal Event Log
+#
+# Since: 8.1
+##
+{ 'enum': 'CxlEventLog',
+  'data': ['informational',
+           'warning',
+           'failure',
+           'fatal'
+           ]
+ }
+
+##
+# @cxl-inject-gen-media-event:
+#
+# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)
+# This event type is reported via one of the event logs specified via
+# the log parameter.
+#
+# @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
+# @transaction-type: Transaction Type
+# @channel: Channel
+# @rank: Rank
+# @device: Device
+# @component-id: Device specific string
+#
+# Since: 8.1
+##
+{ 'command': 'cxl-inject-gen-media-event',
+  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
+            'physaddr': 'uint64', 'descriptor': 'uint8',
+            'type': 'uint8', 'transaction-type': 'uint8',
+            '*channel': 'uint8', '*rank': 'uint8',
+            '*device': 'uint32', '*component-id': 'str'
+            }}
+
 ##
 # @cxl-inject-poison:
 #