linux-kernel - Re: [PATCH v2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2a862199-16c8-2141-d27f-79761c1b1b25@linux.ibm.com>
Date:   Thu, 13 Aug 2020 11:00:24 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     "Oliver O'Halloran" <oohall@...il.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>
Cc:     Alex Williamson <alex.williamson@...hat.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, pmorel@...ux.ibm.com,
        Michael Ellerman <mpe@...erman.id.au>,
        linux-s390@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        linux-pci <linux-pci@...r.kernel.org>,
        Alexey Kardashevskiy <aik@...abs.ru>
Subject: Re: [PATCH v2] PCI: Introduce flag for detached virtual functions



On 8/13/20 3:55 AM, Oliver O'Halloran wrote:
> On Thu, Aug 13, 2020 at 5:21 AM 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.
> 
> Hmm, cool. I think we have a similar feature on pseries so that's
> probably broken too.
> 
>> Signed-off-by: Matthew Rosato <mjrosato@...ux.ibm.com>
>> ---
>>  arch/s390/pci/pci.c                |  8 ++++++++
>>  drivers/vfio/pci/vfio_pci_config.c | 11 +++++++----
>>  include/linux/pci.h                |  1 +
>>  3 files changed, 16 insertions(+), 4 deletions(-)
>>
>> 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;
> 
> The enable hook seems like it's a bit too late for this sort of
> screwing around with the pci_dev. Anything in the setup path that
> looks at ->detached_vf would see it cleared while anything that looks
> after the device is enabled will see it set. Can this go into
> pcibios_add_device() or a fixup instead?
> 

This particular check could go into pcibios_add_device() yes.
We're also currently working on a slight rework of how
we establish the VF to parent PF linking including the sysfs
part of that. The latter sadly can only go after the sysfs
for the virtfn has been created and that only happens
after all fixups. We would like to do both together because
the latter sets pdev->is_virtfn which I think is closely related.

I was thinking of starting another discussion
about adding a hook that is executed just after the sysfs entries
for the PCI device are created but haven't yet.
That said pcibios_enable_device() is called before drivers
like vfio-pci are enabled and so as long as all uses of pdev->detached_vf
are in drivers it should be early enough. AFAIK almost everything
dealing with VFs before that is already skipped with pdev->no_vf_scan
though.