On 13/02/18 23:43, Tian, Kevin wrote: >> From: Jean-Philippe Brucker >> Sent: Tuesday, February 13, 2018 8:40 PM >> >> >> [...] >>>> + >>>> +/** >>>> + * iommu_sva_device_init() - Initialize Shared Virtual Addressing for a >>>> device >>>> + * @dev: the device >>>> + * @features: bitmask of features that need to be initialized >>>> + * @max_pasid: max PASID value supported by the device >>>> + * >>>> + * Users of the bind()/unbind() API must call this function to initialize all >>>> + * features required for SVA. >>>> + * >>>> + * - If the device should support multiple address spaces (e.g. PCI >> PASID), >>>> + * IOMMU_SVA_FEAT_PASID must be requested. >>> >>> I think it is by default assumed when using this API, based on definition of >>> SVA. Can you elaborate the situation where this flag can be cleared? >> >> When passing a device to userspace, you could also share its non-pasid >> address space with the process. It requires a new domain type so is left >> as a TODO in patch 2/37. I did get requests for this feature, though I >> think it was mostly for prototyping. I guess I could remove the flag, and >> reintroduce it as IOMMU_SVA_FEAT_NO_PASID later on. > > sorry I still didn't get the definition of non-pasid address space. > Did you mean the GPA/IOVA address space and no_pasid implies > actually some default PASID associated? Yes I mean merging the process address space and IOVA space. There are no PASIDs involved if the device or the IOMMU doesn't support it. Instead of private DMA page tables you program the mm pgd into the IOMMU. A VFIO userspace driver, instead of sending MAP/UNMAP ioctl, could simply issue a BIND. Technically nothing prevents it, but now the resv problem discussed on patch 2/37 stands out. For example on x86 you'd probably need to carve the IOAPIC MSI range out of the process address space. On Arm you'd need to create a write-only mapping for MSIs (IOMMU translates it to the IRQ chip address, but thankfully accessing the doorbell from CPU side doesn't trigger an MSI.) >> [...] >>>> + ret = domain->ops->sva_device_init(dev, features, &min_pasid, >>>> + &max_pasid); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* FIXME: racy. Next version should have a mutex (same as fault >>>> handler) */ >>>> + dev_param->sva_features = features; >>>> + dev_param->min_pasid = min_pasid; >>>> + dev_param->max_pasid = max_pasid; >>> >>> what's the point of min_pasid here? >> >> Arm SMMUv3 uses entry 0 of the PASID table for the default (non-pasid) >> context, so it needs to set min_pasid to 1. AMD IOMMU recently added a >> similar feature (GIoSup), if I understood correctly. >> > > just for such purpose maybe we should just define a reserved_pasid > otherwise there will be some waste if an implementation allows it > non-zero. What's wasted? It's slightly simpler to use min_pasid because we just pass that limit to idr_alloc(). With a reserved_pasid we'll have to call idr_alloc(reserved_pasid) once, for the same result. Thanks, Jean
