[v3] x86/mm: don't needlessly veto migration
diff mbox series

Message ID d1ca6ac5-0ed4-200f-c4e0-7a657b8d8fa8@suse.com
State New
Headers show
Series
  • [v3] x86/mm: don't needlessly veto migration
Related show

Commit Message

Jan Beulich Oct. 8, 2019, 4:10 p.m. UTC
From: Paul Durrant <paul.durrant@citrix.com>

Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
domain, migration may be needlessly vetoed due to the check of
is_iommu_enabled() in paging_log_dirty_enable().
There is actually no need to prevent logdirty from being enabled unless
devices are assigned to a domain.

NOTE: While in the neighbourhood, the bool_t parameter type in
      paging_log_dirty_enable() is replaced with a bool and the format
      of the comment in assign_device() is fixed.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
v3:
 - reduce to iommu_use_hap_pt()-less variant
v2:
 - expand commit comment

Comments

Andrew Cooper Oct. 8, 2019, 4:38 p.m. UTC | #1
On 08/10/2019 17:10, Jan Beulich wrote:
> From: Paul Durrant <paul.durrant@citrix.com>
>
> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
> domain, migration may be needlessly vetoed due to the check of
> is_iommu_enabled() in paging_log_dirty_enable().
> There is actually no need to prevent logdirty from being enabled unless
> devices are assigned to a domain.
>
> NOTE: While in the neighbourhood, the bool_t parameter type in
>       paging_log_dirty_enable() is replaced with a bool and the format
>       of the comment in assign_device() is fixed.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>

Seriously FFS.  Why am I having to repeat myself?  What if any way
unclear on the previous threads?

NACK NACK NACK.  Xen is, and has always been, the wrong place to have
any logic, because IT DOESN'T HAVE ENOUGH INFORMATION TO MAKE THE
DECISION CORRECTLY.

The toolstack does.

Therefore, the toolstack is the only level capable decide whether it is
safe to migration/suspend/resume/checkpoint the VM.

If I have to write the patches myself, I will, but this patch in this
form is frankly unacceptable.

~Andrew
Jan Beulich Oct. 9, 2019, 8:49 a.m. UTC | #2
On 08.10.2019 18:38, Andrew Cooper wrote:
> On 08/10/2019 17:10, Jan Beulich wrote:
>> From: Paul Durrant <paul.durrant@citrix.com>
>>
>> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
>> domain, migration may be needlessly vetoed due to the check of
>> is_iommu_enabled() in paging_log_dirty_enable().
>> There is actually no need to prevent logdirty from being enabled unless
>> devices are assigned to a domain.
>>
>> NOTE: While in the neighbourhood, the bool_t parameter type in
>>       paging_log_dirty_enable() is replaced with a bool and the format
>>       of the comment in assign_device() is fixed.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Release-acked-by: Juergen Gross <jgross@suse.com>
> 
> Seriously FFS.  Why am I having to repeat myself?  What if any way
> unclear on the previous threads?
> 
> NACK NACK NACK.  Xen is, and has always been, the wrong place to have
> any logic, because IT DOESN'T HAVE ENOUGH INFORMATION TO MAKE THE
> DECISION CORRECTLY.
> 
> The toolstack does.
> 
> Therefore, the toolstack is the only level capable decide whether it is
> safe to migration/suspend/resume/checkpoint the VM.
> 
> If I have to write the patches myself, I will, but this patch in this
> form is frankly unacceptable.

