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

[Zhenzhong Duan <zhenzhong.duan@oracle.com>]

On 2013-07-25 20:25, Konrad Rzeszutek Wilk wrote:
> 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?
Non business of guest kernel, this patch is fixing driver load issue in 
dom0.
Driver qlcnic called pci_reset_function during init. The call path:
pci_reset_function->pci_restore_state->__pci_restore_msix_state->arch_restore_msi_irqs->
xen_initdom_restore_msi_irqs->PHYSDEVOP_restore_msi hypercall

First mask restore is in 
xen_initdom_restore_msi_irqs->PHYSDEVOP_restore_msi hypercall
Second restore is __pci_restore_msix_state->msix_mask_irq(entry, 
entry->masked)

Mask bits are under full control of xen, and the entry->masked in dom0 
kernel is invalid.
We restore an invalid value to mask register could mask the msix interrupt.
>
> 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?
The first restore in hypercall is needed, and second restore should be 
removed for dom0
But baremetal need that restore, so I move it in 
default_restore_msi_irqs which is the func for baremetal.
>
>>>> 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@qlogic.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>>>> ---
>>>>   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?
Yes