[Qemu-devel] Re: [PATCH 07/14] msi: implemented msi.

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

On Mon, Sep 06, 2010 at 04:46:21PM +0900, Isaku Yamahata wrote:
> implemented msi support functions.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>


Good stuff. Below I suggest a couple of minor correctness issues,
and a couple of API changes.  Below are also some suggestions to improve
readability a bit: mostly there are too many one-liners used
only once or twice, with names not informative enough to guess what they
do, so I suggest we opencode them as one ends up reading the source
anyway.

Thanks!

> ---
>  Makefile.objs |    2 +-
>  hw/msi.c      |  362 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/msi.h      |   41 +++++++
>  hw/pci.h      |    5 +
>  4 files changed, 409 insertions(+), 1 deletions(-)
>  create mode 100644 hw/msi.c
>  create mode 100644 hw/msi.h
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 594894b..5f5a4c5 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -186,7 +186,7 @@ hw-obj-$(CONFIG_PIIX4) += piix4.o
>  # PCI watchdog devices
>  hw-obj-y += wdt_i6300esb.o
>  
> -hw-obj-y += msix.o
> +hw-obj-y += msix.o msi.o
>  
>  # PCI network cards
>  hw-obj-y += ne2000.o
> diff --git a/hw/msi.c b/hw/msi.c
> new file mode 100644
> index 0000000..dbe89ee
> --- /dev/null
> +++ b/hw/msi.c
> @@ -0,0 +1,362 @@
> +/*
> + * msi.c
> + *
> + * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "msi.h"
> +
> +#define PCI_MSI_PENDING_32      0x10
> +#define PCI_MSI_PENDING_64      0x14
> +

Longer term, pls try to add this to linux, btw.
If you manage to we'll then move this to pci_regs.h

> +/* PCI_MSI_FLAGS */
> +#define PCI_MSI_FLAGS_QSIZE_SHIFT       4
> +#define PCI_MSI_FLAGS_QMASK_SHIFT       1

Use ffs on macros from pci_regs.h :
PCI_MSI_FLAGS_QSIZE_SHIFT ->
(ffs(PCI_MSI_FLAGS_QSIZE) - 1)
as there's a single user, we don't need a macro then.

> +
> +/* PCI_MSI_ADDRESS_LO */
> +#define PCI_MSI_ADDRESS_LO_RESERVED_MASK        0x3

Better to have the valid bits than invalid bits:
#define PCI_MSI_ADDRESS_LO_MASK        (~0x3)
And then use directly.
again, we can try adding this to linux long term.


> +
> +#define PCI_MSI_32_SIZEOF       0x0a
> +#define PCI_MSI_64_SIZEOF       0x0e
> +#define PCI_MSI_32M_SIZEOF      0x14
> +#define PCI_MSI_64M_SIZEOF      0x18

Note to self: if we get rid of cap allocator, we won't need the above.

> +
> +#define MSI_DEBUG
> +#ifdef MSI_DEBUG
> +# define MSI_DPRINTF(fmt, ...)                                          \
> +    fprintf(stderr, "%s:%d " fmt, __func__, __LINE__, ## __VA_ARGS__)
> +#else
> +# define MSI_DPRINTF(fmt, ...)  do { } while (0)
> +#endif
> +#define MSI_DEV_PRINTF(dev, fmt, ...)                                   \
> +    MSI_DPRINTF("%s:%x " fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__)
> +
> +static inline bool msi_test_bit(uint32_t bitmap, uint8_t bit)
> +{
> +    return bitmap & (1 << bit);
> +}
> +
> +static inline void msi_set_bit(uint32_t *bitmap, uint8_t bit)
> +{
> +    *bitmap |= (1 << bit);
> +}
> +
> +static inline void msi_clear_bit(uint32_t *bitmap, uint8_t bit)
> +{
> +    *bitmap &= ~(1 << bit);
> +}


The above 3 are much better open-coded - all they do is simple math.
Also, I think we should use 1U << - the above is signed and so it
will cause integer overflow when bit is 31.

> +
> +

Two empty lines -> one

> +/* multiple vector only suport power of 2 and up to 32 */
> +static inline uint8_t ilog2(uint8_t nr_vector)
> +{
> +    switch (nr_vector) {
> +    case 1:
> +        return 0;
> +    case 2:
> +        return 1;
> +    case 4:
> +        return 2;
> +    case 8:
> +        return 3;
> +    case 16:
> +        return 4;
> +    case 32:
> +        return 5;
> +    default:
> +        abort();

Not intuitive to abort in a mathematical function IMO.

> +        break;
> +    }
> +    return 0;
> +}
> +

Please just use ffs opencoded, there's a single user.
ilog -> ffs(n) - 1;
and do this after range-checking as suggested below.

> +static inline bool is_mask_bit_support(uint16_t control)
> +{
> +    return control & PCI_MSI_FLAGS_MASKBIT;
> +}
> +
> +static inline bool is_64bit_address(uint16_t control)
> +{
> +    return control & PCI_MSI_FLAGS_64BIT;
> +}
> +

These 2 function names are especially confusing.
The above 2 are better open-coded.

> +static inline uint8_t msi_vector_order(uint16_t control)
> +{
> +    return (control & PCI_MSI_FLAGS_QSIZE) >> PCI_MSI_FLAGS_QSIZE_SHIFT;
> +}
> +

above only used once? better opencoded

> +static inline uint8_t msi_nr_vector(uint16_t control)
> +{
> +    return 1 << msi_vector_order(control);
> +}
> +

Rename msi_nr_vectors.


> +static inline bool msi_control_enabled(uint16_t control)
> +{
> +    return control & PCI_MSI_FLAGS_ENABLE;
> +}
> +

above is better opencoded.

> +static inline uint8_t msi_cap_sizeof(uint16_t control)
> +{
> +    switch (control & (PCI_MSI_FLAGS_MASKBIT | PCI_MSI_FLAGS_64BIT)) {
> +    case PCI_MSI_FLAGS_MASKBIT | PCI_MSI_FLAGS_64BIT:
> +        return PCI_MSI_64M_SIZEOF;
> +    case PCI_MSI_FLAGS_64BIT:
> +        return PCI_MSI_64_SIZEOF;
> +    case PCI_MSI_FLAGS_MASKBIT:
> +        return PCI_MSI_32M_SIZEOF;
> +    case 0:
> +        return PCI_MSI_32_SIZEOF;
> +    default:
> +        abort();
> +        break;
> +    }
> +    return 0;
> +}
> +

Note to self: this will go away too if we get rid of cap allocator.

> +static inline uint16_t msi_control_reg(const PCIDevice* dev)
> +{
> +    return dev->msi_cap + PCI_MSI_FLAGS;
> +}
> +
> +static inline uint32_t msi_lower_address_reg(const PCIDevice* dev)
> +{
> +    return dev->msi_cap + PCI_MSI_ADDRESS_LO;
> +}
> +
> +static inline uint32_t msi_upper_address_reg(const PCIDevice* dev)
> +{
> +    return dev->msi_cap + PCI_MSI_ADDRESS_HI;
> +}
> +

I think above are better open-coded - a bit less sure here as I
understand you want consistency between registers.
In any case, I think names should be consistent with macros
and be explicit in that they return an offset:
e.g. msi_lower_address_reg -> msi_address_lo_off
Also, return uint8_t and not uint32_t: these registers
always live in a traditional config space.


> +static inline uint16_t msi_data_reg(const PCIDevice* dev, bool is64bit)

msi64bit would be a better name than is64bit

> +{
> +    return dev->msi_cap + (is64bit ?  PCI_MSI_DATA_64 : PCI_MSI_DATA_32);
> +}
> +
> +static inline uint32_t msi_mask_reg(const PCIDevice* dev, bool is64bit)
> +{
> +    return dev->msi_cap + (is64bit ? PCI_MSI_MASK_64 : PCI_MSI_MASK_32);
> +}
> +
> +static inline uint32_t msi_pending_reg(const PCIDevice* dev, bool is64bit)
> +{
> +    return dev->msi_cap + (is64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
> +}
> +
> +bool msi_enabled(const PCIDevice *dev)
> +{
> +    return msi_present(dev) &&
> +        msi_control_enabled(pci_get_word(dev->config + msi_control_reg(dev)));
> +}
> +
> +int msi_init(struct PCIDevice *dev, uint8_t offset,
> +             uint8_t nr_vector, uint16_t flags)

rename nr_vector -> nr_vectors here and elsewhere

> +{
> +    uint8_t vector_order = ilog2(nr_vector);
> +    bool is64bit = is_64bit_address(flags);
> +    uint8_t cap_size;
> +    int config_offset;


maybe add assert on nr_vector: should be
	assert(!((nr_vector - 1) & nr_vector));
	assert(nr_vector > 0);
	assert(nr_vector <= 32);


> +    MSI_DEV_PRINTF(dev,
> +                   "init offset: 0x%"PRIx8" vector: %"PRId8
> +                   " flags 0x%"PRIx16"\n", offset, nr_vector, flags);
> +
> +    flags &= PCI_MSI_FLAGS_MASKBIT | PCI_MSI_FLAGS_64BIT;
> +    flags |= (vector_order << PCI_MSI_FLAGS_QMASK_SHIFT) & PCI_MSI_FLAGS_QMASK;
> +
> +    cap_size = msi_cap_sizeof(flags);
> +    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
> +    if (config_offset < 0) {
> +        return config_offset;
> +    }
> +
> +    dev->msi_cap = config_offset;
> +    dev->cap_present |= QEMU_PCI_CAP_MSI;
> +
> +    pci_set_word(dev->config + msi_control_reg(dev), flags);
> +    pci_set_word(dev->wmask + msi_control_reg(dev),
> +                 PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
> +    pci_set_long(dev->wmask + msi_lower_address_reg(dev),
> +                 ~PCI_MSI_ADDRESS_LO_RESERVED_MASK);
> +    if (is64bit) {
> +        pci_set_long(dev->wmask + msi_upper_address_reg(dev), 0xffffffff);
> +    }
> +    pci_set_word(dev->wmask + msi_data_reg(dev, is64bit), 0xffff);
> +
> +    if (is_mask_bit_support(flags)) {
> +        pci_set_long(dev->wmask + msi_mask_reg(dev, is64bit),
> +                     (1 << nr_vector) - 1);

1 is signed. I think this should be 1U << nr_vector, otherwise
you get signed integer overflow for nr >= 31.

> +    }
> +    return config_offset;
> +}
> +
> +void msi_uninit(struct PCIDevice *dev)
> +{
> +    uint16_t control = pci_get_word(dev->config + msi_control_reg(dev));
> +    uint8_t cap_size = msi_cap_sizeof(control);
> +    pci_del_capability(dev, PCI_CAP_ID_MSIX, cap_size);
> +    MSI_DEV_PRINTF(dev, "uninit\n");
> +}
> +
> +void msi_reset(PCIDevice *dev)
> +{
> +    uint16_t control;
> +    bool is64bit;
> +
> +    control = pci_get_word(dev->config + msi_control_reg(dev));
> +    control &= ~(PCI_MSI_FLAGS_QSIZE | PCI_MSI_FLAGS_ENABLE);
> +    is64bit = is_64bit_address(control);
> +
> +    pci_set_word(dev->config + msi_control_reg(dev), control);
> +    pci_set_long(dev->config + msi_lower_address_reg(dev), 0);
> +    if (is64bit) {
> +        pci_set_long(dev->config + msi_upper_address_reg(dev), 0);
> +    }
> +    pci_set_word(dev->config + msi_data_reg(dev, is64bit), 0);
> +    if (is_mask_bit_support(control)) {
> +        pci_set_long(dev->config + msi_mask_reg(dev, is64bit), 0);
> +        pci_set_long(dev->config + msi_pending_reg(dev, is64bit), 0);
> +    }
> +    MSI_DEV_PRINTF(dev, "reset\n");
> +}
> +
> +static bool msi_is_masked(const PCIDevice *dev, uint8_t vector)
> +{
> +    uint16_t control = pci_get_word(dev->config + msi_control_reg(dev));
> +    uint32_t mask;
> +
> +    if (!is_mask_bit_support(control)) {
> +        return false;
> +    }
> +
> +    mask = pci_get_long(dev->config +
> +                        msi_mask_reg(dev, is_64bit_address(control)));
> +    return msi_test_bit(mask, vector);
> +}
> +
> +static void msi_set_pending(PCIDevice *dev, uint8_t vector)
> +{
> +    uint16_t control = pci_get_word(dev->config + msi_control_reg(dev));
> +    bool is64bit = is_64bit_address(control);
> +    uint32_t pending;
> +
> +    assert(is_mask_bit_support(control));
> +
> +    pending = pci_get_long(dev->config + msi_pending_reg(dev, is64bit));
> +    msi_set_bit(&pending, vector);
> +    pci_set_long(dev->config + msi_pending_reg(dev, is64bit), pending);
> +    MSI_DEV_PRINTF(dev, "pending vector 0x%"PRIx8"\n", vector);
> +}
> +
> +void msi_notify(PCIDevice *dev, uint8_t vector)
> +{
> +    uint16_t control = pci_get_word(dev->config + msi_control_reg(dev));
> +    bool is64bit = is_64bit_address(control);
> +    uint8_t nr_vector = msi_nr_vector(control);
> +    uint64_t address;
> +    uint32_t data;
> +
> +    assert(vector < nr_vector);
> +    if (msi_is_masked(dev, vector)) {
> +        msi_set_pending(dev, vector);
> +        return;
> +    }
> +
> +    if (is64bit){
> +        address = pci_get_quad(dev->config + msi_lower_address_reg(dev));
> +    } else {
> +        address = pci_get_long(dev->config + msi_lower_address_reg(dev));
> +    }
> +
> +    /* upper bit 31:16 is zero */
> +    data = pci_get_word(dev->config + msi_data_reg(dev, is64bit));
> +    if (nr_vector > 1) {
> +        data &= ~(nr_vector - 1);
> +        data |= vector;
> +    }
> +
> +    MSI_DEV_PRINTF(dev,
> +                   "notify vector 0x%"PRIx8
> +                   " address: 0x%"PRIx64" data: 0x%"PRIx32"\n",
> +                   vector, address, data);
> +    stl_phys(address, data);
> +}
> +
> +/* call this function after updating configs by pci_default_write_config() */
> +void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
> +{
> +    uint16_t control = pci_get_word(dev->config + msi_control_reg(dev));
> +    bool is64bit = is_64bit_address(control);
> +    uint8_t nr_vector = msi_nr_vector(control);
> +
> +#ifdef MSI_DEBUG
> +    uint8_t cap_size = msi_cap_sizeof(control & (PCI_MSI_FLAGS_MASKBIT |
> +                                                 PCI_MSI_FLAGS_64BIT));
> +    if (ranges_overlap(addr, len, dev->msi_cap, cap_size)) {
> +        MSI_DEV_PRINTF(dev, "addr 0x%"PRIx32" val 0x%"PRIx32" len %d\n",
> +                       addr, val, len);
> +        MSI_DEV_PRINTF(dev, "ctrl: 0x%"PRIx16" address: 0x%"PRIx32,
> +                       control,
> +                       pci_get_long(dev->config + msi_lower_address_reg(dev)));
> +        if (is64bit) {
> +            fprintf(stderr, " addrss-hi: 0x%"PRIx32,
> +                    pci_get_long(dev->config + msi_upper_address_reg(dev)));
> +        }
> +        fprintf(stderr, " data: 0x%"PRIx16,
> +                pci_get_word(dev->config + msi_data_reg(dev, is64bit)));
> +        if (is_mask_bit_support(control)) {
> +            fprintf(stderr, " mask 0x%"PRIx32" pending 0x%"PRIx32,
> +                    pci_get_long(dev->config + msi_mask_reg(dev, is64bit)),
> +                    pci_get_long(dev->config + msi_pending_reg(dev, is64bit)));
> +        }
> +        fprintf(stderr, "\n");
> +    }
> +#endif
> +
> +    if (!msi_control_enabled(control)) {

Should not we clear pending bits here?

> +        return;
> +    }

We must also clear INTx# interrupts when msi is enabled.

> +
> +    if (!is_mask_bit_support(control)) {
> +        /* if per vector masking isn't support,
> +           there is no pending interrupt. */
> +        return;
> +    }
> +

I think we also need to clear pending bits if # of vectors
is reduced.

We need to also decide what to do if software tries to
enable more vectors than supported. I suggest setting
allocated vectors to nr_vectors in this case.


> +    if (range_covers_byte(addr, len, msi_control_reg(dev)) || /* MSI enable */
> +        ranges_overlap(addr, len, msi_mask_reg(dev, is64bit), 4)) {

I think it is better to reduce nesting.
Reverse the condition and return if this is not an msi write.

> +        uint32_t pending =
> +            pci_get_long(dev->config + msi_pending_reg(dev, is64bit));
> +        uint8_t vector;
> +
> +        /* deliver pending interrupts which are unmasked */
> +        for (vector = 0; vector < nr_vector; ++vector) {
> +            if (msi_is_masked(dev, vector) || !msi_test_bit(pending, vector)) {

I am confused. This is called after mask is updated, right?
So msi_is_masked means vector was masked, not unmasked?
I think the logic is reversed here.

> +                continue;
> +            }
> +
> +            msi_clear_bit(&pending, vector);
> +            pci_set_long(dev->config + msi_pending_reg(dev, is64bit), pending);
> +            msi_notify(dev, vector);
> +        }
> +    }
> +}
> +
> +uint8_t msi_nr_allocated_vector(const PCIDevice *dev)

rename msi_nr_vectors_allocated to match spec.

> +{
> +    uint16_t control = pci_get_word(dev->config + msi_control_reg(dev));
> +    return msi_nr_vector(control);
> +}
> diff --git a/hw/msi.h b/hw/msi.h
> new file mode 100644
> index 0000000..1131c6e
> --- /dev/null
> +++ b/hw/msi.h
> @@ -0,0 +1,41 @@
> +/*
> + * msi.h
> + *
> + * Copyright (c) 2010 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef QEMU_MSI_H
> +#define QEMU_MSI_H
> +
> +#include "qemu-common.h"
> +#include "pci.h"
> +
> +bool msi_enabled(const PCIDevice *dev);
> +int msi_init(struct PCIDevice *dev, uint8_t offset,
> +             uint8_t nr_vector, uint16_t flags);

So users need to encode flags? Where do they get them?
And to encode, we need registers internal to msi.c or duplicate ...
Would not it be better to pass msi_64_bit and msi_per_vector_mask?


> +void msi_uninit(struct PCIDevice *dev);
> +void msi_reset(PCIDevice *dev);
> +void msi_notify(PCIDevice *dev, uint8_t vector);
> +void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
> +uint8_t msi_nr_allocated_vector(const PCIDevice *dev);
> +
> +static inline bool msi_present(const PCIDevice *dev)
> +{
> +    return dev->cap_present & QEMU_PCI_CAP_MSI;
> +}
> +
> +#endif /* QEMU_MSI_H */
> diff --git a/hw/pci.h b/hw/pci.h
> index 2b4c318..9387a03 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -115,6 +115,8 @@ enum {
>      /* multifunction capable device */
>  #define QEMU_PCI_CAP_MULTIFUNCTION_BITNR        2
>      QEMU_PCI_CAP_MULTIFUNCTION = (1 << QEMU_PCI_CAP_MULTIFUNCTION_BITNR),
> +
> +    QEMU_PCI_CAP_MSI = 0x4,

These are internal bits so we do not need to preserve any values.
Put this cap right before or right after MSIX please.

>  };
>  
>  struct PCIDevice {
> @@ -168,6 +170,9 @@ struct PCIDevice {
>      /* Version id needed for VMState */
>      int32_t version_id;
>  
> +    /* Offset of MSI capability in config space */
> +    uint8_t msi_cap;
> +
>      /* Location of option rom */
>      char *romfile;
>      ram_addr_t rom_offset;
> -- 
> 1.7.1.1