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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>> On 02.10.18 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel