linux-kernel - RE: [PATCH v5 06/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: <BN9PR11MB5276AAC06F5B90D09923677F8C1F2@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Mon, 13 Jan 2025 02:52:32 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Nicolin Chen <nicolinc@...dia.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 v5 06/14] iommufd: Add IOMMUFD_OBJ_VEVENTQ and
 IOMMUFD_CMD_VEVENTQ_ALLOC

> From: Nicolin Chen <nicolinc@...dia.com>
> Sent: Saturday, January 11, 2025 5:29 AM
> 
> On Fri, Jan 10, 2025 at 07:06:49AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@...dia.com>
> > > Sent: Wednesday, January 8, 2025 1:10 AM
> > > +
> > > +int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
> > > +{
> > > +	struct iommu_veventq_alloc *cmd = ucmd->cmd;
> > > +	struct iommufd_veventq *veventq;
> > > +	struct iommufd_viommu *viommu;
> > > +	int fdno;
> > > +	int rc;
> > > +
> > > +	if (cmd->flags || cmd->type == IOMMU_VEVENTQ_TYPE_DEFAULT)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> > > +	if (IS_ERR(viommu))
> > > +		return PTR_ERR(viommu);
> > > +
> > > +	if (!viommu->ops || !viommu->ops->supports_veventq ||
> > > +	    !viommu->ops->supports_veventq(cmd->type))
> > > +		return -EOPNOTSUPP;
> > > +
> >
> > I'm not sure about the necessity of above check. The event queue
> > is just a software struct with a user-specified format for the iommu
> > driver to report viommu event. The struct itself is not constrained
> > by the hardware capability, though I'm not sure a real usage in
> > which a smmu driver wants to report a vtd event. But legitimately
> > an user can create any type of event queues which might just be
> > never used.
> 
> Allowing a random type that a driver will never use for reporting
> doesn't sound to make a lot of sense to me...
> 
> That being said, yea..I guess we could drop the limit here, since
> it isn't going to break anything?
> 
> > It sounds clearer to do the check when IOPF cap is actually enabled
> > on a device contained in the viommu. At that point check whether
> > a required type eventqueue has been created. If not then fail the
> > iopf enabling.
> 
> Hmm, isn't IOPF a different channel?

We have a fault queue for delivering IOPF on hwpt, when vIOMMU is
not involved

Now with vIOMMU my understanding was that all events including
IOPF are delivered via the event queue in the vIOMMU. Just echoed
by the documentation patch:

+- IOMMUFD_OBJ_VEVENTQ, representing a software queue for a vIOMMU to report its
+  events such as translation faults occurred to a nested stage-1 and HW-specific
+  events.

> 
> And a vEVENTQ is per vIOMMU, not necessarily per vDEVICE/device..

Yes. My point was to verify whether the vEVENTQ type is compatible when
a nested faultable hwpt is created with vIOMMU as the parent. then when
attaching a device to the nested hwpt we dynamically turn on PRI on the
device just like how it's handled in the fault queue path.

> 
> > Then it reveals probably another todo in this series. Seems you still
> > let the smmu driver statically enable iopf when probing the device.
> > Sounds like iommufd_viommu_alloc_hwpt_nested() may accept
> > IOMMU_HWPT_FAULT_ID_VALID to refer to a event queue and
> > later dynamically enable/disable iopf when attaching a device to the
> > hwpt and check the event queue type there. Just like how the fault
> > object is handled.
> 
> You've lost me here :-/
> 

Hope above explanation makes my point clearer. Then for a nested
hwpt created within a vIOMMU there is an open whether we want
a per-hwpt option to mark whether it allows fault, or assume that
every nested hwpt (and the devices attached to it) must be faultable
once any vEVENTQ is created in the vIOMMU.