Re: [Xen-devel] [XTF] [PATCH] lib.c: added unsigned 64bits division for 32 bits arch

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/04/18 18:07, Paul Semel wrote:
> this is a simple implementation of unsigned 64bits divisions
> for 32 bits archs.
>
> Signed-off-by: Paul Semel <semelpaul@xxxxxxxxx>
> ---
>  common/lib.c      | 21 +++++++++++++++++++++
>  include/xtf/lib.h |  4 ++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/common/lib.c b/common/lib.c
> index acf4da1..36d5600 100644
> --- a/common/lib.c
> +++ b/common/lib.c

Thanks.  While x86 is the only supported arch, I am trying keep the code
sensibly split between common and arch specific, so when Julien finally
gets around to adding ARM support, it won't require major refactoring. 
(I'm sure there will be some, but I'm trying to keep it to a minimum.)

Please introduce a new arch/x86/lib.c for this function.  See
arch/x86/decode.c as a decent example of the top and tail metadata.

> @@ -68,6 +68,27 @@ int xtf_get_domid(void)
>      return domid;
>  }
>  
> +#if defined(__i386__)
> +uint32_t __udiv64_32(uint64_t *n, uint32_t base)
> +{

A lot of this function would be clearer to read if you had

uint32_t *n_32 = (uint32_t *)n;

at which point, all other casts can be dropped.

> +    uint32_t rem;
> +    uint32_t high = ((uint32_t *)n)[1];
> +
> +    ((uint32_t *)n)[1] = 0;
> +    if (high >= base) {

XTF uses hypervisor style, so this should be

if ( high >= base )
{

> +        ((uint32_t *)n)[1] = high / base;
> +        high %= base;
> +    }
> +
> +    __asm__("divl %2"
> +            : "=a"(((uint32_t*)n)[0]), "=d"(rem)
> +            : "rm"(base), "0"(((uint32_t*)n)[0]), "1"(high));

You don't need the underscore variant of asm in C files at all, and
because XTF is a freestanding project with no libraries, I don't bother
with them in header files either.

For code clarity, please use named asm parameters, rather than
positional ones, so something like:

asm ("div %[base]"
     : "=a" (n_32[0]), "=d" (rem)
     : [base] "rm" (base), "a" (n_32[0]), "d" (high));

> +
> +    return rem;
> +}
> +
> +#endif
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/include/xtf/lib.h b/include/xtf/lib.h
> index abf8f25..4479286 100644
> --- a/include/xtf/lib.h
> +++ b/include/xtf/lib.h
> @@ -101,6 +101,10 @@ int xtf_probe_sysctl_interface_version(void);
>   */
>  int xtf_get_domid(void);
>  
> +#if defined(__i386__)
> +uint32_t __udiv64_32(uint64_t *n, uint32_t base);
> +#endif

Please move to arch/x86/include/arch/lib.h (to match the change in C
file), add a doxygen comment, and drop the guards.

Dropping the guards is a trick to make it far easier to write common
code which is compiler visible and doesn't bitrot as easily, and does
fairly well at spotting unsigned long vs uint32/64 mismatches.  Your
references to this function need to be inside an "if (
IS_DEFINED(CONFIG_32BIT) )" to allow the 64bit builds to link, but the
end result is easier to read.

~Andrew

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