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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxx>

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