linux-kernel - RE: [PATCH RFCv2 09/13] iommufd: Add IOMMU_OPTION_SW_MSI

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: <BN9PR11MB527616A2EA6C64824576E58F8CE02@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Thu, 23 Jan 2025 10:07:13 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Nicolin Chen <nicolinc@...dia.com>, "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>
CC: "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

> 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'

> 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?

> 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.

>  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?

> 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.