[Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Peter Xu]

We are very strict in the past getting MSIs from commit
d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming
that MSI should be configured before hand when fetching. When we have
unrecognized configurations, we panic the system. However looks like
this is too strict to be working on some platform, and issues occured.
Firstly it's found on a ppc case and fixed by David in:

  6d17a01 vfio/pci: Fix regression in MSI routing configuration

However we encountered another case now with windows virtio driver and
reported (and possibly more):

  http://bugs.debian.org/844361

To make every driver/hardware happy, let's loosen the rule and go back
to the original behavior - instead of panic the system, when we try to
fetch MSI without configured MSI/MSI-X system, we just provide an empty
message to make drivers happy.

Reported-by: Maciej Kotliński <makotlinski@gmail.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/pci/pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 24fae16..0cd2bb0 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2606,9 +2606,11 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
     } else if (msi_enabled(dev)) {
         msg = msi_get_message(dev, vector);
     } else {
-        /* Should never happen */
-        error_report("%s: unknown interrupt type", __func__);
-        abort();
+        /*
+         * Device is not configured with MSI/MSI-X yet, let's provide
+         * an empty message to make all device drivers happy.
+         */
+        memset(&msg, 0, sizeof(msg));
     }
     return msg;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Peter Xu]

On Tue, Nov 22, 2016 at 04:08:50PM +0800, Peter Xu wrote:
> We are very strict in the past getting MSIs from commit
> d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming
> that MSI should be configured before hand when fetching. When we have
> unrecognized configurations, we panic the system. However looks like
> this is too strict to be working on some platform, and issues occured.
> Firstly it's found on a ppc case and fixed by David in:
> 
>   6d17a01 vfio/pci: Fix regression in MSI routing configuration
> 
> However we encountered another case now with windows virtio driver and
> reported (and possibly more):
> 
>   http://bugs.debian.org/844361
> 
> To make every driver/hardware happy, let's loosen the rule and go back
> to the original behavior - instead of panic the system, when we try to
> fetch MSI without configured MSI/MSI-X system, we just provide an empty
> message to make drivers happy.
> 
> Reported-by: Maciej Kotliński <makotlinski@gmail.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Sorry I should mark this as "for-2.8". Also cc stable since this bug
exists since 2.7.0.

Michael, do you think it can be a material for 2.8 rc2?

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Michael S. Tsirkin]

On Tue, Nov 22, 2016 at 04:08:50PM +0800, Peter Xu wrote:
> We are very strict in the past getting MSIs from commit
> d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming
> that MSI should be configured before hand when fetching. When we have
> unrecognized configurations, we panic the system. However looks like
> this is too strict to be working on some platform, and issues occured.
> Firstly it's found on a ppc case and fixed by David in:
> 
>   6d17a01 vfio/pci: Fix regression in MSI routing configuration
> 
> However we encountered another case now with windows virtio driver and
> reported (and possibly more):
> 
>   http://bugs.debian.org/844361
> 
> To make every driver/hardware happy, let's loosen the rule and go back
> to the original behavior - instead of panic the system, when we try to
> fetch MSI without configured MSI/MSI-X system, we just provide an empty
> message to make drivers happy.
> 
> Reported-by: Maciej Kotliński <makotlinski@gmail.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/pci/pci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 24fae16..0cd2bb0 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2606,9 +2606,11 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
>      } else if (msi_enabled(dev)) {
>          msg = msi_get_message(dev, vector);
>      } else {
> -        /* Should never happen */
> -        error_report("%s: unknown interrupt type", __func__);
> -        abort();
> +        /*
> +         * Device is not configured with MSI/MSI-X yet, let's provide
> +         * an empty message to make all device drivers happy.
> +         */
> +        memset(&msg, 0, sizeof(msg));
>      }
>      return msg;
>  }

Looks like a hack to me. Even if callers happen to treat
0 message as a nop, there's no guarantee that's true for
all platforms. Pls change pci_get_msi_message to
return an error code, and fix callers to check that.

> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Peter Xu]

