[Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Naphtali Sprei]

After comments from quintela@trasno.org and mst@redhat.com, here's the 
2nd try:


The sequence of reading from eeprom is "offset by one" moved because of 
a false detection
of a clock cycle after an eeprom reset. Keeping the last clock value 
after a reset keeps it in sync.

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 hw/e1000.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 4ac8918..15598ab 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -262,6 +262,8 @@ set_eecd(E1000State *s, int index, uint32_t val)
     }
     if (!(val & E1000_EECD_CS)) {              // rising, no CS (EEPROM 
reset)
         memset(&s->eecd_state, 0, sizeof s->eecd_state);
+        // restore old_eecd's E1000_EECD_SK to avoid false detection of 
a clock edge
+        s->eecd_state.old_eecd = E1000_EECD_SK;
         return;
     }
     s->eecd_state.val_in <<= 1;
--
1.5.5.6

Re: [Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Hollis Blanchard]

On Wed, 2009-07-15 at 15:18 +0300, Naphtali Sprei wrote:
> After comments from quintela@trasno.org and mst@redhat.com, here's the 
> 2nd try:
> 
> 
> The sequence of reading from eeprom is "offset by one" moved because of 
> a false detection
> of a clock cycle after an eeprom reset. Keeping the last clock value 
> after a reset keeps it in sync.
> 
> Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
> ---
>  hw/e1000.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 4ac8918..15598ab 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -262,6 +262,8 @@ set_eecd(E1000State *s, int index, uint32_t val)
>      }
>      if (!(val & E1000_EECD_CS)) {              // rising, no CS (EEPROM 
> reset)
>          memset(&s->eecd_state, 0, sizeof s->eecd_state);
> +        // restore old_eecd's E1000_EECD_SK to avoid false detection of 
> a clock edge
> +        s->eecd_state.old_eecd = E1000_EECD_SK;
>          return;
>      }
>      s->eecd_state.val_in <<= 1;
> --
> 1.5.5.6

This fixes the problem for me.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Jamie Lokier]

On Wed, 2009-07-15 at 15:18 +0300, Naphtali Sprei wrote:
> After comments from quintela@trasno.org and mst@redhat.com, here's the 
> 2nd try:
> 
> The sequence of reading from eeprom is "offset by one" moved because
> of a false detection of a clock cycle after an eeprom reset. Keeping
> the last clock value after a reset keeps it in sync.

Isn't it more likely that the real hardware resets the clock value to
a fixed level - the polarity being the opposite of what QEMU currently
does, so that there's no edge caused by the first write?

-- Jamie

Re: [Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Naphtali Sprei]

Jamie Lokier wrote:
> On Wed, 2009-07-15 at 15:18 +0300, Naphtali Sprei wrote:
>> After comments from quintela@trasno.org and mst@redhat.com, here's the 
>> 2nd try:
>>
>> The sequence of reading from eeprom is "offset by one" moved because
>> of a false detection of a clock cycle after an eeprom reset. Keeping
>> the last clock value after a reset keeps it in sync.
> 
> Isn't it more likely that the real hardware resets the clock value to
> a fixed level - the polarity being the opposite of what QEMU currently
> does, so that there's no edge caused by the first write?
The eeprom reset I referred to is actually a reset of the internal state of (part of) the device emulation in qemu.
The software drives the clock bit (SK) and the reset shouldn't change it. 
> 
> -- Jamie
> 
> 
> 
> 

Re: [Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Anthony Liguori]

Naphtali Sprei wrote:
> After comments from quintela@trasno.org and mst@redhat.com, here's the 
> 2nd try:
>
>
> The sequence of reading from eeprom is "offset by one" moved because 
> of a false detection
> of a clock cycle after an eeprom reset. Keeping the last clock value 
> after a reset keeps it in sync.
>
> Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
> ---
> hw/e1000.c |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 4ac8918..15598ab 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -262,6 +262,8 @@ set_eecd(E1000State *s, int index, uint32_t val)
>     }
>     if (!(val & E1000_EECD_CS)) {              // rising, no CS 
> (EEPROM reset)
>         memset(&s->eecd_state, 0, sizeof s->eecd_state);
> +        // restore old_eecd's E1000_EECD_SK to avoid false detection 
> of a clock edge

Your mailer is wrapping these lines.  We also prefer C89 comments in QEMU.

Regards,

Anthony Liguori

Re: [Qemu-devel] 2nd try: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

[Naphtali Sprei]

comment format changed, (hopefully) fixed mailer wrapping.


Subject: [PATCH] fix for bad macaddr of e1000 in Windows 2003 server with original Microsoft driver

The sequence of reading from eeprom is "offset by one" moved because of a false detection
of a clock cycle after an eeprom reset. Keeping the last clock value after a reset keeps it in sync.

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 hw/e1000.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 4ac8918..b819501 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -262,6 +262,11 @@ set_eecd(E1000State *s, int index, uint32_t val)
     }
     if (!(val & E1000_EECD_CS)) {		// rising, no CS (EEPROM reset)
         memset(&s->eecd_state, 0, sizeof s->eecd_state);
+        /*
+         * restore old_eecd's E1000_EECD_SK (known to be on)
+         * to avoid false detection of a clock edge
+         */
+        s->eecd_state.old_eecd = E1000_EECD_SK;
         return;
     }
     s->eecd_state.val_in <<= 1;
-- 
1.5.5.6