Re: [PATCH v2 01/15] xen/arm: register mmio handler at runtime

[Julien Grall <julien.grall@linaro.org>]

On 04/04/2014 01:30 PM, Vijay Kilari wrote:
> On Fri, Apr 4, 2014 at 5:48 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello vijay,
>>
>> Thank your for the patch.
>>
>> On 04/04/2014 12:56 PM, vijay.kilari@gmail.com wrote:
>>
>> [..]
>>
>>> -#define MMIO_HANDLER_NR ARRAY_SIZE(mmio_handlers)
>>> +static DEFINE_SPINLOCK(handler_lock);
>>
>> Why a global lock rather than a domain lock?
>>
> OK can move into domain io_handler structure

>> [..]
>>
>>> +
>>> +void register_mmio_handler(struct domain *d, struct mmio_handler * handle)
>>> +{
>>> +    unsigned long flags;
>>> +    struct io_handler *handler = d->arch.io_handlers;
>>> +    BUG_ON(handler->num_entries >= MAX_IO_HANDLER);
>>> +
>>> +    spin_lock_irqsave(&handler_lock, flags);
>>
>> You don't need to disable the IRQ here.
>   My first version is without IRQ disable. I remember Andrii suggested it

(Adding Andrii in the mail).

Unless you are planning to use it in interrupt context, IHMO irqsave is
not needed.

>>
>>> +    handler->mmio_handlers[handler->num_entries++] = handle;
>>> +    spin_unlock_irqrestore(&handler_lock, flags);
>>> +}
>>>  /*
>>>   * Local variables:
>>>   * mode: C
>>> diff --git a/xen/arch/arm/io.h b/xen/arch/arm/io.h
>>> index 8d252c0..5fc1660 100644
>>> --- a/xen/arch/arm/io.h
>>> +++ b/xen/arch/arm/io.h
>>
>> As you are modifying this header. Can you move it in include/asm-arm?
> OK.
> 
>>
>>> @@ -40,10 +40,14 @@ struct mmio_handler {
>>>      mmio_write_t write_handler;
>>>  };
>>>
>>> -extern const struct mmio_handler vgic_distr_mmio_handler;
>>> -extern const struct mmio_handler vuart_mmio_handler;
>>> +#define MAX_IO_HANDLER  16
>>> +struct io_handler {
>>> +    int num_entries;
>>> +    struct mmio_handler *mmio_handlers[MAX_IO_HANDLER];
>>> +};
>>>
>>>  extern int handle_mmio(mmio_info_t *info);
>>> +void register_mmio_handler(struct domain *d, struct mmio_handler * handle);
>>
>> As I said on the previous version, it would be nice to remove check
>> callback in the mmio_handler and replace it by addr/size. It's better if
>> we might want to change the place in the memory following the guest.
>>
>> So the result function would be:
>>
>> register_mmio_handler(struct domain *d, read_t read, write_t write,
>> addr, size);
>>
> OK.
>>>
>>>  #endif
>>>
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index 8616534..77b561e 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -35,6 +35,7 @@
>>>  /* Number of ranks of interrupt registers for a domain */
>>>  #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
>>>
>>> +static struct mmio_handler vgic_distr_mmio_handler;
>>>  /*
>>>   * Rank containing GICD_<FOO><n> for GICD_<FOO> with
>>>   * <b>-bits-per-interrupt
>>> @@ -107,6 +108,7 @@ int domain_vgic_init(struct domain *d)
>>>      }
>>>      for (i=0; i<DOMAIN_NR_RANKS(d); i++)
>>>          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
>>> +    register_mmio_handler(d, &vgic_distr_mmio_handler);
>>>      return 0;
>>>  }
>>>
>>> @@ -673,7 +675,7 @@ static int vgic_distr_mmio_check(struct vcpu *v, paddr_t addr)
>>>      return (addr >= (d->arch.vgic.dbase)) && (addr < (d->arch.vgic.dbase + PAGE_SIZE));
>>>  }
>>>
>>> -const struct mmio_handler vgic_distr_mmio_handler = {
>>> +static struct mmio_handler vgic_distr_mmio_handler = {
>>
>> Why did you remove the const?
> 
> I think, compiler is allowing forward declaration with const

You meant doesn't allow? If so, please move the whole structure. I'd
prefer code movement rather than removing a const because of compiler issue.

> 
>>
>> [..]
>>
>>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>>> index 50b9b54..23dac85 100644
>>> --- a/xen/include/asm-arm/domain.h
>>> +++ b/xen/include/asm-arm/domain.h
>>> @@ -116,6 +116,7 @@ struct arch_domain
>>>      struct hvm_domain hvm_domain;
>>>      xen_pfn_t *grant_table_gpfn;
>>>
>>> +    struct io_handler *io_handlers;
>>
>> Why do you need a pointer here? I think can can directly use
>> struct io_handler iohandlers.
>>
>   I just adds to increase the size of this arch_domain struct.
> so allocated memory at runtime.

Do you hit the page size?

Regards,

-- 
Julien Grall