Re: [Qemu-devel] [PATCH arm-devs v1 05/13] net/cadence_gem: Prefetch rx descriptors ASAP

2013-12-03 Thread Peter Crosthwaite
On Mon, Dec 2, 2013 at 10:14 PM, Peter Maydell  wrote:
> On 2 December 2013 07:11, Peter Crosthwaite
>  wrote:
>> The real hardware prefetches rx buffer descriptors ASAP and
>> potentially throws relevant interrupts following the fetch
>> even in the absence of a recieved packet.
>>
>> Reported-by: Deepika Dhamija 
>> Signed-off-by: Peter Crosthwaite 
>> ---
>>
>>  hw/net/cadence_gem.c | 64 
>> +---
>>  1 file changed, 36 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 73ac0d8..e5a6d87 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -346,6 +346,8 @@ typedef struct GemState {
>>  uint32_t rx_desc_addr;
>>  uint32_t tx_desc_addr;
>>
>> +unsigned rx_desc[2];
>> +
>>  } GemState;
>>
>>  /* The broadcast MAC address: 0x */
>> @@ -579,13 +581,30 @@ static int gem_mac_address_filter(GemState *s, const 
>> uint8_t *packet)
>>  return GEM_RX_REJECT;
>>  }
>>
>> +static void gem_get_rx_desc(GemState *s)
>> +{
>> +DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
>> +/* read current descriptor */
>> +cpu_physical_memory_read(s->rx_desc_addr,
>> + (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
>> +
>> +/* Descriptor owned by software ? */
>> +if (rx_desc_get_ownership(s->rx_desc) == 1) {
>> +DB_PRINT("descriptor 0x%x owned by sw.\n",
>> + (unsigned)s->rx_desc_addr);
>> +s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
>> +s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
>> +/* Handle interrupt consequences */
>> +gem_update_int_status(s);
>> +}
>> +}
>> +
>
> Looks OK codewise but your indent here is wrong...
>

Fixed,

Regards,
Peter

> thanks
> -- PMM
>



Re: [Qemu-devel] [PATCH arm-devs v1 05/13] net/cadence_gem: Prefetch rx descriptors ASAP

2013-12-02 Thread Peter Maydell
On 2 December 2013 07:11, Peter Crosthwaite
 wrote:
> The real hardware prefetches rx buffer descriptors ASAP and
> potentially throws relevant interrupts following the fetch
> even in the absence of a recieved packet.
>
> Reported-by: Deepika Dhamija 
> Signed-off-by: Peter Crosthwaite 
> ---
>
>  hw/net/cadence_gem.c | 64 
> +---
>  1 file changed, 36 insertions(+), 28 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 73ac0d8..e5a6d87 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -346,6 +346,8 @@ typedef struct GemState {
>  uint32_t rx_desc_addr;
>  uint32_t tx_desc_addr;
>
> +unsigned rx_desc[2];
> +
>  } GemState;
>
>  /* The broadcast MAC address: 0x */
> @@ -579,13 +581,30 @@ static int gem_mac_address_filter(GemState *s, const 
> uint8_t *packet)
>  return GEM_RX_REJECT;
>  }
>
> +static void gem_get_rx_desc(GemState *s)
> +{
> +DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
> +/* read current descriptor */
> +cpu_physical_memory_read(s->rx_desc_addr,
> + (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
> +
> +/* Descriptor owned by software ? */
> +if (rx_desc_get_ownership(s->rx_desc) == 1) {
> +DB_PRINT("descriptor 0x%x owned by sw.\n",
> + (unsigned)s->rx_desc_addr);
> +s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
> +s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> +/* Handle interrupt consequences */
> +gem_update_int_status(s);
> +}
> +}
> +

Looks OK codewise but your indent here is wrong...

thanks
-- PMM



[Qemu-devel] [PATCH arm-devs v1 05/13] net/cadence_gem: Prefetch rx descriptors ASAP

2013-12-01 Thread Peter Crosthwaite
The real hardware prefetches rx buffer descriptors ASAP and
potentially throws relevant interrupts following the fetch
even in the absence of a recieved packet.

Reported-by: Deepika Dhamija 
Signed-off-by: Peter Crosthwaite 
---

 hw/net/cadence_gem.c | 64 +---
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 73ac0d8..e5a6d87 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -346,6 +346,8 @@ typedef struct GemState {
 uint32_t rx_desc_addr;
 uint32_t tx_desc_addr;
 
+unsigned rx_desc[2];
+
 } GemState;
 
 /* The broadcast MAC address: 0x */
@@ -579,13 +581,30 @@ static int gem_mac_address_filter(GemState *s, const 
uint8_t *packet)
 return GEM_RX_REJECT;
 }
 
+static void gem_get_rx_desc(GemState *s)
+{
+DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
+/* read current descriptor */
+cpu_physical_memory_read(s->rx_desc_addr,
+ (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
+
+/* Descriptor owned by software ? */
+if (rx_desc_get_ownership(s->rx_desc) == 1) {
+DB_PRINT("descriptor 0x%x owned by sw.\n",
+ (unsigned)s->rx_desc_addr);
+s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
+s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
+/* Handle interrupt consequences */
+gem_update_int_status(s);
+}
+}
+
 /*
  * gem_receive:
  * Fit a packet handed to us by QEMU into the receive descriptor ring.
  */
 static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
-unsigneddesc[2];
 GemState *s;
 unsigned   rxbufsize, bytes_to_copy;
 unsigned   rxbuf_offset;
@@ -595,11 +614,6 @@ static ssize_t gem_receive(NetClientState *nc, const 
uint8_t *buf, size_t size)
 
 s = qemu_get_nic_opaque(nc);
 
-/* Do nothing if receive is not enabled. */
-if (!gem_can_receive(nc)) {
-return -1;
-}
-
 /* Is this destination MAC address "for us" ? */
 if (gem_mac_address_filter(s, buf) == GEM_RX_REJECT) {
 return -1;
@@ -667,61 +681,52 @@ static ssize_t gem_receive(NetClientState *nc, const 
uint8_t *buf, size_t size)
 DB_PRINT("config bufsize: %d packet size: %ld\n", rxbufsize, size);
 
 while (bytes_to_copy) {
-DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr);
-/* read current descriptor */
-cpu_physical_memory_read(s->rx_desc_addr,
- (uint8_t *)&desc[0], sizeof(desc));
-
-/* Descriptor owned by software ? */
-if (rx_desc_get_ownership(desc) == 1) {
-DB_PRINT("descriptor 0x%x owned by sw.\n",
- (unsigned)s->rx_desc_addr);
-s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
-s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
-/* Handle interrupt consequences */
-gem_update_int_status(s);
+/* Do nothing if receive is not enabled. */
+if (!gem_can_receive(nc)) {
+assert(!first_desc);
 return -1;
 }
 
 DB_PRINT("copy %d bytes to 0x%x\n", MIN(bytes_to_copy, rxbufsize),
-rx_desc_get_buffer(desc));
+rx_desc_get_buffer(s->rx_desc));
 
 /*
  * Let's have QEMU lend a helping hand.
  */
-if (rx_desc_get_buffer(desc) == 0) {
+if (rx_desc_get_buffer(s->rx_desc) == 0) {
 DB_PRINT("Invalid RX buffer (NULL) for descriptor 0x%x\n",
  (unsigned)s->rx_desc_addr);
 }
 
 /* Copy packet data to emulated DMA buffer */
-cpu_physical_memory_write(rx_desc_get_buffer(desc) + rxbuf_offset,
+cpu_physical_memory_write(rx_desc_get_buffer(s->rx_desc) + 
rxbuf_offset,
   rxbuf_ptr, MIN(bytes_to_copy, rxbufsize));
 bytes_to_copy -= MIN(bytes_to_copy, rxbufsize);
 rxbuf_ptr += MIN(bytes_to_copy, rxbufsize);
 
 /* Update the descriptor.  */
 if (first_desc) {
-rx_desc_set_sof(desc);
+rx_desc_set_sof(s->rx_desc);
 first_desc = false;
 }
 if (bytes_to_copy == 0) {
-rx_desc_set_eof(desc);
-rx_desc_set_length(desc, size);
+rx_desc_set_eof(s->rx_desc);
+rx_desc_set_length(s->rx_desc, size);
 }
-rx_desc_set_ownership(desc);
+rx_desc_set_ownership(s->rx_desc);
 /* Descriptor write-back.  */
 cpu_physical_memory_write(s->rx_desc_addr,
-  (uint8_t *)&desc[0], sizeof(desc));
+  (uint8_t *)s->rx_desc, sizeof(s->rx_desc));
 
 /* Next descriptor */
-if (rx_desc_get_wrap(desc)) {
+if (rx_desc_get_wrap(s->rx_desc)) {