Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents

[Markus Armbruster <armbru@redhat.com>]

fan <nifan.cxl@gmail.com> writes:

> On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote:
>> nifan.cxl@gmail.com writes:
>> 
>> > From: Fan Ni <fan.ni@samsung.com>
>> >
>> > Since fabric manager emulation is not supported yet, the change implements
>> > the functions to add/release dynamic capacity extents as QMP interfaces.
>> 
>> Will fabric manager emulation obsolete these commands?
>
> If in the future, fabric manager emulation supports commands for dynamic capacity
> extent add/release, it is possible we do not need the commands.
> But it seems not to happen soon, we need the qmp commands for the
> end-to-end test with kernel DCD support.

I asked because if the commands are temporary testing aids, they should
probably be declared unstable.  Even if they are permanent testing aids,
unstable might be the right choice.  This is for the CXL maintainers to
decide.

What does "unstable" mean?  docs/devel/qapi-code-gen.rst: "Interfaces so
marked may be withdrawn or changed incompatibly in future releases."

Management applications need stable interfaces.  Libvirt developers
generally refuse to touch anything in QMP that's declared unstable.

Human users and their ad hoc scripts appreciate stability, but they
don't need it nearly as much as management applications do.

A stability promise increases the maintenance burden.  By how much is
unclear.  In other words, by promising stability, the maintainers take
on risk.  Are the CXL maintainers happy to accept the risk here?

>> > Note: we skips any FM issued extent release request if the exact extent
>> > does not exist in the extent list of the device. We will loose the
>> > restriction later once we have partial release support in the kernel.
>> >
>> > 1. Add dynamic capacity extents:
>> >
>> > For example, the command to add two continuous extents (each 128MiB long)
>> > to region 0 (starting at DPA offset 0) looks like below:
>> >
>> > { "execute": "qmp_capabilities" }
>> >
>> > { "execute": "cxl-add-dynamic-capacity",
>> >   "arguments": {
>> >       "path": "/machine/peripheral/cxl-dcd0",
>> >       "region-id": 0,
>> >       "extents": [
>> >       {
>> >           "dpa": 0,
>> >           "len": 134217728
>> >       },
>> >       {
>> >           "dpa": 134217728,
>> >           "len": 134217728
>> >       }
>> >       ]
>> >   }
>> > }
>> >
>> > 2. Release dynamic capacity extents:
>> >
>> > For example, the command to release an extent of size 128MiB from region 0
>> > (DPA offset 128MiB) look like below:
>> >
>> > { "execute": "cxl-release-dynamic-capacity",
>> >   "arguments": {
>> >       "path": "/machine/peripheral/cxl-dcd0",
>> >       "region-id": 0,
>> >       "extents": [
>> >       {
>> >           "dpa": 134217728,
>> >           "len": 134217728
>> >       }
>> >       ]
>> >   }
>> > }
>> >
>> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/cxl.json b/qapi/cxl.json
>> > index 8cc4c72fa9..2645004666 100644
>> > --- a/qapi/cxl.json
>> > +++ b/qapi/cxl.json
>> > @@ -19,13 +19,16 @@
>> >  #
>> >  # @fatal: Fatal Event Log
>> >  #
>> > +# @dyncap: Dynamic Capacity Event Log
>> > +#
>> >  # Since: 8.1
>> >  ##
>> >  { 'enum': 'CxlEventLog',
>> >    'data': ['informational',
>> >             'warning',
>> >             'failure',
>> > -           'fatal']
>> > +           'fatal',
>> > +           'dyncap']
>> 
>> We tend to avoid abbreviations in QMP identifiers: dynamic-capacity.
>
> FYI. This has been removed to avoid the potential side effect in the
> latest post.
> v7: https://lore.kernel.org/linux-cxl/ZiaFYUB6FC9NR7W4@memverge.com/T/#t
>
>> 
>> >   }
>> >  
>> >  ##
>> > @@ -361,3 +364,59 @@
>> >  ##
>> >  {'command': 'cxl-inject-correctable-error',
>> >   'data': {'path': 'str', 'type': 'CxlCorErrorType'}}
>> > +
>> > +##
>> > +# @CXLDCExtentRecord:
>> 
>> Such traffic jams of capital letters are hard to read.
>> 
>> What does DC mean?
>
> Dynamic capacity

Suggest CxlDynamicCapacityExtent.

