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

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

On Tue, Oct 08, 2019 at 11:42:23AM +0200, Jan Beulich wrote:
> On 08.10.2019 11:23, Roger Pau Monné  wrote:
> > On Wed, Oct 02, 2019 at 03:33:43PM +0200, Jan Beulich wrote:
> >> 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?
> > 
> > Right, or any path that calls into msix_capability_init (ie: QEMU
> > requesting to map a PIRQ to an MSIX entry will also call into
> > msix_capability_init).
> > 
> >> 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.
> > 
> > Is this still valid at this point, even without my patch?
> > 
> > The hardware domain should have performed a reset of the device, so
> > the state of the maskall hardware bit should be true, regardless of
> > the previous state of either the guest_maskall or the host_maskall
> > bits.
> 
> But a reset _clears_ the hardware maskall bit (alongside the
> enable one).

Right you are, I was confusing the reset state of the maskall bit and
the per-entry mask bit.

> >> 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),
> > 
> > That seems like the best option IMO, since it's the reset state of the
> > device, and should be the expected one when assigning a device to a
> > guest.
> 
> As per above - no, it's not. We mask interrupts in hardware
> (through individual mask bits iirc) because pci_prepare_msix()
> gets invoked by Dom0 ahead of giving the device to the guest,
> which involves setting the enable bit (and hence unmasked
> interrupts could trigger).

I'm afraid I'm a little bit lost. So if pci_prepare_msix gets called
before assigning the device it means there's a call to
PHYSDEVOP_prepare_msix done by dom0?

I somehow assumed that would only happen when dom0 is actually using
the device, but not as part of a normal flow when the device is just
being assigned to guests without the dom0 using it at all.

Is there some documentation about what dom0 is expected to perform
when assigning and deassigning devices to guests?

Given that as you correctly point out maskall is unset after device
reset, I feel that option 4 is the best one since it matches the state
of the hardware after reset.

> 
> >> 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.
> > 
> > Maybe name it pci_reset_msix_state?
> 
> The common naming pattern in the file seems to be to start with
> msi_ or msix_, so perhaps better msix_reset_state()? I could
> live with your suggested named though.

I don't have a strong opinion either, added the pci_ prefix before
most global functions in the file seem to do so, msix_ prefixed
functions seem to be mostly static.

Thanks, Roger.

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