Re: [PATCH] x86/PV32: fix physdev_op_compat handling

[Andrew Cooper <andrew.cooper3@citrix.com>]

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 <jbeulich@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)
>