You're kidding, aren't you? By taking only part of Paul's original
patch, we should be able to get rid of two of the current osstest
reported regressions. At the same time this _does not_ exclude an
incremental subsequent patch to also add the other half (see my
reply to him yesterday suggesting this split). The two steps
shouldn't have been merged into a single patch anyway imo: The
part here fixes a regression, while the other part changes original
behavior, and continues to be (irrespective of your wording, which
once again suggests that in certain cases you aren't willing to
tolerate thinking that's different from yours) controversial.

If it helps, I can change the title (and perhaps description) to
make it look less like the original patch, and to put focus on the
regression. I just didn't want to massage it more than absolutely
needed.

Jan
Paul Durrant Oct. 9, 2019, 8:54 a.m. UTC | #3
On Wed, 9 Oct 2019 at 09:49, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.10.2019 18:38, Andrew Cooper wrote:
> > On 08/10/2019 17:10, Jan Beulich wrote:
> >> From: Paul Durrant <paul.durrant@citrix.com>
> >>
> >> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
> >> domain, migration may be needlessly vetoed due to the check of
> >> is_iommu_enabled() in paging_log_dirty_enable().
> >> There is actually no need to prevent logdirty from being enabled unless
> >> devices are assigned to a domain.
> >>
> >> NOTE: While in the neighbourhood, the bool_t parameter type in
> >>       paging_log_dirty_enable() is replaced with a bool and the format
> >>       of the comment in assign_device() is fixed.
> >>
> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> Release-acked-by: Juergen Gross <jgross@suse.com>
> >
> > Seriously FFS.  Why am I having to repeat myself?  What if any way
> > unclear on the previous threads?
> >
> > NACK NACK NACK.  Xen is, and has always been, the wrong place to have
> > any logic, because IT DOESN'T HAVE ENOUGH INFORMATION TO MAKE THE
> > DECISION CORRECTLY.
> >
> > The toolstack does.
> >
> > Therefore, the toolstack is the only level capable decide whether it is
> > safe to migration/suspend/resume/checkpoint the VM.
> >
> > If I have to write the patches myself, I will, but this patch in this
> > form is frankly unacceptable.
>
> You're kidding, aren't you? By taking only part of Paul's original
> patch, we should be able to get rid of two of the current osstest
> reported regressions. At the same time this _does not_ exclude an
> incremental subsequent patch to also add the other half (see my
> reply to him yesterday suggesting this split). The two steps
> shouldn't have been merged into a single patch anyway imo: The
> part here fixes a regression, while the other part changes original
> behavior, and continues to be (irrespective of your wording, which
> once again suggests that in certain cases you aren't willing to
> tolerate thinking that's different from yours) controversial.
>
> If it helps, I can change the title (and perhaps description) to
> make it look less like the original patch, and to put focus on the
> regression. I just didn't want to massage it more than absolutely
> needed.

Agreed. Given where we are w.r.t. regressions and a release schedule,
I think we need to be pragmatic. Realistically I'm not going get a Xen
dev. environment up and running for maybe a week so I can't work on
this myself at the moment. I am happy for Jan to fix the regressions
and then we can move on after 4.13 is out the door.

  Paul

>
> Jan
George Dunlap Oct. 9, 2019, 9:56 a.m. UTC | #4
On 10/8/19 5:10 PM, Jan Beulich wrote:
> From: Paul Durrant <paul.durrant@citrix.com>
> 
> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
> domain, migration may be needlessly vetoed due to the check of
> is_iommu_enabled() in paging_log_dirty_enable().
> There is actually no need to prevent logdirty from being enabled unless
> devices are assigned to a domain.
> 
> NOTE: While in the neighbourhood, the bool_t parameter type in
>       paging_log_dirty_enable() is replaced with a bool and the format
>       of the comment in assign_device() is fixed.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Jan Beulich Oct. 9, 2019, 10:16 a.m. UTC | #5
On 08.10.2019 18:38, Andrew Cooper wrote:
> On 08/10/2019 17:10, Jan Beulich wrote:
>> From: Paul Durrant <paul.durrant@citrix.com>
>>
>> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
>> domain, migration may be needlessly vetoed due to the check of
>> is_iommu_enabled() in paging_log_dirty_enable().
>> There is actually no need to prevent logdirty from being enabled unless
>> devices are assigned to a domain.
>>
>> NOTE: While in the neighbourhood, the bool_t parameter type in
>>       paging_log_dirty_enable() is replaced with a bool and the format
>>       of the comment in assign_device() is fixed.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Release-acked-by: Juergen Gross <jgross@suse.com>
> 
> Seriously FFS.  Why am I having to repeat myself?  What if any way
> unclear on the previous threads?
> 
> NACK NACK NACK.

With George having given his R-b I'm now in an awkward position:
I shouldn't ignore this triple NACK and commit the patch, but
there's also no sensible way forward here which would allow the
regression to be taken care of without committing the patch in
its current shape. Are you willing to take back all three of the
NACKs, allowing us to continue discussion on the controversial
part of Paul's patch while also allowing the non-controversial
part to go in right away?

Jan
George Dunlap Oct. 9, 2019, 10:21 a.m. UTC | #6
On 10/9/19 11:16 AM, Jan Beulich wrote:
> On 08.10.2019 18:38, Andrew Cooper wrote:
>> On 08/10/2019 17:10, Jan Beulich wrote:
>>> From: Paul Durrant <paul.durrant@citrix.com>
>>>
>>> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
>>> domain, migration may be needlessly vetoed due to the check of
>>> is_iommu_enabled() in paging_log_dirty_enable().
>>> There is actually no need to prevent logdirty from being enabled unless
>>> devices are assigned to a domain.
>>>
>>> NOTE: While in the neighbourhood, the bool_t parameter type in
>>>       paging_log_dirty_enable() is replaced with a bool and the format
>>>       of the comment in assign_device() is fixed.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Release-acked-by: Juergen Gross <jgross@suse.com>
>>
>> Seriously FFS.  Why am I having to repeat myself?  What if any way
>> unclear on the previous threads?
>>
>> NACK NACK NACK.
> 
> With George having given his R-b I'm now in an awkward position:
> I shouldn't ignore this triple NACK and commit the patch, but
> there's also no sensible way forward here which would allow the
> regression to be taken care of without committing the patch in
> its current shape. Are you willing to take back all three of the
> NACKs, allowing us to continue discussion on the controversial
> part of Paul's patch while also allowing the non-controversial
> part to go in right away?

Regardless of the merits of the change Andy wants to see, it's not a one
that should be made during a feature freeze.

 -George
Juergen Gross Oct. 9, 2019, 10:23 a.m. UTC | #7
On 09.10.19 12:21, George Dunlap wrote:
> On 10/9/19 11:16 AM, Jan Beulich wrote:
>> On 08.10.2019 18:38, Andrew Cooper wrote:
>>> On 08/10/2019 17:10, Jan Beulich wrote:
>>>> From: Paul Durrant <paul.durrant@citrix.com>
>>>>
>>>> Now that xl.cfg has an option to explicitly enable IOMMU mappings for a
>>>> domain, migration may be needlessly vetoed due to the check of
>>>> is_iommu_enabled() in paging_log_dirty_enable().
>>>> There is actually no need to prevent logdirty from being enabled unless
>>>> devices are assigned to a domain.
>>>>
>>>> NOTE: While in the neighbourhood, the bool_t parameter type in
>>>>        paging_log_dirty_enable() is replaced with a bool and the format
>>>>        of the comment in assign_device() is fixed.
>>>>
>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Release-acked-by: Juergen Gross <jgross@suse.com>
>>>
>>> Seriously FFS.  Why am I having to repeat myself?  What if any way
>>> unclear on the previous threads?
>>>
>>> NACK NACK NACK.
>>
>> With George having given his R-b I'm now in an awkward position:
>> I shouldn't ignore this triple NACK and commit the patch, but
>> there's also no sensible way forward here which would allow the
>> regression to be taken care of without committing the patch in
>> its current shape. Are you willing to take back all three of the
>> NACKs, allowing us to continue discussion on the controversial
>> part of Paul's patch while also allowing the non-controversial
>> part to go in right away?
> 
> Regardless of the merits of the change Andy wants to see, it's not a one
> that should be made during a feature freeze.

Indeed. So either we take this patch or we have to revert the patch(es)
introducing the regression.


Juergen
George Dunlap Oct. 9, 2019, 2:14 p.m. UTC | #8
On 10/9/19 11:23 AM, Jürgen Groß wrote:
>> Regardless of the merits of the change Andy wants to see, it's not a one
>> that should be made during a feature freeze.
> 
> Indeed. So either we take this patch or we have to revert the patch(es)
> introducing the regression.

Actually, just chatting with Ian -- the worse issue ATM, AFAICT, is that
the IOMMU is enabled for a guest which has neither asked for PCI
devices, nor explicitly enabled it; and he's currently working on a fix
for that.  Once that issue is fixed, then osstest should become
unblocked again.

It is, arguably, not ideal to refuse to migrate a VM with IOMMU enabled
but no devices attached; but if it only affected guests who had
specifically requested the IOMMU be enabled, that wouldn't be so
terrible.  (And in fact it has highlighted the other, more important issue.)

In summary, this patch is not strictly needed to get a push to osstest.

That said, the behavior in 4.12 was, as far as I can tell:

1. If a guest had never had a PCI device assigned, Xen will allow
logdirty to be enabled.

2. If a guest has a PCI device assigned, Xen will not allow logdirty to
be enabled (blocking migration).

3. If a guest had a PCI device assigned in the past but does not have
one now, Xen will also not allow logdirty to be enabled (blocking
migration).

At the moment, once Ian fixes the default, the behavior will be:

1 If a guest never had PCI device assigned, and
  a. the IOMMU was not explicitly enabled, Xen will allow logdirty to be
enabled
  b. the IOMMU was explicitly enabled, xen will not allow logdirty

1a, 2, and 3 are the same, and 1b didn't exist in 4.12, so arguably
there's no regression.

This patch changes things so that migration will work in the 1b and 3
cases (if I'm reading it right).  This is arguably better, but also
arguably an unnecessary change post-freeze.

And of course there's the option Andy is proposing, of having Xen allow
logdirty to be enabled in all cases, and having the toolstack keep track
of whether there are devices assigned and refuse migration if so; that's
a technical change which should be avoided post-freeze.

 -George
Jan Beulich Oct. 9, 2019, 2:28 p.m. UTC | #9
On 09.10.2019 16:14, George Dunlap wrote:
> On 10/9/19 11:23 AM, Jürgen Groß wrote:
>>> Regardless of the merits of the change Andy wants to see, it's not a one
>>> that should be made during a feature freeze.
>>
>> Indeed. So either we take this patch or we have to revert the patch(es)
>> introducing the regression.
> 
> Actually, just chatting with Ian -- the worse issue ATM, AFAICT, is that
> the IOMMU is enabled for a guest which has neither asked for PCI
> devices, nor explicitly enabled it; and he's currently working on a fix
> for that.  Once that issue is fixed, then osstest should become
> unblocked again.
> 
> It is, arguably, not ideal to refuse to migrate a VM with IOMMU enabled
> but no devices attached; but if it only affected guests who had
> specifically requested the IOMMU be enabled, that wouldn't be so
> terrible.  (And in fact it has highlighted the other, more important issue.)
> 
> In summary, this patch is not strictly needed to get a push to osstest.
> 
> That said, the behavior in 4.12 was, as far as I can tell:
> 
> 1. If a guest had never had a PCI device assigned, Xen will allow
> logdirty to be enabled.
> 
> 2. If a guest has a PCI device assigned, Xen will not allow logdirty to
> be enabled (blocking migration).
> 
> 3. If a guest had a PCI device assigned in the past but does not have
> one now, Xen will also not allow logdirty to be enabled (blocking
> migration).

No - the connection previously was to whether IOMMU page tables
had been set up; these page tables would have been torn down
upon de-assignment of the last device, allowing migration again.
People actually use this behavior afaik, using a bond of a
passed through SR-IOV NIC and netfront provided device. To
migrate the VM, the SR-IOV NIC is taken away without the domain
losing network access, and a new one might then be assigned
again after migration.

The "IOMMU page tables set up" property was previously identical
to "has devices assigned", i.e. even before we could have used
has_arch_pdevs() instead of is_iommu_enabled().

Jan
George Dunlap Oct. 9, 2019, 2:31 p.m. UTC | #10
On 10/9/19 3:28 PM, Jan Beulich wrote:
> On 09.10.2019 16:14, George Dunlap wrote:
>> On 10/9/19 11:23 AM, Jürgen Groß wrote:
>>>> Regardless of the merits of the change Andy wants to see, it's not a one
>>>> that should be made during a feature freeze.
>>>
>>> Indeed. So either we take this patch or we have to revert the patch(es)
>>> introducing the regression.
>>
>> Actually, just chatting with Ian -- the worse issue ATM, AFAICT, is that
>> the IOMMU is enabled for a guest which has neither asked for PCI
>> devices, nor explicitly enabled it; and he's currently working on a fix
>> for that.  Once that issue is fixed, then osstest should become
>> unblocked again.
>>
>> It is, arguably, not ideal to refuse to migrate a VM with IOMMU enabled
>> but no devices attached; but if it only affected guests who had
>> specifically requested the IOMMU be enabled, that wouldn't be so
>> terrible.  (And in fact it has highlighted the other, more important issue.)
>>
>> In summary, this patch is not strictly needed to get a push to osstest.
>>
>> That said, the behavior in 4.12 was, as far as I can tell:
>>
>> 1. If a guest had never had a PCI device assigned, Xen will allow
>> logdirty to be enabled.
>>
>> 2. If a guest has a PCI device assigned, Xen will not allow logdirty to
>> be enabled (blocking migration).
>>
>> 3. If a guest had a PCI device assigned in the past but does not have
>> one now, Xen will also not allow logdirty to be enabled (blocking
>> migration).
> 
> No - the connection previously was to whether IOMMU page tables
> had been set up; these page tables would have been torn down
> upon de-assignment of the last device, allowing migration again.
> People actually use this behavior afaik, using a bond of a
> passed through SR-IOV NIC and netfront provided device. To
> migrate the VM, the SR-IOV NIC is taken away without the domain
> losing network access, and a new one might then be assigned
> again after migration.
> 
> The "IOMMU page tables set up" property was previously identical
> to "has devices assigned", i.e. even before we could have used
> has_arch_pdevs() instead of is_iommu_enabled().

I didn't realize that.  So yes, this patch fixes a regression over and
above the toolstack fix Ian is working on.

 -George

Patch
diff mbox series

--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -71,7 +71,7 @@  int hap_track_dirty_vram(struct domain *
 
         if ( !paging_mode_log_dirty(d) )
         {
-            rc = paging_log_dirty_enable(d, 0);
+            rc = paging_log_dirty_enable(d, false);
             if ( rc )
                 goto out;
         }
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -209,15 +209,15 @@  static int paging_free_log_dirty_bitmap(
     return rc;
 }
 
-int paging_log_dirty_enable(struct domain *d, bool_t log_global)
+int paging_log_dirty_enable(struct domain *d, bool log_global)
 {
     int ret;
 
-    if ( is_iommu_enabled(d) && log_global )
+    if ( has_arch_pdevs(d) && log_global )
     {
         /*
          * Refuse to turn on global log-dirty mode
-         * if the domain is using the IOMMU.
+         * if the domain is sharing the P2M with the IOMMU.
          */
         return -EINVAL;
     }
@@ -727,7 +727,7 @@  int paging_domctl(struct domain *d, stru
             break;
         /* Else fall through... */
     case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY:
-        return paging_log_dirty_enable(d, 1);
+        return paging_log_dirty_enable(d, true);
 
     case XEN_DOMCTL_SHADOW_OP_OFF:
         if ( (rc = paging_log_dirty_disable(d, resuming)) != 0 )
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -157,7 +157,7 @@  void paging_log_dirty_range(struct domai
                             uint8_t *dirty_bitmap);
 
 /* enable log dirty */
-int paging_log_dirty_enable(struct domain *d, bool_t log_global);
+int paging_log_dirty_enable(struct domain *d, bool log_global);
 
 /* log dirty initialization */
 void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops);