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 18:33:29 +0000 [thread overview]
Message-ID: <DBC12B0F5509554280826E40BCDEE8BE54FDB65F@ORSMSX104.amr.corp.intel.com> (raw)
In-Reply-To: <55AE04FE020000780009376F@prv-mh.provo.novell.com>
>From: Jan Beulich [mailto:JBeulich@suse.com]
>Sent: Monday, July 20, 2015 11:38 PM
>
>>>>>>>> +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?)
>
>Well, the optimization must be optimizing _something_. And hence
>_something_ must go sub-optimal when the optimization is being subverted.
>And the question is how much worse un-optimized is compared to optimized.
>
>It _looks like_ the overall effect really is just to avoid a one time (for a given
>non-preemptible operation) reset, but whether that's really the case depends
>on the calling contexts (which, as said a couple of times before, is hard to see
>for a patch that introduces functions without callers - hence the question).
>
As you now understand, invalidating an altp2m effectively resets it to be a (lazily-copied) exact duplicate of the host p2m again -- so losing any altp2m permissions restrictions or remaps. This is a first cut at minimizing the likelihood of that happening unnecessarily. There's some discussion on this first cut between Tim and Ed going back to February or March. The intention continues to be that this might be further optimized if real-world use shows this first cut to be inefficient.
>>>>>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?
>
>Of course we can, but should we? I.e. why use sub-optimal code when there
>clearly is a way to improve it? Counter question - why are you insisting to use a
>model requiring (in the earlier pointed out
>place) four comparisons when two can do? I realize this is only a small
>inefficiency here, but you should realize that they add up if we don't look to
>avoid them where possible.
We thought the code was easier to understand with min and max set to INVALID.
we can take your approach to avoid this inefficiency if you want.
Ravi
>
>Jan
next prev parent reply other threads:[~2015-07-21 18:33 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
2015-07-21 6:38 ` Jan Beulich
2015-07-21 18:33 ` Sahita, Ravi [this message]
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=DBC12B0F5509554280826E40BCDEE8BE54FDB65F@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.