Re: [PATCH v2 2/3] x86/msi: don't use cached address and data fields in msi_desc for dump_msi()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2025 15:57, Roger Pau Monne wrote:
> Instead compose a dummy MSI message just for the purpose of getting the
> delivery attributes, which are the same for all messages.  Note that the
> previous usage of the cached MSI message wasn't fetching the hardware MSI
> fields either.

This feels not future proof. There's no guarantee special IRQs (HPET, IOMMU)
would necessarily use msi_compose_msg() (or any open-coded subset thereof).

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1454,6 +1454,16 @@ void __init early_msi_init(void)
>  static void cf_check dump_msi(unsigned char key)
>  {
>      unsigned int irq;
> +    struct msi_msg msg = {};
> +    uint32_t addr, data;
> +
> +    /*
> +     * Compose a dummy msg message for the purpose of getting the delivery
> +     * attributes.
> +     */
> +    msi_compose_msg(FIRST_DYNAMIC_VECTOR, NULL, &msg);
> +    addr = msg.address_lo;
> +    data = msg.data;
>  
>      printk("MSI information:\n");
>  
> @@ -1461,7 +1471,7 @@ static void cf_check dump_msi(unsigned char key)
>      {
>          struct irq_desc *desc = irq_to_desc(irq);
>          const struct msi_desc *entry;
> -        u32 addr, data, dest32;
> +        uint32_t dest32;
>          signed char mask;
>          struct msi_attrib attr;
>          unsigned long flags;
> @@ -1495,8 +1505,6 @@ static void cf_check dump_msi(unsigned char key)
>              break;
>          }
>  
> -        data = entry->msg.data;
> -        addr = entry->msg.address_lo;
>          dest32 = entry->msg.dest32;
>          attr = entry->msi_attrib;
>          if ( entry->msi_attrib.type )
> @@ -1512,8 +1520,7 @@ static void cf_check dump_msi(unsigned char key)
>              mask = '?';
>          printk(" %-6s%4u vec=%02x%7s%6s%3sassert%5s%7s"
>                 " dest=%08x mask=%d/%c%c/%c\n",
> -               type, irq,
> -               (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT,
> +               type, irq, desc->arch.vector,

We've already dropped desc's lock, so shouldn't be de-referencing desc anymore.

>                 data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
>                 data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
>                 data & MSI_DATA_LEVEL_ASSERT ? "" : "de",

To add to the comment at the top, plus taking patch 1 into account: If we
uniformly used the output of the dummy msi_compose_msg() invocation, why would
we even bother to log information conditionally upon what is in data/addr?

Jan