Re: [Xen-devel] [PATCH ARM v3 7/7] mini-os: initial ARM support

[Thomas Leonard <talex5@xxxxxxxxx>]

On 11 June 2014 20:24, Samuel Thibault <samuel.thibault@xxxxxxxxxxxx> wrote:
> Hello,
>
> I don't know the arm hardware very much, so I can't really review the
> arm-specific things, there a just a few things which catched my eyes:
>
> Thomas Leonard, le Wed 11 Jun 2014 11:30:18 +0100, a Ãcrit :
>> +#define LOCK_PREFIX ""
>> +#define LOCK ""
>
> You don't need to define them, they were meant for x86 code.
>
>> +#define ADDR (*(volatile long *) addr)
>
> You aren't using it, no need to define it either.

Right, I've deleted these (and a few other bits that looked unused).

>> +static inline unsigned long __xchg(unsigned long x, volatile void * ptr, 
>> int size)
>> +{
>> +    //TODO
>> +    unsigned volatile long y, tmp = 0;
>> +    switch(size){
>> +    case 1:
>> +#if CPU_EXCLUSIVE_LDST
>> +        __asm__ __volatile__("1:ldrexb %0, [%1]\n"
>> +            "strexb %3, %2, [%1]\n"
>> +            "cmp %3, #1\n"
>> +            "beq 1b\n\n"
>> +            "dmb\n":"=&r"(y):"r"(ptr), "r"(x), "r"(tmp):"memory");
>> +#else
>> +        y = (*(char *)ptr) & 0x000000ff;
>> +        *((char *)ptr) = (char)x;
>
> The second version is very fragile :) I'd rather put a strong #warning
> here, to make sure nobody ever uses that code for anything close to
> production level.
>
>
>> +static __inline__ int test_and_clear_bit(int nr, volatile void * addr)
>> +{
>> +    //TODO
>> +    unsigned long *tmp = (unsigned long *)addr;
>> +
>> +    int x = tmp[nr >> 5] & (1 << (nr & 0x1f));
>> +    tmp[nr >> 5] &= ~(1 << (nr & 0x1f));
>> +    return x;
>> +}
>
> And this must be atomic, so an atomic version is really needed.
>
> I guess we could perhaps revert to gcc instrinsics by default to save
> porting work here.

I've had a go at this (attached).

I'm not sure what the difference between the synch and non-synch
versions is supposed to be. Currently, I'm just doing:

#define synch_set_bit(n, addr) set_bit(n, addr)
#define synch_clear_bit(n, addr) clear_bit(n, addr)
#define synch_test_and_set_bit(n, addr) test_and_set_bit(n, addr)
#define synch_test_and_clear_bit(n, addr) test_and_clear_bit(n, addr)
#define synch_test_bit(nr, addr) test_bit(nr, addr)

I used __ATOMIC_SEQ_CST as the memory model as that was the safest
option, but maybe it's overkill.

I also modified __ffs to use ARM's clz instruction, removing another TODO.

The main user of these operations was gic.c, but it turns out that the
atomic versions don't work here as the GIC registers aren't regular
memory. So I replaced those with non-atomic versions.

Does this seem reasonable?

>> +static inline unsigned long __synch_cmpxchg(volatile void *ptr,
>> +        unsigned long old,
>> +        unsigned long new, int size)
>> +{
>> +    //TODO:
>> +    //BUG();
>> +    return 0;
>> +}
>> +
>> +static __inline__ void synch_set_bit(int nr, volatile void * addr)
>> +{
>> +    //TODO:
>> +    set_bit(nr, addr);
>> +}
>
> Same story for these
>
>
> Apart from these, the integration into the existing mini-os looks nice.
>
> Samuel



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

Attachment: os.h
Description: Text Data

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