Re: [PATCH v2] PCI/MSI: Re-add checks for skip masking MSI-X on Xen PV

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Sun, Oct 24, 2021 at 2:55 PM Josef Johansson <josef@xxxxxxxxxxx> wrote:

> I ended up with this patch, I also masked pci_set_mask and
> pci_set_unmask, even though patching __pci_restore_msi_state and
> __pci_restore_msi_state solved this problem, I found that it did not
> properly make the system be able to survive flip_done timeout related
> problems during suspend/resume. Would this be something you had in mind
> Marc? I will make one more try with just patching
> __pci_restore_msi_state and __pci_restore_msix_state just to make sure.
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index
> 4b4792940e86..0b2225066778 100644 --- a/drivers/pci/msi.c +++
> b/drivers/pci/msi.c @@ -420,7 +420,8 @@ static void
> __pci_restore_msi_state(struct pci_dev *dev) arch_restore_msi_irqs(dev);
> pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control); -
> pci_msi_update_mask(entry, 0, 0); + if (!(pci_msi_ignore_mask ||
> entry->msi_attrib.is_virtual)) + pci_msi_update_mask(entry, 0, 0);
> control &= ~PCI_MSI_FLAGS_QSIZE; control |= (entry->msi_attrib.multiple

This patch was mangled.

> This makes sense the patch would be like so, I'm testing this out now
> hoping it will
>
> perform as good. Now the check is performed in four places

Close.  I'll reply with my compiled, but untested patch of what I was thinking.

> That leaves me with a though, will this set masked, and should be checked as 
> well?
>
> void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> {
>         struct pci_dev *dev = msi_desc_to_pci_dev(entry);
>
>         if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
>                 /* Don't touch the hardware now */
>         } else if (entry->msi_attrib.is_msix) {
>                 void __iomem *base = pci_msix_desc_addr(entry);
>                 u32 ctrl = entry->msix_ctrl;
>                 bool unmasked = !(ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT);
>
>                 if (entry->msi_attrib.is_virtual)
>                         goto skip;
>
>                 /*
>                  * The specification mandates that the entry is masked
>                  * when the message is modified:
>                  *
>                  * "If software changes the Address or Data value of an
>                  * entry while the entry is unmasked, the result is
>                  * undefined."
>                  */
>                 if (unmasked)
> >>>                     pci_msix_write_vector_ctrl(entry, ctrl | 
> >>> PCI_MSIX_ENTRY_CTRL_MASKBIT);

My patch adds a check in pci_msix_write_vector_ctrl(), but the comment
above means PV Xen's behavior may be incorrect if Linux is calling
this function and modifying the message.

Regards,
Jason