Re: [PULL 32/62] hw/xen: Implement EVTCHNOP_bind_virq

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 2 Mar 2023 at 12:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Add the array of virq ports to each vCPU so that we can deliver timers,
> debug ports, etc. Global virqs are allocated against vCPU 0 initially,
> but can be migrated to other vCPUs (when we implement that).
>
> The kernel needs to know about VIRQ_TIMER in order to accelerate timers,
> so tell it via KVM_XEN_VCPU_ATTR_TYPE_TIMER. Also save/restore the value
> of the singleshot timer across migration, as the kernel will handle the
> hypercalls automatically now.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>

Hi; Coverity points out (CID 1507534) that we seem to sometimes
access env->xen_singleshot_timer_ns under the protection of
env->xen_timers_lock (eg in xen_vcpu_singleshot_timer_event())
and sometimes not (the specific case Coverity complains about is
in do_vcpu_soft_reset()). Is this a false positive, or is there
missing locking here ?

>  /* The x86 has a strong memory model with some store-after-load re-ordering */
>  #define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
>
> @@ -1806,6 +1808,8 @@ typedef struct CPUArchState {
>      uint64_t xen_vcpu_time_info_gpa;
>      uint64_t xen_vcpu_runstate_gpa;
>      uint8_t xen_vcpu_callback_vector;
> +    uint16_t xen_virq[XEN_NR_VIRQS];
> +    uint64_t xen_singleshot_timer_ns;
>  #endif
>  #if defined(CONFIG_HVF)
>      HVFX86LazyFlags hvf_lflags;

> @@ -387,6 +434,8 @@ static void do_vcpu_soft_reset(CPUState *cs, run_on_cpu_data data)
>      env->xen_vcpu_time_info_gpa = INVALID_GPA;
>      env->xen_vcpu_runstate_gpa = INVALID_GPA;
>      env->xen_vcpu_callback_vector = 0;
> +    env->xen_singleshot_timer_ns = 0;
> +    memset(env->xen_virq, 0, sizeof(env->xen_virq));
>
>      set_vcpu_info(cs, INVALID_GPA);
>      kvm_xen_set_vcpu_attr(cs, KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO,
> @@ -395,6 +444,7 @@ static void do_vcpu_soft_reset(CPUState *cs, run_on_cpu_data data)
>                            INVALID_GPA);
>      if (kvm_xen_has_cap(EVTCHN_SEND)) {
>          kvm_xen_set_vcpu_callback_vector(cs);
> +        kvm_xen_set_vcpu_timer(cs);
>      }
>
>  }

thanks
-- PMM