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

2017-08-30 Thread Cornelia Huck 
On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao  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 
> Reviewed-by: Pierre Morel 
> Signed-off-by: Yi Min Zhao 
> ---
>  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.

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

2017-08-29 Thread Cornelia Huck 
On Tue, 29 Aug 2017 16:33:52 +0800
Yi Min Zhao  wrote:

> 在 2017/8/29 下午4:22, Cornelia Huck 写道:
> > On Tue, 29 Aug 2017 16:12:26 +0800
> > Yi Min Zhao  wrote:
> >  
> >> 在 2017/8/29 下午4:00, Cornelia Huck 写道:  
> >>> On Tue, 29 Aug 2017 12:32:17 +0800
> >>> Yi Min Zhao  wrote:
> >>> 
>  在 2017/8/28 下午10:51, Cornelia Huck 写道:  
> > On Mon, 28 Aug 2017 10:04:44 +0200
> > Yi Min Zhao  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 
> >> Reviewed-by: Pierre Morel 
> >> Signed-off-by: Yi Min Zhao 
> >> ---
> >> 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.

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

2017-08-29 Thread Yi Min Zhao 



在 2017/8/29 下午4:22, Cornelia Huck 写道:

On Tue, 29 Aug 2017 16:12:26 +0800
Yi Min Zhao  wrote:


在 2017/8/29 下午4:00, Cornelia Huck 写道:

On Tue, 29 Aug 2017 12:32:17 +0800
Yi Min Zhao  wrote:
  

在 2017/8/28 下午10:51, Cornelia Huck 写道:

On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao  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 
Reviewed-by: Pierre Morel 
Signed-off-by: Yi Min Zhao 
---
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!

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

2017-08-29 Thread Cornelia Huck 
On Tue, 29 Aug 2017 16:12:26 +0800
Yi Min Zhao  wrote:

> 在 2017/8/29 下午4:00, Cornelia Huck 写道:
> > On Tue, 29 Aug 2017 12:32:17 +0800
> > Yi Min Zhao  wrote:
> >  
> >> 在 2017/8/28 下午10:51, Cornelia Huck 写道:  
> >>> On Mon, 28 Aug 2017 10:04:44 +0200
> >>> Yi Min Zhao  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 
>  Reviewed-by: Pierre Morel 
>  Signed-off-by: Yi Min Zhao 
>  ---
> 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).

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

2017-08-29 Thread Yi Min Zhao 



在 2017/8/29 下午4:00, Cornelia Huck 写道:

On Tue, 29 Aug 2017 12:32:17 +0800
Yi Min Zhao  wrote:


在 2017/8/28 下午10:51, Cornelia Huck 写道:

On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao  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 
Reviewed-by: Pierre Morel 
Signed-off-by: Yi Min Zhao 
---
   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.

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

2017-08-29 Thread Yi Min Zhao 



在 2017/8/29 下午4:00, Cornelia Huck 写道:

On Tue, 29 Aug 2017 12:32:17 +0800
Yi Min Zhao  wrote:


在 2017/8/28 下午10:51, Cornelia Huck 写道:

On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao  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 
Reviewed-by: Pierre Morel 
Signed-off-by: Yi Min Zhao 
---
   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.

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

2017-08-29 Thread Cornelia Huck 
On Tue, 29 Aug 2017 12:32:17 +0800
Yi Min Zhao  wrote:

> 在 2017/8/28 下午10:51, Cornelia Huck 写道:
> > On Mon, 28 Aug 2017 10:04:44 +0200
> > Yi Min Zhao  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 
> >> Reviewed-by: Pierre Morel 
> >> Signed-off-by: Yi Min Zhao 
> >> ---
> >>   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?

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

2017-08-28 Thread Yi Min Zhao 



在 2017/8/28 下午10:51, Cornelia Huck 写道:

On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao  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 
Reviewed-by: Pierre Morel 
Signed-off-by: Yi Min Zhao 
---
  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.

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

2017-08-28 Thread Cornelia Huck 
On Mon, 28 Aug 2017 10:04:44 +0200
Yi Min Zhao  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 
> Reviewed-by: Pierre Morel 
> Signed-off-by: Yi Min Zhao 
> ---
>  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?

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

2017-08-28 Thread Yi Min Zhao 
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 
Reviewed-by: Pierre Morel 
Signed-off-by: Yi Min Zhao 
---
 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;
-- 
2.11.0 (Apple Git-81)