|
[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 Fri, May 12, 2017 at 07:42:51AM -0600, Jan Beulich wrote:
> >>> 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 ...
Yes, I modify pt_irq_destroy_bind to return EOPNOTSUPP when trying to unbind
such an interrupt. I can implement the unbind, but it's not going to be used
ATM.
> > --- 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().
OK, changed.
> > + 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.
I've added this because hvm_gsi_assert doesn't call assert_irq, so a PIC would
not work properly, but it's probably pointless.
> > + 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()).
__hvm_pci_intx_{de}assert has an array that tracks the status of each interrupt
line, and Xen does the routing based on that (the __test_and_clear_bit at the
top of __hvm_pci_intx_assert). That prevents the same line from triggering
multiple times, which is not available here, and thus Xen needs to rely on
gsi_assert_count in order to know if the GSI is pending or not.
Switched to use a set to 1/0 instead of the increment, which I agree makes this
clearer.
>
> > @@ -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."
Fixed, thanks.
> > @@ -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.
OK, let me return error instead to be on the safe side then.
> > @@ -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.
AFAICT max length should be 21, would you be fine with me setting it to 24 to
be safe?
>
> > @@ -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.
OK, I've removed this and fixed the function so it can unbind
HVM_IRQ_DPCI_IDENTITY_GSI.
> > @@ -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.
I've left it in it's previous place and added a !is_hardware_domain check.
> > @@ -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.
Done.
>
> > 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?
Hm, I don't think it's needed, probably just a leftover from when I was testing
the patches.
> > @@ -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.
I will leave the others as they are, or else they should be changed in a
separate patch.
> > + 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.
Done.
Thanks for the review, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |