xen vtd : set msi guest_masked 0 by default
diff mbox

Message ID 1453772092-24866-1-git-send-email-changjzh@gmail.com
State New, archived
Headers show

Commit Message

Jianzhong,Chang Jan. 26, 2016, 1:34 a.m. UTC
There are some problems when msi guest_masked is set to 1 by default.
When guest os is windows 2008 r2 server,
the device(eg X540-AT2 vf) is not initialized correctly.
Host will always receive message like this :"VF Reset msg received from vf".
Guest has network connectivity issues,
and can not correctly receive/send the packet.
So, guest_masked is set to 0 by default.

Signed-off-by: Jianzhong,Chang <changjzh@gmail.com>
---
 xen/arch/x86/msi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Jan. 26, 2016, 12:56 p.m. UTC | #1
>>> On 26.01.16 at 02:34, <changjzh@gmail.com> wrote:
> There are some problems when msi guest_masked is set to 1 by default.
> When guest os is windows 2008 r2 server,
> the device(eg X540-AT2 vf) is not initialized correctly.
> Host will always receive message like this :"VF Reset msg received from vf".
> Guest has network connectivity issues,
> and can not correctly receive/send the packet.
> So, guest_masked is set to 0 by default.

You describe a problem and half of your change, but there's no
connection between the two: What is actually wrong with current
behavior (matching the hardware's - MSI-X mask bits are set when
coming out of reset).

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -512,7 +512,7 @@ void guest_mask_msi_irq(struct irq_desc *desc, bool_t mask)
>  
>  static unsigned int startup_msi_irq(struct irq_desc *desc)
>  {
> -    if ( unlikely(!msi_set_mask_bit(desc, 0, !!(desc->status & IRQ_GUEST))) )
> +    if ( unlikely(!msi_set_mask_bit(desc, 0, 0) ))
>          WARN();
>      return 0;
>  }

Whether this part can go under "set ... by default" is highly
questionable. Plus, while this affects MSI and MSI-X, ...

> @@ -972,7 +972,7 @@ static int msix_capability_init(struct pci_dev *dev,
>          entry->msi_attrib.entry_nr = msi->entry_nr;
>          entry->msi_attrib.maskbit = 1;
>          entry->msi_attrib.host_masked = 1;
> -        entry->msi_attrib.guest_masked = 1;
> +        entry->msi_attrib.guest_masked = 0;
>          entry->msi_attrib.pos = pos;
>          entry->irq = msi->irq;
>          entry->dev = dev;

... this change affect MSI-X only, and doing some guessing from
what you write above I suspect you only really tested one of the
two cases.

So while the change _may_ be necessary, you'll need to do a
better job at explaining why you what you do.

Jan
Tian, Kevin Jan. 26, 2016, 10:24 p.m. UTC | #2
> From: Jan Beulich
> Sent: Tuesday, January 26, 2016 8:57 PM
> To: Chang Jianzhong
> Cc: andrew.cooper3@citrix.com; keir@xen.org; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] xen vtd : set msi guest_masked 0 by default
> 
> >>> On 26.01.16 at 02:34, <changjzh@gmail.com> wrote:
> > There are some problems when msi guest_masked is set to 1 by default.
> > When guest os is windows 2008 r2 server,
> > the device(eg X540-AT2 vf) is not initialized correctly.
> > Host will always receive message like this :"VF Reset msg received from vf".
> > Guest has network connectivity issues,
> > and can not correctly receive/send the packet.
> > So, guest_masked is set to 0 by default.
> 
> You describe a problem and half of your change, but there's no
> connection between the two: What is actually wrong with current
> behavior (matching the hardware's - MSI-X mask bits are set when
> coming out of reset).
> 

Agree. Jianzhong, could you elaborate how mask bit is related to
the error message above? Also instead of changing the behavior
completely, you need consider there must be a reason for 
guest_masked set to 1 originally. So when you fix one issue please
pay attention to not break other scenarios.

Thanks
Kevin
Jianzhong,Chang March 7, 2016, 8:12 a.m. UTC | #3
2016-01-26 20:56 GMT+08:00 Jan Beulich <JBeulich@suse.com>:

> >>> On 26.01.16 at 02:34, <changjzh@gmail.com> wrote:
> > There are some problems when msi guest_masked is set to 1 by default.
> > When guest os is windows 2008 r2 server,
> > the device(eg X540-AT2 vf) is not initialized correctly.
> > Host will always receive message like this :"VF Reset msg received from
> vf".
> > Guest has network connectivity issues,
> > and can not correctly receive/send the packet.
> > So, guest_masked is set to 0 by default.
>
> You describe a problem and half of your change, but there's no
> connection between the two: What is actually wrong with current
> behavior (matching the hardware's - MSI-X mask bits are set when
> coming out of reset).
>
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -512,7 +512,7 @@ void guest_mask_msi_irq(struct irq_desc *desc,
> bool_t mask)
> >
> >  static unsigned int startup_msi_irq(struct irq_desc *desc)
> >  {
> > -    if ( unlikely(!msi_set_mask_bit(desc, 0, !!(desc->status &
> IRQ_GUEST))) )
> > +    if ( unlikely(!msi_set_mask_bit(desc, 0, 0) ))
> >          WARN();
> >      return 0;
> >  }
>
> Whether this part can go under "set ... by default" is highly
> questionable. Plus, while this affects MSI and MSI-X, ...
>
>  If irq is owned by guest,in function msi_set_mask_bit():
...
bool_t flag = host || guest; //The flag should be true.
...
 writel(flag, entry->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
...
PCI device can not generate interrrupt.
windows guest can not change vector_ctrl_mask, guest os get abnormal status
of nic.

> > @@ -972,7 +972,7 @@ static int msix_capability_init(struct pci_dev *dev,
> >          entry->msi_attrib.entry_nr = msi->entry_nr;
> >          entry->msi_attrib.maskbit = 1;
> >          entry->msi_attrib.host_masked = 1;
> > -        entry->msi_attrib.guest_masked = 1;
> > +        entry->msi_attrib.guest_masked = 0;
> >          entry->msi_attrib.pos = pos;
> >          entry->irq = msi->irq;
> >          entry->dev = dev;
>
> ... this change affect MSI-X only, and doing some guessing from
> what you write above I suspect you only really tested one of the
> two cases.
>
> So while the change _may_ be necessary, you'll need to do a
> better job at explaining why you what you do.
>
Msi guest_masked is set to 0 in the original code, only msi-x guest_masked
is modifed in msix_capability_init() function by patch.

>
> Jan
>
>
This issue appears after commited the variable guest_mask.
Initialization operations of pci device may be changed in windows
guest,or Xen need to change the initial state of vtd pci device.
Jan Beulich March 7, 2016, 10:57 a.m. UTC | #4
>>> On 07.03.16 at 09:12, <changjzh@gmail.com> wrote:
> 2016-01-26 20:56 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
> 
>> >>> On 26.01.16 at 02:34, <changjzh@gmail.com> wrote:
>> > There are some problems when msi guest_masked is set to 1 by default.
>> > When guest os is windows 2008 r2 server,
>> > the device(eg X540-AT2 vf) is not initialized correctly.
>> > Host will always receive message like this :"VF Reset msg received from
>> vf".
>> > Guest has network connectivity issues,
>> > and can not correctly receive/send the packet.
>> > So, guest_masked is set to 0 by default.
>>
>> You describe a problem and half of your change, but there's no
>> connection between the two: What is actually wrong with current
>> behavior (matching the hardware's - MSI-X mask bits are set when
>> coming out of reset).
>>
>> > --- a/xen/arch/x86/msi.c
>> > +++ b/xen/arch/x86/msi.c
>> > @@ -512,7 +512,7 @@ void guest_mask_msi_irq(struct irq_desc *desc,
>> bool_t mask)
>> >
>> >  static unsigned int startup_msi_irq(struct irq_desc *desc)
>> >  {
>> > -    if ( unlikely(!msi_set_mask_bit(desc, 0, !!(desc->status &
>> IRQ_GUEST))) )
>> > +    if ( unlikely(!msi_set_mask_bit(desc, 0, 0) ))
>> >          WARN();
>> >      return 0;
>> >  }
>>
>> Whether this part can go under "set ... by default" is highly
>> questionable. Plus, while this affects MSI and MSI-X, ...
>>
>>  If irq is owned by guest,in function msi_set_mask_bit():
> ...
> bool_t flag = host || guest; //The flag should be true.
> ...
>  writel(flag, entry->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> ...
> PCI device can not generate interrrupt.
> windows guest can not change vector_ctrl_mask, guest os get abnormal status
> of nic.

Here and below - I'm sorry, I do not understand what you're trying
to tell us, or how this is meant to extend beyond the original (too
vague) description of your change. Among unclear things is why
"windows guest can not change vector_ctrl_mask" - you again just
make statements without dealing with any of the why-s.

Please can you try to explain things by matching operations done
by the guest, qemu, and the hypervisor to the effect they have
on the state of the mask bit, and then point out which of those
steps needs changing or doesn't work as intended?

Jan

>> > @@ -972,7 +972,7 @@ static int msix_capability_init(struct pci_dev *dev,
>> >          entry->msi_attrib.entry_nr = msi->entry_nr;
>> >          entry->msi_attrib.maskbit = 1;
>> >          entry->msi_attrib.host_masked = 1;
>> > -        entry->msi_attrib.guest_masked = 1;
>> > +        entry->msi_attrib.guest_masked = 0;
>> >          entry->msi_attrib.pos = pos;
>> >          entry->irq = msi->irq;
>> >          entry->dev = dev;
>>
>> ... this change affect MSI-X only, and doing some guessing from
>> what you write above I suspect you only really tested one of the
>> two cases.
>>
>> So while the change _may_ be necessary, you'll need to do a
>> better job at explaining why you what you do.
>>
> Msi guest_masked is set to 0 in the original code, only msi-x guest_masked
> is modifed in msix_capability_init() function by patch.
> 
>>
>> Jan
>>
>>
> This issue appears after commited the variable guest_mask.
> Initialization operations of pci device may be changed in windows
> guest,or Xen need to change the initial state of vtd pci device.
> -- 
> Jianzhong,Chang

Patch
diff mbox

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 5a481f6..b4f60a3 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -512,7 +512,7 @@  void guest_mask_msi_irq(struct irq_desc *desc, bool_t mask)
 
 static unsigned int startup_msi_irq(struct irq_desc *desc)
 {
-    if ( unlikely(!msi_set_mask_bit(desc, 0, !!(desc->status & IRQ_GUEST))) )
+    if ( unlikely(!msi_set_mask_bit(desc, 0, 0) ))
         WARN();
     return 0;
 }
@@ -972,7 +972,7 @@  static int msix_capability_init(struct pci_dev *dev,
         entry->msi_attrib.entry_nr = msi->entry_nr;
         entry->msi_attrib.maskbit = 1;
         entry->msi_attrib.host_masked = 1;
-        entry->msi_attrib.guest_masked = 1;
+        entry->msi_attrib.guest_masked = 0;
         entry->msi_attrib.pos = pos;
         entry->irq = msi->irq;
         entry->dev = dev;