Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 4/3/19 7:04 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 17:48, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>>>> On 03.04.19 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
>>>> existing altp2ms, but they do so in a lazy manner, and also that won't
>>>> happen for altp2ms created after a while. So altp2ms will not
>>>> necessarily know about a page that the hostp2m knows about, which should
>>>> not stop us from setting mem access restrictions or the value of the SVE
>>>> bit.
>>>
>>> But even if the propagation is lazy, a "get" should then return the
>>> propagated value, shouldn't it? Or else callers (like is the case here)
>>> can be mislead. I do recognize that there may be callers who
>>> intentionally _don't_ want the propagated value, but I'd expect this
>>> to be the exception, not the rule. I wonder therefore whether there
>>> shouldn't be a wrapper around ->get_entry() to take care of this for
>>> callers which want the propagated value. Calling the bare hook would
>>> remain as is.
>>
>> But I believe that some hostp2m entries will never be propagated. Sorry 
>> that I've not been clear about this. This is what I meant by "won't 
>> happen for altp2ms created after a while".
>>
>> Take, for example, the case where there's only the hostp2m for the first 
>> 30 minutes of a guest's life, and in this time somebody calls 
>> ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if ( 
>> entry_written && p2m_is_hostp2m(p2m) ).
>>
>> But at this point there are no altp2ms, so there's nowhere to propagate 
>> these changes to.
>>
>> Now, at minute 31 we create a new altp2m. The changes will never be 
>> propagated here, so altp2m->get_entry() will always (rightfully) return 
>> an invalid MFN.
> 
> I guess I'm missing something here: Why "rightfully"? If the guest
> runs on this altp2m, it'll die a quick death then, won't it? Yet if
> this is intended to be demand-populate, then why would there be
> automatic propagation for existing altp2m-s (as you explain
> further up, unless I'm getting this wrong)?
> 
>> We only want to make an exception for setting the SVE bit or mem access 
>> restrictions on an entry in the altp2m, but having get_entry() (or a 
>> wrapper) return the hostp2m values and MFN might not necessarily be what 
>> current callers expect.
> 
> Then I still don't see why you would want to force propagation
> in these cases: If it's generally demand-populate, it can't be right
> to _fully_ populate that slot. You ought to be setting the access
> type or the "suppress #VE" bit alone, without propagating the
> MFN and alike. The later, when the entry actually gets propagated,
> the entered values should be used as overrides to what comes
> from the host p2m.
To try to answer your question to the best of my knowledge, the altp2m
propagation appears to be a hybrid between lazy propagation, in
hvm_hap_nested_page_fault() (xen/arch/x86/hvm/hvm.c):

1748     ap2m_active = altp2m_active(currd);
1749
1750     /*
1751      * Take a lock on the host p2m speculatively, to avoid potential
1752      * locking order problems later and to handle unshare etc.
1753      */
1754     hostp2m = p2m_get_hostp2m(currd);
1755     mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
1756                               P2M_ALLOC | (npfec.write_access ?
P2M_UNSHARE : 0),
1757                               NULL);
1758
1759     if ( ap2m_active )
1760     {
1761         if ( p2m_altp2m_lazy_copy(curr, gpa, gla, npfec, &p2m) )
1762         {
1763             /* entry was lazily copied from host -- retry */
1764             __put_gfn(hostp2m, gfn);
1765             rc = 1;
1766             goto out;
1767         }
1768
1769         mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
1770     }
1771     else
1772         p2m = hostp2m;

and immediate propagation, in ept_set_entry() (xen/arch/x86/mm/p2m-ept.c):

 827     if ( entry_written && p2m_is_hostp2m(p2m) )
 828     {
 829         ret = p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order,
p2mt, p2ma);
 830         if ( !rc )
 831             rc = ret;
 832     }

Either way, doing what our patch does should pose no problem as far as I
can tell. For the lazy copy case, p2m_altp2m_lazy_copy() will do
nothing, as our function has already populated the slot correctly:

2378 bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
2379                             unsigned long gla, struct npfec npfec,
2380                             struct p2m_domain **ap2m)
2381 {
2382     struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain);
2383     p2m_type_t p2mt;
2384     p2m_access_t p2ma;
2385     unsigned int page_order;
2386     gfn_t gfn = _gfn(paddr_to_pfn(gpa));
2387     unsigned long mask;
2388     mfn_t mfn;
2389     int rv;
2390
2391     *ap2m = p2m_get_altp2m(v);
2392
2393     mfn = get_gfn_type_access(*ap2m, gfn_x(gfn), &p2mt, &p2ma,
2394                               0, &page_order);
2395     __put_gfn(*ap2m, gfn_x(gfn));
2396
2397     if ( !mfn_eq(mfn, INVALID_MFN) )
2398         return 0;

And the other case is not a concern at all.

Am I still misunderstanding something?


Thanks,
Razvan

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