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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +                                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.

> +    };
> +    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 )
        ....

> +    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().

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

Jan


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