| Message ID | 1453772092-24866-1-git-send-email-changjzh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
> 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
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.
>>> 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
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;
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(-)