Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On Mon, Oct 08, 2018 at 01:30:20PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > Add support for selecting the Memory Region that the GEM
> > will do DMA to.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
> > hw/net/cadence_gem.c | 63
> >
> > include/hw/net/cadence_gem.h | 2 ++
> > 2 files changed, 43 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index 759c1d7..ab02515 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -28,6 +28,7 @@
> > #include "hw/net/cadence_gem.h"
> > #include "qapi/error.h"
> > #include "qemu/log.h"
> > +#include "sysemu/dma.h"
> > #include "net/checksum.h"
> >
> > #ifdef CADENCE_GEM_ERR_DEBUG
> > @@ -835,9 +836,9 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
> > {
> > DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
> > /* read current descriptor */
> > -cpu_physical_memory_read(s->rx_desc_addr[q],
> > - (uint8_t *)s->rx_desc[q],
> > - sizeof(uint32_t) * gem_get_desc_len(s, true));
> > +address_space_read(s->dma_as, s->rx_desc_addr[q],
> > MEMTXATTRS_UNSPECIFIED,
> > + (uint8_t *)s->rx_desc[q],
> > + sizeof(uint32_t) * gem_get_desc_len(s, true));
>
> At some point you might want to add support for handling "descriptor
> read/write failed", incidentally: address_space_read/write return a
> MemTxResult which you can check for != MEMTX_OK.
Yes, the GEM can report those errors to SW so that's a nice feature to follow
up with.
Thanks,
Edgar
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On Mon, Oct 08, 2018 at 01:24:51PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias
> wrote:
> > From: "Edgar E. Iglesias"
> >
> > Add support for selecting the Memory Region that the GEM
> > will do DMA to.
> >
> > Signed-off-by: Edgar E. Iglesias
> > ---
>
>
> > @@ -1500,6 +1506,13 @@ static void gem_realize(DeviceState *dev, Error
> > **errp)
> > CadenceGEMState *s = CADENCE_GEM(dev);
> > int i;
> >
> > +if (s->dma_mr) {
> > +s->dma_as = g_malloc0(sizeof(AddressSpace));
> > +address_space_init(s->dma_as, s->dma_mr, NULL);
>
> Why not just have the CadenceGEMState embed the AddressSpace
>
> AddressSpace dma_as;
>
> rather than doing a separate memory allocation here?
No reason not to, I copied this from a pattern in our code and didn't reflect
too much about the allocation.
I'll change it for next version.
Cheers,
Edgar
>
> > +} else {
> > +s->dma_as = &address_space_memory;
> > +}
>
> thanks
> -- PMM
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On 3 October 2018 at 16:07, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add support for selecting the Memory Region that the GEM
> will do DMA to.
>
> Signed-off-by: Edgar E. Iglesias
> ---
> hw/net/cadence_gem.c | 63
>
> include/hw/net/cadence_gem.h | 2 ++
> 2 files changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 759c1d7..ab02515 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -28,6 +28,7 @@
> #include "hw/net/cadence_gem.h"
> #include "qapi/error.h"
> #include "qemu/log.h"
> +#include "sysemu/dma.h"
> #include "net/checksum.h"
>
> #ifdef CADENCE_GEM_ERR_DEBUG
> @@ -835,9 +836,9 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
> {
> DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
> /* read current descriptor */
> -cpu_physical_memory_read(s->rx_desc_addr[q],
> - (uint8_t *)s->rx_desc[q],
> - sizeof(uint32_t) * gem_get_desc_len(s, true));
> +address_space_read(s->dma_as, s->rx_desc_addr[q], MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->rx_desc[q],
> + sizeof(uint32_t) * gem_get_desc_len(s, true));
At some point you might want to add support for handling "descriptor
read/write failed", incidentally: address_space_read/write return a
MemTxResult which you can check for != MEMTX_OK.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On 6 October 2018 at 00:14, Philippe Mathieu-Daudé wrote:
> Hi Edgar,
>
> On 03/10/2018 17:07, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias"
>>
>> Add support for selecting the Memory Region that the GEM
>> will do DMA to.
>> @@ -1500,6 +1506,13 @@ static void gem_realize(DeviceState *dev, Error
>> **errp)
>> CadenceGEMState *s = CADENCE_GEM(dev);
>> int i;
>>
>> +if (s->dma_mr) {
>> +s->dma_as = g_malloc0(sizeof(AddressSpace));
>> +address_space_init(s->dma_as, s->dma_mr, NULL);
>> +} else {
>> +s->dma_as = &address_space_memory;
>> +}
>
> This is not the first time I see this if() block.
>
> Should we consider refactor it?
Given that there are only three users of the device in the tree,
I think the nicest cleanup would be to make those callers pass
in a suitable MemoryRegion (which could just be the system memory
MR), and make the "dma" property of the device mandatory to set.
We can do that as followup, though.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On 3 October 2018 at 16:07, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add support for selecting the Memory Region that the GEM
> will do DMA to.
>
> Signed-off-by: Edgar E. Iglesias
> ---
> @@ -1500,6 +1506,13 @@ static void gem_realize(DeviceState *dev, Error **errp)
> CadenceGEMState *s = CADENCE_GEM(dev);
> int i;
>
> +if (s->dma_mr) {
> +s->dma_as = g_malloc0(sizeof(AddressSpace));
> +address_space_init(s->dma_as, s->dma_mr, NULL);
Why not just have the CadenceGEMState embed the AddressSpace
AddressSpace dma_as;
rather than doing a separate memory allocation here?
> +} else {
> +s->dma_as = &address_space_memory;
> +}
thanks
-- PMM
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
Hi Edgar,
On 03/10/2018 17:07, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias"
>
> Add support for selecting the Memory Region that the GEM
> will do DMA to.
>
> Signed-off-by: Edgar E. Iglesias
> ---
> hw/net/cadence_gem.c | 63
>
> include/hw/net/cadence_gem.h | 2 ++
> 2 files changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 759c1d7..ab02515 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -28,6 +28,7 @@
> #include "hw/net/cadence_gem.h"
> #include "qapi/error.h"
> #include "qemu/log.h"
> +#include "sysemu/dma.h"
> #include "net/checksum.h"
>
> #ifdef CADENCE_GEM_ERR_DEBUG
> @@ -835,9 +836,9 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
> {
> DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
> /* read current descriptor */
> -cpu_physical_memory_read(s->rx_desc_addr[q],
> - (uint8_t *)s->rx_desc[q],
> - sizeof(uint32_t) * gem_get_desc_len(s, true));
> +address_space_read(s->dma_as, s->rx_desc_addr[q], MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)s->rx_desc[q],
> + sizeof(uint32_t) * gem_get_desc_len(s, true));
>
> /* Descriptor owned by software ? */
> if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
> @@ -956,10 +957,10 @@ static ssize_t gem_receive(NetClientState *nc, const
> uint8_t *buf, size_t size)
> rx_desc_get_buffer(s->rx_desc[q]));
>
> /* Copy packet data to emulated DMA buffer */
> -cpu_physical_memory_write(rx_desc_get_buffer(s, s->rx_desc[q]) +
> +address_space_write(s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
>
> rxbuf_offset,
> - rxbuf_ptr,
> - MIN(bytes_to_copy, rxbufsize));
> +MEMTXATTRS_UNSPECIFIED, rxbuf_ptr,
> +MIN(bytes_to_copy, rxbufsize));
> rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
> bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
>
> @@ -993,9 +994,10 @@ static ssize_t gem_receive(NetClientState *nc, const
> uint8_t *buf, size_t size)
> }
>
> /* Descriptor write-back. */
> -cpu_physical_memory_write(s->rx_desc_addr[q],
> - (uint8_t *)s->rx_desc[q],
> - sizeof(uint32_t) * gem_get_desc_len(s,
> true));
> +address_space_write(s->dma_as, s->rx_desc_addr[q],
> +MEMTXATTRS_UNSPECIFIED,
> +(uint8_t *)s->rx_desc[q],
> +sizeof(uint32_t) * gem_get_desc_len(s, true));
>
> /* Next descriptor */
> if (rx_desc_get_wrap(s->rx_desc[q])) {
> @@ -1099,9 +1101,9 @@ static void gem_transmit(CadenceGEMState *s)
> packet_desc_addr = s->tx_desc_addr[q];
>
> DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
> -cpu_physical_memory_read(packet_desc_addr,
> - (uint8_t *)desc,
> - sizeof(uint32_t) * gem_get_desc_len(s,
> false));
> +address_space_read(s->dma_as, packet_desc_addr,
> + MEMTXATTRS_UNSPECIFIED, (uint8_t *)desc,
> + sizeof(uint32_t) * gem_get_desc_len(s, false));
> /* Handle all descriptors owned by hardware */
> while (tx_desc_get_used(desc) == 0) {
>
> @@ -1133,8 +1135,9 @@ static void gem_transmit(CadenceGEMState *s)
> /* Gather this fragment of the packet from "dma memory" to our
> * contig buffer.
> */
> -cpu_physical_memory_read(tx_desc_get_buffer(s, desc), p,
> - tx_desc_get_length(desc));
> +address_space_read(s->dma_as, tx_desc_get_buffer(s, desc),
> + MEMTXATTRS_UNSPECIFIED,
> + p, tx_desc_get_length(desc));
> p += tx_desc_get_length(desc);
> total_bytes += tx_desc_get_length(desc);
>
> @@ -1145,13 +1148,15 @@ static void gem_transmit(CadenceGEMState *s)
> /* Modify the 1st descriptor of this packet to be owned by
> * the processor.
> */
> -cpu_physical_memory_read(s->tx_desc_addr[q],
> - (uint8_t *)desc_first,
> - sizeof(desc_first));
> +address_space_read(s->dma_as, s->tx_desc_addr[q],
> + MEMTXATTRS_UNSPECIFIED,
> + (uint8_t *)desc_first,
> + sizeof(desc_first));
>
Re: [Qemu-devel] [PATCH v1 06/12] net: cadence_gem: Add support for selecting the DMA MemoryRegion
On 10/03/2018 08:07 AM, Edgar E. Iglesias wrote:
From: "Edgar E. Iglesias"
Add support for selecting the Memory Region that the GEM
will do DMA to.
Signed-off-by: Edgar E. Iglesias
Reviewed-by: Alistair Francis
Alistair
---
hw/net/cadence_gem.c | 63
include/hw/net/cadence_gem.h | 2 ++
2 files changed, 43 insertions(+), 22 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 759c1d7..ab02515 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -28,6 +28,7 @@
#include "hw/net/cadence_gem.h"
#include "qapi/error.h"
#include "qemu/log.h"
+#include "sysemu/dma.h"
#include "net/checksum.h"
#ifdef CADENCE_GEM_ERR_DEBUG
@@ -835,9 +836,9 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
{
DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
/* read current descriptor */
-cpu_physical_memory_read(s->rx_desc_addr[q],
- (uint8_t *)s->rx_desc[q],
- sizeof(uint32_t) * gem_get_desc_len(s, true));
+address_space_read(s->dma_as, s->rx_desc_addr[q], MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->rx_desc[q],
+ sizeof(uint32_t) * gem_get_desc_len(s, true));
/* Descriptor owned by software ? */
if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
@@ -956,10 +957,10 @@ static ssize_t gem_receive(NetClientState *nc, const
uint8_t *buf, size_t size)
rx_desc_get_buffer(s->rx_desc[q]));
/* Copy packet data to emulated DMA buffer */
-cpu_physical_memory_write(rx_desc_get_buffer(s, s->rx_desc[q]) +
+address_space_write(s->dma_as, rx_desc_get_buffer(s, s->rx_desc[q]) +
rxbuf_offset,
- rxbuf_ptr,
- MIN(bytes_to_copy, rxbufsize));
+MEMTXATTRS_UNSPECIFIED, rxbuf_ptr,
+MIN(bytes_to_copy, rxbufsize));
rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
@@ -993,9 +994,10 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
}
/* Descriptor write-back. */
-cpu_physical_memory_write(s->rx_desc_addr[q],
- (uint8_t *)s->rx_desc[q],
- sizeof(uint32_t) * gem_get_desc_len(s,
true));
+address_space_write(s->dma_as, s->rx_desc_addr[q],
+MEMTXATTRS_UNSPECIFIED,
+(uint8_t *)s->rx_desc[q],
+sizeof(uint32_t) * gem_get_desc_len(s, true));
/* Next descriptor */
if (rx_desc_get_wrap(s->rx_desc[q])) {
@@ -1099,9 +1101,9 @@ static void gem_transmit(CadenceGEMState *s)
packet_desc_addr = s->tx_desc_addr[q];
DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
-cpu_physical_memory_read(packet_desc_addr,
- (uint8_t *)desc,
- sizeof(uint32_t) * gem_get_desc_len(s,
false));
+address_space_read(s->dma_as, packet_desc_addr,
+ MEMTXATTRS_UNSPECIFIED, (uint8_t *)desc,
+ sizeof(uint32_t) * gem_get_desc_len(s, false));
/* Handle all descriptors owned by hardware */
while (tx_desc_get_used(desc) == 0) {
@@ -1133,8 +1135,9 @@ static void gem_transmit(CadenceGEMState *s)
/* Gather this fragment of the packet from "dma memory" to our
* contig buffer.
*/
-cpu_physical_memory_read(tx_desc_get_buffer(s, desc), p,
- tx_desc_get_length(desc));
+address_space_read(s->dma_as, tx_desc_get_buffer(s, desc),
+ MEMTXATTRS_UNSPECIFIED,
+ p, tx_desc_get_length(desc));
p += tx_desc_get_length(desc);
total_bytes += tx_desc_get_length(desc);
@@ -1145,13 +1148,15 @@ static void gem_transmit(CadenceGEMState *s)
/* Modify the 1st descriptor of this packet to be owned by
* the processor.
*/
-cpu_physical_memory_read(s->tx_desc_addr[q],
- (uint8_t *)desc_first,
- sizeof(desc_first));
+address_space_read(s->dma_as, s->tx_desc_addr[q],
+ MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)desc_first,
+ sizeof(desc_first));
tx_desc_set_used(desc_first);
-cpu_physical_memory_write(s->tx_desc_addr[q],
-
