On 2/26/19 9:17 AM, Alexandru Stefan ISAILA wrote: > Ping. Is this ok with you, George?
Sorry -- somehow I thought I'd responded to this, but apparently not. > On 17.01.2019 11:06, Alexandru Stefan ISAILA wrote: >> Changed the return value of 1 to 0 so now p2m_finish_type_change returns >> 0 for success or <0 for error. >> The “root” caller of p2m_finish_type_change() is >> XEN_DMOP_map_mem_type_to_ioreq_server and this does nothing useful with >> positive values. I realize you're just copying what I said in the email where I suggested this, but as Jan pointed out in v1, commit message needs to be more thorough than an off-hand comment during review. I also agree with him that it makes more sense to 'sanitize' ept's recalc_entry() return value in finish_type_change(), rather than at p2m_finish_type_change(). I think it's probably good to get feedback from Paul, who wrote this interface (IIRC) and works with one of its main users (QEMU). My normal 'template' is "Situation / Problem / Solution": What's the current situation, why is that a problem, and how does this patch fix it. With that in mind (and also bringing Paul up to speed), I think I'd say something like this: --- 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(). --- Thoughts? -George _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
