Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Jun 2022 16:06:16 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Bjorn Helgaas <helgaas@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "Alison Schofield" <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V11 3/8] PCI: Create PCI library functions in support of
 DOE mailboxes.
Message-ID: <YrOgaBPFvBzGy5oe@iweiny-desk3>
References: <20220610202259.3544623-4-ira.weiny@intel.com>
 <20220617224019.GA1208614@bhelgaas>
 <20220620102449.000041d4@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220620102449.000041d4@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?7QLBMwAduMdXoQk5wFwOvDXuCd2KLu2mCLVTmWRswx4kCJTt5Mduiqo39WR7?=
 =?us-ascii?Q?PUFhK8brwZIJiKN7YNN7PtxwZm9xN4XZT2C+9Bly2dvG/lI2J9IuO3083XWy?=
 =?us-ascii?Q?FrJUntgwiR/z5+z6epwYApHbcci3XNh7m+6DWLKElBKQl8LoGawbBGic6OjP?=
 =?us-ascii?Q?AhSd42k+sdfRgPCmuVBSqFxGBUF3Hx6FbfIe6S0DFElN0TykWBfnjDwzWuUv?=
 =?us-ascii?Q?Fv6wZ5BxQVCoDB+FvTu6+2R4bvzVIOzPiaUV6PwJvFx9mjoVkSz3Z8aEhyR0?=
 =?us-ascii?Q?XmBFPPiqvaZpw2BFMd7/365r43Eko8sKnBgp4TG2kt3alzx2lkmdExApuxBY?=
 =?us-ascii?Q?KuXb5s3Y1vPI8CGr8jq9QxwFCZBwh40OALmb6r4o8i1A52Hpir40mBrRGpsc?=
 =?us-ascii?Q?I8l6TshslTxOMf/mbH5lw62mlea8FuI300Juz0Ebs7IeYv4PYzk6vcjtVofy?=
 =?us-ascii?Q?LZ04IiyKY/VoX0PltwqAw79LwJxhW9PZg5AuXYNQztkKztuY+u2TRyrTr5VE?=
 =?us-ascii?Q?UYHpbjMD3k64sEClJmcZf4WXts287lRL8/1GOgxDml6XQs2VkpaEiclbQE04?=
 =?us-ascii?Q?G9k4RBroJ5b1EWn+ESJo8PZbZtEYyeM9Ew6mIIuR/ZGCihi1zN9PHdLxKf7A?=
 =?us-ascii?Q?r6vrqrsbUTYuNBnhaG3iNwJXQ5yjVXVu374DkQCysCycgQK4nGTmkDfJXpEe?=
 =?us-ascii?Q?6j2BG9EVVG5UrbFTttMoFIkEiitPune5ZWhMg3UnJOXVZ0kI1JvElA6hLIm4?=
 =?us-ascii?Q?8oGfclHoQEX2fnkz9b37dLNolcuDNsbv0yaZ5q+9hYKCJ+Kr849K0RdxGyOj?=
 =?us-ascii?Q?x1VqD6T8XCGLF5rfIJrWDb6xtSHlrRnGL5G/MZ9xyy/iD5+u2Jm7QLu7hl4A?=
 =?us-ascii?Q?JmHM9DO1+WONvvtr+swc6iuIuLYdtT3+thmIGAkQFur6+pxDty/wGKOVgOwT?=
 =?us-ascii?Q?Set3ePL1TbrbB8edcAqQ2PzApmmLmBpPcJmR118gqpW24zpGx/5FE84BGU8X?=
 =?us-ascii?Q?GvjcAcau7maccabHW9Lzrmg0AHo3VObg3Q+RCrtYs7eMeOYNuF2MbqOSFqZH?=
 =?us-ascii?Q?HhDlUOEtkJWf3s2YWvx6sWtqf5HkFvd7C0KhKAcoEB1bALsJ4gjE3PgBGAv4?=
 =?us-ascii?Q?kjMQi2+3RcDyR8dCWEucubKLXy1DOuDCDnXp/6EWVf21x9Onkvd2jnPbpjIb?=
 =?us-ascii?Q?gdWBOrazXAAlNQkzAsCmk691pagRTNp5dqNsw2PIm+DfhL1NRGbTIeTQZKPR?=
 =?us-ascii?Q?k+8SHrOV3nyxRuw6O08ub2VnQEob3APtBNNzJ3sgIl7d9PaBMDJ1DeV4QVKt?=
 =?us-ascii?Q?AbI00buH+EoHzepROZ5oKG/9SGynopXz/p5KqLr+kKPls/FQZ7WoRTBTRG8t?=
 =?us-ascii?Q?UOA1sGwM01/lacKybiW7h44Ogmy45IIy2omHOOqo+gqvLVVn69DIp/1oOyuM?=
 =?us-ascii?Q?aP/uTzJTH/+6pCp4/n2QR8ZzhinFIeLNhr/MtnDA6O4Psw0AHDS45X7ndFRt?=
 =?us-ascii?Q?w10h0waneKHzxM5qsU0QhaLTytocfxAbFrUMzQ1ccnOjyH28ckGRBNY8fTWQ?=
 =?us-ascii?Q?xEHpoVtD07Qal44AAPSi+4UvGeU7DzzKLMpk1SxZmEv8xCgtDjRA4F1FPyrl?=
 =?us-ascii?Q?/a/zXaeWYb402pmj4Nja2TzqNZd6oUHObB/DrT6ijlXKSE4B4dBHRHAnPvyV?=
 =?us-ascii?Q?OAUlIQljWiZLINfqE+uVakBwdweQTgUuLCjdTQ7DZf2BHSF82rIbPxrvvUFk?=
 =?us-ascii?Q?uXmVdXkN078HkLfLXV7JXPDi/oxS8FM=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e1a3547d-202d-43f8-9604-08da54a3d20b
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jun 2022 23:06:20.7817
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: DXZw594k/3T9W5ZClZGG+49mUJA7kpMmDoh7/WMCwnBdu1YTShLaeMKNkowqf/1GGlzBX+1d4TKvcvwa3Bu5FQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6542
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;
> > > +};  
>