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

[Jan Beulich <jbeulich@xxxxxxxx>]

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