| Message ID | 1459864004-2869-2-git-send-email-xyjxie@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Apr 05, 2016 at 09:46:44PM +0800, Yongji Xie wrote: >This patch enables mmapping MSI-X tables if >hardware supports interrupt remapping which >can ensure that a given pci device can only >shoot the MSIs assigned for it. > >Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >--- > drivers/vfio/pci/vfio_pci.c | 9 +++++++-- > drivers/vfio/pci/vfio_pci_private.h | 1 + > drivers/vfio/pci/vfio_pci_rdwr.c | 2 +- > 3 files changed, 9 insertions(+), 3 deletions(-) > >diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >index c60d790..ef02896 100644 >--- a/drivers/vfio/pci/vfio_pci.c >+++ b/drivers/vfio/pci/vfio_pci.c >@@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > } else > vdev->msix_bar = 0xFF; > >+ if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) || >+ pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP) >+ vdev->msi_remap = true; >+ I guess you probably need a "&" here. Otherwise, the condition is always true. > if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev)) > vdev->has_vga = true; > >@@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data, > VFIO_REGION_INFO_FLAG_WRITE; > if (vdev->bar_mmap_supported[info.index]) { > info.flags |= VFIO_REGION_INFO_FLAG_MMAP; >- if (info.index == vdev->msix_bar) { >+ if (info.index == vdev->msix_bar && >+ !vdev->msi_remap) { > ret = msix_sparse_mmap_cap(vdev, &caps); > if (ret) > return ret; >@@ -1067,7 +1072,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma) > if (req_start + req_len > phys_len) > return -EINVAL; > >- if (index == vdev->msix_bar) { >+ if (index == vdev->msix_bar && !vdev->msi_remap) { > /* > * Disallow mmaps overlapping the MSI-X table; users don't > * get to touch this directly. We could find somewhere >diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h >index 0ea4c62..4f20963 100644 >--- a/drivers/vfio/pci/vfio_pci_private.h >+++ b/drivers/vfio/pci/vfio_pci_private.h >@@ -78,6 +78,7 @@ struct vfio_pci_device { > int irq_type; > int num_regions; > struct vfio_pci_region *region; >+ bool msi_remap; > u8 msi_qmax; > u8 msix_bar; > u16 msix_size; >diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c >index 5ffd1d9..606ee3c 100644 >--- a/drivers/vfio/pci/vfio_pci_rdwr.c >+++ b/drivers/vfio/pci/vfio_pci_rdwr.c >@@ -164,7 +164,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_device *vdev, char __user *buf, > } else > io = vdev->barmap[bar]; > >- if (bar == vdev->msix_bar) { >+ if (bar == vdev->msix_bar && !vdev->msi_remap) { > x_start = vdev->msix_offset; > x_end = vdev->msix_offset + vdev->msix_size; > } When PCI_BUS_FLAGS_MSI_REMAP is set, the MSIx table can be accessed by read/write interface except mmap(). The commit log doesn't mention it. It would be better if you have some words about it. Thanks, Gavin >-- >1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/4/6 8:00, Gavin Shan wrote: > On Tue, Apr 05, 2016 at 09:46:44PM +0800, Yongji Xie wrote: >> This patch enables mmapping MSI-X tables if >> hardware supports interrupt remapping which >> can ensure that a given pci device can only >> shoot the MSIs assigned for it. >> >> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >> --- >> drivers/vfio/pci/vfio_pci.c | 9 +++++++-- >> drivers/vfio/pci/vfio_pci_private.h | 1 + >> drivers/vfio/pci/vfio_pci_rdwr.c | 2 +- >> 3 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index c60d790..ef02896 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) >> } else >> vdev->msix_bar = 0xFF; >> >> + if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) || >> + pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP) >> + vdev->msi_remap = true; >> + > I guess you probably need a "&" here. Otherwise, the condition > is always true. Yes, you are right. I'll fix it. >> if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev)) >> vdev->has_vga = true; >> >> @@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data, >> VFIO_REGION_INFO_FLAG_WRITE; >> if (vdev->bar_mmap_supported[info.index]) { >> info.flags |= VFIO_REGION_INFO_FLAG_MMAP; >> - if (info.index == vdev->msix_bar) { >> + if (info.index == vdev->msix_bar && >> + !vdev->msi_remap) { >> ret = msix_sparse_mmap_cap(vdev, &caps); >> if (ret) >> return ret; >> @@ -1067,7 +1072,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma) >> if (req_start + req_len > phys_len) >> return -EINVAL; >> >> - if (index == vdev->msix_bar) { >> + if (index == vdev->msix_bar && !vdev->msi_remap) { >> /* >> * Disallow mmaps overlapping the MSI-X table; users don't >> * get to touch this directly. We could find somewhere >> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h >> index 0ea4c62..4f20963 100644 >> --- a/drivers/vfio/pci/vfio_pci_private.h >> +++ b/drivers/vfio/pci/vfio_pci_private.h >> @@ -78,6 +78,7 @@ struct vfio_pci_device { >> int irq_type; >> int num_regions; >> struct vfio_pci_region *region; >> + bool msi_remap; >> u8 msi_qmax; >> u8 msix_bar; >> u16 msix_size; >> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c >> index 5ffd1d9..606ee3c 100644 >> --- a/drivers/vfio/pci/vfio_pci_rdwr.c >> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c >> @@ -164,7 +164,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_device *vdev, char __user *buf, >> } else >> io = vdev->barmap[bar]; >> >> - if (bar == vdev->msix_bar) { >> + if (bar == vdev->msix_bar && !vdev->msi_remap) { >> x_start = vdev->msix_offset; >> x_end = vdev->msix_offset + vdev->msix_size; >> } > When PCI_BUS_FLAGS_MSI_REMAP is set, the MSIx table can be accessed by > read/write interface except mmap(). The commit log doesn't mention it. > It would be better if you have some words about it. > > Thanks, > Gavin > OK. I will mention it in commit log. Thanks, Yongji >> -- >> 1.7.9.5 >> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 5 Apr 2016 21:46:44 +0800 Yongji Xie <xyjxie@linux.vnet.ibm.com> wrote: > This patch enables mmapping MSI-X tables if > hardware supports interrupt remapping which > can ensure that a given pci device can only > shoot the MSIs assigned for it. > > Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> > --- > drivers/vfio/pci/vfio_pci.c | 9 +++++++-- > drivers/vfio/pci/vfio_pci_private.h | 1 + > drivers/vfio/pci/vfio_pci_rdwr.c | 2 +- > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index c60d790..ef02896 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) > } else > vdev->msix_bar = 0xFF; > > + if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) || This doesn't address the issue I raised earlier where ARM SMMU sets this capability, but doesn't really provide per vector isolation. ARM either needs to be fixed or we need to consider the whole capability tainted for this application and standardize around the bus flags. It's not very desirable to have two different ways to test this anyway. > + pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP) Perhaps some sort of wrapper for testing these flags would help avoid this kind of coding error (| vs &) > + vdev->msi_remap = true; > + > if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev)) > vdev->has_vga = true; > > @@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data, > VFIO_REGION_INFO_FLAG_WRITE; > if (vdev->bar_mmap_supported[info.index]) { > info.flags |= VFIO_REGION_INFO_FLAG_MMAP; > - if (info.index == vdev->msix_bar) { > + if (info.index == vdev->msix_bar && > + !vdev->msi_remap) { > ret = msix_sparse_mmap_cap(vdev, &caps); > if (ret) > return ret; > @@ -1067,7 +1072,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma) > if (req_start + req_len > phys_len) > return -EINVAL; > > - if (index == vdev->msix_bar) { > + if (index == vdev->msix_bar && !vdev->msi_remap) { > /* > * Disallow mmaps overlapping the MSI-X table; users don't > * get to touch this directly. We could find somewhere > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h > index 0ea4c62..4f20963 100644 > --- a/drivers/vfio/pci/vfio_pci_private.h > +++ b/drivers/vfio/pci/vfio_pci_private.h > @@ -78,6 +78,7 @@ struct vfio_pci_device { > int irq_type; > int num_regions; > struct vfio_pci_region *region; > + bool msi_remap; > u8 msi_qmax; > u8 msix_bar; > u16 msix_size; > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c > index 5ffd1d9..606ee3c 100644 > --- a/drivers/vfio/pci/vfio_pci_rdwr.c > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c > @@ -164,7 +164,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_device *vdev, char __user *buf, > } else > io = vdev->barmap[bar]; > > - if (bar == vdev->msix_bar) { > + if (bar == vdev->msix_bar && !vdev->msi_remap) { > x_start = vdev->msix_offset; > x_end = vdev->msix_offset + vdev->msix_size; > } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index c60d790..ef02896 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -201,6 +201,10 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev) } else vdev->msix_bar = 0xFF; + if (iommu_capable(pdev->dev.bus, IOMMU_CAP_INTR_REMAP) || + pdev->bus->bus_flags | PCI_BUS_FLAGS_MSI_REMAP) + vdev->msi_remap = true; + if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev)) vdev->has_vga = true; @@ -635,7 +639,8 @@ static long vfio_pci_ioctl(void *device_data, VFIO_REGION_INFO_FLAG_WRITE; if (vdev->bar_mmap_supported[info.index]) { info.flags |= VFIO_REGION_INFO_FLAG_MMAP; - if (info.index == vdev->msix_bar) { + if (info.index == vdev->msix_bar && + !vdev->msi_remap) { ret = msix_sparse_mmap_cap(vdev, &caps); if (ret) return ret; @@ -1067,7 +1072,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma) if (req_start + req_len > phys_len) return -EINVAL; - if (index == vdev->msix_bar) { + if (index == vdev->msix_bar && !vdev->msi_remap) { /* * Disallow mmaps overlapping the MSI-X table; users don't * get to touch this directly. We could find somewhere diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h index 0ea4c62..4f20963 100644 --- a/drivers/vfio/pci/vfio_pci_private.h +++ b/drivers/vfio/pci/vfio_pci_private.h @@ -78,6 +78,7 @@ struct vfio_pci_device { int irq_type; int num_regions; struct vfio_pci_region *region; + bool msi_remap; u8 msi_qmax; u8 msix_bar; u16 msix_size; diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c index 5ffd1d9..606ee3c 100644 --- a/drivers/vfio/pci/vfio_pci_rdwr.c +++ b/drivers/vfio/pci/vfio_pci_rdwr.c @@ -164,7 +164,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_device *vdev, char __user *buf, } else io = vdev->barmap[bar]; - if (bar == vdev->msix_bar) { + if (bar == vdev->msix_bar && !vdev->msi_remap) { x_start = vdev->msix_offset; x_end = vdev->msix_offset + vdev->msix_size; }
This patch enables mmapping MSI-X tables if hardware supports interrupt remapping which can ensure that a given pci device can only shoot the MSIs assigned for it. Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> --- drivers/vfio/pci/vfio_pci.c | 9 +++++++-- drivers/vfio/pci/vfio_pci_private.h | 1 + drivers/vfio/pci/vfio_pci_rdwr.c | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-)