Re: [Xen-devel] [PATCH ARM v4 07/12] mini-os: initial ARM support

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 06/23/2014 04:10 PM, Thomas Leonard wrote:
> On 18 June 2014 23:40, Julien Grall <julien.grall@xxxxxxxxxx> wrote: 
> Hi Julien,

Hi Thomas,

>>
>>
>>> +void set_vtimer_compare(uint64_t value) {
>>> +    uint32_t x, y;
>>> +
>>> +    DEBUG("New CompareValue : %llx\n", value);
>>> +    x = 0xFFFFFFFFULL & value;
>>> +    y = (value >> 32) & 0xFFFFFFFF;
>>> +
>>> +    __asm__ __volatile__("mcrr p15, 3, %0, %1, c14\n"
>>> +            "isb"::"r"(x), "r"(y));
>>> +
>>> +    __asm__ __volatile__("mov %0, #0x1\n"
>>> +            "mcr p15, 0, %0, c14, c3, 1\n" /* Enable timer and unmask the
>>> output signal */
>>> +            "isb":"=r"(x));
>>
>>
>> I don't think you need to add an isb per instruction. One should be enough.
> 
> Actually, do we need one at all? Setting the timer shouldn't affect
> the instruction pipline, I think.

The isb is also used to make sure that an access to a system control
registers (such as setting the page table) has finished before executing
the next instruction.

>> [..]
>>
>>
>>> diff --git a/extras/mini-os/drivers/gic.c b/extras/mini-os/drivers/gic.c
>>> new file mode 100644
>>> index 0000000..3141830
>>> --- /dev/null
>>> +++ b/extras/mini-os/drivers/gic.c
>>> @@ -0,0 +1,187 @@
>>> +// ARM GIC implementation
>>> +
>>> +#include <mini-os/os.h>
>>> +#include <mini-os/hypervisor.h>
>>> +
>>> +//#define VGIC_DEBUG
>>> +#ifdef VGIC_DEBUG
>>> +#define DEBUG(_f, _a...) \
>>> +    DEBUG("MINI_OS(file=vgic.c, line=%d) " _f , __LINE__, ## _a)
>>> +#else
>>> +#define DEBUG(_f, _a...)    ((void)0)
>>> +#endif
>>> +
>>> +extern void (*IRQ_handler)(void);
>>> +
>>> +struct gic {
>>> +    volatile char *gicd_base;
>>> +    volatile char *gicc_base;
>>> +};
>>> +
>>> +static struct gic gic;
>>> +
>>> +// Distributor Interface
>>> +#define GICD_CTLR        0x0
>>> +#define GICD_ISENABLER    0x100
>>> +#define GICD_PRIORITY    0x400
>>
>>
>> You use the right name for every macro except this one. I would rename it to
>> GICD_IPRIORITYR to stay consistent.
> 
> Done.

Thanks !

>>> +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int
>>> value)
>>> +{
>>> +    __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr));
>>> +    wmb();
>>> +}
>>> +
> 
> Do we need inline assembler to write these values? I'd imagine that a
> plain C store to a volatile 32-bit pointer would always do the same
> thing.

The compiler may do strange things to store the value, such as 4
byte-store instead of a word-store.

So we have to use inline assembly to make sure the correct access will
be done.

