Taking a fault on a non-byte-granular insn means that the "number of
bytes not handled" return value would need extra care in calculating, if
we want callers to be able to derive e.g. exception context (to be
injected to the guest) - CR2 for #PF in particular - from the value. To
simplify things rather than complicating them, reduce inline assembly to
just byte-granular string insns. On recent CPUs that's also supposed to
be more efficient anyway.
For singular element accessors, however, alignment checks are added,
hence slightly complicating the code. Misaligned (user) buffer accesses
will now be forwarded to copy_{from,to}_guest_ll().
Naturally copy_{from,to}_unsafe_ll() accessors end up being adjusted the
same way, as they're produced by mere re-processing of the same code.
Otoh copy_{from,to}_unsafe() aren't similarly adjusted, but have their
comments made match reality; down the road we may want to change their
return types, e.g. to bool.
Fixes: 76974398a63c ("Added user-memory accessing functionality for x86_64")
Fixes: 7b8c36701d26 ("Introduce clear_user and clear_guest")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -251,7 +251,8 @@ do {
static always_inline unsigned long
__copy_to_guest_pv(void __user *to, const void *from, unsigned long n)
{
- if (__builtin_constant_p(n)) {
+ if ( __builtin_constant_p(n) && !((unsigned long)to & (n - 1)) )
+ {
unsigned long ret;
switch (n) {
@@ -291,7 +292,8 @@ __copy_to_guest_pv(void __user *to, cons
static always_inline unsigned long
__copy_from_guest_pv(void *to, const void __user *from, unsigned long n)
{
- if (__builtin_constant_p(n)) {
+ if ( __builtin_constant_p(n) && !((unsigned long)from & (n - 1)) )
+ {
unsigned long ret;
switch (n) {
@@ -321,8 +323,7 @@ __copy_from_guest_pv(void *to, const voi
*
* Copy data from hypervisor space to a potentially unmapped area.
*
- * Returns number of bytes that could not be copied.
- * On success, this will be zero.
+ * Returns zero on success and non-zero if some bytes could not be copied.
*/
static always_inline unsigned int
copy_to_unsafe(void __user *to, const void *from, unsigned int n)
@@ -358,8 +359,7 @@ copy_to_unsafe(void __user *to, const vo
*
* Copy data from a potentially unmapped area space to hypervisor space.
*
- * Returns number of bytes that could not be copied.
- * On success, this will be zero.
+ * Returns zero on success and non-zero if some bytes could not be copied.
*
* If some data could not be copied, this function will pad the copied
* data to the requested size using zero bytes.
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -16,42 +16,19 @@
unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned int n)
{
- unsigned dummy;
+ GUARD(unsigned dummy);
stac();
asm volatile (
GUARD(
" guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
)
- " cmp $"STR(2*BYTES_PER_LONG-1)", %[cnt]\n"
- " jbe 1f\n"
- " mov %k[to], %[cnt]\n"
- " neg %[cnt]\n"
- " and $"STR(BYTES_PER_LONG-1)", %[cnt]\n"
- " sub %[cnt], %[aux]\n"
- "4: rep movsb\n" /* make 'to' address aligned */
- " mov %[aux], %[cnt]\n"
- " shr $"STR(LONG_BYTEORDER)", %[cnt]\n"
- " and $"STR(BYTES_PER_LONG-1)", %[aux]\n"
- " .align 2,0x90\n"
- "0: rep movs"__OS"\n" /* as many words as possible... */
- " mov %[aux],%[cnt]\n"
- "1: rep movsb\n" /* ...remainder copied as bytes */
+ "1: rep movsb\n"
"2:\n"
- ".section .fixup,\"ax\"\n"
- "5: add %[aux], %[cnt]\n"
- " jmp 2b\n"
- "3: lea (%q[aux], %q[cnt], "STR(BYTES_PER_LONG)"), %[cnt]\n"
- " jmp 2b\n"
- ".previous\n"
- _ASM_EXTABLE(4b, 5b)
- _ASM_EXTABLE(0b, 3b)
_ASM_EXTABLE(1b, 2b)
- : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
- [aux] "=&r" (dummy)
+ : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
- : "[aux]" (n)
- : "memory" );
+ :: "memory" );
clac();
return n;
@@ -66,25 +43,9 @@ unsigned int copy_from_guest_ll(void *to
GUARD(
" guest_access_mask_ptr %[from], %q[scratch1], %q[scratch2]\n"
)
- " cmp $"STR(2*BYTES_PER_LONG-1)", %[cnt]\n"
- " jbe 1f\n"
- " mov %k[to], %[cnt]\n"
- " neg %[cnt]\n"
- " and $"STR(BYTES_PER_LONG-1)", %[cnt]\n"
- " sub %[cnt], %[aux]\n"
- "4: rep movsb\n" /* make 'to' address aligned */
- " mov %[aux],%[cnt]\n"
- " shr $"STR(LONG_BYTEORDER)", %[cnt]\n"
- " and $"STR(BYTES_PER_LONG-1)", %[aux]\n"
- " .align 2,0x90\n"
- "0: rep movs"__OS"\n" /* as many words as possible... */
- " mov %[aux], %[cnt]\n"
- "1: rep movsb\n" /* ...remainder copied as bytes */
+ "1: rep movsb\n"
"2:\n"
".section .fixup,\"ax\"\n"
- "5: add %[aux], %[cnt]\n"
- " jmp 6f\n"
- "3: lea (%q[aux], %q[cnt], "STR(BYTES_PER_LONG)"), %[cnt]\n"
"6: mov %[cnt], %k[from]\n"
" xchg %%eax, %[aux]\n"
" xor %%eax, %%eax\n"
@@ -93,14 +54,11 @@ unsigned int copy_from_guest_ll(void *to
" mov %k[from], %[cnt]\n"
" jmp 2b\n"
".previous\n"
- _ASM_EXTABLE(4b, 5b)
- _ASM_EXTABLE(0b, 3b)
_ASM_EXTABLE(1b, 6b)
: [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
[aux] "=&r" (dummy)
GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
- : "[aux]" (n)
- : "memory" );
+ :: "memory" );
clac();
return n;
@@ -145,20 +103,12 @@ unsigned int clear_guest_pv(void __user
stac();
asm volatile (
" guest_access_mask_ptr %[to], %[scratch1], %[scratch2]\n"
- "0: rep stos"__OS"\n"
- " mov %[bytes], %[cnt]\n"
"1: rep stosb\n"
"2:\n"
- ".section .fixup,\"ax\"\n"
- "3: lea (%q[bytes], %q[longs], "STR(BYTES_PER_LONG)"), %[cnt]\n"
- " jmp 2b\n"
- ".previous\n"
- _ASM_EXTABLE(0b,3b)
_ASM_EXTABLE(1b,2b)
- : [cnt] "=&c" (n), [to] "+D" (to), [scratch1] "=&r" (dummy),
+ : [cnt] "+c" (n), [to] "+D" (to), [scratch1] "=&r" (dummy),
[scratch2] "=&r" (dummy)
- : [bytes] "r" (n & (BYTES_PER_LONG - 1)),
- [longs] "0" (n / BYTES_PER_LONG), "a" (0) );
+ : "a" (0) );
clac();
}On 02/09/2024 1:28 pm, Jan Beulich wrote:
> Taking a fault on a non-byte-granular insn means that the "number of
> bytes not handled" return value would need extra care in calculating, if
> we want callers to be able to derive e.g. exception context (to be
> injected to the guest) - CR2 for #PF in particular - from the value. To
> simplify things rather than complicating them, reduce inline assembly to
> just byte-granular string insns. On recent CPUs that's also supposed to
> be more efficient anyway.
>
> For singular element accessors, however, alignment checks are added,
> hence slightly complicating the code. Misaligned (user) buffer accesses
> will now be forwarded to copy_{from,to}_guest_ll().
>
> Naturally copy_{from,to}_unsafe_ll() accessors end up being adjusted the
> same way, as they're produced by mere re-processing of the same code.
> Otoh copy_{from,to}_unsafe() aren't similarly adjusted, but have their
> comments made match reality; down the road we may want to change their
> return types, e.g. to bool.
>
> Fixes: 76974398a63c ("Added user-memory accessing functionality for x86_64")
> Fixes: 7b8c36701d26 ("Introduce clear_user and clear_guest")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
This is definitely simpler, however the code gen less so.
add/remove: 0/0 grow/shrink: 42/9 up/down: 2759/-178 (2581)
> --- a/xen/arch/x86/include/asm/uaccess.h
> +++ b/xen/arch/x86/include/asm/uaccess.h
> @@ -251,7 +251,8 @@ do {
> static always_inline unsigned long
> __copy_to_guest_pv(void __user *to, const void *from, unsigned long n)
> {
> - if (__builtin_constant_p(n)) {
> + if ( __builtin_constant_p(n) && !((unsigned long)to & (n - 1)) )
> + {
The problem is that we now emit this if condition unconditionally,
because the alignment check isn't constant-foldable. This in turn
forces setup for both the trivial case and the function call case,
compounding the bloat.
e.g. the same example:
unsigned int foo(void *ptr)
{
uint16_t val;
return __copy_from_guest_pv(&val, ptr, sizeof(val));
}
now generates:
<foo>:
48 89 f8 mov %rdi,%rax
48 83 ec 08 sub $0x8,%rsp
48 89 fe mov %rdi,%rsi
83 e0 01 and $0x1,%eax
75 31 jne <foo+0x40>
0f 1f 00 nopl (%rax) // STAC
48 89 fa mov %rdi,%rdx
49 b8 ff ff ff ff ff movabs $0xffff87ffffffffff,%r8
87 ff ff
48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
49 39 d0 cmp %rdx,%r8
48 d1 df rcr %rdi
48 21 fa and %rdi,%rdx
66 8b 0a mov (%rdx),%cx
66 89 4c 24 06 mov %cx,0x6(%rsp)
0f 1f 00 nopl (%rax) // CLAC
48 83 c4 08 add $0x8,%rsp
c3 ret
90 nop
48 8d 7c 24 06 lea 0x6(%rsp),%rdi
ba 02 00 00 00 mov $0x2,%edx
e8 11 bc 03 00 call <copy_from_guest_ll>
48 83 c4 08 add $0x8,%rsp
c3 ret
If we're not sure of the alignment in the first place, then it's better
to hand off to copy_*_guest_ll than to emit logic like this.
However, I can't think of any way of doing this without forgoing the
optimisation entirely. We can't base anything on the type of the
guest-side pointer because while a guest expected to align it, there's
no hard requirement. In turn, this means there's about nothing we can
do with compiler heuristics in Xen.
Perhaps the right thing to do is have copy_*_guest_ll have a fastpath
for aligned single accesses, and forgo the inline code generation entirely.
~Andrew
On 02.09.2024 16:13, Andrew Cooper wrote:
> On 02/09/2024 1:28 pm, Jan Beulich wrote:
>> Taking a fault on a non-byte-granular insn means that the "number of
>> bytes not handled" return value would need extra care in calculating, if
>> we want callers to be able to derive e.g. exception context (to be
>> injected to the guest) - CR2 for #PF in particular - from the value. To
>> simplify things rather than complicating them, reduce inline assembly to
>> just byte-granular string insns. On recent CPUs that's also supposed to
>> be more efficient anyway.
>>
>> For singular element accessors, however, alignment checks are added,
>> hence slightly complicating the code. Misaligned (user) buffer accesses
>> will now be forwarded to copy_{from,to}_guest_ll().
>>
>> Naturally copy_{from,to}_unsafe_ll() accessors end up being adjusted the
>> same way, as they're produced by mere re-processing of the same code.
>> Otoh copy_{from,to}_unsafe() aren't similarly adjusted, but have their
>> comments made match reality; down the road we may want to change their
>> return types, e.g. to bool.
>>
>> Fixes: 76974398a63c ("Added user-memory accessing functionality for x86_64")
>> Fixes: 7b8c36701d26 ("Introduce clear_user and clear_guest")
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> This is definitely simpler, however the code gen less so.
>
> add/remove: 0/0 grow/shrink: 42/9 up/down: 2759/-178 (2581)
Not nice, but entirely expected.
>> --- a/xen/arch/x86/include/asm/uaccess.h
>> +++ b/xen/arch/x86/include/asm/uaccess.h
>> @@ -251,7 +251,8 @@ do {
>> static always_inline unsigned long
>> __copy_to_guest_pv(void __user *to, const void *from, unsigned long n)
>> {
>> - if (__builtin_constant_p(n)) {
>> + if ( __builtin_constant_p(n) && !((unsigned long)to & (n - 1)) )
>> + {
>
> The problem is that we now emit this if condition unconditionally,
> because the alignment check isn't constant-foldable. This in turn
> forces setup for both the trivial case and the function call case,
> compounding the bloat.
>
> e.g. the same example:
>
> unsigned int foo(void *ptr)
> {
> uint16_t val;
> return __copy_from_guest_pv(&val, ptr, sizeof(val));
> }
>
> now generates:
>
> <foo>:
> 48 89 f8 mov %rdi,%rax
> 48 83 ec 08 sub $0x8,%rsp
> 48 89 fe mov %rdi,%rsi
> 83 e0 01 and $0x1,%eax
> 75 31 jne <foo+0x40>
> 0f 1f 00 nopl (%rax) // STAC
> 48 89 fa mov %rdi,%rdx
> 49 b8 ff ff ff ff ff movabs $0xffff87ffffffffff,%r8
> 87 ff ff
> 48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
> 49 39 d0 cmp %rdx,%r8
> 48 d1 df rcr %rdi
> 48 21 fa and %rdi,%rdx
> 66 8b 0a mov (%rdx),%cx
> 66 89 4c 24 06 mov %cx,0x6(%rsp)
> 0f 1f 00 nopl (%rax) // CLAC
> 48 83 c4 08 add $0x8,%rsp
> c3 ret
> 90 nop
> 48 8d 7c 24 06 lea 0x6(%rsp),%rdi
> ba 02 00 00 00 mov $0x2,%edx
> e8 11 bc 03 00 call <copy_from_guest_ll>
> 48 83 c4 08 add $0x8,%rsp
> c3 ret
>
>
> If we're not sure of the alignment in the first place, then it's better
> to hand off to copy_*_guest_ll than to emit logic like this.
>
> However, I can't think of any way of doing this without forgoing the
> optimisation entirely. We can't base anything on the type of the
> guest-side pointer because while a guest expected to align it, there's
> no hard requirement. In turn, this means there's about nothing we can
> do with compiler heuristics in Xen.
Well, I'm pretty sure we can retain the optimization, just that to do
so the patch will need to be more intrusive. I'd need to fiddle with
{get,put}_unsafe_asm() to make sure we produce a correct error
indicator (rather than the build-time constants passed into the macro).
Yet that'll still be more code per call site, even if most (all?) new
code would then live in .fixup.
If we went this route, we'd want to settle up front whether for the
to-guest operations we actually need the correct return values. The
problematic uses, after all, are all from-user if I'm not mistaken.
And determining how much of a buffer we can write to (ideally without
actually altering any data) is going to be a little more tricky than a
simple REP LODSB that we could use for read probing. Of course we
could take the position that since we were asked to write to the
buffer, we could as well REP STOSB e.g. zeroes into it.
Obviously there's then the corner case of the buffer becoming
accessible between the 1st and 2nd access attempt ...
Another option might be to actually make the CR2 value visible to the
fixup code, for use in calculating the correct error indicator. (If
the fault isn't #PF, returning the full size is imo good enough in
any event, even if there's the corner case of crossing the canonical-
noncanonical boundary.)
> Perhaps the right thing to do is have copy_*_guest_ll have a fastpath
> for aligned single accesses, and forgo the inline code generation entirely.
I expect that'll still be an increase in code size, plus presumably
some loss of performance (for the added calls and the new conditional).
Jan
On 02/09/2024 4:09 pm, Jan Beulich wrote:
> On 02.09.2024 16:13, Andrew Cooper wrote:
>> On 02/09/2024 1:28 pm, Jan Beulich wrote:
>>> Taking a fault on a non-byte-granular insn means that the "number of
>>> bytes not handled" return value would need extra care in calculating, if
>>> we want callers to be able to derive e.g. exception context (to be
>>> injected to the guest) - CR2 for #PF in particular - from the value. To
>>> simplify things rather than complicating them, reduce inline assembly to
>>> just byte-granular string insns. On recent CPUs that's also supposed to
>>> be more efficient anyway.
>>>
>>> For singular element accessors, however, alignment checks are added,
>>> hence slightly complicating the code. Misaligned (user) buffer accesses
>>> will now be forwarded to copy_{from,to}_guest_ll().
>>>
>>> Naturally copy_{from,to}_unsafe_ll() accessors end up being adjusted the
>>> same way, as they're produced by mere re-processing of the same code.
>>> Otoh copy_{from,to}_unsafe() aren't similarly adjusted, but have their
>>> comments made match reality; down the road we may want to change their
>>> return types, e.g. to bool.
>>>
>>> Fixes: 76974398a63c ("Added user-memory accessing functionality for x86_64")
>>> Fixes: 7b8c36701d26 ("Introduce clear_user and clear_guest")
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> This is definitely simpler, however the code gen less so.
>>
>> add/remove: 0/0 grow/shrink: 42/9 up/down: 2759/-178 (2581)
> Not nice, but entirely expected.
Yes. Having considered this for a while, the usual rule prevails; get
it correct first, fast second.
So, lets go with it like this.
I already want to get rid of .fixup for backtrace reasons, so don't want
to go expanding our use of it.
I'm also not thrilled with the idea of doing a second access. At the
time any fault has occurred, we're delivering an error of some form, and
finding unexpected success the second time around is about the worst
available outcome.
As to your comment about to-guest, that does matter for correct %cr2 on
an emulated store. The INS emulation tries precisely this.
I've got some ad-hoc XTF tests which I'll run this patch over, but I'm
expecting to R-by/T-by in this form.
~Andrew
On 26/09/2024 6:59 pm, Andrew Cooper wrote:
> On 02/09/2024 4:09 pm, Jan Beulich wrote:
>> On 02.09.2024 16:13, Andrew Cooper wrote:
>>> On 02/09/2024 1:28 pm, Jan Beulich wrote:
>>>> Taking a fault on a non-byte-granular insn means that the "number of
>>>> bytes not handled" return value would need extra care in calculating, if
>>>> we want callers to be able to derive e.g. exception context (to be
>>>> injected to the guest) - CR2 for #PF in particular - from the value. To
>>>> simplify things rather than complicating them, reduce inline assembly to
>>>> just byte-granular string insns. On recent CPUs that's also supposed to
>>>> be more efficient anyway.
>>>>
>>>> For singular element accessors, however, alignment checks are added,
>>>> hence slightly complicating the code. Misaligned (user) buffer accesses
>>>> will now be forwarded to copy_{from,to}_guest_ll().
>>>>
>>>> Naturally copy_{from,to}_unsafe_ll() accessors end up being adjusted the
>>>> same way, as they're produced by mere re-processing of the same code.
>>>> Otoh copy_{from,to}_unsafe() aren't similarly adjusted, but have their
>>>> comments made match reality; down the road we may want to change their
>>>> return types, e.g. to bool.
>>>>
>>>> Fixes: 76974398a63c ("Added user-memory accessing functionality for x86_64")
>>>> Fixes: 7b8c36701d26 ("Introduce clear_user and clear_guest")
>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> This is definitely simpler, however the code gen less so.
>>>
>>> add/remove: 0/0 grow/shrink: 42/9 up/down: 2759/-178 (2581)
>> Not nice, but entirely expected.
> Yes. Having considered this for a while, the usual rule prevails; get
> it correct first, fast second.
>
> So, lets go with it like this.
>
> I already want to get rid of .fixup for backtrace reasons, so don't want
> to go expanding our use of it.
>
> I'm also not thrilled with the idea of doing a second access. At the
> time any fault has occurred, we're delivering an error of some form, and
> finding unexpected success the second time around is about the worst
> available outcome.
>
> As to your comment about to-guest, that does matter for correct %cr2 on
> an emulated store. The INS emulation tries precisely this.
>
> I've got some ad-hoc XTF tests which I'll run this patch over, but I'm
> expecting to R-by/T-by in this form.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
See the "x86/pv: Fixes to guest_io_okay()" for the other aspects of this.
~Andrew
© 2016 - 2024 Red Hat, Inc.