| Message ID | 20220107125523.212649-1-dvrabel@amazon.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] x86/hvm: add more callback/upcall info to 'I' debug key | expand |
On 07.01.2022 13:55, David Vrabel wrote: > Include the type of the callback via and the per-VCPU upcall vector. > > Signed-off-by: David Vrabel <dvrabel@amazon.co.uk> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 07/01/2022 12:55, David Vrabel wrote: > @@ -630,9 +634,46 @@ static void irq_dump(struct domain *d) > hvm_irq->pci_link_assert_count[1], > hvm_irq->pci_link_assert_count[2], > hvm_irq->pci_link_assert_count[3]); > - printk("Callback via %i:%#"PRIx32",%s asserted\n", > - hvm_irq->callback_via_type, hvm_irq->callback_via.gsi, > - hvm_irq->callback_via_asserted ? "" : " not"); > + > + printk("Per-VCPU upcall vector:\n"); > + for_each_vcpu ( d, v ) > + { > + if ( v->arch.hvm.evtchn_upcall_vector ) > + { > + printk(" v%u: %u\n", > + v->vcpu_id, v->arch.hvm.evtchn_upcall_vector); Here, and... > + have_upcall_vector = true; > + } > + } > + if ( !have_upcall_vector ) > + printk(" none\n"); > + > + via_asserted = hvm_irq->callback_via_asserted ? " (asserted)" : ""; > + switch( hvm_irq->callback_via_type ) > + { > + case HVMIRQ_callback_none: > + printk("Callback via none\n"); > + break; > + > + case HVMIRQ_callback_gsi: > + printk("Callback via GSI %u%s\n", > + hvm_irq->callback_via.gsi, > + via_asserted); > + break; > + > + case HVMIRQ_callback_pci_intx: > + printk("Callback via PCI dev %u INTx %u%s\n", PCI 00:%02x.0 ? Also, how about INT%c with 'A' + intx as a parameter? > + hvm_irq->callback_via.pci.dev, > + hvm_irq->callback_via.pci.intx, > + via_asserted); > + break; > + > + case HVMIRQ_callback_vector: > + printk("Callback via vector %u%s\n", > + hvm_irq->callback_via.vector, > + via_asserted); ... here, vectors ought to be 0x%02x. Amongst other things, it makes the priority class instantly readable. I realise this is all a complete mess, but is via_asserted correct for HVMIRQ_callback_vector? It's mismatched between the two, and the best metric that exists is "is pending in IRR". Also, looking at struct hvm_irq, all the callback information is in the wrong structure, because it absolutely shouldn't be duplicated for each GSI. ~Andrew
On 07/01/2022 13:45, Andrew Cooper wrote: > I realise this is all a complete mess, but is via_asserted correct for > HVMIRQ_callback_vector? It's mismatched between the two, and the best > metric that exists is "is pending in IRR". Urgh. HVMIRQ_callback_vector is the one that is utterly broken, and doesn't conform to the Local APIC spec, and must not end up in IRR. We still need to figure out how to prevent a domain using HVMIRQ_callback_vector when hardware APIC acceleration is in use, but that's definitely getting off topic for this patch. ~Andrew
On 07/01/2022 13:45, Andrew Cooper wrote: > printk("Callback via PCI dev %u INTx %u%s\n", > > PCI 00:%02x.0 ? Is this correct? If I remember right, the INTx lines are associated with a PCI device, with the function then reporting which line it uses. So Xen neither knows (nor cares) what the function is, so it would be misleading to report it. >> + hvm_irq->callback_via.pci.dev, >> + hvm_irq->callback_via.pci.intx, >> + via_asserted); >> + break; >> + >> + case HVMIRQ_callback_vector: >> + printk("Callback via vector %u%s\n", >> + hvm_irq->callback_via.vector, >> + via_asserted); > > ... here, vectors ought to be 0x%02x. Amongst other things, it makes > the priority class instantly readable. > > I realise this is all a complete mess, but is via_asserted correct for > HVMIRQ_callback_vector? It's mismatched between the two, and the best > metric that exists is "is pending in IRR". Also, looking at struct > hvm_irq, all the callback information is in the wrong structure, because > it absolutely shouldn't be duplicated for each GSI. I'm not sure what changes to this patch you want here.. David
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index 52aae4565f..8b1bb79127 100644 --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -598,7 +598,11 @@ int hvm_local_events_need_delivery(struct vcpu *v) static void irq_dump(struct domain *d) { struct hvm_irq *hvm_irq = hvm_domain_irq(d); - int i; + unsigned int i; + const struct vcpu *v; + bool have_upcall_vector = false; + const char *via_asserted; + printk("Domain %d:\n", d->domain_id); printk("PCI 0x%16.16"PRIx64"%16.16"PRIx64 " ISA 0x%8.8"PRIx32" ROUTE %u %u %u %u\n", @@ -630,9 +634,46 @@ static void irq_dump(struct domain *d) hvm_irq->pci_link_assert_count[1], hvm_irq->pci_link_assert_count[2], hvm_irq->pci_link_assert_count[3]); - printk("Callback via %i:%#"PRIx32",%s asserted\n", - hvm_irq->callback_via_type, hvm_irq->callback_via.gsi, - hvm_irq->callback_via_asserted ? "" : " not"); + + printk("Per-VCPU upcall vector:\n"); + for_each_vcpu ( d, v ) + { + if ( v->arch.hvm.evtchn_upcall_vector ) + { + printk(" v%u: %u\n", + v->vcpu_id, v->arch.hvm.evtchn_upcall_vector); + have_upcall_vector = true; + } + } + if ( !have_upcall_vector ) + printk(" none\n"); + + via_asserted = hvm_irq->callback_via_asserted ? " (asserted)" : ""; + switch( hvm_irq->callback_via_type ) + { + case HVMIRQ_callback_none: + printk("Callback via none\n"); + break; + + case HVMIRQ_callback_gsi: + printk("Callback via GSI %u%s\n", + hvm_irq->callback_via.gsi, + via_asserted); + break; + + case HVMIRQ_callback_pci_intx: + printk("Callback via PCI dev %u INTx %u%s\n", + hvm_irq->callback_via.pci.dev, + hvm_irq->callback_via.pci.intx, + via_asserted); + break; + + case HVMIRQ_callback_vector: + printk("Callback via vector %u%s\n", + hvm_irq->callback_via.vector, + via_asserted); + break; + } } static void dump_irq_info(unsigned char key)
Include the type of the callback via and the per-VCPU upcall vector. Signed-off-by: David Vrabel <dvrabel@amazon.co.uk> --- v2: - fix style - make upcall vector output distinguishable from logs prior to this patch - use fewer lines for callback via. --- xen/arch/x86/hvm/irq.c | 49 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)