From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 02/16] xen/arm: make mmio handlers domain specific Date: Tue, 15 Apr 2014 18:07:39 +0100 Message-ID: <534D675B.2070509@linaro.org> References: <1397560675-29861-1-git-send-email-vijay.kilari@gmail.com> <1397560675-29861-3-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: <1397560675-29861-3-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 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 > #include > +#include > +#include > #include > #include > > -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_ for GICD_ with > * -bits-per-interrupt > @@ -98,6 +100,10 @@ int domain_vgic_init(struct domain *d) > } > for (i=0; 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