Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Fri, Apr 26, 2024 at 04:55:55PM +0100, Jonathan Cameron wrote: > On Wed, 24 Apr 2024 10:33:33 -0700 > Ira Weiny wrote: > > > Markus Armbruster wrote: > > > nifan@gmail.com writes: > > > > > > > From: Fan Ni > > > > > > > > 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 > fwiw it's useful in modeling the Orchestrator/Fabric Manager interaction, since you can basically build a little emulated MHD FM-LD on top of this. You basically just put a tiny software layer in front that converts what would be MCTP or whatever commands into QMP commands forwarded to the appropriate socket. When a real device comes around, you just point it at the real thing instead of that small software layer. But for the actual fabric manager, less useful. (Also, if you're confused, it's because fabric manager is such an overloaded term *laughcry*) ~Gregory
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Thu, 25 Apr 2024 10:30:51 -0700 Ira Weiny wrote: > Markus Armbruster wrote: > > fan writes: > > > > > On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote: > > >> nifan@gmail.com writes: > > >> > > >> > From: Fan Ni > > >> > > > >> > 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? > > > > Ah... All great points. > > Outside of CXL development I don't think there is a strong need for them > to be stable. I would like to see more than ad hoc scripts use them > though. So I don't think they are going to be changed without some > thought though. These align closely with the data that comes from the fabric management API in the CXL spec. So I don't see a big maintenance burden problem in having these as stable interfaces. Whilst they aren't doing quite the same job as the FM-API (which will be emulated such that it is visible to the guest as that aids some other types of testing) that interface defines the limits on what we can tell the device to do. So yes, risk for these is minimal and I'm happy to accept that. It'll be a while before we need libvirt to use them but I do expect to see that happen. (subject to some guessing on a future virtualization stack!) Jonathan > > Ira > > [snip]
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Wed, 24 Apr 2024 10:33:33 -0700
Ira Weiny wrote:
> Markus Armbruster wrote:
> > nifan@gmail.com writes:
> >
> > > From: Fan Ni
> > >
> > > 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
> >
> > [...]
> >
> > > 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
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
Markus Armbruster wrote: > fan writes: > > > On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote: > >> nifan@gmail.com writes: > >> > >> > From: Fan Ni > >> > > >> > 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? > Ah... All great points. Outside of CXL development I don't think there is a strong need for them to be stable. I would like to see more than ad hoc scripts use them though. So I don't think they are going to be changed without some thought though. Ira [snip]
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
fan writes:
> On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote:
>> nifan@gmail.com writes:
>>
>> > From: Fan Ni
>> >
>> > 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
>>
>> [...]
>>
>> > 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/ziafyub6fc9nr...@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?
>
>
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Wed, Apr 24, 2024 at 07:26:23PM +0200, Markus Armbruster wrote: > fan writes: > > > On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote: > >> nifan@gmail.com writes: > >> > >> > From: Fan Ni > >> > > >> > 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? > >> > > > > Hi Markus, > > Thanks for reviewing the patchset. This is v5 and we have sent out v7 > > recently, there are a lot of changes from v5 to v7. > > > > FYI. v7: > > https://lore.kernel.org/linux-cxl/ziafyub6fc9nr...@memverge.com/T/#t > > Missed it because you neglected to cc: me for qapi/cxl.json :) > > Thanks! Sorry for that. This is the first time I made changes to qapi/cxl.json so missed that. I will cc you when I sent out the next version. Btw, thanks for the review. I have replied to your comments in another reply. Fan >
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote:
> nifan@gmail.com writes:
>
> > From: Fan Ni
> >
> > 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.
>
> > 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
>
> [...]
>
> > 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/ziafyub6fc9nr...@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
>
> > +#
> > +# 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.
>
> 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).
>
> > +# @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
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
Markus Armbruster wrote:
> nifan@gmail.com writes:
>
> > From: Fan Ni
> >
> > 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.
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
>
> [...]
>
> > 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' ]
> > + }
> > +}
>
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
fan writes: > On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote: >> nifan@gmail.com writes: >> >> > From: Fan Ni >> > >> > 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? >> > > Hi Markus, > Thanks for reviewing the patchset. This is v5 and we have sent out v7 > recently, there are a lot of changes from v5 to v7. > > FYI. v7: https://lore.kernel.org/linux-cxl/ziafyub6fc9nr...@memverge.com/T/#t Missed it because you neglected to cc: me for qapi/cxl.json :) Thanks!
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote:
> nifan@gmail.com writes:
>
> > From: Fan Ni
> >
> > 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?
>
Hi Markus,
Thanks for reviewing the patchset. This is v5 and we have sent out v7
recently, there are a lot of changes from v5 to v7.
FYI. v7: https://lore.kernel.org/linux-cxl/ziafyub6fc9nr...@memverge.com/T/#t
Fan
> > 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
>
> [...]
>
> > 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' ]
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
nifan@gmail.com writes:
> From: Fan Ni
>
> 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?
> 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
[...]
> 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' ]
> + }
> +}
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Tue, Mar 12, 2024 at 12:37:23PM +, Jonathan Cameron wrote:
> On Fri, 8 Mar 2024 20:35:53 -0800
> fan wrote:
>
> > On Thu, Mar 07, 2024 at 12:45:55PM +, Jonathan Cameron wrote:
> > > ...
> > >
> > > > > > +list = records;
> > > > > > +extents = g_new0(CXLDCExtentRaw, num_extents);
> > > > > > +while (list) {
> > > > > > +CXLDCExtent *ent;
> > > > > > +bool skip_extent = false;
> > > > > > +
> > > > > > +offset = list->value->offset;
> > > > > > +len = list->value->len;
> > > > > > +
> > > > > > +extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> > > > > > +extents[i].len = len;
> > > > > > +memset(extents[i].tag, 0, 0x10);
> > > > > > +extents[i].shared_seq = 0;
> > > > > > +
> > > > > > +if (type == DC_EVENT_RELEASE_CAPACITY ||
> > > > > > +type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> > > > > > +/*
> > > > > > + * if the extent is still pending to be added to the
> > > > > > host,
> > > > >
> > > > > Odd spacing.
> > > > >
> > > > > > + * remove it from the pending extent list, so later
> > > > > > when the add
> > > > > > + * response for the extent arrives, the device can
> > > > > > reject the
> > > > > > + * extent as it is not in the pending list.
> > > > > > + */
> > > > > > +ent =
> > > > > > cxl_dc_extent_exists(>dc.extents_pending_to_add,
> > > > > > +[i]);
> > > > > > +if (ent) {
> > > > > > +QTAILQ_REMOVE(>dc.extents_pending_to_add,
> > > > > > ent, node);
> > > > > > +g_free(ent);
> > > > > > +skip_extent = true;
> > > > > > +} else if (!cxl_dc_extent_exists(>dc.extents,
> > > > > > [i])) {
> > > > > > +/* If the exact extent is not in the accepted
> > > > > > list, skip */
> > > > > > +skip_extent = true;
> > > > > > +}
> > > > > I think we need to reject case of some extents skipped and others not.
> > > > > That's not supported yet so we need to complain if we get it at
> > > > > least. Maybe we need
> > > > > to do two passes so we know this has happened early (or perhaps this
> > > > > is a later
> > > > > patch in which case a todo here would help).
> > > >
> > > > Skip here does not mean the extent is invalid, it just means the extent
> > > > is still pending to add, so remove them from pending list would be
> > > > enough to reject the extent, no need to release further. That is based
> > > > on your feedback on v4.
> > >
> > > Ah. I'd missunderstood.
> >
> > Hi Jonathan,
> >
> > I think we should not allow to release extents that are still pending to
> > add.
> > If we allow it, there is a case that will not work.
> > Let's see the following case (time order):
> > 1. Send request to add extent A to host; (A --> pending list)
> > 2. Send request to release A from the host; (Delete A from pending list,
> > hoping the following add response for A will fail as there is not a matched
> > extent in the pending list).
>
> Definitely not allow the host to release something it hasn't accepted.
> Should allow QMP to release such entrees though (and same for fmapi when
> we get there). Any such requested from host should be treated as whatever
> it says to do if you release an extent that you don't have.
Not sure how it works here. If we allow QMP to release such extents and
clear the pending list entrees accordingly, later if the host response with
empty extent list, how can the device figure out which request the response is
for. The spec assumes the response comes in order, so the head of the
pending list should be removed from the pending list, however, if QMP
process already removed it.
The key problem here is for empty updated extent list, we do not have a way to
figure out the corresponding request as there is no DPA info to look
into.
>
> > 3. Host send response to the device for the add request, however, for
> > some reason, it does not accept any of it, so updated list is empty,
> > spec allows it. Based on the spec, we need to drop the extent at the
> > head of the event log. Now we have problem. Since extent A is already
> > dropped from the list, we either cannot drop as the list is empty, which
> > is not the worst. If we have more extents in the list, we may drop the
> > one following A, which is for another request. If this happens, all the
> > following extents will be acked incorrectly as the order has been
> > shifted.
> >
> > Does the above reasoning make sense to you?
> Absolutely. I got confused here on who was doing release.
> Host definitely can't release stuff it hasn't successfully accepted.
>
> Jonathan
>
The assumption here is FM first initiates the request to add some
extents to the hosts, and later FM initiates to release the extents
while the extents has not been accepted by
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Fri, 8 Mar 2024 20:35:53 -0800
fan wrote:
> On Thu, Mar 07, 2024 at 12:45:55PM +, Jonathan Cameron wrote:
> > ...
> >
> > > > > +list = records;
> > > > > +extents = g_new0(CXLDCExtentRaw, num_extents);
> > > > > +while (list) {
> > > > > +CXLDCExtent *ent;
> > > > > +bool skip_extent = false;
> > > > > +
> > > > > +offset = list->value->offset;
> > > > > +len = list->value->len;
> > > > > +
> > > > > +extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> > > > > +extents[i].len = len;
> > > > > +memset(extents[i].tag, 0, 0x10);
> > > > > +extents[i].shared_seq = 0;
> > > > > +
> > > > > +if (type == DC_EVENT_RELEASE_CAPACITY ||
> > > > > +type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> > > > > +/*
> > > > > + * if the extent is still pending to be added to the
> > > > > host,
> > > >
> > > > Odd spacing.
> > > >
> > > > > + * remove it from the pending extent list, so later when
> > > > > the add
> > > > > + * response for the extent arrives, the device can
> > > > > reject the
> > > > > + * extent as it is not in the pending list.
> > > > > + */
> > > > > +ent =
> > > > > cxl_dc_extent_exists(>dc.extents_pending_to_add,
> > > > > +[i]);
> > > > > +if (ent) {
> > > > > +QTAILQ_REMOVE(>dc.extents_pending_to_add, ent,
> > > > > node);
> > > > > +g_free(ent);
> > > > > +skip_extent = true;
> > > > > +} else if (!cxl_dc_extent_exists(>dc.extents,
> > > > > [i])) {
> > > > > +/* If the exact extent is not in the accepted list,
> > > > > skip */
> > > > > +skip_extent = true;
> > > > > +}
> > > > I think we need to reject case of some extents skipped and others not.
> > > > That's not supported yet so we need to complain if we get it at least.
> > > > Maybe we need
> > > > to do two passes so we know this has happened early (or perhaps this is
> > > > a later
> > > > patch in which case a todo here would help).
> > >
> > > Skip here does not mean the extent is invalid, it just means the extent
> > > is still pending to add, so remove them from pending list would be
> > > enough to reject the extent, no need to release further. That is based
> > > on your feedback on v4.
> >
> > Ah. I'd missunderstood.
>
> Hi Jonathan,
>
> I think we should not allow to release extents that are still pending to
> add.
> If we allow it, there is a case that will not work.
> Let's see the following case (time order):
> 1. Send request to add extent A to host; (A --> pending list)
> 2. Send request to release A from the host; (Delete A from pending list,
> hoping the following add response for A will fail as there is not a matched
> extent in the pending list).
Definitely not allow the host to release something it hasn't accepted.
Should allow QMP to release such entrees though (and same for fmapi when
we get there). Any such requested from host should be treated as whatever
it says to do if you release an extent that you don't have.
> 3. Host send response to the device for the add request, however, for
> some reason, it does not accept any of it, so updated list is empty,
> spec allows it. Based on the spec, we need to drop the extent at the
> head of the event log. Now we have problem. Since extent A is already
> dropped from the list, we either cannot drop as the list is empty, which
> is not the worst. If we have more extents in the list, we may drop the
> one following A, which is for another request. If this happens, all the
> following extents will be acked incorrectly as the order has been
> shifted.
>
> Does the above reasoning make sense to you?
Absolutely. I got confused here on who was doing release.
Host definitely can't release stuff it hasn't successfully accepted.
Jonathan
>
> Fan
>
> >
> > >
> > > The loop here is only to collect the extents to sent to the event log.
> > > But as you said, we need one pass before updating pending list.
> > > Actually if we do not allow the above case where extents to release is
> > > still in the pending to add list, we can just return here with error, no
> > > extra dry run needed.
> > >
> > > What do you think?
> >
> > I think we need a way to back out extents from the pending to add list
> > so we can create the race where they are offered to the OS and it takes
> > forever to accept and by the time it does we've removed them.
> >
> > >
> > > >
> > > > > +
> > > > > +
> > > > > +/* No duplicate or overlapped extents are allowed */
> > > > > +if (test_any_bits_set(blk_bitmap, offset / block_size,
> > > > > + len / block_size)) {
> > > > > +error_setg(errp, "duplicate or overlapped extents are
> > > > > detected");
>
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Thu, Mar 07, 2024 at 12:45:55PM +, Jonathan Cameron wrote:
> ...
>
> > > > +list = records;
> > > > +extents = g_new0(CXLDCExtentRaw, num_extents);
> > > > +while (list) {
> > > > +CXLDCExtent *ent;
> > > > +bool skip_extent = false;
> > > > +
> > > > +offset = list->value->offset;
> > > > +len = list->value->len;
> > > > +
> > > > +extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> > > > +extents[i].len = len;
> > > > +memset(extents[i].tag, 0, 0x10);
> > > > +extents[i].shared_seq = 0;
> > > > +
> > > > +if (type == DC_EVENT_RELEASE_CAPACITY ||
> > > > +type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> > > > +/*
> > > > + * if the extent is still pending to be added to the
> > > > host,
> > >
> > > Odd spacing.
> > >
> > > > + * remove it from the pending extent list, so later when
> > > > the add
> > > > + * response for the extent arrives, the device can reject
> > > > the
> > > > + * extent as it is not in the pending list.
> > > > + */
> > > > +ent = cxl_dc_extent_exists(>dc.extents_pending_to_add,
> > > > +[i]);
> > > > +if (ent) {
> > > > +QTAILQ_REMOVE(>dc.extents_pending_to_add, ent,
> > > > node);
> > > > +g_free(ent);
> > > > +skip_extent = true;
> > > > +} else if (!cxl_dc_extent_exists(>dc.extents,
> > > > [i])) {
> > > > +/* If the exact extent is not in the accepted list,
> > > > skip */
> > > > +skip_extent = true;
> > > > +}
> > > I think we need to reject case of some extents skipped and others not.
> > > That's not supported yet so we need to complain if we get it at least.
> > > Maybe we need
> > > to do two passes so we know this has happened early (or perhaps this is a
> > > later
> > > patch in which case a todo here would help).
> >
> > Skip here does not mean the extent is invalid, it just means the extent
> > is still pending to add, so remove them from pending list would be
> > enough to reject the extent, no need to release further. That is based
> > on your feedback on v4.
>
> Ah. I'd missunderstood.
Hi Jonathan,
I think we should not allow to release extents that are still pending to
add.
If we allow it, there is a case that will not work.
Let's see the following case (time order):
1. Send request to add extent A to host; (A --> pending list)
2. Send request to release A from the host; (Delete A from pending list,
hoping the following add response for A will fail as there is not a matched
extent in the pending list).
3. Host send response to the device for the add request, however, for
some reason, it does not accept any of it, so updated list is empty,
spec allows it. Based on the spec, we need to drop the extent at the
head of the event log. Now we have problem. Since extent A is already
dropped from the list, we either cannot drop as the list is empty, which
is not the worst. If we have more extents in the list, we may drop the
one following A, which is for another request. If this happens, all the
following extents will be acked incorrectly as the order has been
shifted.
Does the above reasoning make sense to you?
Fan
>
> >
> > The loop here is only to collect the extents to sent to the event log.
> > But as you said, we need one pass before updating pending list.
> > Actually if we do not allow the above case where extents to release is
> > still in the pending to add list, we can just return here with error, no
> > extra dry run needed.
> >
> > What do you think?
>
> I think we need a way to back out extents from the pending to add list
> so we can create the race where they are offered to the OS and it takes
> forever to accept and by the time it does we've removed them.
>
> >
> > >
> > > > +
> > > > +
> > > > +/* No duplicate or overlapped extents are allowed */
> > > > +if (test_any_bits_set(blk_bitmap, offset / block_size,
> > > > + len / block_size)) {
> > > > +error_setg(errp, "duplicate or overlapped extents are
> > > > detected");
> > > > +return;
> > > > +}
> > > > +bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> > > > +
> > > > +list = list->next;
> > > > +if (!skip_extent) {
> > > > +i++;
> > > Problem is if we skip one in the middle the records will be wrong below.
> >
> > Why? Only extents passed the check will be stored in variable extents and
> > processed further and i be updated.
> > For skipped ones, since i is not updated, they will be
> > overwritten by following valid ones.
> Ah. I'd missed the fact you store into the extent without a check on validity
> but only move the index on if they were valid. Then rely on not passing a
> trailing
>
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
> >
> > > + * remove it from the pending extent list, so later when the
> > > add
> > > + * response for the extent arrives, the device can reject the
> > > + * extent as it is not in the pending list.
> > > + */
> > > +ent = cxl_dc_extent_exists(>dc.extents_pending_to_add,
> > > +[i]);
> > > +if (ent) {
> > > +QTAILQ_REMOVE(>dc.extents_pending_to_add, ent,
> > > node);
> > > +g_free(ent);
> > > +skip_extent = true;
> > > +} else if (!cxl_dc_extent_exists(>dc.extents,
> > > [i])) {
> > > +/* If the exact extent is not in the accepted list, skip
> > > */
> > > +skip_extent = true;
> > > +}
> > I think we need to reject case of some extents skipped and others not.
> > That's not supported yet so we need to complain if we get it at least.
> > Maybe we need
> > to do two passes so we know this has happened early (or perhaps this is a
> > later
> > patch in which case a todo here would help).
>
> If the second skip_extent case, I will reject earlier instead of
> skipping.
That was me misunderstanding the flow. I think this is fine as you have it
already.
Jonathan
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
...
> > > +list = records;
> > > +extents = g_new0(CXLDCExtentRaw, num_extents);
> > > +while (list) {
> > > +CXLDCExtent *ent;
> > > +bool skip_extent = false;
> > > +
> > > +offset = list->value->offset;
> > > +len = list->value->len;
> > > +
> > > +extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> > > +extents[i].len = len;
> > > +memset(extents[i].tag, 0, 0x10);
> > > +extents[i].shared_seq = 0;
> > > +
> > > +if (type == DC_EVENT_RELEASE_CAPACITY ||
> > > +type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> > > +/*
> > > + * if the extent is still pending to be added to the host,
> >
> > Odd spacing.
> >
> > > + * remove it from the pending extent list, so later when the
> > > add
> > > + * response for the extent arrives, the device can reject the
> > > + * extent as it is not in the pending list.
> > > + */
> > > +ent = cxl_dc_extent_exists(>dc.extents_pending_to_add,
> > > +[i]);
> > > +if (ent) {
> > > +QTAILQ_REMOVE(>dc.extents_pending_to_add, ent,
> > > node);
> > > +g_free(ent);
> > > +skip_extent = true;
> > > +} else if (!cxl_dc_extent_exists(>dc.extents,
> > > [i])) {
> > > +/* If the exact extent is not in the accepted list, skip
> > > */
> > > +skip_extent = true;
> > > +}
> > I think we need to reject case of some extents skipped and others not.
> > That's not supported yet so we need to complain if we get it at least.
> > Maybe we need
> > to do two passes so we know this has happened early (or perhaps this is a
> > later
> > patch in which case a todo here would help).
>
> Skip here does not mean the extent is invalid, it just means the extent
> is still pending to add, so remove them from pending list would be
> enough to reject the extent, no need to release further. That is based
> on your feedback on v4.
Ah. I'd missunderstood.
>
> The loop here is only to collect the extents to sent to the event log.
> But as you said, we need one pass before updating pending list.
> Actually if we do not allow the above case where extents to release is
> still in the pending to add list, we can just return here with error, no
> extra dry run needed.
>
> What do you think?
I think we need a way to back out extents from the pending to add list
so we can create the race where they are offered to the OS and it takes
forever to accept and by the time it does we've removed them.
>
> >
> > > +
> > > +
> > > +/* No duplicate or overlapped extents are allowed */
> > > +if (test_any_bits_set(blk_bitmap, offset / block_size,
> > > + len / block_size)) {
> > > +error_setg(errp, "duplicate or overlapped extents are
> > > detected");
> > > +return;
> > > +}
> > > +bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> > > +
> > > +list = list->next;
> > > +if (!skip_extent) {
> > > +i++;
> > Problem is if we skip one in the middle the records will be wrong below.
>
> Why? Only extents passed the check will be stored in variable extents and
> processed further and i be updated.
> For skipped ones, since i is not updated, they will be
> overwritten by following valid ones.
Ah. I'd missed the fact you store into the extent without a check on validity
but only move the index on if they were valid. Then rely on not passing a
trailing
entry at the end.
If would be more readable I think if local variables were used for the
parameters
until we've decided not to skip and the this ended with
if (!skip_extent) {
extents[i] = (DCXLDCExtentRaw) {
.start_dpa = ...
...
};
i++
}
We have local len already so probably just need
uint64_t start_dpa = offset + dcd->dc.regions[rid].base;
Also maybe skip_extent_evlog or something like that to explain we are only
skipping that part.
Helps people like me who read it completely wrong!
Jonathan
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Wed, Mar 06, 2024 at 05:48:11PM +, Jonathan Cameron wrote:
> On Mon, 4 Mar 2024 11:34:04 -0800
> nifan@gmail.com wrote:
>
> > From: Fan Ni
> >
> > Since fabric manager emulation is not supported yet, the change implements
> > the functions to add/release dynamic capacity extents as QMP interfaces.
>
> We'll need them anyway, or to implement an fm interface via QMP which is
> going to be ugly and complex.
>
> >
> > 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.
>
> Maybe the kernel will treat it as a request to release the extent it
> is tracking that contains it. So we may want to add a way to poke that.
> Not today though!
>
> >
> > 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
>
> ...
>
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index dccfaaad3a..e9c8994cdb 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -674,6 +674,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d,
> > Error **errp)
> > ct3d->dc.total_capacity += region->len;
> > }
> > QTAILQ_INIT(>dc.extents);
> > +QTAILQ_INIT(>dc.extents_pending_to_add);
> >
> > return true;
> > }
> > @@ -686,6 +687,12 @@ static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
> > ent = QTAILQ_FIRST(>dc.extents);
> > cxl_remove_extent_from_extent_list(>dc.extents, ent);
> > }
> > +
> > +while (!QTAILQ_EMPTY(>dc.extents_pending_to_add)) {
>
> QTAILQ_FOR_EACHSAFE
>
> > +ent = QTAILQ_FIRST(>dc.extents_pending_to_add);
> > +
> > cxl_remove_extent_from_extent_list(>dc.extents_pending_to_add,
> > + ent);
> > +}
> > }
>
> > +/*
> > + * The main function to process dynamic capacity event. Currently DC
> > extents
> > + * add/release requests are processed.
> > + */
> > +static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog
> > log,
> > + CXLDCEventType type, uint16_t
> > hid,
> > + uint8_t rid,
> > + CXLDCExtentRecordList
> > *records,
> > + Error **errp)
> > +{
> > +Object *obj;
> > +CXLEventDynamicCapacity dCap = {};
> > +CXLEventRecordHdr *hdr =
> > +CXLType3Dev *dcd;
> > +uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> > +uint32_t num_extents = 0;
> > +CXLDCExtentRecordList *list;
> > +g_autofree CXLDCExtentRaw *extents = NULL;
> > +uint8_t enc_log;
> > +uint64_t offset, len, block_size;
> > +int i;
> > +int rc;
>
> Combine the two lines above.
>
> > +g_autofree unsigned long *blk_bitmap = NULL;
> > +
> > +obj = object_resolve_path(path, NULL);
> > +if (!obj) {
> > +error_setg(errp, "Unable to resolve path");
> > +return;
> > +}
>
> object_resolve_path_type() and skip a step (should do this in various places
> in our existing code!)
>
> > +if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> > +error_setg(errp, "Path not point to a valid CXL type3 device");
> > +return;
> > +}
> > +
> > +dcd = CXL_TYPE3(obj);
> > +if (!dcd->dc.num_regions) {
> > +error_setg(errp, "No dynamic capacity support from the device");
> > +return;
> > +}
> > +
> > +rc = ct3d_qmp_cxl_event_log_enc(log);
> > +if (rc < 0) {
> > +error_setg(errp, "Unhandled error log type");
> > +return;
> > +}
> > +enc_log = rc;
> > +
> > +if (rid >= dcd->dc.num_regions) {
> > +error_setg(errp, "region id is too large");
> > +return;
> > +}
> > +block_size = dcd->dc.regions[rid].block_size;
> > +
> > +/* Sanity check and count the extents */
> > +list =
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Wed, Mar 06, 2024 at 05:48:11PM +, Jonathan Cameron wrote:
> On Mon, 4 Mar 2024 11:34:04 -0800
> nifan@gmail.com wrote:
>
> > From: Fan Ni
> >
> > Since fabric manager emulation is not supported yet, the change implements
> > the functions to add/release dynamic capacity extents as QMP interfaces.
>
> We'll need them anyway, or to implement an fm interface via QMP which is
> going to be ugly and complex.
>
> >
> > 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.
>
> Maybe the kernel will treat it as a request to release the extent it
> is tracking that contains it. So we may want to add a way to poke that.
> Not today though!
>
> >
> > 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
>
> ...
>
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index dccfaaad3a..e9c8994cdb 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -674,6 +674,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d,
> > Error **errp)
> > ct3d->dc.total_capacity += region->len;
> > }
> > QTAILQ_INIT(>dc.extents);
> > +QTAILQ_INIT(>dc.extents_pending_to_add);
> >
> > return true;
> > }
> > @@ -686,6 +687,12 @@ static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
> > ent = QTAILQ_FIRST(>dc.extents);
> > cxl_remove_extent_from_extent_list(>dc.extents, ent);
> > }
> > +
> > +while (!QTAILQ_EMPTY(>dc.extents_pending_to_add)) {
>
> QTAILQ_FOR_EACHSAFE
>
> > +ent = QTAILQ_FIRST(>dc.extents_pending_to_add);
> > +
> > cxl_remove_extent_from_extent_list(>dc.extents_pending_to_add,
> > + ent);
> > +}
> > }
>
> > +/*
> > + * The main function to process dynamic capacity event. Currently DC
> > extents
> > + * add/release requests are processed.
> > + */
> > +static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog
> > log,
> > + CXLDCEventType type, uint16_t
> > hid,
> > + uint8_t rid,
> > + CXLDCExtentRecordList
> > *records,
> > + Error **errp)
> > +{
> > +Object *obj;
> > +CXLEventDynamicCapacity dCap = {};
> > +CXLEventRecordHdr *hdr =
> > +CXLType3Dev *dcd;
> > +uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> > +uint32_t num_extents = 0;
> > +CXLDCExtentRecordList *list;
> > +g_autofree CXLDCExtentRaw *extents = NULL;
> > +uint8_t enc_log;
> > +uint64_t offset, len, block_size;
> > +int i;
> > +int rc;
>
> Combine the two lines above.
>
> > +g_autofree unsigned long *blk_bitmap = NULL;
> > +
> > +obj = object_resolve_path(path, NULL);
> > +if (!obj) {
> > +error_setg(errp, "Unable to resolve path");
> > +return;
> > +}
>
> object_resolve_path_type() and skip a step (should do this in various places
> in our existing code!)
>
> > +if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> > +error_setg(errp, "Path not point to a valid CXL type3 device");
> > +return;
> > +}
> > +
> > +dcd = CXL_TYPE3(obj);
> > +if (!dcd->dc.num_regions) {
> > +error_setg(errp, "No dynamic capacity support from the device");
> > +return;
> > +}
> > +
> > +rc = ct3d_qmp_cxl_event_log_enc(log);
> > +if (rc < 0) {
> > +error_setg(errp, "Unhandled error log type");
> > +return;
> > +}
> > +enc_log = rc;
> > +
> > +if (rid >= dcd->dc.num_regions) {
> > +error_setg(errp, "region id is too large");
> > +return;
> > +}
> > +block_size = dcd->dc.regions[rid].block_size;
> > +
> > +/* Sanity check and count the extents */
> > +list =
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
On Mon, 4 Mar 2024 11:34:04 -0800
nifan@gmail.com wrote:
> From: Fan Ni
>
> Since fabric manager emulation is not supported yet, the change implements
> the functions to add/release dynamic capacity extents as QMP interfaces.
We'll need them anyway, or to implement an fm interface via QMP which is
going to be ugly and complex.
>
> 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.
Maybe the kernel will treat it as a request to release the extent it
is tracking that contains it. So we may want to add a way to poke that.
Not today though!
>
> 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
...
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index dccfaaad3a..e9c8994cdb 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -674,6 +674,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d,
> Error **errp)
> ct3d->dc.total_capacity += region->len;
> }
> QTAILQ_INIT(>dc.extents);
> +QTAILQ_INIT(>dc.extents_pending_to_add);
>
> return true;
> }
> @@ -686,6 +687,12 @@ static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
> ent = QTAILQ_FIRST(>dc.extents);
> cxl_remove_extent_from_extent_list(>dc.extents, ent);
> }
> +
> +while (!QTAILQ_EMPTY(>dc.extents_pending_to_add)) {
QTAILQ_FOR_EACHSAFE
> +ent = QTAILQ_FIRST(>dc.extents_pending_to_add);
> +cxl_remove_extent_from_extent_list(>dc.extents_pending_to_add,
> + ent);
> +}
> }
> +/*
> + * The main function to process dynamic capacity event. Currently DC extents
> + * add/release requests are processed.
> + */
> +static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog
> log,
> + CXLDCEventType type, uint16_t
> hid,
> + uint8_t rid,
> + CXLDCExtentRecordList *records,
> + Error **errp)
> +{
> +Object *obj;
> +CXLEventDynamicCapacity dCap = {};
> +CXLEventRecordHdr *hdr =
> +CXLType3Dev *dcd;
> +uint8_t flags = 1 << CXL_EVENT_TYPE_INFO;
> +uint32_t num_extents = 0;
> +CXLDCExtentRecordList *list;
> +g_autofree CXLDCExtentRaw *extents = NULL;
> +uint8_t enc_log;
> +uint64_t offset, len, block_size;
> +int i;
> +int rc;
Combine the two lines above.
> +g_autofree unsigned long *blk_bitmap = NULL;
> +
> +obj = object_resolve_path(path, NULL);
> +if (!obj) {
> +error_setg(errp, "Unable to resolve path");
> +return;
> +}
object_resolve_path_type() and skip a step (should do this in various places
in our existing code!)
> +if (!object_dynamic_cast(obj, TYPE_CXL_TYPE3)) {
> +error_setg(errp, "Path not point to a valid CXL type3 device");
> +return;
> +}
> +
> +dcd = CXL_TYPE3(obj);
> +if (!dcd->dc.num_regions) {
> +error_setg(errp, "No dynamic capacity support from the device");
> +return;
> +}
> +
> +rc = ct3d_qmp_cxl_event_log_enc(log);
> +if (rc < 0) {
> +error_setg(errp, "Unhandled error log type");
> +return;
> +}
> +enc_log = rc;
> +
> +if (rid >= dcd->dc.num_regions) {
> +error_setg(errp, "region id is too large");
> +return;
> +}
> +block_size = dcd->dc.regions[rid].block_size;
> +
> +/* Sanity check and count the extents */
> +list = records;
> +while (list) {
> +offset = list->value->offset;
> +len = list->value->len;
> +
> +if (len == 0) {
> +error_setg(errp, "extent with 0 length is not allowed");
> +return;
> +}
> +
> +if (offset % block_size || len % block_size) {
> +error_setg(errp, "dpa or len is not aligned to region block
> size");
> +
