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

2017-07-03 Thread Jan Beulich 
>>> On 03.07.17 at 16:22,  wrote:
> On 22/06/17 10:07, Jan Beulich wrote:
> On 22.06.17 at 10:21,  wrote:
>>> On 22/06/2017 09:14, Jan Beulich wrote:
>>> On 21.06.17 at 17:12,  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_EXCEPTIONERR_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 
>>> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

2017-07-03 Thread Andrew Cooper 
On 22/06/17 10:07, Jan Beulich wrote:
 On 22.06.17 at 10:21,  wrote:
>> On 22/06/2017 09:14, Jan Beulich wrote:
>> On 21.06.17 at 17:12,  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_EXCEPTIONERR_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 
>> 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.

>
>> The only requirement (now) is that the X86EMUL constants are all within
>> MAX_ERRNO so they count as errors as far as the ERR_PTR logic is
>> concerned, and I suppose those should gain BUILD_BUG_ON()s
> That would be helpful too.

Its sadly not possible, as ERR_PTR() is a static inline and can't be
evaluated in the context of BUILD_BUG_ON().

The best which is possible (without altering the errptr infrastructure)
is BUILD_BUG_ON(IS_ERR_VALUE(~(long)X86EMUL_*)), but this won't catch a
change which redefines the MAPPING_* constants.

~Andrew

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

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

2017-06-22 Thread Andrew Cooper 
On 22/06/17 13:09, Tim Deegan wrote:
> Hi,
>
> At 16:12 +0100 on 21 Jun (1498061549), Andrew Cooper wrote:
>> sh_emulate_map_dest() predates the introduction of the generic ERR_PTR()
>> infrasturcture, but take the opportunity to avoid opencoding it.
> s/sturct/struct/.

D'oh - I'm sure you spotted this before.  I will try to actually fix it
this time.

>
>> @@ -4752,8 +4752,8 @@ sh_x86_emulate_write(struct vcpu *v, unsigned long 
>> vaddr, void *src,
>>  return X86EMUL_UNHANDLEABLE;
>>  
>>  addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
>> -if ( sh_emulate_map_dest_failed(addr) )
>> -return (long)addr;
>> +if ( IS_ERR(addr) )
>> +return ~PTR_ERR(addr);
> Using "return ~PTR_ERR(addr)" when the usual idiom is
> "return -PTR_ERR(foo)" is a bit subtle.  Still, the code seems to be
> correct, so if people prefer it,
>
> Acked-by: Tim Deegan 

It is necessary to use ~ rather than - because of MAPPING_SILENT_FAIL
being X86EMUL_OKAY under the hood.

Short of sliding the X86EMUL_* constants along by 1 (which itself would
cause other chaos, as there are plenty of return value checks against 0
rather than X86EMUL_OKAY), I can't see any other way of making this
function.

All in all, write-discard p2m types are a pain to work with :(

~Andrew

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

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

2017-06-22 Thread Tim Deegan 
Hi,

At 16:12 +0100 on 21 Jun (1498061549), Andrew Cooper wrote:
> sh_emulate_map_dest() predates the introduction of the generic ERR_PTR()
> infrasturcture, but take the opportunity to avoid opencoding it.

s/sturct/struct/.

> @@ -4752,8 +4752,8 @@ sh_x86_emulate_write(struct vcpu *v, unsigned long 
> vaddr, void *src,
>  return X86EMUL_UNHANDLEABLE;
>  
>  addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
> -if ( sh_emulate_map_dest_failed(addr) )
> -return (long)addr;
> +if ( IS_ERR(addr) )
> +return ~PTR_ERR(addr);

Using "return ~PTR_ERR(addr)" when the usual idiom is
"return -PTR_ERR(foo)" is a bit subtle.  Still, the code seems to be
correct, so if people prefer it,

Acked-by: Tim Deegan 

Cheers,

Tim.


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

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

2017-06-22 Thread Jan Beulich 
>>> On 22.06.17 at 10:21,  wrote:
> On 22/06/2017 09:14, Jan Beulich wrote:
> On 21.06.17 at 17:12,  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_EXCEPTIONERR_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 
> 
> 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.

> The only requirement (now) is that the X86EMUL constants are all within
> MAX_ERRNO so they count as errors as far as the ERR_PTR logic is
> concerned, and I suppose those should gain BUILD_BUG_ON()s

That would be helpful too.

Jan


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

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

2017-06-22 Thread Andrew Cooper 
On 22/06/2017 09:14, Jan Beulich wrote:
 On 21.06.17 at 17:12,  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_EXCEPTIONERR_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 

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 only requirement (now) is that the X86EMUL constants are all within
MAX_ERRNO so they count as errors as far as the ERR_PTR logic is
concerned, and I suppose those should gain BUILD_BUG_ON()s

~Andrew

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

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

2017-06-22 Thread Jan Beulich 
>>> On 21.06.17 at 17:12,  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_EXCEPTIONERR_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 

Jan


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

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

2017-06-21 Thread Andrew Cooper 
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.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Tim Deegan 

v2:
 * Use ~(long)X86EMUL rather than -X86EMUL so MAPPING_SILENT_FAIL is
   considered an error to IS_ERR()
---
 xen/arch/x86/mm/shadow/multi.c   | 8 
 xen/arch/x86/mm/shadow/private.h | 7 +++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index f65ffc6..74c3641 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4752,8 +4752,8 @@ sh_x86_emulate_write(struct vcpu *v, unsigned long vaddr, 
void *src,
 return X86EMUL_UNHANDLEABLE;
 
 addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
-if ( sh_emulate_map_dest_failed(addr) )
-return (long)addr;
+if ( IS_ERR(addr) )
+return ~PTR_ERR(addr);
 
 paging_lock(v->domain);
 memcpy(addr, src, bytes);
@@ -4794,8 +4794,8 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, unsigned long 
vaddr,
 return X86EMUL_UNHANDLEABLE;
 
 addr = sh_emulate_map_dest(v, vaddr, bytes, sh_ctxt);
-if ( sh_emulate_map_dest_failed(addr) )
-return (long)addr;
+if ( IS_ERR(addr) )
+return ~PTR_ERR(addr);
 
 paging_lock(v->domain);
 switch ( bytes )
diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index 472676c..a59ff7a 100644
--- 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_EXCEPTIONERR_PTR(~(long)X86EMUL_EXCEPTION)
+#define MAPPING_SILENT_FAIL  ERR_PTR(~(long)X86EMUL_OKAY)
 void *sh_emulate_map_dest(struct vcpu *v, unsigned long vaddr,
   unsigned int bytes, struct sh_emulate_ctxt *sh_ctxt);
 void sh_emulate_unmap_dest(struct vcpu *v, void *addr, unsigned int bytes,
-- 
2.1.4


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