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 and that domain is sharing HAP mappings
with the IOMMU (in which case disabling write permissions in the P2M may
cause DMA faults). It is quite possible that some assigned devices may
provide information about which pages may have been dirtied by DMA via
an API exported by their managing emulator. Thus Xen's logdirty map is only
one source of information that may be available to the toolstack when
performing a migration and hence it is the toolstack that is best placed
to decide under what circumstances it can be performed, not the hypervisor.

This patch therefore reverts commit 37201c62 "make logdirty and iommu
mutually exclusive" and replaces it with checks to ensure that, if
iommu_use_hap_pt() is true, that logdirty and device assignment are mutually
exclusive.

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>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Juergen Gross <jgross@suse.com>

v2:
 - expand commit comment
---
 xen/arch/x86/mm/hap/hap.c     |  2 +-
 xen/arch/x86/mm/paging.c      |  8 ++++----
 xen/drivers/passthrough/pci.c | 10 +++++++---
 xen/include/asm-x86/paging.h  |  2 +-
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 412a442b6a..3d93f3451c 100644
--- 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 *d,
 
         if ( !paging_mode_log_dirty(d) )
         {
-            rc = paging_log_dirty_enable(d, 0);
+            rc = paging_log_dirty_enable(d, false);
             if ( rc )
                 goto out;
         }
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index d9a52c4db4..240f6f93fb 100644
--- 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(struct domain *d, int rc)
     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) && iommu_use_hap_pt(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, struct xen_domctl_shadow_op *sc,
             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 )
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 7deef2f12b..9614dca8c1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1486,11 +1486,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
     if ( !is_iommu_enabled(d) )
         return 0;
 
-    /* Prevent device assign if mem paging or mem sharing have been 
-     * enabled for this domain */
+    /*
+     * Prevent device assign if mem paging or mem sharing have been
+     * enabled for this domain, or logdirty is enabled and the P2M is
+     * shared with the IOMMU.
+     */
     if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
                   vm_event_check_ring(d->vm_event_paging) ||
-                  p2m_get_hostp2m(d)->global_logdirty) )
+                  (p2m_get_hostp2m(d)->global_logdirty &&
+                   iommu_use_hap_pt(d))) )
         return -EXDEV;
 
     if ( !pcidevs_trylock() )
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index ab7887f23c..8c2027c791 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -157,7 +157,7 @@ void paging_log_dirty_range(struct domain *d,
                             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);
-- 
2.20.1.2.gb21ebb671


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

On 01.10.2019 17:11, Paul Durrant wrote:
> 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 and that domain is sharing HAP mappings
> with the IOMMU (in which case disabling write permissions in the P2M may
> cause DMA faults). It is quite possible that some assigned devices may
> provide information about which pages may have been dirtied by DMA via
> an API exported by their managing emulator. Thus Xen's logdirty map is only
> one source of information that may be available to the toolstack when
> performing a migration and hence it is the toolstack that is best placed
> to decide under what circumstances it can be performed, not the hypervisor.

While I'm happy about the extended description, it's still written in
a way suggesting that this is the only possible way of viewing things.
As expressed by George and me, putting the hypervisor in a position to
be able to judge is at least an alternative worth considering.

