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
