Re: [Qemu-devel] [PATCH 05/10] hyperv: add synic message delivery

Re: [Qemu-devel] [PATCH 05/10] hyperv: add synic message delivery

[Paolo Bonzini]

On 21/09/2018 10:22, Roman Kagan wrote:
> +typedef struct HvSintStagedMesage {
> +    /* message content staged by hyperv_post_msg */
> +    struct hyperv_message msg;
> +    /* callback + data (r/o) to complete the processing in a BH */
> +    HvSintMsgCb cb;
> +    void *cb_data;
> +    /* message posting status filled by cpu_post_msg */
> +    int status;
> +    /* passing the buck: */
> +    enum {
> +        /* initial state */
> +        HV_STAGED_MSG_FREE,
> +        /*
> +         * hyperv_post_msg (e.g. in main loop) grabs the staged area (FREE ->
> +         * BUSY), copies msg, and schedules cpu_post_msg on the assigned cpu
> +         */
> +        HV_STAGED_MSG_BUSY,
> +        /*
> +         * cpu_post_msg (vcpu thread) tries to copy staged msg to msg slot,
> +         * notify the guest, records the status, marks the posting done (BUSY
> +         * -> POSTED), and schedules sint_msg_bh BH
> +         */
> +        HV_STAGED_MSG_POSTED,
> +        /*
> +         * sint_msg_bh (BH) verifies that the posting is done, runs the
> +         * callback, and starts over (POSTED -> FREE)
> +         */
> +    } state;
> +} HvSintStagedMesage;

s/Mesage/Message/

> +    if (atomic_read(&staged_msg->state) != HV_STAGED_MSG_POSTED) {
> +        /* status nor ready yet (spurious ack from guest?), ignore */
> +        return;
> +    }
> +

Can this actually happen?  It seems scary...

Paolo

Re: [Qemu-devel] [PATCH 05/10] hyperv: add synic message delivery

[Roman Kagan]

On Wed, Oct 03, 2018 at 01:06:58PM +0200, Paolo Bonzini wrote:
> On 21/09/2018 10:22, Roman Kagan wrote:
> > +typedef struct HvSintStagedMesage {
> > +    /* message content staged by hyperv_post_msg */
> > +    struct hyperv_message msg;
> > +    /* callback + data (r/o) to complete the processing in a BH */
> > +    HvSintMsgCb cb;
> > +    void *cb_data;
> > +    /* message posting status filled by cpu_post_msg */
> > +    int status;
> > +    /* passing the buck: */
> > +    enum {
> > +        /* initial state */
> > +        HV_STAGED_MSG_FREE,
> > +        /*
> > +         * hyperv_post_msg (e.g. in main loop) grabs the staged area (FREE ->
> > +         * BUSY), copies msg, and schedules cpu_post_msg on the assigned cpu
> > +         */
> > +        HV_STAGED_MSG_BUSY,
> > +        /*
> > +         * cpu_post_msg (vcpu thread) tries to copy staged msg to msg slot,
> > +         * notify the guest, records the status, marks the posting done (BUSY
> > +         * -> POSTED), and schedules sint_msg_bh BH
> > +         */
> > +        HV_STAGED_MSG_POSTED,
> > +        /*
> > +         * sint_msg_bh (BH) verifies that the posting is done, runs the
> > +         * callback, and starts over (POSTED -> FREE)
> > +         */
> > +    } state;
> > +} HvSintStagedMesage;
> 
> s/Mesage/Message/

:facepalm:

> > +    if (atomic_read(&staged_msg->state) != HV_STAGED_MSG_POSTED) {
> > +        /* status nor ready yet (spurious ack from guest?), ignore */
> > +        return;
> > +    }
> > +
> 
> Can this actually happen?  It seems scary...

