Re: [Xen-devel] [PATCH v4 1/3] libxl/cpumap: Add xc_cpumap_[setcpu, clearcpu, testcpu] to complement xc_cpumap_alloc.

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-04-03 at 20:43 -0400, Konrad Rzeszutek Wilk wrote:
> +/*
> + * xc_bitops.h has macros that do this as well - however xc_cpumap_t
> + * is an array of uint8 (1byte) while said macros expect an array
> + * of unsigned long (8 bytes). This is not a problem when setting
> + * an bit (as the computation will come with the same offset) - the
> + * issue is when clearing an bit. The aligment on ARM could affect
> + * other structures that are close in memory causing memory corruption.
> + * Hence our own macros that differ only in that we compute the
> + * size of the array elements (sizeof(*map)) as opposed to assuming
> + * it is unsigned long.

I don't think that it is at all.

The issue is that when compiling a function which takes, say, an
unsigned long * the compiler is entitled (via the calling conventions,
ABI standards etc) to assume that the pointer will be correctly aligned
for an unsigned long type and can therefore emit instructions which
assume that (i.e. on ARM a ldr instruction assumes a 4 byte aligned
pointer and can trap if that isn't true).

On the calling side the things normally work out, because when using the
& operator on a variable the compiler already arranges that it is
appropriately aligned and because the caller also knows that it needs to
follow the calling conventions etc.

The issue with your previous patch was that you were casting a uint8_t *
to an unsigned long *, which was effectively telling the compiler "I
know what I'm doing and this is all completely fine" -- which if the
uint8_t* wasn't 4 byte aligned (which it isn't required to be by the
calling conventions etc AFAIK) isn't true and would violate the
assumptions made by the compiler in the called function.

All this affects test and set equally, and it has nothing to do with
memory corruption.

Does that make sense?

I don't think you need to go into so much detail in the comment, you can
just say that xc_bitops.h assumes that the bitmask is word aligned but
that cpumaps are only guaranteed to be byte aligned and so we need byte
versions for architectures which do not support misaligned accesses
(which is basically everyone but x86, although even on x86 it can be
inefficient).

> + */
> +#define BITS_PER_CPUMAP(map) (sizeof(*map) * 8)
> +#define CPUMAP_ENTRY(cpu, map) ((map))[(cpu) / BITS_PER_CPUMAP(map)]
> +#define CPUMAP_SHIFT(cpu, map) ((cpu) % BITS_PER_CPUMAP(map))
> +void xc_cpumap_clearcpu(int cpu, xc_cpumap_t map)
> +{
> +    CPUMAP_ENTRY(cpu, map) &= ~(1U << CPUMAP_SHIFT(cpu, map));
> +}
> +
> +void xc_cpumap_setcpu(int cpu, xc_cpumap_t map)
> +{
> +    CPUMAP_ENTRY(cpu, map) |= (1U << CPUMAP_SHIFT(cpu, map));
> +}
> +
> +int xc_cpumap_testcpu(int cpu, xc_cpumap_t map)
> +{
> +    return (CPUMAP_ENTRY(cpu, map) >> CPUMAP_SHIFT(cpu, map)) & 1;
> +}
> +
>  xc_nodemap_t xc_nodemap_alloc(xc_interface *xch)
>  {
>      int sz;



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