On Wed, 24 Apr 2024 10:33:33 -0700
Ira Weiny <ira.we...@intel.com> wrote:
> Markus Armbruster wrote:
> > nifan....@gmail.com writes:
> >
> > > From: Fan Ni <fan...@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?
>
> I don't think so. In the development of the kernel, I see these being
> valuable to do CI and regression testing without the complexity of an FM.
Fully agree - I also long term see these as the drivers for one
possible virtualization stack for DCD devices (whether it turns
out to be the way forwards for that is going to take a while to
resolve!)
It doesn't make much sense to add a fabric manager into that flow
or to expose an appropriate (maybe MCTP) interface from QEMU just
to poke the emulated device.
Jonathan
>
> Ira
>
> >
> > > 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...@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.
> >
> > > }
> > >
> > > ##
> > > @@ -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?
> >
> > > +#
> > > +# 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
> >
> > 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.
> >
> > > +#
> > > +# @path: CXL DCD canonical QOM path
> >
> > What is a CXL DCD? Is it a device?
> >
> > 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?
> >
> > > +# @extents: Extents to add
> >
> > Blank lines between argument descriptions, please.
> >
> > > +#
> > > +# Since : 9.0
> >
> > 9.1
> >
> > > +##
> > > +{ '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.
> >
> > > +# 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?
> >
> > > +#
> > > +# @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
> >
> > > +##
> > > +{ 'command': 'cxl-release-dynamic-capacity',
> > > + 'data': { 'path': 'str',
> > > + 'region-id': 'uint8',
> > > + 'extents': [ 'CXLDCExtentRecord' ]
> > > + }
> > > +}
> >
>
>
