Re: [Xen-devel] [PATCH v4 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 07, 2019 at 02:21:24PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Feb 07, 2019 at 10:57:19AM +0100, Roger Pau Monné wrote:
> > On Thu, Feb 07, 2019 at 01:07:47AM +0100, Marek Marczykowski-Górecki wrote:
> > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > index 8b44d6c..5e5dcac 100644
> > > --- a/xen/arch/x86/irq.c
> > > +++ b/xen/arch/x86/irq.c
> > > @@ -2674,6 +2674,22 @@ int allocate_and_map_msi_pirq(struct domain *d, 
> > > int index, int *pirq_p,
> > >          {
> > >      case MAP_PIRQ_TYPE_MULTI_MSI:
> > >              irq = create_irq(NUMA_NO_NODE);
> > > +            if ( !(irq < nr_irqs_gsi || irq >= nr_irqs) &&
> > > +                    current->domain->target == d )
> > > +            {
> > > +                ret = irq_permit_access(current->domain, irq);
> > > +                if ( ret ) {
> > > +                    dprintk(XENLOG_G_ERR,
> > > +                            "dom%d: can't grant it's stubdom (%d) access 
> > > to "
> > > +                            "irq %d for msi: %d!\n",
> > > +                            d->domain_id,
> > > +                            current->domain->domain_id,
> > > +                            irq,
> > > +                            ret);
> > > +                    destroy_irq(irq);
> > > +                    return ret;
> > 
> > I'm afraid his won't work for devices that support multiple MSI vectors.
> > Note that map_domain_pirq also has a call to create_irq, and you are
> > not adding the sutbdom permissions there.
> > 
> > IMO, the safer way to fix this would be to modify create_irq and
> > destroy_irq so that you give permissions to the subtdomain in the same
> > place that hardware domain permissions are given. Note that you will
> > have to change the function to take an extra domain parameter
> > AFAICT.
> 
> That may be a good idea, I'll try.
> 
> > Alternatively the permissions could be granted/revoked in
> > {un}map_domain_pirq, which already contains a call to
> > irq_access_permitted/irq_deny_access, I think I've suggested this in a
> > previous version already [0]. This seems less intrusive that modifying
> > create_irq/destroy_irq if viable.
> 
> I've already tried that. And as responded there, it won't fly, as the
> first thing map_domain_pirq does is checking irq permission, which would
> fail for irq allocated by allocate_and_map_msi_pirq.

Oh sorry, that thread then went on about the addition of the new
hypercall and I likely missed that reply.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel