Re: [Qemu-devel] [PATCH] kvm: add set_one_reg/get_one_reg

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

On 18 September 2013 05:21, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> This adds QEMU wrappers for KVM_SET_ONE_REG/KVM_GET_ONE_REG ioctls.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/sysemu/kvm.h |  4 ++++
>  kvm-all.c            | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index c7bc07b..b2d61e9 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -319,4 +319,8 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq);
>  void kvm_pc_gsi_handler(void *opaque, int n, int level);
>  void kvm_pc_setup_irq_routing(bool pci_enabled);
>  void kvm_init_irq_routing(KVMState *s);
> +
> +int kvm_set_one_reg(CPUState *cs, uint64_t id, void *addr);
> +int kvm_get_one_reg(CPUState *cs, uint64_t id, void *addr);

Doc comments, please.

> +
>  #endif
> diff --git a/kvm-all.c b/kvm-all.c
> index ded7fc8..c24ab76 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -34,6 +34,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/event_notifier.h"
>  #include "trace.h"
> +#include "qemu/error-report.h"
>
>  /* This check must be after config-host.h is included */
>  #ifdef CONFIG_EVENTFD
> @@ -2049,3 +2050,33 @@ int kvm_on_sigbus(int code, void *addr)
>  {
>      return kvm_arch_on_sigbus(code, addr);
>  }
> +
> +int kvm_set_one_reg(CPUState *cs, uint64_t id, void *addr)
> +{
> +    struct kvm_one_reg reg = {
> +        .id = id,
> +        .addr = (uintptr_t)addr,
> +    };
> +    int ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +
> +    if (ret) {
> +        error_report("Unable to set reg#0x%"PRIx64" to KVM: %m\n", id);
> +    }

This makes these functions useless for cases where you expect that
they might fail (of which we have a number in target-arm). Please
just return the error and leave the reporting/handling to the caller.

Incidentally, %m isn't portable -- it's a glibc extension and we have
to work with non-glibc libc on some platforms. Also your error
at this point isn't in errno, it's in ret (negated).

thanks
-- PMM