On Tue, May 27, 2014 at 11:08:10PM -0700, Victor Kamensky wrote:
> Hi Marc, Christoffer,
Hi Victor,
>
> The most controversial piece of my BE KVM series is patches
> that deals with coproc registers byteswaps, especially in V7
> case. Here I send my lasted proposed version as RFC, hopping
> to get more discussion on the subject. In the following this
> cover letter patches I did address some of Christoffer's
> suggestions but still not completely.
Before looking at the patches, here are some thoughts:
>
> Approach 1
> ----------
>
> That is the one I try to pursue and current patches are about
> it. With current scheme which I try to change we have two main
> issues:
>
> 1) current reg_from_user takes 'void *' and can take pointer to
> host u64 value or u32 value as well as register size could 8 or
> 4 bytes. In case of BE, and register size 4 bytes, it is
> impossible to differentiate between case where pointer to
> u32 is passed to function or pointer to u64 passed to function.
> In case of u32 pointer 4 bytes should be copied to val address
> itself but in case of u64 pointer in order to update low word
> of u64 value 4 bytes should be copied at 'val + 4' address.
> To fix it strong typed versions depending on kernel target type
> of functions are introduced:
> reg_from_user_to_u32 and reg_to_user_from_u32
> reg_from_user_to_u64 and reg_to_user_from_u64
> It seems to me that Christoffer is fine with this approach.
> Marc, from your comment it was not clear whether you against
> this approach or needed clarification why reg_xxx_[u32|u64]
> function were introduced.
I am fine with having two separate functions, but we could also ahve a
single function.
My concern is that these functions are tied to how things happen to be
stored in the kernel, where I think they should only deal with copying
things to/from user space.
So, in my oppinion, reg_*_32 should *always*, *only*, deal with
registers that are defined to user space as 32 bit registers, and should
*always*, *only* take a u32 *pointer. Same thing for 64 bit versions.
>
> 2) Size of target kernel variable u32 and u64 may mismatch
> size of requested register. We have two sub-cases here.
> a) 4 bytes register could be read from kernel u64
> b) 8 byte register (LPAE related TTBR, etc) could be
> read from array of two u32 values.
This is just an unfortuante way of storing things internally in the
kernel when we start dealing with multiple endianness.
I think either change the way we are storing things to make them clearly
typed, or deal with the quirky storage in separate functions or close
to the logic that knows how to deal with this, not in a convoluted
generic function.
>
> It seems that for case 2a) we have only couple cases in
> get_invariant_cp15 and set_invariant_cp15 functions. I
> followed Christoffer's suggestion, I disallowed use of
> 4 bytes register size along with u64 kernel target and
> changed get_invariant_cp15 and set_invariant_cp15 to
> deals with different register sizes calling _u64 variant
> of _u32 variant separately. I don't know how to clean
> up 2b) case I left it alone.
>
> Approach 2
> ----------
>
> Alex Graf suggested to move in this direction. I looked
> at it and I don't see how to apply it. But for completeness
> sake here is my observation.
>
> PPC deals with similar issues already. But PPC coproc
> handling code is structured fundamentally in different
> way. If in ARM access of userspace is pushed to leaf
> functions in PPC userspace value is read or written by
> top level function. Guts of coproc handling code deal
I don't understand what you mean by leaf functions and top level
functions.
> with kvmppc_one_reg union that is defined like this:
>
> union kvmppc_one_reg {
> u32 wval;
> u64 dval;
> vector128 vval;
> u64 vsxval[2];
> struct {
> u64 addr;
> u64 length;
> } vpaval;
> };
>
> But note that besides wval and daval, there are PPC specific
> values like vval, vsxval, vpaval ...
Being quite ignorant about PPC, what is the problem that the PPC user
space access functions are trying to solve?
>
> kvmppc_one_reg union is passed to different functions that
> deal with different classes of coproc values. Those functions
> use get_reg_val and set_reg_val macros which are
> defined like this:
>
> #define get_reg_val(id, reg) ({ \
> union kvmppc_one_reg __u; \
> switch (one_reg_size(id)) { \
> case 4: __u.wval = (reg); break; \
> case 8: __u.dval = (reg); break; \
> default: BUG(); \
> } \
> __u; \
> })
>
> #define set_reg_val(id, val) ({ \
> u64 __v; \
> switch (one_reg_size(id)) { \
> case 4: __v = (val).wval; break; \
> case 8: __v = (val).dval; break; \
> default: BUG(); \
> } \
> __v; \
> })
>
> (On the side: note BUG() call with sizes other 4 or 8;
> it seems that this code has similar issue that Marc
> caught in one of my diffs: BUG_ON call. It seems to
> me that userspace can craft special reg id that could
> trigger this BUG() call)
>
> My main problem with this approach is that we
> would need to rework where ARM KVM coproc handling
> functions access userspace. IMHO it is a lot of
> changes. In fact it will be way more changes than
> in my diff because in ARM KVM code there are cases
> where put_user/get_user is used in leaf functions
> instead of reg_from_user function. Also it is not
> clear how to handle 2b) case in above Approach 1.
> Also code sharing would not be that great. Unless
> I am missing something It looks like only get_reg_val
> set_reg_val could be reused. And it is not clear
> how to define common between ARM and PPC kvm_one_reg
> union.
I don't think we should be as concerned with the number of changes to
the code as with how the final code ends up looking like. If we can
make everything clear by starting to use arrays of unions instead of
arrays of u32s, then I would strongly prefer that.
Unfortunately I think you have to try a few different implementations to
see how it looks like.
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
