linux-kernel - Re: [PATCH] PCI: Introduce flag for detached virtual functions

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] [day] [month] [year] [list]

Date:   Wed, 12 Aug 2020 14:04:01 -0400
From:   Matthew Rosato <mjrosato@...ux.ibm.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     bhelgaas@...gle.com, schnelle@...ux.ibm.com, pmorel@...ux.ibm.com,
        mpe@...erman.id.au, oohall@...il.com, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH] PCI: Introduce flag for detached virtual functions

On 8/12/20 12:43 PM, Alex Williamson wrote:
> On Wed, 12 Aug 2020 10:50:17 -0400
> Matthew Rosato <mjrosato@...ux.ibm.com> wrote:
> 
>> s390x has the notion of providing VFs to the kernel in a manner
>> where the associated PF is inaccessible other than via firmware.
>> These are not treated as typical VFs and access to them is emulated
>> by underlying firmware which can still access the PF.  After
>> abafbc55 however these detached VFs were no longer able to work
>> with vfio-pci as the firmware does not provide emulation of the
>> PCI_COMMAND_MEMORY bit.  In this case, let's explicitly recognize
>> these detached VFs so that vfio-pci can allow memory access to
>> them again.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@...ux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@...ux.ibm.com>
>> Reviewed-by: Niklas Schnelle <schnelle@...ux.ibm.com>
>> ---
>>   arch/s390/pci/pci.c                | 8 ++++++++
>>   drivers/vfio/pci/vfio_pci_config.c | 3 ++-
>>   include/linux/pci.h                | 1 +
>>   3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>> index 3902c9f..04ac76d 100644
>> --- a/arch/s390/pci/pci.c
>> +++ b/arch/s390/pci/pci.c
>> @@ -581,6 +581,14 @@ int pcibios_enable_device(struct pci_dev *pdev, int mask)
>>   {
>>   	struct zpci_dev *zdev = to_zpci(pdev);
>>   
>> +	/*
>> +	 * If we have a VF on a non-multifunction bus, it must be a VF that is
>> +	 * detached from its parent PF.  We rely on firmware emulation to
>> +	 * provide underlying PF details.
>> +	 */
>> +	if (zdev->vfn && !zdev->zbus->multifunction)
>> +		pdev->detached_vf = 1;
>> +
>>   	zpci_debug_init_device(zdev, dev_name(&pdev->dev));
>>   	zpci_fmb_enable_device(zdev);
>>   
>> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
>> index d98843f..17845fc 100644
>> --- a/drivers/vfio/pci/vfio_pci_config.c
>> +++ b/drivers/vfio/pci/vfio_pci_config.c
>> @@ -406,7 +406,8 @@ bool __vfio_pci_memory_enabled(struct vfio_pci_device *vdev)
>>   	 * PF SR-IOV capability, there's therefore no need to trigger
>>   	 * faults based on the virtual value.
>>   	 */
>> -	return pdev->is_virtfn || (cmd & PCI_COMMAND_MEMORY);
>> +	return pdev->is_virtfn || pdev->detached_vf ||
>> +	       (cmd & PCI_COMMAND_MEMORY);
>>   }
>>   
>>   /*
> 
> Wouldn't we also want to enable the is_virtfn related code in
> vfio_basic_config_read() and at least the initial setting of the
> command register in vfio_config_init()?  Otherwise we're extending the
> incomplete emulation out to userspace.  Thanks,
> 

We had discussed doing this internally and I ultimately left it out of 
this patch to keep the changes small...  But I agree that it makes sense 
to fix the emulation for userspace.  I can add the changes you suggest + 
would also need to change vfio_restore_bar() with:

-       if (pdev->is_virtfn)
+       if (pdev->is_virtfn || pdev->detached_vf)
                 return;

to prevent the config changes from tripping the restore code.

I'll send a V2 with this included.