Re: [Xen-devel] [PATCH v7 for-next 10/12] vpci/msi: add MSI handlers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.10.17 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
> Changes since v6:
>...
>  - Prevent the guest from writing to the pending bits field, it's read
>    only as defined in the spec.

I think we've been there before: Dom0 should be permitted whatever
it wants; DomU would need to be restricted (once supported, but as
we know there are numerous other areas in this new code you add
which would need changing for this to be secure).

> +static void control_write(const struct pci_dev *pdev, unsigned int reg,
> +                          uint32_t val, void *data)
> +{
> +    struct vpci_msi *msi = data;
> +    unsigned int vectors = min_t(uint8_t,
> +                                 1u << MASK_EXTR(val, PCI_MSI_FLAGS_QSIZE),
> +                                 msi->max_vectors);
> +    bool new_enabled = val & PCI_MSI_FLAGS_ENABLE;
> +
> +    /*
> +     * No change if the enable field and the number of vectors is
> +     * the same or the device is not enabled, in which case the
> +     * vectors field can be updated directly.
> +     */
> +    if ( new_enabled == msi->enabled &&
> +         (vectors == msi->vectors || !msi->enabled) )
> +    {
> +        msi->vectors = vectors;
> +        return;
> +    }
> +
> +    if ( new_enabled )
> +    {
> +        unsigned int i;
> +
> +        /*
> +         * If the device is already enabled it means the number of
> +         * enabled messages has changed. Disable and re-enable the
> +         * device in order to apply the change.
> +         */
> +        if ( msi->enabled )
> +        {
> +            vpci_msi_arch_disable(msi, pdev);
> +            msi->enabled = false;
> +        }
> +
> +        if ( vpci_msi_arch_enable(msi, pdev, vectors) )
> +            return;
> +
> +        for ( i = 0; msi->masking && i < vectors; i++ )
> +            vpci_msi_arch_mask(msi, pdev, i, (msi->mask >> i) & 1);

The ordering looks wrong at the first (and second) glance: It gives
the impression that you enable the vectors and only then mask
them. I _assume_ the ordering is the way it is because
vpci_msi_arch_enable() leaves the vectors masked (albeit that's
sort of contradicting the msi->masking part of the loop condition),
and if so this should be explained in a comment. If, however, this
assumption of mine is wrong, then the order needs changing.

> +static int init_msi(struct pci_dev *pdev)
> +{
> +    uint8_t seg = pdev->seg, bus = pdev->bus;

A PCI segment identifier is a 16-bit quantity. Please check the rest of
the series for similar issues.

> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    struct vpci_msi *msi;
> +    unsigned int pos = pci_find_cap_offset(seg, bus, slot, func,
> +                                           PCI_CAP_ID_MSI);
> +    uint16_t control;
> +    int ret;
> +
> +    if ( !pos )
> +        return 0;
> +
> +    msi = xzalloc(struct vpci_msi);
> +    if ( !msi )
> +        return -ENOMEM;
> +
> +    ret = vpci_add_register(pdev->vpci, control_read, control_write,
> +                            msi_control_reg(pos), 2, msi);
> +    if ( ret )
> +    {
> +        xfree(msi);
> +        return ret;
> +    }

You probably know that I'm not a fan of labels and goto-s, but the
number of identical error paths here would benefit from folding all
of them into one. Or alternatively arrange the function such that
each step first checks whether "ret" is still zero, having a single
conditional xfree() at the end of the function.

> @@ -152,6 +153,8 @@ int msi_free_irq(struct msi_desc *entry);
>       ( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
>  #define msi_mask_bits_reg(base, is64bit) \
>       ( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> +#define msi_pending_bits_reg(base, is64bit) \
> +     ( (is64bit == 1) ? base+PCI_MSI_MASK_BIT+4 : base+PCI_MSI_MASK_BIT)

Please avoid repeating mistakes in the other macros:

#define msi_pending_bits_reg(base, is64bit) \
        ((is64bit) ? (base) + PCI_MSI_MASK_BIT + 4 : (base) + PCI_MSI_MASK_BIT)

or even

#define msi_pending_bits_reg(base, is64bit) \
        ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -79,6 +79,28 @@ 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 {
> +        /* Address. */
> +        uint64_t address;
> +        /* Mask bitfield. */
> +        uint32_t mask;
> +        /* Data. */
> +        uint16_t data;
> +        /* Maximum number of vectors supported by the device. */
> +        uint8_t max_vectors : 5;
> +        /* Number of vectors configured. */
> +        uint8_t vectors     : 5;
> +        /* Enabled? */
> +        bool enabled        : 1;
> +        /* Supports per-vector masking? */
> +        bool masking        : 1;
> +        /* 64-bit address capable? */
> +        bool address64      : 1;

At least on x86, better code will result if you move the three single
bit fields between the two 5-bit ones.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel