Re: [PATCH v11 47/59] i386/xen: handle PV timer hypercalls

[Paul Durrant]

On 16/02/2023 06:24, David Woodhouse wrote:
> From: Joao Martins <joao.m.martins@oracle.com>
>
> Introduce support for one shot and periodic mode of Xen PV timers,
> whereby timer interrupts come through a special virq event channel
> with deadlines being set through:
>
> 1) set_timer_op hypercall (only oneshot)
> 2) vcpu_op hypercall for {set,stop}_{singleshot,periodic}_timer
> hypercalls
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_evtchn.c  |  31 +++++
>   hw/i386/kvm/xen_evtchn.h  |   2 +
>   target/i386/cpu.h         |   5 +
>   target/i386/kvm/xen-emu.c | 252 +++++++++++++++++++++++++++++++++++++-
>   target/i386/machine.c     |   1 +
>   5 files changed, 289 insertions(+), 2 deletions(-)
[snip]
>   static bool kvm_xen_hcall_vcpu_op(struct kvm_xen_exit *exit, X86CPU *cpu,
>                                     int cmd, int vcpu_id, uint64_t arg)
>   {
> -    CPUState *dest = qemu_get_cpu(vcpu_id);
>       CPUState *cs = CPU(cpu);
> +    CPUState *dest = cs->cpu_index == vcpu_id ? cs : qemu_get_cpu(vcpu_id);
>       int err;
>   
> +    if (!dest) {
> +        return -ENOENT;
> +    }
> +

I thought the patch format was catching me out somehow but I don't think 
so...

The function declaration says 'static bool kvm_xen_hcall_vcpu_op(...)' 
but that return value doesn't look very boolean to me. I think you also 
have the same issue...

>       switch (cmd) {
>       case VCPUOP_register_runstate_memory_area:
>           err = vcpuop_register_runstate_info(cs, dest, arg);
> @@ -892,6 +1092,26 @@ static bool kvm_xen_hcall_vcpu_op(struct kvm_xen_exit 
> *exit, X86CPU *cpu,
>       case VCPUOP_register_vcpu_info:
>           err = vcpuop_register_vcpu_info(cs, dest, arg);
>           break;
> +    case VCPUOP_set_singleshot_timer: {
> +        if (cs->cpu_index != vcpu_id) {
> +            return -EINVAL;
> +        }
> +        err = vcpuop_set_singleshot_timer(dest, arg);
> +        break;
> +    }
> +    case VCPUOP_stop_singleshot_timer:
> +        if (cs->cpu_index != vcpu_id) {
> +            return -EINVAL;
> +        }
> +        err = vcpuop_stop_singleshot_timer(dest);
> +        break;
> +    case VCPUOP_set_periodic_timer: {
> +        err = vcpuop_set_periodic_timer(cs, dest, arg);
> +        break;
> +    }
> +    case VCPUOP_stop_periodic_timer:
> +        err = vcpuop_stop_periodic_timer(dest);
> +        break;
>   
>       default:
>           return false;
> @@ -1246,6 +1466,16 @@ static bool do_kvm_xen_handle_exit(X86CPU *cpu, struct 
> kvm_xen_exit *exit)
>       }
>   
>       switch (code) {
> +    case __HYPERVISOR_set_timer_op:
> +        if (exit->u.hcall.longmode) {
> +            return kvm_xen_hcall_set_timer_op(exit, cpu,
> +                                              exit->u.hcall.params[0]);
> +        } else {
> +            /* In 32-bit mode, the 64-bit timer value is in two args. */
> +            uint64_t val = ((uint64_t)exit->u.hcall.params[1]) << 32 |
> +                (uint32_t)exit->u.hcall.params[0];
> +            return kvm_xen_hcall_set_timer_op(exit, cpu, val);
> +        }

... with these returns above.

  Paul

>       case __HYPERVISOR_grant_table_op:
>           return kvm_xen_hcall_gnttab_op(exit, cpu, exit->u.hcall.params[0],
>                                          exit->u.hcall.params[1],
> @@ -1355,7 +1585,25 @@ int kvm_put_xen_state(CPUState *cs)
>           }
>       }
>   
> +    if (env->xen_periodic_timer_period) {
> +        ret = do_set_periodic_timer(cs, env->xen_periodic_timer_period);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
>       if (!kvm_xen_has_cap(EVTCHN_SEND)) {
> +        /*
> +         * If the kernel has EVTCHN_SEND support then it handles timers too,
> +         * so the timer will be restored by kvm_xen_set_vcpu_timer() below.
> +         */
> +        if (env->xen_singleshot_timer_ns) {
> +            ret = do_set_singleshot_timer(cs, env->xen_singleshot_timer_ns,
> +                                    false, false);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
>           return 0;
>       }
>   
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 603a1077e3..c7ac8084b2 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -1277,6 +1277,7 @@ static const VMStateDescription vmstate_xen_vcpu = {
>           VMSTATE_UINT8(env.xen_vcpu_callback_vector, X86CPU),
>           VMSTATE_UINT16_ARRAY(env.xen_virq, X86CPU, XEN_NR_VIRQS),
>           VMSTATE_UINT64(env.xen_singleshot_timer_ns, X86CPU),
> +        VMSTATE_UINT64(env.xen_periodic_timer_period, X86CPU),
>           VMSTATE_END_OF_LIST()
>       }
>   };