[PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru]

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 <rcojocaru@bitdefender.com>
Suggested-by: George Dunlap <george.dunlap@citrix.com>

---
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 <xen/domain_page.h>
 #include <xen/sched.h>
+#include <asm/altp2m.h>
 #include <asm/current.h>
 #include <asm/paging.h>
 #include <asm/types.h>
@@ -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(&p2m->domain->pause_count));
+    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
+    ASSERT(atomic_read(&d->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 = &p2m->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

Re: [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Jan Beulich]

>>> On 02.10.18 at 17:17, <rcojocaru@bitdefender.com> 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(&p2m->domain->pause_count));
> +    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
> +    ASSERT(atomic_read(&d->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

Re: [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru]

On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>> On 02.10.18 at 17:17, <rcojocaru@bitdefender.com> 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(&p2m->domain->pause_count));
>> +    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
>> +    ASSERT(atomic_read(&d->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: [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Jan Beulich]

>>> On 04.10.18 at 18:40, <rcojocaru@bitdefender.com> wrote:
> On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>>> On 02.10.18 at 17:17, <rcojocaru@bitdefender.com> 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(&p2m->domain->pause_count));
>>> +    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
>>> +    ASSERT(atomic_read(&d->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: [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru]

On 10/5/18 11:17 AM, Jan Beulich wrote:
>>>> On 04.10.18 at 18:40, <rcojocaru@bitdefender.com> wrote:
>> On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>>>> On 02.10.18 at 17:17, <rcojocaru@bitdefender.com> 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(&p2m->domain->pause_count));
>>>> +    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
>>>> +    ASSERT(atomic_read(&d->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?


Thanks,
Razvan

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

Re: [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Jan Beulich]

>>> On 05.10.18 at 10:41, <rcojocaru@bitdefender.com> wrote:
> On 10/5/18 11:17 AM, Jan Beulich wrote:
>>>>> On 04.10.18 at 18:40, <rcojocaru@bitdefender.com> wrote:
>>> On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>>>>> On 02.10.18 at 17:17, <rcojocaru@bitdefender.com> 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(&p2m->domain->pause_count));
>>>>> +    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
>>>>> +    ASSERT(atomic_read(&d->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: [PATCH V3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru]

On 10/5/18 12:01 PM, Jan Beulich wrote:
>>>> On 05.10.18 at 10:41, <rcojocaru@bitdefender.com> wrote:
>> On 10/5/18 11:17 AM, Jan Beulich wrote:
>>>>>> On 04.10.18 at 18:40, <rcojocaru@bitdefender.com> wrote:
>>>> On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>>>>>> On 02.10.18 at 17:17, <rcojocaru@bitdefender.com> 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(&p2m->domain->pause_count));
>>>>>> +    ASSERT(atomic_read(&d->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(&p2m->domain->pause_count));
>>>>>> +    ASSERT(atomic_read(&d->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 let me know if you're okay with Jan's
suggestion when you get a chance to read the thread.


Thanks,
Razvan

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