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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 9/28/18 5:52 PM, Jan Beulich wrote:
>>>> On 28.09.18 at 13:55, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> @@ -1218,34 +1219,67 @@ 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 *p2m, int value)
> 
> Can the second parameter be bool please (and the true/false
> arguments in the callers)?

Of course.

>> +{
>> +    struct domain *d = p2m->domain;
>> +    unsigned int i;
>> +
>> +    if ( likely(!altp2m_active(d)) )
>> +    {
>> +        p2m_lock(p2m);
>> +        p2m->ept.ad = value;
>> +        p2m_unlock(p2m);
>> +
>> +        return;
>> +    }
> 
> Why would you want to skip updating the host p2m's flag when
> altp2m is active?

It's not really skipped if I understand the altp2m code correctly: in
that case the hostp2m is d->arch.altp2m_p2m[0], which is take care of in
the loop below the code you've quoted.

>> --- 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);
>> -    }
>>  }
> 
> I don't understand how this removal can be correct.

Do you mean because the lock is supposed to protect the
vmx_domain_disable_pml(d); and vmx_domain_update_eptp(d); calls as well?
Otherwise, I've removed them because the locking is now done in
ept_set_ad_sync().

I had hoped that would address the following comments from George from a
previous RFC patch:

">  static void ept_enable_pml(struct p2m_domain *p2m)
>  {
> +    unsigned int i;
> +    struct domain *d = p2m->domain;
> +
>      /* 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);
> +
> +    if ( altp2m_active(d) )
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +        {
> +            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> +                continue;
> +
> +            p2m = d->arch.altp2m_p2m[i];
> +            p2m->ept.ad = 1;
> +        }

You're not grabbing the respective p2m locks here -- I'm pretty sure
this will end up being three separate instructions (read, set ad bit,
write).

But there would something a bit funny here about grabbing the main p2m
lock in p2m.c, and then grabbing altp2m locks within the function.  But
on the other hand, you clearly only want to call this...

> +    vmx_domain_update_eptp(d);

...once.  Some refactoring might be wanted."

Hence my refactoring attempt.


Thanks,
Razvan

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