>>> +{
>>> +    uint32_t value;
>>> +    value = REG_READ32(REG(gicd(gic, GICD_PRIORITY)) + irq_number);
>>
>>
>> There is 4 interrupts describe per register, this should be (irq_number &
>> ~3).
> 
> Or should it be (irq_number >> 2)? REG casts to uint32_t.

It's the same :).

>>> +}
>>> +
>>> +static void gic_route_interrupt(struct gic *gic, unsigned char
>>> irq_number, unsigned char cpu_set)
>>> +{
>>> +    uint32_t value;
>>> +    value = REG_READ32(REG(gicd(gic, GICD_ITARGETSR)) + irq_number);
>>
>>
>> Same remark as gic_set_priority here.
>>
>>
>>> +    value &= ~(0xff << (8 * (irq_number & 0x3))); // set priority to '0'
>>> +    value |= cpu_set << (8 * (irq_number & 0x3)); // add our priority
>>
>>
>> The comments are wrong in the 2 lines above.
>>
>>
>>> +    REG_WRITE32(REG(gicd(gic, GICD_ITARGETSR)) + irq_number, value);
>>
>>
>> irq_number & ~3;
> 
> The ARM Generic Interrupt Controller Architecture Specification says
> "These registers are byte-accessible.". So we should be able to write
> the byte directly. I thought this might work:
> 
> static void gic_route_interrupt(struct gic *gic, int irq_number,
> unsigned char cpu_set)
> {
>     gicd(gic, GICD_ITARGETSR)[irq_number] = cpu_set;
>     wmb();
> }

This code looks good to me.

> 
> But it doesn't, because Xen's write_ignore handler (vgic.c:656 on the
> 4.4 branch) does:
> 
> write_ignore:
>   if ( dabt.size != 2 ) goto bad_width;
> 
> Bug?

This is a bug in Xen. I think, write_ignore should just ignore the write
rather checking badly the size.

Can you send a patch for it?

>>> +}
>>> +
>>> +#define local_irq_save(x) { \
>>> +    __asm__ __volatile__("mrs %0, cpsr;cpsid i; and %0, %0,
>>> #0x80":"=r"(x)::"memory");    \
>>> +}
>>> +
>>> +#define local_irq_restore(x) {    \
>>> +    __asm__ __volatile__("msr cpsr_c, %0"::"r"(x):"memory");    \
>>> +}
>>
>>
>> If you mask the result in local_irq_save, and use this value directly is
>> local_irq_restore, you will disable by mistake the Asynchronous Abort
>> exception....
>>
>> You have to remove the and %0... at the end.
> 
> This makes schedule() fail, because it checks whether the result is zero.
> Perhaps we should continue to mask it, but only apply the IRQ bit in
> local_irq_restore?

I would prefer to change the schedule code if it's possible to keep
local_irq_restore as simple as possible. This helper is often used.

I don't know much the code of schedule. Why don't we only check that the
IRQ are disabled rather than call local_irq_{save,restore}?

>>> +#define local_irq_disable()    __cli()
>>> +#define local_irq_enable() __sti()
>>> +
>>> +#if defined(__arm__)
>>> +#define mb() __asm__("dmb");
>>> +#define rmb() __asm__("dmb");
>>> +#define wmb() __asm__("dmb");
>>
>>
>> I suspect you want to dsb here. You want to make sure that every memory
>> access has finished avoid executing new instruction until this is done.
> 
> Is this necessary? As I understand it, using Xen's ring system
> requires a strict ordering (dmb), but we don't need to stall the
> processor waiting for each write to complete before going on to the
> next one.

Is it written somewhere? FIY, Linux is using dsb with ring system. Even
tho, I think dmb is fine here. I'd like confirmation for either Ian C.
or Stefano.

Futhermore, there is other place in mini-os (such as the GIC) where I
think we have to use dsb.

>>> +#define unlikely(x)  __builtin_expect((x),0)
>>> +#define likely(x)  __builtin_expect((x),1)
>>
>>
>> Can't it be common with x86?
> 
> Which header file should it go in?

I don't know much mini-os, but I would add either in lib.h or create a
new header compiler.h.

>> [..]
>>
>>
>>> +#if defined(__i386__) || defined(__arm__)
>>>  typedef long long           quad_t;
>>>  typedef unsigned long long  u_quad_t;
>>>
>>> @@ -40,7 +40,7 @@ typedef unsigned long       u_quad_t;
>>>  typedef struct { unsigned long pte; } pte_t;
>>>  #endif /* __i386__ || __x86_64__ */
>>>
>>> -#ifdef __x86_64__
>>> +#if defined(__x86_64__) || defined(__aarch64__)
>>>  #define __pte(x) ((pte_t) { (x) } )
>>
>>
>> This looks wrong to me. The pte layout is exactly the same on arm32 and
>> arm64. Hence, this is only used in the x86 code. Please either move pte_t in
>> x86 part or define it correctly on ARM...
> 
> Moved to x86. Actually, this simplifies things further, because it can
> go in the separate hypercall-*.h files and lose the #ifdef completely.

Great, thanks!

Regards,

-- 
Julien Grall

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