What's worse though - you don't go all the way to the end of what your
argumentation would lead to: There's no reason for Xen to veto the
request then even in the shared page table case. The only device
assigned to a guest in question may be doing DMA reads only. Following
your reasoning, Xen shouldn't be getting in the way then either. Once
again the situation could be taken care of by informing Xen about this
property of a device (assuming it can't tell all by itself).

Jan

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

On Wed, 2 Oct 2019 at 09:42, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2019 17:11, Paul Durrant wrote:
> > 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 and that domain is sharing HAP mappings
> > with the IOMMU (in which case disabling write permissions in the P2M may
> > cause DMA faults). It is quite possible that some assigned devices may
> > provide information about which pages may have been dirtied by DMA via
> > an API exported by their managing emulator. Thus Xen's logdirty map is only
> > one source of information that may be available to the toolstack when
> > performing a migration and hence it is the toolstack that is best placed
> > to decide under what circumstances it can be performed, not the hypervisor.
>
> While I'm happy about the extended description, it's still written in
> a way suggesting that this is the only possible way of viewing things.
> As expressed by George and me, putting the hypervisor in a position to
> be able to judge is at least an alternative worth considering.
>

This is a small patch and it does not close the door on being able to
add such an interface later. I'm not saying that I dislike that
alternative, but it will inevitably be quite a lot more code and I'm
not sure it really buys anything.

> What's worse though - you don't go all the way to the end of what your
> argumentation would lead to: There's no reason for Xen to veto the
> request then even in the shared page table case.

Well, I address that in the commit comment.

> The only device
> assigned to a guest in question may be doing DMA reads only. Following
> your reasoning, Xen shouldn't be getting in the way then either. Once
> again the situation could be taken care of by informing Xen about this
> property of a device (assuming it can't tell all by itself).

I am not aware of a mechansim to configure even a PCI express device
to only allow read TLPs and thus we must assume that any device with
bus mastering enabled may attempt to issue a write TLP. Thus I think
it is reasonable for Xen to veto logdirty in the case of shared EPT
because a side effect of Xen's behaviour may have detrimental affect
on device functionality, and cause bus errors to be reported. I guess
it would be reasonable to check all assigned devices' BME bit and only
veto if any are set though. I would prefer that be an incremental
patch though.

  Paul

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

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

On 02.10.2019 10:59, Paul Durrant wrote:
> On Wed, 2 Oct 2019 at 09:42, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 01.10.2019 17:11, Paul Durrant wrote:
>>> 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 and that domain is sharing HAP mappings
>>> with the IOMMU (in which case disabling write permissions in the P2M may
>>> cause DMA faults). It is quite possible that some assigned devices may
>>> provide information about which pages may have been dirtied by DMA via
>>> an API exported by their managing emulator. Thus Xen's logdirty map is only
>>> one source of information that may be available to the toolstack when
>>> performing a migration and hence it is the toolstack that is best placed
>>> to decide under what circumstances it can be performed, not the hypervisor.
>>
>> While I'm happy about the extended description, it's still written in
>> a way suggesting that this is the only possible way of viewing things.
>> As expressed by George and me, putting the hypervisor in a position to
>> be able to judge is at least an alternative worth considering.
>>
> 
> This is a small patch and it does not close the door on being able to
> add such an interface later. I'm not saying that I dislike that
> alternative, but it will inevitably be quite a lot more code and I'm
> not sure it really buys anything.
> 
>> What's worse though - you don't go all the way to the end of what your
>> argumentation would lead to: There's no reason for Xen to veto the
>> request then even in the shared page table case.
> 
> Well, I address that in the commit comment.

Do you? I've just read it again, without finding mention of this case.

>> The only device
>> assigned to a guest in question may be doing DMA reads only. Following
>> your reasoning, Xen shouldn't be getting in the way then either. Once
>> again the situation could be taken care of by informing Xen about this
>> property of a device (assuming it can't tell all by itself).
> 
> I am not aware of a mechansim to configure even a PCI express device
> to only allow read TLPs and thus we must assume that any device with
> bus mastering enabled may attempt to issue a write TLP. Thus I think
> it is reasonable for Xen to veto logdirty in the case of shared EPT
> because a side effect of Xen's behaviour may have detrimental affect
> on device functionality, and cause bus errors to be reported.

I don't follow you here: There's no need to configure a device this
way. If it is claimed (e.g. by an admin or its manufacturer) to only
ever issue DMA reads, then that's all we need. Any erroneous or
malicious write (other than perhaps the ones to trigger MSI) would
potentially result in misbehavior of the guest, but not the host (I
don't see why you mention "bus errors" - it would be IOMMU faults,
with associated aborts reported back to the device). And it's only
the latter you say you're concerned about when it comes to allow Xen
to veto something.

> I guess
> it would be reasonable to check all assigned devices' BME bit and only
> veto if any are set though. I would prefer that be an incremental
> patch though.

Sure - this wouldn't really belong here.

Jan

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

On Wed, 2 Oct 2019 at 10:26, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.10.2019 10:59, Paul Durrant wrote:
> > On Wed, 2 Oct 2019 at 09:42, Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 01.10.2019 17:11, Paul Durrant wrote:
> >>> 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 and that domain is sharing HAP mappings
> >>> with the IOMMU (in which case disabling write permissions in the P2M may
> >>> cause DMA faults). It is quite possible that some assigned devices may
> >>> provide information about which pages may have been dirtied by DMA via
> >>> an API exported by their managing emulator. Thus Xen's logdirty map is only
> >>> one source of information that may be available to the toolstack when
> >>> performing a migration and hence it is the toolstack that is best placed
> >>> to decide under what circumstances it can be performed, not the hypervisor.
> >>
> >> While I'm happy about the extended description, it's still written in
> >> a way suggesting that this is the only possible way of viewing things.
> >> As expressed by George and me, putting the hypervisor in a position to
> >> be able to judge is at least an alternative worth considering.
> >>
> >
> > This is a small patch and it does not close the door on being able to
> > add such an interface later. I'm not saying that I dislike that
> > alternative, but it will inevitably be quite a lot more code and I'm
> > not sure it really buys anything.
> >
> >> What's worse though - you don't go all the way to the end of what your
> >> argumentation would lead to: There's no reason for Xen to veto the
> >> request then even in the shared page table case.
> >
> > Well, I address that in the commit comment.
>
> Do you? I've just read it again, without finding mention of this case.
>
> >> The only device
> >> assigned to a guest in question may be doing DMA reads only. Following
> >> your reasoning, Xen shouldn't be getting in the way then either. Once
> >> again the situation could be taken care of by informing Xen about this
> >> property of a device (assuming it can't tell all by itself).
> >
> > I am not aware of a mechansim to configure even a PCI express device
> > to only allow read TLPs and thus we must assume that any device with
> > bus mastering enabled may attempt to issue a write TLP. Thus I think
> > it is reasonable for Xen to veto logdirty in the case of shared EPT
> > because a side effect of Xen's behaviour may have detrimental affect
> > on device functionality, and cause bus errors to be reported.
>
> I don't follow you here: There's no need to configure a device this
> way. If it is claimed (e.g. by an admin or its manufacturer) to only
> ever issue DMA reads, then that's all we need. Any erroneous or
> malicious write (other than perhaps the ones to trigger MSI) would
> potentially result in misbehavior of the guest, but not the host (I
> don't see why you mention "bus errors" - it would be IOMMU faults,
> with associated aborts reported back to the device). And it's only
> the latter you say you're concerned about when it comes to allow Xen
> to veto something.

ISTR that for at least some systems IOMMU faults are reported as bus
errors in the f/w logs. Such faults (deliberately caused during
development of IOMMU code) have actually led to one of our sysadmins
declaring my test rig to be faulty. This is why I'm keen to avoid such
faults.

>
> > I guess
> > it would be reasonable to check all assigned devices' BME bit and only
> > veto if any are set though. I would prefer that be an incremental
> > patch though.
>
> Sure - this wouldn't really belong here.
>

Ok, good.

  Paul

> Jan

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

On 02/10/2019 09:40, Jan Beulich wrote:
> On 01.10.2019 17:11, Paul Durrant wrote:
>> 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 and that domain is sharing HAP mappings
>> with the IOMMU (in which case disabling write permissions in the P2M may
>> cause DMA faults). It is quite possible that some assigned devices may
>> provide information about which pages may have been dirtied by DMA via
>> an API exported by their managing emulator. Thus Xen's logdirty map is only
>> one source of information that may be available to the toolstack when
>> performing a migration and hence it is the toolstack that is best placed
>> to decide under what circumstances it can be performed, not the hypervisor.
> While I'm happy about the extended description, it's still written in
> a way suggesting that this is the only possible way of viewing things.
> As expressed by George and me, putting the hypervisor in a position to
> be able to judge is at least an alternative worth considering.

No, for exactly the same reason as I'm purging the disable_migrate flag.

This is totally backwards thinking, because the check is in the wrong place.

There really are cases where the toolstack, *and only* the toolstack is
in a position to determine migration safety.  When it comes to
disable_migrate, the area under argument is the ITSC flag, which *is*
safe to offer on migrate for viridian guests which are known to use
reference_tsc, or if the destination hardware supports tsc scaling. 
(Hilariously, nothing, not even the toolstack, prohibits migration based
on Xen's no-migrate flag, because its a write-only field which can't be
retrieved by the tools.)

The two options are:

1) New hypercall,
DOMCTL_the_toolstack_knows_wtf_its_doing_so_let_the_doimain_migrate,
which disables the vetos,

or

2) Delete the erroneous vetos, and trust that the toolstack knows what
it is doing, and will only initiate a migrate in safe situations.

Option 2 has the safety checks perfomed at the level which is actually
capable of calculating the results correctly.

One of these options is substantially less bone-headed than the other.

~Andrew

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

On Wed, 2 Oct 2019 at 10:12, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 02/10/2019 09:40, Jan Beulich wrote:
> > On 01.10.2019 17:11, Paul Durrant wrote:
> >> 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 and that domain is sharing HAP mappings
> >> with the IOMMU (in which case disabling write permissions in the P2M may
> >> cause DMA faults). It is quite possible that some assigned devices may
> >> provide information about which pages may have been dirtied by DMA via
> >> an API exported by their managing emulator. Thus Xen's logdirty map is only
> >> one source of information that may be available to the toolstack when
> >> performing a migration and hence it is the toolstack that is best placed
> >> to decide under what circumstances it can be performed, not the hypervisor.
> > While I'm happy about the extended description, it's still written in
> > a way suggesting that this is the only possible way of viewing things.
> > As expressed by George and me, putting the hypervisor in a position to
> > be able to judge is at least an alternative worth considering.
>
> No, for exactly the same reason as I'm purging the disable_migrate flag.
>
> This is totally backwards thinking, because the check is in the wrong place.
>
> There really are cases where the toolstack, *and only* the toolstack is
> in a position to determine migration safety.  When it comes to
> disable_migrate, the area under argument is the ITSC flag, which *is*
> safe to offer on migrate for viridian guests which are known to use
> reference_tsc, or if the destination hardware supports tsc scaling.
> (Hilariously, nothing, not even the toolstack, prohibits migration based
> on Xen's no-migrate flag, because its a write-only field which can't be
> retrieved by the tools.)
>
> The two options are:
>
> 1) New hypercall,
> DOMCTL_the_toolstack_knows_wtf_its_doing_so_let_the_doimain_migrate,
> which disables the vetos,
>
> or
>
> 2) Delete the erroneous vetos, and trust that the toolstack knows what
> it is doing, and will only initiate a migrate in safe situations.
>
> Option 2 has the safety checks perfomed at the level which is actually
> capable of calculating the results correctly.

