Re: [PATCH v3 1/2] KVM: PPC: Book3S: Get/set guest SPRs using the GET/SET_ONE_REG interface

[Paul Mackerras <paulus@samba.org>]

On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote:
> 
> On 21.09.2012, at 07:44, Paul Mackerras wrote:
> 
> > +union kvmppc_one_reg {
> > +	u32	wval;
> > +	u64	dval;
> 
> Phew. Is this guaranteed to always pad on the right, rather than left?

Absolutely (for big-endian targets).  A union is basically a struct
with all the members at offset 0.  So we read N bytes into the union
and then access the N-byte member.

> > +static int one_reg_size(u64 id)
> > +{
> > +	id = (id & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT;
> > +	return 1 << id;
> > +}
> 
> Rusty has a patch to export this as a generic function for all kvm targets. Check out
> 
>   http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/97553
> 
> I actually like static inlines better, but we really should keep this code on top level :).

Once Rusty's patch goes in I'd be happy to use it.

> > +		switch (reg->id) {
> > +		case KVM_REG_PPC_DAR:
> > +			val.dval = vcpu->arch.shared->dar;
> 
> Hmm. I would like to keep the amount of places who need to manually know about the size of a register as low as possible. We do know the size of the register already on in the REG id and in sizeof(register), right?

This code doesn't "manually" know about the size of the register.  It
could be that vcpu->arch.shared->dar is 4 bytes, or 2 bytes, instead
of 8 bytes, and that wouldn't affect this code.  All this code "knows"
is that the definition of KVM_REG_PPC_DAR includes the "8 byte"
encoding.  And since that definition is -- or will be -- part of the
userspace ABI, it's not something that can be changed later, so I
don't see any problem with just using val.dval here.

> I think there will be valid scenarios for them to be of different size. For example when a register suddenly increases in size. But the assignment should always happen on the size the register id tells us.

It doesn't matter if a register changes in size, that doesn't affect
this code.  We may want to define an extra KVM_REG_PPC_* symbol if it
increases in size, but that would be a different symbol to the ones we
already have.  As I said, we couldn't just arbitrarily change the
existing symbol because that would break existing userspace programs.

In other words the assignment already does happen on the size the
register id tells us, with my code.

> So how about something like
> 
> #define kvmppc_set_reg(id, val, reg) { \
>   switch (one_reg_size(id)) { \
>   case 4: val.wval = reg; break; \
>   case 8: val.dval = reg; break; \
>   default: BUG(); \
>   } \
> }
> 
> case KVM_REG_PPC_DAR:
>   kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar);
>   break;
> 
> Maybe you can come up with something even easier though :).

Seems unnecessarily complex to me.  If we did something like that we
should do

  kvmppc_set_reg(KVM_REG_PPC_DAR, &val, vcpu->arch.shared->dar);

and use a macro instead of one_reg_size() to give the compiler a
better chance to do constant folding and only compile in the branch of
the switch statement that we need.

> Btw, with such a scheme in place, we wouldn't even need the union. We could just reserve a char array and cast it in the setter/getter functions.

I'm always wary of accessing a variable of one type as a different
type using a cast on its address, because of the C99 type-punning
rules.  It's probably OK with a char array, but I think a union is
cleaner, because you're explicitly stating that you want some storage
that can be accessed as different types.

Paul.