Re: [Qemu-devel] [PATCH V7 11/11] xen passthrough: clean up MSI-X table handling

[Jan Beulich]

>>> On 17.02.12 at 18:08, Anthony PERARD <address@hidden> 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 <address@hidden>
> 
> 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 <address@hidden>
> 
> Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting.
> 
> Signed-off-by: Jan Beulich <address@hidden>
> Signed-off-by: Anthony PERARD <address@hidden>
> ---
>  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