That does remind me that I must check that xl will not initiate a
migrate with arbitrary h/w passed through. I know XAPI does
appropriate checking.

  Paul


>
> One of these options is substantially less bone-headed than the other.
>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

On 10/2/19 10:10 AM, Andrew Cooper wrote:
> On 02/10/2019 09:40, Jan Beulich wrote:
>> On 01.10.2019 17:11, Paul Durrant wrote:
>>> 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 and that domain is sharing HAP mappings
>>> with the IOMMU (in which case disabling write permissions in the P2M may
>>> cause DMA faults). It is quite possible that some assigned devices may
>>> provide information about which pages may have been dirtied by DMA via
>>> an API exported by their managing emulator. Thus Xen's logdirty map is only
>>> one source of information that may be available to the toolstack when
>>> performing a migration and hence it is the toolstack that is best placed
>>> to decide under what circumstances it can be performed, not the hypervisor.
>> While I'm happy about the extended description, it's still written in
>> a way suggesting that this is the only possible way of viewing things.
>> As expressed by George and me, putting the hypervisor in a position to
>> be able to judge is at least an alternative worth considering.
> 
> No, for exactly the same reason as I'm purging the disable_migrate flag.
> 
> This is totally backwards thinking, because the check is in the wrong place.
> 
> There really are cases where the toolstack, *and only* the toolstack is
> in a position to determine migration safety.  When it comes to
> disable_migrate, the area under argument is the ITSC flag, which *is*
> safe to offer on migrate for viridian guests which are known to use
> reference_tsc, or if the destination hardware supports tsc scaling. 
> (Hilariously, nothing, not even the toolstack, prohibits migration based
> on Xen's no-migrate flag, because its a write-only field which can't be
> retrieved by the tools.)
> 
> The two options are:
> 
> 1) New hypercall,
> DOMCTL_the_toolstack_knows_wtf_its_doing_so_let_the_doimain_migrate,
> which disables the vetos,
> 
> or
> 
> 2) Delete the erroneous vetos, and trust that the toolstack knows what
> it is doing, and will only initiate a migrate in safe situations.
> 
> Option 2 has the safety checks perfomed at the level which is actually
> capable of calculating the results correctly.
> 
> One of these options is substantially less bone-headed than the other.

