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.

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).

--- 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
 #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)

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

On 08.10.2021 19:47, Andrew Cooper wrote:
> 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.

But the XTF would have used the modern hypercall, wouldn't it? At
least the pv-iopl test does.

>> 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.

You're right - I think I did look at do_physdev_op_compat() deriving
evtchn-compat behavior from there as well.

>> --- 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.

Oh, indeed - thanks for pointing out.

Jan