[Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Bill Paul]

Hi, I hope this is the right forum for this. Apologies if it's not.

I downloaded QEMU 0.10.5 and tested it against VxWorks 6.7 using the e1000 
emulated network interface, and ran into a couple of problems. The VxWorks 
Intel PRO/1000 driver has been tested against a real Intel 82540EM adapter, 
and it works fine, however it does not work with the emulated 82540 in QEMU, 
because it doesn't quite duplicate the behavior of real hardware.

There are two issues:

1) The ICS register is not emulated correctly. It's not easy to discern from 
the Intel documentation, but the ICS register can be used in place of the ICR 
register in order to read the currently pending interrupt sources without 
automatically clearing them. The VxWorks driver needs to check interrupt 
events twice: once in its ISR, and again in task context. The auto-clear 
behavior of ICR makes it undesirable to use in the interrupt service routine, 
since it will clear the interrupt events, preventing the task level code from 
seeing them too (unless you preserve the values in software, which is tricky 
to do correcly). Consequently, VxWorks reads the ICS register in its 
interrupt service routine instead. This doesn't work in QEMU because:

- There is no entry in the readops table for reading the ICS register, so 
reading it always returns 0.
- The ICS register contents are not updated to reflect pending events in the 
set_interrupt_cause() routine.

2) The EERD register is not emulated correctly, which breaks VxWorks' EEPROM 
access code. The commonly available Intel drivers for Linux and *BSD don't 
use this register, and neither does the e1000 PXE ROM that comes with QEMU, 
so it probably hasn't been tested extensively. In real hardware, the register 
should only be updated when both an EEPROM offset and the START bit are 
written -- setting the START bit is what triggers an actual EEPROM read 
transaction. When the transaction is complete, the START bit is cleared, and 
the DONE bit is set. In QEMU, writing just the EEPROM offset is enough to 
cause the read transaction to occur: the simulated EEPROM contents appear and 
the DONE bit is set whether the START bit was set or not.

I was able to fix both of these issues in my local copy of e1000.c, and now 
the VxWorks PRO/1000 driver works correctly. I put the original code, patched 
version, and a context diff at the following URL:

http://www.freebsd.org/~wpaul/qemu

-Bill

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul@windriver.com | Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Anthony Liguori]

Hi Bill,

Bill Paul wrote:
> Hi, I hope this is the right forum for this. Apologies if it's not.
>
> I downloaded QEMU 0.10.5 and tested it against VxWorks 6.7 using the e1000 
> emulated network interface, and ran into a couple of problems. The VxWorks 
> Intel PRO/1000 driver has been tested against a real Intel 82540EM adapter, 
> and it works fine, however it does not work with the emulated 82540 in QEMU, 
> because it doesn't quite duplicate the behavior of real hardware.
>
> There are two issues:
>
> 1) The ICS register is not emulated correctly. It's not easy to discern from 
> the Intel documentation, but the ICS register can be used in place of the ICR 
> register in order to read the currently pending interrupt sources without 
> automatically clearing them. The VxWorks driver needs to check interrupt 
> events twice: once in its ISR, and again in task context. The auto-clear 
> behavior of ICR makes it undesirable to use in the interrupt service routine, 
> since it will clear the interrupt events, preventing the task level code from 
> seeing them too (unless you preserve the values in software, which is tricky 
> to do correcly). Consequently, VxWorks reads the ICS register in its 
> interrupt service routine instead. This doesn't work in QEMU because:
>
> - There is no entry in the readops table for reading the ICS register, so 
> reading it always returns 0.
> - The ICS register contents are not updated to reflect pending events in the 
> set_interrupt_cause() routine.
>
> 2) The EERD register is not emulated correctly, which breaks VxWorks' EEPROM 
> access code. The commonly available Intel drivers for Linux and *BSD don't 
> use this register, and neither does the e1000 PXE ROM that comes with QEMU, 
> so it probably hasn't been tested extensively. In real hardware, the register 
> should only be updated when both an EEPROM offset and the START bit are 
> written -- setting the START bit is what triggers an actual EEPROM read 
> transaction. When the transaction is complete, the START bit is cleared, and 
> the DONE bit is set. In QEMU, writing just the EEPROM offset is enough to 
> cause the read transaction to occur: the simulated EEPROM contents appear and 
> the DONE bit is set whether the START bit was set or not.
>
> I was able to fix both of these issues in my local copy of e1000.c, and now 
> the VxWorks PRO/1000 driver works correctly. I put the original code, patched 
> version, and a context diff at the following URL:
>
> http://www.freebsd.org/~wpaul/qemu
>   

Thanks for the thorough explanation!  Can you send the patch to the 
mailing list as a diff -u and include a Signed-off-by?

Is this only an issue with VxWorks or is it also reproducible in 
FreeBSD?  If the former, is there anything like an evaluation copy of 
VxWorks that I could use as a test harness?

Regards,

Anthony Liguori

> -Bill
>
>   

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Bill Paul]

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

Of all the gin joints in all the towns in all the world, Anthony Liguori had 
to walk into mine and say:
> Hi Bill,
>
> Bill Paul wrote:
> > Hi, I hope this is the right forum for this. Apologies if it's not.
> >
> > I downloaded QEMU 0.10.5 and tested it against VxWorks 6.7 using the
> > e1000 emulated network interface, and ran into a couple of problems. The
> > VxWorks Intel PRO/1000 driver has been tested against a real Intel
> > 82540EM adapter, and it works fine, however it does not work with the
> > emulated 82540 in QEMU, because it doesn't quite duplicate the behavior
> > of real hardware.
> >
> > There are two issues:
> >
> > 1) The ICS register is not emulated correctly. It's not easy to discern
> > from the Intel documentation, but the ICS register can be used in place
> > of the ICR register in order to read the currently pending interrupt
> > sources without automatically clearing them. The VxWorks driver needs to
> > check interrupt events twice: once in its ISR, and again in task context.
> > The auto-clear behavior of ICR makes it undesirable to use in the
> > interrupt service routine, since it will clear the interrupt events,
> > preventing the task level code from seeing them too (unless you preserve
> > the values in software, which is tricky to do correcly). Consequently,
> > VxWorks reads the ICS register in its interrupt service routine instead.
> > This doesn't work in QEMU because:
> >
> > - There is no entry in the readops table for reading the ICS register, so
> > reading it always returns 0.
> > - The ICS register contents are not updated to reflect pending events in
> > the set_interrupt_cause() routine.
> >
> > 2) The EERD register is not emulated correctly, which breaks VxWorks'
> > EEPROM access code. The commonly available Intel drivers for Linux and
> > *BSD don't use this register, and neither does the e1000 PXE ROM that
> > comes with QEMU, so it probably hasn't been tested extensively. In real
> > hardware, the register should only be updated when both an EEPROM offset
> > and the START bit are written -- setting the START bit is what triggers
> > an actual EEPROM read transaction. When the transaction is complete, the
> > START bit is cleared, and the DONE bit is set. In QEMU, writing just the
> > EEPROM offset is enough to cause the read transaction to occur: the
> > simulated EEPROM contents appear and the DONE bit is set whether the
> > START bit was set or not.
> >
> > I was able to fix both of these issues in my local copy of e1000.c, and
> > now the VxWorks PRO/1000 driver works correctly. I put the original code,
> > patched version, and a context diff at the following URL:
> >
> > http://www.freebsd.org/~wpaul/qemu
>
> Thanks for the thorough explanation!  Can you send the patch to the
> mailing list as a diff -u and include a Signed-off-by?

I can generate a unified diff, but I'm not sure I understand what is meant by 
"Signed-off-by." Can you elaborate? (Sorry, I'm not familiar with the QEMU 
development process. I just wanted to send a bug report. :)

> Is this only an issue with VxWorks or is it also reproducible in
> FreeBSD?

FreeBSD's PRO/1000 driver is written by Intel, just like the Linux driver. 
Unfortunately, neither driver uses the ICS or EERD registers, so the problems 
aren't visible on either OS, which is probably why nobody noticed them 
before. It's fairly easy to modify the FreeBSD driver to expose the problem 
with the ICS register (like I said, it always returns 0), but testing the 
problem with the EERD register is slightly harder.

>  If the former, is there anything like an evaluation copy of
> VxWorks that I could use as a test harness?

Strictly speaking, no. There's no such thing as a demo or eval version of 
VxWorks. However, I've built some binaries which should make it easy to 
validate the patches I've sent. I put the following files at 
http://www.freebsd.org/~wpaul/qemu :

-rwxr-xr-x  1 wpaul  devel   335360 Jun  9 02:47 bootrom.bin
-rw-r--r--  1 wpaul  devel   341939 Jun  9 02:53 bootrom.pxe
-rw-r--r--  1 wpaul  devel    36246 Jun  8 23:38 e1000.c
-rw-r--r--  1 wpaul  devel     1354 Jun  9 03:01 e1000.c.diff_u
-rw-r--r--  1 wpaul  devel    36077 Jun  8 23:38 e1000.c.orig
-rw-r--r--  1 wpaul  devel     1977 Jun  8 23:38 e1000.c.patch
-rwxr-xr-x  1 wpaul  devel  1721417 Jun  9 02:47 vxWorks
-rw-r--r--  1 wpaul  devel  1474560 Jun  9 02:47 vxworks.img

bootrom.bin and bootrom.pxe are floppy and PXE based VxWorks bootroms for the 
x86 arch. The bootrom.bin image is meant to go on a floppy with a special 
boot block. The bootrom.pxe image is largely the same code, but designed to 
be downloaded and booted via PXE. It works with QEMU, but you need to set up 
a DHCP and TFTP server in order to use it.

vxWorks is a standalone VxWorks 6.7 image for x86, which you can download and 
boot via ethernet with the boot floppy image as described below.

vxworks.img is a floppy disk image with bootrom.bin and the loader boot block 
already on it. I was able to run it using:

# qemu -net nic,macaddr=0:0:e8:1:2:3,model=e1000 -net tap -fda vxworks.img

Once the bootrom loads, you'll see a blue screen and a "Press any key to stop 
auto-boot" prompt with a 7 second countdown. Press a key, and you should be 
at the "[VxWorks Boot]:" prompt.

The bootrom normally wants to boot from floppy, so you need to change the boot 
configuration to tell it to boot from network. If you press "?" and then 
ENTER at the boot prompt, you should see a list of available boot devices, 
and "gei0" should be listed. To change the boot parameters, type "c" and then 
ENTER.

For "boot device," specify "gei0".
For "processor number," just hit ENTER.
For "host name," just hit ENTER.
For "File name," you can specify the path to the vxWorks image on your FTP 
server.
For "inet on ethernet," specify the IP address and netmask for the simulated 
target in the form of <IP>:<netmask>, e.g.: "10.0.0.1:ffffff00"
For "inet on backplane," just hit ENTER.
For "host inet", specify the IP address of the FTP server from which you want 
to download the vxWorks image.
For "gateway imet," specify the IP address of the default gateway for the 
simulated network.
For "user" and "ftp password," provide a valid username and password 
combination that can be used to log into the FTP server where the vxWorks 
image resides. (If the password is left blank, it will try to load the image 
via rsh instead, which is clunky, but sometimes useful.)
For all the rest of the fields, just hit ENTER (they're not useful here, but 
feel free to play with them).

Once you get back to the "[VxWorks Boot]:" prompt, the ethernet should be 
live, and you should be able to ping the bootrom via the simulated network. 
To boot the vxWorks image, type "@" and then ENTER at the prompt, and it 
should say "Loading...." The image should transfer via FTP, and you should be 
presented with a small VxWorks banner and the target shell prompt ("->").

The supplied VxWorks image has ping and the telnet server built in. I till 
inherit the IP address and other information from the bootrom. You can type 
"version" at the shell prompt to see. You should be able to telnet to the 
VxWorks image via the simulated network. You can also do things like:

-> ifconfig "-a"
-> ifconfig "-h"
-> i (list tasks)
-> help (small amount of shell command help)
-> ping "<ip address>", <number of ping packets> (ping "10.0.1.2", 3)
-> muxShow (show network interfaces)
-> vxBusShow (show drivers and devices)

With the broken e1000 code, the bootrom will crash QEMU almost as soon as it 
starts. This is because the problem with the ICS register causes the PRO/1000 
driver to fail to detect that any interrupts are asserted, so it never acks 
and clears them. This causes the VxWorks driver's interrupt service routine 
to re-trigger endlessly. The problem with the EERD register causes it to 
botch the first 2 bytes of the ethernet address when reading it from NVRAM 
during startup. With the patch applied, it all works fine.

I hope all this makes sense. VxWorks is designed to be fairly bare bones, 
since it's usually up to the customer to design their application code on top 
of it.

-Bill

> Regards,
>
> Anthony Liguori
>
> > -Bill

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul@windriver.com | Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

[-- Attachment #2: e1000.c.diff_u --]
[-- Type: text/x-diff, Size: 1354 bytes --]

--- e1000.c.orig	2009-05-20 13:46:59.000000000 -0700
+++ e1000.c	2009-06-08 14:23:24.000000000 -0700
@@ -155,6 +155,7 @@
     if (val)
         val |= E1000_ICR_INT_ASSERTED;
     s->mac_reg[ICR] = val;
+    s->mac_reg[ICS] = val;
     qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0);
 }
 
@@ -275,10 +276,14 @@
 {
     unsigned int index, r = s->mac_reg[EERD] & ~E1000_EEPROM_RW_REG_START;
 
+    if ((s->mac_reg[EERD] & E1000_EEPROM_RW_REG_START) == 0)
+        return (s->mac_reg[EERD]);
+
     if ((index = r >> E1000_EEPROM_RW_ADDR_SHIFT) > EEPROM_CHECKSUM_REG)
-        return 0;
-    return (s->eeprom_data[index] << E1000_EEPROM_RW_REG_DATA) |
-           E1000_EEPROM_RW_REG_DONE | r;
+        return (E1000_EEPROM_RW_REG_DONE | r);
+
+    return ((s->eeprom_data[index] << E1000_EEPROM_RW_REG_DATA) |
+           E1000_EEPROM_RW_REG_DONE | r);
 }
 
 static void
@@ -767,7 +772,7 @@
     getreg(WUFC),	getreg(TDT),	getreg(CTRL),	getreg(LEDCTL),
     getreg(MANC),	getreg(MDIC),	getreg(SWSM),	getreg(STATUS),
     getreg(TORL),	getreg(TOTL),	getreg(IMS),	getreg(TCTL),
-    getreg(RDH),	getreg(RDT),	getreg(VET),
+    getreg(RDH),	getreg(RDT),	getreg(VET),	getreg(ICS),
 
     [TOTH] = mac_read_clr8,	[TORH] = mac_read_clr8,	[GPRC] = mac_read_clr4,
     [GPTC] = mac_read_clr4,	[TPR] = mac_read_clr4,	[TPT] = mac_read_clr4,

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Richard W.M. Jones]

On Mon, Jun 08, 2009 at 08:39:26PM -0700, Bill Paul wrote:
> I can generate a unified diff, but I'm not sure I understand what is meant by 
> "Signed-off-by." Can you elaborate? (Sorry, I'm not familiar with the QEMU 
> development process. I just wanted to send a bug report. :)

It's just a convention started on the Linux kernel mailing lists a
few years back.  Just put this in your email with the patch:

  Signed-off-by: Your Name <your@email.example.com>

See also: http://kerneltrap.org/node/3929

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Bill Paul]

Of all the gin joints in all the towns in all the world, Richard W.M. Jones 
had to walk into mine and say:

> On Mon, Jun 08, 2009 at 08:39:26PM -0700, Bill Paul wrote:
> > I can generate a unified diff, but I'm not sure I understand what is
> > meant by "Signed-off-by." Can you elaborate? (Sorry, I'm not familiar
> > with the QEMU development process. I just wanted to send a bug report. :)
>
> It's just a convention started on the Linux kernel mailing lists a
> few years back.  Just put this in your email with the patch:
>
>   Signed-off-by: Your Name <your@email.example.com>
>
> See also: http://kerneltrap.org/node/3929
>
> Rich.

Ah, ok. Will do.

Note: this diff is against the 0.10.5 released sources. not the latest code in 
the repository (though the same bugs are still there, I'm sure).

-Bill

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul@windriver.com | Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================


Signed-off-by: Bill Paul <wpaul@windriver.com>
---

--- e1000.c.orig	2009-05-20 13:46:59.000000000 -0700
+++ e1000.c	2009-06-08 14:23:24.000000000 -0700
@@ -155,6 +155,7 @@
     if (val)
         val |= E1000_ICR_INT_ASSERTED;
     s->mac_reg[ICR] = val;
+    s->mac_reg[ICS] = val;
     qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0);
 }
 
@@ -275,10 +276,14 @@
 {
     unsigned int index, r = s->mac_reg[EERD] & ~E1000_EEPROM_RW_REG_START;
 
+    if ((s->mac_reg[EERD] & E1000_EEPROM_RW_REG_START) == 0)
+        return (s->mac_reg[EERD]);
+
     if ((index = r >> E1000_EEPROM_RW_ADDR_SHIFT) > EEPROM_CHECKSUM_REG)
-        return 0;
-    return (s->eeprom_data[index] << E1000_EEPROM_RW_REG_DATA) |
-           E1000_EEPROM_RW_REG_DONE | r;
+        return (E1000_EEPROM_RW_REG_DONE | r);
+
+    return ((s->eeprom_data[index] << E1000_EEPROM_RW_REG_DATA) |
+           E1000_EEPROM_RW_REG_DONE | r);
 }
 
 static void
@@ -767,7 +772,7 @@
     getreg(WUFC),	getreg(TDT),	getreg(CTRL),	getreg(LEDCTL),
     getreg(MANC),	getreg(MDIC),	getreg(SWSM),	getreg(STATUS),
     getreg(TORL),	getreg(TOTL),	getreg(IMS),	getreg(TCTL),
-    getreg(RDH),	getreg(RDT),	getreg(VET),
+    getreg(RDH),	getreg(RDT),	getreg(VET),	getreg(ICS),
 
     [TOTH] = mac_read_clr8,	[TORH] = mac_read_clr8,	[GPRC] = mac_read_clr4,
     [GPTC] = mac_read_clr4,	[TPR] = mac_read_clr4,	[TPT] = mac_read_clr4,

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Bill Paul]

Hi guys:

I submitted a patch for e1000.c a while ago, to fix issues with the ICS and 
EERD registers not being emulated properly, which I discovered while testing 
QEMU with VxWorks. I provided binary VxWorks images and a bootable bootrom 
floppy that works with QEMU for testing, but so far I haven't heard anything 
further on this issue. I looked at the git repository, and it doesn't look 
like the patch I submitted has been applied yet.

Does anyone have an update on this? Just checking to make sure the patch 
doesn't fall through the cracks.

-Bill

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul@windriver.com | Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Anthony Liguori]

Bill Paul wrote:
> Hi guys:
>
> I submitted a patch for e1000.c a while ago, to fix issues with the ICS and 
> EERD registers not being emulated properly, which I discovered while testing 
> QEMU with VxWorks. I provided binary VxWorks images and a bootable bootrom 
> floppy that works with QEMU for testing, but so far I haven't heard anything 
> further on this issue. I looked at the git repository, and it doesn't look 
> like the patch I submitted has been applied yet.
>
> Does anyone have an update on this? Just checking to make sure the patch 
> doesn't fall through the cracks.
>   

I see you sent out a version with a Signed-off-by, but you didn't send 
it as a top-level patch.

Please resend it as a top level patch (a new subject starting with 
[PATCH]) and that will get the ball rolling.

Regards,

Anthony Liguori

> -Bill
>
>   

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Bill Paul]

Of all the gin joints in all the towns in all the world, Anthony Liguori had 
to walk into mine and say:

> Bill Paul wrote:
> > Hi guys:
> >
> > I submitted a patch for e1000.c a while ago, to fix issues with the ICS
> > and EERD registers not being emulated properly, which I discovered while
> > testing QEMU with VxWorks. I provided binary VxWorks images and a
> > bootable bootrom floppy that works with QEMU for testing, but so far I
> > haven't heard anything further on this issue. I looked at the git
> > repository, and it doesn't look like the patch I submitted has been
> > applied yet.
> >
> > Does anyone have an update on this? Just checking to make sure the patch
> > doesn't fall through the cracks.
>
> I see you sent out a version with a Signed-off-by, but you didn't send
> it as a top-level patch.
>
> Please resend it as a top level patch (a new subject starting with
> [PATCH]) and that will get the ball rolling.

Let me make sure I understand correctly.

