On Mon, May 20, 2024 at 05:50:12PM +0100, Jonathan Cameron wrote:
> On Wed, 1 May 2024 15:29:31 -0700
> fan <[email protected]> wrote:
>
> > From 873f59ec06c38645768ada452d9b18920a34723e Mon Sep 17 00:00:00 2001
> > From: Fan Ni <[email protected]>
> > Date: Tue, 20 Feb 2024 09:48:31 -0800
> > Subject: [PATCH] hw/cxl/events: Add qmp interfaces to add/release dynamic
> > capacity extents
> > Status: RO
> > Content-Length: 25172
> > Lines: 731
> >
> > To simulate FM functionalities for initiating Dynamic Capacity Add
> > (Opcode 5604h) and Dynamic Capacity Release (Opcode 5605h) as in CXL spec
> > r3.1 7.6.7.6.5 and 7.6.7.6.6, we implemented two QMP interfaces to issue
> > add/release dynamic capacity extents requests.
> >
> > With the change, we allow to release an extent only when its DPA range
> > is contained by a single accepted extent in the device. That is to say,
> > extent superset release is not supported yet.
> >
> > 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",
> > "host-id": 0,
> > "selection-policy": 2,
> > "region": 0,
> > "tag": "",
> > "extents": [
> > {
> > "offset": 0,
> > "len": 134217728
> > },
> > {
> > "offset": 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) looks like below:
> >
> > { "execute": "cxl-release-dynamic-capacity",
> > "arguments": {
> > "path": "/machine/peripheral/cxl-dcd0",
> > "host-id": 0,
> > "flags": 1,
> > "region": 0,
> > "tag": "",
> > "extents": [
> > {
> > "offset": 134217728,
> > "len": 134217728
> > }
> > ]
> > }
> > }
> >
> > Signed-off-by: Fan Ni <[email protected]>
>
> Hi Fan,
>
> A few trivial questions inline. I don't feel particularly strongly
> about breaking up the flags fields, but I'd like to understand your
> reasoning for keeping them as single fields?
>
> Is it mainly to keep aligned with the specification or something else?
>
> Thanks,
>
> Jonathan
>
>
> > #endif /* CXL_EVENTS_H */
> > diff --git a/qapi/cxl.json b/qapi/cxl.json
> > index 4281726dec..27cf39f448 100644
> > --- a/qapi/cxl.json
> > +++ b/qapi/cxl.json
> > @@ -361,3 +361,93 @@
> > ##
> > {'command': 'cxl-inject-correctable-error',
> > 'data': {'path': 'str', 'type': 'CxlCorErrorType'}}
> > +
> > +##
> > +# @CXLDynamicCapacityExtent:
> > +#
> > +# A single dynamic capacity extent
> > +#
> > +# @offset: The offset (in bytes) to the start of the region
> > +# where the extent belongs to
> > +#
> > +# @len: The length of the extent in bytes
> > +#
> > +# Since: 9.1
> > +##
> > +{ 'struct': 'CXLDynamicCapacityExtent',
> > + 'data': {
> > + 'offset':'uint64',
> > + 'len': 'uint64'
> > + }
> > +}
> > +
> > +##
> > +# @cxl-add-dynamic-capacity:
> > +#
> > +# Command to initiate to add dynamic capacity extents to a host. It
> > +# simulates operations defined in cxl spec r3.1 7.6.7.6.5.
> > +#
> > +# @path: CXL DCD canonical QOM path
> > +#
> > +# @host-id: The "Host ID" field as defined in cxl spec r3.1
> > +# Table 7-70.
> > +#
> > +# @selection-policy: The "Selection Policy" bits as defined in
> > +# cxl spec r3.1 Table 7-70. It specifies the policy to use for
> > +# selecting which extents comprise the added capacity.
>
> Hmm. This one is defined as a selection of nameable choices. Perhaps
> worth an enum? If we did do that, we'd also need to break the flags
> on in the release flags below.
Initially, I defined a enum for selection policy. But for users who are
not familiar with CXL spec, I think the enum definition is not very clear to
to them without reading the spec, so I removed it. Also, there are some
reserved bits there, let it as uint8 may help keep the interface unchanged
if some of the bits are used in the future?
>
>
> > +#
> > +# @region: The "Region Number" field as defined in cxl spec r3.1
> > +# Table 7-70. The dynamic capacity region where the capacity
> > +# is being added. Valid range is from 0-7.
> > +#
> > +# @tag: The "Tag" field as defined in cxl spec r3.1 Table 7-70.
> > +#
> > +# @extents: The "Extent List" field as defined in cxl spec r3.1
> > +# Table 7-70.
> > +#
> > +# Since : 9.1
> > +##
> > +{ 'command': 'cxl-add-dynamic-capacity',
> > + 'data': { 'path': 'str',
> > + 'host-id': 'uint16',
> > + 'selection-policy': 'uint8',
> > + 'region': 'uint8',
> > + 'tag': 'str',
> > + 'extents': [ 'CXLDynamicCapacityExtent' ]
> > + }
> > +}
> > +
> > +##
> > +# @cxl-release-dynamic-capacity:
> > +#
> > +# Command to initiate to release dynamic capacity extents from a
> > +# host. It simulates operations defined in cxl spec r3.1 7.6.7.6.6.
> > +#
> > +# @path: CXL DCD canonical QOM path
> > +#
> > +# @host-id: The "Host ID" field as defined in cxl spec r3.1
> > +# Table 7-71.
> > +#
> > +# @flags: The "Flags" field as defined in cxl spec r3.1 Table 7-71,
> > +# with bit[3:0] for removal policy, bit[4] for forced removal,
> > +# bit[5] for sanitize on release, bit[7:6] reserved.
>
> This can be nicely broken up into removal policy enum plus two flags.
> It might be worth doing so to give a nicer interface?
So if we choose to do it this way, maybe we use enum for the above add
command also?
>
> > +#
> > +# @region: The dynamic capacity region where the extents will be
> > +# released.
>
> This has a better definition in the add dynamic capacity entry above.
Yeah, will fix.
Fan
>
> > +#
> > +# @tag: The "Tag" field as defined in cxl spec r3.1 Table 7-71.
> > +#
> > +# @extents: The "Extent List" field as defined in cxl spec r3.1
> > +# Table 7-71.
> > +#
> > +# Since : 9.1
> > +##
> > +{ 'command': 'cxl-release-dynamic-capacity',
> > + 'data': { 'path': 'str',
> > + 'host-id': 'uint16',
> > + 'flags': 'uint8',
> > + 'region': 'uint8',
> > + 'tag': 'str',
> > + 'extents': [ 'CXLDynamicCapacityExtent' ]
> > + }
> > +}
>
