Re: [PATCH v7 8/9] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges

[Eduardo Habkost]

On Fri, Jun 04, 2021 at 09:35:27AM +0200, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Jun 03, 2021 at 01:48:34PM +0200, Vitaly Kuznetsov wrote:
> >> When Hyper-V SynIC is enabled, we may need to allow Windows guests to make
> >> hypercalls (POST_MESSAGES/SIGNAL_EVENTS). No issue is currently observed
> >> because KVM is very permissive, allowing these hypercalls regarding of
> >> guest visible CPUid bits.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  target/i386/kvm/hyperv-proto.h | 6 ++++++
> >>  target/i386/kvm/kvm.c          | 6 ++++++
> >>  2 files changed, 12 insertions(+)
> >> 
> >> diff --git a/target/i386/kvm/hyperv-proto.h 
> >> b/target/i386/kvm/hyperv-proto.h
> >> index e30d64b4ade4..5fbb385cc136 100644
> >> --- a/target/i386/kvm/hyperv-proto.h
> >> +++ b/target/i386/kvm/hyperv-proto.h
> >> @@ -38,6 +38,12 @@
> >>  #define HV_ACCESS_FREQUENCY_MSRS     (1u << 11)
> >>  #define HV_ACCESS_REENLIGHTENMENTS_CONTROL  (1u << 13)
> >>  
> >> +/*
> >> + * HV_CPUID_FEATURES.EBX bits
> >> + */
> >> +#define HV_POST_MESSAGES             (1u << 4)
> >> +#define HV_SIGNAL_EVENTS             (1u << 5)
> >> +
> >>  /*
> >>   * HV_CPUID_FEATURES.EDX bits
> >>   */
> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >> index a3897d4d8788..6a32d43e6ec1 100644
> >> --- a/target/i386/kvm/kvm.c
> >> +++ b/target/i386/kvm/kvm.c
> >> @@ -1343,6 +1343,12 @@ static int hyperv_fill_cpuids(CPUState *cs,
> >>      /* Unconditionally required with any Hyper-V enlightenment */
> >>      c->eax |= HV_HYPERCALL_AVAILABLE;
> >>  
> >> +    /* SynIC and Vmbus devices require messages/signals hypercalls */
> >> +    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) &&
> >> +        !cpu->hyperv_synic_kvm_only) {
> >> +        c->ebx |= HV_POST_MESSAGES | HV_SIGNAL_EVENTS;
> >
> > Why exactly is the hyperv_synic_kvm_only check needed?
> >
> 
> 'hyperv_synic_kvm_only' means SynIC is only used for in-KVM stimers and
> in this case Post Messages / Signal Events hypercalls are not used. KVM
> will also return an error to the guest directly (as it can't forward
> them to QEMU). No need to expose HV_POST_MESSAGES | HV_SIGNAL_EVENTS.
> 
> > Is the hyperv_synic_kvm_only check the only reason this was done
> > here and not at kvm_hyperv_properties?
> >
> 
> Yes, basically.

Thanks for the clarification!

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo