[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 02/16] xen/arm: make mmio handlers domain specific
On Tue, 2014-04-15 at 18:07 +0100, Julien Grall wrote: > > + 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. A temporary variable to hold &mmio_handle->mmio_handlers[i] inside the loop body would make this a lot clearer IMHO. I'd be tempted to rename the existing mmio_handle to mmio_handlers. > > > 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); I don't think this needs to be sy, ish should be enough, possibly even ishst. I expect you'd also need a corresponding read barrier in handle_mmio. I suspect you might need to do there: num = mmio_handle->num_entries barrier for (... i < num ;...) (this wouldn't handle deregistration, which doesn't matter here but might in the future?) > > + 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. Absolutely! > I think my solution on V1 was correct. I.e smth like > > register_mmio_handler(d, &vgic_distr_mmio_handler, address, size); Or make iohandlers be an array of actual struct mmio_handler rather than pointers to them and copy the input of register_mmio_handler (which is what x86 does) Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |