| Message ID | 20191001151159.861-1-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-for-4.13,v2] x86/mm: don't needlessly veto migration | expand |
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
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
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
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
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
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
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
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
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
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
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
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);
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(-)