Indeed, duplicating the knowledge of the internal details of how
logdirty works in every single copy of the toolstack is a boneheaded
idea. </sarcasm>

Now can we please drop the inflammatory language?

At the moment, this patch (if I understand correctly) will cause a
regression in xl: before this patch, `xl migrate` would correctly fail
to start migration if pci devices were assigned; after this patch, `xl
migrate` will go through with migration, only to potentially have a
garbled domain on the far side.  Is that not correct?

Having a flag to paging_log_dirty_enable() which says, "Ignore device
conflicts, I can get logdirty information from external sources" is a
perfectly sensible interface: it allows more capable toolstacks to
specify their capabilities, while allowing less capable toolstacks not
to need to know the internals of Xen.

And there's yet another option you don't list here which I thought I'd
mentioned: the emulators and/or the toolstack which report logdirty
information to the toolstack can tell Xen, "I am providing logdirty
information for device $BDF."  Then the logdirty check can be, "Are all
devices assigned to the domain providing logdirty information?" And Xen
can fail the migration if there's a device assigned that doesn't have
this flag set.

 -George

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

On 02.10.2019 11:10, Andrew Cooper wrote:
> On 02/10/2019 09:40, Jan Beulich wrote:
>> On 01.10.2019 17:11, Paul Durrant wrote:
>>> 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 and that domain is sharing HAP mappings
>>> with the IOMMU (in which case disabling write permissions in the P2M may
>>> cause DMA faults). It is quite possible that some assigned devices may
>>> provide information about which pages may have been dirtied by DMA via
>>> an API exported by their managing emulator. Thus Xen's logdirty map is only
>>> one source of information that may be available to the toolstack when
>>> performing a migration and hence it is the toolstack that is best placed
>>> to decide under what circumstances it can be performed, not the hypervisor.
>> While I'm happy about the extended description, it's still written in
>> a way suggesting that this is the only possible way of viewing things.
>> As expressed by George and me, putting the hypervisor in a position to
>> be able to judge is at least an alternative worth considering.
> 
> No, for exactly the same reason as I'm purging the disable_migrate flag.
> 
> This is totally backwards thinking, because the check is in the wrong place.
> 
> There really are cases where the toolstack, *and only* the toolstack is
> in a position to determine migration safety.

Then, as already said to Paul, the remaining vetoing in Xen is still too
much. It then should wholesale trust the tool stack. But no, I don't think
that's a viable model - when multiple parties are involved in some
operation, it is quite common that all of them have to say "yes" in order
for it to actually succeed. (It's not like anyone would have suggested
for the tool stack to blindly trust Xen's judgement.)

>  When it comes to
> disable_migrate, the area under argument is the ITSC flag, which *is*
> safe to offer on migrate for viridian guests which are known to use
> reference_tsc, or if the destination hardware supports tsc scaling. 
> (Hilariously, nothing, not even the toolstack, prohibits migration based
> on Xen's no-migrate flag, because its a write-only field which can't be
> retrieved by the tools.)
> 
> The two options are:
> 
> 1) New hypercall,
> DOMCTL_the_toolstack_knows_wtf_its_doing_so_let_the_doimain_migrate,
> which disables the vetos,

Nobody has ever suggested something like this. If (see above) the tool
stack was to be blindly trusted on all matters, then such an operation
wouldn't be needed in the first place.

> or
> 
> 2) Delete the erroneous vetos, and trust that the toolstack knows what
> it is doing, and will only initiate a migrate in safe situations.
> 
> Option 2 has the safety checks perfomed at the level which is actually
> capable of calculating the results correctly.

Then why don't we trust the tool stack to avoid assigning a device to
a mem-paging or mem-sharing enabled guest? Surely the tool stack has
means to know whether one of these is in use? And I'm not convinced
there's a risk to the host if such was done in error; it's merely
known that it wouldn't end well for the involved guest(s).

3) Have the tool stack (assuming we consider emulators part of it)
report the necessary (device) properties such that Xen is in a position
to judge correctly itself.

Jan

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

