Re: [Xen-devel] [PATCH v6 09/11] vpci/msi: add MSI handlers

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

On Wed, Oct 04, 2017 at 08:34:13AM +0000, Jan Beulich wrote:
> >>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
> > +static void vpci_msi_enable(const struct pci_dev *pdev, struct vpci_msi 
> > *msi,
> > +                            unsigned int vectors)
> > +{
> > +    int ret;
> > +
> > +    ASSERT(!msi->enabled);
> > +    ret = vpci_msi_arch_enable(msi, pdev, vectors);
> > +    if ( ret )
> > +        return;
> > +
> > +    /* Apply the mask bits. */
> > +    if ( msi->masking )
> > +    {
> > +        unsigned int i;
> > +        uint32_t mask = msi->mask;
> > +
> > +        for ( i = ffs(mask) - 1; mask && i < vectors; i = ffs(mask) - 1 )
> > +        {
> > +            vpci_msi_arch_mask(msi, pdev, i, true);
> > +            __clear_bit(i, &mask);
> > +        }
> > +    }
> > +
> > +    __msi_set_enable(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> > +                     PCI_FUNC(pdev->devfn), msi->pos, 1);
> 
> This is very unlikely to be a function that arch-independent code is
> permitted to call.

Right, I could remove the '__' prefix, or introduce a
vpci_msi_arch_dev_enable helper that calls this function.

> > --- a/xen/include/xen/vpci.h
> > +++ b/xen/include/xen/vpci.h
> > @@ -72,6 +72,30 @@ struct vpci {
> >          } bars[7]; /* At most 6 BARS + 1 expansion ROM BAR. */
> >          /* FIXME: currently there's no support for SR-IOV. */
> >      } header;
> > +
> > +    /* MSI data. */
> > +    struct vpci_msi {
> > +        /* Arch-specific data. */
> > +        struct vpci_arch_msi arch;
> > +        /* Address. */
> > +        uint64_t address;
> > +        /* Offset of the capability in the config space. */
> > +        unsigned int pos;
> > +        /* Maximum number of vectors supported by the device. */
> > +        unsigned int max_vectors;
> > +        /* Number of vectors configured. */
> > +        unsigned int vectors;
> > +        /* Mask bitfield. */
> > +        uint32_t mask;
> > +        /* Data. */
> > +        uint16_t data;
> > +        /* Enabled? */
> > +        bool enabled;
> > +        /* Supports per-vector masking? */
> > +        bool masking;
> > +        /* 64-bit address capable? */
> > +        bool address64;
> > +    } *msi;
> >  #endif
> >  };
> 
> As there may be quite a few instance of this structure, please strive to
> keep its size down. Many of the fields above have a pretty limited valid
> value range and hence would benefit from using more narrow types and/or
> bitfields.

max_vectors/vectors can be uint8_t, the rest I'm not sure how to
reduce. I could turn the bools into a bitfield, but isn't a bool
already limited to 1 bit?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel