|
|
|
|
|
|
|
|
|
|
xen-devel
RE: [Xen-devel] [PATCH v2] add SMEP support to HVM guest
To: |
Tim Deegan <Tim.Deegan@xxxxxxxxxx> |
Subject: |
RE: [Xen-devel] [PATCH v2] add SMEP support to HVM guest |
From: |
"Li, Xin" <xin.li@xxxxxxxxx> |
Date: |
Mon, 6 Jun 2011 18:51:42 +0800 |
Accept-language: |
zh-CN, en-US |
Acceptlanguage: |
zh-CN, en-US |
Cc: |
Jan, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <keir@xxxxxxx>, "'Keir Fraser \(keir.xen@xxxxxxxxx\)'" <keir.xen@xxxxxxxxx>, Beulich <JBeulich@xxxxxxxxxx> |
Delivery-date: |
Mon, 06 Jun 2011 03:53:54 -0700 |
Envelope-to: |
www-data@xxxxxxxxxxxxxxxxxxx |
In-reply-to: |
<20110606083750.GU5098@xxxxxxxxxxxxxxxxxxxxxxx> |
List-help: |
<mailto:xen-devel-request@lists.xensource.com?subject=help> |
List-id: |
Xen developer discussion <xen-devel.lists.xensource.com> |
List-post: |
<mailto:xen-devel@lists.xensource.com> |
List-subscribe: |
<http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe> |
List-unsubscribe: |
<http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe> |
References: |
<FC2FB65B4D919844ADE4BE3C2BB739AD5AB9EE1D@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20110606083750.GU5098@xxxxxxxxxxxxxxxxxxxxxxx> |
Sender: |
xen-devel-bounces@xxxxxxxxxxxxxxxxxxx |
Thread-index: |
AcwkJQjRKE5HbSOrRIqSjoi0JPEMTwACuT6g |
Thread-topic: |
[Xen-devel] [PATCH v2] add SMEP support to HVM guest |
> > - if ( hvm_nx_enabled(current) )
> > + if ( hvm_nx_enabled(current) ||
> > + (!(pfec & PFEC_user_mode) && hvm_smep_enabled(current)) )
>
> Shouldn't that be
> "if ( hvm_nx_enabled(current) || hvm_smep_enabled(current) )"?
A smep fault happens when
1) current privilege level < 3
> > + (!(pfec & PFEC_user_mode) && hvm_smep_enabled(current)) )
>
> Likewise.
>
> > l4p = (guest_l4e_t *) top_map;
> > gw->l4e = l4p[guest_l4_table_offset(va)];
> > gflags = guest_l4e_get_flags(gw->l4e) ^ iflags;
> > + user_flag &= gflags;
>
> There's no need to add SMEP-specific code at every level. The existing
> code already checks for flags that must be clear, so just arrange for
> _PAGE_USER to be in both mflags and iflags whenever SMEP is enabled and
> PFEC_user is clear.
2) user flags in all page table level entries are set instead of clear.
In current code, what I read is that it assumes some permission(s) is missing
in guest_walk_tables(). NX is special, but your code wisely used inverted logic
to seamlessly cover it. I did want to reuse the existing logic, but seems it
can't
accommodate SMEP logic easily.
Please advise!
> > @@ -277,6 +281,11 @@ guest_walk_tables(struct vcpu *v, struct
> > * walkers behave this way. */
> > if ( rc == 0 )
> > {
> > + if ( guest_supports_smep(v) && user_flag &&
> > + ((pfec & (PFEC_insn_fetch|PFEC_user_mode)) ==
> PFEC_insn_fetch) ) {
> > + rc = _PAGE_SMEP;
> > + goto out;
> > + }
>
> I think this hunk will probably go away entirely, but if not, please
> don't put it between the code below and the comment above that describes
> it.
I didn't figure out a better way than this, will add comments if we have to do
this way. Or do you have any other suggestion?
> > +#define _PAGE_SMEP 0x8000U
>
> What does this new code mean? You added code that returns it but not
> any that checks for it.
Actually the current logic doesn't check what is missing returned from
guest_walk_tables unless it needs to change error code, and the callers
mostly return INVALID_GFN.
I introduced _PAGE_SMEP just to let the caller know it is not 0. Probably
I can reuse one of the _PAGE_ macros, but the memory virtualization code
is subtle, I'm afraid I'll introduce bugs in other parts. That's to say, I do
want
to hear from you :)
Thanks!
-Xin
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
|
|
|
|