[PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"
QCOM IOMMU driver calls bus_set_iommu() for every IOMMU device controller,
what fails for the second and latter IOMMU devices. This is intended and
must be not fatal to the driver registration process. Also the cleanup
path should take care of the runtime PM state, what is missing in the
current patch. Revert relevant changes to the QCOM IOMMU driver until
a proper fix is prepared.
This partially reverts commit 249c9dc6aa0db74a0f7908efd04acf774e19b155.
Fixes: 249c9dc6aa0d ("iommu/arm: Cleanup resources in case of probe error path")
Suggested-by: Will Deacon
Signed-off-by: Marek Szyprowski
---
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 25ed444ff94d..021cf8f65ffc 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -849,12 +849,10 @@ static int qcom_iommu_device_probe(struct platform_device
*pdev)
ret = iommu_device_register(&qcom_iommu->iommu, &qcom_iommu_ops, dev);
if (ret) {
dev_err(dev, "Failed to register iommu\n");
- goto err_sysfs_remove;
+ return ret;
}
- ret = bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
- if (ret)
- goto err_unregister_device;
+ bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
if (qcom_iommu->local_base) {
pm_runtime_get_sync(dev);
@@ -863,13 +861,6 @@ static int qcom_iommu_device_probe(struct platform_device
*pdev)
}
return 0;
-
-err_unregister_device:
- iommu_device_unregister(&qcom_iommu->iommu);
-
-err_sysfs_remove:
- iommu_device_sysfs_remove(&qcom_iommu->iommu);
- return ret;
}
static int qcom_iommu_device_remove(struct platform_device *pdev)
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 10/10] Documentation: Add documentation for VDUSE
在 2021/7/4 下午5:49, Yongji Xie 写道: OK, I get you now. Since the VIRTIO specification says "Device configuration space is generally used for rarely-changing or initialization-time parameters". I assume the VDUSE_DEV_SET_CONFIG ioctl should not be called frequently. The spec uses MUST and other terms to define the precise requirements. Here the language (especially the word "generally") is weaker and means there may be exceptions. Another type of access that doesn't work with the VDUSE_DEV_SET_CONFIG approach is reads that have side-effects. For example, imagine a field containing an error code if the device encounters a problem unrelated to a specific virtqueue request. Reading from this field resets the error code to 0, saving the driver an extra configuration space write access and possibly race conditions. It isn't possible to implement those semantics suing VDUSE_DEV_SET_CONFIG. It's another corner case, but it makes me think that the interface does not allow full VIRTIO semantics. Note that though you're correct, my understanding is that config space is not suitable for this kind of error propagating. And it would be very hard to implement such kind of semantic in some transports. Virtqueue should be much better. As Yong Ji quoted, the config space is used for "rarely-changing or intialization-time parameters". Agreed. I will use VDUSE_DEV_GET_CONFIG in the next version. And to handle the message failure, I'm going to add a return value to virtio_config_ops.get() and virtio_cread_* API so that the error can be propagated to the virtio device driver. Then the virtio-blk device driver can be modified to handle that. Jason and Stefan, what do you think of this way? I'd like to stick to the current assumption thich get_config won't fail. That is to say, 1) maintain a config in the kernel, make sure the config space read can always succeed 2) introduce an ioctl for the vduse usersapce to update the config space. 3) we can synchronize with the vduse userspace during set_config Does this work? Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 0/5] iommu/arm-smmu: adreno-smmu page fault handling
I suspect you are getting a dpu fault, and need: https://lore.kernel.org/linux-arm-msm/CAF6AEGvTjTUQXqom-xhdh456tdLscbVFPQ+iud1H1gHc8A2=h...@mail.gmail.com/ I suppose Bjorn was expecting me to send that patch BR, -R On Sun, Jul 4, 2021 at 5:53 AM Dmitry Baryshkov wrote: > > Hi, > > I've had splash screen disabled on my RB3. However once I've enabled it, > I've got the attached crash during the boot on the msm/msm-next. It > looks like it is related to this particular set of changes. > > On 11/06/2021 00:44, Rob Clark wrote: > > From: Rob Clark > > > > This picks up an earlier series[1] from Jordan, and adds additional > > support needed to generate GPU devcore dumps on iova faults. Original > > description: > > > > This is a stack to add an Adreno GPU specific handler for pagefaults. The > > first > > patch starts by wiring up report_iommu_fault for arm-smmu. The next patch > > adds > > a adreno-smmu-priv function hook to capture a handful of important debugging > > registers such as TTBR0, CONTEXTIDR, FSYNR0 and others. This is used by the > > third patch to print more detailed information on page fault such as the > > TTBR0 > > for the pagetable that caused the fault and the source of the fault as > > determined by a combination of the FSYNR1 register and an internal GPU > > register. > > > > This code provides a solid base that we can expand on later for even more > > extensive GPU side page fault debugging capabilities. > > > > v5: [Rob] Use RBBM_STATUS3.SMMU_STALLED_ON_FAULT to detect case where > > GPU snapshotting needs to avoid crashdumper, and check the > > RBBM_STATUS3.SMMU_STALLED_ON_FAULT in GPU hang irq paths > > v4: [Rob] Add support to stall SMMU on fault, and let the GPU driver > > resume translation after it has had a chance to snapshot the GPUs > > state > > v3: Always clear FSR even if the target driver is going to handle resume > > v2: Fix comment wording and function pointer check per Rob Clark > > > > [1] > > https://lore.kernel.org/dri-devel/20210225175135.91922-1-jcro...@codeaurora.org/ > > > > Jordan Crouse (3): > >iommu/arm-smmu: Add support for driver IOMMU fault handlers > >iommu/arm-smmu-qcom: Add an adreno-smmu-priv callback to get pagefault > > info > >drm/msm: Improve the a6xx page fault handler > > > > Rob Clark (2): > >iommu/arm-smmu-qcom: Add stall support > >drm/msm: devcoredump iommu fault support > > > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 23 +++- > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 110 +++- > > drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 42 ++-- > > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 +++ > > drivers/gpu/drm/msm/msm_gem.h | 1 + > > drivers/gpu/drm/msm/msm_gem_submit.c| 1 + > > drivers/gpu/drm/msm/msm_gpu.c | 48 + > > drivers/gpu/drm/msm/msm_gpu.h | 17 +++ > > drivers/gpu/drm/msm/msm_gpummu.c| 5 + > > drivers/gpu/drm/msm/msm_iommu.c | 22 +++- > > drivers/gpu/drm/msm/msm_mmu.h | 5 +- > > drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 50 + > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +- > > drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 + > > include/linux/adreno-smmu-priv.h| 38 ++- > > 15 files changed, 367 insertions(+), 21 deletions(-) > > > > > -- > With best wishes > Dmitry ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 0/5] iommu/arm-smmu: adreno-smmu page fault handling
Hi, I've had splash screen disabled on my RB3. However once I've enabled it, I've got the attached crash during the boot on the msm/msm-next. It looks like it is related to this particular set of changes. On 11/06/2021 00:44, Rob Clark wrote: From: Rob Clark This picks up an earlier series[1] from Jordan, and adds additional support needed to generate GPU devcore dumps on iova faults. Original description: This is a stack to add an Adreno GPU specific handler for pagefaults. The first patch starts by wiring up report_iommu_fault for arm-smmu. The next patch adds a adreno-smmu-priv function hook to capture a handful of important debugging registers such as TTBR0, CONTEXTIDR, FSYNR0 and others. This is used by the third patch to print more detailed information on page fault such as the TTBR0 for the pagetable that caused the fault and the source of the fault as determined by a combination of the FSYNR1 register and an internal GPU register. This code provides a solid base that we can expand on later for even more extensive GPU side page fault debugging capabilities. v5: [Rob] Use RBBM_STATUS3.SMMU_STALLED_ON_FAULT to detect case where GPU snapshotting needs to avoid crashdumper, and check the RBBM_STATUS3.SMMU_STALLED_ON_FAULT in GPU hang irq paths v4: [Rob] Add support to stall SMMU on fault, and let the GPU driver resume translation after it has had a chance to snapshot the GPUs state v3: Always clear FSR even if the target driver is going to handle resume v2: Fix comment wording and function pointer check per Rob Clark [1] https://lore.kernel.org/dri-devel/20210225175135.91922-1-jcro...@codeaurora.org/ Jordan Crouse (3): iommu/arm-smmu: Add support for driver IOMMU fault handlers iommu/arm-smmu-qcom: Add an adreno-smmu-priv callback to get pagefault info drm/msm: Improve the a6xx page fault handler Rob Clark (2): iommu/arm-smmu-qcom: Add stall support drm/msm: devcoredump iommu fault support drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 23 +++- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 110 +++- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 42 ++-- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 15 +++ drivers/gpu/drm/msm/msm_gem.h | 1 + drivers/gpu/drm/msm/msm_gem_submit.c| 1 + drivers/gpu/drm/msm/msm_gpu.c | 48 + drivers/gpu/drm/msm/msm_gpu.h | 17 +++ drivers/gpu/drm/msm/msm_gpummu.c| 5 + drivers/gpu/drm/msm/msm_iommu.c | 22 +++- drivers/gpu/drm/msm/msm_mmu.h | 5 +- drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 50 + drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +- drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 + include/linux/adreno-smmu-priv.h| 38 ++- 15 files changed, 367 insertions(+), 21 deletions(-) -- With best wishes Dmitry Handling Cmd: set_active:a SetActiveSlot: _a already active slot Handling Cmd: download:061b8800 Download Finished Handling Cmd: boot A/B retry count NOT decremented Booting Into Mission Mode No dtbo partition is found, Skip dtbo Exit key detection timer GetVmData: making ScmCall to get HypInfo GetVmData: No Vm data present! Status = (0x3) No Ffbm cookie found, ignore: Not Found Memory Base Address: 0x8000 Decompressing kernel image start: 32313 ms Decompressing kernel image done: 35477 ms BootLinux: failed to get dtbo image DTB offset is incorrect, kernel image does not have appended DTB Cmdline: ignore_loglevel console=ttyMSM0,115200n8 clk_ignore_unused pd_ignore_unused earlycon pcie_pme=nomsi root=PARTLABEL=userdata rootwait androidboot.bootdevice=1d84000.ufshc androidboot.serialno=8f186bb6 androidboot.baseband=msm msm_drm.dsi_display0= RAM Partitions Add Base: 0x8000 Available Length: 0xFDFA WARNING: Unsupported EFI_RAMPARTITION_PROTOCOL ERROR: Could not get splash memory region node kaslr-Seed is added to chosen node Shutting Down UEFI Boot Services: 36987 ms BDS: LogFs sync skipped, Unsupported App Log Flush : 0 ms Exit BS[37141] UEFI End [0.00] Booting Linux on physical CPU 0x00 [0x517f803c] [0.00] Linux version 5.13.0-rc3-00115-g9b6193ea776c-dirty (lumag@eriador) (aarch64-linux-gnu-gcc (Debian 11.1.0-1) 11.1.0, GNU ld (GNU Binutils for Debian) 2.35.2) #142 SMP PREEMPT Sun Jul 4 15:25:15 MSK 2021 [0.00] Machine model: Thundercomm Dragonboard 845c [0.00] printk: debug: ignoring loglevel setting. [0.00] earlycon: qcom_geni0 at MMIO 0x00a84000 (options '115200n8') [0.00] printk: bootconsole [qcom_geni0] enabled [0.00] efi: UEFI not found. [0.00] NUMA: No NUMA configuration found [0.00] NUMA: Faking a node at [mem 0x8000-0x00017df9] [0.00] NUMA: NODE_DATA [mem 0x17d78c200-0x17d78dfff] [0.00] Zone ranges: [0.00] DMA [mem 0x8000
Re: Re: Re: [PATCH v8 10/10] Documentation: Add documentation for VDUSE
On Thu, Jul 1, 2021 at 9:15 PM Stefan Hajnoczi wrote:
>
> On Thu, Jul 01, 2021 at 06:00:48PM +0800, Yongji Xie wrote:
> > On Wed, Jun 30, 2021 at 6:06 PM Stefan Hajnoczi wrote:
> > >
> > > On Tue, Jun 29, 2021 at 01:43:11PM +0800, Yongji Xie wrote:
> > > > On Mon, Jun 28, 2021 at 9:02 PM Stefan Hajnoczi
> > > > wrote:
> > > > > On Tue, Jun 15, 2021 at 10:13:31PM +0800, Xie Yongji wrote:
> > > > > > + static void *iova_to_va(int dev_fd, uint64_t iova, uint64_t
> > > > > > *len)
> > > > > > + {
> > > > > > + int fd;
> > > > > > + void *addr;
> > > > > > + size_t size;
> > > > > > + struct vduse_iotlb_entry entry;
> > > > > > +
> > > > > > + entry.start = iova;
> > > > > > + entry.last = iova + 1;
> > > > >
> > > > > Why +1?
> > > > >
> > > > > I expected the request to include *len so that VDUSE can create a
> > > > > bounce
> > > > > buffer for the full iova range, if necessary.
> > > > >
> > > >
> > > > The function is used to translate iova to va. And the *len is not
> > > > specified by the caller. Instead, it's used to tell the caller the
> > > > length of the contiguous iova region from the specified iova. And the
> > > > ioctl VDUSE_IOTLB_GET_FD will get the file descriptor to the first
> > > > overlapped iova region. So using iova + 1 should be enough here.
> > >
> > > Does the entry.last field have any purpose with VDUSE_IOTLB_GET_FD? I
> > > wonder why userspace needs to assign a value at all if it's always +1.
> > >
> >
> > If we need to get some iova regions in the specified range, we need
> > the entry.last field. For example, we can use [0, ULONG_MAX] to get
> > the first overlapped iova region which might be [4096, 8192]. But in
> > this function, we don't use VDUSE_IOTLB_GET_FD like this. We need to
> > get the iova region including the specified iova.
>
> I see, thanks for explaining!
>
> > > > > > + return addr + iova - entry.start;
> > > > > > + }
> > > > > > +
> > > > > > +- VDUSE_DEV_GET_FEATURES: Get the negotiated features
> > > > >
> > > > > Are these VIRTIO feature bits? Please explain how feature negotiation
> > > > > works. There must be a way for userspace to report the device's
> > > > > supported feature bits to the kernel.
> > > > >
> > > >
> > > > Yes, these are VIRTIO feature bits. Userspace will specify the
> > > > device's supported feature bits when creating a new VDUSE device with
> > > > ioctl(VDUSE_CREATE_DEV).
> > >
> > > Can the VDUSE device influence feature bit negotiation? For example, if
> > > the VDUSE virtio-blk device does not implement discard/write-zeroes, how
> > > does QEMU or the guest find out about this?
> > >
> >
> > There is a "features" field in struct vduse_dev_config which is used
> > to do feature negotiation.
>
> This approach is more restrictive than required by the VIRTIO
> specification:
>
> "The device SHOULD accept any valid subset of features the driver
> accepts, otherwise it MUST fail to set the FEATURES_OK device status
> bit when the driver writes it."
>
>
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-130002
>
> The spec allows a device to reject certain subsets of features. For
> example, if feature B depends on feature A and can only be enabled when
> feature A is also enabled.
>
> From your description I think VDUSE would accept feature B without
> feature A since the device implementation has no opportunity to fail
> negotiation with custom logic.
>
Yes, we discussed it [1] before. So I'd like to re-introduce
SET_STATUS messages so that the userspace can fail feature negotiation
during setting FEATURES_OK status bit.
[1] https://lkml.org/lkml/2021/6/28/1587
> Ideally VDUSE would send a SET_FEATURES message to userspace, allowing
> the device implementation full flexibility in which subsets of features
> to accept.
>
> This is a corner case. Many or maybe even all existing VIRTIO devices
> don't need this flexibility, but I want to point out this limitation in
> the VDUSE interface because it may cause issues in the future.
>
> > > > > > +- VDUSE_DEV_UPDATE_CONFIG: Update the configuration space and
> > > > > > inject a config interrupt
> > > > >
> > > > > Does this mean the contents of the configuration space are cached by
> > > > > VDUSE?
> > > >
> > > > Yes, but the kernel will also store the same contents.
> > > >
> > > > > The downside is that the userspace code cannot generate the
> > > > > contents on demand. Most devices doin't need to generate the contents
> > > > > on demand, so I think this is okay but I had expected a different
> > > > > interface:
> > > > >
> > > > > kernel->userspace VDUSE_DEV_GET_CONFIG
> > > > > userspace->kernel VDUSE_DEV_INJECT_CONFIG_IRQ
> > > > >
> > > >
> > > > The problem is how to handle the failure of VDUSE_DEV_GET_CONFIG. We
> > > > will need lots of modification of virtio codes to support that. So to
> > > > make it simple, we choose this way:
>
Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
On Tue, Jun 29, 2021 at 7:34 PM Jon Nettleton wrote:
>
> On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
> wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Robin Murphy [mailto:robin.mur...@arm.com]
> > > Sent: 14 June 2021 12:23
> > > To: Shameerali Kolothum Thodi ;
> > > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > > iommu@lists.linux-foundation.org
> > > Cc: j...@solid-run.com; Linuxarm ;
> > > steven.pr...@arm.com; Guohanjun (Hanjun Guo) ;
> > > yangyicong ; sami.muja...@arm.com;
> > > wanghuiqiang
> > > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > > regions
> > >
> > > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > > Add a helper function that retrieves RMR memory descriptors
> > > > associated with a given IOMMU. This will be used by IOMMU
> > > > drivers to setup necessary mappings.
> > > >
> > > > Now that we have this, invoke it from the generic helper
> > > > interface.
> > > >
> > > > Signed-off-by: Shameer Kolothum
> > >
> > > > ---
> > > > drivers/acpi/arm64/iort.c | 50
> > > +++
> > > > drivers/iommu/dma-iommu.c | 4
> > > > include/linux/acpi_iort.h | 7 ++
> > > > 3 files changed, 61 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index fea1ffaedf3b..01917caf58de 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -12,6 +12,7 @@
> > > >
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > > device *dev)
> > > > return err;
> > > > }
> > > >
> > > > +/**
> > > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > > IOMMU
> > > > + * @iommu: fwnode for the IOMMU
> > > > + * @head: RMR list head to be populated
> > > > + *
> > > > + * Returns: 0 on success, <0 failure
> > > > + */
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > + struct list_head *head)
> > > > +{
> > > > + struct iort_rmr_entry *e;
> > > > + struct acpi_iort_node *iommu;
> > > > + int rmrs = 0;
> > > > +
> > > > + iommu = iort_get_iort_node(iommu_fwnode);
> > > > + if (!iommu || list_empty(&iort_rmr_list))
> > > > + return -ENODEV;
> > > > +
> > > > + list_for_each_entry(e, &iort_rmr_list, list) {
> > > > + int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > > IOMMU_MMIO;
> > > > + struct iommu_resv_region *region;
> > > > + enum iommu_resv_type type;
> > > > + struct acpi_iort_rmr_desc *rmr_desc;
> > > > +
> > > > + if (e->smmu != iommu)
> > > > + continue;
> > > > +
> > > > + rmr_desc = e->rmr_desc;
> > > > + if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > > + type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > > + else
> > > > + type = IOMMU_RESV_DIRECT;
> > >
>
> I have been looking at this a bit more and looking at the history of
> RMR_REMAP_PERMITTED. Based on the history I have found it
> seems to me like this would be the better options for prot.
>
> @@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> return -ENODEV;
>
> list_for_each_entry(e, &iort_rmr_list, list) {
> - int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> IOMMU_MMIO;
> + int prot = IOMMU_READ | IOMMU_WRITE;
> struct iommu_resv_region *region;
> enum iommu_resv_type type;
> struct acpi_iort_rmr_desc *rmr_desc;
> @@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> continue;
>
> rmr_desc = e->rmr_desc;
> - if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> + if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
> type = IOMMU_RESV_DIRECT_RELAXABLE;
> - else
> + prot |= IOMMU_CACHE;
> + } else {
> type = IOMMU_RESV_DIRECT;
> + prot |= IOMMU_MMIO;
> + }
>
> region = iommu_alloc_resv_region(rmr_desc->base_address,
> rmr_desc->length,
>
> any input on this? My reasoning is that IOMMU_RESV_DIRECT is specifically
> referenced for things like MSI doorbell and in all the examples needs
> IOMMU_MMIO, while REMAP_PERMITTED seems to be used for allocated
> system memory that is then used for device specific reserved regions which
> commits say would be like a GPU or USB device.
>
> -Jon
>
> > > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > > we don't even have iort_rmr_entry, we store them as iommu_resv_regions