I don't see off-hand what can stop the guest from wrmsr HV_X64_MSR_EOM
at an arbitrary moment, which AFAICS will result in this eventfd being
signaled.  I haven't seen this codepath being taken in real life but
I think it provides enough protection for the internal state from being
screwed up by such an unexpected signal.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 00/10] hyperv: add connection infrastructure

[Paolo Bonzini]

On 21/09/2018 10:22, Roman Kagan wrote:
> This series introduces the infrastructure to send and receive Hyper-V
> messages and events.
> 
> More specifically,
> 
> - SynIC is turned into a full-fledged device managing the memory regions
>   used for QEMU->guest communication
> - machinery is introduced to post messages and signal events to the
>   guest
> - infrastructure is added to subscribe to messages and events from the
>   guest, and to dispatch the received messages and events to the
>   subscribers
> 
> Based-on: 20180921082041.29380-1-rkagan@virtuozzo.com
> 
> Roman Kagan (10):
>   hyperv:synic: split capability testing and setting
>   hyperv: qom-ify SynIC
>   hyperv: only add SynIC in compatible configurations
>   hyperv: make overlay pages for SynIC
>   hyperv: add synic message delivery
>   hyperv: add synic event flag signaling
>   hyperv: process SIGNAL_EVENT hypercall
>   hyperv: add support for KVM_HYPERV_EVENTFD
>   hyperv: process POST_MESSAGE hypercall
>   hyperv_testdev: add SynIC message and event testmodes
> 
>  include/hw/hyperv/hyperv-proto.h |   1 +
>  include/hw/hyperv/hyperv.h       |  58 +++-
>  include/hw/i386/pc.h             |   8 +
>  target/i386/cpu.h                |   1 +
>  target/i386/hyperv.h             |   4 +
>  hw/hyperv/hyperv.c               | 542 ++++++++++++++++++++++++++++++-
>  hw/misc/hyperv_testdev.c         | 165 +++++++++-
>  target/i386/cpu.c                |   2 +
>  target/i386/hyperv-stub.c        |  13 +
>  target/i386/hyperv.c             |  54 ++-
>  target/i386/kvm.c                |  45 ++-
>  target/i386/machine.c            |   9 +
>  12 files changed, 872 insertions(+), 30 deletions(-)
> 

I queued all three series, though if I were to post a pull request now
I'd stop before "hyperv: add synic message delivery".

Paolo

Re: [Qemu-devel] [PATCH 00/10] hyperv: add connection infrastructure

[Roman Kagan]

On Wed, Oct 03, 2018 at 01:12:10PM +0200, Paolo Bonzini wrote:
> On 21/09/2018 10:22, Roman Kagan wrote:
> > This series introduces the infrastructure to send and receive Hyper-V
> > messages and events.
> > 
> > More specifically,
> > 
> > - SynIC is turned into a full-fledged device managing the memory regions
> >   used for QEMU->guest communication
> > - machinery is introduced to post messages and signal events to the
> >   guest
> > - infrastructure is added to subscribe to messages and events from the
> >   guest, and to dispatch the received messages and events to the
> >   subscribers
> > 
> > Based-on: 20180921082041.29380-1-rkagan@virtuozzo.com
> > 
> > Roman Kagan (10):
> >   hyperv:synic: split capability testing and setting
> >   hyperv: qom-ify SynIC
> >   hyperv: only add SynIC in compatible configurations
> >   hyperv: make overlay pages for SynIC
> >   hyperv: add synic message delivery
> >   hyperv: add synic event flag signaling
> >   hyperv: process SIGNAL_EVENT hypercall
> >   hyperv: add support for KVM_HYPERV_EVENTFD
> >   hyperv: process POST_MESSAGE hypercall
> >   hyperv_testdev: add SynIC message and event testmodes
> > 
> >  include/hw/hyperv/hyperv-proto.h |   1 +
> >  include/hw/hyperv/hyperv.h       |  58 +++-
> >  include/hw/i386/pc.h             |   8 +
> >  target/i386/cpu.h                |   1 +
> >  target/i386/hyperv.h             |   4 +
> >  hw/hyperv/hyperv.c               | 542 ++++++++++++++++++++++++++++++-
> >  hw/misc/hyperv_testdev.c         | 165 +++++++++-
> >  target/i386/cpu.c                |   2 +
> >  target/i386/hyperv-stub.c        |  13 +
> >  target/i386/hyperv.c             |  54 ++-
> >  target/i386/kvm.c                |  45 ++-
> >  target/i386/machine.c            |   9 +
> >  12 files changed, 872 insertions(+), 30 deletions(-)
> > 
> 
> I queued all three series, though if I were to post a pull request now
> I'd stop before "hyperv: add synic message delivery".

