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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.07.15 at 11:16, <ravi.sahita@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>Sent: Tuesday, July 14, 2015 7:32 AM
>>>>> On 14.07.15 at 02:14, <edmund.h.white@intel.com> wrote:
>>> @@ -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

To be honest I expected a little more than just "no" here. Now
I have to ask - why?

>>> --- 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.

The only two calls currently there are ept_p2m_{,un}init(), which
need to be there with the current code structuring. Everything
else that's EPT-specific should be abstracted through hooks set
by ept_p2m_init().

>>> +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.

I don't see the connection. The function only returns zero or -E...
values, so why would its return type be "long"?

>>> +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.

And will always produce worse code.

>>> +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.

Sadly it doesn't, it just re-states what I already understood and doesn't
answer the question: What happens if min=0 and
max=<end-of-address-space>? I.e. can the guest nullify the
optimization by careful fiddling issuing some of the new hypercalls, and if
so will this have any negative impact on the hypervisor? I'm asking this
from a security standpoint ...

Nor do I find my question answered why max can't be initialized to zero:
You don't care whether max is a valid GFN when a certain GFN doesn't
fall in the (then empty) [min, max] range. What am I missing?

Jan