Re: [Xen-devel] [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms
On 10/5/18 12:01 PM, Jan Beulich wrote: On 05.10.18 at 10:41, wrote: >> On 10/5/18 11:17 AM, Jan Beulich wrote: >> On 04.10.18 at 18:40, wrote: On 10/4/18 7:04 PM, Jan Beulich wrote: On 02.10.18 at 17:17, wrote: >> static void ept_enable_pml(struct p2m_domain *p2m) >> { >> +struct domain *d = p2m->domain; >> +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >> + >> +p2m_lock(hostp2m); >> + >> /* Domain must have been paused */ >> -ASSERT(atomic_read(>domain->pause_count)); >> +ASSERT(atomic_read(>pause_count)); >> >> /* >> * No need to return whether vmx_domain_enable_pml has succeeded, as >> * ept_p2m_type_to_flags will do the check, and write protection >> will >> be >> * used if PML is not enabled. >> */ >> -if ( vmx_domain_enable_pml(p2m->domain) ) >> +if ( vmx_domain_enable_pml(d) ) >> return; >> >> /* Enable EPT A/D bit for PML */ >> -p2m->ept.ad = 1; >> -vmx_domain_update_eptp(p2m->domain); >> +ept_set_ad_sync(hostp2m, true); >> + >> +vmx_domain_update_eptp(d); >> + >> +p2m_unlock(hostp2m); >> } >> >> static void ept_disable_pml(struct p2m_domain *p2m) >> { >> +struct domain *d = p2m->domain; >> +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >> + >> +p2m_lock(hostp2m); >> + >> /* Domain must have been paused */ >> -ASSERT(atomic_read(>domain->pause_count)); >> +ASSERT(atomic_read(>pause_count)); >> >> -vmx_domain_disable_pml(p2m->domain); >> +vmx_domain_disable_pml(d); >> >> /* Disable EPT A/D bit */ >> -p2m->ept.ad = 0; >> -vmx_domain_update_eptp(p2m->domain); >> +ept_set_ad_sync(hostp2m, false); >> + >> +vmx_domain_update_eptp(d); >> + >> +p2m_unlock(hostp2m); >> } > > While in certain cases I would appreciate such transformations, > I'm afraid the switch from p2m->domain to d in these two > functions is hiding the meat of the change pretty well. In > particular it is only now that I notice that you go from passed in > p2m to domain to hostp2m. This makes me assume some altp2m > could come in here too. Is it really intended for a change to > an altp2m to be propagate to the hostp2m (and all other > altp2m-s)? I can see why altp2m-s want to stay in sync (in > certain regards) with the hostp2m, but for a sync the other > way around there need to be deeper reasons. > > I admit that part of the problem here might be that the whole > function hierarchy you change is tied to log-dirty enabling/ > disabling, but I'm not convinced PML as well as A/D enabled > status has to always match global(?) log-dirty enabled status. > > But I'm not the maintainer of this code, so please don't > interpret my response as a strict request for change. As far as I've understood from George, they do all need to be kept in sync. And I see no reason why an altp2m couldn't be passed in there as well - are you referring to the fact that p2m->domain is not right in that case? I should probably add p2m->domain = hostp2m->domain; in p2m_init_altp2m_ept() in this patch, I think that slipped when I've split the patches. >>> >>> No, I don't think the domain pointer can conflict. Instead I think >>> there could be reasons why one view may want to have A/D >>> and/or PML activated without this being wanted/needed on all >>> others, let alone the host one. But I've meanwhile realized that >>> this may also merely be a function naming issue: >>> ept_{en,dis}able_pml are not overly helpful names for something >>> to be put in directly as {en,dis}able_hardware_log_dirty hooks. >>> By their names, the functions should act on just the specified >>> P2M imo. The hook handlers, otoh, would validly sync the setting >>> to all P2Ms, as log-dirty mode is a domain-wide setting. >> >> Would sending out V4 with ept_{en,dis}able_pml() renamed to >> ept_{en,dis}able_hardware_log_dirty() be a reasonable solution to the >> problem? > > Afaic - I'd prefer the functions to remain as they are, with _new_ > ept_{en,dis}able_hardware_log_dirty() introduced, to be put in > the hook pointers. The new functions would then call the existing > ones for all P2Ms (with the host p2m lock acquired). The question > then is what to do with the calls to the domain-scope > vmx_domain_{en,dis}able_pml(); looking at its implementation I > think it could simply stay where it is. The boolean > vmx_domain_pml_enabled() would need to eventually change to > an enable count, but that's nothing you need to be concerned > about for your purposes. > > But again - please check what maintainers want before going > this route. I see, in which case George please
Re: [Xen-devel] [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms
>>> On 05.10.18 at 10:41, wrote: > On 10/5/18 11:17 AM, Jan Beulich wrote: > On 04.10.18 at 18:40, wrote: >>> On 10/4/18 7:04 PM, Jan Beulich wrote: >>> On 02.10.18 at 17:17, wrote: > static void ept_enable_pml(struct p2m_domain *p2m) > { > +struct domain *d = p2m->domain; > +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > + > +p2m_lock(hostp2m); > + > /* Domain must have been paused */ > -ASSERT(atomic_read(>domain->pause_count)); > +ASSERT(atomic_read(>pause_count)); > > /* > * No need to return whether vmx_domain_enable_pml has succeeded, as > * ept_p2m_type_to_flags will do the check, and write protection > will > be > * used if PML is not enabled. > */ > -if ( vmx_domain_enable_pml(p2m->domain) ) > +if ( vmx_domain_enable_pml(d) ) > return; > > /* Enable EPT A/D bit for PML */ > -p2m->ept.ad = 1; > -vmx_domain_update_eptp(p2m->domain); > +ept_set_ad_sync(hostp2m, true); > + > +vmx_domain_update_eptp(d); > + > +p2m_unlock(hostp2m); > } > > static void ept_disable_pml(struct p2m_domain *p2m) > { > +struct domain *d = p2m->domain; > +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > + > +p2m_lock(hostp2m); > + > /* Domain must have been paused */ > -ASSERT(atomic_read(>domain->pause_count)); > +ASSERT(atomic_read(>pause_count)); > > -vmx_domain_disable_pml(p2m->domain); > +vmx_domain_disable_pml(d); > > /* Disable EPT A/D bit */ > -p2m->ept.ad = 0; > -vmx_domain_update_eptp(p2m->domain); > +ept_set_ad_sync(hostp2m, false); > + > +vmx_domain_update_eptp(d); > + > +p2m_unlock(hostp2m); > } While in certain cases I would appreciate such transformations, I'm afraid the switch from p2m->domain to d in these two functions is hiding the meat of the change pretty well. In particular it is only now that I notice that you go from passed in p2m to domain to hostp2m. This makes me assume some altp2m could come in here too. Is it really intended for a change to an altp2m to be propagate to the hostp2m (and all other altp2m-s)? I can see why altp2m-s want to stay in sync (in certain regards) with the hostp2m, but for a sync the other way around there need to be deeper reasons. I admit that part of the problem here might be that the whole function hierarchy you change is tied to log-dirty enabling/ disabling, but I'm not convinced PML as well as A/D enabled status has to always match global(?) log-dirty enabled status. But I'm not the maintainer of this code, so please don't interpret my response as a strict request for change. >>> >>> As far as I've understood from George, they do all need to be kept in >>> sync. And I see no reason why an altp2m couldn't be passed in there as >>> well - are you referring to the fact that p2m->domain is not right in >>> that case? I should probably add p2m->domain = hostp2m->domain; in >>> p2m_init_altp2m_ept() in this patch, I think that slipped when I've >>> split the patches. >> >> No, I don't think the domain pointer can conflict. Instead I think >> there could be reasons why one view may want to have A/D >> and/or PML activated without this being wanted/needed on all >> others, let alone the host one. But I've meanwhile realized that >> this may also merely be a function naming issue: >> ept_{en,dis}able_pml are not overly helpful names for something >> to be put in directly as {en,dis}able_hardware_log_dirty hooks. >> By their names, the functions should act on just the specified >> P2M imo. The hook handlers, otoh, would validly sync the setting >> to all P2Ms, as log-dirty mode is a domain-wide setting. > > Would sending out V4 with ept_{en,dis}able_pml() renamed to > ept_{en,dis}able_hardware_log_dirty() be a reasonable solution to the > problem? Afaic - I'd prefer the functions to remain as they are, with _new_ ept_{en,dis}able_hardware_log_dirty() introduced, to be put in the hook pointers. The new functions would then call the existing ones for all P2Ms (with the host p2m lock acquired). The question then is what to do with the calls to the domain-scope vmx_domain_{en,dis}able_pml(); looking at its implementation I think it could simply stay where it is. The boolean vmx_domain_pml_enabled() would need to eventually change to an enable count, but that's nothing you need to be concerned about for your purposes. But again - please check what maintainers want before going this route. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms
>>> On 04.10.18 at 18:40, wrote: > On 10/4/18 7:04 PM, Jan Beulich wrote: > On 02.10.18 at 17:17, wrote: >>> static void ept_enable_pml(struct p2m_domain *p2m) >>> { >>> +struct domain *d = p2m->domain; >>> +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >>> + >>> +p2m_lock(hostp2m); >>> + >>> /* Domain must have been paused */ >>> -ASSERT(atomic_read(>domain->pause_count)); >>> +ASSERT(atomic_read(>pause_count)); >>> >>> /* >>> * No need to return whether vmx_domain_enable_pml has succeeded, as >>> * ept_p2m_type_to_flags will do the check, and write protection will >>> be >>> * used if PML is not enabled. >>> */ >>> -if ( vmx_domain_enable_pml(p2m->domain) ) >>> +if ( vmx_domain_enable_pml(d) ) >>> return; >>> >>> /* Enable EPT A/D bit for PML */ >>> -p2m->ept.ad = 1; >>> -vmx_domain_update_eptp(p2m->domain); >>> +ept_set_ad_sync(hostp2m, true); >>> + >>> +vmx_domain_update_eptp(d); >>> + >>> +p2m_unlock(hostp2m); >>> } >>> >>> static void ept_disable_pml(struct p2m_domain *p2m) >>> { >>> +struct domain *d = p2m->domain; >>> +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >>> + >>> +p2m_lock(hostp2m); >>> + >>> /* Domain must have been paused */ >>> -ASSERT(atomic_read(>domain->pause_count)); >>> +ASSERT(atomic_read(>pause_count)); >>> >>> -vmx_domain_disable_pml(p2m->domain); >>> +vmx_domain_disable_pml(d); >>> >>> /* Disable EPT A/D bit */ >>> -p2m->ept.ad = 0; >>> -vmx_domain_update_eptp(p2m->domain); >>> +ept_set_ad_sync(hostp2m, false); >>> + >>> +vmx_domain_update_eptp(d); >>> + >>> +p2m_unlock(hostp2m); >>> } >> >> While in certain cases I would appreciate such transformations, >> I'm afraid the switch from p2m->domain to d in these two >> functions is hiding the meat of the change pretty well. In >> particular it is only now that I notice that you go from passed in >> p2m to domain to hostp2m. This makes me assume some altp2m >> could come in here too. Is it really intended for a change to >> an altp2m to be propagate to the hostp2m (and all other >> altp2m-s)? I can see why altp2m-s want to stay in sync (in >> certain regards) with the hostp2m, but for a sync the other >> way around there need to be deeper reasons. >> >> I admit that part of the problem here might be that the whole >> function hierarchy you change is tied to log-dirty enabling/ >> disabling, but I'm not convinced PML as well as A/D enabled >> status has to always match global(?) log-dirty enabled status. >> >> But I'm not the maintainer of this code, so please don't >> interpret my response as a strict request for change. > > As far as I've understood from George, they do all need to be kept in > sync. And I see no reason why an altp2m couldn't be passed in there as > well - are you referring to the fact that p2m->domain is not right in > that case? I should probably add p2m->domain = hostp2m->domain; in > p2m_init_altp2m_ept() in this patch, I think that slipped when I've > split the patches. No, I don't think the domain pointer can conflict. Instead I think there could be reasons why one view may want to have A/D and/or PML activated without this being wanted/needed on all others, let alone the host one. But I've meanwhile realized that this may also merely be a function naming issue: ept_{en,dis}able_pml are not overly helpful names for something to be put in directly as {en,dis}able_hardware_log_dirty hooks. By their names, the functions should act on just the specified P2M imo. The hook handlers, otoh, would validly sync the setting to all P2Ms, as log-dirty mode is a domain-wide setting. > To be honest, I have thought about changing the function signature and > pass in a domain - however I didn't because this appears to be actually > tied to a platform-independent callback, and it's likely that the p2m > parameter makes more sense for those. (Also, all the other callbacks > take a p2m parameter, which probably serves a similar purpose to C++'s > "this".) Yeah, this model should perhaps be kept as is. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms
On 10/4/18 7:04 PM, Jan Beulich wrote: On 02.10.18 at 17:17, wrote: >> +static void ept_set_ad_sync(struct p2m_domain *hostp2m, bool value) >> +{ >> +struct domain *d = hostp2m->domain; >> + >> +ASSERT(p2m_is_hostp2m(hostp2m)); >> +ASSERT(p2m_locked_by_me(hostp2m)); >> + >> +hostp2m->ept.ad = value; >> + >> +if ( unlikely(altp2m_active(d)) ) >> +{ >> +unsigned int i; >> + >> +for ( i = 0; i < MAX_ALTP2M; i++ ) >> +{ >> +struct p2m_domain *p2m; >> + >> +if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) >> +continue; >> + >> +p2m = d->arch.altp2m_p2m[i]; >> + >> +p2m_lock(p2m); >> +p2m->ept.ad = value; >> +p2m_unlock(p2m); > > Just one further general remark here, coming back to whether [0] > represent the hostp2m: How would acquiring the lock here not > deadlock (the hostp2m is already locked, after all) if that were the > case? As George has pointed out, it's not really the case. The mem_access code treats index 0 as the altp2m, but it does so "manually" (i.e. it tests if the index is 0 and then uses the host p2m). This does waste the altp2m at position 0 in the array. It would be fantastic if we could always and consistenly use the first altp2m in the array as 0 and eliminate these corner cases, but on the other hand it's very understandable (in light of the recent trouble we're having with altp2m) that there's reluctance to fully embrace the altp2m code yet. >> static void ept_enable_pml(struct p2m_domain *p2m) >> { >> +struct domain *d = p2m->domain; >> +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >> + >> +p2m_lock(hostp2m); >> + >> /* Domain must have been paused */ >> -ASSERT(atomic_read(>domain->pause_count)); >> +ASSERT(atomic_read(>pause_count)); >> >> /* >> * No need to return whether vmx_domain_enable_pml has succeeded, as >> * ept_p2m_type_to_flags will do the check, and write protection will be >> * used if PML is not enabled. >> */ >> -if ( vmx_domain_enable_pml(p2m->domain) ) >> +if ( vmx_domain_enable_pml(d) ) >> return; >> >> /* Enable EPT A/D bit for PML */ >> -p2m->ept.ad = 1; >> -vmx_domain_update_eptp(p2m->domain); >> +ept_set_ad_sync(hostp2m, true); >> + >> +vmx_domain_update_eptp(d); >> + >> +p2m_unlock(hostp2m); >> } >> >> static void ept_disable_pml(struct p2m_domain *p2m) >> { >> +struct domain *d = p2m->domain; >> +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >> + >> +p2m_lock(hostp2m); >> + >> /* Domain must have been paused */ >> -ASSERT(atomic_read(>domain->pause_count)); >> +ASSERT(atomic_read(>pause_count)); >> >> -vmx_domain_disable_pml(p2m->domain); >> +vmx_domain_disable_pml(d); >> >> /* Disable EPT A/D bit */ >> -p2m->ept.ad = 0; >> -vmx_domain_update_eptp(p2m->domain); >> +ept_set_ad_sync(hostp2m, false); >> + >> +vmx_domain_update_eptp(d); >> + >> +p2m_unlock(hostp2m); >> } > > While in certain cases I would appreciate such transformations, > I'm afraid the switch from p2m->domain to d in these two > functions is hiding the meat of the change pretty well. In > particular it is only now that I notice that you go from passed in > p2m to domain to hostp2m. This makes me assume some altp2m > could come in here too. Is it really intended for a change to > an altp2m to be propagate to the hostp2m (and all other > altp2m-s)? I can see why altp2m-s want to stay in sync (in > certain regards) with the hostp2m, but for a sync the other > way around there need to be deeper reasons. > > I admit that part of the problem here might be that the whole > function hierarchy you change is tied to log-dirty enabling/ > disabling, but I'm not convinced PML as well as A/D enabled > status has to always match global(?) log-dirty enabled status. > > But I'm not the maintainer of this code, so please don't > interpret my response as a strict request for change. As far as I've understood from George, they do all need to be kept in sync. And I see no reason why an altp2m couldn't be passed in there as well - are you referring to the fact that p2m->domain is not right in that case? I should probably add p2m->domain = hostp2m->domain; in p2m_init_altp2m_ept() in this patch, I think that slipped when I've split the patches. To be honest, I have thought about changing the function signature and pass in a domain - however I didn't because this appears to be actually tied to a platform-independent callback, and it's likely that the p2m parameter makes more sense for those. (Also, all the other callbacks take a p2m parameter, which probably serves a similar purpose to C++'s "this".) Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms
>>> On 02.10.18 at 17:17, wrote: > +static void ept_set_ad_sync(struct p2m_domain *hostp2m, bool value) > +{ > +struct domain *d = hostp2m->domain; > + > +ASSERT(p2m_is_hostp2m(hostp2m)); > +ASSERT(p2m_locked_by_me(hostp2m)); > + > +hostp2m->ept.ad = value; > + > +if ( unlikely(altp2m_active(d)) ) > +{ > +unsigned int i; > + > +for ( i = 0; i < MAX_ALTP2M; i++ ) > +{ > +struct p2m_domain *p2m; > + > +if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) > +continue; > + > +p2m = d->arch.altp2m_p2m[i]; > + > +p2m_lock(p2m); > +p2m->ept.ad = value; > +p2m_unlock(p2m); Just one further general remark here, coming back to whether [0] represent the hostp2m: How would acquiring the lock here not deadlock (the hostp2m is already locked, after all) if that were the case? > static void ept_enable_pml(struct p2m_domain *p2m) > { > +struct domain *d = p2m->domain; > +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > + > +p2m_lock(hostp2m); > + > /* Domain must have been paused */ > -ASSERT(atomic_read(>domain->pause_count)); > +ASSERT(atomic_read(>pause_count)); > > /* > * No need to return whether vmx_domain_enable_pml has succeeded, as > * ept_p2m_type_to_flags will do the check, and write protection will be > * used if PML is not enabled. > */ > -if ( vmx_domain_enable_pml(p2m->domain) ) > +if ( vmx_domain_enable_pml(d) ) > return; > > /* Enable EPT A/D bit for PML */ > -p2m->ept.ad = 1; > -vmx_domain_update_eptp(p2m->domain); > +ept_set_ad_sync(hostp2m, true); > + > +vmx_domain_update_eptp(d); > + > +p2m_unlock(hostp2m); > } > > static void ept_disable_pml(struct p2m_domain *p2m) > { > +struct domain *d = p2m->domain; > +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > + > +p2m_lock(hostp2m); > + > /* Domain must have been paused */ > -ASSERT(atomic_read(>domain->pause_count)); > +ASSERT(atomic_read(>pause_count)); > > -vmx_domain_disable_pml(p2m->domain); > +vmx_domain_disable_pml(d); > > /* Disable EPT A/D bit */ > -p2m->ept.ad = 0; > -vmx_domain_update_eptp(p2m->domain); > +ept_set_ad_sync(hostp2m, false); > + > +vmx_domain_update_eptp(d); > + > +p2m_unlock(hostp2m); > } While in certain cases I would appreciate such transformations, I'm afraid the switch from p2m->domain to d in these two functions is hiding the meat of the change pretty well. In particular it is only now that I notice that you go from passed in p2m to domain to hostp2m. This makes me assume some altp2m could come in here too. Is it really intended for a change to an altp2m to be propagate to the hostp2m (and all other altp2m-s)? I can see why altp2m-s want to stay in sync (in certain regards) with the hostp2m, but for a sync the other way around there need to be deeper reasons. I admit that part of the problem here might be that the whole function hierarchy you change is tied to log-dirty enabling/ disabling, but I'm not convinced PML as well as A/D enabled status has to always match global(?) log-dirty enabled status. But I'm not the maintainer of this code, so please don't interpret my response as a strict request for change. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms
This patch is a pre-requisite for fixing the logdirty VGA issue (display freezes when switching to a new altp2m view early in a domain's lifetime), but sent separately for easier review. The new ept_set_ad_sync() function has been added to update all active altp2ms' ept.ad. New altp2ms will inherit the hostp2m's ept.ad value. ept_set_ad_sync() is now also the code responsible for locking updated p2ms. Signed-off-by: Razvan Cojocaru Suggested-by: George Dunlap --- Changes since V2: - Proper hostp2m locking in ept_{enable,disable}_pml(). - Added two asserts in ept_set_ad_sync(), checking that the passed p2m is the hostp2m, and that it has been locked. --- xen/arch/x86/mm/p2m-ept.c | 64 +-- xen/arch/x86/mm/p2m.c | 8 -- 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index d376966..3d228c2 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -1218,34 +1219,79 @@ static void ept_tlb_flush(struct p2m_domain *p2m) ept_sync_domain_mask(p2m, p2m->domain->dirty_cpumask); } +static void ept_set_ad_sync(struct p2m_domain *hostp2m, bool value) +{ +struct domain *d = hostp2m->domain; + +ASSERT(p2m_is_hostp2m(hostp2m)); +ASSERT(p2m_locked_by_me(hostp2m)); + +hostp2m->ept.ad = value; + +if ( unlikely(altp2m_active(d)) ) +{ +unsigned int i; + +for ( i = 0; i < MAX_ALTP2M; i++ ) +{ +struct p2m_domain *p2m; + +if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) ) +continue; + +p2m = d->arch.altp2m_p2m[i]; + +p2m_lock(p2m); +p2m->ept.ad = value; +p2m_unlock(p2m); +} +} +} + static void ept_enable_pml(struct p2m_domain *p2m) { +struct domain *d = p2m->domain; +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); + +p2m_lock(hostp2m); + /* Domain must have been paused */ -ASSERT(atomic_read(>domain->pause_count)); +ASSERT(atomic_read(>pause_count)); /* * No need to return whether vmx_domain_enable_pml has succeeded, as * ept_p2m_type_to_flags will do the check, and write protection will be * used if PML is not enabled. */ -if ( vmx_domain_enable_pml(p2m->domain) ) +if ( vmx_domain_enable_pml(d) ) return; /* Enable EPT A/D bit for PML */ -p2m->ept.ad = 1; -vmx_domain_update_eptp(p2m->domain); +ept_set_ad_sync(hostp2m, true); + +vmx_domain_update_eptp(d); + +p2m_unlock(hostp2m); } static void ept_disable_pml(struct p2m_domain *p2m) { +struct domain *d = p2m->domain; +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); + +p2m_lock(hostp2m); + /* Domain must have been paused */ -ASSERT(atomic_read(>domain->pause_count)); +ASSERT(atomic_read(>pause_count)); -vmx_domain_disable_pml(p2m->domain); +vmx_domain_disable_pml(d); /* Disable EPT A/D bit */ -p2m->ept.ad = 0; -vmx_domain_update_eptp(p2m->domain); +ept_set_ad_sync(hostp2m, false); + +vmx_domain_update_eptp(d); + +p2m_unlock(hostp2m); } static void ept_flush_pml_buffers(struct p2m_domain *p2m) @@ -1386,8 +1432,10 @@ void setup_ept_dump(void) void p2m_init_altp2m_ept(struct domain *d, unsigned int i) { struct p2m_domain *p2m = d->arch.altp2m_p2m[i]; +struct p2m_domain *hostp2m = p2m_get_hostp2m(d); struct ept_data *ept; +p2m->ept.ad = hostp2m->ept.ad; p2m->min_remapped_gfn = gfn_x(INVALID_GFN); p2m->max_remapped_gfn = 0; ept = >ept; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index d6a8810..d90c624 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -360,11 +360,7 @@ void p2m_enable_hardware_log_dirty(struct domain *d) struct p2m_domain *p2m = p2m_get_hostp2m(d); if ( p2m->enable_hardware_log_dirty ) -{ -p2m_lock(p2m); p2m->enable_hardware_log_dirty(p2m); -p2m_unlock(p2m); -} } void p2m_disable_hardware_log_dirty(struct domain *d) @@ -372,11 +368,7 @@ void p2m_disable_hardware_log_dirty(struct domain *d) struct p2m_domain *p2m = p2m_get_hostp2m(d); if ( p2m->disable_hardware_log_dirty ) -{ -p2m_lock(p2m); p2m->disable_hardware_log_dirty(p2m); -p2m_unlock(p2m); -} } void p2m_flush_hardware_cached_dirty(struct domain *d) -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel