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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/28/2018 05:19 PM, Razvan Cojocaru wrote:
> On 9/28/18 6:55 PM, Jan Beulich wrote:
>>>>> On 28.09.18 at 17:25, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> 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)
>>>>> +{
>>>>> +    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.
>>
>> p2m_init_altp2m() (and other code in p2m.c) suggests otherwise to me.
> 
> That's interesting, p2m_set_mem_access() is treating altp2m index 0 as
> the hostp2m:
> 
> 360 /*
> 361  * Set access type for a region of gfns.
> 362  * If gfn == INVALID_GFN, sets the default access type.
> 363  */
> 364 long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> 365                         uint32_t start, uint32_t mask,
> xenmem_access_t access,
> 366                         unsigned int altp2m_idx)
> 367 {
> 368     struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
> 369     p2m_access_t a;
> 370     unsigned long gfn_l;
> 371     long rc = 0;
> 372
> 373     /* altp2m view 0 is treated as the hostp2m */
> 374 #ifdef CONFIG_HVM
> 375     if ( altp2m_idx )
> 376     {
> 377         if ( altp2m_idx >= MAX_ALTP2M ||
> 378              d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> 379             return -EINVAL;
> 380
> 381         ap2m = d->arch.altp2m_p2m[altp2m_idx];
> 382     }
> 383 #else
> 384     ASSERT(!altp2m_idx);
> 385 #endif
> 
> which would seem to imply that either we should be able to treat
> d->arch.altp2m_p2m[0] and hostp2m interchangeably, or we are currently
> wasting an altp2m array slot.

Yes, ISTR that altp2m_idx 0 was specifically meant to be the host p2m
(unmodified).  But there seems to be some confusion over that -- this
seems to say it needs to be "manually" interpreted; some of the
mem_access code seems to think that it can just grab
d->arch.altp2m_p2m[0] and that will affect the hostp2m.  And as you say,
if we do the "manual interpretation", then we're allocating an extra p2m
in slot 0 that we're not using.

 -George

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