linux-kernel - Re: [RFC PATCH 7/8] vfio/pci: Support dynamic MSI-x

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]

Message-ID: <e2d3f5a6-0a36-f19d-8f19-748197c3308d@intel.com>
Date:   Fri, 17 Mar 2023 15:54:41 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Alex Williamson <alex.williamson@...hat.com>
CC:     <jgg@...dia.com>, <yishaih@...dia.com>,
        <shameerali.kolothum.thodi@...wei.com>, <kevin.tian@...el.com>,
        <tglx@...utronix.de>, <darwi@...utronix.de>, <kvm@...r.kernel.org>,
        <dave.jiang@...el.com>, <jing2.liu@...el.com>,
        <ashok.raj@...el.com>, <fenghua.yu@...el.com>,
        <tom.zanussi@...ux.intel.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 7/8] vfio/pci: Support dynamic MSI-x

Hi Alex,

On 3/17/2023 2:58 PM, Alex Williamson wrote:
> On Wed, 15 Mar 2023 13:59:27 -0700
> Reinette Chatre <reinette.chatre@...el.com> wrote:
> 

...
 
>> +static void vfio_irq_ctx_free(struct vfio_pci_core_device *vdev,
>> +			      unsigned long index)
>> +{
>> +	struct vfio_pci_irq_ctx *ctx;
>> +
>> +	ctx = xa_load(&vdev->ctx, index);
>> +	if (ctx) {
>> +		xa_erase(&vdev->ctx, index);
>> +		kfree(ctx);
>> +	}
>> +}
> 
> The only places calling this have a known valid ctx, so it seems
> redundant that we xa_load it again.  Should ctx be a function arg to
> reduce this to simply xa_erase() + kfree()?

Good point. Will do.

...

>> +	if (!ctx) {
>> +		ret = vfio_irq_ctx_alloc_single(vdev, vector);
>> +		if (ret)
>> +			return ret;
>> +		ctx = vfio_irq_ctx_get(vdev, vector);
> 
> This suggests vfio_irq_ctx_alloc_single() should return ctx.
> 

Thank you. Yes, will do.

>> @@ -464,25 +506,38 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  		goto out_free_name;
>>  	}
>>  
>> -	/*
>> -	 * The MSIx vector table resides in device memory which may be cleared
>> -	 * via backdoor resets. We don't allow direct access to the vector
>> -	 * table so even if a userspace driver attempts to save/restore around
>> -	 * such a reset it would be unsuccessful. To avoid this, restore the
>> -	 * cached value of the message prior to enabling.
>> -	 */
>>  	cmd = vfio_pci_memory_lock_and_enable(vdev);
>>  	if (msix) {
>> -		struct msi_msg msg;
>> -
>> -		get_cached_msi_msg(irq, &msg);
>> -		pci_write_msi_msg(irq, &msg);
>> +		if (irq == -EINVAL) {
>> +			msix_map = pci_msix_alloc_irq_at(pdev, vector, NULL);
> 
> It looks to me like we need to support MSI-X with both NORESIZE
> behavior and dynamic allocation based on pci_msix_can_alloc_dyn().
> It's not entirely clear to me where this is and isn't supported, but
> the existence of the test helper suggests we can't assume support.

As I mentioned in my other response ([1]) I cannot see how pci_msix_can_alloc_dyn()
can return false. Even so, yes, I can rework this series to support both the
original and dynamic MSI-x allocation mechanisms.

>> +			if (msix_map.index < 0) {
>> +				vfio_pci_memory_unlock_and_restore(vdev, cmd);
>> +				ret = msix_map.index;
>> +				goto out_put_eventfd_ctx;
>> +			}
>> +			irq = msix_map.virq;
>> +		} else {
>> +			/*
>> +			 * The MSIx vector table resides in device memory which
>> +			 * may be cleared via backdoor resets. We don't allow
>> +			 * direct access to the vector table so even if a
>> +			 * userspace driver attempts to save/restore around
>> +			 * such a reset it would be unsuccessful. To avoid
>> +			 * this, restore the cached value of the message prior
>> +			 * to enabling.
>> +			 */
>> +			struct msi_msg msg;
>> +
>> +			get_cached_msi_msg(irq, &msg);
>> +			pci_write_msi_msg(irq, &msg);
>> +		}
> 
> I don't follow when this latter branch is ever taken in the new flow.
> It's stated earlier that ctx and irq are coupled, and I believe so is
> trigger.  So if we had a previous ctx and irq (and trigger), we removed
> it and irq is now always -EINVAL here.  Thanks,

>From what I understand MSI-X can be enabled without providing any triggers.
That will result in the ctx and irq existing, but not trigger. When a trigger
is assigned later, it will run the latter branch.

Thank you very much

Reinette


[1] https://lore.kernel.org/lkml/61296e93-6268-05cd-e876-680e07645a16@intel.com/