From: "Jan Beulich" <JBeulich@suse.com>
To: Ed White <edmund.h.white@intel.com>
Cc: Tim Deegan <tim@xen.org>, Ravi Sahita <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>,
xen-devel@lists.xen.org, tlengyel@novetta.com,
Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH v5 05/15] x86/altp2m: basic data structures and support routines.
Date: Tue, 14 Jul 2015 14:13:24 +0100 [thread overview]
Message-ID: <55A527140200007800090B74@mail.emea.novell.com> (raw)
In-Reply-To: <1436832903-12639-6-git-send-email-edmund.h.white@intel.com>
>>> On 14.07.15 at 02:14, <edmund.h.white@intel.com> wrote:
> +void
> +altp2m_vcpu_initialise(struct vcpu *v)
> +{
> + if ( v != current )
> + vcpu_pause(v);
> +
> + altp2m_vcpu_reset(v);
> + vcpu_altp2m(v).p2midx = 0;
> + atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
> +
> + altp2m_vcpu_update_eptp(v);
> +
> + if ( v != current )
> + vcpu_unpause(v);
> +}
> +
> +void
> +altp2m_vcpu_destroy(struct vcpu *v)
> +{
> + struct p2m_domain *p2m;
> +
> + if ( v != current )
> + vcpu_pause(v);
> +
> + if ( (p2m = p2m_get_altp2m(v)) )
> + atomic_dec(&p2m->active_vcpus);
> +
> + altp2m_vcpu_reset(v);
> +
> + altp2m_vcpu_update_eptp(v);
> + altp2m_vcpu_update_vmfunc_ve(v);
> +
> + if ( v != current )
> + vcpu_unpause(v);
> +}
There not being any caller of altp2m_vcpu_initialise() I can't judge
about its pausing requirements, but for the destroy case I can't
see what the pausing is good for. Considering its sole user it's also
not really clear why the two update operations need to be done
while destroying.
> @@ -6498,6 +6500,25 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
> return hvm_funcs.nhvm_intr_blocked(v);
> }
>
> +void altp2m_vcpu_update_eptp(struct vcpu *v)
> +{
> + if ( hvm_funcs.altp2m_vcpu_update_eptp )
> + hvm_funcs.altp2m_vcpu_update_eptp(v);
> +}
> +
> +void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v)
> +{
> + if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
> + hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
> +}
> +
> +bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
> +{
> + if ( hvm_funcs.altp2m_vcpu_emulate_ve )
> + return hvm_funcs.altp2m_vcpu_emulate_ve(v);
> + return 0;
> +}
The patch context here suggests that you're using pretty outdated
a tree - nhvm_interrupt_blocked() went away a week ago. In line
with the commit doing so, all of the above should become inline
functions.
> +uint16_t p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> +{
> + struct p2m_domain *p2m;
> + struct ept_data *ept;
> + uint16_t i;
> +
> + 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];
> + ept = &p2m->ept;
> +
> + if ( eptp == ept_get_eptp(ept) )
> + goto out;
> + }
> +
> + i = INVALID_ALTP2M;
> +
> +out:
Labels should be indented by at least one space.
Pending the rename, the function may also live in the wrong file (the
use of ept_get_eptp() suggests so even if the function itself got
renamed).
> +bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, uint16_t idx)
> +{
> + struct domain *d = v->domain;
> + bool_t rc = 0;
> +
> + if ( idx > MAX_ALTP2M )
> + return rc;
> +
> + altp2m_list_lock(d);
> +
> + if ( d->arch.altp2m_eptp[idx] != INVALID_MFN )
> + {
> + if ( idx != vcpu_altp2m(v).p2midx )
> + {
> + atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
> + vcpu_altp2m(v).p2midx = idx;
> + atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
Are the two results of p2m_get_altp2m(v) here guaranteed to be
distinct? If they aren't, is it safe to decrement first (potentially
dropping the count to zero)?
> @@ -722,6 +731,27 @@ void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
> l1_pgentry_t *p, l1_pgentry_t new, unsigned int level);
>
> /*
> + * Alternate p2m: shadow p2m tables used for alternate memory views
> + */
> +
> +/* get current alternate p2m table */
> +static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
> +{
> + struct domain *d = v->domain;
> + uint16_t index = vcpu_altp2m(v).p2midx;
> +
> + ASSERT(index < MAX_ALTP2M);
> +
> + return (index == INVALID_ALTP2M) ? NULL : d->arch.altp2m_p2m[index];
> +}
Looking at this again, I'm afraid I'd still prefer index < MAX_ALTP2M
in the return statement (and the ASSERT() dropped): The ASSERT()
does nothing in a debug=n build, and hence wouldn't shield us from
possibly having to issue an XSA if somehow an access outside the
array's bounds turned out possible.
I've also avoided to repeat any of the un-addressed points that I
raised against earlier versions.
Jan
next prev parent reply other threads:[~2015-07-14 13:13 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 [this message]
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
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=55A527140200007800090B74@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.