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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.12.2020 19:24, Roger Pau Monné wrote:
> 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.

Yes, but I view this as an orthogonal aspect to consider (down
the road).

>> 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>

Thanks.

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

I'll give it some time before committing for him to confirm,
but I guess I'd like to time out by the end of the week.

Jan