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

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 27 Jul 2020 17:40:39 +0200
Pierre Morel <pmorel@linux.ibm.com> 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.

Where is the vendor and device ID virtualization done for these
devices, we can't have a PF with IDs 0000:0000.

> Shouldn't we fix this inside the kernel, to keep older QMEU working?
> 
> 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?

Probably each instance of is_virtfn in vfio-pci should be looked at to
see if it applies to s390.  If we're going to recognize this as a VF,
I'd rather we complete the emulation that the lower level hypervisor
has missed.  If we can enable all the is_virtfn code on s390, then we
should probably cache is_virtfn on the vfio_pci_device object and allow
s390 a place to set it once at probe or enable time.

> 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?

It sure seems preferable to recognize that it is a VF in the kernel
than to require userspace to have arch specific hacks.  Thanks,

Alex