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

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

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?

[..]

> +
> +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.

> +    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?

> @@ -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);

>  
>  #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?

[..]

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

Regards,

-- 
Julien Grall