[PATCH] x86/hvm: add more callback/upcall info to 'I' debug key

[PATCH] x86/hvm: add more callback/upcall info to 'I' debug key

[David Vrabel]

Include the type of the callback via and the per-VCPU upcall vector.

Signed-off-by: David Vrabel <dvrabel@amazon.co.uk>
---
 xen/arch/x86/hvm/irq.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 52aae4565f..6a1edb99f2 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -598,7 +598,9 @@ 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; 
+    int i;
+    struct vcpu *v;
+
     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 +632,30 @@ 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");
+    for_each_vcpu( d, v )
+    {
+        if ( v->arch.hvm.evtchn_upcall_vector )
+            printk("%pv: upcall vector: %u\n",
+                   v, v->arch.hvm.evtchn_upcall_vector);
+    }
+    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\n", hvm_irq->callback_via.gsi);
+        break;
+    case HVMIRQ_callback_pci_intx:
+        printk("Callback via PCI dev %u INTx %u\n",
+               hvm_irq->callback_via.pci.dev,
+               hvm_irq->callback_via.pci.intx);
+        break;
+    case HVMIRQ_callback_vector:
+        printk("Callback via vector %u\n", hvm_irq->callback_via.vector);
+        break;
+    }
+    printk("  %s asserted\n", hvm_irq->callback_via_asserted ? "" : " not");
 }
 
 static void dump_irq_info(unsigned char key)
-- 
2.30.2

Re: [PATCH] x86/hvm: add more callback/upcall info to 'I' debug key

[Jan Beulich]

On 06.01.2022 16:46, 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>

Welcome back!

A couple of stylistic / cosmetic remarks:

> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -598,7 +598,9 @@ 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; 
> +    int i;

Since you touch this line anyway, would you mind switching to
"unsigned int"?

> +    struct vcpu *v;

const?

> @@ -630,9 +632,30 @@ 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");
> +    for_each_vcpu( d, v )

Depending on whether you consider for_each_vcpu a pseudo-keyword,
there's a blank missing here (like for "switch" below), or there
are two excess ones.

> +    {
> +        if ( v->arch.hvm.evtchn_upcall_vector )
> +            printk("%pv: upcall vector: %u\n",
> +                   v, v->arch.hvm.evtchn_upcall_vector);

I'm not convinced of (but also not outright opposed to) the
resulting redundancy here from using %pv: The domain already got
printed once at the top of the function.

> +    }
> +    switch( hvm_irq->callback_via_type )

Missing blank ahead of opening parenthesis.

> +    {
> +    case HVMIRQ_callback_none:
> +        printk("Callback via none\n");
> +        break;
> +    case HVMIRQ_callback_gsi:
> +        printk("Callback via GSI %u\n", hvm_irq->callback_via.gsi);
> +        break;
> +    case HVMIRQ_callback_pci_intx:
> +        printk("Callback via PCI dev %u INTx %u\n",
> +               hvm_irq->callback_via.pci.dev,
> +               hvm_irq->callback_via.pci.intx);
> +        break;
> +    case HVMIRQ_callback_vector:
> +        printk("Callback via vector %u\n", hvm_irq->callback_via.vector);
> +        break;
> +    }

We try to put blank lines between non-fall-through case blocks within
a switch().

> +    printk("  %s asserted\n", hvm_irq->callback_via_asserted ? "" : " not");

I'm a little puzzled by the blank preceding "not"; how about

    printk("  %sasserted\n", hvm_irq->callback_via_asserted ? "" : "not ");

?

Jan