>> > +#
>> > +# Record of a single extent to add/release
>> > +#
>> > +# @offset: offset to the start of the region where the extent to be operated
>> 
>> Blank line here, please
>> 
>> > +# @len: length of the extent
>> > +#
>> > +# Since: 9.0
>> > +##
>> > +{ 'struct': 'CXLDCExtentRecord',
>> > +  'data': {
>> > +      'offset':'uint64',
>> > +      'len': 'uint64'
>> > +  }
>> > +}
>> > +
>> > +##
>> > +# @cxl-add-dynamic-capacity:
>> > +#
>> > +# Command to start add dynamic capacity extents flow. The device will
>> 
>> I think we're missing an article here.  Is it "a flow" or "the flow"?
>> 
>> > +# have to acknowledged the acceptance of the extents before they are usable.
>> 
>> to acknowledge
>
> It should be "to be acknowledged". 
>
>> 
>> docs/devel/qapi-code-gen.rst:
>> 
>>     For legibility, wrap text paragraphs so every line is at most 70
>>     characters long.
>> 
>>     Separate sentences with two spaces.
>
> Thanks. Will fix.
>> 
>> > +#
>> > +# @path: CXL DCD canonical QOM path
>> 
>> What is a CXL DCD?  Is it a device?
>
> Dynamic capacity device.
> Yes. It is cxl memory device that can change capacity dynamically.

Sure the QOM path needs to be canonical?

If not, what about "path to the CXL dynamic capacity device in the QOM
tree".  Intentionally close to existing descriptions of @qom-path
elsewhere.

>> I'd prefer @qom-path, unless you can make a consistency argument for
>> @path.
>> 
>> > +# @region-id: id of the region where the extent to add
>> 
>> What's a region, and how do they get their IDs?
>
> Each DCD device can support up to 8 regions (0-7).  

Is "region ID" the established terminology in CXL-land?  Or is "region
number" also used?  I'm asking because "ID" in this QEMU device context
suggests a connection to a qdev ID.

If region number is fine, I'd rename to just @region, and rephrase the
description to avoid "ID".  Perhaps "number of the region the extent is
to be added to".  Not entirely happy with the phrasing, doesn't exactly
roll off the tongue, but "where the extent to add" sounds worse to my
ears.  Mind, I'm not a native speaker.

>> > +# @extents: Extents to add
>> 
>> Blank lines between argument descriptions, please.
>> 
>> > +#
>> > +# Since : 9.0
>> 
>> 9.1
>
> Already fixed in the latest post.
>
>> 
>> > +##
>> > +{ 'command': 'cxl-add-dynamic-capacity',
>> > +  'data': { 'path': 'str',
>> > +            'region-id': 'uint8',
>> > +            'extents': [ 'CXLDCExtentRecord' ]
>> > +           }
>> > +}
>> > +
>> > +##
>> > +# @cxl-release-dynamic-capacity:
>> > +#
>> > +# Command to start release dynamic capacity extents flow. The host will
>> 
>> Article again.
>> 
>> The host?  In cxl-add-dynamic-capacity's doc comment, it's the device.
>
> For add command, the host will send a mailbox command to response to the add
> request to the device to indicate whether it accepts the add capacity offer
> or not.
>
> For release command, the host send a mailbox command (not always a response
> since the host can proactively release capacity if it does not need it
> any more) to device to ask device release the capacity.
>
> But yes, the text needs to be polished.

Please do.  You may have to briefly explain which peer initiates what
for this to make sense.

>> > +# need to respond to indicate that it has released the capacity before it
>> > +# is made unavailable for read and write and can be re-added.
>> 
>> Is "and can be re-added" relevant here?
>
> Not really. Will fix.
>
>> 
>> > +#
>> > +# @path: CXL DCD canonical QOM path
>> > +# @region-id: id of the region where the extent to release
>> > +# @extents: Extents to release
>> > +#
>> > +# Since : 9.0
>> 
>> 9.1
>
> Already fixed in the latest post.
>
> Thanks again for the review. Will take care of the comments in the next
> version.

You're welcome!

> Fan
>> 
>> > +##
>> > +{ 'command': 'cxl-release-dynamic-capacity',
>> > +  'data': { 'path': 'str',
>> > +            'region-id': 'uint8',
>> > +            'extents': [ 'CXLDCExtentRecord' ]
>> > +           }
>> > +}
>>