Re: [PATCH v2 13/40] vfio: Add support for Shared Virtual Addressing

Xu Zaibo
2018-05-24T12:35:08Z

[-- Attachment #1.1: Type: text/plain, Size: 2914 bytes --]

On 2018/5/24 19:44, Jean-Philippe Brucker wrote:
> Hi,
>
> On 23/05/18 10:38, Xu Zaibo wrote:
>>> +static int vfio_iommu_bind_group(struct vfio_iommu *iommu,
>>> + struct vfio_group *group,
>>> + struct vfio_mm *vfio_mm)
>>> +{
>>> + int ret;
>>> + bool enabled_sva = false;
>>> + struct vfio_iommu_sva_bind_data data = {
>>> + .vfio_mm = vfio_mm,
>>> + .iommu = iommu,
>>> + .count = 0,
>>> + };
>>> +
>>> + if (!group->sva_enabled) {
>>> + ret = iommu_group_for_each_dev(group->iommu_group, NULL,
>>> + vfio_iommu_sva_init);
>> Do we need to do *sva_init here or do anything to avoid repeated
>> initiation?
>> while another process already did initiation at this device, I think
>> that current process will get an EEXIST.
> Right, sva_init() must be called once for any device that intends to use
> bind(). For the second process though, group->sva_enabled will be true
> so we won't call sva_init() again, only bind().
>
Well, while I create mediated devices based on one parent device to
support multiple
processes(a new process will create a new 'vfio_group' for the
corresponding mediated device,
and 'sva_enabled' cannot work any more), in fact, *sva_init and
*sva_shutdown are basically
working on parent device, so, as a result, I just only need sva
initiation and shutdown on the
parent device only once. So I change the two as following:

/@@ -551,8 +565,18 @@ int iommu_sva_device_init(struct device *dev,
unsigned long features,/
if (features & ~IOMMU_SVA_FEAT_IOPF)
return -EINVAL;

/+ /* If already exists, do nothing *///
//+ mutex_lock(&dev->iommu_param->lock);//
//+ if (dev->iommu_param->sva_param) {//
//+ mutex_unlock(&dev->iommu_param->lock);//
//+ return 0;//
//+ }//
//+ mutex_unlock(&dev->iommu_param->lock);//
////
// if (features & IOMMU_SVA_FEAT_IOPF) {//
// ret = iommu_register_device_fault_handler(dev,
iommu_queue_iopf,///// /
//
//
//@@ -621,6 +646,14 @@ int iommu_sva_device_shutdown(struct device *dev)//
// if (!domain)//
// return -ENODEV;//
////
//+ /* If any other process is working on the device, shut down does
nothing. *///
//+ mutex_lock(&dev->iommu_param->lock);//
//+ if (!list_empty(&dev->iommu_param->sva_param->mm_list)) {//
//+ mutex_unlock(&dev->iommu_param->lock);//
//+ return 0;//
//+ }//
//+ mutex_unlock(&dev->iommu_param->lock);//
//+//
// __iommu_sva_unbind_dev_all(dev);//
////
// mutex_lock(&dev->iommu_param->lock);/

I add the above two checkings in both *sva_init and *sva_shutdown, it is
working now,
but i don't know if it will cause any new problems. What's more, i doubt
if it is
reasonable to check this to avoid repeating operation in VFIO, maybe it
is better to check
in IOMMU. :)

Thanks
Zaibo
>
> .
>

[-- Attachment #1.2: Type: text/html, Size: 42100 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]