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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Fri, Apr 4, 2014 at 5:48 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello vijay,
>
> Thank your for the patch.
>
> On 04/04/2014 12:56 PM, vijay.kilari@xxxxxxxxx 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
>
>> +    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

>
> [..]
>
>> 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.

> Regards,
>
> --
> Julien Grall

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