Re: [Xen-devel] [PATCH v2 3/3] x86/vioapic: bind interrupts to PVH Dom0

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, May 12, 2017 at 07:55:04AM -0600, Jan Beulich wrote:
> >>> On 19.04.17 at 17:11, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -158,6 +158,62 @@ static int vioapic_read(
> >      return X86EMUL_OKAY;
> >  }
> >  
> > +static int vioapic_dom0_map_gsi(unsigned int gsi, unsigned int trig,
> 
> Considering the conditional in the caller, please use hwdom instead
> of dom0.

Done.

> > +                                unsigned int pol)
> > +{
> > +    struct domain *d = current->domain;
> > +    xen_domctl_bind_pt_irq_t pt_irq_bind = {
> > +        .irq_type = PT_IRQ_TYPE_PCI,
> > +        .machine_irq = gsi,
> > +        .hvm_domid = DOMID_SELF,
> 
> I'm struggling with this field: Did you not notice it's entirely
> unused? We should really delete it from the interface, as
> redundant with the domain specifier in the domctl container
> structure.

Right, I've removed it.

> > +    };
> > +    int ret, pirq = gsi;
> > +
> > +    ASSERT(is_hardware_domain(d));
> > +
> > +    /* Interrupt has been unmasked, bind it now. */
> > +    ret = mp_register_gsi(gsi, trig, pol);
> > +    if ( ret && ret != -EEXIST )
> > +    {
> > +        gdprintk(XENLOG_WARNING, "vioapic: error registering GSI %u: %d\n",
> > +                 gsi, ret);
> > +        goto error;
> > +    }
> > +    else if ( ret )
> > +        /* Already in use. */
> > +        return 0;
> 
> I think this would better be
> 
>     if ( ret == -EEXIST )
>         return 0;
>     if ( ret )
>         ....

Done.

> > +    ret = allocate_and_map_gsi_pirq(d, &pirq, &pirq);
> > +    if ( ret )
> > +    {
> > +        gdprintk(XENLOG_WARNING, "vioapic: error mapping GSI %u: %d\n",
> > +                 gsi, ret);
> > +        goto error;
> > +    }
> > +
> > +    pcidevs_lock();
> > +    ret = pt_irq_create_bind(d, &pt_irq_bind);
> > +    pcidevs_unlock();
> > +    if ( ret )
> > +    {
> > +        gdprintk(XENLOG_WARNING, "vioapic: error binding GSI %u: %d\n",
> > +                 gsi, ret);
> > +        goto error_unmap;
> > +    }
> > +
> > +    return 0;
> > +
> > + error_unmap:
> 
> I can live with the "error" label below, but the one above clearly can be
> avoided quite easily by simply inverting the preceding if().

I've changed this to:

    pcidevs_lock();
    ret = pt_irq_create_bind(d, &pt_irq_bind);
    if ( ret )
    {
        gprintk(XENLOG_WARNING, "vioapic: error binding GSI %u: %d\n",
                gsi, ret);
        spin_lock(&d->event_lock);
        unmap_domain_pirq(d, pirq);
        spin_unlock(&d->event_lock);
    }
    pcidevs_unlock();

    return ret;

And got rid of both labels.

> Also, considering this is Dom0-only, I wonder whether all of the log
> messages wouldn't better use gprintk().

Done.

Thanks, Roger.

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