[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] PVH Dom0 Intel IOMMU issues



On Mon, Apr 17, 2017 at 11:38:33AM +0100, Roger Pau Monné wrote:
>On Mon, Apr 17, 2017 at 10:49:45AM +0800, Chao Gao wrote:
>> On Mon, Apr 17, 2017 at 09:38:54AM +0100, Roger Pau Monné wrote:
>> >On Mon, Apr 17, 2017 at 09:03:12AM +0800, Chao Gao wrote:
>> >> On Mon, Apr 17, 2017 at 08:47:48AM +0100, Roger Pau Monné wrote:
>> >> >On Mon, Apr 17, 2017 at 07:32:45AM +0800, Chao Gao wrote:
>> >> >> On Fri, Apr 14, 2017 at 04:34:41PM +0100, Roger Pau Monné wrote:
>> >> >> >Hello,
>> >> >> >
>> >> >> >Although PVHv2 Dom0 is not yet finished, I've been trying the current 
>> >> >> >code on
>> >> >> >different hardware, and found that with pre-Haswell Intel hardware 
>> >> >> >PVHv2 Dom0
>> >> >> >completely freezes the box when calling iommu_hwdom_init in 
>> >> >> >dom0_construct_pvh.
>> >> >> >OTOH the same doesn't happen when using a newer CPU (ie: haswell or 
>> >> >> >newer).
>> >> >> >
>> >> >> >I'm not able to debug that in any meaningful way because the box 
>> >> >> >seems to lock
>> >> >> >up completely, even the watchdog NMI stops working. Here is the boot 
>> >> >> >log, up to
>> >> >> >the point where it freezes:
>> >> >> 
>> >> >> I try "dom0=pvh" with my skylake. An assertion failed. Is it a 
>> >> >> software bug?
>> >> >> 
>> >
>> >It seems like we are not properly adding/accounting the vIO APICs, but I 
>> >cannot
>> >really see how. I have another patch for you to try below.
>> >
>> >Thanks, Roger.
>> >
>> >---8<---
>> >    diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
>> >index 527ac2aadd..40075e2756 100644
>> >--- a/xen/arch/x86/hvm/vioapic.c
>> >+++ b/xen/arch/x86/hvm/vioapic.c
>> >@@ -610,11 +610,15 @@ int vioapic_init(struct domain *d)
>> >            xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
>> >         return -ENOMEM;
>> > 
>> >+    printk("Adding %u vIO APICs\n", nr_vioapics);
>> >+
>> >     for ( i = 0; i < nr_vioapics; i++ )
>> >     {
>> >         unsigned int nr_pins = is_hardware_domain(d) ? 
>> > nr_ioapic_entries[i] :
>> >             ARRAY_SIZE(domain_vioapic(d, 0)->domU.redirtbl);
>> > 
>> >+        printk("vIO APIC %u has %u pins\n", i, nr_pins);
>> >+
>> >         if ( (domain_vioapic(d, i) =
>> >               xmalloc_bytes(hvm_vioapic_size(nr_pins))) == NULL )
>> >         {
>> >@@ -623,8 +627,12 @@ int vioapic_init(struct domain *d)
>> >         }
>> >         domain_vioapic(d, i)->nr_pins = nr_pins;
>> >         nr_gsis += nr_pins;
>> >+        printk("nr_gsis: %u\n", nr_gsis);
>> >     }
>> > 
>> >+    printk("domain nr_gsis: %u vioapic gsis: %u nr_irqs_gsi: %u 
>> >highest_gsi: %u\n",
>> >+           hvm_domain_irq(d)->nr_gsis, nr_gsis, nr_irqs_gsi, 
>> >highest_gsi());
>> >+
>> >     ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis);
>> > 
>> >     d->arch.hvm_domain.nr_vioapics = nr_vioapics;
>> >
>> 
>> Please Cc or To me.  Is there some holes in all physical IOAPICs gsi ranges?
>
>That's weird, my MUA (Mutt) seems to automatically remove your address from the
>"To:" field. I have no idea why it does that.
>
>So yes, your box has as GSI gap which is not handled by any IO APIC. TBH, I
>didn't even knew that was possible. In any case, patch below should solve it.
>
>---8<---
>commit f52d05fca03440d771eb56077c9d60bb630eb423
>diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
>index 5157db7a4e..ec87a97651 100644
>--- a/xen/arch/x86/hvm/vioapic.c
>+++ b/xen/arch/x86/hvm/vioapic.c
>@@ -64,37 +64,23 @@ static struct hvm_vioapic *addr_vioapic(const struct 
>domain *d,
> struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
>                                 unsigned int *pin)
> {
>-    unsigned int i, base_gsi = 0;
>+    unsigned int i;
> 
>     for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
>     {
>         struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> 
>-        if ( gsi >= base_gsi && gsi < base_gsi + vioapic->nr_pins )
>+        if ( gsi >= vioapic->base_gsi &&
>+             gsi < vioapic->base_gsi + vioapic->nr_pins )
>         {
>-            *pin = gsi - base_gsi;
>+            *pin = gsi - vioapic->base_gsi;
>             return vioapic;
>         }
>-
>-        base_gsi += vioapic->nr_pins;
>     }
> 
>     return NULL;
> }
> 
>-static unsigned int base_gsi(const struct domain *d,
>-                             const struct hvm_vioapic *vioapic)
>-{
>-    unsigned int nr_vioapics = d->arch.hvm_domain.nr_vioapics;
>-    unsigned int base_gsi = 0, i = 0;
>-    const struct hvm_vioapic *tmp;
>-
>-    while ( i < nr_vioapics && (tmp = domain_vioapic(d, i++)) != vioapic )
>-        base_gsi += tmp->nr_pins;
>-
>-    return base_gsi;
>-}
>-
> static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic)
> {
>     uint32_t result = 0;
>@@ -180,7 +166,7 @@ static void vioapic_write_redirent(
>     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>     union vioapic_redir_entry *pent, ent;
>     int unmasked = 0;
>-    unsigned int gsi = base_gsi(d, vioapic) + idx;
>+    unsigned int gsi = vioapic->base_gsi + idx;
> 
>     spin_lock(&d->arch.hvm_domain.irq_lock);
> 
>@@ -340,7 +326,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, 
>unsigned int pin)
>     struct domain *d = vioapic_domain(vioapic);
>     struct vlapic *target;
>     struct vcpu *v;
>-    unsigned int irq = base_gsi(d, vioapic) + pin;
>+    unsigned int irq = vioapic->base_gsi + pin;
> 
>     ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
> 
>@@ -451,7 +437,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> {
>     struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>     union vioapic_redir_entry *ent;
>-    unsigned int i, base_gsi = 0;
>+    unsigned int i;
> 
>     ASSERT(has_vioapic(d));
> 
>@@ -473,19 +459,18 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>             if ( iommu_enabled )
>             {
>                 spin_unlock(&d->arch.hvm_domain.irq_lock);
>-                hvm_dpci_eoi(d, base_gsi + pin, ent);
>+                hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
>                 spin_lock(&d->arch.hvm_domain.irq_lock);
>             }
> 
>             if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
>                  !ent->fields.mask &&
>-                 hvm_irq->gsi_assert_count[base_gsi + pin] )
>+                 hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
>             {
>                 ent->fields.remote_irr = 1;
>                 vioapic_deliver(vioapic, pin);
>             }
>         }
>-        base_gsi += vioapic->nr_pins;
>     }
> 
>     spin_unlock(&d->arch.hvm_domain.irq_lock);
>@@ -554,6 +539,7 @@ void vioapic_reset(struct domain *d)
>         {
>             vioapic->base_address = mp_ioapics[i].mpc_apicaddr;
>             vioapic->id = mp_ioapics[i].mpc_apicid;
>+            vioapic->base_gsi = io_apic_gsi_base(i);
>         }
>         vioapic->nr_pins = nr_pins;
>         vioapic->domain = d;
>@@ -601,7 +587,12 @@ int vioapic_init(struct domain *d)
>         nr_gsis += nr_pins;
>     }
> 
>-    ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis);
>+    /*
>+     * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but
>+     * there might be holes in this range (ie: GSIs that don't belong to any
>+     * vIO APIC).
>+     */
>+    ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis);
> 
>     d->arch.hvm_domain.nr_vioapics = nr_vioapics;
>     vioapic_reset(d);
>diff --git a/xen/include/asm-x86/hvm/vioapic.h 
>b/xen/include/asm-x86/hvm/vioapic.h
>index 8ec91d2bd1..2ceb60eaef 100644
>--- a/xen/include/asm-x86/hvm/vioapic.h
>+++ b/xen/include/asm-x86/hvm/vioapic.h
>@@ -50,6 +50,7 @@
> struct hvm_vioapic {
>     struct domain *domain;
>     uint32_t nr_pins;
>+    unsigned int base_gsi;
>     union {
>         XEN_HVM_VIOAPIC(,);
>         struct hvm_hw_vioapic domU;
>

It works. I can test for you when you send out a formal patch.

Thanks
Chao

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.