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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 06, 2025 at 05:45:27PM +0100, Jan Beulich wrote:
> 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).

Hm, even if not using msi_compose_msg() I don't see how any device
would use a different MSI settings from physical delivery and fixed
destination.  I think it's unlikely for a device to use anything
different from the current values set by msi_compose_msg().

Otherwise I can see about returning whether the entry needs to be
updated from iommu_update_ire_from_msi() (if the offset into the IRT
for the entry has changed).  However that requires adding code to both
AMD and Intel IOMMU implementations, and will need at least a way to
signal that the MSI fields must forcefully be written on resume.

> > --- 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.

Right, I need to cache it before dropping the lock.

> >                 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?

We could change what's set by msi_compose_msg(), and then the
information here would be incorrect (if hardcoded).

Thanks, Roger.