Re: [Qemu-devel] [PATCH v2 8/9] msix: Align MSI-X constants to libpci definitions and extend them

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

On Wed, Jun 08, 2011 at 06:21:51PM +0200, Jan Kiszka wrote:
> Add PCI_MSIX_TABLE and PCI_MSIX_PBA, align other MSIX related constant
> names to libpci style. Will be used for device assignment code in
> qemu-kvm.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Besides keeping pci_regs.h aligned with the original,
I also think ideally pci register banging should stay
within the pci subsystem.

Could we add high-level APIs to help with that,
instead of having kvm look at config space directly?


> ---
>  hw/msix.c     |   24 +++++++++++-------------
>  hw/pci_regs.h |   14 ++++++++------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 600f5fb..b20cf7c 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -16,15 +16,12 @@
>  #include "pci.h"
>  #include "range.h"
>  
> -/* MSI-X capability structure */
> -#define MSIX_TABLE_OFFSET 4
> -#define MSIX_PBA_OFFSET 8
>  #define MSIX_CAP_LENGTH 12
>  
> -/* MSI enable bit and maskall bit are in byte 1 in FLAGS register */
> -#define MSIX_CONTROL_OFFSET (PCI_MSIX_FLAGS + 1)
> -#define MSIX_ENABLE_MASK (PCI_MSIX_FLAGS_ENABLE >> 8)
> -#define MSIX_MASKALL_MASK (PCI_MSIX_FLAGS_MASKALL >> 8)
> +/* MSI enable bit and maskall bit are in byte 1 in control register */
> +#define MSIX_CONTROL_OFFSET (PCI_MSIX_CTRL + 1)
> +#define MSIX_ENABLE_MASK (PCI_MSIX_ENABLE >> 8)
> +#define MSIX_MASKALL_MASK (PCI_MSIX_MASK >> 8)
>  
>  /* MSI-X table format */
>  #define MSIX_MSG_ADDR 0
> @@ -58,8 +55,9 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>      uint8_t *config;
>      uint32_t new_size;
>  
> -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1)
> +    if (nentries < 1 || nentries > PCI_MSIX_TABSIZE + 1) {
>          return -EINVAL;
> +    }
>      if (bar_size > 0x80000000)
>          return -ENOSPC;
>  
> @@ -80,11 +78,11 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>          return config_offset;
>      config = pdev->config + config_offset;
>  
> -    pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1);
> +    pci_set_word(config + PCI_MSIX_CTRL, nentries - 1);
>      /* Table on top of BAR */
> -    pci_set_long(config + MSIX_TABLE_OFFSET, bar_size | bar_nr);
> +    pci_set_long(config + PCI_MSIX_TABLE, bar_size | bar_nr);
>      /* Pending bits on top of that */
> -    pci_set_long(config + MSIX_PBA_OFFSET, (bar_size + MSIX_PAGE_PENDING) |
> +    pci_set_long(config + PCI_MSIX_PBA, (bar_size + MSIX_PAGE_PENDING) |
>                   bar_nr);
>      pdev->msix_cap = config_offset;
>      /* Make flags bit writable. */
> @@ -208,11 +206,11 @@ void msix_mmio_map(PCIDevice *d, int region_num,
>                     pcibus_t addr, pcibus_t size, int type)
>  {
>      uint8_t *config = d->config + d->msix_cap;
> -    uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> +    uint32_t table = pci_get_long(config + PCI_MSIX_TABLE);
>      uint32_t offset = table & ~(MSIX_PAGE_SIZE - 1);
>      /* TODO: for assigned devices, we'll want to make it possible to map
>       * pending bits separately in case they are in a separate bar. */
> -    int table_bir = table & PCI_MSIX_FLAGS_BIRMASK;
> +    int table_bir = table & PCI_MSIX_BIR;
>  
>      if (table_bir != region_num)
>          return;
> diff --git a/hw/pci_regs.h b/hw/pci_regs.h
> index 5a5ab89..c17c22f 100644
> --- a/hw/pci_regs.h
> +++ b/hw/pci_regs.h
> @@ -300,12 +300,14 @@
>  #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
>  #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
>  
> -/* MSI-X registers (these are at offset PCI_MSIX_FLAGS) */
> -#define PCI_MSIX_FLAGS		2
> -#define  PCI_MSIX_FLAGS_QSIZE	0x7FF
> -#define  PCI_MSIX_FLAGS_ENABLE	(1 << 15)
> -#define  PCI_MSIX_FLAGS_MASKALL	(1 << 14)
> -#define PCI_MSIX_FLAGS_BIRMASK	(7 << 0)
> +/* MSI-X registers */
> +#define PCI_MSIX_CTRL           2       /* Message control */
> +#define  PCI_MSIX_TABSIZE       0x7FF   /* Table size - 1 */
> +#define  PCI_MSIX_MASK          0x4000  /* Mask all vectors */
> +#define  PCI_MSIX_ENABLE        0x8000  /* Enable MSI-X */
> +#define PCI_MSIX_TABLE          4       /* MSI-X table */
> +#define PCI_MSIX_PBA            8       /* Pending bit array */
> +#define  PCI_MSIX_BIR           0x7     /* BAR indication register */
>  
>  /* CompactPCI Hotswap Register */
>  
> -- 
> 1.7.1