Re: [Xen-devel] [PATCH v6 11/11] vpci/msix: add MSI-X handlers

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

On Wed, Oct 04, 2017 at 08:34:43AM +0000, Jan Beulich wrote:
> >>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
> > +    const struct vpci_bar *bars;
> > +    struct vpci_msix *msix;
> > +    const struct vpci_msix_entry *entry;
> > +    unsigned int offset;
> > +
> > +    *data = ~0ul;
> > +
> > +    msix = vpci_msix_find(d, addr);
> > +    if ( !msix || !vpci_msix_access_allowed(msix->pdev, addr, len) )
> > +        return X86EMUL_OKAY;
> 
> In the !msix case I'm once again not convinced returning OKAY is correct
> here.

From what we have spoken in the mmcfg case, for the !msix case Xen
should return _RETRY. This error can only happen when the msix table
is unmapped in between a accept and read/write call, so calling the
accept handler again will return the correct value.

> > --- a/xen/include/xen/vpci.h
> > +++ b/xen/include/xen/vpci.h
> > @@ -100,6 +100,40 @@ struct vpci {
> >          /* 64-bit address capable? */
> >          bool address64;
> >      } *msi;
> > +
> > +    /* MSI-X data. */
> > +    struct vpci_msix {
> > +        struct pci_dev *pdev;
> > +        /* List link. */
> > +        struct list_head next;
> > +        /* Table information. */
> > +        struct vpci_msix_mem {
> > +            /* MSI-X table offset. */
> > +            unsigned int offset;
> > +            /* MSI-X table BIR. */
> > +            unsigned int bir;
> > +            /* Table size. */
> > +            unsigned int size;
> > +#define VPCI_MSIX_TABLE     0
> > +#define VPCI_MSIX_PBA       1
> > +#define VPCI_MSIX_MEM_NUM   2
> > +        } mem[VPCI_MSIX_MEM_NUM];
> > +        /* Maximum number of vectors supported by the device. */
> > +        unsigned int max_entries;
> > +        /* MSI-X enabled? */
> > +        bool enabled;
> > +        /* Masked? */
> > +        bool masked;
> > +        /* Entries. */
> > +        struct vpci_msix_entry {
> > +            uint64_t addr;
> > +            uint32_t data;
> > +            unsigned int nr;
> > +            struct vpci_arch_msix_entry arch;
> > +            bool masked;
> > +            bool updated;
> > +        } entries[];
> > +    } *msix;
> 
> Same remark as for MSI regarding optimizing structure size.

Going over the fields, bir can be turned into a uint8_t, and size into
a uint16_t. max_entries can also be converted to a uint16_t together
with nr.

Apart from that I don't see much more optimization, unless we start
packaging fields (ie: offset and bir could reside in a uint32_t), but
IMHO that's going to make the code harder to parse for little gain,
and will involve more calculations in the handlers.

Thanks, Roger.

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