From: Luke Shumaker <lukeshu@parabola.nu>

Instead of defining a bogus validate_guest_space that always returns 1 on
targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
targets.  This makes the "normal" flow control clearer.

Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
 linux-user/elfload.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 20f3d8c2c3..cac991159c 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -354,7 +354,6 @@ enum {
 
 /* The commpage only exists for 32 bit kernels */
 
-#define TARGET_HAS_VALIDATE_GUEST_SPACE
 /* Return 1 if the proposed guest space is suitable for the guest.
  * Return 0 if the proposed guest space isn't suitable, but another
  * address space should be tried.
@@ -1823,15 +1822,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
     return sp;
 }
 
-#ifndef TARGET_HAS_VALIDATE_GUEST_SPACE
-/* If the guest doesn't have a validation function just agree */
-static int validate_guest_space(unsigned long guest_base,
-                                unsigned long guest_size)
-{
-    return 1;
-}
-#endif
-
 unsigned long init_guest_space(unsigned long host_start,
                                unsigned long host_size,
                                unsigned long guest_start,
@@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
     /* If just a starting address is given, then just verify that
      * address.  */
     if (host_start && !host_size) {
+#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
         if (validate_guest_space(host_start, host_size) == 1) {
-            return host_start;
-        } else {
             return (unsigned long)-1;
         }
+#endif
+        return host_start;
     }
 
     /* Setup the initial flags and start address.  */
@@ -1888,6 +1879,8 @@ unsigned long init_guest_space(unsigned long host_start,
 
         /* Check to see if the address is valid.  */
         if (!host_start || real_start == current_start) {
+#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
+            /* On 32-bit ARM, we need to also be able to map the commpage.  */
             int valid = validate_guest_space(real_start - guest_start,
                                              real_size);
             if (valid == 1) {
@@ -1896,6 +1889,10 @@ unsigned long init_guest_space(unsigned long host_start,
                 return (unsigned long)-1;
             }
             /* valid == 0, so try again. */
+#else
+            /* On other architectures, whatever we have here is fine.  */
+            break;
+#endif
         }
 
         /* That address didn't work.  Unmap and try a different one.
-- 
2.15.1

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of defining a bogus validate_guest_space that always returns 1 on
> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
> targets.  This makes the "normal" flow control clearer.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 20f3d8c2c3..cac991159c 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -354,7 +354,6 @@ enum {
>
>  /* The commpage only exists for 32 bit kernels */
>
> -#define TARGET_HAS_VALIDATE_GUEST_SPACE
>  /* Return 1 if the proposed guest space is suitable for the guest.
>   * Return 0 if the proposed guest space isn't suitable, but another
>   * address space should be tried.
> @@ -1823,15 +1822,6 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
>      return sp;
>  }
>
> -#ifndef TARGET_HAS_VALIDATE_GUEST_SPACE
> -/* If the guest doesn't have a validation function just agree */
> -static int validate_guest_space(unsigned long guest_base,
> -                                unsigned long guest_size)
> -{
> -    return 1;
> -}
> -#endif
> -
>  unsigned long init_guest_space(unsigned long host_start,
>                                 unsigned long host_size,
>                                 unsigned long guest_start,
> @@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
>      /* If just a starting address is given, then just verify that
>       * address.  */
>      if (host_start && !host_size) {
> +#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)

I would strongly prefer us not to add new "these targets do
this" ifdefs, please. The current approach means that any
target can say it needs an implementation of this hook by
providing one and defining the TARGET_HAS_VALIDATE_GUEST_SPACE
macro to say so. I think that's a better approach.

thanks
-- PMM

On 23 February 2018 at 18:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
>> From: Luke Shumaker <lukeshu@parabola.nu>
>>
>> Instead of defining a bogus validate_guest_space that always returns 1 on
>> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
>> targets.  This makes the "normal" flow control clearer.
>>
>> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>

>> @@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
>>      /* If just a starting address is given, then just verify that
>>       * address.  */
>>      if (host_start && !host_size) {
>> +#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>
> I would strongly prefer us not to add new "these targets do
> this" ifdefs, please. The current approach means that any
> target can say it needs an implementation of this hook by
> providing one and defining the TARGET_HAS_VALIDATE_GUEST_SPACE
> macro to say so. I think that's a better approach.

Looking through some of the rest of this patchset I might change
my mind on that (the code in master is very confusing). I won't
have time to get to this til Tuesday now, though.

thanks
-- PMM

On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of defining a bogus validate_guest_space that always returns 1 on
> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
> targets.  This makes the "normal" flow control clearer.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>

Yes, I think on reflection that you're right and we're better off
leaving this arm-specific. If/when we find another target that needs
special handling, we can look at a more generic API then.

> @@ -1845,11 +1835,12 @@ unsigned long init_guest_space(unsigned long host_start,
>      /* If just a starting address is given, then just verify that
>       * address.  */
>      if (host_start && !host_size) {
> +#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>          if (validate_guest_space(host_start, host_size) == 1) {
> -            return host_start;
> -        } else {
>              return (unsigned long)-1;
>          }
> +#endif
> +        return host_start;

This is accidentally changing behaviour -- if validate_guest_space()
returned 1 we used to return host_start, and now we return -1.
The condition should be "!= 1". This change is made in patch 2,
but it should be done here, to avoid breaking bisection.

Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Instead of defining a bogus validate_guest_space that always returns 1 on
> targets other than 32-bit ARM, use #if blocks to only call it on 32-bit ARM
> targets.  This makes the "normal" flow control clearer.
> 
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
>  linux-user/elfload.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)

With the change request by Peter (condition "!= 1"),
applied to my 'linux-user-for-2.12' branch.

Thanks,
Laurent