On 21.09.2012, at 11:52, Paul Mackerras wrote:
> 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.
It's not really a problem, it's really about redundancy. We write the same thing twice
case <id that encodes length>:
val.<length dependent value> = foo;
And every time we do constructs like that, someone will one day come in and do fun like
case <id with length 8>:
val.<length 4> = foo;
So if we can get rid of that complete bug category from day 1, it makes me happy :).
>
>> 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.
Imagine we get new awesome 128bit GPRs now. The lower 64bit are still the same as they always were. The new machine is also backwards compatible, so we could be running 64bit code.
To transfer the bigger registers we could then do
case KVM_REG_PPC_GPR1:
case KVM_REG_PPC_GPR1_128:
kvmppc_set_reg(id, val, foo);
But the bit I care a lot more about is the redundancy as explained above.
>
>> 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.
What variable type would the 3rd argument be? Also, constant folding should still work with one_reg_size(), no?
>
>> 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.
If unions are guaranteed to be laid out the way you describe above, that's perfectly fine for me. I just remember nightmares when I had to convert x86 incomplete unions (the 2 members were of different size) to ppc, where they started at the reverse end. A scheme that isn't tied to BE would maybe allow us to share code with ARM and x86.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
