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, <rcojoc...@bitdefender.com> wrote:
>>> On 9/28/18 5:52 PM, Jan Beulich wrote:
>>>>>>> On 28.09.18 at 13:55, <rcojoc...@bitdefender.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
