|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4.1 3/4] x86/pt: enable binding of GSIs to a PVH Dom0
On Wed, Jun 07, 2017 at 07:17:16AM -0600, Jan Beulich wrote:
> >>> 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.
Done, I've removed the local variable.
> > @@ -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?
Done.
> > {
> > 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 ..."
Fixed.
> > @@ -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).
I've changed this to:
if ( hvm_irq_dpci )
{
...
}
else
{
ASSERT(is_hardware_domain(d));
...
}
> > {
> > - 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/ ?
OK. I guess this is better because d is a target in this context?
> > @@ -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?
I've changed the condition to "if ( hvm_irq_dpci )" and left the ASSERT
as-is.
> > @@ -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 )".
Yes, I think that's better.
> > @@ -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.
Sadly there is, in __do_IRQ_guest the following is used:
if ( !hvm_do_IRQ_dpci(d, pirq) )
send_guest_pirq(d, pirq);
Without checking if d is a HVM or PV guest, so the check is needed (or
needs to be moved further up in the call chain into __do_IRQ_guest).
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |