diff mbox

[1/4] s390x/pci: fixup trap_msix()

Message ID 1503907487-2764-2-git-send-email-zyimin@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yi Min Zhao Aug. 28, 2017, 8:04 a.m. UTC
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 <bjsdjshi@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Cornelia Huck Aug. 28, 2017, 2:51 p.m. UTC | #1
On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  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?
Yi Min Zhao Aug. 29, 2017, 4:32 a.m. UTC | #2
在 2017/8/28 下午10:51, Cornelia Huck 写道:
> On Mon, 28 Aug 2017 10:04:44 +0200
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   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.
Cornelia Huck Aug. 29, 2017, 8 a.m. UTC | #3
On Tue, 29 Aug 2017 12:32:17 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/8/28 下午10:51, Cornelia Huck 写道:
> > On Mon, 28 Aug 2017 10:04:44 +0200
> > Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
> >> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> ---
> >>   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?
Yi Min Zhao Aug. 29, 2017, 8:05 a.m. UTC | #4
在 2017/8/29 下午4:00, Cornelia Huck 写道:
> On Tue, 29 Aug 2017 12:32:17 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> 在 2017/8/28 下午10:51, Cornelia Huck 写道:
>>> On Mon, 28 Aug 2017 10:04:44 +0200
>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
>>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>    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?
>
>
Yes, didn't get any exception. The guest just kept waiting for something
(I guess that might be the response for interrupt register) and then the
system had no response. What I can do is only destroy the guest.
Yi Min Zhao Aug. 29, 2017, 8:12 a.m. UTC | #5
在 2017/8/29 下午4:00, Cornelia Huck 写道:
> On Tue, 29 Aug 2017 12:32:17 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> 在 2017/8/28 下午10:51, Cornelia Huck 写道:
>>> On Mon, 28 Aug 2017 10:04:44 +0200
>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
>>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>    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.
Cornelia Huck Aug. 29, 2017, 8:22 a.m. UTC | #6
On Tue, 29 Aug 2017 16:12:26 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/8/29 下午4:00, Cornelia Huck 写道:
> > On Tue, 29 Aug 2017 12:32:17 +0800
> > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> >  
> >> 在 2017/8/28 下午10:51, Cornelia Huck 写道:  
> >>> On Mon, 28 Aug 2017 10:04:44 +0200
> >>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
> >>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>>> ---
> >>>>    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).
Yi Min Zhao Aug. 29, 2017, 8:33 a.m. UTC | #7
在 2017/8/29 下午4:22, Cornelia Huck 写道:
> On Tue, 29 Aug 2017 16:12:26 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> 在 2017/8/29 下午4:00, Cornelia Huck 写道:
>>> On Tue, 29 Aug 2017 12:32:17 +0800
>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>>>   
>>>> 在 2017/8/28 下午10:51, Cornelia Huck 写道:
>>>>> On Mon, 28 Aug 2017 10:04:44 +0200
>>>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
>>>>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>     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)?
Because any passthroughed pci devices which I tested has more than 2 
msix entries. And not all
entries will be used. I find that the last entry is never touched. But 
virtio pci is much fancy and only
uses two entries. So I encountered the issue when I tested virtio-pci 
device.
>
> 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).
>
>
Thanks!
Cornelia Huck Aug. 29, 2017, 8:58 a.m. UTC | #8
On Tue, 29 Aug 2017 16:33:52 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/8/29 下午4:22, Cornelia Huck 写道:
> > On Tue, 29 Aug 2017 16:12:26 +0800
> > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> >  
> >> 在 2017/8/29 下午4:00, Cornelia Huck 写道:  
> >>> On Tue, 29 Aug 2017 12:32:17 +0800
> >>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> 在 2017/8/28 下午10:51, Cornelia Huck 写道:  
> >>>>> On Mon, 28 Aug 2017 10:04:44 +0200
> >>>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
> >>>>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >>>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>>     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)?  
> Because any passthroughed pci devices which I tested has more than 2 
> msix entries. And not all
> entries will be used. I find that the last entry is never touched. But 
> virtio pci is much fancy and only
> uses two entries. So I encountered the issue when I tested virtio-pci 
> device.

So that really sounds to me like "we've been lucky"...

> >
> > 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).

...and I'll add cc:stable, as we don't really have any control from
qemu which kind of devices are handled by vfio.
Cornelia Huck Aug. 30, 2017, 9:47 a.m. UTC | #9
On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao <zyimin@linux.vnet.ibm.com> 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 <bjsdjshi@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  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;

Added cc:stable and applied to s390-next.
diff mbox

Patch

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;