[Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Peter Maydell]

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(-)

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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 3829 bytes --]

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[David Gibson]

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?

-- 
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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Peter Crosthwaite]

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
>

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[David Gibson]

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.

-- 
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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Gerd Hoffmann]

  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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] dma: Define dma_context_memory and use in sysbus-ohci

[Avi Kivity]

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.


-- 
error compiling committee.c: too many arguments to function