Re: [Qemu-devel] patch for ne2000.c

[Fabrice Bellard]

OK for (2).

For (1) It would be good to find the exact behaviour of the NE2000 card.
Maybe ENISR_RX remain set as long are there are packets in the buffer ?
Otherwise your fix is a workaround to correct a bug in the OS driver...

Fabrice.

Han, Zhu wrote:
> Any comments for this patch?
> 
> Best Regards, 
> hanzhu
> -----Original Message-----
> From: address@hidden [mailto:address@hidden On Behalf Of Han, Zhu
> Sent: 2006年5月9日 12:27
> To: address@hidden
> Subject: [Qemu-devel] patch for ne2000.c
> 
> Hi, All!
> 
> I'm a developer working on xen project! It's well known that xen has
> adopted a lot of codes and features from QEMU, especially the Device
> Mode Part!
> 
> I fix a bug for ne2000 device emulation code in XEN and I expect it to
> be a potential bug for QEMU, either! Because you are all device mode
> experts, I submit this patch to you at first in order to ask you to
> review my patch. 
> 
> Several notes:
> 1) Because XEN use event driven mechanism in the main_loop(), irq may be
> missed due to the rather high speed and large file! For example, the
> ne2000_receive will filled up with the buffer and set up the ENISR_RX
> signal, however, the driver could ack and clear the ENISR_RX signal due
> to it could only handle a certain amount of packets once in it's
> interrupt handling routine!  The consequence for this specific steps is
> the netcard buffer is full but it never resend the ENISR_RX signal, at
> the last, the netcard will be halted! This problem could be rather rare
> for QEMU. Anyway, it's a potential bug.
> 2) Many of the ne2000 spec said we should set boundary register should
> be set to indicate the last receive buffer page the host has read, and
> the driver in linux follows this guideline. So, we boundary == index,
> the buffer for the netcard is full and we can't write any packets into
> this buffer. This minor fix could prevent the ne2000 emulated card from
> overflow and destroying the previous received packet page! This problem
> could also be rare for QEMU since it could happen only under extreme
> circumstance! 
> 
> Any feedbacks and comments will be appreciated! 
> 
> --- qemu-snapshot-2006-05-07_23\hw\ne2000.c   Mon May 08 16:13:49 2006
> +++ ./ne2000.c        Mon May 08 16:57:33 2006
> @@ -159,9 +159,19 @@
>      }
>  }
>  
> +static int ne2000_buffer_full(NE2000State *s);
>  static void ne2000_update_irq(NE2000State *s)
>  {
>      int isr;
> +
> +    if(ne2000_buffer_full(s)
> +            && !(s->isr & ENISR_RX)){
> +     /* The freeing space is not enough, tell the ne2k driver
> +      * to fetch these packets!
> +      */
> +        s->isr |= ENISR_RX;
> +    }
> +    
>      isr = (s->isr & s->imr) & 0x7f;
>  #if defined(DEBUG_NE2000)
>      printf("NE2000: Set IRQ line %d to %d (%02x %02x)\n",
> @@ -206,7 +216,10 @@
>  
>      index = s->curpag << 8;
>      boundary = s->boundary << 8;
> -    if (index < boundary)
> +    if (index <= boundary)
> +     /* when index == boundary, we should assume 
> +      * the buffer is full instead of empty!
> +      */
>          avail = boundary - index;
>      else
>          avail = (s->stop - s->start) - (index - boundary);
> 
> Best Regards, 
> hanzhu
> 
> 
> _______________________________________________
> Qemu-devel mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
> 
>