On Wed, 16 Jan 2013 13:17:34 +0100
Alexander Graf <ag...@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.h...@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.
