From: "Sahita, Ravi" <ravi.sahita@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>, "Sahita, Ravi" <ravi.sahita@intel.com>,
Wei Liu <wei.liu2@citrix.com>,
George Dunlap <george.dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
"White, Edmund H" <edmund.h.white@intel.com>,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
"tlengyel@novetta.com" <tlengyel@novetta.com>,
Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH v5 10/15] x86/altp2m: add remaining support routines.
Date: Tue, 21 Jul 2015 05:46:31 +0000 [thread overview]
Message-ID: <DBC12B0F5509554280826E40BCDEE8BE54FDACDF@ORSMSX104.amr.corp.intel.com> (raw)
In-Reply-To: <55ACB6EF0200007800092E1B@mail.emea.novell.com>
>From: Jan Beulich [mailto:JBeulich@suse.com]
>Sent: Sunday, July 19, 2015 11:53 PM
>
>>>> On 18.07.15 at 00:32, <ravi.sahita@intel.com> wrote:
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>Sent: Thursday, July 16, 2015 2:34 AM
>>>
>>>>>> 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?
>>>
>>
>> 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.
>
It only drops the lock on the altp2m, which is no longer required in this function anyway. The important aspect is that there is still a lock held on the host p2m, and that is dropped after the log-dirty updates, as it would be in the non-altp2m case (maybe that was the part I should have explained clearly in the para above). Does that clarify or do you see a particular race condition here? (We don't ).
>>>>>> +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).
>
What do you suggest the return type be?
>>>>>> +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
>>>space>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)?
>
The extent of the range affects the likelihood of an invalidation. It has no impact on the cost of an invalidation (so no its not a DoS issue).
I'm not sure what change you are suggesting here or just clarification (if you think this optimization is confusing perhaps some documentation of this optimization will help?)
>>>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
>> p2m->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.
>
You are suggesting an alternative which will work, but what we have will also produce the same results, and the results are correct for both cases - so can we go with the approach we have taken currently?
Ravi
>Jan
next prev parent reply other threads:[~2015-07-21 5:46 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-14 0:14 [PATCH v5 00/15] Alternate p2m: support multiple copies of host p2m Ed White
2015-07-14 0:14 ` [PATCH v5 01/15] common/domain: Helpers to pause a domain while in context Ed White
2015-07-14 0:14 ` [PATCH v5 02/15] VMX: VMFUNC and #VE definitions and detection Ed White
2015-07-14 0:14 ` [PATCH v5 03/15] VMX: implement suppress #VE Ed White
2015-07-14 12:46 ` Jan Beulich
2015-07-14 13:47 ` George Dunlap
2015-07-14 0:14 ` [PATCH v5 04/15] x86/HVM: Hardware alternate p2m support detection Ed White
2015-07-14 0:14 ` [PATCH v5 05/15] x86/altp2m: basic data structures and support routines Ed White
2015-07-14 13:13 ` Jan Beulich
2015-07-14 14:45 ` George Dunlap
2015-07-14 14:58 ` Jan Beulich
2015-07-16 8:57 ` Sahita, Ravi
2015-07-16 9:07 ` Jan Beulich
2015-07-17 22:36 ` Sahita, Ravi
2015-07-20 6:20 ` Jan Beulich
2015-07-21 5:18 ` Sahita, Ravi
2015-07-14 15:57 ` George Dunlap
2015-07-21 17:44 ` Sahita, Ravi
2015-07-14 0:14 ` [PATCH v5 06/15] VMX/altp2m: add code to support EPTP switching and #VE Ed White
2015-07-14 13:57 ` Jan Beulich
2015-07-16 9:20 ` Sahita, Ravi
2015-07-16 9:38 ` Jan Beulich
2015-07-17 21:08 ` Sahita, Ravi
2015-07-20 6:21 ` Jan Beulich
2015-07-21 5:49 ` Sahita, Ravi
2015-07-14 0:14 ` [PATCH v5 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator Ed White
2015-07-14 14:04 ` Jan Beulich
2015-07-14 17:56 ` Sahita, Ravi
2015-07-17 22:41 ` Sahita, Ravi
2015-07-14 0:14 ` [PATCH v5 08/15] x86/altp2m: add control of suppress_ve Ed White
2015-07-14 17:03 ` George Dunlap
2015-07-14 0:14 ` [PATCH v5 09/15] x86/altp2m: alternate p2m memory events Ed White
2015-07-14 14:08 ` Jan Beulich
2015-07-16 9:22 ` Sahita, Ravi
2015-07-14 0:14 ` [PATCH v5 10/15] x86/altp2m: add remaining support routines Ed White
2015-07-14 14:31 ` Jan Beulich
2015-07-16 9:16 ` Sahita, Ravi
2015-07-16 9:34 ` Jan Beulich
2015-07-17 22:32 ` Sahita, Ravi
2015-07-20 6:53 ` Jan Beulich
2015-07-21 5:46 ` Sahita, Ravi [this message]
2015-07-21 6:38 ` Jan Beulich
2015-07-21 18:33 ` Sahita, Ravi
2015-07-22 7:33 ` Jan Beulich
2015-07-16 14:44 ` George Dunlap
2015-07-17 21:01 ` Sahita, Ravi
2015-07-14 0:14 ` [PATCH v5 11/15] x86/altp2m: define and implement alternate p2m HVMOP types Ed White
2015-07-14 14:36 ` Jan Beulich
2015-07-16 9:02 ` Sahita, Ravi
2015-07-16 9:09 ` Jan Beulich
2015-07-14 0:15 ` [PATCH v5 12/15] x86/altp2m: Add altp2mhvm HVM domain parameter Ed White
2015-07-14 0:15 ` [PATCH v5 13/15] x86/altp2m: XSM hooks for altp2m HVM ops Ed White
2015-07-14 0:15 ` [PATCH v5 14/15] tools/libxc: add support to altp2m hvmops Ed White
2015-07-14 0:15 ` [PATCH v5 15/15] tools/xen-access: altp2m testcases Ed White
2015-07-14 9:56 ` Wei Liu
2015-07-14 11:52 ` Lengyel, Tamas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DBC12B0F5509554280826E40BCDEE8BE54FDACDF@ORSMSX104.amr.corp.intel.com \
--to=ravi.sahita@intel.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=dgdegra@tycho.nsa.gov \
--cc=edmund.h.white@intel.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=tim@xen.org \
--cc=tlengyel@novetta.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.