| 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: <7f8daef9-36db-f67c-a3e2-b96b5fa70291@linux.intel.com>
Date: Wed, 10 Mar 2021 10:09:04 +0800
From: Lu Baolu <baolu.lu@...ux.intel.com>
To: Shenming Lu <lushenming@...wei.com>,
Alex Williamson <alex.williamson@...hat.com>,
Cornelia Huck <cohuck@...hat.com>,
Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>,
Joerg Roedel <joro@...tes.org>,
Jean-Philippe Brucker <jean-philippe@...aro.org>,
Eric Auger <eric.auger@...hat.com>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
iommu@...ts.linux-foundation.org, linux-api@...r.kernel.org
Cc: baolu.lu@...ux.intel.com, Kevin Tian <kevin.tian@...el.com>,
yi.l.liu@...el.com, Christoph Hellwig <hch@...radead.org>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Barry Song <song.bao.hua@...ilicon.com>,
wanghaibin.wang@...wei.com, yuzenghui@...wei.com,
zhukeqian1@...wei.com
Subject: Re: [RFC PATCH v2 1/6] iommu: Evolve to support more scenarios of
using IOPF
Hi Shenming,
On 3/9/21 2:22 PM, Shenming Lu wrote:
> This patch follows the discussion here:
>
> https://lore.kernel.org/linux-acpi/YAaxjmJW+ZMvrhac@myrica/
>
> In order to support more scenarios of using IOPF (mainly consider
> the nested extension), besides keeping IOMMU_DEV_FEAT_IOPF as a
> general capability for whether delivering faults through the IOMMU,
> we extend iommu_register_fault_handler() with flags and introduce
> IOPF_REPORT_FLAT and IOPF_REPORT_NESTED to describe the page fault
> reporting capability under a specific configuration.
> IOPF_REPORT_NESTED needs additional info to indicate which level/stage
> is concerned since the fault client may be interested in only one
> level.
>
> Signed-off-by: Shenming Lu <lushenming@...wei.com>
> ---
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++--
> drivers/iommu/io-pgfault.c | 4 --
> drivers/iommu/iommu.c | 56 ++++++++++++++++++-
> include/linux/iommu.h | 21 ++++++-
> include/uapi/linux/iommu.h | 3 +
> 6 files changed, 85 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index ee66d1f4cb81..5de9432349d4 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -482,7 +482,8 @@ static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
> if (ret)
> return ret;
>
> - ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
> + ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
> + IOPF_REPORT_FLAT, dev);
> if (ret) {
> iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> return ret;
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 363744df8d51..f40529d0075d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1447,10 +1447,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> return -EOPNOTSUPP;
> }
>
> - /* Stage-2 is always pinned at the moment */
> - if (evt[1] & EVTQ_1_S2)
> - return -EFAULT;
> -
> if (evt[1] & EVTQ_1_RnW)
> perm |= IOMMU_FAULT_PERM_READ;
> else
> @@ -1468,13 +1464,18 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> .perm = perm,
> - .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> };
>
> if (ssid_valid) {
> flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> }
> +
> + if (evt[1] & EVTQ_1_S2) {
> + flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_L2;
> + flt->prm.addr = FIELD_GET(EVTQ_3_IPA, evt[3]);
> + } else
> + flt->prm.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]);
> } else {
> flt->type = IOMMU_FAULT_DMA_UNRECOV;
> flt->event = (struct iommu_fault_unrecoverable) {
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 1df8c1dcae77..abf16e06bcf5 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -195,10 +195,6 @@ int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
>
> lockdep_assert_held(¶m->lock);
>
> - if (fault->type != IOMMU_FAULT_PAGE_REQ)
> - /* Not a recoverable page fault */
> - return -EOPNOTSUPP;
> -
Any reasons why do you want to remove this check?
> /*
> * As long as we're holding param->lock, the queue can't be unlinked
> * from the device and therefore cannot disappear.
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d0b0a15dba84..cb1d93b00f7d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1056,6 +1056,40 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
> }
> EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>
> +/*
> + * iommu_update_device_fault_handler - Update the device fault handler via flags
> + * @dev: the device
> + * @mask: bits(not set) to clear
> + * @set: bits to set
> + *
> + * Update the device fault handler installed by
> + * iommu_register_device_fault_handler().
> + *
> + * Return 0 on success, or an error.
> + */
> +int iommu_update_device_fault_handler(struct device *dev, u32 mask, u32 set)
> +{
> + struct dev_iommu *param = dev->iommu;
> + int ret = 0;
> +
> + if (!param)
> + return -EINVAL;
> +
> + mutex_lock(¶m->lock);
> +
> + if (param->fault_param) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> + param->fault_param->flags = (param->fault_param->flags & mask) | set;
> +
> +out_unlock:
> + mutex_unlock(¶m->lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_update_device_fault_handler);
When and why will this API be used? Why not registering the fault
handling capabilities of a device driver only once during probe()?
> +
> /**
> * iommu_register_device_fault_handler() - Register a device fault handler
> * @dev: the device
> @@ -1076,11 +1110,14 @@ EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
> */
> int iommu_register_device_fault_handler(struct device *dev,
> iommu_dev_fault_handler_t handler,
> - void *data)
> + u32 flags, void *data)
> {
> struct dev_iommu *param = dev->iommu;
> int ret = 0;
>
> + if (flags & IOPF_REPORT_FLAT && flags & IOPF_REPORT_NESTED)
> + return -EINVAL;
> +
> if (!param)
> return -EINVAL;
>
> @@ -1099,6 +1136,7 @@ int iommu_register_device_fault_handler(struct device *dev,
> goto done_unlock;
> }
> param->fault_param->handler = handler;
> + param->fault_param->flags = flags;
> param->fault_param->data = data;
> mutex_init(¶m->fault_param->lock);
> INIT_LIST_HEAD(¶m->fault_param->faults);
> @@ -1177,6 +1215,22 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
> goto done_unlock;
> }
>
> + /* The unrecoverable fault reporting is not supported at the moment. */
> + if (evt->fault.type != IOMMU_FAULT_PAGE_REQ)
> + return -EOPNOTSUPP;
Any reasons why do you want to disable reporting an unrecoverable fault?
> +
> + if (evt->fault.type == IOMMU_FAULT_PAGE_REQ) {
> + if (fparam->flags & IOPF_REPORT_NESTED) {
> + if (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_L2 &&
> + !(fparam->flags & IOPF_REPORT_NESTED_L2_CONCERNED))
> + return -EOPNOTSUPP;
> + if (!(evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_L2) &&
> + !(fparam->flags & IOPF_REPORT_NESTED_L1_CONCERNED))
> + return -EOPNOTSUPP;
> + } else if (!(fparam->flags & IOPF_REPORT_FLAT))
> + return -EOPNOTSUPP;
> + }
> +
> if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
> (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> evt_pending = kmemdup(evt, sizeof(struct iommu_fault_event),
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 86d688c4418f..f03d761e8310 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -352,12 +352,21 @@ struct iommu_fault_event {
> /**
> * struct iommu_fault_param - per-device IOMMU fault data
> * @handler: Callback function to handle IOMMU faults at device level
> + * @flags: Indicates whether the fault reporting is available under a
> + * specific configuration (1st/2nd-level-only(FLAT), or nested).
> + * IOPF_REPORT_NESTED needs to additionally know which level/stage
> + * is concerned.
If IOPF_REPORT_NESTED only is not valid why do you want to define it?
> * @data: handler private data
> * @faults: holds the pending faults which needs response
> * @lock: protect pending faults list
> */
> struct iommu_fault_param {
> iommu_dev_fault_handler_t handler;
> +#define IOPF_REPORT_FLAT (1 << 0)
> +#define IOPF_REPORT_NESTED (1 << 1)
> +#define IOPF_REPORT_NESTED_L1_CONCERNED (1 << 2)
> +#define IOPF_REPORT_NESTED_L2_CONCERNED (1 << 3)
> + u32 flags;
> void *data;
> struct list_head faults;
> struct mutex lock;
> @@ -509,9 +518,11 @@ extern int iommu_group_register_notifier(struct iommu_group *group,
> struct notifier_block *nb);
> extern int iommu_group_unregister_notifier(struct iommu_group *group,
> struct notifier_block *nb);
> +extern int iommu_update_device_fault_handler(struct device *dev,
> + u32 mask, u32 set);
> extern int iommu_register_device_fault_handler(struct device *dev,
> iommu_dev_fault_handler_t handler,
> - void *data);
> + u32 flags, void *data);
>
> extern int iommu_unregister_device_fault_handler(struct device *dev);
>
> @@ -873,10 +884,16 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
> return 0;
> }
>
> +static inline int iommu_update_device_fault_handler(struct device *dev,
> + u32 mask, u32 set)
> +{
> + return -ENODEV;
> +}
> +
> static inline
> int iommu_register_device_fault_handler(struct device *dev,
> iommu_dev_fault_handler_t handler,
> - void *data)
> + u32 flags, void *data)
> {
> return -ENODEV;
> }
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index e1d9e75f2c94..0ce0dfb7713e 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -85,6 +85,8 @@ struct iommu_fault_unrecoverable {
> * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
> * must have the same PASID value as the page request. When it is clear,
> * the page response should not have a PASID.
> + * If IOMMU_FAULT_PAGE_REQUEST_L2 is set, the fault occurred at the
> + * second level/stage, otherwise, occurred at the first level.
> * @pasid: Process Address Space ID
> * @grpid: Page Request Group Index
> * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
> @@ -96,6 +98,7 @@ struct iommu_fault_page_request {
> #define IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE (1 << 1)
> #define IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA (1 << 2)
> #define IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID (1 << 3)
> +#define IOMMU_FAULT_PAGE_REQUEST_L2 (1 << 4)
> __u32 flags;
> __u32 pasid;
> __u32 grpid;
>
Best regards,
baolu
Powered by blists - more mailing lists