Hi Markus, Michael and Jonathan, FYI. I have updated this patch based on the feedbacks so far, and posted here: https://lore.kernel.org/linux-cxl/20240418232902.583744-1-fan...@samsung.com/T/#ma25b6657597d39df23341dc43c22a8c49818e5f9
Comments are welcomed and appreciated. Fan On Wed, May 01, 2024 at 03:58:12PM +0100, Jonathan Cameron wrote: > > > > > > >> > +# @hid: host id > > > >> > > > >> @host-id, unless "HID" is established terminology in CXL DCD land. > > > > > > > > host-id works. > > > >> > > > >> What is a host ID? > > > > > > > > It is an id identifying the host to which the capacity is being added. > > > > > > How are these IDs assigned? > > > > All the arguments passed to the command here are defined in CXL spec. I > > will add reference to the spec. > > > > Based on the spec, for LD-FAM (Fabric attached memory represented as > > logical device), host id is the LD-ID of the host interface to which > > the capacity is being added. LD-ID is a unique number (16-bit) assigned > > to a host interface. > > Key here is the host doesn't know it. This ID exists purely for rooting > to the appropriate host interface either via choosing a port on a > multihead Single Logical Device (SLD) (so today it's always 0 as we only > have one head) or if we ever implement a switch capable of handling MLDs > then the switch will handle routing of host PCIe accesses so it lands > on the interface defined by this ID (and the event turns up in that event log. > > Host A Host B - could in theory be a RP on host A ;) > | | Doesn't exist (yet, but there are partial. > _|______________|_ patches for this on list. > | LD 0 LD 1| > | | > | Multi Head | > | Single Logical | > | Device (MH-SLD) | > |__________________| > Host view similar to the switch case, but just two direct > connected devices. > > Or Switch and MLD case - we aren't emulating this yet at all > > Wiring / real topology Host View > > Host A Host B Host A Host B > | | | | > ___|__________|___ _|_ _|_ > | \ SWITCH / | |SW0| | | | > | \ / | | | | | | | > | LD0 LD1 | | | | | | | > | \ / | | | | | | | > | | | | | | | | | > |________|_________| |_|_| |_|_| > | | | > Traffic tagged with LD | | > | | | > ________|________________ ____|___ ____|___ > | Multilogical Device MLD | | | | | > | | | | Simple | | Another| > | / \ | | CXL | | CXL | > | / \ | | Memory | | Memory | > | Interfaces | | Device | | Device | > | LD0 LD1 | | | | | > |_________________________| |________| |________| > > Note the hosts just see separate devices and switches with the fun exception > that the > memory may actually be available to both at the same time. > > Control plane for the switches and MLD see what is actually going on. > > At this stage upshot is we could just default this to zero and add an optional > parameter to set it later. > > > > ... > > > > >> > +# @extents: Extents to release > > > >> > +# > > > >> > +# Since : 9.1 > > > >> > +## > > > >> > +{ 'command': 'cxl-release-dynamic-capacity', > > > >> > + 'data': { 'path': 'str', > > > >> > + 'hid': 'uint16', > > > >> > + 'flags': 'uint8', > > > >> > + 'region-id': 'uint8', > > > >> > + 'tag': 'str', > > > >> > + 'extents': [ 'CXLDCExtentRecord' ] > > > >> > + } > > > >> > +} > > > >> > > > >> During review of v5, you wrote: > > > >> > > > >> For add command, the host will send a mailbox command to response > > > >> to > > > >> the add request to the device to indicate whether it accepts the > > > >> add > > > >> capacity offer or not. > > > >> > > > >> For release command, the host send a mailbox command (not always a > > > >> response since the host can proactively release capacity if it does > > > >> not need it any more) to device to ask device release the capacity. > > > >> > > > >> Can you briefly sketch the protocol? Peers and messages involved. > > > >> Possibly as a state diagram. > > > > > > > > Need to think about it. If we can polish the text nicely, maybe the > > > > sketch is not needed. My concern is that the sketch may > > > > introduce unwanted complexity as we expose too much details. The two > > > > commands provide ways to add/release dynamic capacity to/from a host, > > > > that is all. All the other information, like what the host will do, or > > > > how the device will react, are consequence of the command, not sure > > > > whether we want to include here. > > > > > > The protocol sketch is for me, not necessarily the doc comment. I'd > > > like to understand at high level how this stuff works, because only then > > > can I meaningfully review the docs. > > > > -------------------------------- > > For add command, saying a user sends a request to FM to ask to add > > extent A of the device (managed by FM) to host 0. > > The function cxl-add-dynamic-capacity simulates what FM needs to do. > > This gets a little fiddly as an explanation. I'd argue this is more or > less at the level of the FM to device command flow so it's the device > verifying etc. (you could explain this interface as talking to an FM > that is talking to the device, but that just feels complicated to me). > > > 1. Verify extent A is valid (behaviour defined by the spec), return > > error if not; otherwise, > > 2. Add a record to the device's event log (indicating the intent to > > add extent A to host 0), update device internal extent tracking status, > > signal an interrupt to host 0; > > (The above step 1 & 2 are performed in the QMP interface, following > > operations are QMP irrelevant, only host and device involved.) > > In this patch. > > > 3. Once the interrupt is received, host 0 fetch the event record from > > the device's event log through some mailbox command (out of scope > > of this patch series). > > It's in patch 8. > > > 4. Host 0 decides whether it accepts extent A or not. Whether accept or > > reject, host needs to send a response (add-response mailbox command) to > > the device so the device can update its internal extent tracking > > status accordingly. > > The device return a value to the host showing whether the response is > > successful or failed. > > (assuming the host isn't buggy this always succeeds) > > > 5. Based on the mailbox command return value, the host process > > accordingly. > > Memory now useable by host if it accepted it successfully. > > > 6. The host sends a mailbox command to the device to clear the event > > record in the device's event log. > > > > --------------------------------- > > For release command, saying a user sends a request to FM to ask host 0 > > to release extent A and return it back to the device (managed by FM). > > > > The function cxl-release-dynamic-capacity simulates what FM needs to do. > > 1. Verify extent A is valid (defined by the spec), return error if not; > > otherwise, > > 2. Add a record to the event log (indicating the intent to > > release extent A from host 0), signal an interrupt to host 0; > > (The above step 1 & 2 are performed in the QMP interface, following > > operations are QMP irrelevant, only host and device involved. > > 3. Once the interrupt is received, host 0 fetch the event record from > > the device's event log through some mailbox command (out of scope > > of this patch series). > > 4. Host 0 decides whether it can release extent A or not. Whether can or > > cannot release, host needs to send a release (mailbox command) to the device > > so the device can update its internal extent tracking status accordingly. > > The device returns a value to host 0 showing whether the release is > > successful or failed. > > 5. Based on the returned value, the host process accordingly. > > 6. The host sends mailbox command to clear the event record in the > > device's event log. > > > > For release command, it is more complicated. Based on the release flag > > passed to FM, FM can behaviour differently. For example, if the > > forced-removal flag is set, FM can directly get the extent back from a > > host for other uses without waiting for the host to send command to the > > device. For the above step 2, their may be not event record to the event > > log (no supported in this patch series yet). > I thought we weren't doing force remove yet? So for that we could > set default value as normal release until we add that support perhaps. > > > > > Also, for the release interface here, it simulates FM initializes the > > release request. > > There is another case where the host can proactively release extents it > > do not need any more back to device. However, this case is out of the > > scope of this release interface. > > > > Hope the above text helps a little for the context here. > > Let me know if further clarification is needed. > > 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. > > Jonathan > > p.s. Our emails raced yesterday, so great you put together this explanation > of the flows before I got to it :) > > > > > Thanks, > > Fan > > > > > > > > > > > > > @Jonathan, Any thoughts on this? > > > > > > Thanks! > > > >