Re: [Xen-devel] [PATCH v2 23/30] xen/x86: route legacy PCI interrupts to Dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
> This is done adding some Dom0 specific logic to the IO APIC emulation inside
> of Xen, so that writes to the IO APIC registers that should unmask an
> interrupt will take care of setting up this interrupt with Xen. A Dom0
> specific EIO handler also has to be used, since Xen doesn't know the
> topology of the PCI devices and it just has to passthrough what Dom0 does.

Without having looked at the patch (yet) I have a hard time seeing
the connection between EOI and PCI topology. I therefore think the
description needs improvement.

> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -148,6 +148,29 @@ static void vioapic_write_redirent(
>          unmasked = unmasked && !ent.fields.mask;
>      }
>  
> +    if ( is_hardware_domain(d) && unmasked )
> +    {
> +        int ret, gsi;
> +
> +        /* Interrupt has been unmasked */
> +        gsi = idx;
> +        ret = mp_register_gsi(gsi, ent.fields.trig_mode, 
> ent.fields.polarity);
> +        if ( ret && ret != -EEXIST )
> +        {
> +            gdprintk(XENLOG_WARNING,
> +                     "%s: error registering GSI %d\n", __func__, ret);

The message text suggests the number is the GSI, whereas it really
looks to be an error code (and I guess you really mean to log both).
Also please no unnecessary new uses of __func__.

> +        }
> +        if ( !ret )
> +        {
> +            ret = physdev_map_pirq(DOMID_SELF, MAP_PIRQ_TYPE_GSI, &gsi, &gsi,
> +                                   NULL);
> +            BUG_ON(ret);
> +
> +            ret = pt_irq_bind_hw_domain(gsi);
> +            BUG_ON(ret);

Why BUG_ON() (in both cases)? I don't think we're necessarily hosed
just because of one IRQ setup failure.

> @@ -409,7 +432,10 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>          if ( iommu_enabled )
>          {
>              spin_unlock(&d->arch.hvm_domain.irq_lock);
> -            hvm_dpci_eoi(d, gsi, ent);
> +            if ( is_hardware_domain(d) )
> +                hvm_hw_dpci_eoi(d, gsi, ent);
> +            else
> +                hvm_dpci_eoi(d, gsi, ent);

This looks like you rather want to make the distinction inside the
called function.

> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -159,26 +159,29 @@ static int pt_irq_guest_eoi(struct domain *d, struct 
> hvm_pirq_dpci *pirq_dpci,
>  static void pt_irq_time_out(void *data)
>  {
>      struct hvm_pirq_dpci *irq_map = data;
> -    const struct hvm_irq_dpci *dpci;
>      const struct dev_intx_gsi_link *digl;
>  
>      spin_lock(&irq_map->dom->event_lock);
>  
> -    dpci = domain_get_irq_dpci(irq_map->dom);
> -    ASSERT(dpci);
> -    list_for_each_entry ( digl, &irq_map->digl_list, list )
> +    if ( !is_hardware_domain(irq_map->dom) )
>      {
> -        unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> -        const struct hvm_girq_dpci_mapping *girq;
> -
> -        list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
> +        const struct hvm_irq_dpci *dpci = domain_get_irq_dpci(irq_map->dom);
> +        ASSERT(dpci);

Blank line between declarations and statements please.

> +        list_for_each_entry ( digl, &irq_map->digl_list, list )
>          {
> -            struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi);
> +            unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, 
> digl->intx);
> +            const struct hvm_girq_dpci_mapping *girq;
> +
> +            list_for_each_entry ( girq, &dpci->girq[guest_gsi], list )
> +            {
> +                struct pirq *pirq = pirq_info(irq_map->dom, 
> girq->machine_gsi);
>  
> -            pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH;
> +                pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH;
> +            }
> +            hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
>          }
> -        hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx);
> -    }
> +    } else

Coding style.

> +        irq_map->flags |= HVM_IRQ_DPCI_EOI_LATCH;

And I'm afraid I can't conclude anyway why you do what you do
here, as you don't really describe your the changes in any detail.

> @@ -557,6 +560,85 @@ int pt_irq_create_bind(
>      return 0;
>  }
>  
> +int pt_irq_bind_hw_domain(int gsi)
> +{
> +    struct domain *d = hardware_domain;
> +    struct hvm_pirq_dpci *pirq_dpci;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
> +    struct pirq *info;
> +    int rc;
> +
> +    if ( gsi < 0 || gsi >= d->nr_pirqs )
> +        return -EINVAL;
> +
> +restart:

Labels (if they're needed at all) indented by at least one blank
please.

And I'm afraid I'm giving up again.

Jan

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