| Message ID | 1351013211-1907-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 9e11908f12f92e31ea94dc2a4c962c836cba9f2a |
| Headers | show |
On Oct 24, 2012 3:27 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote: > > Define a new global dma_context_memory which is a DMAContext corresponding > to the global address_space_memory AddressSpace. This can be used by > sysbus peripherals like sysbus-ohci which need to do DMA. > > In particular, use it in the sysbus-ohci device, which fixes a > segfault when attempting to use that device. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > As suggested by Avi. I could have split this patch into one defining > the new global and one actually using it, but since the hcd-ohci > change would be a one-liner it didn't seem worthwhile. > > dma.h | 5 +++++ > exec.c | 5 +++++ > hw/usb/hcd-ohci.c | 2 +- > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/dma.h b/dma.h > index 1bd6f4a..f7cf5e7 100644 > --- a/dma.h > +++ b/dma.h > @@ -68,6 +68,11 @@ struct DMAContext { > DMAUnmapFunc *unmap; > }; > > +/* A global DMA context corresponding to the address_space_memory > + * AddressSpace, for sysbus devices which do DMA. > + */ > +extern DMAContext dma_context_memory; > + > static inline void dma_barrier(DMAContext *dma, DMADirection dir) > { > /* > diff --git a/exec.c b/exec.c > index 750008c..a59ed31 100644 > --- a/exec.c > +++ b/exec.c > @@ -34,6 +34,7 @@ > #include "hw/xen.h" > #include "qemu-timer.h" > #include "memory.h" > +#include "dma.h" > #include "exec-memory.h" > #if defined(CONFIG_USER_ONLY) > #include <qemu.h> > @@ -103,6 +104,7 @@ static MemoryRegion *system_io; > > AddressSpace address_space_io; > AddressSpace address_space_memory; > +DMAContext dma_context_memory; > > MemoryRegion io_mem_ram, io_mem_rom, io_mem_unassigned, io_mem_notdirty; > static MemoryRegion io_mem_subpage_ram; > @@ -3276,6 +3278,9 @@ static void memory_map_init(void) > memory_listener_register(&core_memory_listener, &address_space_memory); > memory_listener_register(&io_memory_listener, &address_space_io); > memory_listener_register(&tcg_memory_listener, &address_space_memory); > + > + dma_context_init(&dma_context_memory, &address_space_memory, > + NULL, NULL, NULL); > } > > MemoryRegion *get_system_memory(void) > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index 59c7055..eb1cb70 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -1846,7 +1846,7 @@ static int ohci_init_pxa(SysBusDevice *dev) > > /* Cannot fail as we pass NULL for masterbus */ > usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0, > - NULL); > + &dma_context_memory); > sysbus_init_irq(dev, &s->ohci.irq); > sysbus_init_mmio(dev, &s->ohci.mem); > > -- > 1.7.9.5 > >
On Thu, Oct 25, 2012 at 08:33:13PM +1000, Peter Crosthwaite wrote: > On Oct 24, 2012 3:27 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote: > > > > Define a new global dma_context_memory which is a DMAContext corresponding > > to the global address_space_memory AddressSpace. This can be used by > > sysbus peripherals like sysbus-ohci which need to do DMA. > > > > In particular, use it in the sysbus-ohci device, which fixes a > > segfault when attempting to use that device. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Hrm. So, as I originally conceived DMAContext, a NULL context pointer means "no translation" which is to say that DMA addresses are the same as memory space addresses. Which would mean a context explicitly for this purpose should not be necessary. Has this assumption changed with the newer memory region integrated dma context stuff?
On Fri, Oct 26, 2012 at 10:48 AM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Oct 25, 2012 at 08:33:13PM +1000, Peter Crosthwaite wrote: >> On Oct 24, 2012 3:27 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote: >> > >> > Define a new global dma_context_memory which is a DMAContext corresponding >> > to the global address_space_memory AddressSpace. This can be used by >> > sysbus peripherals like sysbus-ohci which need to do DMA. >> > >> > In particular, use it in the sysbus-ohci device, which fixes a >> > segfault when attempting to use that device. >> > >> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > Hrm. So, as I originally conceived DMAContext, a NULL context pointer > means "no translation" which is to say that DMA addresses are the same > as memory space addresses. Which would mean a context explicitly for > this purpose should not be necessary. > > Has this assumption changed with the newer memory region integrated > dma context stuff? Yes, The Segfaulting line is in dma.h: static inline int dma_memory_rw_relaxed(DMAContext *dma, dma_addr_t addr, void *buf, dma_addr_t len, DMADirection dir) { if (!dma_has_iommu(dma)) { /* Fast-path for no IOMMU */ address_space_rw(dma->as, addr, buf, len, dir == DMA_DIRECTION_FROM_DEVICE); return 0; } else { return iommu_dma_memory_rw(dma, addr, buf, len, dir); } } Dereferencing of dma->as segfaults sd dma==NULL in the cas you described. > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson >
On Fri, Oct 26, 2012 at 12:53:27PM +1000, Peter Crosthwaite wrote: > On Fri, Oct 26, 2012 at 10:48 AM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > On Thu, Oct 25, 2012 at 08:33:13PM +1000, Peter Crosthwaite wrote: > >> On Oct 24, 2012 3:27 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote: > >> > > >> > Define a new global dma_context_memory which is a DMAContext corresponding > >> > to the global address_space_memory AddressSpace. This can be used by > >> > sysbus peripherals like sysbus-ohci which need to do DMA. > >> > > >> > In particular, use it in the sysbus-ohci device, which fixes a > >> > segfault when attempting to use that device. > >> > > >> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > >> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > > > Hrm. So, as I originally conceived DMAContext, a NULL context pointer > > means "no translation" which is to say that DMA addresses are the same > > as memory space addresses. Which would mean a context explicitly for > > this purpose should not be necessary. > > > > Has this assumption changed with the newer memory region integrated > > dma context stuff? > > > Yes, The Segfaulting line is in dma.h: > > static inline int dma_memory_rw_relaxed(DMAContext *dma, dma_addr_t addr, > void *buf, dma_addr_t len, > DMADirection dir) > { > if (!dma_has_iommu(dma)) { > /* Fast-path for no IOMMU */ > address_space_rw(dma->as, addr, buf, len, dir == > DMA_DIRECTION_FROM_DEVICE); > return 0; > } else { > return iommu_dma_memory_rw(dma, addr, buf, len, dir); > } > } > > Dereferencing of dma->as segfaults sd dma==NULL in the cas you described. Ok. My inclination would be to special case that in that function, setting as to the standard memory as if !dma, but others may have a different opinion.
Il 26/10/2012 05:58, David Gibson ha scritto: >> > static inline int dma_memory_rw_relaxed(DMAContext *dma, dma_addr_t addr, >> > void *buf, dma_addr_t len, >> > DMADirection dir) >> > { >> > if (!dma_has_iommu(dma)) { >> > /* Fast-path for no IOMMU */ >> > address_space_rw(dma->as, addr, buf, len, dir == >> > DMA_DIRECTION_FROM_DEVICE); >> > return 0; >> > } else { >> > return iommu_dma_memory_rw(dma, addr, buf, len, dir); >> > } >> > } >> > >> > Dereferencing of dma->as segfaults sd dma==NULL in the cas you described. > Ok. My inclination would be to special case that in that function, > setting as to the standard memory as if !dma, but others may have a > different opinion. Me too, because I'm seeing the exact same segfault with virtio-scsi. Reproducible with: x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci -drive if=none,id=cd -device scsi-cd,drive=cd (you don't even need a medium in the drive, it segfaults as soon as the BIOS probes the device). As soon as Avi's iommu patches go in, in fact, dma->as will just be as. Even if as == NULL were to be outlawed and you'd be forced to write get_address_space_memory(), taking the pain to create dummy DMAContexts now is just not worth it. Paolo
On 26 October 2012 14:09, Paolo Bonzini <pbonzini@redhat.com> wrote: > As soon as Avi's iommu patches go in, in fact, dma->as will just be as. > Even if as == NULL were to be outlawed and you'd be forced to write > get_address_space_memory(), taking the pain to create dummy DMAContexts > now is just not worth it. Personally I think it's better not to permit NULL DMAContexts or AddressSpaces here, because they're kind of a hack (in the same way that the "system address space" is kind of a hack). In real hardware you probably aren't really doing dma to that address space but to some more local bus. dma_context_memory/address_space_memory are nice and easy to search for, whereas NULL isn't. [And in general I think NULL is too easy to use; if you have to go looking for the system dma context you've been prompted to think about whether that's the right one...] -- PMM
On 26 October 2012 17:00, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 October 2012 14:09, Paolo Bonzini <pbonzini@redhat.com> wrote: >> As soon as Avi's iommu patches go in, in fact, dma->as will just be as. >> Even if as == NULL were to be outlawed and you'd be forced to write >> get_address_space_memory(), taking the pain to create dummy DMAContexts >> now is just not worth it. > > Personally I think it's better not to permit NULL DMAContexts or > AddressSpaces here, because they're kind of a hack (in the same way > that the "system address space" is kind of a hack). In real hardware > you probably aren't really doing dma to that address space but to > some more local bus. dma_context_memory/address_space_memory are > nice and easy to search for, whereas NULL isn't. [And in general > I think NULL is too easy to use; if you have to go looking for the > system dma context you've been prompted to think about whether > that's the right one...] Ping! Can we have a ruling on what the right fix for this is so we can fix these segfaults before 1.3 release, please? thanks -- PMM
Il 13/11/2012 12:44, Peter Maydell ha scritto: >>> >> As soon as Avi's iommu patches go in, in fact, dma->as will just be as. >>> >> Even if as == NULL were to be outlawed and you'd be forced to write >>> >> get_address_space_memory(), taking the pain to create dummy DMAContexts >>> >> now is just not worth it. >> > >> > Personally I think it's better not to permit NULL DMAContexts or >> > AddressSpaces here, because they're kind of a hack (in the same way >> > that the "system address space" is kind of a hack). In real hardware >> > you probably aren't really doing dma to that address space but to >> > some more local bus. dma_context_memory/address_space_memory are >> > nice and easy to search for, whereas NULL isn't. [And in general >> > I think NULL is too easy to use; if you have to go looking for the >> > system dma context you've been prompted to think about whether >> > that's the right one...] > Ping! Can we have a ruling on what the right fix for this is so > we can fix these segfaults before 1.3 release, please? I think this patch is already in Gerd's queue. Paolo
Hi, >> Ping! Can we have a ruling on what the right fix for this is so >> we can fix these segfaults before 1.3 release, please? > > I think this patch is already in Gerd's queue. It isn't. It used to be in the usb queue, but after the debate how to fix this property restarted I've dropped it again so the discussion doesn't block the usb queue. cheers, Gerd
Il 13/11/2012 16:21, Gerd Hoffmann ha scritto: >>> >> Ping! Can we have a ruling on what the right fix for this is so >>> >> we can fix these segfaults before 1.3 release, please? >> > >> > I think this patch is already in Gerd's queue. > It isn't. It used to be in the usb queue, but after the debate how to > fix this property restarted I've dropped it again so the discussion > doesn't block the usb queue. Ok, so since this breaks virtio-scsi too, I'm adding it to the scsi queue. Paolo
On 11/13/2012 01:44 PM, Peter Maydell wrote: > On 26 October 2012 17:00, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 26 October 2012 14:09, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> As soon as Avi's iommu patches go in, in fact, dma->as will just be as. >>> Even if as == NULL were to be outlawed and you'd be forced to write >>> get_address_space_memory(), taking the pain to create dummy DMAContexts >>> now is just not worth it. >> >> Personally I think it's better not to permit NULL DMAContexts or >> AddressSpaces here, because they're kind of a hack (in the same way >> that the "system address space" is kind of a hack). In real hardware >> you probably aren't really doing dma to that address space but to >> some more local bus. dma_context_memory/address_space_memory are >> nice and easy to search for, whereas NULL isn't. [And in general >> I think NULL is too easy to use; if you have to go looking for the >> system dma context you've been prompted to think about whether >> that's the right one...] > > Ping! Can we have a ruling on what the right fix for this is so > we can fix these segfaults before 1.3 release, please? I agree with you here. Callers should be fixed to supply the proper AddressSpace.
diff --git a/dma.h b/dma.h index 1bd6f4a..f7cf5e7 100644 --- a/dma.h +++ b/dma.h @@ -68,6 +68,11 @@ struct DMAContext { DMAUnmapFunc *unmap; }; +/* A global DMA context corresponding to the address_space_memory + * AddressSpace, for sysbus devices which do DMA. + */ +extern DMAContext dma_context_memory; + static inline void dma_barrier(DMAContext *dma, DMADirection dir) { /* diff --git a/exec.c b/exec.c index 750008c..a59ed31 100644 --- a/exec.c +++ b/exec.c @@ -34,6 +34,7 @@ #include "hw/xen.h" #include "qemu-timer.h" #include "memory.h" +#include "dma.h" #include "exec-memory.h" #if defined(CONFIG_USER_ONLY) #include <qemu.h> @@ -103,6 +104,7 @@ static MemoryRegion *system_io; AddressSpace address_space_io; AddressSpace address_space_memory; +DMAContext dma_context_memory; MemoryRegion io_mem_ram, io_mem_rom, io_mem_unassigned, io_mem_notdirty; static MemoryRegion io_mem_subpage_ram; @@ -3276,6 +3278,9 @@ static void memory_map_init(void) memory_listener_register(&core_memory_listener, &address_space_memory); memory_listener_register(&io_memory_listener, &address_space_io); memory_listener_register(&tcg_memory_listener, &address_space_memory); + + dma_context_init(&dma_context_memory, &address_space_memory, + NULL, NULL, NULL); } MemoryRegion *get_system_memory(void) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 59c7055..eb1cb70 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1846,7 +1846,7 @@ static int ohci_init_pxa(SysBusDevice *dev) /* Cannot fail as we pass NULL for masterbus */ usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0, - NULL); + &dma_context_memory); sysbus_init_irq(dev, &s->ohci.irq); sysbus_init_mmio(dev, &s->ohci.mem);
Define a new global dma_context_memory which is a DMAContext corresponding to the global address_space_memory AddressSpace. This can be used by sysbus peripherals like sysbus-ohci which need to do DMA. In particular, use it in the sysbus-ohci device, which fixes a segfault when attempting to use that device. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- As suggested by Avi. I could have split this patch into one defining the new global and one actually using it, but since the hcd-ohci change would be a one-liner it didn't seem worthwhile. dma.h | 5 +++++ exec.c | 5 +++++ hw/usb/hcd-ohci.c | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-)