On 08/10/2021 11:47, Jan Beulich wrote:
> The conversion of the original code failed to recognize that the 32-bit
> compat variant of this (sorry, two different meanings of "compat" here)
> needs to continue to invoke the compat handler, not the native one.
> Arrange for this and also remove the one #define that hasn't been
> necessary anymore as of that change.
>
> Affected functions (having existed prior to the introduction of the new
> hypercall) are PHYSDEVOP_set_iobitmap and PHYSDEVOP_apic_{read,write}.
> For all others the operand struct layout doesn't differ.
:-/
Neither of those ABI breakages would be subtle. But why didn't XTF
notice? Edit: It appears as if my PHYSDEVOP_set_iobitmap tests never
got completed.
>
> Fixes: 1252e2823117 ("x86/pv: Export pv_hypercall_table[] rather than working
> around it in several ways")
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> ---
> Additionally the XSA-344 fix causes guest register corruption afaict,
> when EVTCHNOP_reset gets called through the compat function and needs a
> continuation. While guests shouldn't invoke that function this way, I
> think we would better have forced all pre-3.2-unavailable functions into
> an error path, rather than forwarding them to the actual handler. I'm
> not sure though how relevant we consider it to fix this (one way or
> another).
EVTCHNOP_reset{,_cont} are -ENOSYS'd in do_event_channel_op_compat()
without being forwarded. I can't see a problem.
But yes - we'd have problems if any pre-3.2-available functions needed
to become continuable.
We ought to consider dropping compatibility for guests that obsolete...
>
> --- a/xen/arch/x86/x86_64/compat.c
> +++ b/xen/arch/x86/x86_64/compat.c
> @@ -10,8 +10,8 @@ EMIT_FILE;
>
> #define physdev_op compat_physdev_op
> #define physdev_op_t physdev_op_compat_t
> -#define do_physdev_op compat_physdev_op
This is still needed, technically. It impacts the typeof() expression:
typeof(do_physdev_op) *fn =
(void *)pv_hypercall_table[__HYPERVISOR_physdev_op].native;
and the reason why everything compiles is because
{do,compat}_physdev_op() have identical types.
~Andrew
> #define do_physdev_op_compat(x) compat_physdev_op_compat(_##x)
> +#define native compat
>
> #define COMPAT
> #define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
>
