Re: [PATCH 1/5] x86/vPCI: tolerate (un)masking a disabled MSI-X entry

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Dec 07, 2020 at 11:36:38AM +0100, Jan Beulich wrote:
> None of the four reasons causing vpci_msix_arch_mask_entry() to get
> called (there's just a single call site) are impossible or illegal prior
> to an entry actually having got set up:
> - the entry may remain masked (in this case, however, a prior masked ->
>   unmasked transition would already not have worked),
> - MSI-X may not be enabled,
> - the global mask bit may be set,
> - the entry may not otherwise have been updated.
> Hence the function asserting that the entry was previously set up was
> simply wrong. Since the caller tracks the masked state (and setting up
> of an entry would only be effected when that software bit is clear),
> it's okay to skip both masking and unmasking requests in this case.

On the original approach I just added this because I convinced myself
that scenario was impossible. I think we could also do:

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 64dd0a929c..509cf3962c 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -357,7 +357,11 @@ static int msix_write(struct vcpu *v, unsigned long addr, 
unsigned int len,
          * so that it picks the new state.
          */
         entry->masked = new_masked;
-        if ( !new_masked && msix->enabled && !msix->masked && entry->updated )
+
+        if ( !msix->enabled )
+            break;
+
+        if ( !new_masked && !msix->masked && entry->updated )
         {
             /*
              * If MSI-X is enabled, the function mask is not active, the entry
@@ -470,6 +474,7 @@ static int init_msix(struct pci_dev *pdev)
     for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
     {
         pdev->vpci->msix->entries[i].masked = true;
+        pdev->vpci->msix->entries[i].updated = true;
         vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
     }

In order to solve the issue.

As pointed out in another patch, regardless of what we end up doing
with the issue at hand we might have to consider setting updated to
true in init_msix in case we want to somehow support enabling an entry
that has it's address and data fields set to 0.

> 
> Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers')
> Reported-by: Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Manuel, can we get confirmation that this fixes your issue?

Thanks, Roger.