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