You must have my previous e-mail with the patch in front of you, with the 
attached unified diff. Are you saying that rather than just taking that 
unidiff, from the e-mail I already sent, you want me to send you exactly the 
same file, only with a different subject line that starts with [PATCH]?

I'd like to point out that a) while this may be part of some standardized 
project etiquette, I've yet to see these required steps clearly spelled out 
anywhere, and b) why can't you just take the diff I already sent and 
apply/test/molest/etc it now that you have it?

-Bill

> Regards,
>
> Anthony Liguori
>
> > -Bill

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul@windriver.com | Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Anthony Liguori]

Bill Paul wrote:
> Let me make sure I understand correctly.
>
> You must have my previous e-mail with the patch in front of you, with the 
> attached unified diff. Are you saying that rather than just taking that 
> unidiff, from the e-mail I already sent, you want me to send you exactly the 
> same file, only with a different subject line that starts with [PATCH]?
>   

Yes.

> I'd like to point out that a) while this may be part of some standardized 
> project etiquette, I've yet to see these required steps clearly spelled out 
> anywhere,

Yes, we're definitely overdue for a SubmittingPatches file to live in 
the tree.  I suggested that you follow these steps not due to any sort 
of desire to follow arbitrary procedures but because it guarantees your 
patch will get attention.

>  and b) why can't you just take the diff I already sent and 
> apply/test/molest/etc it now that you have it?
>   

Well first, it's against 0.10.5 which means there's nothing to apply it 
to.  It has to be against our development tree.  Second, a system scales 
better when you push work down to the outer most nodes.  It's easier for 
to have you resubmit a patch and have everyone follow the same 
procedures than to have me manually extract individual patches from 
random threads, tweak them to apply to git, etc.

Regards,

Anthony Liguori

Re: [Qemu-devel] bug report + fix: e1000.c in 0.10.5 does not properly emulate real hardware

[Bill Paul]

Of all the gin joints in all the towns in all the world, Anthony Liguori had 
to walk into mine and say:

> Bill Paul wrote:
> > Let me make sure I understand correctly.
> >
> > You must have my previous e-mail with the patch in front of you, with the
> > attached unified diff. Are you saying that rather than just taking that
> > unidiff, from the e-mail I already sent, you want me to send you exactly
> > the same file, only with a different subject line that starts with
> > [PATCH]?
>
> Yes.

There is this thing called the principle of least astonishment. You just 
violated it.

> > I'd like to point out that a) while this may be part of some standardized
> > project etiquette, I've yet to see these required steps clearly spelled
> > out anywhere,
>
> Yes, we're definitely overdue for a SubmittingPatches file to live in
> the tree.  I suggested that you follow these steps not due to any sort
> of desire to follow arbitrary procedures but because it guarantees your
> patch will get attention.

It clearly had already gotten attention since you e-mailed me about it right 
after I submitted it.

> >  and b) why can't you just take the diff I already sent and
> > apply/test/molest/etc it now that you have it?
>
> Well first, it's against 0.10.5 which means there's nothing to apply it
> to.  It has to be against our development tree.

I'm sorry, but this argument is based on the faulty assumption that the patch 
as I sent it wouldn't apply cleanly to the development version of e1000.c. In 
fact, it does. ("But Bill, there are a few lines offset..." Yes. I know. The 
resulting patched source is still correct.)

>  Second, a system scales
> better when you push work down to the outer most nodes.  It's easier for
> to have you resubmit a patch and have everyone follow the same
> procedures than to have me manually extract individual patches from
> random threads, tweak them to apply to git, etc.

Again, I'm sorry, but no. The most efficient thing to do here would have 
simply been to save the patch that I had previously sent you and apply it. 
There would have been no tweaking required, and it would have taken you less 
time to do that than to e-mail me asking me to resend the same patch over 
again.

And another thing: I am not an "outer-most node" in a "system." I'm a person 
who already has far too many demands on his time, not a script that emits 
carefully formatted output designed conform to some strictly-enforced set of 
rules that aren't even written down anywhere.

I sent in the patch one more time. If it turns out there's some other tiny 
thing wrong with it ("Wait, Bill... you didn't run git exactly the right 
way!") then either fix it, or just forget the whole thing. Ok?

-Bill

> Regards,
>
> Anthony Liguori

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul@windriver.com | Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================