On Wed, 2 Oct 2019 at 10:42, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.10.2019 11:10, Andrew Cooper wrote:
> > On 02/10/2019 09:40, Jan Beulich wrote:
> >> On 01.10.2019 17:11, Paul Durrant wrote:
> >>> 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 and that domain is sharing HAP mappings
> >>> with the IOMMU (in which case disabling write permissions in the P2M may
> >>> cause DMA faults). It is quite possible that some assigned devices may
> >>> provide information about which pages may have been dirtied by DMA via
> >>> an API exported by their managing emulator. Thus Xen's logdirty map is only
> >>> one source of information that may be available to the toolstack when
> >>> performing a migration and hence it is the toolstack that is best placed
> >>> to decide under what circumstances it can be performed, not the hypervisor.
> >> While I'm happy about the extended description, it's still written in
> >> a way suggesting that this is the only possible way of viewing things.
> >> As expressed by George and me, putting the hypervisor in a position to
> >> be able to judge is at least an alternative worth considering.
> >
> > No, for exactly the same reason as I'm purging the disable_migrate flag.
> >
> > This is totally backwards thinking, because the check is in the wrong place.
> >
> > There really are cases where the toolstack, *and only* the toolstack is
> > in a position to determine migration safety.
>
> Then, as already said to Paul, the remaining vetoing in Xen is still too
> much. It then should wholesale trust the tool stack. But no, I don't think
> that's a viable model - when multiple parties are involved in some
> operation, it is quite common that all of them have to say "yes" in order
> for it to actually succeed. (It's not like anyone would have suggested
> for the tool stack to blindly trust Xen's judgement.)
>
> >  When it comes to
> > disable_migrate, the area under argument is the ITSC flag, which *is*
> > safe to offer on migrate for viridian guests which are known to use
> > reference_tsc, or if the destination hardware supports tsc scaling.
> > (Hilariously, nothing, not even the toolstack, prohibits migration based
> > on Xen's no-migrate flag, because its a write-only field which can't be
> > retrieved by the tools.)
> >
> > The two options are:
> >
> > 1) New hypercall,
> > DOMCTL_the_toolstack_knows_wtf_its_doing_so_let_the_doimain_migrate,
> > which disables the vetos,
>
> Nobody has ever suggested something like this. If (see above) the tool
> stack was to be blindly trusted on all matters, then such an operation
> wouldn't be needed in the first place.
>
> > or
> >
> > 2) Delete the erroneous vetos, and trust that the toolstack knows what
> > it is doing, and will only initiate a migrate in safe situations.
> >
> > Option 2 has the safety checks perfomed at the level which is actually
> > capable of calculating the results correctly.
>
> Then why don't we trust the tool stack to avoid assigning a device to
> a mem-paging or mem-sharing enabled guest? Surely the tool stack has
> means to know whether one of these is in use? And I'm not convinced
> there's a risk to the host if such was done in error; it's merely
> known that it wouldn't end well for the involved guest(s).

I haven't looked but I'd expect sharing or paging out a page to
involve clearing write perms or invalidating a P2M entry, in which
case the main danger with sharing (as with logdirty) comes from
sharing the P2M with the IOMMU and hence taking faults. Far paging
though, I don't know whether there is any logic to clear entries from
the IOMMU mappings when a page is invalidated. If not then that would
pose a significant danger to system integrity.

  Paul

>
> 3) Have the tool stack (assuming we consider emulators part of it)
> report the necessary (device) properties such that Xen is in a position
> to judge correctly itself.
>
> Jan
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

On 01.10.2019 17:11, Paul Durrant wrote:
> --- 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(struct domain *d, int rc)
>      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) && iommu_use_hap_pt(d) && log_global )

To unblock a push to master, the is_iommu_enabled() -> has_arch_pdevs()
transformation here is needed afaict. Since the other half of the
change here (and a corresponding change to assign_device()) continues
to be controversial, could we agree on splitting this patch into two?
(I'd be fine doing the legwork.)

Jan

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

On Tue, 8 Oct 2019 at 09:25, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2019 17:11, Paul Durrant wrote:
> > --- 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(struct domain *d, int rc)
> >      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) && iommu_use_hap_pt(d) && log_global )
>
> To unblock a push to master, the is_iommu_enabled() -> has_arch_pdevs()
> transformation here is needed afaict. Since the other half of the
> change here (and a corresponding change to assign_device()) continues
> to be controversial, could we agree on splitting this patch into two?
> (I'd be fine doing the legwork.)

Yes, please. I don't think there's a realistic chance of me getting
back to this in time for 4.13. Hopefully shortly thereafter I'll be
able to deal with it though.

  Paul

>
> Jan

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