On Fri, Nov 25, 2016 at 06:11:01AM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 22, 2016 at 04:08:50PM +0800, Peter Xu wrote:
> > We are very strict in the past getting MSIs from commit
> > d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming
> > that MSI should be configured before hand when fetching. When we have
> > unrecognized configurations, we panic the system. However looks like
> > this is too strict to be working on some platform, and issues occured.
> > Firstly it's found on a ppc case and fixed by David in:
> > 
> >   6d17a01 vfio/pci: Fix regression in MSI routing configuration
> > 
> > However we encountered another case now with windows virtio driver and
> > reported (and possibly more):
> > 
> >   http://bugs.debian.org/844361
> > 
> > To make every driver/hardware happy, let's loosen the rule and go back
> > to the original behavior - instead of panic the system, when we try to
> > fetch MSI without configured MSI/MSI-X system, we just provide an empty
> > message to make drivers happy.
> > 
> > Reported-by: Maciej Kotliński <makotlinski@gmail.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/pci/pci.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 24fae16..0cd2bb0 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2606,9 +2606,11 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
> >      } else if (msi_enabled(dev)) {
> >          msg = msi_get_message(dev, vector);
> >      } else {
> > -        /* Should never happen */
> > -        error_report("%s: unknown interrupt type", __func__);
> > -        abort();
> > +        /*
> > +         * Device is not configured with MSI/MSI-X yet, let's provide
> > +         * an empty message to make all device drivers happy.
> > +         */
> > +        memset(&msg, 0, sizeof(msg));
> >      }
> >      return msg;
> >  }
> 
> Looks like a hack to me. Even if callers happen to treat
> 0 message as a nop, there's no guarantee that's true for
> all platforms. Pls change pci_get_msi_message to
> return an error code, and fix callers to check that.

Actually I'd say I still cannot understand why some guests will
encounter this issue - the driver just tries to fetch and enable
MSI/MSI-X messages without fully activate MSI/MSI-X in general (that's
why msi_enabled() and msix_enabled() both returned false).

But indeed some drivers won't work properly after commit d1f6af6a.
David fixed some case for ppc and vfio (6d17a01), but problem still
exist, like Debian's report (http://bugs.debian.org/844361).

To avoid going into every details of guest drivers, I was thinking
maybe we can just "recover" the behavior to the past: old QEMU (before
d1f6af6a) allows to fetch an meaningless message (e.g., when
msi_get_message() is called, it's possible that MSI is still not
setup, but IMHO that message is meaningless). So here we emulate that
case, and return a meaningless message. From the Debian bug report, we
can see that this does solve the issue (yeah I know this is not even
an excuse to agree on this patch, but again I just think this is
currently the best way to solve the problem...).

If we change this into return error, then kvm_irqchip_add_msi_route()
might fail, and that's a different behavior again, I don't know
whether it'll bring more troubles (only if I can test every guest
driver with that code, but that's merely impossible). So IMHO the best
way to solve the problem is use the current patch and try to keep the
old behavior.

I would really appreciate if you (or anyone) has better suggestion.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Michael S. Tsirkin]

On Fri, Nov 25, 2016 at 01:28:36PM +0800, Peter Xu wrote:
> On Fri, Nov 25, 2016 at 06:11:01AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 22, 2016 at 04:08:50PM +0800, Peter Xu wrote:
> > > We are very strict in the past getting MSIs from commit
> > > d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming
> > > that MSI should be configured before hand when fetching. When we have
> > > unrecognized configurations, we panic the system. However looks like
> > > this is too strict to be working on some platform, and issues occured.
> > > Firstly it's found on a ppc case and fixed by David in:
> > > 
> > >   6d17a01 vfio/pci: Fix regression in MSI routing configuration
> > > 
> > > However we encountered another case now with windows virtio driver and
> > > reported (and possibly more):
> > > 
> > >   http://bugs.debian.org/844361
> > > 
> > > To make every driver/hardware happy, let's loosen the rule and go back
> > > to the original behavior - instead of panic the system, when we try to
> > > fetch MSI without configured MSI/MSI-X system, we just provide an empty
> > > message to make drivers happy.
> > > 
> > > Reported-by: Maciej Kotliński <makotlinski@gmail.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  hw/pci/pci.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 24fae16..0cd2bb0 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -2606,9 +2606,11 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
> > >      } else if (msi_enabled(dev)) {
> > >          msg = msi_get_message(dev, vector);
> > >      } else {
> > > -        /* Should never happen */
> > > -        error_report("%s: unknown interrupt type", __func__);
> > > -        abort();
> > > +        /*
> > > +         * Device is not configured with MSI/MSI-X yet, let's provide
> > > +         * an empty message to make all device drivers happy.
> > > +         */
> > > +        memset(&msg, 0, sizeof(msg));
> > >      }
> > >      return msg;
> > >  }
> > 
> > Looks like a hack to me. Even if callers happen to treat
> > 0 message as a nop, there's no guarantee that's true for
> > all platforms. Pls change pci_get_msi_message to
> > return an error code, and fix callers to check that.
> 
> Actually I'd say I still cannot understand why some guests will
> encounter this issue - the driver just tries to fetch and enable
> MSI/MSI-X messages without fully activate MSI/MSI-X in general (that's
> why msi_enabled() and msix_enabled() both returned false).
> 
> But indeed some drivers won't work properly after commit d1f6af6a.
> David fixed some case for ppc and vfio (6d17a01), but problem still
> exist, like Debian's report (http://bugs.debian.org/844361).
> 
> To avoid going into every details of guest drivers, I was thinking
> maybe we can just "recover" the behavior to the past: old QEMU (before
> d1f6af6a) allows to fetch an meaningless message (e.g., when
> msi_get_message() is called, it's possible that MSI is still not
> setup, but IMHO that message is meaningless). So here we emulate that
> case, and return a meaningless message. From the Debian bug report, we
> can see that this does solve the issue (yeah I know this is not even
> an excuse to agree on this patch, but again I just think this is
> currently the best way to solve the problem...).
> 
> If we change this into return error, then kvm_irqchip_add_msi_route()
> might fail, and that's a different behavior again, I don't know
> whether it'll bring more troubles (only if I can test every guest
> driver with that code, but that's merely impossible). So IMHO the best
> way to solve the problem is use the current patch and try to keep the
> old behavior.
> 
> I would really appreciate if you (or anyone) has better suggestion.
> 
> Thanks,
> 
> -- peterx

You need to figure out the call stack. What does driver do?
I'm fine with working around it for now but
1. maybe we should fix the driver anyway
2. we might want to emit an error message

-- 
MST

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Peter Xu]

On Fri, Nov 25, 2016 at 08:47:26AM +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 25, 2016 at 01:28:36PM +0800, Peter Xu wrote:
> > On Fri, Nov 25, 2016 at 06:11:01AM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Nov 22, 2016 at 04:08:50PM +0800, Peter Xu wrote:
> > > > We are very strict in the past getting MSIs from commit
> > > > d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming
> > > > that MSI should be configured before hand when fetching. When we have
> > > > unrecognized configurations, we panic the system. However looks like
> > > > this is too strict to be working on some platform, and issues occured.
> > > > Firstly it's found on a ppc case and fixed by David in:
> > > > 
> > > >   6d17a01 vfio/pci: Fix regression in MSI routing configuration
> > > > 
> > > > However we encountered another case now with windows virtio driver and
> > > > reported (and possibly more):
> > > > 
> > > >   http://bugs.debian.org/844361
> > > > 
> > > > To make every driver/hardware happy, let's loosen the rule and go back
> > > > to the original behavior - instead of panic the system, when we try to
> > > > fetch MSI without configured MSI/MSI-X system, we just provide an empty
> > > > message to make drivers happy.
> > > > 
> > > > Reported-by: Maciej Kotliński <makotlinski@gmail.com>
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  hw/pci/pci.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 24fae16..0cd2bb0 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -2606,9 +2606,11 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int vector)
> > > >      } else if (msi_enabled(dev)) {
> > > >          msg = msi_get_message(dev, vector);
> > > >      } else {
> > > > -        /* Should never happen */
> > > > -        error_report("%s: unknown interrupt type", __func__);
> > > > -        abort();
> > > > +        /*
> > > > +         * Device is not configured with MSI/MSI-X yet, let's provide
> > > > +         * an empty message to make all device drivers happy.
> > > > +         */
> > > > +        memset(&msg, 0, sizeof(msg));
> > > >      }
> > > >      return msg;
> > > >  }
> > > 
> > > Looks like a hack to me. Even if callers happen to treat
> > > 0 message as a nop, there's no guarantee that's true for
> > > all platforms. Pls change pci_get_msi_message to
> > > return an error code, and fix callers to check that.
> > 
> > Actually I'd say I still cannot understand why some guests will
> > encounter this issue - the driver just tries to fetch and enable
> > MSI/MSI-X messages without fully activate MSI/MSI-X in general (that's
> > why msi_enabled() and msix_enabled() both returned false).
> > 
> > But indeed some drivers won't work properly after commit d1f6af6a.
> > David fixed some case for ppc and vfio (6d17a01), but problem still
> > exist, like Debian's report (http://bugs.debian.org/844361).
> > 
> > To avoid going into every details of guest drivers, I was thinking
> > maybe we can just "recover" the behavior to the past: old QEMU (before
> > d1f6af6a) allows to fetch an meaningless message (e.g., when
> > msi_get_message() is called, it's possible that MSI is still not
> > setup, but IMHO that message is meaningless). So here we emulate that
> > case, and return a meaningless message. From the Debian bug report, we
> > can see that this does solve the issue (yeah I know this is not even
> > an excuse to agree on this patch, but again I just think this is
> > currently the best way to solve the problem...).
> > 
> > If we change this into return error, then kvm_irqchip_add_msi_route()
> > might fail, and that's a different behavior again, I don't know
> > whether it'll bring more troubles (only if I can test every guest
> > driver with that code, but that's merely impossible). So IMHO the best
> > way to solve the problem is use the current patch and try to keep the
> > old behavior.
> > 
> > I would really appreciate if you (or anyone) has better suggestion.
> > 
> > Thanks,
> > 
> > -- peterx
> 
> You need to figure out the call stack. What does driver do?
> I'm fine with working around it for now but
> 1. maybe we should fix the driver anyway
> 2. we might want to emit an error message
> 
> -- 
> MST

Limin posted another comment here:

  http://bugs.debian.org/844361

We can see whether that bug is finally caused by pci-assign too, and
whether that pci-assign fix can solve the problem.

If that works, it'll be nice, then we won't need this patch at least
for now.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Changlimin]

-device pci-assign,configfd=30,host=05:00.0,id=hostdev0,bus=pci.0,addr=0x3
KVM pci-assign is configured, maybe the same reason as
http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04668.html
please try the patch inside it.

Chang limin


On Fri, Nov 25, 2016 at 06:11:01AM +0200, Michael S. Tsirkin wrote:

> On Tue, Nov 22, 2016 at 04:08:50PM +0800, Peter Xu wrote:

> > We are very strict in the past getting MSIs from commit

> > d1f6af6a1 ("kvm-irqchip: simplify kvm_irqchip_add_msi_route"), assuming

> > that MSI should be configured before hand when fetching. When we have

> > unrecognized configurations, we panic the system. However looks like

> > this is too strict to be working on some platform, and issues occured.

> > Firstly it's found on a ppc case and fixed by David in:

> >

> >   6d17a01 vfio/pci: Fix regression in MSI routing configuration

> >

> > However we encountered another case now with windows virtio driver and

> > reported (and possibly more):

> >

> >   http://bugs.debian.org/844361

> >

> > To make every driver/hardware happy, let's loosen the rule and go back

> > to the original behavior - instead of panic the system, when we try to

> > fetch MSI without configured MSI/MSI-X system, we just provide an empty

> > message to make drivers happy.

> >

> > Reported-by: Maciej Kotliński <address@hidden>

> > Signed-off-by: Peter Xu <address@hidden>

> > ---

