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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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.

> 
> If you're aiming for stable, you need to convince us the interface can
> support the foreseeable purposes without incompatible changes.  In
> particular, I'd like to see how you intend to support "finding out what
> the state is".  I suspect that's related to my question in review of v8:
> how to detect completion, and maybe track progress.

There is a need for completion information but given it's completely
asynchronous to the commands defined here (Can be out of order, lots of
ongoing capacity add / remove flows in flight etc) and for the hardware
definition the feedback occurs via an event record log I'm not expecting it
to affect the interfaces added so far.

> 
> There's infrastructure for background jobs: job.json.  We might be
> better off using it, unless completion is trivial and progress tracking
> unnecessary.

I'll take a look, but these are not conventional background commands
(We do have those as well, but that's a whole different set of future
problems!)

The actual command itself completes synchronously but not the flow
it kicked off which is not background as such because it may never
finish and involves lots of moving parts.  This is similar to any
form of error injection.  We inject the error synchronously and that
creates a bunch of records in various registers / firmware etc but
the actions the host OS takes are asynchronous and may only happen
when it decides to poll some register or a driver loads much later.

So I'm not sure if job.json approach fits.

> 
> [...]
> 
>