| Message ID | 1488341440-4347-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 01, 2017 at 12:10:40PM +0800, Jason Wang wrote: > Commit c611c76417f5 ("virtio: add MemoryListener to cache ring > translations") registers a memory listener to dma_as. This may not > work when IOMMU is enabled: dma_as(bus_master_as) were initialized in > pcibus_machine_done() after virtio_realize(). This will cause a > segfault. Fixing this by using pci_device_iommu_address_space() > instead to make sure address space were initialized at this time. > > With this fix, IOMMU device were required to be initialized before any > virtio-pci devices. > > Fixes: c611c76417f5 ("virtio: add MemoryListener to cache ring translations") > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> This is very ugly. I guess it's better than broken IOMMU ... Paolo? > --- > hw/virtio/virtio-pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 5ce42af..b76f3f6 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1153,7 +1153,7 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d) > VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > PCIDevice *dev = &proxy->pci_dev; > > - return pci_get_address_space(dev); > + return pci_device_iommu_address_space(dev); > } > > static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, > -- > 2.7.4
On 2017年03月02日 13:10, Michael S. Tsirkin wrote: > On Wed, Mar 01, 2017 at 12:10:40PM +0800, Jason Wang wrote: >> Commit c611c76417f5 ("virtio: add MemoryListener to cache ring >> translations") registers a memory listener to dma_as. This may not >> work when IOMMU is enabled: dma_as(bus_master_as) were initialized in >> pcibus_machine_done() after virtio_realize(). This will cause a >> segfault. Fixing this by using pci_device_iommu_address_space() >> instead to make sure address space were initialized at this time. >> >> With this fix, IOMMU device were required to be initialized before any >> virtio-pci devices. >> >> Fixes: c611c76417f5 ("virtio: add MemoryListener to cache ring translations") >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> > This is very ugly. I guess it's better than broken IOMMU ... > Paolo? Maybe we can delay the registering of memory listener on bus master enabling or status setting. Thanks > >> --- >> hw/virtio/virtio-pci.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 5ce42af..b76f3f6 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -1153,7 +1153,7 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d) >> VirtIOPCIProxy *proxy = VIRTIO_PCI(d); >> PCIDevice *dev = &proxy->pci_dev; >> >> - return pci_get_address_space(dev); >> + return pci_device_iommu_address_space(dev); >> } >> >> static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, >> -- >> 2.7.4
On 02/03/2017 08:47, Jason Wang wrote: >>> >>> Fixes: c611c76417f5 ("virtio: add MemoryListener to cache ring >>> translations") >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Jason Wang <jasowang@redhat.com> >> This is very ugly. I guess it's better than broken IOMMU ... >> Paolo? > > Maybe we can delay the registering of memory listener on bus master > enabling or status setting. Can we add a callback to PCIDeviceClass, and invoke it from pci_init_bus_master? Paolo
On 2017年03月02日 18:30, Paolo Bonzini wrote: > > On 02/03/2017 08:47, Jason Wang wrote: >>>> Fixes: c611c76417f5 ("virtio: add MemoryListener to cache ring >>>> translations") >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> This is very ugly. I guess it's better than broken IOMMU ... >>> Paolo? >> Maybe we can delay the registering of memory listener on bus master >> enabling or status setting. > Can we add a callback to PCIDeviceClass, and invoke it from > pci_init_bus_master? > > Paolo > This looks pci specific, I post a fix that reset the dma_as during status set which can solve this issue too. Please have a look. Thanks
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 5ce42af..b76f3f6 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1153,7 +1153,7 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d) VirtIOPCIProxy *proxy = VIRTIO_PCI(d); PCIDevice *dev = &proxy->pci_dev; - return pci_get_address_space(dev); + return pci_device_iommu_address_space(dev); } static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
Commit c611c76417f5 ("virtio: add MemoryListener to cache ring translations") registers a memory listener to dma_as. This may not work when IOMMU is enabled: dma_as(bus_master_as) were initialized in pcibus_machine_done() after virtio_realize(). This will cause a segfault. Fixing this by using pci_device_iommu_address_space() instead to make sure address space were initialized at this time. With this fix, IOMMU device were required to be initialized before any virtio-pci devices. Fixes: c611c76417f5 ("virtio: add MemoryListener to cache ring translations") Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio/virtio-pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)