|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH] Qemu MSI Cleaning Up
>>> On 22.09.11 at 02:35, Haitao Shan <maillists.shan@xxxxxxxxx> wrote:
> Hi, Ian, Keir, Jan,
>
> 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.
You should mention that writes to the mask bits get only used for
feeding data back into reads with that change (i.e. actually masking
an interrupt isn't possible), which ought to be addressed in a
subsequent change (no matter that KVM doing so as well appears to
be getting away up to now).
> Could you please have review?
>
> Signed-off-by: Shan Haitao <haitao.shan@xxxxxxxxx>
>
>
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 9c5620d..6ebed64 100644
> --- a/hw/pass-through.c
> +++ b/hw/pass-through.c
> @@ -92,6 +92,7 @@
>
> #include <unistd.h>
> #include <sys/ioctl.h>
> +#include <assert.h>
>
> extern int gfx_passthru;
> int igd_passthru = 0;
> @@ -1103,6 +1104,7 @@ static void pt_iomem_map(PCIDevice *d, int i,
> uint32_t e_phys, uint32_t e_size,
> {
> struct pt_dev *assigned_device = (struct pt_dev *)d;
> uint32_t old_ebase = assigned_device->bases[i].e_physbase;
> + uint32_t msix_last_pfn = 0, bar_last_pfn = 0;
> int first_map = ( assigned_device->bases[i].e_size == 0 );
> int ret = 0;
>
> @@ -1118,39 +1120,127 @@ static void pt_iomem_map(PCIDevice *d, int i,
> uint32_t e_phys, uint32_t e_size,
>
> if ( !first_map && old_ebase != -1 )
> {
> - add_msix_mapping(assigned_device, i);
> - /* Remove old mapping */
> - ret = xc_domain_memory_mapping(xc_handle, domid,
> + if ( has_msix_mapping(assigned_device, i) )
> + {
> + msix_last_pfn = (assigned_device->msix->mmio_base_addr - 1 +
> + assigned_device->msix->total_entries * 16) >>
> XC_PAGE_SHIFT;
> + bar_last_pfn = (old_ebase + e_size - 1) >> XC_PAGE_SHIFT;
> +
> + unregister_iomem(assigned_device->msix->mmio_base_addr);
> +
> + if ( assigned_device->msix->table_off )
> + {
> + ret = xc_domain_memory_mapping(xc_handle, domid,
> + old_ebase >> XC_PAGE_SHIFT,
> + assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
> + (assigned_device->msix->mmio_base_addr >> XC_PAGE_SHIFT)
> + - (old_ebase >> XC_PAGE_SHIFT),
> + DPCI_REMOVE_MAPPING);
Proper indentation (and possibly using some more local variables) would
certainly help reviewing as well as future readers (also further down).
Which also raises the question of what the plans are wrt the upstream
qemu variant.
Overall the changes look good to me, but I'm hesitant to formally ack
it as I'm too unfamiliar with the qemu code.
In any case, unless we're concerned about old qemu, we should remove
the special treatment of Dom0 by the hypervisor (in allowing it write
access to these pages) once the other qemu flavor also got fixed.
Jan
> + if ( ret != 0 )
> + {
> + PT_LOG("Error: remove old mapping failed!\n");
> + return;
> + }
> + }
> + if ( msix_last_pfn != bar_last_pfn )
> + {
> + assert(msix_last_pfn < bar_last_pfn);
> + ret = xc_domain_memory_mapping(xc_handle, domid,
> + msix_last_pfn + 1,
> + (assigned_device->bases[i].access.maddr +
> + assigned_device->msix->table_off +
> + assigned_device->msix->total_entries * 16 +
> + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
> + bar_last_pfn - msix_last_pfn,
> + DPCI_REMOVE_MAPPING);
> + if ( ret != 0 )
> + {
> + PT_LOG("Error: remove old mapping failed!\n");
> + return;
> + }
> + }
> + }
> + else
> + {
> + /* Remove old mapping */
> + ret = xc_domain_memory_mapping(xc_handle, domid,
> old_ebase >> XC_PAGE_SHIFT,
> assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
> (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
> DPCI_REMOVE_MAPPING);
> - if ( ret != 0 )
> - {
> - PT_LOG("Error: remove old mapping failed!\n");
> - return;
> + if ( ret != 0 )
> + {
> + PT_LOG("Error: remove old mapping failed!\n");
> + return;
> + }
> }
> }
>
> /* map only valid guest address */
> if (e_phys != -1)
> {
> - /* Create new mapping */
> - ret = xc_domain_memory_mapping(xc_handle, domid,
> + if ( has_msix_mapping(assigned_device, i) )
> + {
> + assigned_device->msix->mmio_base_addr =
> + assigned_device->bases[i].e_physbase
> + + assigned_device->msix->table_off;
> +
> + msix_last_pfn = (assigned_device->msix->mmio_base_addr - 1 +
> + assigned_device->msix->total_entries * 16) >>
> XC_PAGE_SHIFT;
> + bar_last_pfn = (e_phys + e_size - 1) >> XC_PAGE_SHIFT;
> +
> +
> cpu_register_physical_memory(assigned_device->msix->mmio_base_addr,
> + (assigned_device->msix->total_entries * 16 + XC_PAGE_SIZE -
> 1)
> + & XC_PAGE_MASK,
> + assigned_device->msix->mmio_index);
> +
> + if ( assigned_device->msix->table_off )
> + {
> + ret = xc_domain_memory_mapping(xc_handle, domid,
> + assigned_device->bases[i].e_physbase >> XC_PAGE_SHIFT,
> + assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
> + (assigned_device->msix->mmio_base_addr >> XC_PAGE_SHIFT)
> + - (assigned_device->bases[i].e_physbase >>
> XC_PAGE_SHIFT),
> + DPCI_ADD_MAPPING);
> + if ( ret != 0 )
> + {
> + PT_LOG("Error: remove old mapping failed!\n");
> + return;
> + }
> + }
> + if ( msix_last_pfn != bar_last_pfn )
> + {
> + assert(msix_last_pfn < bar_last_pfn);
> + ret = xc_domain_memory_mapping(xc_handle, domid,
> + msix_last_pfn + 1,
> + (assigned_device->bases[i].access.maddr +
> + assigned_device->msix->table_off +
> + assigned_device->msix->total_entries * 16 +
> + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
> + bar_last_pfn - msix_last_pfn,
> + DPCI_ADD_MAPPING);
> + if ( ret != 0 )
> + {
> + PT_LOG("Error: remove old mapping failed!\n");
> + return;
> + }
> + }
> + }
> + else
> + {
> + /* Create new mapping */
> + ret = xc_domain_memory_mapping(xc_handle, domid,
> assigned_device->bases[i].e_physbase >> XC_PAGE_SHIFT,
> assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
> (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
> DPCI_ADD_MAPPING);
>
> - if ( ret != 0 )
> - {
> - PT_LOG("Error: create new mapping failed!\n");
> + if ( ret != 0 )
> + {
> + PT_LOG("Error: create new mapping failed!\n");
> + }
> }
>
> - ret = remove_msix_mapping(assigned_device, i);
> - if ( ret != 0 )
> - PT_LOG("Error: remove MSI-X mmio mapping failed!\n");
> -
> if ( old_ebase != e_phys && old_ebase != -1 )
> pt_msix_update_remap(assigned_device, i);
> }
> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> index 71fa6f0..0134e62 100644
> --- a/hw/pt-msi.c
> +++ b/hw/pt-msi.c
> @@ -284,15 +284,6 @@ void pt_disable_msi_translate(struct pt_dev *dev)
> dev->msi_trans_en = 0;
> }
>
> -/* MSI-X virtulization functions */
> -static void mask_physical_msix_entry(struct pt_dev *dev, int
> entry_nr, int mask)
> -{
> - void *phys_off;
> -
> - phys_off = dev->msix->phys_iomem_base + 16 * entry_nr + 12;
> - *(uint32_t *)phys_off = mask;
> -}
> -
> static int pt_msix_update_one(struct pt_dev *dev, int entry_nr)
> {
> struct msix_entry_info *entry = &dev->msix->msix_entry[entry_nr];
> @@ -486,7 +477,6 @@ static void pci_msix_writel(void *opaque,
> target_phys_addr_t addr, uint32_t val)
> {
> if ( msix->enabled && !(val & 0x1) )
> pt_msix_update_one(dev, entry_nr);
> - mask_physical_msix_entry(dev, entry_nr, entry->io_mem[3] & 0x1);
> }
> }
>
> @@ -519,7 +509,11 @@ static uint32_t pci_msix_readl(void *opaque,
> target_phys_addr_t addr)
> entry_nr = (addr - msix->mmio_base_addr) / 16;
> offset = ((addr - msix->mmio_base_addr) % 16) / 4;
>
> - return msix->msix_entry[entry_nr].io_mem[offset];
> + if ( addr - msix->mmio_base_addr < msix->total_entries * 16 )
> + return msix->msix_entry[entry_nr].io_mem[offset];
> + else
> + return *(uint32_t *)(msix->phys_iomem_base +
> + (addr - msix->mmio_base_addr));
> }
>
> static CPUReadMemoryFunc *pci_msix_read[] = {
> @@ -528,39 +522,12 @@ static CPUReadMemoryFunc *pci_msix_read[] = {
> pci_msix_readl
> };
>
> -int add_msix_mapping(struct pt_dev *dev, int bar_index)
> -{
> - if ( !(dev->msix && dev->msix->bar_index == bar_index) )
> - return 0;
> -
> - return xc_domain_memory_mapping(xc_handle, domid,
> - dev->msix->mmio_base_addr >> XC_PAGE_SHIFT,
> - (dev->bases[bar_index].access.maddr
> - + dev->msix->table_off) >> XC_PAGE_SHIFT,
> - (dev->msix->total_entries * 16
> - + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
> - DPCI_ADD_MAPPING);
> -}
> -
> -int remove_msix_mapping(struct pt_dev *dev, int bar_index)
> +int has_msix_mapping(struct pt_dev *dev, int bar_index)
> {
> if ( !(dev->msix && dev->msix->bar_index == bar_index) )
> return 0;
>
> - dev->msix->mmio_base_addr = dev->bases[bar_index].e_physbase
> - + dev->msix->table_off;
> -
> - cpu_register_physical_memory(dev->msix->mmio_base_addr,
> - dev->msix->total_entries * 16,
> - dev->msix->mmio_index);
> -
> - return xc_domain_memory_mapping(xc_handle, domid,
> - dev->msix->mmio_base_addr >> XC_PAGE_SHIFT,
> - (dev->bases[bar_index].access.maddr
> - + dev->msix->table_off) >> XC_PAGE_SHIFT,
> - (dev->msix->total_entries * 16
> - + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
> - DPCI_REMOVE_MAPPING);
> + return 1;
> }
>
> int pt_msix_init(struct pt_dev *dev, int pos)
> @@ -616,7 +583,7 @@ int pt_msix_init(struct pt_dev *dev, int pos)
> PT_LOG("table_off = %x, total_entries = %d\n", table_off,
> total_entries);
> dev->msix->table_offset_adjust = table_off & 0x0fff;
> dev->msix->phys_iomem_base = mmap(0, total_entries * 16 +
> dev->msix->table_offset_adjust,
> - PROT_WRITE | PROT_READ, MAP_SHARED | MAP_LOCKED,
> + PROT_READ, MAP_SHARED | MAP_LOCKED,
> fd, dev->msix->table_base + table_off -
> dev->msix->table_offset_adjust);
> dev->msix->phys_iomem_base = (void *)((char *)dev->msix->phys_iomem_base
> +
> dev->msix->table_offset_adjust);
> diff --git a/hw/pt-msi.h b/hw/pt-msi.h
> index 9664f89..2dc1720 100644
> --- a/hw/pt-msi.h
> +++ b/hw/pt-msi.h
> @@ -107,10 +107,7 @@ void
> pt_msix_disable(struct pt_dev *dev);
>
> int
> -remove_msix_mapping(struct pt_dev *dev, int bar_index);
> -
> -int
> -add_msix_mapping(struct pt_dev *dev, int bar_index);
> +has_msix_mapping(struct pt_dev *dev, int bar_index);
>
> int
> pt_msix_init(struct pt_dev *dev, int pos);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |