Re: [Xen-devel] [PATCH v5 10/15] x86/altp2m: add remaining support routines.

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Tuesday, July 14, 2015 7:32 AM
>
>>>> On 14.07.15 at 02:14, <edmund.h.white@xxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2802,10 +2802,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>unsigned long gla,
>>      mfn_t mfn;
>>      struct vcpu *curr = current;
>>      struct domain *currd = curr->domain;
>> -    struct p2m_domain *p2m;
>> +    struct p2m_domain *p2m, *hostp2m;
>>      int rc, fall_through = 0, paged = 0;
>>      int sharing_enomem = 0;
>>      vm_event_request_t *req_ptr = NULL;
>> +    bool_t ap2m_active = 0;
>
>Pointless initializer afaict.
>

True - didn't use to be - will fix.

>> @@ -2865,11 +2866,31 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>unsigned long gla,
>>          goto out;
>>      }
>>
>> -    p2m = p2m_get_hostp2m(currd);
>> -    mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
>> +    ap2m_active = altp2m_active(currd);
>> +
>> +    /* Take a lock on the host p2m speculatively, to avoid potential
>> +     * locking order problems later and to handle unshare etc.
>> +     */
>
>Comment style.
>

Got it

>> @@ -2965,9 +3003,15 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>unsigned long gla,
>>          if ( npfec.write_access )
>>          {
>>              paging_mark_dirty(currd, mfn_x(mfn));
>> +            /* If p2m is really an altp2m, unlock here to avoid lock 
>> ordering
>> +             * violation when the change below is propagated from host p2m 
>> */
>> +            if ( ap2m_active )
>> +                __put_gfn(p2m, gfn);
>>              p2m_change_type_one(currd, gfn, p2m_ram_logdirty,
>> p2m_ram_rw);
>
>And this won't result in any races?
>

No

>Also - comment style again (and more elsewhere).
>

Got it

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2037,6 +2037,391 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct
>vcpu *v, uint16_t idx)
>>      return rc;
>>  }
>>
>> +void p2m_flush_altp2m(struct domain *d) {
>> +    uint16_t i;
>> +
>> +    altp2m_list_lock(d);
>> +
>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        p2m_flush_table(d->arch.altp2m_p2m[i]);
>> +        /* Uninit and reinit ept to force TLB shootdown */
>> +        ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>> +        ept_p2m_init(d->arch.altp2m_p2m[i]);
>
>ept_... in non-EPT code again.
>

There is no non-EPT altp2m implementation, and this file already includes ept.. 
callouts for p2m's implemented using EPT's.


>> +        d->arch.altp2m_eptp[i] = INVALID_MFN;
>> +    }
>> +
>> +    altp2m_list_unlock(d);
>> +}
>> +
>> +static void p2m_init_altp2m_helper(struct domain *d, uint16_t i) {
>> +    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>> +    struct ept_data *ept;
>> +
>> +    p2m->min_remapped_gfn = INVALID_GFN;
>> +    p2m->max_remapped_gfn = INVALID_GFN;
>> +    ept = &p2m->ept;
>> +    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
>> +    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
>
>Same here.
>

Same as above... this file already includes ept.. callouts for p2m's 
implemented using EPT's.

>> +long p2m_init_altp2m_by_id(struct domain *d, uint16_t idx) {
>> +    long rc = -EINVAL;
>
>Why long (for both variable and function return type)? (More of these in
>functions below.)
>

Because the error variable in the code that calls these (in hvm.c) is a long, 
and you had given feedback earlier to propagate the returns from these 
functions through that calling code.


>> +long p2m_init_next_altp2m(struct domain *d, uint16_t *idx) {
>> +    long rc = -EINVAL;
>> +    uint16_t i;
>
>As in the earlier patch(es) - unsigned int.
>

Ok, but why does it matter? uint16_t will always suffice.


>> +long p2m_change_altp2m_gfn(struct domain *d, uint16_t idx,
>> +                             gfn_t old_gfn, gfn_t new_gfn) {
>> +    struct p2m_domain *hp2m, *ap2m;
>> +    p2m_access_t a;
>> +    p2m_type_t t;
>> +    mfn_t mfn;
>> +    unsigned int page_order;
>> +    long rc = -EINVAL;
>> +
>> +    if ( idx > MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN )
>> +        return rc;
>> +
>> +    hp2m = p2m_get_hostp2m(d);
>> +    ap2m = d->arch.altp2m_p2m[idx];
>> +
>> +    p2m_lock(ap2m);
>> +
>> +    mfn = ap2m->get_entry(ap2m, gfn_x(old_gfn), &t, &a, 0, NULL,
>> + NULL);
>> +
>> +    if ( gfn_x(new_gfn) == INVALID_GFN )
>> +    {
>> +        if ( mfn_valid(mfn) )
>> +            p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn),
>PAGE_ORDER_4K);
>> +        rc = 0;
>> +        goto out;
>> +    }
>> +
>> +    /* Check host p2m if no valid entry in alternate */
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        mfn = hp2m->get_entry(hp2m, gfn_x(old_gfn), &t, &a,
>> +                              P2M_ALLOC | P2M_UNSHARE, &page_order,
>> + NULL);
>> +
>> +        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>> +            goto out;
>> +
>> +        /* If this is a superpage, copy that first */
>> +        if ( page_order != PAGE_ORDER_4K )
>> +        {
>> +            gfn_t gfn;
>> +            unsigned long mask;
>> +
>> +            mask = ~((1UL << page_order) - 1);
>> +            gfn = _gfn(gfn_x(old_gfn) & mask);
>> +            mfn = _mfn(mfn_x(mfn) & mask);
>> +
>> +            if ( ap2m->set_entry(ap2m, gfn_x(gfn), mfn, page_order,
>> + t, a, 1)
>> )
>> +                goto out;
>> +        }
>> +    }
>> +
>> +    mfn = ap2m->get_entry(ap2m, gfn_x(new_gfn), &t, &a, 0, NULL,
>> + NULL);
>> +
>> +    if ( !mfn_valid(mfn) )
>> +        mfn = hp2m->get_entry(hp2m, gfn_x(new_gfn), &t, &a, 0, NULL,
>> + NULL);
>> +
>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
>> +        goto out;
>> +
>> +    if ( !ap2m->set_entry(ap2m, gfn_x(old_gfn), mfn, PAGE_ORDER_4K, t, a,
>> +                          (current->domain != d)) )
>> +    {
>> +        rc = 0;
>> +
>> +        if ( ap2m->min_remapped_gfn == INVALID_GFN ||
>> +             gfn_x(new_gfn) < ap2m->min_remapped_gfn )
>> +            ap2m->min_remapped_gfn = gfn_x(new_gfn);
>> +        if ( ap2m->max_remapped_gfn == INVALID_GFN ||
>> +             gfn_x(new_gfn) > ap2m->max_remapped_gfn )
>> +            ap2m->max_remapped_gfn = gfn_x(new_gfn);
>
>For the purpose here (and without conflict with the consumer side) it would
>seem to be better to initialize max_remapped_gfn to zero, as then both if()
>can get away with just one comparison.
>

See below for a full explanation of this portion and why max_remapped_gfn 
cannot be inited to 0 (zero is a valid gfn)...

>> +void p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
>> +                                 mfn_t mfn, unsigned int page_order,
>> +                                 p2m_type_t p2mt, p2m_access_t p2ma)
>> +{
>> +    struct p2m_domain *p2m;
>> +    p2m_access_t a;
>> +    p2m_type_t t;
>> +    mfn_t m;
>> +    uint16_t i;
>> +    bool_t reset_p2m;
>> +    unsigned int reset_count = 0;
>> +    uint16_t last_reset_idx = ~0;
>> +
>> +    if ( !altp2m_active(d) )
>> +        return;
>> +
>> +    altp2m_list_lock(d);
>> +
>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
>> +            continue;
>> +
>> +        p2m = d->arch.altp2m_p2m[i];
>> +        m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL);
>> +
>> +        reset_p2m = 0;
>> +
>> +        /* Check for a dropped page that may impact this altp2m */
>> +        if ( mfn_x(mfn) == INVALID_MFN &&
>> +             gfn_x(gfn) >= p2m->min_remapped_gfn &&
>> +             gfn_x(gfn) <= p2m->max_remapped_gfn )
>> +            reset_p2m = 1;
>
>Considering that this looks like an optimization, what's the downside of
>possibly having min=0 and max=<end-of-address-space>? I.e.
>can there a long latency operation result that's this way a guest can effect?
>

... A p2m is a gfn->mfn map, amongst other things. There is a reverse mfn->gfn 
map, but that is only valid for the host p2m. Unless the remap altp2m hypercall 
is used, the gfn->mfn map in every altp2m mirrors the gfn->mfn map in the host 
p2m (or a subset thereof, due to lazy-copy), so handling removal of an mfn from 
a guest is simple: do a reverse look up for the host p2m and mark the relevant 
gfn as invalid, then do the same for every altp2m where that gfn is currently 
valid.

Remap changes things: it says take gfn1 and replace ->mfn with the ->mfn of 
gfn2. Here is where the optimization is used and the  invalidate logic is: 
record the lowest and highest gfn2's that have been used in remap ops; if an 
mfn is dropped from the hostp2m, for the purposes of altp2m invalidation, see 
if the gfn derived from the host p2m reverse lookup falls within the range of 
used gfn2's. If it does, an invalidation is required. Which is why min and max 
are inited the way they are - hope the explanation clarifies this optimization.

Ravi

>Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel