Message ID | 1379478118-20448-1-git-send-email-aik@ozlabs.ru |
---|---|
State | New |
Headers | show |
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, ®); > + > + 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
On 09/18/2013 05:14 PM, Peter Maydell wrote: > 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. What comments? Do not the function names speak for themselves already?
On 18 September 2013 16:46, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 09/18/2013 05:14 PM, Peter Maydell wrote: >> 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. > > What comments? Do not the function names speak for themselves already? It's a new public function, it should have a comment in the standard format saying what it does and what the input parameters and output are. Just to pick some examples at random, the fact that the return value is 0-on-success-or-negative-errno should be documented, and we should refer the user to the ioctl docs for valid id values. It doesn't need to be a long essay, but we should be documenting new functions as we add them. -- PMM
On 09/18/2013 05:59 PM, Peter Maydell wrote: > On 18 September 2013 16:46, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> On 09/18/2013 05:14 PM, Peter Maydell wrote: >>> 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. >> >> What comments? Do not the function names speak for themselves already? > > It's a new public function, it should have a comment in the standard > format saying what it does and what the input parameters and output are. > Just to pick some examples at random, the fact that the return value > is 0-on-success-or-negative-errno should be documented, and we should > refer the user to the ioctl docs for valid id values. It doesn't need to be > a long essay, but we should be documenting new functions as we add them. It would be awesome if you just gave me any really good example of what you expect from such a comment as kvm-all.c does not have any whatsoever. And pci.c does not. And exec.c does not. But I am sure there is some as it cannot possibly be me who starts making such comments in qemu. Thanks.
Il 18/09/2013 10:08, Alexey Kardashevskiy ha scritto: > On 09/18/2013 05:59 PM, Peter Maydell wrote: >> On 18 September 2013 16:46, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>> On 09/18/2013 05:14 PM, Peter Maydell wrote: >>>> 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. >>> >>> What comments? Do not the function names speak for themselves already? >> >> It's a new public function, it should have a comment in the standard >> format saying what it does and what the input parameters and output are. >> Just to pick some examples at random, the fact that the return value >> is 0-on-success-or-negative-errno should be documented, and we should >> refer the user to the ioctl docs for valid id values. It doesn't need to be >> a long essay, but we should be documenting new functions as we add them. > > > It would be awesome if you just gave me any really good example of what you > expect from such a comment as kvm-all.c does not have any whatsoever. And > pci.c does not. And exec.c does not. But I am sure there is some as > it cannot possibly be me who starts making such comments in qemu. Thanks. include/exec/memory.h, include/qom/object.h, include/block/blockjob.h, include/qemu/main-loop.h, include/block/aio.h have many, thankfully. Paolo
On 18 September 2013 17:08, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 09/18/2013 05:59 PM, Peter Maydell wrote: > It would be awesome if you just gave me any really good example of what you > expect from such a comment as kvm-all.c does not have any whatsoever. And > pci.c does not. And exec.c does not. But I am sure there is some as > it cannot possibly be me who starts making such comments in qemu. Thanks. Yes, we have a lot of preexisting undocumented functions. It's very common in QEMU for existing code not to be up to preferred standards; you can't use current code as a yardstick for "good enough to pass code review". The example I usually crib from for formatting is the extract/deposit APIs in include/qemu/bitops.h. include/exec/memory.h is also well documented. -- PMM
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); + #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, ®); + + if (ret) { + error_report("Unable to set reg#0x%"PRIx64" to KVM: %m\n", id); + } + + return ret; +} + +int kvm_get_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_GET_ONE_REG, ®); + + if (ret) { + error_report("Unable to get reg#0x%"PRIx64" from KVM: %m\n", id); + } + + return ret; +}
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(+)