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

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

[Roger Pau Monne]

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. Also note
that doing the reset of host_maskall there would allow the guest to
reset such field by enabling and disabling MSIX, which is not
intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Chao Gao <chao.gao@intel.com>
Cc: "Spassov, Stanislav" <stanspas@amazon.de>
Cc: Pasi Kärkkäinen <pasik@iki.fi>
---
Chao, Stanislav, can you please check if this patch fixes your
issues?
---
 xen/drivers/passthrough/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 7deef2f12b..b4f1ac2dd9 100644
--- 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;
+    }
 
     pdev->fault.count = 0;
 
-- 
2.23.0


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

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

[Jan Beulich]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Roger Pau Monné]

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.

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

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

Thanks, Roger.

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

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

[Jan Beulich]

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

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

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

Jan

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

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

[Roger Pau Monné]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

On 08.10.2019 13:09, Roger Pau Monné  wrote:
> 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?

This happens when pciback takes control of the device, not before
every individual assignment (which is why ->host_maskall may
remain set in the first place).

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

I'm afraid there isn't.

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

Right, that's the variant coming closest to what hardware state
ought to be at that point. We'd need to double check that the
per-entry mask bits are all set at that point.

Jan

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

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

[Roger Pau Monné]

On Tue, Oct 08, 2019 at 01:28:49PM +0200, Jan Beulich wrote:
> On 08.10.2019 13:09, Roger Pau Monné  wrote:
> > 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.
> 
> Right, that's the variant coming closest to what hardware state
> ought to be at that point. We'd need to double check that the
> per-entry mask bits are all set at that point.

I'm not saying such check is not worth doing, but why do it in this
case but not when also clearing the maskall (in msix_capability_init)
when called from prepare_msix?

Thanks, Roger.

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

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

[Jan Beulich]

On 08.10.2019 15:14, Roger Pau Monné  wrote:
> On Tue, Oct 08, 2019 at 01:28:49PM +0200, Jan Beulich wrote:
>> On 08.10.2019 13:09, Roger Pau Monné  wrote:
>>> 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.
>>
>> Right, that's the variant coming closest to what hardware state
>> ought to be at that point. We'd need to double check that the
>> per-entry mask bits are all set at that point.
> 
> I'm not saying such check is not worth doing, but why do it in this
> case but not when also clearing the maskall (in msix_capability_init)
> when called from prepare_msix?

By "double check" I meant inspect the source, not to add checking logic.

Jan

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

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

[Roger Pau Monné]

On Tue, Oct 08, 2019 at 03:32:25PM +0200, Jan Beulich wrote:
> On 08.10.2019 15:14, Roger Pau Monné  wrote:
> > On Tue, Oct 08, 2019 at 01:28:49PM +0200, Jan Beulich wrote:
> >> On 08.10.2019 13:09, Roger Pau Monné  wrote:
> >>> 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.
> >>
> >> Right, that's the variant coming closest to what hardware state
> >> ought to be at that point. We'd need to double check that the
> >> per-entry mask bits are all set at that point.
> > 
> > I'm not saying such check is not worth doing, but why do it in this
> > case but not when also clearing the maskall (in msix_capability_init)
> > when called from prepare_msix?
> 
> By "double check" I meant inspect the source, not to add checking logic.

Oh, I implied you wanted to iterate over all entries and check that
the mask bit is set for each.

It's my understanding that Xen relies on dom0 having done a device
reset before it being assigned, which masks all entries. I've checked
the pciback code and the reset is performed when the device is
assigned to dom0 (ie: guest shutdown or hot-unplug), and hence when
the device is assigned to a different domain the state of it should be
the after reset one.

I can add a comment in assign_device that Xen expects the device state
to be the after reset one, and hence host_maskall = guest_maskall =
false and all entries should have the mask bit set.

Roger.

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

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

[Jan Beulich]

On 08.10.2019 16:16, Roger Pau Monné  wrote:
> On Tue, Oct 08, 2019 at 03:32:25PM +0200, Jan Beulich wrote:
>> On 08.10.2019 15:14, Roger Pau Monné  wrote:
>>> On Tue, Oct 08, 2019 at 01:28:49PM +0200, Jan Beulich wrote:
>>>> On 08.10.2019 13:09, Roger Pau Monné  wrote:
>>>>> 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.
>>>>
>>>> Right, that's the variant coming closest to what hardware state
>>>> ought to be at that point. We'd need to double check that the
>>>> per-entry mask bits are all set at that point.
>>>
>>> I'm not saying such check is not worth doing, but why do it in this
>>> case but not when also clearing the maskall (in msix_capability_init)
>>> when called from prepare_msix?
>>
>> By "double check" I meant inspect the source, not to add checking logic.
> 
> Oh, I implied you wanted to iterate over all entries and check that
> the mask bit is set for each.
> 
> It's my understanding that Xen relies on dom0 having done a device
> reset before it being assigned, which masks all entries. I've checked
> the pciback code and the reset is performed when the device is
> assigned to dom0 (ie: guest shutdown or hot-unplug), and hence when
> the device is assigned to a different domain the state of it should be
> the after reset one.
> 
> I can add a comment in assign_device that Xen expects the device state
> to be the after reset one, and hence host_maskall = guest_maskall =
> false and all entries should have the mask bit set.

Yes please.

Jan

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

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

[Chao Gao]

On Wed, Oct 02, 2019 at 12:49:35PM +0200, 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. Also note
>that doing the reset of host_maskall there would allow the guest to
>reset such field by enabling and disabling MSIX, which is not
>intended.
>
>Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>---
>Cc: Chao Gao <chao.gao@intel.com>
>Cc: "Spassov, Stanislav" <stanspas@amazon.de>
>Cc: Pasi Kärkkäinen <pasik@iki.fi>
>---
>Chao, Stanislav, can you please check if this patch fixes your
>issues?

I am glad to. For your testing, you can just kill qemu and destroy the
guest. Then maskall bit of a pass-thru device will be set. And in this
case, try to recreate the guest and check whether the maskall bit is
cleared in guest.

The solution is similar to my v1 [1]. One question IMO (IIRC, it is why
I changed to another approach) is: why not do such reset at deivce
deassignment such that dom0 can use a clean device. Otherwise, the
device won't work after being unbound from pciback. But I am not so
sure, I can check it next Tuesday.

[1]: https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg00863.html

Thanks
Chao

>---
> xen/drivers/passthrough/pci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>index 7deef2f12b..b4f1ac2dd9 100644
>--- 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;
>+    }

It is similar to my v1 patch here.
[1]: https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg00863.html

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

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

[Jan Beulich]

On 05.10.2019 01:58, Chao Gao wrote:
> On Wed, Oct 02, 2019 at 12:49:35PM +0200, 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. Also note
>> that doing the reset of host_maskall there would allow the guest to
>> reset such field by enabling and disabling MSIX, which is not
>> intended.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Chao Gao <chao.gao@intel.com>
>> Cc: "Spassov, Stanislav" <stanspas@amazon.de>
>> Cc: Pasi Kärkkäinen <pasik@iki.fi>
>> ---
>> Chao, Stanislav, can you please check if this patch fixes your
>> issues?
> 
> I am glad to. For your testing, you can just kill qemu and destroy the
> guest. Then maskall bit of a pass-thru device will be set. And in this
> case, try to recreate the guest and check whether the maskall bit is
> cleared in guest.
> 
> The solution is similar to my v1 [1]. One question IMO (IIRC, it is why
> I changed to another approach) is: why not do such reset at deivce
> deassignment such that dom0 can use a clean device. Otherwise, the
> device won't work after being unbound from pciback. But I am not so
> sure, I can check it next Tuesday.

I too did think about this, but aiui pciback needs to issue
PHYSDEVOP_release_msix anyway, and Dom0 would then re-setup MSI-X
"from scratch", i.e. we'd clear the flag anyway in
msix_capability_init() due to msix->used_entries being zero at
the first (of possibly several) invocation(s).

Jan

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

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

[Chao Gao]

On Mon, Oct 07, 2019 at 09:38:48AM +0200, Jan Beulich wrote:
>On 05.10.2019 01:58, Chao Gao wrote:
>> On Wed, Oct 02, 2019 at 12:49:35PM +0200, 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. Also note
>>> that doing the reset of host_maskall there would allow the guest to
>>> reset such field by enabling and disabling MSIX, which is not
>>> intended.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Cc: Chao Gao <chao.gao@intel.com>
>>> Cc: "Spassov, Stanislav" <stanspas@amazon.de>
>>> Cc: Pasi Kärkkäinen <pasik@iki.fi>
>>> ---
>>> Chao, Stanislav, can you please check if this patch fixes your
>>> issues?
>> 
>> I am glad to. For your testing, you can just kill qemu and destroy the
>> guest. Then maskall bit of a pass-thru device will be set. And in this
>> case, try to recreate the guest and check whether the maskall bit is
>> cleared in guest.
>> 
>> The solution is similar to my v1 [1]. One question IMO (IIRC, it is why
>> I changed to another approach) is: why not do such reset at deivce
>> deassignment such that dom0 can use a clean device. Otherwise, the
>> device won't work after being unbound from pciback. But I am not so
>> sure, I can check it next Tuesday.
>
>I too did think about this, but aiui pciback needs to issue
>PHYSDEVOP_release_msix anyway, and Dom0 would then re-setup MSI-X
>"from scratch", i.e. we'd clear the flag anyway in
>msix_capability_init() due to msix->used_entries being zero at
>the first (of possibly several) invocation(s).

Yes. I just checked it on my machine and found you are right.

Thanks
Chao

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