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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 22 May 2023 13:57:37 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> 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.
> 
> 
> ...
> 
> > > +#
> > > +# 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
> 
> 
> 
>