Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2013-08-26 at 15:11 +0200, Alexander Graf wrote:
> On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Recent PowerKVM allows the kernel to intercept some RTAS calls from
> the
> > guest directly.  This is used to implement the more efficient
> in-kernel
> > XICS for example.  qemu is still responsible for assigning the RTAS
> token
> > numbers however, and needs to tell the kernel which RTAS function
> name is
> > assigned to a given token value.  This patch adds a convenience
> wrapper for
> > the KVM_PPC_RTAS_DEFINE_TOKEN ioctl() which is used for this
> purpose.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > target-ppc/kvm.c     | 14 ++++++++++++++
> > target-ppc/kvm_ppc.h |  7 +++++++
> > 2 files changed, 21 insertions(+)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index eea8c09..ab46aaa 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1792,6 +1792,20 @@ static int
> kvm_ppc_register_host_cpu_type(void)
> >     return 0;
> > }
> > 
> > +int kvmppc_define_rtas_token(uint32_t token, const char *function)
> 
> The naming here is slightly confusing. What the ioctl does is define
> an rtas token that gets handled in-kernel. The name of the function
> should reflect this. How about something like
> kvmppc_define_rtas_in_kernel()?

Am I dreaming ? Those patches were written about a year ago and you guys
are still nitpicking on names ? They should have been merged a LONG time
ago ...

I'm seriously wondering how people get anything done with qemu KVM when
it takes about a year of bike shed painting to get the most basic
functionality merged.

Alex, you know, a maintainer's job is not to bounce a patch over and
over again until it looks EXACTLY what you would have written in the
first place. If that was the case you may as well write it yourself.

In this specific case, it's even more annoying because that comment
could have been done ages ago, and because the name you propose is even
more horrible than what was there... If you are really keen about that
pick up at least something that doesn't suck such as

	kvmppc_define_rtas_kernel_token()

What you propose is even more confusing as to what the function is for.

At the end of the day, I despair though. Really.

Ben.

> 
> Alex
> 
> > +{
> > +    struct kvm_rtas_token_args args = {
> > +        .token = token,
> > +    };
> > +
> > +    if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_RTAS)) {
> > +        return -ENOENT;
> > +    }
> > +
> > +    strncpy(args.name, function, sizeof(args.name));
> > +
> > +    return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN,
> &args);
> > +}
> > 
> > int kvmppc_get_htab_fd(bool write)
> > {
> > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> > index 4ae7bf2..12564ef 100644
> > --- a/target-ppc/kvm_ppc.h
> > +++ b/target-ppc/kvm_ppc.h
> > @@ -38,6 +38,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size,
> unsigned int hash_shift);
> > #endif /* !CONFIG_USER_ONLY */
> > int kvmppc_fixup_cpu(PowerPCCPU *cpu);
> > bool kvmppc_has_cap_epr(void);
> > +int kvmppc_define_rtas_token(uint32_t token, const char *function);
> > int kvmppc_get_htab_fd(bool write);
> > int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t
> max_ns);
> > int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> > @@ -164,6 +165,12 @@ static inline bool kvmppc_has_cap_epr(void)
> >     return false;
> > }
> > 
> > +static inline int kvmppc_define_rtas_token(uint32_t token,
> > +                                           const char *function)
> > +{
> > +    return -1;
> > +}
> > +
> > static inline int kvmppc_get_htab_fd(bool write)
> > {
> >     return -1;
> > -- 
> > 1.8.3.2
> >