>>> On 17.02.12 at 18:08, Anthony PERARD <anthony.per...@citrix.com> wrote:
Wouldn't thus much better be merged into the prior patch(es)? After
all you're not trying to reconstruct the Xen-side history of this code
anyway.
Jan
> From: Shan Haitao <maillists.s...@gmail.com>
>
> This patch does cleaning up of QEMU MSI handling. The fixes are:
> 1. Changes made to MSI-X table mapping handling to eliminate the small
> windows in which guest could have access to physical MSI-X table.
> 2. MSI-X table is mapped as read-only to QEMU, as masking of MSI-X is
> already in Xen now.
> 3. For registers that coexists inside the MSI-X table (this could be
> only PBA I think), value read from physical page would be returned.
>
> Signed-off-by: Shan Haitao <maillists.s...@gmail.com>
>
> Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting.
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> Signed-off-by: Anthony PERARD <anthony.per...@citrix.com>
> ---
> hw/xen_pci_passthrough.c | 80
> ++++++++++++++++++++++++++++++++----------
> hw/xen_pci_passthrough.h | 11 +++++-
> hw/xen_pci_passthrough_msi.c | 62 ++++----------------------------
> 3 files changed, 78 insertions(+), 75 deletions(-)
>
> diff --git a/hw/xen_pci_passthrough.c b/hw/xen_pci_passthrough.c
> index 6ea2c45..fab9fbe 100644
> --- a/hw/xen_pci_passthrough.c
> +++ b/hw/xen_pci_passthrough.c
> @@ -356,6 +356,53 @@ out:
> }
> }
>
> +static int pt_iomem_helper(XenPCIPassthroughState *s, int i,
> + pcibus_t e_base, pcibus_t e_size, int op)
> +{
> + uint64_t msix_last_pfn = 0;
> + pcibus_t bar_last_pfn = 0;
> + pcibus_t msix_table_size = 0;
> + XenPTMSIX *msix = s->msix;
> + int rc = 0;
> +
> + if (!pt_has_msix_mapping(s, i)) {
> + return xc_domain_memory_mapping(xen_xc, xen_domid,
> + PFN(e_base),
> + PFN(s->bases[i].access.maddr),
> + PFN(e_size + XC_PAGE_SIZE - 1),
> + op);
> +
> + }
> +
> + if (msix->table_off) {
> + rc = xc_domain_memory_mapping(xen_xc, xen_domid,
> + PFN(e_base),
> + PFN(s->bases[i].access.maddr),
> + PFN(msix->mmio_base_addr) -
> PFN(e_base),
> + op);
> + }
> +
> + msix_table_size = msix->total_entries * PCI_MSIX_ENTRY_SIZE;
> + msix_last_pfn = PFN(msix->mmio_base_addr + msix_table_size - 1);
> + bar_last_pfn = PFN(e_base + e_size - 1);
> +
> + if (rc == 0 && msix_last_pfn != bar_last_pfn) {
> + assert(msix_last_pfn < bar_last_pfn);
> +
> + rc = xc_domain_memory_mapping(xen_xc, xen_domid,
> + msix_last_pfn + 1,
> + PFN(s->bases[i].access.maddr
> + + msix->table_off
> + + msix_table_size
> + + XC_PAGE_SIZE - 1),
> + bar_last_pfn - msix_last_pfn,
> + op);
> + }
> +
> + return rc;
> +}
> +
> +
> /* ioport/iomem space*/
> static void pt_iomem_map(XenPCIPassthroughState *s, int i,
> pcibus_t e_phys, pcibus_t e_size)
> @@ -376,13 +423,10 @@ static void pt_iomem_map(XenPCIPassthroughState *s, int
> i,
> }
>
> if (!first_map && old_ebase != PT_PCI_BAR_UNMAPPED) {
> - pt_add_msix_mapping(s, i);
> - /* Remove old mapping */
> - rc = xc_domain_memory_mapping(xen_xc, xen_domid,
> - old_ebase >> XC_PAGE_SHIFT,
> - s->bases[i].access.maddr >> XC_PAGE_SHIFT,
> - (e_size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT,
> - DPCI_REMOVE_MAPPING);
> + if (pt_has_msix_mapping(s, i)) {
> + memory_region_set_enabled(&s->msix->mmio, false);
> + }
> + rc = pt_iomem_helper(s, i, old_ebase, e_size, DPCI_REMOVE_MAPPING);
> if (rc) {
> PT_ERR(&s->dev, "remove old mapping failed! (rc: %i)\n", rc);
> return;
> @@ -391,21 +435,19 @@ static void pt_iomem_map(XenPCIPassthroughState *s, int
> i,
>
> /* map only valid guest address */
> if (e_phys != PCI_BAR_UNMAPPED) {
> - /* Create new mapping */
> - rc = xc_domain_memory_mapping(xen_xc, xen_domid,
> - s->bases[i].e_physbase >> XC_PAGE_SHIFT,
> - s->bases[i].access.maddr >> XC_PAGE_SHIFT,
> - (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
> - DPCI_ADD_MAPPING);
> + if (pt_has_msix_mapping(s, i)) {
> + XenPTMSIX *msix = s->msix;
>
> - if (rc) {
> - PT_ERR(&s->dev, "create new mapping failed! (rc: %i)\n", rc);
> + msix->mmio_base_addr = s->bases[i].e_physbase + msix->table_off;
> +
> + memory_region_set_enabled(&msix->mmio, true);
> }
>
> - rc = pt_remove_msix_mapping(s, i);
> - if (rc != 0) {
> - PT_ERR(&s->dev, "Remove MSI-X MMIO mapping failed! (rc: %d)\n",
> - rc);
> + rc = pt_iomem_helper(s, i, e_phys, e_size, DPCI_ADD_MAPPING);
> +
> + if (rc) {
> + PT_ERR(&s->dev, "create new mapping failed! (rc: %i)\n", rc);
> + return;
> }
>
> if (old_ebase != e_phys && old_ebase != -1) {
> diff --git a/hw/xen_pci_passthrough.h b/hw/xen_pci_passthrough.h
> index 3d41e04..f36e0d2 100644
> --- a/hw/xen_pci_passthrough.h
> +++ b/hw/xen_pci_passthrough.h
> @@ -30,6 +30,9 @@ void pt_log(const PCIDevice *d, const char *f, ...)
> GCC_FMT_ATTR(2, 3);
> #endif
>
>
> +/* Helper */
> +#define PFN(x) ((x) >> XC_PAGE_SHIFT)
> +
> typedef struct XenPTRegInfo XenPTRegInfo;
> typedef struct XenPTReg XenPTReg;
>
> @@ -295,7 +298,11 @@ void pt_msix_delete(XenPCIPassthroughState *s);
> int pt_msix_update(XenPCIPassthroughState *s);
> int pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
> void pt_msix_disable(XenPCIPassthroughState *s);
> -int pt_add_msix_mapping(XenPCIPassthroughState *s, int bar_index);
> -int pt_remove_msix_mapping(XenPCIPassthroughState *s, int bar_index);
> +
> +static inline bool pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
> +{
> + return s->msix && s->msix->bar_index == bar;
> +}
> +
>
> #endif /* !QEMU_HW_XEN_PCI_PASSTHROUGH_H */
> diff --git a/hw/xen_pci_passthrough_msi.c b/hw/xen_pci_passthrough_msi.c
> index e848c61..e775d21 100644
> --- a/hw/xen_pci_passthrough_msi.c
> +++ b/hw/xen_pci_passthrough_msi.c
> @@ -300,16 +300,6 @@ static int msix_set_enable(XenPCIPassthroughState *s,
> bool enabled)
> enabled);
> }
>
> -static void mask_physical_msix_entry(XenPCIPassthroughState *s,
> - int entry_nr, int mask)
> -{
> - void *phys_off;
> -
> - phys_off = s->msix->phys_iomem_base + PCI_MSIX_ENTRY_SIZE * entry_nr
> - + PCI_MSIX_ENTRY_VECTOR_CTRL;
> - *(uint32_t *)phys_off = mask;
> -}
> -
> static int pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
> {
> XenPTMSIXEntry *entry = NULL;
> @@ -480,8 +470,6 @@ static void pci_msix_write(void *opaque,
> target_phys_addr_t addr,
> if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> pt_msix_update_one(s, entry_nr);
> }
> - mask_physical_msix_entry(s, entry_nr,
> - entry->vector_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT);
> }
> }
>
> @@ -493,14 +481,19 @@ static uint64_t pci_msix_read(void *opaque,
> target_phys_addr_t addr,
> int entry_nr, offset;
>
> entry_nr = addr / PCI_MSIX_ENTRY_SIZE;
> - if (entry_nr < 0 || entry_nr >= msix->total_entries) {
> + if (entry_nr < 0) {
> PT_ERR(&s->dev, "asked MSI-X entry '%i' invalid!\n", entry_nr);
> return 0;
> }
>
> offset = addr % PCI_MSIX_ENTRY_SIZE;
>
> - return get_entry_value(&msix->msix_entry[entry_nr], offset);
> + if (addr < msix->total_entries * PCI_MSIX_ENTRY_SIZE) {
> + return get_entry_value(&msix->msix_entry[entry_nr], offset);
> + } else {
> + /* Pending Bit Array (PBA) */
> + return *(uint32_t *)(msix->phys_iomem_base + addr);
> + }
> }
>
> static const MemoryRegionOps pci_msix_ops = {
> @@ -514,45 +507,6 @@ static const MemoryRegionOps pci_msix_ops = {
> },
> };
>
> -int pt_add_msix_mapping(XenPCIPassthroughState *s, int bar_index)
> -{
> - XenPTMSIX *msix = s->msix;
> -
> - if (!(s->msix && s->msix->bar_index == bar_index)) {
> - return 0;
> - }
> -
> - memory_region_set_enabled(&msix->mmio, false);
> - return xc_domain_memory_mapping(xen_xc, xen_domid,
> - s->msix->mmio_base_addr >> XC_PAGE_SHIFT,
> - (s->bases[bar_index].access.maddr + s->msix->table_off)
> - >> XC_PAGE_SHIFT,
> - (s->msix->total_entries * PCI_MSIX_ENTRY_SIZE + XC_PAGE_SIZE - 1)
> - >> XC_PAGE_SHIFT,
> - DPCI_ADD_MAPPING);
> -}
> -
> -int pt_remove_msix_mapping(XenPCIPassthroughState *s, int bar_index)
> -{
> - XenPTMSIX *msix = s->msix;
> -
> - if (!(msix && msix->bar_index == bar_index)) {
> - return 0;
> - }
> -
> - memory_region_set_enabled(&msix->mmio, true);
> -
> - msix->mmio_base_addr = s->bases[bar_index].e_physbase + msix->table_off;
> -
> - return xc_domain_memory_mapping(xen_xc, xen_domid,
> - s->msix->mmio_base_addr >> XC_PAGE_SHIFT,
> - (s->bases[bar_index].access.maddr + s->msix->table_off)
> - >> XC_PAGE_SHIFT,
> - (s->msix->total_entries * PCI_MSIX_ENTRY_SIZE + XC_PAGE_SIZE - 1)
> - >> XC_PAGE_SHIFT,
> - DPCI_REMOVE_MAPPING);
> -}
> -
> int pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
> {
> uint8_t id = 0;
> @@ -609,7 +563,7 @@ int pt_msix_init(XenPCIPassthroughState *s, uint32_t
> base)
> msix->phys_iomem_base =
> mmap(NULL,
> total_entries * PCI_MSIX_ENTRY_SIZE + msix->table_offset_adjust,
> - PROT_WRITE | PROT_READ,
> + PROT_READ,
> MAP_SHARED | MAP_LOCKED,
> fd,
> msix->table_base + table_off - msix->table_offset_adjust);
> --
> Anthony PERARD
