Re: [Xen-devel] [PATCH 3/6] x86/shadow: Use ERR_PTR infrastructure for sh_emulate_map_dest()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 16:22, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/06/17 10:07, Jan Beulich wrote:
>>>>> On 22.06.17 at 10:21, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 22/06/2017 09:14, Jan Beulich wrote:
>>>>>>> On 21.06.17 at 17:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> sh_emulate_map_dest() predates the introduction of the generic ERR_PTR()
>>>>> infrasturcture, but take the opportunity to avoid opencoding it.
>>>>>
>>>>> The chosen error constants require need to be negative to work with 
>>> IS_ERR(),
>>>>> but no other changes.
>>>> Drop one of "require" or "need"?
>>>>
>>>>> --- a/xen/arch/x86/mm/shadow/private.h
>>>>> +++ b/xen/arch/x86/mm/shadow/private.h
>>>>> @@ -395,10 +395,9 @@ void shadow_unhook_mappings(struct domain *d, mfn_t 
>>> smfn, int user_only);
>>>>>  
>>>>>  /* Returns a mapped pointer to write to, or one of the following error
>>>>>   * indicators. */
>>>>> -#define MAPPING_UNHANDLEABLE ((void *)(unsigned 
>>>>> long)X86EMUL_UNHANDLEABLE)
>>>>> -#define MAPPING_EXCEPTION    ((void *)(unsigned long)X86EMUL_EXCEPTION)
>>>>> -#define MAPPING_SILENT_FAIL  ((void *)(unsigned long)X86EMUL_OKAY)
>>>>> -#define sh_emulate_map_dest_failed(rc) ((unsigned long)(rc) <= 3)
>>>>> +#define MAPPING_UNHANDLEABLE ERR_PTR(~(long)X86EMUL_UNHANDLEABLE)
>>>>> +#define MAPPING_EXCEPTION    ERR_PTR(~(long)X86EMUL_EXCEPTION)
>>>>> +#define MAPPING_SILENT_FAIL  ERR_PTR(~(long)X86EMUL_OKAY)
>>>> While this doesn't change with your patch, this last definition has a
>>>> string dependency on X86EMUL_OKAY to be zero, yet there's no
>>>> respective BUILD_BUG_ON() anywhere. Would you mind adding
>>>> one at once? In any event
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> There is no longer any dependence on X86EMUL_OKAY being 0 (so far as I
>>> can see).  All this logic would work fine if OKAY had the value 10 for
>>> example.
>> The mapping failure is "silent" only if X86EMUL_OKAY is zero, afaict.
> 
> It is silent because MAPPING_SILENT_FAIL is considered an error by the
> caller (sh_x86_emulate_{write,cmpxchg}()), which unpacks the result
> (turning it into X86EMUL_OKAY) and passes it back into the emulation logic.
> 
> This doesn't depend on the absolute value of X86EMUL_OKAY.

Oh, indeed. R-b then is unconditional.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel