* [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
@ 2018-05-04 17:28 Andrew Cooper
2018-05-04 18:45 ` Boris Ostrovsky
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Andrew Cooper @ 2018-05-04 17:28 UTC (permalink / raw)
To: Xen-devel
Cc: Juergen Gross, Jan Beulich, Andrew Cooper, Suravee Suthikulpanit,
Boris Ostrovsky, Brian Woods
We don't advertise SVM in CPUID so a PV guest shouldn't be under the
impression that it can use SVM functionality, but despite this, it really
shouldn't see SVME set when reading EFER.
Introduce EFER_KNOWN_MASK to whitelist the features Xen knows about, and use
this to clamp the guests view.
Take the opportunity to reuse the mask to simplify svm_vmcb_isvalid(), and
change "undefined" to "unknown" in the print message, as there is at least
EFER.TCE (Translation Cache Extension) defined but unknown to Xen.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>
CC: Juergen Gross <jgross@suse.com>
Arguably, this wants backporting to the stable trees, so should be considered
for 4.11 at this point.
---
xen/arch/x86/hvm/svm/svmdebug.c | 5 ++---
xen/arch/x86/pv/emul-priv-op.c | 4 +++-
xen/include/asm-x86/msr-index.h | 3 +++
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 6c215d1..d35e405 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -133,9 +133,8 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
PRINTF("DR7: bits [63:32] are not zero (%#"PRIx64")\n",
vmcb_get_dr7(vmcb));
- if ( efer & ~(EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | EFER_SVME |
- EFER_LMSLE | EFER_FFXSE) )
- PRINTF("EFER: undefined bits are not zero (%#"PRIx64")\n", efer);
+ if ( efer & ~EFER_KNOWN_MASK )
+ PRINTF("EFER: unknown bits are not zero (%#"PRIx64")\n", efer);
if ( hvm_efer_valid(v, efer, -1) )
PRINTF("EFER: %s (%"PRIx64")\n", hvm_efer_valid(v, efer, -1), efer);
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 15f42b3..8293f31 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -867,7 +867,9 @@ static int read_msr(unsigned int reg, uint64_t *val,
return X86EMUL_OKAY;
case MSR_EFER:
- *val = read_efer();
+ /* Hide unknown bits, and unconditionally hide SVME from guests. */
+ *val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
+ /* Hide the 64-bit features from 32-bit guests. */
if ( is_pv_32bit_domain(currd) )
*val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
return X86EMUL_OKAY;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index c9f44eb..6d94d65 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -31,6 +31,9 @@
#define EFER_LMSLE (1<<_EFER_LMSLE)
#define EFER_FFXSE (1<<_EFER_FFXSE)
+#define EFER_KNOWN_MASK (EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \
+ EFER_SVME | EFER_LMSLE | EFER_FFXSE)
+
/* Speculation Controls. */
#define MSR_SPEC_CTRL 0x00000048
#define SPEC_CTRL_IBRS (_AC(1, ULL) << 0)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-04 17:28 [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests Andrew Cooper
@ 2018-05-04 18:45 ` Boris Ostrovsky
2018-05-04 18:48 ` Andrew Cooper
2018-05-07 6:28 ` Juergen Gross
2018-05-07 7:03 ` Jan Beulich
2 siblings, 1 reply; 8+ messages in thread
From: Boris Ostrovsky @ 2018-05-04 18:45 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Juergen Gross, Brian Woods, Suravee Suthikulpanit, Jan Beulich
On 05/04/2018 01:28 PM, Andrew Cooper wrote:
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -31,6 +31,9 @@
> #define EFER_LMSLE (1<<_EFER_LMSLE)
> #define EFER_FFXSE (1<<_EFER_FFXSE)
>
> +#define EFER_KNOWN_MASK (EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \
I think there is an extra tab here (but this may be my email client not
showing it properly)
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-04 18:45 ` Boris Ostrovsky
@ 2018-05-04 18:48 ` Andrew Cooper
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2018-05-04 18:48 UTC (permalink / raw)
To: Boris Ostrovsky, Xen-devel
Cc: Juergen Gross, Brian Woods, Suravee Suthikulpanit, Jan Beulich
On 04/05/18 19:45, Boris Ostrovsky wrote:
> On 05/04/2018 01:28 PM, Andrew Cooper wrote:
>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -31,6 +31,9 @@
>> #define EFER_LMSLE (1<<_EFER_LMSLE)
>> #define EFER_FFXSE (1<<_EFER_FFXSE)
>>
>> +#define EFER_KNOWN_MASK (EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \
>
> I think there is an extra tab here (but this may be my email client not
> showing it properly)
Its correct in the file, but renders incorrectly everywhere else. (I
did a doubletake the first time I saw the email.)
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Thanks,
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-04 17:28 [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests Andrew Cooper
2018-05-04 18:45 ` Boris Ostrovsky
@ 2018-05-07 6:28 ` Juergen Gross
2018-05-07 7:03 ` Jan Beulich
2 siblings, 0 replies; 8+ messages in thread
From: Juergen Gross @ 2018-05-07 6:28 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Boris Ostrovsky, Brian Woods, Suravee Suthikulpanit, Jan Beulich
On 04/05/18 19:28, Andrew Cooper wrote:
> We don't advertise SVM in CPUID so a PV guest shouldn't be under the
> impression that it can use SVM functionality, but despite this, it really
> shouldn't see SVME set when reading EFER.
>
> Introduce EFER_KNOWN_MASK to whitelist the features Xen knows about, and use
> this to clamp the guests view.
>
> Take the opportunity to reuse the mask to simplify svm_vmcb_isvalid(), and
> change "undefined" to "unknown" in the print message, as there is at least
> EFER.TCE (Translation Cache Extension) defined but unknown to Xen.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Juergen Gross <jgross@suse.com>
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-04 17:28 [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests Andrew Cooper
2018-05-04 18:45 ` Boris Ostrovsky
2018-05-07 6:28 ` Juergen Gross
@ 2018-05-07 7:03 ` Jan Beulich
2018-05-07 7:30 ` Andrew Cooper
2 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2018-05-07 7:03 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Boris Ostrovsky, brian.woods,
Suravee Suthikulpanit, Xen-devel
>>> On 04.05.18 at 19:28, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -867,7 +867,9 @@ static int read_msr(unsigned int reg, uint64_t *val,
> return X86EMUL_OKAY;
>
> case MSR_EFER:
> - *val = read_efer();
> + /* Hide unknown bits, and unconditionally hide SVME from guests. */
> + *val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
> + /* Hide the 64-bit features from 32-bit guests. */
> if ( is_pv_32bit_domain(currd) )
> *val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
Wouldn't it be better then to also move the LMSLE hiding up? And what about
SCE? PV guests not being allowed to write EFER, I would think they shouldn't
see bits they aren't supposed to care about and aren't able to set. If we
were to allow such writes, I assume it would only be NX and maybe FFXSE
which we'd permit the guest to control. Obviously (I think) LME and LMA ought
to be seen set by 64-bit guests.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-07 7:03 ` Jan Beulich
@ 2018-05-07 7:30 ` Andrew Cooper
2018-05-07 8:00 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2018-05-07 7:30 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Boris Ostrovsky, brian.woods,
Suravee Suthikulpanit, Xen-devel
On 07/05/2018 08:03, Jan Beulich wrote:
>>>> On 04.05.18 at 19:28, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -867,7 +867,9 @@ static int read_msr(unsigned int reg, uint64_t *val,
>> return X86EMUL_OKAY;
>>
>> case MSR_EFER:
>> - *val = read_efer();
>> + /* Hide unknown bits, and unconditionally hide SVME from guests. */
>> + *val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
>> + /* Hide the 64-bit features from 32-bit guests. */
>> if ( is_pv_32bit_domain(currd) )
>> *val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
> Wouldn't it be better then to also move the LMSLE hiding up?
Actually, on second consideration, LMSLE shouldn't be hidden. If LMSLE
is actually active, then segment descriptors behave differently whether
the guest knows about it or not.
> And what about SCE?
Rather more difficult to say, given its cross-vendor behaviour. It
should probably be clobbered if 32bit && Intel, but 32bit AMD can use
SYSCALL if it wishes.
> PV guests not being allowed to write EFER, I would think they shouldn't
> see bits they aren't supposed to care about and aren't able to set. If we
> were to allow such writes, I assume it would only be NX and maybe FFXSE
> which we'd permit the guest to control. Obviously (I think) LME and LMA ought
> to be seen set by 64-bit guests.
I don't see any value in letting PV guests write to EFER. They never
have been able to in the past, and letting them play with NX has a
direct safety impact on Xen when XPTI is not in use.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-07 7:30 ` Andrew Cooper
@ 2018-05-07 8:00 ` Jan Beulich
2018-05-07 8:10 ` Andrew Cooper
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2018-05-07 8:00 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Boris Ostrovsky, brian.woods,
Suravee Suthikulpanit, Xen-devel
>>> On 07.05.18 at 09:30, <andrew.cooper3@citrix.com> wrote:
> On 07/05/2018 08:03, Jan Beulich wrote:
>>>>> On 04.05.18 at 19:28, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/pv/emul-priv-op.c
>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>>> @@ -867,7 +867,9 @@ static int read_msr(unsigned int reg, uint64_t *val,
>>> return X86EMUL_OKAY;
>>>
>>> case MSR_EFER:
>>> - *val = read_efer();
>>> + /* Hide unknown bits, and unconditionally hide SVME from guests. */
>>> + *val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
>>> + /* Hide the 64-bit features from 32-bit guests. */
>>> if ( is_pv_32bit_domain(currd) )
>>> *val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
>> Wouldn't it be better then to also move the LMSLE hiding up?
>
> Actually, on second consideration, LMSLE shouldn't be hidden. If LMSLE
> is actually active, then segment descriptors behave differently whether
> the guest knows about it or not.
Actually - the placement shouldn't matter at all: I think it can't be set anyway,
as we would only possibly allow HVM guests to set it, i.e. the EFER value
loaded during #VMEXIT won't ever have the bit set at present. And even the
HVM side code uniformly forces cpu_has_lmsl to false right now.
>> And what about SCE?
>
> Rather more difficult to say, given its cross-vendor behaviour. It
> should probably be clobbered if 32bit && Intel, but 32bit AMD can use
> SYSCALL if it wishes.
Ah, yes, agreed.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests
2018-05-07 8:00 ` Jan Beulich
@ 2018-05-07 8:10 ` Andrew Cooper
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2018-05-07 8:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Juergen Gross, Boris Ostrovsky, brian.woods,
Suravee Suthikulpanit, Xen-devel
On 07/05/2018 09:00, Jan Beulich wrote:
>>>> On 07.05.18 at 09:30, <andrew.cooper3@citrix.com> wrote:
>> On 07/05/2018 08:03, Jan Beulich wrote:
>>>>>> On 04.05.18 at 19:28, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/pv/emul-priv-op.c
>>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>>>> @@ -867,7 +867,9 @@ static int read_msr(unsigned int reg, uint64_t *val,
>>>> return X86EMUL_OKAY;
>>>>
>>>> case MSR_EFER:
>>>> - *val = read_efer();
>>>> + /* Hide unknown bits, and unconditionally hide SVME from guests. */
>>>> + *val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
>>>> + /* Hide the 64-bit features from 32-bit guests. */
>>>> if ( is_pv_32bit_domain(currd) )
>>>> *val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
>>> Wouldn't it be better then to also move the LMSLE hiding up?
>> Actually, on second consideration, LMSLE shouldn't be hidden. If LMSLE
>> is actually active, then segment descriptors behave differently whether
>> the guest knows about it or not.
> Actually - the placement shouldn't matter at all: I think it can't be set anyway,
> as we would only possibly allow HVM guests to set it, i.e. the EFER value
> loaded during #VMEXIT won't ever have the bit set at present. And even the
> HVM side code uniformly forces cpu_has_lmsl to false right now.
On 3rd consideration, the current placement is correct. LMSLE, if set,
only has any effect on data segment when running with a long mode %cs.
Therefore, the clobber is in the right place. Were we ever to enable it
in Xen, this code would be correct.
On the HVM side, yes - we still have it disabled for migration reasons.
That said, I think we should consider dropping it entirely. Even AMD
think it is an unused feature.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-07 8:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-04 17:28 [PATCH for-4.11] x86/pv: Unconditionally hide EFER.SVME from PV guests Andrew Cooper
2018-05-04 18:45 ` Boris Ostrovsky
2018-05-04 18:48 ` Andrew Cooper
2018-05-07 6:28 ` Juergen Gross
2018-05-07 7:03 ` Jan Beulich
2018-05-07 7:30 ` Andrew Cooper
2018-05-07 8:00 ` Jan Beulich
2018-05-07 8:10 ` Andrew Cooper
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.