|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Qemu MSI Cleaning Up
On Thu, 22 Sep 2011, Haitao Shan 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.
>
> 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);
> + 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;
the log message is wrong here: we are trying to create new mappings
> + }
> + }
> + 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;
same here
> + }
> + }
> + }
> + 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);
> }
The two mapping and unmapping big chuncks of code looks very similar
apart from the DPCI_ADD_MAPPING/DPCI_REMOVE_MAPPING parameter.
Could they be refactored into a single function called "change_msix_mappings"?
This is more or less what I have in mind:
change_msix_mappings(assigned_device, DPCI_REMOVE_MAPPING);
update mmio_base_addr
change_msix_mappings(assigned_device, DPCI_ADD_MAPPING);
The rest looks OK to me.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |