[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 01/15] xen/arm: register mmio handler at runtime
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |