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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 4 Jun 2024 12:54:28 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 04 Jun 2024 11:18:18 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > I finally got around to read this slowly.  Thank you, Fan and Jonathan!
> > 
> > I'm getting some "incomplete" vibes: "if we ever implement", "patches
> > for this on list", "we aren't emulating this yet at all", and ...  
> 
> Absolutely.  There is a bunch of stuff that we reject today but
> the interfaces allow you to specify it and align with the CXL specification
> Fabric Management API definition which is the spec used to control this
> stuff from a BMC etc.  If that doesn't work we have a hardware errata
> problem, so hopefully that is fairly stable.
> 
> I think I can publicly confirm there are some related errata in flight,
> seeing as we said we'd raise questions on some aspects in the kernel and
> QEMU series preceding this one (so no IP secrecy issues). These are as a
> result of this work from Fan, but we have carefully avoided implementing
> anything that 'may' change.
> 
> 
> > 
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
> > 
> > [...]
> >   
> > > Only thing I'd add is that for now (because we don't need it for testing
> > > the kernel flows) is that this does not provide any way for external
> > > agents (e.g. our 'fabric manager' to find out what the state is - i.e.
> > > if the extents have been accepted by the host etc). That stuff is all
> > > defined by the spec, but not yet in the QMP interface.  At somepoint
> > > we may want to add that as a state query type interface.    
> > 
> > ... this, too.
> > 
> > In review of v5, I asked whether this interface needs to be stable.
> > 
> > "Not stable" doesn't mean we change an interface without thought.  It
> > merely means we can change it much, much faster, and with much less
> > overhead.
> > 
> > I understand you want it chiefly for CXL development.  Development aids
> > commonly don't need to be stable.  
> 
> Ok. If it makes sense to make this unstable for now I'm fine with that.
> I don't see why it would change beyond in backwards compatible fashion
> with new optional fields to cover future specification updates allowing
> for more information. However I've been wrong on such things before.
> 
> So I'm fine adding a patch on top of v8 marking them unstable for now.

Does this look correct?  Applied on top of the docs update patch just
posted in response to v8.  In my view neither is a blocker on Michael
Tsirkin sending a pull request but +CC so he is aware of discussion.

[PATCH] hw/cxl/events: Mark cxl-add-dynamic-capacity and cxl-release-dynamic-capcity unstable

Markus suggested that we make the unstable. I don't expect these
interfaces to change because of their tight coupling to the Compute
Express Link (CXL) Specification, Revision 3.1 Fabric Management API
definitions which can only be extended in backwards compatible way.
However, there seems little disadvantage in taking a cautious path
for now and marking them as unstable interfaces.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 qapi/cxl.json | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/qapi/cxl.json b/qapi/cxl.json
index a38622a0d1..bdfac67c47 100644
--- a/qapi/cxl.json
+++ b/qapi/cxl.json
@@ -453,6 +453,10 @@
 # @extents: The "Extent List" field as defined in Compute Express Link
 #     (CXL) Specification, Revision 3.1, Table 7-70.
 #
+# Features:
+#
+# @unstable: For now this command is subject to change.
+#
 # Since : 9.1
 ##
 { 'command': 'cxl-add-dynamic-capacity',
@@ -462,7 +466,8 @@
             'region': 'uint8',
             '*tag': 'str',
             'extents': [ 'CxlDynamicCapacityExtent' ]
-           }
+           },
+  'features': [ 'unstable' ]
 }
 
 ##
@@ -527,6 +532,10 @@
 # @extents: The "Extent List" field as defined in Compute Express
 #     Link (CXL) Specification, Revision 3.1, Table 7-71.
 #
+# Features:
+#
+# @unstable: For now this command is subject to change.
+#
 # Since : 9.1
 ##
 { 'command': 'cxl-release-dynamic-capacity',
@@ -538,5 +547,6 @@
             'region': 'uint8',
             '*tag': 'str',
             'extents': [ 'CxlDynamicCapacityExtent' ]
-           }
+           },
+  'features': [ 'unstable' ]
 }
-- 
2.39.2