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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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