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

[Markus Armbruster]

Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:

> On Mon, 22 May 2023 09:19:57 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
>> 
>> > From: Ira Weiny <ira.weiny@intel.com>
>> >
>> > 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>
>> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
>> 
>> [...]
>> 
>> > 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?