Re: [Xen-devel] [PATCH 2/3] Refactor msi/msix restore code Part2

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Jul 25, 2013 at 02:52:00PM +0800, Zhenzhong Duan wrote:
> 
> On 2013-07-24 21:46, Konrad Rzeszutek Wilk wrote:
> >On Wed, Jul 24, 2013 at 11:08:10AM +0800, Zhenzhong Duan wrote:
> >>xen_initdom_restore_msi_irqs trigger a hypercall to restore addr/data/mask
> >>in dom0. It's better to do the same for default_restore_msi_irqs in 
> >>baremetal.
> >>
> >>Move restore of mask in default_restore_msi_irqs, this could avoid mask
> >>restored twice in dom0, once in hypercall, the other in kernel.
> >Why not remove the hypercall then?
> If removed, msi entry couldn't be restored, such as
> pci_reset_function who will reset pci registers.

I did not read your email first time correctly. You are saying
that we restore it twice in the host kernel (aka dom0), once in the
hypervisor (b/c the guest tries to do MSI-X write and it ends up in
the hypervisor), and then we also do it in the guest kernel?

That is a lot of duplicate calls.
> >Or alter the function to detect
> >whether the restore of the mask has occurred?
> Then we need to add the check for dom0 only.

I am not sure I completly follow this. Is the reason for the lost of
interrupt b/c one of those four MSI-X writes ends up masking and the
subsequent writes end up with invalid data?

> >
> >>Without that, qlcnic driver calling pci_reset_function will lost interrupt
> >>in dom0.

But if you pass said PCI device to a guest there is no need for the
interrupts to go to the host (dom0). They should go to the hypervisor
which will deliever them to the guest.

Is that what you meant by 'in dom0' ?


> >>
> >>Tested-by: Sucheta Chakraborty <sucheta.chakraborty@xxxxxxxxxx>
> >>Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx>
> >>---
> >>  drivers/pci/msi.c |   17 ++++++++++++++---
> >>  1 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> >>index 87223ae..922fb49 100644
> >>--- a/drivers/pci/msi.c
> >>+++ b/drivers/pci/msi.c
> >>@@ -216,6 +216,8 @@ void unmask_msi_irq(struct irq_data *data)
> >>  #ifdef HAVE_DEFAULT_MSI_RESTORE_IRQS
> >>  void default_restore_msi_irqs(struct pci_dev *dev, int irq)
> >>  {
> >>+   int pos;
> >>+   u16 control;
> >>    struct msi_desc *entry;
> >>    entry = NULL;
> >>@@ -228,8 +230,19 @@ void default_restore_msi_irqs(struct pci_dev *dev, int 
> >>irq)
> >>            entry = irq_get_msi_desc(irq);
> >>    }
> >>-   if (entry)
> >>+   if (entry) {
> >>            write_msi_msg(irq, &entry->msg);
> >>+           if (dev->msix_enabled) {
> >>+                   msix_mask_irq(entry, entry->masked);
> >>+                   readl(entry->mask_base);
> >>+           } else {
> >>+                   pos = entry->msi_attrib.pos;
> >>+                   pci_read_config_word(dev, pos + PCI_MSI_FLAGS,
> >>+                                        &control);
> >>+                   msi_mask_irq(entry, msi_capable_mask(control),
> >>+                                entry->masked);
> >>+           }
> >>+   }
> >>  }
> >>  #endif
> >>@@ -406,7 +419,6 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
> >>    arch_restore_msi_irqs(dev, dev->irq);
> >>    pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
> >>-   msi_mask_irq(entry, msi_capable_mask(control), entry->masked);
> >Before this patch we had:
> >
> >     write_msi_msg(..)
> >     pci_read_config_work(PCI_MSI_FLAGS, &control)
> >     pci_write_config_dword(~msi_capable_mask(control) | entry->masked)
> >     control &= ~_PCI_MSI_FLAGS_QSIZE;
> >     control |= ...
> >     pci_write_config_dword(PCI_MSI_FLAGS, control)
> >
> >while with this you have now:
> >
> >     write_msi_msg(..)
> >     pci_read_config_work(PCI_MSI_FLAGS, &_control)
> >     pci_write_config_dword(~msi_capable_mask(_control) | entry->masked)
> >-->  pci_read_config_work(PCI_MSI_FLAGS, &control)
> >     control &= ~_PCI_MSI_FLAGS_QSIZE;
> >     control |= ...
> >     pci_write_config_dword(PCI_MSI_FLAGS, control)
> >
> >see the problem? The 'control' value in __pci_restore_msi_state reads the
> >value _after_ it has been masked (which is now done in 
> >default_restore_msi_irqs).
> >
> >Wouldn't that cause problems?
> >
> pci_write_config_dword(~msi_capable_mask(_control) | entry->masked) restore 
> per vector
> msi mask bits based on the support of PCI_MSI_FLAGS_MASKBIT. This is 
> different from the
> global mask bit in PCI_MSI_FLAGS.

So I think you are saying that it won't cause problems? 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel