From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 01/15] xen/arm: register mmio handler at runtime Date: Fri, 04 Apr 2014 13:18:15 +0100 Message-ID: <533EA307.4000900@linaro.org> References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com> <1396612593-443-2-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1396612593-443-2-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com Cc: Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.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_ for GICD_ with > * -bits-per-interrupt > @@ -107,6 +108,7 @@ int domain_vgic_init(struct domain *d) > } > for (i=0; 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