Re: [RFC PATCH] s390x/pci: vfio-pci breakage with disabled mem enforcement

[Matthew Rosato]

On 7/27/20 11:40 AM, Pierre Morel wrote:
> On 2020-07-23 18:29, Alex Williamson wrote:
> > On Thu, 23 Jul 2020 11:13:55 -0400
> > Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> >
> > > I noticed that after kernel commit abafbc55 'vfio-pci: Invalidate mmaps
> > > and block MMIO access on disabled memory' vfio-pci via qemu on s390x
> > > fails spectacularly, with errors in qemu like:
> > >
> > > qemu-system-s390x: vfio_region_read(0001:00:00.0:region0+0x0, 4) 
> > > failed: Input/output error
> > >
> > >  From read to bar 0 originating out of 
> > > hw/s390x/s390-pci-inst.c:zpci_read_bar().
> > >
> > > So, I'm trying to figure out how to get vfio-pci happy again on 
> > > s390x.  From
> > > a bit of tracing, we seem to be triggering the new trap in
> > > __vfio_pci_memory_enabled().  Sure enough, if I just force this 
> > > function to
> > > return 'true' as a test case, things work again.
> > > The included patch attempts to enforce the setting, which restores 
> > > everything
> > > to working order but also triggers vfio_bar_restore() in the 
> > > process....  So
> > > this isn't the right answer, more of a proof-of-concept.
> > >
> > > @Alex: Any guidance on what needs to happen to make qemu-s390x happy 
> > > with this
> > > recent kernel change?
> >
> > Bummer!  I won't claim to understand s390 PCI, but if we have a VF
> > exposed to the "host" (ie. the first level where vfio-pci is being
> > used), but we can't tell that it's a VF, how do we know whether the
> > memory bit in the command register is unimplemented because it's a VF
> > or unimplemented because the device doesn't support MMIO?  How are the
> > device ID, vendor ID, and BAR registers virtualized to the host?  Could
> > the memory enable bit also be emulated by that virtualization, much
> > like vfio-pci does for userspace?  If the other registers are
> > virtualized, but these command register bits are left unimplemented, do
> > we need code to deduce that we have a VF based on the existence of MMIO
> > BARs, but lack of memory enable bit?  Thanks,
> >
> > Alex
>
> Alex, Matt,
>
> in s390 we have the possibility to assign a virtual function to a 
> logical partition as function 0.
> In this case it can not be treated as a virtual function but must be 
> treated as a physical function.
> This is currently working very well.
> However, these functions do not set PCI_COMMAND_MEMORY as we need.

Thanks for the explanation.

> Shouldn't we fix this inside the kernel, to keep older QMEU working?

Yes, ideally.

> Then would it be OK to add a new bit/boolean inside the 
> pci_dev/vfio_pci_device like, is_detached_vfn, that we could set during 
> enumeration and test inside __vfio_pci_memory_enabled() to return true?
>
> In the enumeration we have the possibility to know if the function is a 
> HW/Firmware virtual function on devfn 0 or if it is created by SRIOV.
>
> It seems an easy fix without side effects.
>
> What do you think?

From my perspective at least, this would resolve the issue and is 
in-line with the sort of suggestion Alex floated the other day of "do
we need code to deduce that we have a VF based on the existence of MMIO
BARs, but lack of memory enable bit?" -- it just sounds like a different 
way of coming to the conclusion.  Effectively we need a way to say: 'for 
the purposes of memory_enable detection, treat this thing like a virtual 
function because it is one, even though it doesn't always act like one'

> > > @Nilkas/@Pierre: I wonder if this might be related to host device 
> > > is_virtfn?
> > > I note that my host device lspci output looks like:
> > >
> > > 0000:00:00.0 Ethernet controller: Mellanox Technologies MT27710 
> > > Family [ConnectX-4 Lx Virtual Function]
> > >
> > > But the device is not marked as is_virtfn..  Otherwise, Alex's fix
> > > from htps://lkml.org/lkml/2020/6/25/628 should cover the case.
>
> I do not think we can fix this problem by forcing the is_virtfn bit.
> AFAIU, our HW virtual function works a lot like a physical function.
>
> > > Matthew Rosato (1):
> > >    s390x/pci: Enforce PCI_COMMAND_MEMORY for vfio-pci
> > >
> > >   hw/s390x/s390-pci-inst.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
>
> Regards,
> Pierre