Re: [PATCH] x86/PV: simplify (and thus correct) guest accessor functions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> 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@xxxxxxxxxx>
Tested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

See the "x86/pv: Fixes to guest_io_okay()" for the other aspects of this.

~Andrew