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@intel.com>
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;
+ }
+ }
+ 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@lists.xensource.com
http://lists.xensource.com/xen-devel
>>> On 22.09.11 at 02:35, Haitao Shan <maillists.shan@gmail.com> 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@intel.com> > > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
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@intel.com> > > > 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.
>>> On 02.12.11 at 13:40, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > 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);Please see below/attached for the outcome. Without MSI-X capable devices to pass through, I have to rely on others to do some testing on this. Jan qemu: clean up MSI-X table handling 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.shan@gmail.com> Consolidated duplicate code into _pt_iomem_helper(). Fixed formatting. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- 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; @@ -1097,6 +1098,44 @@ uint8_t pci_intx(struct pt_dev *ptdev) return r_val; } +static int _pt_iomem_helper(struct pt_dev *assigned_device, int i, + uint32_t e_base, uint32_t e_size, int op) +{ + if ( has_msix_mapping(assigned_device, i) ) + { + uint32_t msix_last_pfn = (assigned_device->msix->mmio_base_addr - 1 + + assigned_device->msix->total_entries * 16) >> XC_PAGE_SHIFT; + uint32_t bar_last_pfn = (e_base + e_size - 1) >> XC_PAGE_SHIFT; + int ret = 0; + + if ( assigned_device->msix->table_off ) + ret = xc_domain_memory_mapping(xc_handle, domid, + e_base >> XC_PAGE_SHIFT, + assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT, + (assigned_device->msix->mmio_base_addr >> XC_PAGE_SHIFT) + - (e_base >> XC_PAGE_SHIFT), op); + + if ( ret == 0 && 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, op); + } + + return ret; + } + + return xc_domain_memory_mapping(xc_handle, domid, + e_base >> XC_PAGE_SHIFT, + assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT, + (e_size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT, op); +} + /* Being called each time a mmio region has been updated */ static void pt_iomem_map(PCIDevice *d, int i, uint32_t e_phys, uint32_t e_size, int type) @@ -1118,13 +1157,11 @@ static void pt_iomem_map(PCIDevice *d, i if ( !first_map && old_ebase != -1 ) { - add_msix_mapping(assigned_device, i); - /* 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 ( has_msix_mapping(assigned_device, i) ) + unregister_iomem(assigned_device->msix->mmio_base_addr); + + ret = _pt_iomem_helper(assigned_device, i, old_ebase, e_size, + DPCI_REMOVE_MAPPING); if ( ret != 0 ) { PT_LOG("Error: remove old mapping failed!\n"); @@ -1135,22 +1172,26 @@ static void pt_iomem_map(PCIDevice *d, i /* map only valid guest address */ if (e_phys != -1) { - /* 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 ( has_msix_mapping(assigned_device, i) ) + { + assigned_device->msix->mmio_base_addr + assigned_device->bases[i].e_physbase + + assigned_device->msix->table_off; + + 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); + } + ret = _pt_iomem_helper(assigned_device, i, e_phys, e_size, + DPCI_ADD_MAPPING); if ( ret != 0 ) { PT_LOG("Error: create new mapping failed!\n"); + return; } - 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); } --- a/hw/pt-msi.c +++ b/hw/pt-msi.c @@ -284,15 +284,6 @@ void pt_disable_msi_translate(struct pt_ 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 { 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 *opa 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) +int has_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) -{ - 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 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); --- 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@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, 6 Dec 2011, Jan Beulich wrote:> >>> On 02.12.11 at 13:40, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > 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); > > Please see below/attached for the outcome. Without MSI-X capable > devices to pass through, I have to rely on others to do some testing > on this.It looks fine. Haitao, are you happy with the new patch?
Fine to me! Thanks, Jan! Shan Haitao> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: Wednesday, December 07, 2011 3:08 AM > To: Jan Beulich > Cc: Stefano Stabellini; Haitao Shan; Ian Jackson; Shan, Haitao; xen- > devel@lists.xensource.com; Keir (Xen.org) > Subject: Re: [Xen-devel] [PATCH] Qemu MSI Cleaning Up > > On Tue, 6 Dec 2011, Jan Beulich wrote: > > >>> On 02.12.11 at 13:40, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > > 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); > > > > Please see below/attached for the outcome. Without MSI-X capable > > devices to pass through, I have to rely on others to do some testing > > on this. > > It looks fine. > Haitao, are you happy with the new patch?_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 09.12.11 at 07:01, "Shan, Haitao" <haitao.shan@intel.com> wrote: > Fine to me! Thanks, Jan! > > Shan HaitaoIan, any chance to get this committed then? Also, how are we going to deal with the "modern" qemu tree (is passthrough for Xen implemented at all in that tree)? Thanks, Jan>> -----Original Message----- >> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] >> Sent: Wednesday, December 07, 2011 3:08 AM >> To: Jan Beulich >> Cc: Stefano Stabellini; Haitao Shan; Ian Jackson; Shan, Haitao; xen- >> devel@lists.xensource.com; Keir (Xen.org) >> Subject: Re: [Xen-devel] [PATCH] Qemu MSI Cleaning Up >> >> On Tue, 6 Dec 2011, Jan Beulich wrote: >> > >>> On 02.12.11 at 13:40, Stefano Stabellini >> <stefano.stabellini@eu.citrix.com> wrote: >> > > 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); >> > >> > Please see below/attached for the outcome. Without MSI-X capable >> > devices to pass through, I have to rely on others to do some testing >> > on this. >> >> It looks fine. >> Haitao, are you happy with the new patch?
On Mon, 12 Dec 2011, Jan Beulich wrote:> >>> On 09.12.11 at 07:01, "Shan, Haitao" <haitao.shan@intel.com> wrote: > > Fine to me! Thanks, Jan! > > > > Shan Haitao > > Ian, > > any chance to get this committed then? > > Also, how are we going to deal with the "modern" qemu tree (is > passthrough for Xen implemented at all in that tree)?It is only a patch series at the moment, even though it is close to be merged upstream I believe. This is the link to the last version: http://marc.info/?l=qemu-devel&m=132215676720458&w=2