Re: [PATCH v12 47/60] i386/xen: handle PV timer hypercalls

[Paul Durrant <xadimgnik@gmail.com>]

On 22/02/2023 12:03, Paul Durrant wrote:
> On 22/02/2023 09:21, David Woodhouse wrote:
>> On Mon, 2023-02-20 at 20:47 +0000, David Woodhouse wrote:
>>> @@ -1246,6 +1470,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);
>>> +        }
>>
>> Argh, there I'm returning -errno from a function that ought to set it
>> in exit->u.hcall.result and return 'true' for a handled syscall. Again.
>>
>> Still *slightly* regretting my life choices there and wishing the
>> compiler caught that for me, but not enough to change it because we
>> really *do* want to track which unhandled calls guests are trying to
>> make. I'll fix it and then (if I make load_multiboot() tolerate 64-bit
>> binaries as previously discussed) the XTF tests work:
>>
>>   $ ./bkvm/qemu-system-x86_64 -serial mon:stdio  -accel 
>> kvm,xen-version=0x4000a,kernel-irqchip=split -cpu host -display none 
>> -kernel$XTFDIR/tests/set_timer_op/test-hvm64-set_timer_op
>> --- Xen Test Framework ---
>> Environment: HVM 64bit (Long mode 4 levels)
>> Test hypercall set_timer_op
>> Test result: SUCCESS
>> ******************************
>> PANIC: xtf_exit(): hypercall_shutdown(SHUTDOWN_poweroff) returned
>> ******************************
>> QEMU: Terminated
>> $ ./bkvm/qemu-system-x86_64 -serial mon:stdio  -accel 
>> kvm,xen-version=0x4000a,kernel-irqchip=split -cpu host -display none 
>> -kernel $XTFDIR/tests/set_timer_op/test-hvm32-set_timer_op
>> --- Xen Test Framework ---
>> Environment: HVM 32bit (No paging)
>> Test hypercall set_timer_op
>> Test result: SUCCESS
>> ******************************
>> PANIC: xtf_exit(): hypercall_shutdown(SHUTDOWN_poweroff) returned
>> ******************************
>> QEMU: Terminated
>>
>>
>> (Dunno why it whines about poweroff; it isn't even calling the
>> hypercall. And the test to explicitly test that hypercall does work.)
>>
>>
>> --- a/target/i386/kvm/xen-emu.c
>> +++ b/target/i386/kvm/xen-emu.c
>> @@ -1118,14 +1118,18 @@ static int 
>> vcpuop_stop_singleshot_timer(CPUState *cs)
>>       return 0;
>>   }
>> -static int kvm_xen_hcall_set_timer_op(struct kvm_xen_exit *exit, 
>> X86CPU *cpu,
>> -                                      uint64_t timeout)
>> +static bool kvm_xen_hcall_set_timer_op(struct kvm_xen_exit *exit, 
>> X86CPU *cpu,
>> +                                       uint64_t timeout)
>>   {
>> +    int err;
>> +
>>       if (unlikely(timeout == 0)) {
>> -        return vcpuop_stop_singleshot_timer(CPU(cpu));
>> +        err = vcpuop_stop_singleshot_timer(CPU(cpu));
>>       } else {
>> -        return do_set_singleshot_timer(CPU(cpu), timeout, false, true);
>> +        err = do_set_singleshot_timer(CPU(cpu), timeout, false, true);
>>       }
>> +    exit->u.hcall.result = err;
>> +    return true;
>>   }
>>   static bool kvm_xen_hcall_vcpu_op(struct kvm_xen_exit *exit, X86CPU 
>> *cpu,
>>
> 
> That looks better :-)
> 

NB I think you still need to fix kvm_xen_hcall_vcpu_op() to not return 
the -ENOENT too.