From: "Jan Beulich" <JBeulich@suse.com>
To: Ravi Sahita <ravi.sahita@intel.com>
Cc: Tim Deegan <tim@xen.org>, 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>,
Edmund H White <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: Mon, 20 Jul 2015 07:53:03 +0100 [thread overview]
Message-ID: <55ACB6EF0200007800092E1B@mail.emea.novell.com> (raw)
In-Reply-To: <DBC12B0F5509554280826E40BCDEE8BE54FD9227@ORSMSX104.amr.corp.intel.com>
>>> 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.
>>>>> +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
next prev parent reply other threads:[~2015-07-20 6:53 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 [this message]
2015-07-21 5:46 ` Sahita, Ravi
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=55ACB6EF0200007800092E1B@mail.emea.novell.com \
--to=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=ravi.sahita@intel.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.