> >  hw/pci/pci.c | 8 +++++---

> >  1 file changed, 5 insertions(+), 3 deletions(-)

> >

> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c

> > index 24fae16..0cd2bb0 100644

> > --- a/hw/pci/pci.c

> > +++ b/hw/pci/pci.c

> > @@ -2606,9 +2606,11 @@ MSIMessage pci_get_msi_message(PCIDevice *dev, int

> > vector)

> >      } else if (msi_enabled(dev)) {

> >          msg = msi_get_message(dev, vector);

> >      } else {

> > -        /* Should never happen */

> > -        error_report("%s: unknown interrupt type", __func__);

> > -        abort();

> > +        /*

> > +         * Device is not configured with MSI/MSI-X yet, let's provide

> > +         * an empty message to make all device drivers happy.

> > +         */

> > +        memset(&msg, 0, sizeof(msg));

> >      }

> >      return msg;

> >  }

>

> Looks like a hack to me. Even if callers happen to treat

> 0 message as a nop, there's no guarantee that's true for

> all platforms. Pls change pci_get_msi_message to

> return an error code, and fix callers to check that.



Actually I'd say I still cannot understand why some guests will

encounter this issue - the driver just tries to fetch and enable

MSI/MSI-X messages without fully activate MSI/MSI-X in general (that's

why msi_enabled() and msix_enabled() both returned false).



But indeed some drivers won't work properly after commit d1f6af6a.

David fixed some case for ppc and vfio (6d17a01), but problem still

exist, like Debian's report (http://bugs.debian.org/844361).



To avoid going into every details of guest drivers, I was thinking

maybe we can just "recover" the behavior to the past: old QEMU (before

d1f6af6a) allows to fetch an meaningless message (e.g., when

msi_get_message() is called, it's possible that MSI is still not

setup, but IMHO that message is meaningless). So here we emulate that

case, and return a meaningless message. From the Debian bug report, we

can see that this does solve the issue (yeah I know this is not even

an excuse to agree on this patch, but again I just think this is

currently the best way to solve the problem...).



If we change this into return error, then kvm_irqchip_add_msi_route()

might fail, and that's a different behavior again, I don't know

whether it'll bring more troubles (only if I can test every guest

driver with that code, but that's merely impossible). So IMHO the best

way to solve the problem is use the current patch and try to keep the

old behavior.



I would really appreciate if you (or anyone) has better suggestion.



Thanks,



-- peterx

-------------------------------------------------------------------------------------------------------------------------------------
本邮件及其附件含有杭州华三通信技术有限公司的保密信息，仅限于发送给上面地址中列出
的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
邮件！
This e-mail and its attachments contain confidential information from H3C, which is
intended only for the person or entity whose address is listed above. Any use of the
information contained herein in any way (including, but not limited to, total or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
by phone or email immediately and delete it!

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Changlimin]

update the link

-device pci-assign,configfd=30,host=05:00.0,id=hostdev0,bus=pci.0,addr=0x3
KVM pci-assign is configured, maybe the same reason as
http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04649.html
please try the patch inside it.

Chang limin

-------------------------------------------------------------------------------------------------------------------------------------
本邮件及其附件含有杭州华三通信技术有限公司的保密信息，仅限于发送给上面地址中列出
的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
邮件！
This e-mail and its attachments contain confidential information from H3C, which is
intended only for the person or entity whose address is listed above. Any use of the
information contained herein in any way (including, but not limited to, total or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
by phone or email immediately and delete it!

Re: [Qemu-devel] [PATCH v2] pci: relax pci_msi_get_message()

[Peter Xu]

On Fri, Nov 25, 2016 at 06:03:59AM +0000, Changlimin wrote:
> update the link
> 
> -device pci-assign,configfd=30,host=05:00.0,id=hostdev0,bus=pci.0,addr=0x3
> KVM pci-assign is configured, maybe the same reason as
> http://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg04649.html
> please try the patch inside it.

Limin,

You can post to the following address:

  844361@bugs.debian.org

To leave any comment in that tracker.

Thanks,

-- peterx