Re: [Xen-devel] [PATCH v3] x86/mm: Clean up p2m_finish_type_change return value

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.03.19 at 10:09, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Alexandru Stefan ISAILA [mailto:aisaila@xxxxxxxxxxxxxxx]
>> Sent: 22 March 2019 12:09
>> 
>> In the case of any errors, finish_type_change() passes values returned
>> from p2m->recalc() up the stack (with some exceptions in the case where
>> an error is expected); this eventually ends up being returned to the
>> XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.
>> 
>> However, on Intel processors (but not on AMD processor), p2m->recalc()
>> can also return '1' as well as '0'.  This case is handled very
>> inconsistently: finish_type_change() will return the value of the final
>> entry it attempts, discarding results for other entries;
>> p2m_finish_type_change() will attempt to accumulate '1's, so that it
>> returns '1' if any of the calls to finish_type_change() returns '1'; and
>> dm_op() will again return '1' only if the very last call to
>> p2m_finish_type_change() returns '1'.  The result is that the
>> XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
>> 0 and sometimes return 1 on success, in an unpredictable manner.
>> 
>> The hypercall documentation doesn't mention return values; but it's not
>> clear what the caller could do with the information about whether
>> entries had been changed or not.  At the moment it's always 0 on AMD
>> boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
>> '1' return value for correctness (or if it is, it's broken).
>> 
>> Make the return value on success consistently '0' by only returning
>> 0/-ERROR from finish_type_change().  Also remove the accumulation code
>> from p2m_finish_type_change().
> 
> Sorry, I don't think I was cc-ed on the original and I managed to miss George 
> cc-ing me on his response to v2. Digging into the code a bit I can't honestly 
> see what the point of returning anything other than 0/-errno out of 
> p2m->recalc 
> is. The only use of rc > 0 from p2m-ept.c:resolve_misconfig() is in 
> ept_handle_misconfig() AFAICT so it would make more sense to me to tighten up 
> the semantics of recalc (which I believe Jan suggested in response to v1) and 
> turn any > 0 return from resolve_misconfig() into 0. So, the code below looks 
> fine but the patch just needs to do a little more (and then your rc < 0 test 
> can also be simplified to !rc too).

Yes, indeed I'd prefer everything to be cleaned up in one go.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel