Re: [Xen-devel] [PATCH v4.1 3/4] x86/pt: enable binding of GSIs to a PVH Dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.17 at 15:58, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -164,6 +164,25 @@ static void pt_irq_time_out(void *data)
>  
>      spin_lock(&irq_map->dom->event_lock);
>  
> +    if ( irq_map->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
> +    {
> +        struct pirq *pirq = dpci_pirq(irq_map);

This could (and hence should) be const. However, ...

> +        ASSERT(is_hardware_domain(irq_map->dom));
> +        /*
> +         * Identity mapped, no need to iterate over the guest GSI list to 
> find
> +         * other pirqs sharing the same guest GSI.
> +         *
> +         * In the identity mapped case the EOI can also be done now, this way
> +         * the iteration over the list of domain pirqs is avoided.
> +         */
> +        hvm_gsi_deassert(irq_map->dom, pirq->pirq);

... this is its only use, so I'm not convinced a local variable is
needed at all.

> @@ -274,10 +293,16 @@ int pt_irq_create_bind(
>      spin_lock(&d->event_lock);
>  
>      hvm_irq_dpci = domain_get_irq_dpci(d);
> -    if ( hvm_irq_dpci == NULL )
> +    if ( hvm_irq_dpci == NULL && !is_hardware_domain(d) )

Would you mind at once switching to the shorter !hvm_irq_dpci
(also further down), the more that you're using the inverse
without " != NULL" elsewhere?

>      {
>          unsigned int i;
>  
> +        /*
> +         * NB: the hardware domain doesn't use a hvm_irq_dpci struct because
> +         * it's only allowed to identity map GSIs, and so the data contained 
> in
> +         * that struct (used to map guest GSIs into machine GSIs and perform
> +         * interrupt routing) it's completely useless to it.

"is completely ..."

> @@ -422,35 +447,52 @@ int pt_irq_create_bind(
>      case PT_IRQ_TYPE_PCI:
>      case PT_IRQ_TYPE_MSI_TRANSLATE:
>      {
> -        unsigned int bus = pt_irq_bind->u.pci.bus;
> -        unsigned int device = pt_irq_bind->u.pci.device;
> -        unsigned int intx = pt_irq_bind->u.pci.intx;
> -        unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx);
> -        unsigned int link = hvm_pci_intx_link(device, intx);
> -        struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link);
> -        struct hvm_girq_dpci_mapping *girq =
> -            xmalloc(struct hvm_girq_dpci_mapping);
> +        struct dev_intx_gsi_link *digl = NULL;
> +        struct hvm_girq_dpci_mapping *girq = NULL;
> +        unsigned int guest_gsi;
>  
> -        if ( !digl || !girq )
> +        /*
> +         * Mapping GSIs for the hardware domain is different than doing it 
> for
> +         * an unpriviledged guest, the hardware domain is only allowed to
> +         * identity map GSIs, and as such all the data in the u.pci union is
> +         * discarded.
> +         */
> +        if ( !is_hardware_domain(d) )

I think I did indicate before that it would feel more safe if you
checked hvm_irq_dpci here (which is NULL if and only if d is
hwdom). At the very least I'd expect a respective ASSERT()
below (but I think the alternative condition here and
ASSERT(is_hardware_domain(d)) in the "else" block would be
better).

>          {
> -            spin_unlock(&d->event_lock);
> -            xfree(girq);
> -            xfree(digl);
> -            return -ENOMEM;
> -        }
> +            unsigned int link;
> +
> +            digl = xmalloc(struct dev_intx_gsi_link);
> +            girq = xmalloc(struct hvm_girq_dpci_mapping);
>  
> -        hvm_irq_dpci->link_cnt[link]++;
> +            if ( !digl || !girq )
> +            {
> +                spin_unlock(&d->event_lock);
> +                xfree(girq);
> +                xfree(digl);
> +                return -ENOMEM;
> +            }
> +
> +            girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
> +            girq->device = digl->device = pt_irq_bind->u.pci.device;
> +            girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
> +            list_add_tail(&digl->list, &pirq_dpci->digl_list);
>  
> -        digl->bus = bus;
> -        digl->device = device;
> -        digl->intx = intx;
> -        list_add_tail(&digl->list, &pirq_dpci->digl_list);
> +            guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> +            link = hvm_pci_intx_link(digl->device, digl->intx);
>  
> -        girq->bus = bus;
> -        girq->device = device;
> -        girq->intx = intx;
> -        girq->machine_gsi = pirq;
> -        list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> +            hvm_irq_dpci->link_cnt[link]++;
> +
> +            girq->machine_gsi = pirq;
> +            list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> +        }
> +        else
> +        {
> +            /* MSI_TRANSLATE is not supported by the hardware domain. */

s/by/for/ ?

> @@ -472,7 +514,28 @@ int pt_irq_create_bind(
>                  pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED |
>                                     HVM_IRQ_DPCI_MACH_PCI |
>                                     HVM_IRQ_DPCI_GUEST_PCI;
> -                share = BIND_PIRQ__WILL_SHARE;
> +                if ( !is_hardware_domain(d) )
> +                    share = BIND_PIRQ__WILL_SHARE;
> +                else
> +                {
> +                    unsigned int pin;
> +                    struct hvm_vioapic *vioapic = gsi_vioapic(d, guest_gsi,
> +                                                              &pin);

const

> @@ -489,9 +552,16 @@ int pt_irq_create_bind(
>                   * IRQ_GUEST is not set. As such we can reset 'dom' 
> directly.
>                   */
>                  pirq_dpci->dom = NULL;
> -                list_del(&girq->list);
> -                list_del(&digl->list);
> -                hvm_irq_dpci->link_cnt[link]--;
> +                if ( girq || digl )
> +                {
> +                    unsigned int link;
> +
> +                    ASSERT(girq && digl);

Perhaps even "ASSERT(girq && digl && hvm_irq_dpci)" or follow the
model outlined above for consistency?

> @@ -504,10 +574,17 @@ int pt_irq_create_bind(
>          spin_unlock(&d->event_lock);
>  
>          if ( iommu_verbose )
> -            printk(XENLOG_G_INFO
> -                   "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n",
> -                   d->domain_id, pirq, guest_gsi, bus,
> -                   PCI_SLOT(device), PCI_FUNC(device), intx);
> +        {
> +            char buf[24] = "";
> +
> +            if ( !is_hardware_domain(d) )
> +                snprintf(buf, ARRAY_SIZE(buf), " dev=%02x.%02x.%u intx=%u",
> +                         digl->bus, PCI_SLOT(digl->device),
> +                         PCI_FUNC(digl->device), digl->intx);

Perhaps again better "if ( digl )".

> @@ -696,7 +777,8 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
>      struct hvm_irq_dpci *dpci = domain_get_irq_dpci(d);
>      struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>  
> -    if ( !iommu_enabled || !dpci || !pirq_dpci ||
> +    if ( !is_hvm_domain(d) || !iommu_enabled ||
> +         (!is_hardware_domain(d) && !dpci) || !pirq_dpci ||
>           !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
>          return 0;

So why again do we suddenly need !is_hvm_domain() here? With
the name of the function there shouldn't be any caller invoking it
for a PV guest.

Jan

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