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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.19 at 19:41, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 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,
>[...]
> and immediate propagation,

And I've been questioning whether this is an appropriate model,
i.e. whether the result at all times is always the same as either of
the pure variants. Thinking about it some more, it looks like it
indeed is. Yet then the question is why both functions don't use
p2m_altp2m_lazy_copy(), but rather construct things by other
means. I realize the function's gla and npfec parameters might
make it look non-suitable here, but neither parameter is actually
used by the function afaics.

And of course the other point remains: For callers who want to
see the propagate value (irrespective of what the physical
tables say), there would better be a function giving them the
intended result, rather than making every such caller go through
extra hoops.

Jan



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