Re: [Xen-devel] [PATCH] pci: clear host_maskall field on assign

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.10.2019 12:49, Roger Pau Monne wrote:
> The current implementation of host_maskall makes it sticky across
> assign and deassign calls, which means that once a guest forces Xen to
> set host_maskall the maskall bit is not going to be cleared until a
> call to PHYSDEVOP_prepare_msix is performed. Such call however
> shouldn't be part of the normal flow when doing PCI passthrough, and
> hence the flag needs to be cleared when assigning in order to prevent
> host_maskall being carried over from previous assignations.
> 
> Note that other mask fields, like guest_masked or the entry maskbit
> are already reset when the msix capability is initialized.

I take it you mean a guest action here, as PHYSDEVOP_prepare_msix is
specifically about setting up the actual hardware's one? This happens
quite a bit later though, i.e. ->guest_maskall may need explicitly
setting at the same time as you clear ->host_maskall. Furthermore ...

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1504,7 +1504,10 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>      }
>  
>      if ( pdev->msix )
> +    {
>          msixtbl_init(d);
> +        pdev->msix->host_maskall = false;
> +    }

... doing just this would violate an assumed property: It ought to
be fine to assert at every entry or exit point that the physical
maskall bit of an MSI-X-enabled device is the logical OR of
->host_maskall and ->guest_maskall. I.e. I see the following
options:

1) your variant accompanied by updating of the hardware bit,

2)

        pdev->msix->guest_maskall = pdev->msix->host_maskall;
        pdev->msix->host_maskall = false;

leaving the hardware bit alone, as the above transformation
wouldn't change what it's supposed to be set to,

3)

        pdev->msix->guest_maskall = true;
        pdev->msix->host_maskall = false;

alongside setting the bit in hardware (if not already set),

4)

        pdev->msix->guest_maskall = false;
        pdev->msix->host_maskall = false;

alongside clearing the bit in hardware (if not already clear),
relying on all entries being individually masked (which ought
to be the state after the initial msix_capability_init()).

In all cases the operation would likely better be done by
calling a function to be put in x86/msi.c.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel