Re: MSI for Virtio PCI transport

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 25, 2024 at 08:18:03AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, June 25, 2024 1:40 PM
> > 
> > On Tue, Jun 25, 2024 at 08:00:45AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Tuesday, June 25, 2024 1:25 PM
> > > > To: Parav Pandit <parav@nvidia.com>
> > > > Cc: Manivannan Sadhasivam <manisadhasivam.linux@gmail.com>; virtio-
> > > > comment@lists.linux.dev; mie@igel.co.jp
> > > > Subject: Re: MSI for Virtio PCI transport
> > > >
> > > > On Tue, Jun 25, 2024 at 06:18:46AM +0000, Parav Pandit wrote:
> > > > > > So MSI-X is clearly an optional feature which simple devices tend to
> > ignore.
> > > > > > But if both are supported, then obviously Virtio will make use
> > > > > > of MSI-X, but that's not the case here.
> > > > > >
> > > > > If both are supported, and required scale by the driver is <=32,
> > > > > driver can
> > > > choose MSI due to its lightweight nature.
> > > >
> > > > Unlikely, MSI vectors are tricky to mask and this is a problem for
> > > > interrupt balancing. So MSIX is better for performance even if the # of
> > vectors is low.
> > > Masking to my knowledge is not used by MSIX.
> > 
> > There's a mask bit per vector, yes.
> > 
> > > Didn't follow how MSIX helps with performance.
> > 
> > Linux uses mask/change/unmask to balance interrupts between CPUs.
> > *That* is important for performance.
> Ah, I see, didn't know this.
> Frequently reprogramming the mask in device is expensive and also requires synchronization to not miss the interrupt.
> In case if you have pointer to the Linux code please share.


static inline void pci_msix_mask(struct msi_desc *desc)
{
        desc->pci.msix_ctrl |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
        pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
        /* Flush write to device */
        readl(desc->pci.mask_base);
}

and

static inline void pci_write_msg_msix(struct msi_desc *desc, struct msi_msg *msg)
{
        void __iomem *base = pci_msix_desc_addr(desc);
        u32 ctrl = desc->pci.msix_ctrl;
        bool unmasked = !(ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT);

        if (desc->pci.msi_attrib.is_virtual)
                return;
        /*
         * The specification mandates that the entry is masked
         * when the message is modified:
         * 
         * "If software changes the Address or Data value of an
         * entry while the entry is unmasked, the result is
         * undefined."
         */
        if (unmasked)
                pci_msix_write_vector_ctrl(desc, ctrl | PCI_MSIX_ENTRY_CTRL_MASKBIT);

        writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
        writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
        writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
        
        if (unmasked)
                pci_msix_write_vector_ctrl(desc, ctrl);

        /* Ensure that the writes are visible in the device */
        readl(base + PCI_MSIX_ENTRY_DATA);
}


both do this.


> A while back, I recollect seeing reprogramming done by the IOAPIC at the interrupt table level within the cpu (without involving the device) to forward to different cpu.

AFAIK MSI goes to lapic not to ioapic. I might be confused though.

> > 
> > 
> > > The benefit of MSI is it does not need to store addr+data pair per vector.
> > 
> > The addr/data thing wasn't invented just to make hardware costs go up.
> >
> I am aware of it. :)
> I will let other non virtio forums finish their ongoing optimization work in this area to reduce hardware costs.

Absolutely.

-- 
MST