Re: [Xen-devel] [PATCH v2 04/17] x86/hvm: unify dpci portio intercept with standard portio intercept

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 17 June 2015 15:58
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v2 04/17] x86/hvm: unify dpci portio intercept with
> standard portio intercept
> 
> >>> On 17.06.15 at 16:46, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 17 June 2015 15:37
> >> >>> On 11.06.15 at 17:42, <paul.durrant@xxxxxxxxxx> wrote:
> >> > + found:
> >> > +    mport = (gport - start) + g2m_ioport->mport;
> >> > +
> >> > +    if ( !ioports_access_permitted(current->domain, mport,
> >>
> >> Isn't v == current here (which of course should be made obvious
> >> again by naming it curr)? And then - is this check needed here at
> >> all? I realize you just move it, but the permission check should be
> >> (and is being) done in the XEN_DOMCTL_ioport_mapping handler.
> >>
> >
> > Ok. I'll happily drop the check.
> 
> Just please make sure you mention this in the description.
> 

Ok.

> >> > +void register_dpci_portio_handler(struct domain *d)
> >> > +{
> >> > +    struct hvm_io_handler *handler = &d-
> >> >arch.hvm_domain.dpci_handler;
> >> >
> >> > -    switch ( p->dir )
> >> > -    {
> >> > -    case IOREQ_READ:
> >> > -        rc = dpci_ioport_read(mport, p);
> >> > -        break;
> >> > -    case IOREQ_WRITE:
> >> > -        rc = dpci_ioport_write(mport, p);
> >> > -        break;
> >> > -    default:
> >> > -        gdprintk(XENLOG_ERR, "Error: couldn't handle p->dir = %d", p-
> >dir);
> >> > -        rc = X86EMUL_UNHANDLEABLE;
> >> > -    }
> >> > +    handler->type = IOREQ_TYPE_PIO;
> >> > +    handler->ops = &dpci_portio_ops;
> >> >
> >> > -    return rc;
> >> > +    hvm_register_io_handler(d, handler, 1);
> >>
> >> Again registering a pointer into struct domain, with invariant
> >> initialization.
> >
> > Agreed, but I don't see any other way of unifying this with the main
> process
> > loop. Do you have an alternative suggestion?
> 
> Since the list elements are all part of struct domain, why can't this
> be an array instead (presumably yielding cheaper iteration over it
> at once)?
> 

I had envisaged that maybe msixtbl code might be adapted to use the list too 
(rather than iterating over its own list but I never got as far as that). I'll 
use an array for now.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel