Re: [PATCH v3 02/16] xen/arm: make mmio handlers domain specific

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

Hello Vijaya,

Thank your for the patch.

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index ada1918..1a0be89 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -18,29 +18,47 @@
>  
>  #include <xen/config.h>
>  #include <xen/lib.h>
> +#include <xen/spinlock.h>
> +#include <xen/sched.h>
>  #include <asm/current.h>
>  #include <asm/mmio.h>
>  
> -static const struct mmio_handler *const mmio_handlers[] =
> -{
> -    &vgic_distr_mmio_handler,
> -    &vuart_mmio_handler,
> -};
> -#define MMIO_HANDLER_NR ARRAY_SIZE(mmio_handlers)
> -
>  int handle_mmio(mmio_info_t *info)
>  {
>      struct vcpu *v = current;
>      int i;
> +    struct io_handler *mmio_handle = &v->domain->arch.io_handlers;
>  
> -    for ( i = 0; i < MMIO_HANDLER_NR; i++ )
> -        if ( mmio_handlers[i]->check_handler(v, info->gpa) )
> +    for ( i = 0; i < mmio_handle->num_entries; i++ )
> +    {
> +        if ( (info->gpa >= mmio_handle->mmio_handlers[i]->addr) &&
> +              info->gpa < (mmio_handle->mmio_handlers[i]->addr + mmio_handle->mmio_handlers[i]->size) )

The coding style request line length below 80 characters.

>              return info->dabt.write ?
> -                mmio_handlers[i]->write_handler(v, info) :
> -                mmio_handlers[i]->read_handler(v, info);
> -
> +                mmio_handle->mmio_handlers[i]->write_handler(v, info) :
> +                mmio_handle->mmio_handlers[i]->read_handler(v, info);
> +    }

Newline here for clarity.

>      return 0;
>  }
> +
> +void register_mmio_handler(struct domain *d, struct mmio_handler * handle)
> +{
> +    struct io_handler *handler = &d->arch.io_handlers;
> +
> +    BUG_ON(handler->num_entries >= MAX_IO_HANDLER);
> +
> +    spin_lock(&handler->lock);
> +    handler->mmio_handlers[handler->num_entries] = handle;
> +    handler->num_entries++;

If you plan to use spinlock only when a new entry is added, you need a
dsb(is) before increment num_entries. Smth like

handler->mmio_handlers[handler->num_entries] = handle;
dbs(sy);
handler->num_entries++;

> +    spin_unlock(&handler->lock);
> +}
> +
> +int domain_io_init(struct domain *d)
> +{
> +   spin_lock_init(&d->arch.io_handlers.lock);
> +   d->arch.io_handlers.num_entries = 0;

Newline here for clarity.

> +   return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 9c404fe..d36058a 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -35,6 +35,8 @@
>  /* 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
> @@ -98,6 +100,10 @@ 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);
> +
> +    vgic_distr_mmio_handler.addr = d->arch.vgic.dbase;
> +    vgic_distr_mmio_handler.size = PAGE_SIZE;
> +    register_mmio_handler(d, &vgic_distr_mmio_handler);

This is wrong. register_mmio_handler is directly copying the MMIO
handler. So updating addr will breaks other domains as the vgic.dbase
may differ.

I think my solution on V1 was correct. I.e smth like

register_mmio_handler(d, &vgic_distr_mmio_handler, address, size);

And your internal structure looks like:

struct io_handler
{
      ....
      struct
      {
          const struct mmio_handler *handler;
          paddr_t addr, size;
      } iohandlers[MAX_SIZE];
};

[..]

> diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
> index 895e5cd..f43f2e5 100644
> --- a/xen/arch/arm/vuart.c
> +++ b/xen/arch/arm/vuart.c
> @@ -44,6 +44,8 @@
>  
>  #define domain_has_vuart(d) ((d)->arch.vuart.info != NULL)
>  
> +static struct mmio_handler vuart_mmio_handler;
> +
>  int domain_vuart_init(struct domain *d)
>  {
>      ASSERT( !d->domain_id );
> @@ -59,6 +61,10 @@ int domain_vuart_init(struct domain *d)
>      if ( !d->arch.vuart.buf )
>          return -ENOMEM;
>  
> +    vuart_mmio_handler.addr = d->arch.vuart.info->base_addr;
> +    vuart_mmio_handler.size = d->arch.vuart.info->size;
> +    register_mmio_handler(d, &vuart_mmio_handler);
> +

Same here...

[..]
> --- a/xen/include/asm-arm/mmio.h
> +++ b/xen/include/asm-arm/mmio.h
> @@ -35,15 +35,22 @@ typedef int (*mmio_write_t)(struct vcpu *v, mmio_info_t *info);
>  typedef int (*mmio_check_t)(struct vcpu *v, paddr_t addr);
>  
>  struct mmio_handler {
> -    mmio_check_t check_handler;
> +    paddr_t addr;
> +    unsigned long size;

size should be paddr_t.

>      mmio_read_t read_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;
> +    spinlock_t lock;
> +    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);

The IO handler will never be modified so it should be:

const struct mmio_handler *handler

> +int domain_io_init(struct domain *d);
>  
>  #endif
>  
> 

Regards,

-- 
Julien Grall