Proposal: TX API change with PCIe binding

[Andrew Jeffery]

Richard,

On Wed, 6 May 2020, at 19:03, Thomaiyar, Richard Marian wrote:
>  
> Discussed the same today (esp. reg Src / Destination EID 0 handling), 
> we need this private pointer data handling (local mapping will not 
> work, when there are multiple devices querying with EID 0).

I think your previous email is correct. Handling of e.g. BDF for the
PCIe VDM binding must be done by application of the route table
prior to placing the message on the physical transport. Transport-
specific addressing must not be something we concern ourselves
with further up the stack.

Messages with EID 0 must come from devices using local-only
addressing, which implies that they've not yet been assigned an EID.
See Table 2 of DSP0236 v1.3.0. For EID 0 destination messages it
implies we're sending a Set EID command to the device, for EID 0
source messages it implies that we've received a Discovery Notify
or some such event (are there other commands that we might receive)?

Either way, the upshot is the next action is very likely to be that we
assign an EID to the device, which means we can provisionally update
the route table as I outlined in my previous email to Sumanth about
handling physical layer routing. Provisional updates eliminate most
(all?) of the issues.

> Even we may 
> need to expose the physical address to the upper layer (on certain 
> conditions / OEM handling), and don’t have any other mechanism other 
> than having this privateBindingPtr.

Can you please outline the use-cases here? I'm not keen to implement
this before we have a concrete need for it. I want to see evidence that
it's necessary, not just the suggestion that it might be.

Finally, please make sure to Cc openbmc at lists.ozlabs.org on these
queries. I've taken the liberty of doing so with my reply. Other people
probably have the same questions as you do so I'd like to make sure
they can see the conversation, and that it's publicly archived for
historical purposes.

Also apologies for the delayed reply, the last couple of weeks have
been quite hectic for me.

Andrew

> 
> 
> Added this topic in OpenBMC PMCI WG agenda (for wide discussion).
> 
> 
> Regards,
> 
> Richard
> 
> 
> *From:* Thomaiyar, Richard Marian 
> *Sent:* Tuesday, May 5, 2020 7:10 PM
> *To:* Hawrylewicz Czarnowski, Przemyslaw 
> <przemyslaw.hawrylewicz.czarnowski at intel.com>; Andrew Jeffery 
> <andrew at aj.id.au>
> *Cc:* Bhat, Sumanth <sumanth.bhat at intel.com>
> *Subject:* RE: Proposal: TX API change with PCIe binding
> 
> 
> Hi Przemyslaw, 
> 
> 
> I am not in favor of adding this private pointer(binding specific) in 
> each MCTP message packet (i.e. Want to keep MCTP message packet beyond 
> the physical transport binding).
> 
> 
> Same problem exists even in SMBus binding, i.e. we need to convert the 
> EID to SMBus address, but that doesn’t require it to be added in every 
> mctp_message_tx packet. (i.e. This information must be hidden inside 
> the binding layer or managed by the control command handler code along 
> with the binding layer. Internally the binding layer (along with daemon 
> code) must maintain a table for EID to transport layer physical address 
> mapping) Basically, the binding_tx() must internally check this table 
> and get it converted. Note: Also we need a mechanism to find / discover 
> devices, which must be beyond the scope of this core mctp api’s (i.e. 
> mctp_message_tx / rx etc).
> 
> 
> Regards,
> 
> Richard
> 
> 
> *From:* Hawrylewicz Czarnowski, Przemyslaw 
> <przemyslaw.hawrylewicz.czarnowski at intel.com> 
> *Sent:* Tuesday, May 5, 2020 6:54 PM
> *To:* Andrew Jeffery <andrew at aj.id.au>
> *Cc:* Bhat, Sumanth <sumanth.bhat at intel.com>; Thomaiyar, Richard Marian 
> <richard.marian.thomaiyar at intel.com>
> *Subject:* Proposal: TX API change with PCIe binding
> 
> 
> Hi,
> 
> 
> I am working on PCIe binding and came across the following problem. 
> PCIe binding TX call needs some additional information for medium 
> specific header like sender/receiver BDF or routing type. Current core 
> implementation does not allow to pass any specific data to binding on 
> level of single request. 
> 
> 
> For details let me present some code:
> 
> 
> int mctp_message_tx(struct mctp *mctp, mctp_eid_t eid, void *msg,
> 
> - size_t msg_len)
> 
> + size_t msg_len, void *prv)
> 
> {
> 
> 
> static int mctp_message_tx_on_bus(struct mctp *mctp, struct mctp_bus *bus,
> 
>  mctp_eid_t src, mctp_eid_t dest, void *msg,
> 
> - size_t msg_len);
> 
> + size_t msg_len, void *prv);
> 
> 
> Also added this:
> 
> 
> struct mctp_pktbuf {
> 
>  size_t start, end, size;
> 
>  size_t mctp_hdr_off;
> 
>  struct mctp_binding *binding;
> 
>  struct mctp_pktbuf *next;
> 
> + /* binding private data */
> 
> + void *prv;
> 
>  uint8_t data[];
> 
> };
> 
> 
> to pass it down to binding specific tx handler. 
> 
> 
> The rationale for such change is simplification for passing binding 
> specific data and is also major advantage of this approach.
> 
> On the other hand it has also a significant shortage - breaks current 
> API adding additional parameters to existing apicall what implies 
> changes in all components using libmctp.
> 
> 
> Other approach is to use callbacks used directly by binding. I don’t 
> like this concept as, in my opinion, it complicates TX flow on client 
> side and creates need to match specific part of 
> 
> messages with header (which can be divided in core). This would look like this:
> 
> 
> set_tx_medium_header_callback(pcie_binding, &local_callback);
> 
> 
> mctp_message_tx(mctp, dest, payload, length);
> 
> . . .
> 
>  ---> mctp_pcie_binding_message_tx() invokes callback to gather needed 
> data, then prepares header.
> 
> 
> 
> What do you think about it? Please share your opinion.
> 
> 
> -- 
> 
> Best regards,
> 
> Przemyslaw Czarnowski
> 
>