| 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
| ||
|
Message-ID: <Z6EMtXD6Gaq3+eVw@Asurada-Nvidia>
Date: Mon, 3 Feb 2025 10:36:37 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "will@...nel.org" <will@...nel.org>, "robin.murphy@....com"
<robin.murphy@....com>, "jgg@...dia.com" <jgg@...dia.com>,
"tglx@...utronix.de" <tglx@...utronix.de>, "maz@...nel.org" <maz@...nel.org>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>, "joro@...tes.org"
<joro@...tes.org>, "shuah@...nel.org" <shuah@...nel.org>, "Chatre, Reinette"
<reinette.chatre@...el.com>, "eric.auger@...hat.com" <eric.auger@...hat.com>,
"yebin10@...wei.com" <yebin10@...wei.com>, "apatel@...tanamicro.com"
<apatel@...tanamicro.com>, "shivamurthy.shastri@...utronix.de"
<shivamurthy.shastri@...utronix.de>, "bhelgaas@...gle.com"
<bhelgaas@...gle.com>, "anna-maria@...utronix.de" <anna-maria@...utronix.de>,
"yury.norov@...il.com" <yury.norov@...il.com>, "nipun.gupta@....com"
<nipun.gupta@....com>, "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "kvm@...r.kernel.org"
<kvm@...r.kernel.org>, "linux-kselftest@...r.kernel.org"
<linux-kselftest@...r.kernel.org>, "patches@...ts.linux.dev"
<patches@...ts.linux.dev>, "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>
Subject: Re: [PATCH RFCv2 09/13] iommufd: Add IOMMU_OPTION_SW_MSI_START/SIZE
ioctls
On Thu, Jan 23, 2025 at 10:07:13AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@...dia.com>
> > Sent: Saturday, January 11, 2025 11:32 AM
> >
> > @@ -294,7 +294,9 @@ struct iommu_ioas_unmap {
> >
> > /**
> > * enum iommufd_option - ioctl(IOMMU_OPTION_RLIMIT_MODE) and
> > - * ioctl(IOMMU_OPTION_HUGE_PAGES)
> > + * ioctl(IOMMU_OPTION_HUGE_PAGES) and
> > + * ioctl(IOMMU_OPTION_SW_MSI_START) and
> > + * ioctl(IOMMU_OPTION_SW_MSI_SIZE)
> > * @IOMMU_OPTION_RLIMIT_MODE:
> > * Change how RLIMIT_MEMLOCK accounting works. The caller must have
> > privilege
> > * to invoke this. Value 0 (default) is user based accounting, 1 uses process
> > @@ -304,10 +306,24 @@ struct iommu_ioas_unmap {
> > * iommu mappings. Value 0 disables combining, everything is mapped to
> > * PAGE_SIZE. This can be useful for benchmarking. This is a per-IOAS
> > * option, the object_id must be the IOAS ID.
> > + * @IOMMU_OPTION_SW_MSI_START:
> > + * Change the base address of the IOMMU mapping region for MSI
> > doorbell(s).
> > + * It must be set this before attaching a device to an IOAS/HWPT,
>
> remove 'this'
Ack.
> > otherwise
> > + * this option will be not effective on that IOAS/HWPT. User can
>
> Do we want to explicitly check this instead of leaving it no effect
> silently?
So, the idea here is:
If this option is unset, use the default SW_MSI from the driver
If this option is set, use it over the default SW_MSI from the driver
That's what the following statement "User can choose to let.." means.
> > choose to
> > + * let kernel pick a base address, by simply ignoring this option or setting
> > + * a value 0 to IOMMU_OPTION_SW_MSI_SIZE. Global option, object_id
> > must be 0
> > + * @IOMMU_OPTION_SW_MSI_SIZE:
> > + * Change the size of the IOMMU mapping region for MSI doorbell(s). It
> > must
> > + * be set this before attaching a device to an IOAS/HWPT, otherwise it
> > won't
> > + * be effective on that IOAS/HWPT. The value is in MB, and the minimum
> > value
> > + * is 1 MB. A value 0 (default) will invalidate the MSI doorbell base address
> > + * value set to IOMMU_OPTION_SW_MSI_START. Global option, object_id
> > must be 0
>
> hmm there is no check on the minimal value and enable the effect
> of value 0 in this patch.
Well, it's somewhat enforced by __aligned_u64 since it can't be any
value between 0 (disable) and 1 (minimal)?
And the override code checks "ctx->sw_msi_size".
> > iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
> > struct iommufd_hwpt_paging
> > *hwpt_paging)
> > {
> > + struct iommufd_ctx *ictx = idev->ictx;
> > int rc;
> >
> > lockdep_assert_held(&idev->igroup->lock);
> >
> > + /* Override it with a user-programmed SW_MSI region */
> > + if (ictx->sw_msi_size && ictx->sw_msi_start != PHYS_ADDR_MAX)
> > + idev->igroup->sw_msi_start = ictx->sw_msi_start;
> > rc = iopt_table_enforce_dev_resv_regions(&hwpt_paging->ioas->iopt,
> > idev->dev,
> > &idev->igroup-
> > >sw_msi_start);
>
> what about moving above additions into
> iopt_table_enforce_dev_resv_regions() which is all about finding
> a sw_msi address and can check the user setting internally?
We could. Probably would be cleaner by doing that in one place.
> > diff --git a/drivers/iommu/iommufd/io_pagetable.c
> > b/drivers/iommu/iommufd/io_pagetable.c
> > index 8a790e597e12..5d7f5ca1eecf 100644
> > --- a/drivers/iommu/iommufd/io_pagetable.c
> > +++ b/drivers/iommu/iommufd/io_pagetable.c
> > @@ -1446,7 +1446,9 @@ int iopt_table_enforce_dev_resv_regions(struct
> > io_pagetable *iopt,
> > if (sw_msi_start && resv->type == IOMMU_RESV_MSI)
> > num_hw_msi++;
> > if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI) {
> > - *sw_msi_start = resv->start;
> > + /* Bypass the driver-defined SW_MSI region, if preset
> > */
> > + if (*sw_msi_start == PHYS_ADDR_MAX)
> > + *sw_msi_start = resv->start;
>
> the code is not about bypass. Instead it's to use the driver-defined
> region if user doesn't set it.
Ack:
/* If being unset, Use the default IOMMU_RESV_SW_MSI */
Thanks
Nicolin
Powered by blists - more mailing lists