Re: [Qemu-devel] [PATCH 1/4] s390x/pci: fixup trap_msix()

[Cornelia Huck]

On Tue, 29 Aug 2017 16:12:26 +0800
Yi Min Zhao <address@hidden> wrote:

> 在 2017/8/29 下午4:00, Cornelia Huck 写道:
> > On Tue, 29 Aug 2017 12:32:17 +0800
> > Yi Min Zhao <address@hidden> wrote:
> >  
> >> 在 2017/8/28 下午10:51, Cornelia Huck 写道:  
> >>> On Mon, 28 Aug 2017 10:04:44 +0200
> >>> Yi Min Zhao <address@hidden> wrote:
> >>>     
> >>>> The function trap_msix() is to check if pcistg instruction would access
> >>>> msix table entries. The correct boundary condition should be
> >>>> [table_offset, table_offset+entries*entry_size). But the current
> >>>> condition calculated misses the last entry. So let's fixup it.
> >>>>
> >>>> Acked-by: Dong Jia Shi <address@hidden>
> >>>> Reviewed-by: Pierre Morel <address@hidden>
> >>>> Signed-off-by: Yi Min Zhao <address@hidden>
> >>>> ---
> >>>>    hw/s390x/s390-pci-inst.c | 4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >>>> index b7beb8c36a..eba9ffb5f2 100644
> >>>> --- a/hw/s390x/s390-pci-inst.c
> >>>> +++ b/hw/s390x/s390-pci-inst.c
> >>>> @@ -440,8 +440,8 @@ static int trap_msix(S390PCIBusDevice *pbdev, 
> >>>> uint64_t offset, uint8_t pcias)
> >>>>    {
> >>>>        if (pbdev->msix.available && pbdev->msix.table_bar == pcias &&
> >>>>            offset >= pbdev->msix.table_offset &&
> >>>> -        offset <= pbdev->msix.table_offset +
> >>>> -                  (pbdev->msix.entries - 1) * PCI_MSIX_ENTRY_SIZE) {
> >>>> +        offset < (pbdev->msix.table_offset +
> >>>> +                  pbdev->msix.entries * PCI_MSIX_ENTRY_SIZE)) {
> >>>>            return 1;
> >>>>        } else {
> >>>>            return 0;  
> >>> What happened before due to the miscalculation? Write to wrong memory
> >>> region?
> >>>
> >>>     
> >> We tried to plug virtio-net pci device but failed. After inspected, we
> >> found that the device uses two msix entries but the last one was
> >> missed. Then we cannot register interrupt successfully because we
> >> should call trap_msixi() in order to save some useful and arch
> >> information into msix message. But what about wrong memory region
> >> didn't happen.  
> > So, the guest just was not able to use the second msix entry, but did
> > not get any exception?
> >
> >  
> Forget one thing. The zpci idx is saved in msix message. The second msix 
> entry has not been
> trapped so that no idx has been saved, on the other hand idx 0 is saved. So
> kvm_arch_fixup_msi_route() will update irq route according to the zpci 
> device whose idx is 0.
> So the wrong logic in trap_msix() will result that flic mixes different 
> pci devices' adapter interrupts.

Ouch. So this only ever worked for the small subset of pci devices we
can passthrough (assuming none of them use more than one msix entry)?

I'm tempted to have this cc:ed to stable so we can fixup 2.10 (which is
the first version with usable zpci support).