linux-kernel - Re: [PATCH v6 05/14] iommufd: Add IOMMUFD_OBJ_VEVENTQ and IOMMUFD_CMD_VEVENTQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7TJCcH9iFvcEDuO@nvidia.com>
Date: Tue, 18 Feb 2025 09:53:13 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "jgg@...dia.com" <jgg@...dia.com>, "corbet@....net" <corbet@....net>,
	"will@...nel.org" <will@...nel.org>, "joro@...tes.org" <joro@...tes.org>,
	"suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
	"robin.murphy@....com" <robin.murphy@....com>, "dwmw2@...radead.org"
	<dwmw2@...radead.org>, "baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>,
	"shuah@...nel.org" <shuah@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "eric.auger@...hat.com" <eric.auger@...hat.com>,
	"jean-philippe@...aro.org" <jean-philippe@...aro.org>, "mdf@...nel.org"
	<mdf@...nel.org>, "mshavit@...gle.com" <mshavit@...gle.com>,
	"shameerali.kolothum.thodi@...wei.com"
	<shameerali.kolothum.thodi@...wei.com>, "smostafa@...gle.com"
	<smostafa@...gle.com>, "ddutile@...hat.com" <ddutile@...hat.com>, "Liu, Yi L"
	<yi.l.liu@...el.com>, "patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: Re: [PATCH v6 05/14] iommufd: Add IOMMUFD_OBJ_VEVENTQ and
 IOMMUFD_CMD_VEVENTQ_ALLOC

On Tue, Feb 18, 2025 at 05:13:47AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@...dia.com>
> > Sent: Saturday, January 25, 2025 8:31 AM
> > +
> > +/*
> > + * An iommufd_veventq object represents an interface to deliver vIOMMU
> > events to
> > + * the user space. It is created/destroyed by the user space and associated
> > with
> > + * vIOMMU object(s) during the allocations.
> 
> s/object(s)/object/, given the eventq cannot be shared between vIOMMUs.

Done. Adding an "a" too.

> > +static inline void iommufd_vevent_handler(struct iommufd_veventq
> > *veventq,
> > +					  struct iommufd_vevent *vevent)
> > +{
> > +	struct iommufd_eventq *eventq = &veventq->common;
> > +
> > +	/*
> > +	 * Remove the overflow node and add the new node at the same
> > time. Note
> > +	 * it is possible that vevent == &veventq->overflow for sequence
> > update
> > +	 */
> > +	spin_lock(&eventq->lock);
> > +	if (veventq->overflow.on_list) {
> > +		list_del(&veventq->overflow.node);
> > +		veventq->overflow.on_list = false;
> > +	}
> 
> We can save one field 'on_list' in every entry by:
> 
> 	if (list_is_last(&veventq->overflow.node, &eventq->deliver))
> 		list_del(&veventq->overflow.node);

Hmm. Given that the overflow node, if being on the list, should be
always the last one... yes!

> > +struct iommufd_vevent_header {
> > +	__aligned_u64 flags;
> > +	__u32 sequence;
> > +	__u32 __reserved;
> > +};
> 
> Is there a reason that flags must be u64? At a glance all flags fields
> (except the one in iommu_hwpt_vtd_s1) in iommufd uAPIs are u32
> which can cut the size of the header by half...

Not having a particular reason yet. Just thought that a 64-bit
could make the uAPI more expandable. It's true that u32 would
be cleaner. I will make a change.

> 
> > +void iommufd_veventq_abort(struct iommufd_object *obj)
> > +{
> > +	struct iommufd_eventq *eventq =
> > +		container_of(obj, struct iommufd_eventq, obj);
> > +	struct iommufd_veventq *veventq = eventq_to_veventq(eventq);
> > +	struct iommufd_viommu *viommu = veventq->viommu;
> > +	struct iommufd_vevent *cur, *next;
> > +
> > +	lockdep_assert_held_write(&viommu->veventqs_rwsem);
> > +
> > +	list_for_each_entry_safe(cur, next, &eventq->deliver, node) {
> > +		list_del(&cur->node);
> > +		kfree(cur);
> 
> kfree() doesn't apply to the overflow node.

Oh right, that's missed.

> otherwise it looks good to me:
> 
> Reviewed-by: Kevin Tian <kevin.tian@...el.com>

Thanks!
Nicolin