>>> On 18.07.15 at 00:32, <ravi.sah...@intel.com> wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>>Sent: Thursday, July 16, 2015 2:34 AM
>>
>>>>> On 16.07.15 at 11:16, <ravi.sah...@intel.com> wrote:
>>>> From: Jan Beulich [mailto:jbeul...@suse.com]
>>>>Sent: Tuesday, July 14, 2015 7:32 AM
>>>>>>> On 14.07.15 at 02:14, <edmund.h.wh...@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?
>>
>
> Yes, I should have described it more than that :-) so this part of the code
> is handling the log dirty transition of the page, and this page permission
> transition happens always on the hostp2m. Given the way the locking order is
> setup (hostp2m->altp2m-list-lock->altp2m and there was a separate writeup and
> discussion with George on that), at this point in this sequence there is a
> p2m lock (whether it's a hostp2m or altp2m lock depends on the mode of the
> domain) - the reason we have to drop the lock here first is due to what
> happens next; the permission changes in hostp2m will be serially propagated
> to altp2ms and not dropping the lock here would cause a locking order
> violation. Hope that clarifies.
Sadly it doesn't at all: You re-explain why you need to drop the lock,
while you fail to say anything on why this won't cause a race.
>>>>> +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"?
>>
>
> do_hvm_op declares a rc that is of type "long" and hence this returns a
> "long"
What type your caller(s) return is of no interest at all here: What
would you do if you had multiple callers with differing return types?
A function's return type should be chosen based on the range of
values it may return, and the result possibly widened to not yield
inefficient code (like in some of the uint16_t cases elsewhere in the
series would be necessary).
>>>>> +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 ...
>>
>
> To take that exact case, If min=0 and max=<end of address space> then any
> hostp2m change where the first mfn is dropped, will cause all altp2ms to be
> reset even if the mfn dropped doesn't affect altp2ms at all, which wont serve
> as an optimization at all - Hope that clarifies.
Again - no. I understand the optimization is gone then. But what's the
effect? I.e. will the guest, by extending this range to be arbitrarily
wide, be able to cause a long latency hypervisor operation (i.e. a DoS)?
>>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?
>
> Since 0 is a valid GFN so we cannot initialize min or max to 0 - since
> matching this condition (gfn_x(gfn) >= p2m->min_remapped_gfn && gfn_x(gfn) <=
> p2m->max_remapped_gfn) will cause a reset (throw away) of the altp2m to
> rebuild it from the hostp2m. So essentially what is being done here is the
> range is the non-existent set to start with, unless some altp2m changes
> occur,
> and then it is grown to be the smallest set around the gfns affected.
Again you just re-state what was already clear, yet you neglect
answering the actual question. Taking what you wrote above,
when max=0 (and min=INVALID_GFN), then
gfn_x(gfn) >= p2m->min_remapped_gfn &&
gfn_x(gfn) <= p2m->max_remapped_gfn
will be false for any value of gfn; in fact the "max" part won't
even be looked at because the "min" part will already be false
for any valid gfn, i.e. only the INVALID_GFN case would make it
to the "max" part.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
