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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 May 2021 17:22:22 -0700
From:   Dave Jiang <dave.jiang@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     alex.williamson@...hat.com, kwankhede@...dia.com,
        tglx@...utronix.de, vkoul@...nel.org, megha.dey@...el.com,
        jacob.jun.pan@...el.com, ashok.raj@...el.com, yi.l.liu@...el.com,
        baolu.lu@...el.com, kevin.tian@...el.com, sanjay.k.kumar@...el.com,
        tony.luck@...el.com, dan.j.williams@...el.com,
        eric.auger@...hat.com, pbonzini@...hat.com,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH v6 15/20] vfio/mdev: idxd: ims domain setup for the vdcm

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