From 860192ce0d4fa1eaa7fc2766b1ba9c2211b4be2d Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Thu, 8 Mar 2012 11:00:17 +0000 Subject: x86/vMSI: miscellaneous fixes This addresses a number of problems in msixtbl_{read,write}(): - address alignment was not checked, allowing for memory corruption in the hypervisor (write case) or returning of hypervisor private data to the guest (read case) - the interrupt mask bit was permitted to be written by the guest (while Xen's interrupt flow control routines need to control it) - MAX_MSIX_TABLE_{ENTRIES,PAGES} were pointlessly defined to plain numbers (making it unobvious why they have these values, and making the latter non-portable) - MAX_MSIX_TABLE_PAGES was also off by one (failing to account for a non-zero table offset); this was also affecting host MSI-X code - struct msixtbl_entry's table_flags[] was one element larger than necessary due to improper open-coding of BITS_TO_LONGS() - msixtbl_read() unconditionally accessed the physical table, even though the data was only needed in a quarter of all cases - various calculations were done unnecessarily for both of the rather distinct code paths in msixtbl_read() Additionally it is unclear on what basis MAX_MSIX_ACC_ENTRIES was chosen to be 3. Signed-off-by: Jan Beulich Committed-by: Keir Fraser xen-unstable changeset: 24535:fb81b807c154 xen-unstable date: Mon Jan 23 09:35:17 2012 +0000 --- xen/arch/x86/hvm/vmsi.c | 38 ++++++++++++++++++++++++-------------- xen/include/asm-x86/msi.h | 6 ------ xen/include/xen/pci.h | 8 ++++++-- xen/include/xen/pci_regs.h | 7 +++++++ 4 files changed, 37 insertions(+), 22 deletions(-) diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index d9ec9fb0a4..87f3bd1396 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -157,7 +157,7 @@ struct msixtbl_entry struct pci_dev *pdev; unsigned long gtable; /* gpa of msix table */ unsigned long table_len; - unsigned long table_flags[MAX_MSIX_TABLE_ENTRIES / BITS_PER_LONG + 1]; + unsigned long table_flags[BITS_TO_LONGS(MAX_MSIX_TABLE_ENTRIES)]; struct rcu_head rcu; }; @@ -179,17 +179,14 @@ static struct msixtbl_entry *msixtbl_find_entry( static void __iomem *msixtbl_addr_to_virt( struct msixtbl_entry *entry, unsigned long addr) { - int idx, nr_page; + unsigned int idx, nr_page; - if ( !entry ) + if ( !entry || !entry->pdev ) return NULL; nr_page = (addr >> PAGE_SHIFT) - (entry->gtable >> PAGE_SHIFT); - if ( !entry->pdev ) - return NULL; - idx = entry->pdev->msix_table_idx[nr_page]; if ( !idx ) return NULL; @@ -207,10 +204,10 @@ static int msixtbl_read( void *virt; int r = X86EMUL_UNHANDLEABLE; - rcu_read_lock(); + if ( len != 4 || (address & 3) ) + return r; - if ( len != 4 ) - goto out; + rcu_read_lock(); offset = address & (PCI_MSIX_ENTRY_SIZE - 1); if ( offset != PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) @@ -235,13 +232,13 @@ static int msixtbl_write(struct vcpu *v, unsigned long address, unsigned long offset; struct msixtbl_entry *entry; void *virt; - int nr_entry; + unsigned int nr_entry; int r = X86EMUL_UNHANDLEABLE; - rcu_read_lock(); + if ( len != 4 || (address & 3) ) + return r; - if ( len != 4 ) - goto out; + rcu_read_lock(); entry = msixtbl_find_entry(v, address); nr_entry = (address - entry->gtable) / PCI_MSIX_ENTRY_SIZE; @@ -261,9 +258,22 @@ static int msixtbl_write(struct vcpu *v, unsigned long address, if ( !virt ) goto out; + /* Do not allow the mask bit to be changed. */ +#if 0 /* XXX + * As the mask bit is the only defined bit in the word, and as the + * host MSI-X code doesn't preserve the other bits anyway, doing + * this is pointless. So for now just discard the write (also + * saving us from having to determine the matching irq_desc). + */ + spin_lock_irqsave(&desc->lock, flags); + orig = readl(virt); + val &= ~PCI_MSIX_VECTOR_BITMASK; + val |= orig & PCI_MSIX_VECTOR_BITMASK; writel(val, virt); - r = X86EMUL_OKAY; + spin_unlock_irqrestore(&desc->lock, flags); +#endif + r = X86EMUL_OKAY; out: rcu_read_unlock(); return r; diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h index bc8ab0e870..75e73dd259 100644 --- a/xen/include/asm-x86/msi.h +++ b/xen/include/asm-x86/msi.h @@ -127,12 +127,6 @@ extern const struct hw_interrupt_type pci_msi_type; #define PCI_MSIX_FLAGS_BIRMASK (7 << 0) #define PCI_MSIX_FLAGS_BITMASK (1 << 0) -#define PCI_MSIX_ENTRY_SIZE 16 -#define PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET 0 -#define PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET 4 -#define PCI_MSIX_ENTRY_DATA_OFFSET 8 -#define PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET 12 - #define msi_control_reg(base) (base + PCI_MSI_FLAGS) #define msi_lower_address_reg(base) (base + PCI_MSI_ADDRESS_LO) #define msi_upper_address_reg(base) (base + PCI_MSI_ADDRESS_HI) diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 2f2014fd64..634ed106d8 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -11,6 +11,8 @@ #include #include #include +#include +#include /* * The PCI interface treats multi-function devices as independent @@ -29,8 +31,10 @@ #define PCI_BDF(b,d,f) ((((b) & 0xff) << 8) | PCI_DEVFN(d,f)) #define PCI_BDF2(b,df) ((((b) & 0xff) << 8) | ((df) & 0xff)) -#define MAX_MSIX_TABLE_ENTRIES 2048 -#define MAX_MSIX_TABLE_PAGES 8 +#define MAX_MSIX_TABLE_ENTRIES (PCI_MSIX_FLAGS_QSIZE + 1) +#define MAX_MSIX_TABLE_PAGES PFN_UP(MAX_MSIX_TABLE_ENTRIES * \ + PCI_MSIX_ENTRY_SIZE + \ + (~PCI_MSIX_FLAGS_BIRMASK & (PAGE_SIZE - 1))) struct pci_dev_info { unsigned is_extfn; unsigned is_virtfn; diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h index b92a171052..8bd193a5d7 100644 --- a/xen/include/xen/pci_regs.h +++ b/xen/include/xen/pci_regs.h @@ -302,6 +302,13 @@ #define PCI_MSIX_FLAGS_ENABLE (1 << 15) #define PCI_MSIX_FLAGS_MASKALL (1 << 14) #define PCI_MSIX_FLAGS_BIRMASK (7 << 0) + +#define PCI_MSIX_ENTRY_SIZE 16 +#define PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET 0 +#define PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET 4 +#define PCI_MSIX_ENTRY_DATA_OFFSET 8 +#define PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET 12 + #define PCI_MSIX_FLAGS_BITMASK (1 << 0) /* CompactPCI Hotswap Register */ -- cgit v1.2.3