Re: [PATCH v6 15/20] vfio/mdev: idxd: ims domain setup for the vdcm

Dave Jiang
2021-05-27T00:22:29Z

On 5/23/2021 4:50 PM, Jason Gunthorpe wrote:
> On Fri, May 21, 2021 at 05:20:37PM -0700, Dave Jiang wrote:
>> @@ -77,8 +80,18 @@ int idxd_mdev_host_init(struct idxd_device *idxd, struct mdev_driver *drv)
>> return rc;
>> }
>>
>> + ims_info.max_slots = idxd->ims_size;
>> + ims_info.slots = idxd->reg_base + idxd->ims_offset;
>> + idxd->ims_domain = pci_ims_array_create_msi_irq_domain(idxd->pdev, &ims_info);
>> + if (!idxd->ims_domain) {
>> + dev_warn(dev, "Fail to acquire IMS domain\n");
>> + iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_AUX);
>> + return -ENODEV;
>> + }
> I'm quite surprised that every mdev doesn't create its own ims_domain
> in its probe function.
>
> This places a global total limit on the # of vectors which makes me
> ask what was the point of using IMS in the first place ?
>
> The entire idea for IMS was to make the whole allocation system fully
> dynamic based on demand.

Hi Jason, thank you for the review of the series.

My understanding is that the driver creates a single IMS domain for the
device and provides the address base and IMS numbers for the domain
based on device IMS resources. So the IMS region needs to be contiguous.
Each mdev can call msi_domain_alloc_irqs() and acquire the number of IMS
vectors it desires and the DEV MSI core code will keep track of which
vectors are being used. This allows the mdev devices to dynamically
allocate based on demand. If the driver allocates a domain per mdev,
it'll needs to do internal accounting of the base and vector numbers for
each of those domains that the MSI core already provides. Isn't that
what we are trying to avoid? As mdevs come and go, that partitioning
will become fragmented.

For example, mdev 0 allocates 1 vector, mdev 1 allocates 2 vectors, and
mdev 3 allocates 3 vector. You have 1 vectors unallocated. When mdev 1
goes away and a new mdev shows up wanting 3 vectors, you won't be able
to allocate the domain because of fragmentation even though you have
enough vectors.

If all mdevs allocate the same IMS numbers, the fragmentation issue does
not exist. But the driver still has to keep track of which vectors are
free and which ones are used in order to provide the appropriate base.
And dev msi core already does this for us if we have a single domain.
Feels like we would just be duplicating code doing the same thing?

>
>> rc = mdev_register_device(dev, drv);
>> if (rc < 0) {
>> + irq_domain_remove(idxd->ims_domain);
>> iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_AUX);
>> return rc;
>> }
> This really wants a goto error unwind
>
> Jason