To make sure I interpret this correctly: do the remaining patches need
more work beside the things you've commented on?

Also do you want me to post the corrected stuff as incremental fixups or
as a full-fledged respin?  In the latter case should I assume your r-b
on all patches before that one?

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 00/10] hyperv: add connection infrastructure

[Paolo Bonzini]

On 03/10/2018 16:25, Roman Kagan wrote:
>> I queued all three series, though if I were to post a pull request now
>> I'd stop before "hyperv: add synic message delivery".
> To make sure I interpret this correctly: do the remaining patches need
> more work beside the things you've commented on?
> 
> Also do you want me to post the corrected stuff as incremental fixups or
> as a full-fledged respin?  In the latter case should I assume your r-b
> on all patches before that one?

No need to do anything, I just wanted an explanation of patch 5 (I can
fix the "Mesage" thing myself).

Paolo

Re: [Qemu-devel] [PATCH 03/10] hyperv: only add SynIC in compatible configurations

[Eduardo Habkost]

On Fri, Sep 21, 2018 at 11:22:10AM +0300, Roman Kagan wrote:
> Certain configurations do not allow SynIC to be used in QEMU.  In
> particular,
> 
> - when hyperv_vpindex is off, SINT routes can't be used as they refer to
>   the destination vCPU by vp_index
> 
> - older KVM (which doesn't expose KVM_CAP_HYPERV_SYNIC2) zeroes out
>   SynIC message and event pages on every msr load, breaking migration
> 
> OTOH in-KVM users of SynIC -- SynIC timers -- do work in those
> configurations, and we shouldn't stop the guest from using them.
> 
> To cover both scenarios, introduce an X86CPU property that makes CPU
> init code to skip creation of the SynIC object (and thus disables any
> SynIC use in QEMU) but keeps the KVM part of the SynIC working.
> The property is clear by default but is set via compat logic for older
> machine types.
> 
> As a result, when hv_synic and a modern machine type are specified, QEMU
> will refuse to run unless vp_index is on and the kernel is recent
> enough.  OTOH with an older machine type QEMU will run fine with
> hv_synic=on against an older kernel and/or without vp_index enabled but
> will disallow the in-QEMU uses of SynIC (in e.g. VMBus).
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  include/hw/i386/pc.h |  8 ++++++++
>  target/i386/cpu.h    |  1 +
>  target/i386/cpu.c    |  2 ++
>  target/i386/kvm.c    | 30 ++++++++++++++++++++++--------
>  4 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 6894f37df1..dfe6746692 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -294,6 +294,14 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
> +#define PC_COMPAT_3_0 \
> +    HW_COMPAT_3_0 \
> +    {\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "x-hv-synic-kvm-only",\
> +        .value    = "on",\
> +    }

Oops.  This macro does nothing if the 3.1 machine-types aren't
updated to use it.  This patch breaks compatibility on pc-*-3.1.

> [...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/10] hyperv: only add SynIC in compatible configurations

[Igor Mammedov]

On Fri, 21 Sep 2018 11:22:10 +0300
Roman Kagan <rkagan@virtuozzo.com> wrote:

> Certain configurations do not allow SynIC to be used in QEMU.  In
> particular,
> 
> - when hyperv_vpindex is off, SINT routes can't be used as they refer to
>   the destination vCPU by vp_index
> 
> - older KVM (which doesn't expose KVM_CAP_HYPERV_SYNIC2) zeroes out
>   SynIC message and event pages on every msr load, breaking migration
> 
> OTOH in-KVM users of SynIC -- SynIC timers -- do work in those
> configurations, and we shouldn't stop the guest from using them.
> 
> To cover both scenarios, introduce an X86CPU property that makes CPU
> init code to skip creation of the SynIC object (and thus disables any
> SynIC use in QEMU) but keeps the KVM part of the SynIC working.
> The property is clear by default but is set via compat logic for older
> machine types.
> 
> As a result, when hv_synic and a modern machine type are specified, QEMU
> will refuse to run unless vp_index is on and the kernel is recent
> enough.  OTOH with an older machine type QEMU will run fine with
> hv_synic=on against an older kernel and/or without vp_index enabled but
> will disallow the in-QEMU uses of SynIC (in e.g. VMBus).
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>

With current upstream and x-hv-synic-kvm-only=on QEMU will SIGSEGV.
Problem was unnoticed since added compat property wasn't actually used
until much later commit
  4a93722f9c hw/i386: add pc-i440fx-3.1 & pc-q35-3.1
which put compat property in use.

qemu-system-x86_64 -machine pc-i440fx-2.10,accel=kvm \
 -cpu host,-vmx,hv-relaxed,hv_spinlocks=0x1fff,hv-vpindex,hv-synic 

simpler reproducer:
 qemu-system-x86_64 -enable-kvm -cpu host,hv-synic,x-hv-synic-kvm-only=on

Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>

> ---
>  include/hw/i386/pc.h |  8 ++++++++
>  target/i386/cpu.h    |  1 +
>  target/i386/cpu.c    |  2 ++
>  target/i386/kvm.c    | 30 ++++++++++++++++++++++--------
>  4 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 6894f37df1..dfe6746692 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -294,6 +294,14 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
> +#define PC_COMPAT_3_0 \
> +    HW_COMPAT_3_0 \
> +    {\
> +        .driver   = TYPE_X86_CPU,\
> +        .property = "x-hv-synic-kvm-only",\
> +        .value    = "on",\
> +    }
> +
>  #define PC_COMPAT_2_12 \
>      HW_COMPAT_2_12 \
>      {\
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index b572a8e4aa..e2e87dc13f 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1376,6 +1376,7 @@ struct X86CPU {
>      bool hyperv_vpindex;
>      bool hyperv_runtime;
>      bool hyperv_synic;
> +    bool hyperv_synic_kvm_only;
>      bool hyperv_stimer;
>      bool hyperv_frequencies;
>      bool hyperv_reenlightenment;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f24295e6e4..9c29c5db81 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5575,6 +5575,8 @@ static Property x86_cpu_properties[] = {
>       * to the specific Windows version being used."
>       */
>      DEFINE_PROP_INT32("x-hv-max-vps", X86CPU, hv_max_vps, -1),
> +    DEFINE_PROP_BOOL("x-hv-synic-kvm-only", X86CPU, hyperv_synic_kvm_only,
> +                     false),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 47427d98f8..056a482d3a 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -733,8 +733,18 @@ static int hyperv_handle_properties(CPUState *cs)
>          env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
>      }
>      if (cpu->hyperv_synic) {
> -        if (!has_msr_hv_synic ||
> -            !kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV_SYNIC)) {
> +        unsigned int cap = KVM_CAP_HYPERV_SYNIC;
> +        if (!cpu->hyperv_synic_kvm_only) {
> +            if (!cpu->hyperv_vpindex) {
> +                fprintf(stderr, "Hyper-V SynIC "
> +                        "(requested by 'hv-synic' cpu flag) "
> +                        "requires Hyper-V VP_INDEX ('hv-vpindex')\n");
> +            return -ENOSYS;
> +            }
> +            cap = KVM_CAP_HYPERV_SYNIC2;
> +        }
> +
> +        if (!has_msr_hv_synic || !kvm_check_extension(cs->kvm_state, cap)) {
>              fprintf(stderr, "Hyper-V SynIC (requested by 'hv-synic' cpu flag) "
>                      "is not supported by kernel\n");
>              return -ENOSYS;
> @@ -783,18 +793,22 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>      }
>  
>      if (cpu->hyperv_synic) {
> -        ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0);
> +        uint32_t synic_cap = cpu->hyperv_synic_kvm_only ?
> +            KVM_CAP_HYPERV_SYNIC : KVM_CAP_HYPERV_SYNIC2;
> +        ret = kvm_vcpu_enable_cap(cs, synic_cap, 0);
>          if (ret < 0) {
>              error_report("failed to turn on HyperV SynIC in KVM: %s",
>                           strerror(-ret));
>              return ret;
>          }
>  
> -        ret = hyperv_x86_synic_add(cpu);
> -        if (ret < 0) {
> -            error_report("failed to create HyperV SynIC: %s",
> -                         strerror(-ret));
> -            return ret;
> +        if (!cpu->hyperv_synic_kvm_only) {
> +            ret = hyperv_x86_synic_add(cpu);
> +            if (ret < 0) {
> +                error_report("failed to create HyperV SynIC: %s",
> +                             strerror(-ret));
> +                return ret;
> +            }
>          }
>      }
>  

Re: [Qemu-devel] [PATCH 03/10] hyperv: only add SynIC in compatible configurations

[Roman Kagan]

On Mon, Nov 26, 2018 at 03:45:03PM +0100, Igor Mammedov wrote:
> On Fri, 21 Sep 2018 11:22:10 +0300
> Roman Kagan <rkagan@virtuozzo.com> wrote:
> 
> > Certain configurations do not allow SynIC to be used in QEMU.  In
> > particular,
> > 
> > - when hyperv_vpindex is off, SINT routes can't be used as they refer to
> >   the destination vCPU by vp_index
> > 
> > - older KVM (which doesn't expose KVM_CAP_HYPERV_SYNIC2) zeroes out
> >   SynIC message and event pages on every msr load, breaking migration
> > 
> > OTOH in-KVM users of SynIC -- SynIC timers -- do work in those
> > configurations, and we shouldn't stop the guest from using them.
> > 
> > To cover both scenarios, introduce an X86CPU property that makes CPU
> > init code to skip creation of the SynIC object (and thus disables any
> > SynIC use in QEMU) but keeps the KVM part of the SynIC working.
> > The property is clear by default but is set via compat logic for older
> > machine types.
> > 
> > As a result, when hv_synic and a modern machine type are specified, QEMU
> > will refuse to run unless vp_index is on and the kernel is recent
> > enough.  OTOH with an older machine type QEMU will run fine with
> > hv_synic=on against an older kernel and/or without vp_index enabled but
> > will disallow the in-QEMU uses of SynIC (in e.g. VMBus).
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> 
> With current upstream and x-hv-synic-kvm-only=on QEMU will SIGSEGV.
> Problem was unnoticed since added compat property wasn't actually used
> until much later commit
>   4a93722f9c hw/i386: add pc-i440fx-3.1 & pc-q35-3.1
> which put compat property in use.
> 
> qemu-system-x86_64 -machine pc-i440fx-2.10,accel=kvm \
>  -cpu host,-vmx,hv-relaxed,hv_spinlocks=0x1fff,hv-vpindex,hv-synic 
> 
> simpler reproducer:
>  qemu-system-x86_64 -enable-kvm -cpu host,hv-synic,x-hv-synic-kvm-only=on
> 
> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Thanks for the report, fix is on the way to ML.

Roman.