Hi Marc, Christoffer,
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.
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.
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.
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
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 ...
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.
Also for V8 case as per following patch (ARM64: KVM:
set and get of sys registers in BE case) we don't
have big issue, because all registers are 64 bit.
But for consistency sake it will need to be reworked
too.
I hope you would agree that benefits of following
on this approach are not that great. Please let me
know if you think otherwise.
Thanks,
Victor
Victor Kamensky (2):
ARM: KVM: one_reg coproc set and get BE fixes
ARM64: KVM: set and get of sys registers in BE case
arch/arm/kvm/coproc.c | 111 +++++++++++++++++++++++++++++++++++-----------
arch/arm64/kvm/sys_regs.c | 21 ++++++---
2 files changed, 99 insertions(+), 33 deletions(-)
--
1.8.1.4
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
