| Message ID | 20210407211954.32755-5-sidcha@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] KVM: x86: Move FPU register accessors into fpu.h | expand |
Siddharth Chandrasekaran <sidcha@amazon.de> writes: > Now that all extant hypercalls that can use XMM registers (based on > spec) for input/outputs are patched to support them, we can start > advertising this feature to guests. > > Cc: Alexander Graf <graf@amazon.com> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > --- > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > arch/x86/kvm/hyperv.c | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > index e6cd3fee562b..1f160ef60509 100644 > --- a/arch/x86/include/asm/hyperv-tlfs.h > +++ b/arch/x86/include/asm/hyperv-tlfs.h > @@ -49,10 +49,10 @@ > /* Support for physical CPU dynamic partitioning events is available*/ > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > /* > - * Support for passing hypercall input parameter block via XMM > + * Support for passing hypercall input and output parameter block via XMM > * registers is available > */ > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) TLFS 6.0b states that there are two distinct bits for input and output: CPUID Leaf 0x40000003.EDX: Bit 4: support for passing hypercall input via XMM registers is available. Bit 15: support for returning hypercall output via XMM registers is available. and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used anywhere, I'd suggest we just rename HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > /* Support for a virtual guest idle state is available */ > #define HV_X64_GUEST_IDLE_STATE_AVAILABLE BIT(5) > /* Frequency MSRs available */ > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index bf2f86f263f1..dd462c1d641d 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -2254,6 +2254,7 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, > ent->ebx |= HV_POST_MESSAGES; > ent->ebx |= HV_SIGNAL_EVENTS; > > + ent->edx |= HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE; > ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE; > ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: > Siddharth Chandrasekaran <sidcha@amazon.de> writes: > > > Now that all extant hypercalls that can use XMM registers (based on > > spec) for input/outputs are patched to support them, we can start > > advertising this feature to guests. > > > > Cc: Alexander Graf <graf@amazon.com> > > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > > --- > > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > > arch/x86/kvm/hyperv.c | 1 + > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > > index e6cd3fee562b..1f160ef60509 100644 > > --- a/arch/x86/include/asm/hyperv-tlfs.h > > +++ b/arch/x86/include/asm/hyperv-tlfs.h > > @@ -49,10 +49,10 @@ > > /* Support for physical CPU dynamic partitioning events is available*/ > > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > > /* > > - * Support for passing hypercall input parameter block via XMM > > + * Support for passing hypercall input and output parameter block via XMM > > * registers is available > > */ > > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) > > TLFS 6.0b states that there are two distinct bits for input and output: > > CPUID Leaf 0x40000003.EDX: > Bit 4: support for passing hypercall input via XMM registers is available. > Bit 15: support for returning hypercall output via XMM registers is available. > > and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used > anywhere, I'd suggest we just rename > > HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE > and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). That is how I had it initially; but then noticed that we would never need to use either of them separately. So it seemed like a reasonable abstraction to put them together. ~ Sid. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Siddharth Chandrasekaran <sidcha@amazon.de> writes: > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: >> >> > Now that all extant hypercalls that can use XMM registers (based on >> > spec) for input/outputs are patched to support them, we can start >> > advertising this feature to guests. >> > >> > Cc: Alexander Graf <graf@amazon.com> >> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> >> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> >> > --- >> > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- >> > arch/x86/kvm/hyperv.c | 1 + >> > 2 files changed, 3 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h >> > index e6cd3fee562b..1f160ef60509 100644 >> > --- a/arch/x86/include/asm/hyperv-tlfs.h >> > +++ b/arch/x86/include/asm/hyperv-tlfs.h >> > @@ -49,10 +49,10 @@ >> > /* Support for physical CPU dynamic partitioning events is available*/ >> > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) >> > /* >> > - * Support for passing hypercall input parameter block via XMM >> > + * Support for passing hypercall input and output parameter block via XMM >> > * registers is available >> > */ >> > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) >> > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) >> >> TLFS 6.0b states that there are two distinct bits for input and output: >> >> CPUID Leaf 0x40000003.EDX: >> Bit 4: support for passing hypercall input via XMM registers is available. >> Bit 15: support for returning hypercall output via XMM registers is available. >> >> and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used >> anywhere, I'd suggest we just rename >> >> HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE >> and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > > That is how I had it initially; but then noticed that we would never > need to use either of them separately. So it seemed like a reasonable > abstraction to put them together. > Actually, we may. In theory, KVM userspace may decide to expose just one of these two to the guest as it is not obliged to copy everything from KVM_GET_SUPPORTED_HV_CPUID so we will need separate guest_cpuid_has() checks. (This reminds me of something I didn't see in your series: we need to check that XMM hypercall parameters support was actually exposed to the guest as it is illegal for a guest to use it otherwise -- and we will likely need two checks, for input and output). Also, (and that's what triggered my comment) all other HV_ACCESS_* in kvm_get_hv_cpuid() are single bits so my first impression was that you forgot one bit, but then I saw that you combined them together.
On Thu, Apr 08, 2021 at 04:20:54PM +0200, Siddharth Chandrasekaran wrote: > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: > > Siddharth Chandrasekaran <sidcha@amazon.de> writes: > > > > > Now that all extant hypercalls that can use XMM registers (based on > > > spec) for input/outputs are patched to support them, we can start > > > advertising this feature to guests. > > > > > > Cc: Alexander Graf <graf@amazon.com> > > > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > > > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > > > --- > > > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > > > arch/x86/kvm/hyperv.c | 1 + > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > > > index e6cd3fee562b..1f160ef60509 100644 > > > --- a/arch/x86/include/asm/hyperv-tlfs.h > > > +++ b/arch/x86/include/asm/hyperv-tlfs.h > > > @@ -49,10 +49,10 @@ > > > /* Support for physical CPU dynamic partitioning events is available*/ > > > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > > > /* > > > - * Support for passing hypercall input parameter block via XMM > > > + * Support for passing hypercall input and output parameter block via XMM > > > * registers is available > > > */ > > > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > > > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) > > > > TLFS 6.0b states that there are two distinct bits for input and output: > > > > CPUID Leaf 0x40000003.EDX: > > Bit 4: support for passing hypercall input via XMM registers is available. > > Bit 15: support for returning hypercall output via XMM registers is available. > > > > and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used > > anywhere, I'd suggest we just rename > > > > HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE > > and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > > That is how I had it initially; but then noticed that we would never > need to use either of them separately. So it seemed like a reasonable > abstraction to put them together. > They are two separate things in TLFS. Please use two macros here. Wei.
On Thu, Apr 08, 2021 at 04:44:23PM +0200, Vitaly Kuznetsov wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > Siddharth Chandrasekaran <sidcha@amazon.de> writes: > > > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: > >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: > >> > >> > Now that all extant hypercalls that can use XMM registers (based on > >> > spec) for input/outputs are patched to support them, we can start > >> > advertising this feature to guests. > >> > > >> > Cc: Alexander Graf <graf@amazon.com> > >> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > >> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > >> > --- > >> > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > >> > arch/x86/kvm/hyperv.c | 1 + > >> > 2 files changed, 3 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > >> > index e6cd3fee562b..1f160ef60509 100644 > >> > --- a/arch/x86/include/asm/hyperv-tlfs.h > >> > +++ b/arch/x86/include/asm/hyperv-tlfs.h > >> > @@ -49,10 +49,10 @@ > >> > /* Support for physical CPU dynamic partitioning events is available*/ > >> > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > >> > /* > >> > - * Support for passing hypercall input parameter block via XMM > >> > + * Support for passing hypercall input and output parameter block via XMM > >> > * registers is available > >> > */ > >> > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > >> > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) > >> > >> TLFS 6.0b states that there are two distinct bits for input and output: > >> > >> CPUID Leaf 0x40000003.EDX: > >> Bit 4: support for passing hypercall input via XMM registers is available. > >> Bit 15: support for returning hypercall output via XMM registers is available. > >> > >> and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used > >> anywhere, I'd suggest we just rename > >> > >> HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE > >> and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > > > > That is how I had it initially; but then noticed that we would never > > need to use either of them separately. So it seemed like a reasonable > > abstraction to put them together. > > > > Actually, we may. In theory, KVM userspace may decide to expose just > one of these two to the guest as it is not obliged to copy everything > from KVM_GET_SUPPORTED_HV_CPUID so we will need separate > guest_cpuid_has() checks. Makes sense. I'll split them and add the checks. > (This reminds me of something I didn't see in your series: > we need to check that XMM hypercall parameters support was actually > exposed to the guest as it is illegal for a guest to use it otherwise -- > and we will likely need two checks, for input and output). We observed that Windows expects Hyper-V to support XMM params even if we don't advertise this feature but if userspace wants to hide this feature and the guest does it anyway, then it makes sense to treat it as an illegal OP. > Also, (and that's what triggered my comment) all other HV_ACCESS_* in > kvm_get_hv_cpuid() are single bits so my first impression was that you > forgot one bit, but then I saw that you combined them together. ~ Sid. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Thu, Apr 08, 2021 at 03:44:46PM +0000, Wei Liu wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > On Thu, Apr 08, 2021 at 04:20:54PM +0200, Siddharth Chandrasekaran wrote: > > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: > > > Siddharth Chandrasekaran <sidcha@amazon.de> writes: > > > > > > > Now that all extant hypercalls that can use XMM registers (based on > > > > spec) for input/outputs are patched to support them, we can start > > > > advertising this feature to guests. > > > > > > > > Cc: Alexander Graf <graf@amazon.com> > > > > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > > > > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > > > > --- > > > > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > > > > arch/x86/kvm/hyperv.c | 1 + > > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > > > > index e6cd3fee562b..1f160ef60509 100644 > > > > --- a/arch/x86/include/asm/hyperv-tlfs.h > > > > +++ b/arch/x86/include/asm/hyperv-tlfs.h > > > > @@ -49,10 +49,10 @@ > > > > /* Support for physical CPU dynamic partitioning events is available*/ > > > > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > > > > /* > > > > - * Support for passing hypercall input parameter block via XMM > > > > + * Support for passing hypercall input and output parameter block via XMM > > > > * registers is available > > > > */ > > > > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > > > > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) > > > > > > TLFS 6.0b states that there are two distinct bits for input and output: > > > > > > CPUID Leaf 0x40000003.EDX: > > > Bit 4: support for passing hypercall input via XMM registers is available. > > > Bit 15: support for returning hypercall output via XMM registers is available. > > > > > > and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used > > > anywhere, I'd suggest we just rename > > > > > > HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE > > > and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > > > > That is how I had it initially; but then noticed that we would never > > need to use either of them separately. So it seemed like a reasonable > > abstraction to put them together. > > > > They are two separate things in TLFS. Please use two macros here. Ack, will split them. ~ Sid. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Siddharth Chandrasekaran <sidcha@amazon.de> writes: > On Thu, Apr 08, 2021 at 04:44:23PM +0200, Vitaly Kuznetsov wrote: >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. >> >> >> >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: >> >> > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: >> >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: >> >> >> >> > Now that all extant hypercalls that can use XMM registers (based on >> >> > spec) for input/outputs are patched to support them, we can start >> >> > advertising this feature to guests. >> >> > >> >> > Cc: Alexander Graf <graf@amazon.com> >> >> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> >> >> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> >> >> > --- >> >> > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- >> >> > arch/x86/kvm/hyperv.c | 1 + >> >> > 2 files changed, 3 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h >> >> > index e6cd3fee562b..1f160ef60509 100644 >> >> > --- a/arch/x86/include/asm/hyperv-tlfs.h >> >> > +++ b/arch/x86/include/asm/hyperv-tlfs.h >> >> > @@ -49,10 +49,10 @@ >> >> > /* Support for physical CPU dynamic partitioning events is available*/ >> >> > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) >> >> > /* >> >> > - * Support for passing hypercall input parameter block via XMM >> >> > + * Support for passing hypercall input and output parameter block via XMM >> >> > * registers is available >> >> > */ >> >> > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) >> >> > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) >> >> >> >> TLFS 6.0b states that there are two distinct bits for input and output: >> >> >> >> CPUID Leaf 0x40000003.EDX: >> >> Bit 4: support for passing hypercall input via XMM registers is available. >> >> Bit 15: support for returning hypercall output via XMM registers is available. >> >> >> >> and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used >> >> anywhere, I'd suggest we just rename >> >> >> >> HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE >> >> and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). >> > >> > That is how I had it initially; but then noticed that we would never >> > need to use either of them separately. So it seemed like a reasonable >> > abstraction to put them together. >> > >> >> Actually, we may. In theory, KVM userspace may decide to expose just >> one of these two to the guest as it is not obliged to copy everything >> from KVM_GET_SUPPORTED_HV_CPUID so we will need separate >> guest_cpuid_has() checks. > > Makes sense. I'll split them and add the checks. > >> (This reminds me of something I didn't see in your series: >> we need to check that XMM hypercall parameters support was actually >> exposed to the guest as it is illegal for a guest to use it otherwise -- >> and we will likely need two checks, for input and output). > > We observed that Windows expects Hyper-V to support XMM params even if > we don't advertise this feature but if userspace wants to hide this > feature and the guest does it anyway, then it makes sense to treat it as > an illegal OP. > Out of pure curiosity, which Windows version behaves like that? And how does this work with KVM without your patches? Sane KVM userspaces will certainly expose both XMM input and output capabilities together but having an ability to hide one or both of them may come handy while debugging. Also, we weren't enforcing the rule that enlightenments not exposed to the guest don't work, even the whole Hyper-V emulation interface was available to all guests who were smart enough to know how to enable it! I don't like this for two reasons: security (large attack surface) and the fact that someone 'smart' may decide to use Hyper-V emulation features on KVM as 'general purpose' features saying 'they're always available anyway', this risks becoming an ABI. Let's at least properly check if the feature was exposed to the guest for all new enlightenments.
On Fri, Apr 09, 2021 at 09:38:03AM +0200, Vitaly Kuznetsov wrote: > Siddharth Chandrasekaran <sidcha@amazon.de> writes: > > On Thu, Apr 08, 2021 at 04:44:23PM +0200, Vitaly Kuznetsov wrote: > >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: > >> > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: > >> >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: > >> >> > >> >> > Now that all extant hypercalls that can use XMM registers (based on > >> >> > spec) for input/outputs are patched to support them, we can start > >> >> > advertising this feature to guests. > >> >> > > >> >> > Cc: Alexander Graf <graf@amazon.com> > >> >> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > >> >> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > >> >> > --- > >> >> > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > >> >> > arch/x86/kvm/hyperv.c | 1 + > >> >> > 2 files changed, 3 insertions(+), 2 deletions(-) > >> >> > > >> >> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > >> >> > index e6cd3fee562b..1f160ef60509 100644 > >> >> > --- a/arch/x86/include/asm/hyperv-tlfs.h > >> >> > +++ b/arch/x86/include/asm/hyperv-tlfs.h > >> >> > @@ -49,10 +49,10 @@ > >> >> > /* Support for physical CPU dynamic partitioning events is available*/ > >> >> > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > >> >> > /* > >> >> > - * Support for passing hypercall input parameter block via XMM > >> >> > + * Support for passing hypercall input and output parameter block via XMM > >> >> > * registers is available > >> >> > */ > >> >> > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > >> >> > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) > >> >> > >> >> TLFS 6.0b states that there are two distinct bits for input and output: > >> >> > >> >> CPUID Leaf 0x40000003.EDX: > >> >> Bit 4: support for passing hypercall input via XMM registers is available. > >> >> Bit 15: support for returning hypercall output via XMM registers is available. > >> >> > >> >> and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used > >> >> anywhere, I'd suggest we just rename > >> >> > >> >> HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE > >> >> and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > >> > > >> > That is how I had it initially; but then noticed that we would never > >> > need to use either of them separately. So it seemed like a reasonable > >> > abstraction to put them together. > >> > > >> > >> Actually, we may. In theory, KVM userspace may decide to expose just > >> one of these two to the guest as it is not obliged to copy everything > >> from KVM_GET_SUPPORTED_HV_CPUID so we will need separate > >> guest_cpuid_has() checks. > > > > Makes sense. I'll split them and add the checks. > > > >> (This reminds me of something I didn't see in your series: > >> we need to check that XMM hypercall parameters support was actually > >> exposed to the guest as it is illegal for a guest to use it otherwise -- > >> and we will likely need two checks, for input and output). > > > > We observed that Windows expects Hyper-V to support XMM params even if > > we don't advertise this feature but if userspace wants to hide this > > feature and the guest does it anyway, then it makes sense to treat it as > > an illegal OP. > > > > Out of pure curiosity, which Windows version behaves like that? And how > does this work with KVM without your patches? The guest is a Windows Server 2016 on which we are trying to enable VBS and handle it through the VSM API. When VBS is enabled on the guest, it starts using many other (new) hypercalls and some of them don't honor the CPUID bits (4, 15) that indicate the presence of XMM params support. > Sane KVM userspaces will certainly expose both XMM input and output > capabilities together but having an ability to hide one or both of them > may come handy while debugging. > > Also, we weren't enforcing the rule that enlightenments not exposed to > the guest don't work, even the whole Hyper-V emulation interface was > available to all guests who were smart enough to know how to enable it! > I don't like this for two reasons: security (large attack surface) and > the fact that someone 'smart' may decide to use Hyper-V emulation > features on KVM as 'general purpose' features saying 'they're always > available anyway', this risks becoming an ABI. > > Let's at least properly check if the feature was exposed to the guest > for all new enlightenments. Agreed. ~ Sid. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Thu, Apr 08, 2021 at 04:44:23PM +0200, Vitaly Kuznetsov wrote: > Siddharth Chandrasekaran <sidcha@amazon.de> writes: > > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: > >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: > >> > Now that all extant hypercalls that can use XMM registers (based on > >> > spec) for input/outputs are patched to support them, we can start > >> > advertising this feature to guests. > >> > > >> > Cc: Alexander Graf <graf@amazon.com> > >> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> > >> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > >> > --- > >> > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- > >> > arch/x86/kvm/hyperv.c | 1 + > >> > 2 files changed, 3 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h > >> > index e6cd3fee562b..1f160ef60509 100644 > >> > --- a/arch/x86/include/asm/hyperv-tlfs.h > >> > +++ b/arch/x86/include/asm/hyperv-tlfs.h > >> > @@ -49,10 +49,10 @@ > >> > /* Support for physical CPU dynamic partitioning events is available*/ > >> > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) > >> > /* > >> > - * Support for passing hypercall input parameter block via XMM > >> > + * Support for passing hypercall input and output parameter block via XMM > >> > * registers is available > >> > */ > >> > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) > >> > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) > >> > >> TLFS 6.0b states that there are two distinct bits for input and output: > >> > >> CPUID Leaf 0x40000003.EDX: > >> Bit 4: support for passing hypercall input via XMM registers is available. > >> Bit 15: support for returning hypercall output via XMM registers is available. > >> > >> and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used > >> anywhere, I'd suggest we just rename > >> > >> HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE > >> and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). > > > > That is how I had it initially; but then noticed that we would never > > need to use either of them separately. So it seemed like a reasonable > > abstraction to put them together. > > > > Actually, we may. In theory, KVM userspace may decide to expose just > one of these two to the guest as it is not obliged to copy everything > from KVM_GET_SUPPORTED_HV_CPUID so we will need separate > guest_cpuid_has() checks. Looks like guest_cpuid_has() check is for x86 CPU features only (if I'm not mistaken) and I don't see a suitable alternative that looks into vcpu->arch.cpuid_entries[]. So I plan to add a new method hv_guest_cpuid_has() in hyperv.c to have this check; does that sound right to you? If you can give a quick go-ahead, I'll make the changes requested so far and send v2 this series. Thanks. ~ Sid. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Siddharth Chandrasekaran <sidcha@amazon.de> writes: > On Thu, Apr 08, 2021 at 04:44:23PM +0200, Vitaly Kuznetsov wrote: >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: >> > On Thu, Apr 08, 2021 at 02:05:53PM +0200, Vitaly Kuznetsov wrote: >> >> Siddharth Chandrasekaran <sidcha@amazon.de> writes: >> >> > Now that all extant hypercalls that can use XMM registers (based on >> >> > spec) for input/outputs are patched to support them, we can start >> >> > advertising this feature to guests. >> >> > >> >> > Cc: Alexander Graf <graf@amazon.com> >> >> > Cc: Evgeny Iakovlev <eyakovl@amazon.de> >> >> > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> >> >> > --- >> >> > arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- >> >> > arch/x86/kvm/hyperv.c | 1 + >> >> > 2 files changed, 3 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h >> >> > index e6cd3fee562b..1f160ef60509 100644 >> >> > --- a/arch/x86/include/asm/hyperv-tlfs.h >> >> > +++ b/arch/x86/include/asm/hyperv-tlfs.h >> >> > @@ -49,10 +49,10 @@ >> >> > /* Support for physical CPU dynamic partitioning events is available*/ >> >> > #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) >> >> > /* >> >> > - * Support for passing hypercall input parameter block via XMM >> >> > + * Support for passing hypercall input and output parameter block via XMM >> >> > * registers is available >> >> > */ >> >> > -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) >> >> > +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) >> >> >> >> TLFS 6.0b states that there are two distinct bits for input and output: >> >> >> >> CPUID Leaf 0x40000003.EDX: >> >> Bit 4: support for passing hypercall input via XMM registers is available. >> >> Bit 15: support for returning hypercall output via XMM registers is available. >> >> >> >> and HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE is not currently used >> >> anywhere, I'd suggest we just rename >> >> >> >> HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE to HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE >> >> and add HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (bit 15). >> > >> > That is how I had it initially; but then noticed that we would never >> > need to use either of them separately. So it seemed like a reasonable >> > abstraction to put them together. >> > >> >> Actually, we may. In theory, KVM userspace may decide to expose just >> one of these two to the guest as it is not obliged to copy everything >> from KVM_GET_SUPPORTED_HV_CPUID so we will need separate >> guest_cpuid_has() checks. > > Looks like guest_cpuid_has() check is for x86 CPU features only (if I'm > not mistaken) and I don't see a suitable alternative that looks into > vcpu->arch.cpuid_entries[]. So I plan to add a new method > hv_guest_cpuid_has() in hyperv.c to have this check; does that sound > right to you? > If you can give a quick go-ahead, I'll make the changes requested so > far and send v2 this series. Sorry my mistake, guest_cpuid_has() was the wrong function to name. In the meantime I started working on fine-grained access to the existing Hyper-V enlightenments as well and I think the best approach would be to cache CPUID 0x40000003 (EAX, EBX, EDX) in kvm_hv_set_cpuid() to avoid looping through all guest CPUID entries on every hypercall. Your check will then look like if (hv_vcpu->cpuid_cache.features_edx & HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE) ... if (hv_vcpu->cpuid_cache.features_edx & HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE) ... We can wrap this into a hv_guest_cpuid_has() helper indeed, it'll look like: if (hv_guest_cpuid_has(vcpu, HYPERV_CPUID_FEATURES, CPUID_EDX, HV_X64_HYPERCALL_XMM_INPUT_AVAILABLE)) ... but I'm not sure it's worth it, maybe raw check is shorter and better. I plan to send something out in a day or two, I'll Cc: you. Feel free to do v2 without this, if your series gets merged first I can just add the 'fine-grained access' to mine. Thanks!
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h index e6cd3fee562b..1f160ef60509 100644 --- a/arch/x86/include/asm/hyperv-tlfs.h +++ b/arch/x86/include/asm/hyperv-tlfs.h @@ -49,10 +49,10 @@ /* Support for physical CPU dynamic partitioning events is available*/ #define HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE BIT(3) /* - * Support for passing hypercall input parameter block via XMM + * Support for passing hypercall input and output parameter block via XMM * registers is available */ -#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) +#define HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE BIT(4) | BIT(15) /* Support for a virtual guest idle state is available */ #define HV_X64_GUEST_IDLE_STATE_AVAILABLE BIT(5) /* Frequency MSRs available */ diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index bf2f86f263f1..dd462c1d641d 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -2254,6 +2254,7 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, ent->ebx |= HV_POST_MESSAGES; ent->ebx |= HV_SIGNAL_EVENTS; + ent->edx |= HV_X64_HYPERCALL_PARAMS_XMM_AVAILABLE; ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE; ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE;
Now that all extant hypercalls that can use XMM registers (based on spec) for input/outputs are patched to support them, we can start advertising this feature to guests. Cc: Alexander Graf <graf@amazon.com> Cc: Evgeny Iakovlev <eyakovl@amazon.de> Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> --- arch/x86/include/asm/hyperv-tlfs.h | 4 ++-- arch/x86/kvm/hyperv.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)