Re: [PATCH V11 3/8] PCI: Create PCI library functions in support of DOE mailboxes.

[Ira Weiny <ira.weiny@intel.com>]

On Mon, Jun 20, 2022 at 10:24:49AM +0100, Jonathan Cameron wrote:
> 
> Hi Bjorn,
> 
> Thanks for reviewing!  Up to Ira of course, but I agree with all your
> comments

Me too!  :-D

> - a few responses to questions follow.
>
> 
> > 
> > > + * pci_doe_supports_prot() - Return if the DOE instance supports the given
> > > + *			     protocol
> > > + * @doe_mb: DOE mailbox capability to query
> > > + * @vid: Protocol Vendor ID
> > > + * @type: Protocol type
> > > + *
> > > + * RETURNS: True if the DOE mailbox supports the protocol specified  
> > 
> > Is the typical use that the caller has a few specific protocols it
> > cares about?  There's no case where a caller might want to enumerate
> > them all?  I guess they're all in prots[], but that's supposed to be
> > opaque to users.
> 
> Given each protocol needs specific handling in the driver, the only
> usecase for a general enumeration would be debug I think.  Maybe
> it makes sense to provide that info to userspace somewhere, but
> definitely feels like something for a follow up discussion.

Yep, CXL just needs to find out which mailbox has CDAT on it.

> > 
> > > + */
> > > +bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
> > > +{
> > > +	int i;
> > > +
> > > +	/* The discovery protocol must always be supported */
> > > +	if (vid == PCI_VENDOR_ID_PCI_SIG && type == PCI_DOE_PROTOCOL_DISCOVERY)
> > > +		return true;
> > > +
> > > +	for (i = 0; i < doe_mb->num_prots; i++)
> > > +		if ((doe_mb->prots[i].vid == vid) &&
> > > +		    (doe_mb->prots[i].type == type))
> > > +			return true;
> > > +
> > > +	return false;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_doe_supports_prot);  
> > 
> > > + * struct pci_doe_task - represents a single query/response
> > > + *
> > > + * @prot: DOE Protocol
> > > + * @request_pl: The request payload
> > > + * @request_pl_sz: Size of the request payload  
> > 
> > Size is in dwords, not bytes, I guess?
> 
> It's in bytes (IIRC) - we divide it by. It's a bit of a mess,
> but there are parts of SPDM over CMA where messages are not
> full number of dwords. My thinking was that we 'might' move
> the padding into the generic code if this becomes something
> multiple protocols need.  For now the RFC does the
> padding at the CMA layer.

I think at this layer the DOE protocol specifies all message sizes are in
multiples of DW's.  So I think this layer should enforce that.  Other protocols
will need to pad if they need to based on their need.

> Let's avoid this being unclear in future by stating that it's
> in bytes in the comment.

Already done!

Thanks Jonathan!
Ira

> 
> Jonathan
> 
> > 
> > > + * @response_pl: The response payload
> > > + * @response_pl_sz: Size of the response payload
> > > + * @rv: Return value.  Length of received response or error
> > > + * @complete: Called when task is complete
> > > + * @private: Private data for the consumer
> > > + */
> > > +struct pci_doe_task {
> > > +	struct pci_doe_protocol prot;
> > > +	u32 *request_pl;
> > > +	size_t request_pl_sz;
> > > +	u32 *response_pl;
> > > +	size_t response_pl_sz;
> > > +	int rv;
> > > +	void (*complete)(struct pci_doe_task *task);
> > > +	void *private;
> > > +};  
>