xen/arch/x86/hvm/svm/svm.c | 5 ++--- xen/arch/x86/hvm/svm/vmcb.c | 3 ++- xen/include/asm-x86/hvm/svm/vmcb.h | 4 +++- 3 files changed, 7 insertions(+), 5 deletions(-)
The RDPRU instruction isn't supported at all (and it is unclear how this can
ever be offered safely to guests). However, a guest which ignores CPUID and
blindly executes RDPRU will find that it functions.
Use the intercept and terminate with #UD. While at it, fold SKINIT into the
same "unconditionally disabled" path.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
I could have sworn that I'd posted this before, but I can't locate any
evidence of it. I've got a separate patch adding the CPUID infrastructure for
rdpru, but that is better left until we've got more libx86 levelling logic in
place.
---
xen/arch/x86/hvm/svm/svm.c | 5 ++---
xen/arch/x86/hvm/svm/vmcb.c | 3 ++-
xen/include/asm-x86/hvm/svm/vmcb.h | 4 +++-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8dc92c8b9f96..18c4831f98ad 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2945,6 +2945,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
case VMEXIT_MONITOR:
case VMEXIT_MWAIT:
+ case VMEXIT_SKINIT:
+ case VMEXIT_RDPRU:
hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
break;
@@ -2963,9 +2965,6 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
case VMEXIT_CLGI:
svm_vmexit_do_clgi(regs, v);
break;
- case VMEXIT_SKINIT:
- hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
- break;
case VMEXIT_XSETBV:
if ( vmcb_get_cpl(vmcb) )
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 55da9302e5d7..565e997155f2 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI |
GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT |
GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR |
- GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP;
+ GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP |
+ GENERAL2_INTERCEPT_RDPRU;
/* Intercept all debug-register writes. */
vmcb->_dr_intercepts = ~0u;
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index 4fa2ddfb2ff2..ed7cebea7174 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -74,7 +74,8 @@ enum GenericIntercept2bits
GENERAL2_INTERCEPT_MONITOR = 1 << 10,
GENERAL2_INTERCEPT_MWAIT = 1 << 11,
GENERAL2_INTERCEPT_MWAIT_CONDITIONAL = 1 << 12,
- GENERAL2_INTERCEPT_XSETBV = 1 << 13
+ GENERAL2_INTERCEPT_XSETBV = 1 << 13,
+ GENERAL2_INTERCEPT_RDPRU = 1 << 14,
};
@@ -300,6 +301,7 @@ enum VMEXIT_EXITCODE
VMEXIT_MWAIT = 139, /* 0x8b */
VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */
VMEXIT_XSETBV = 141, /* 0x8d */
+ VMEXIT_RDPRU = 142, /* 0x8e */
VMEXIT_NPF = 1024, /* 0x400, nested paging fault */
VMEXIT_INVALID = -1
};
--
2.11.0
On 08.09.2021 18:19, Andrew Cooper wrote: > The RDPRU instruction isn't supported at all (and it is unclear how this can > ever be offered safely to guests). An implicit hint to me to consider "x86emul: support RDPRU" rejected? That's still in my queue waiting for ... > However, a guest which ignores CPUID and > blindly executes RDPRU will find that it functions. > > Use the intercept and terminate with #UD. While at it, fold SKINIT into the > same "unconditionally disabled" path. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > I could have sworn that I'd posted this before, but I can't locate any > evidence of it. I've got a separate patch adding the CPUID infrastructure for > rdpru, but that is better left until we've got more libx86 levelling logic in > place. ... this. Which - if exposure to guests makes no sense - would seem pretty pointless then as well? > --- a/xen/arch/x86/hvm/svm/vmcb.c > +++ b/xen/arch/x86/hvm/svm/vmcb.c > @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v) > GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI | > GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT | > GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR | > - GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP; > + GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP | > + GENERAL2_INTERCEPT_RDPRU; Some of the other intercepts here suggest it is okay to enable ones in the absence of support in the underlying hardware, but I thought I'd double check. I couldn't find any statement either way in the PM. Assuming this is fine Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 09/09/2021 10:57, Jan Beulich wrote: > On 08.09.2021 18:19, Andrew Cooper wrote: >> The RDPRU instruction isn't supported at all (and it is unclear how this can >> ever be offered safely to guests). > An implicit hint to me to consider "x86emul: support RDPRU" rejected? That's > still in my queue waiting for ... > >> However, a guest which ignores CPUID and >> blindly executes RDPRU will find that it functions. >> >> Use the intercept and terminate with #UD. While at it, fold SKINIT into the >> same "unconditionally disabled" path. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> >> I could have sworn that I'd posted this before, but I can't locate any >> evidence of it. I've got a separate patch adding the CPUID infrastructure for >> rdpru, but that is better left until we've got more libx86 levelling logic in >> place. > ... this. Which - if exposure to guests makes no sense - would seem pretty > pointless then as well? Well - given how many people want aperf/mperf, I suspect we might end up having it as an opt-in only thing, so no - this isn't a rejection of the x86emul work. We can *almost* provide a safe way for VM's to use this infrastructure if we had something like Linux's restartable sequences infrastructure. In that case, only an SMI would mess things up, from the guest kernel's perspective. On the other hand, it's probably easier to lobby Intel and AMD for an interface here which isn't fundamentally broken in the face of NMI/SMI/etc. > >> --- a/xen/arch/x86/hvm/svm/vmcb.c >> +++ b/xen/arch/x86/hvm/svm/vmcb.c >> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v) >> GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI | >> GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT | >> GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR | >> - GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP; >> + GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP | >> + GENERAL2_INTERCEPT_RDPRU; > Some of the other intercepts here suggest it is okay to enable ones > in the absence of support in the underlying hardware, but I thought > I'd double check. I couldn't find any statement either way in the PM. > Assuming this is fine They're just bits in memory. Older CPUs will ignore them, and indeed - pre-RDPRU hardware is fine running with this intercept bit set. Honestly, I've got half a mind to default the intercepts to ~0 rather than 0. For running older Xen on new hardware, it will lead to fewer unexpected surprises. > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
On 09.09.2021 13:34, Andrew Cooper wrote:
> On 09/09/2021 10:57, Jan Beulich wrote:
>> On 08.09.2021 18:19, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>>> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
>>> GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI |
>>> GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT |
>>> GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR |
>>> - GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP;
>>> + GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP |
>>> + GENERAL2_INTERCEPT_RDPRU;
>> Some of the other intercepts here suggest it is okay to enable ones
>> in the absence of support in the underlying hardware, but I thought
>> I'd double check. I couldn't find any statement either way in the PM.
>> Assuming this is fine
>
> They're just bits in memory. Older CPUs will ignore them, and indeed -
> pre-RDPRU hardware is fine running with this intercept bit set.
>
> Honestly, I've got half a mind to default the intercepts to ~0 rather
> than 0. For running older Xen on new hardware, it will lead to fewer
> unexpected surprises.
I, too, was considering this, but then we have
default:
unexpected_exit_type:
gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", "
"exitinfo1 %#"PRIx64", exitinfo2 %#"PRIx64"\n",
exit_reason, vmcb->exitinfo1, vmcb->exitinfo2);
crash_or_fault:
svm_crash_or_fault(v);
break;
}
at the bottom of the switch() statement handling the exit codes.
Getting crashed (or crashing because of an unexpected fault) is
surely a surprise as well (to a guest).
Jan
On 09/09/2021 12:47, Jan Beulich wrote: > On 09.09.2021 13:34, Andrew Cooper wrote: >> On 09/09/2021 10:57, Jan Beulich wrote: >>> On 08.09.2021 18:19, Andrew Cooper wrote: >>>> --- a/xen/arch/x86/hvm/svm/vmcb.c >>>> +++ b/xen/arch/x86/hvm/svm/vmcb.c >>>> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v) >>>> GENERAL2_INTERCEPT_STGI | GENERAL2_INTERCEPT_CLGI | >>>> GENERAL2_INTERCEPT_SKINIT | GENERAL2_INTERCEPT_MWAIT | >>>> GENERAL2_INTERCEPT_WBINVD | GENERAL2_INTERCEPT_MONITOR | >>>> - GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP; >>>> + GENERAL2_INTERCEPT_XSETBV | GENERAL2_INTERCEPT_ICEBP | >>>> + GENERAL2_INTERCEPT_RDPRU; >>> Some of the other intercepts here suggest it is okay to enable ones >>> in the absence of support in the underlying hardware, but I thought >>> I'd double check. I couldn't find any statement either way in the PM. >>> Assuming this is fine >> They're just bits in memory. Older CPUs will ignore them, and indeed - >> pre-RDPRU hardware is fine running with this intercept bit set. >> >> Honestly, I've got half a mind to default the intercepts to ~0 rather >> than 0. For running older Xen on new hardware, it will lead to fewer >> unexpected surprises. > I, too, was considering this, but then we have > > default: > unexpected_exit_type: > gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", " > "exitinfo1 %#"PRIx64", exitinfo2 %#"PRIx64"\n", > exit_reason, vmcb->exitinfo1, vmcb->exitinfo2); > crash_or_fault: > svm_crash_or_fault(v); > break; > } > > at the bottom of the switch() statement handling the exit codes. > Getting crashed (or crashing because of an unexpected fault) is > surely a surprise as well (to a guest). It is less bad than the alternative, which was failing to intercept e.g. XSETBV. Something's going to crash or malfunction either way. Intercepting everything makes it far more obvious, and limits the damage to only the offending guest. And yes - this is once again why we really need a credible support statement for CPUs. ~Andrew
© 2016 - 2024 Red Hat, Inc.