|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/3] x86/pt: enable binding of GSIs to a PVH Dom0
>>> On 19.04.17 at 17:11, <roger.pau@xxxxxxxxxx> wrote:
> Note that currently there's no support for unbinding this interrupts.
Do you plan to deal with that before this changes goes in? Aiui this
not working means you can't pass through devices with pin based
interrupts once Dom0 chose to bind to them. Otoh hand you modify
pt_irq_destroy_bind(), so I'm a little puzzled ...
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -126,6 +126,34 @@ void hvm_pci_intx_deassert(
> spin_unlock(&d->arch.hvm_domain.irq_lock);
> }
>
> +void hvm_gsi_assert(struct domain *d, unsigned int gsi)
> +{
> + struct hvm_irq *hvm_irq = hvm_domain_irq(d);
> +
> + ASSERT(gsi < hvm_irq->nr_gsis);
This would probably better match the alternative construct in
__hvm_pci_intx_assert().
> + ASSERT(!has_vpic(d));
This doesn't look to be relevant for the rest of the function. Is there
a particular reason you've added it? If so, a brief comment might
help.
> + spin_lock(&d->arch.hvm_domain.irq_lock);
> + if ( !hvm_irq->gsi_assert_count[gsi] )
> + {
> + hvm_irq->gsi_assert_count[gsi]++;
Why is this an increment instead of a simple write of 1? Or the
other way around - why is this not always incrementing, just like
__hvm_pci_intx_assert() does? (Symmetric questions then for
hvm_gsi_deassert()).
> @@ -274,10 +289,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) )
> {
> 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 for it.
"...) is completely useless to it."
> @@ -422,35 +443,51 @@ 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) )
> {
> - 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);
> +
> + 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);
>
> - hvm_irq_dpci->link_cnt[link]++;
> + guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
> + link = hvm_pci_intx_link(digl->device, digl->intx);
>
> - digl->bus = bus;
> - digl->device = device;
> - digl->intx = intx;
> - list_add_tail(&digl->list, &pirq_dpci->digl_list);
> + hvm_irq_dpci->link_cnt[link]++;
>
> - girq->bus = bus;
> - girq->device = device;
> - girq->intx = intx;
> - girq->machine_gsi = pirq;
> - list_add_tail(&girq->list, &hvm_irq_dpci->girq[guest_gsi]);
> + 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. */
> + ASSERT(pt_irq_bind->irq_type == PT_IRQ_TYPE_PCI);
> + guest_gsi = pirq;
> + ASSERT(guest_gsi < hvm_domain_irq(d)->nr_gsis);
> + }
This is dangerous: For one it is impossible to judge the correctness
of at least the first of these assertions for the hwdom case without
looking at patch 3. And then the domctl path leading here does not
exclude the subject domain equaling the calling one, i.e. you
potentially assert guest input correctness here. Yes, we have XSA-77
in place, but no, we shouldn't introduce new issues anywhere unless
that's entirely unavoidable.
> @@ -504,10 +567,18 @@ 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[50];
> +
> + 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);
The buffer array seems heavily over-sized - my counting gives at best
slightly over 20 characters you actually need.
> @@ -522,7 +593,6 @@ int pt_irq_create_bind(
> int pt_irq_destroy_bind(
> struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
> {
> - struct hvm_irq_dpci *hvm_irq_dpci;
> struct hvm_pirq_dpci *pirq_dpci;
> unsigned int machine_gsi = pt_irq_bind->machine_irq;
> struct pirq *pirq;
> @@ -552,17 +622,15 @@ int pt_irq_destroy_bind(
>
> spin_lock(&d->event_lock);
>
> - hvm_irq_dpci = domain_get_irq_dpci(d);
> -
> - if ( hvm_irq_dpci == NULL )
> + pirq = pirq_info(d, machine_gsi);
> + pirq_dpci = pirq_dpci(pirq);
> + if ( pirq_dpci->flags & HVM_IRQ_DPCI_IDENTITY_GSI )
I'm sure we've discussed aspects of this before: pirq_dpci may be
NULL here, i.e. you can't blindly dereference it. All other uses in
the function indeed have a NULL check first.
> @@ -570,9 +638,16 @@ int pt_irq_destroy_bind(
> 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 hvm_irq_dpci *hvm_irq_dpci = domain_get_irq_dpci(d);
> struct hvm_girq_dpci_mapping *girq;
> struct dev_intx_gsi_link *digl, *tmp;
>
> + if ( hvm_irq_dpci == NULL )
> + {
> + spin_unlock(&d->event_lock);
> + return -EINVAL;
> + }
Moving this check here is a behavioral modification. Perhaps a
good one, yet it doesn't belong into this patch. Instead it should
be properly reasoned about in a separate patch, if it is a correct
thing to do.
> @@ -814,17 +896,12 @@ static void hvm_dirq_assist(struct domain *d, struct
> hvm_pirq_dpci *pirq_dpci)
> spin_unlock(&d->event_lock);
> }
>
> -static void __hvm_dpci_eoi(struct domain *d,
> - const struct hvm_girq_dpci_mapping *girq,
> +static void __hvm_pirq_eoi(struct pirq *pirq,
Please drop the double leading underscores.
> const union vioapic_redir_entry *ent)
> {
> - struct pirq *pirq = pirq_info(d, girq->machine_gsi);
> - struct hvm_pirq_dpci *pirq_dpci;
> -
> - if ( !hvm_domain_use_pirq(d, pirq) )
> - hvm_pci_intx_deassert(d, girq->device, girq->intx);
> + struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
>
> - pirq_dpci = pirq_dpci(pirq);
> + ASSERT(pirq_dpci);
Why is this useful / needed all of the sudden?
> @@ -839,6 +916,32 @@ static void __hvm_dpci_eoi(struct domain *d,
> pirq_guest_eoi(pirq);
> }
>
> +static void __hvm_dpci_eoi(struct domain *d,
> + const struct hvm_girq_dpci_mapping *girq,
> + const union vioapic_redir_entry *ent)
> +{
> + struct pirq *pirq = pirq_info(d, girq->machine_gsi);
> +
> + if ( !hvm_domain_use_pirq(d, pirq) )
> + hvm_pci_intx_deassert(d, girq->device, girq->intx);
> +
> + __hvm_pirq_eoi(pirq, ent);
> +}
> +
> +static void __hvm_gsi_eoi(struct domain *d, unsigned int gsi,
Same here for the double underscores. For the pre-existing
function I'd leave it up to you whether to also drop them. What
I care about is that we don't gain new non-conforming names.
> + const union vioapic_redir_entry *ent)
> +{
> + struct pirq *pirq = pirq_info(d, gsi);
> + struct hvm_pirq_dpci *pirq_dpci = pirq_dpci(pirq);
> +
> + /* Check if GSI is actually mapped. */
> + if ( !pirq_dpci )
Please avoid the local variable when used just once here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |