| Message ID | 20230423165140.16833-6-Jonathan.Cameron@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | QEMU CXL Provide mock CXL events and irq support | expand |
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. > > 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
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 > > > >
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?
> > > >> > +# > >> > +# 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. > > Please link to cxl.rst, too: add a label to cxl.rst, :ref: it from > cxl.json. Ok, I'm find doing that if it doesn't break the contents page as above. > >> 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? Guest software in this case. > > 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. > > >> 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? > For the event record injection, this is definitely the dumb transport case. Given a real device firmware might well create records that aren't 'valid' guest software has to be written with that in mind. Also, additional information can be added to these records in reserved bits and the guest software has to deal with that (+ the spec change has to be written so that it is backwards compatible). For the case of Events, I don't think there is anything the device model needs to know (beyond the 'which device question') That is different from the poison case in the other series, where at some stage we will emulate the other way of getting poison (normal CPU read of poisoned memory) and need to be able to use the provided address and length to implement that. Note that a future series will tie together generating some of these event records as other things happen - e.g. poison injection. This will be similar to how poison list elements are added either by QMP (to simulate real hardware failures) and via Guest Software using a mailbox on the device (to simulate software driven error injection sequences). Jonathan
Jonathan Cameron <Jonathan.Cameron@Huawei.com> 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. [...]
On Tue, 23 May 2023 14:46:37 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Jonathan Cameron <Jonathan.Cameron@Huawei.com> 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 ## ##
On Wed, 24 May 2023 10:11:58 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Tue, 23 May 2023 14:46:37 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> 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 > ## > > ##
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: #