Re: [Qemu-devel] [PATCH 1/2] s390: Add a hypercall registration interface.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Wed, 16 Jan 2013 13:17:34 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 16.01.2013, at 12:57, Cornelia Huck wrote:
> 
> > Allow virtio machines to register for different diag500 function
> > codes and convert s390-virtio to use it.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Nice cleanup :). One minor nitpick below

> > +int s390_virtio_hypercall(CPUS390XState *env)
> > +{
> > +    s390_virtio_fn fn = s390_diag500_table[env->regs[1]];
> > +
> > +    return fn ? fn(env->regs[2], env->regs[3], env->regs[4], env->regs[5],
> > +                   env->regs[6], env->regs[7]) : -EINVAL;
> 
> if (!fn) {
>     return -EINVAL;
> }
> 
> return fn(&env->regs[2]);
> 
> That way the hypercall handling function can determine itself which registers it really needs to access.

Yes, this looks a bit nicer. v2 is on the way.

> 
> 
> > +}

> > static int handle_hypercall(CPUS390XState *env, struct kvm_run *run)
> > {
> >     cpu_synchronize_state(env);
> > -    env->regs[2] = s390_virtio_hypercall(env, env->regs[2], env->regs[1]);
> > +    env->regs[2] = s390_virtio_hypercall(env);
> 
> Just thinking out loud here. With synchronized registers, we have full access to the GPRs already without copying them to env. So if instead we would call
> 
>     s390_virtio_hypercall(env->regs);
> 
> we could in case we support synchronized registers call
> 
>     s390_virtio_hypercall(kvm_run->s.regs.gprs);
> 
> which would completely remove the need for cpu_synchronize_state() for normal hypercalls.
> 
> This is outside of the scope of this patch, but might be a useful thing to do :). As a nice side effect, the global s390_virtio_hypercall function wouldn't have to know anything about CPUState either.

Sounds like a good future improvement.