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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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
>