[Qemu-devel] [PATCH] rtl8139: implement 8139cp link status

[Qemu-devel] [PATCH] rtl8139: implement 8139cp link status

[Amos Kong]

From: Jason Wang <jasowang@redhat.com>

Add a link status chang callback and change the link status bit in BMSR
& MSR accordingly. Tested in Linux/Windows guests.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/rtl8139.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 844f1b8..3c33908 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -167,7 +167,7 @@ enum IntrStatusBits {
     PCIErr = 0x8000,
     PCSTimeout = 0x4000,
     RxFIFOOver = 0x40,
-    RxUnderrun = 0x20,
+    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
     RxOverflow = 0x10,
     TxErr = 0x08,
     TxOK = 0x04,
@@ -452,6 +452,7 @@ typedef struct RTL8139State {
     uint8_t  Config0;
     uint8_t  Config1;
     uint8_t  Config3;
+    uint8_t  MediaStatus;
     uint8_t  Config4;
     uint8_t  Config5;
 
@@ -1246,6 +1247,7 @@ static void rtl8139_reset(DeviceState *d)
     /* set initial state data */
     s->Config0 = 0x0; /* No boot ROM */
     s->Config1 = 0xC; /* IO mapped and MEM mapped registers available */
+    s->MediaStatus = 0xd0; /* Power is present, TX/RX flow control enable */
     s->Config3 = 0x1; /* fast back-to-back compatible */
     s->Config5 = 0x0;
 
@@ -3007,7 +3009,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
             break;
 
         case MediaStatus:
-            ret = 0xd0;
+            ret = s->MediaStatus;
             DPRINTF("MediaStatus read 0x%x\n", ret);
             break;
 
@@ -3453,12 +3455,29 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
     qemu_del_net_client(&s->nic->nc);
 }
 
+static void rtl8139_set_link_status(NetClientState *nc)
+{
+    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+
+    if (nc->link_down) {
+        s->BasicModeStatus &= ~0x0004;
+        s->MediaStatus |= 0x0004;
+    } else {
+        s->BasicModeStatus |= 0x0004;
+        s->MediaStatus &= ~0x0004;
+    }
+
+    s->IntrStatus |= RxUnderrun;
+    rtl8139_update_irq(s);
+}
+
 static NetClientInfo net_rtl8139_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
     .can_receive = rtl8139_can_receive,
     .receive = rtl8139_receive,
     .cleanup = rtl8139_cleanup,
+    .link_status_changed = rtl8139_set_link_status,
 };
 
 static int pci_rtl8139_init(PCIDevice *dev)
-- 
1.7.1

Re: [Qemu-devel] [PATCH] rtl8139: implement 8139cp link status

[Jason Wang]

On 09/10/2012 03:59 PM, Amos Kong wrote:
> From: Jason Wang<jasowang@redhat.com>
>
> Add a link status chang callback and change the link status bit in BMSR
> &  MSR accordingly. Tested in Linux/Windows guests.
>
> Signed-off-by: Jason Wang<jasowang@redhat.com>
> Signed-off-by: Amos Kong<akong@redhat.com>
> ---
>   hw/rtl8139.c |   23 +++++++++++++++++++++--
>   1 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 844f1b8..3c33908 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -167,7 +167,7 @@ enum IntrStatusBits {
>       PCIErr = 0x8000,
>       PCSTimeout = 0x4000,
>       RxFIFOOver = 0x40,
> -    RxUnderrun = 0x20,
> +    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
>       RxOverflow = 0x10,
>       TxErr = 0x08,
>       TxOK = 0x04,
> @@ -452,6 +452,7 @@ typedef struct RTL8139State {
>       uint8_t  Config0;
>       uint8_t  Config1;
>       uint8_t  Config3;
> +    uint8_t  MediaStatus;
>       uint8_t  Config4;
>       uint8_t  Config5;
>   

So the content of this register would be lost after migration?
> @@ -1246,6 +1247,7 @@ static void rtl8139_reset(DeviceState *d)
>       /* set initial state data */
>       s->Config0 = 0x0; /* No boot ROM */
>       s->Config1 = 0xC; /* IO mapped and MEM mapped registers available */
> +    s->MediaStatus = 0xd0; /* Power is present, TX/RX flow control enable */
>       s->Config3 = 0x1; /* fast back-to-back compatible */
>       s->Config5 = 0x0;
>
> @@ -3007,7 +3009,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
>               break;
>
>           case MediaStatus:
> -            ret = 0xd0;
> +            ret = s->MediaStatus;
>               DPRINTF("MediaStatus read 0x%x\n", ret);
>               break;
>
> @@ -3453,12 +3455,29 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
>       qemu_del_net_client(&s->nic->nc);
>   }
>
> +static void rtl8139_set_link_status(NetClientState *nc)
> +{
> +    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
> +
> +    if (nc->link_down) {
> +        s->BasicModeStatus&= ~0x0004;
> +        s->MediaStatus |= 0x0004;
> +    } else {
> +        s->BasicModeStatus |= 0x0004;
> +        s->MediaStatus&= ~0x0004;
> +    }
> +
> +    s->IntrStatus |= RxUnderrun;
> +    rtl8139_update_irq(s);
> +}
> +
>   static NetClientInfo net_rtl8139_info = {
>       .type = NET_CLIENT_OPTIONS_KIND_NIC,
>       .size = sizeof(NICState),
>       .can_receive = rtl8139_can_receive,
>       .receive = rtl8139_receive,
>       .cleanup = rtl8139_cleanup,
> +    .link_status_changed = rtl8139_set_link_status,
>   };
>
>   static int pci_rtl8139_init(PCIDevice *dev)

Re: [Qemu-devel] [PATCH] rtl8139: implement 8139cp link status

[Jason Wang]

On 09/10/2012 04:15 PM, Jason Wang wrote:
> On 09/10/2012 03:59 PM, Amos Kong wrote:
>> From: Jason Wang<jasowang@redhat.com>
>>
>> Add a link status chang callback and change the link status bit in BMSR
>> &  MSR accordingly. Tested in Linux/Windows guests.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   hw/rtl8139.c |   23 +++++++++++++++++++++--
>>   1 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index 844f1b8..3c33908 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -167,7 +167,7 @@ enum IntrStatusBits {
>>       PCIErr = 0x8000,
>>       PCSTimeout = 0x4000,
>>       RxFIFOOver = 0x40,
>> -    RxUnderrun = 0x20,
>> +    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
>>       RxOverflow = 0x10,
>>       TxErr = 0x08,
>>       TxOK = 0x04,
>> @@ -452,6 +452,7 @@ typedef struct RTL8139State {
>>       uint8_t  Config0;
>>       uint8_t  Config1;
>>       uint8_t  Config3;
>> +    uint8_t  MediaStatus;
>>       uint8_t  Config4;
>>       uint8_t  Config5;
>
> So the content of this register would be lost after migration?

Or you can do some hack to avoid hanling migration, e.g infer the the 
link status bit of MediaStatus from BasicModeStatus.
>> @@ -1246,6 +1247,7 @@ static void rtl8139_reset(DeviceState *d)
>>       /* set initial state data */
>>       s->Config0 = 0x0; /* No boot ROM */
>>       s->Config1 = 0xC; /* IO mapped and MEM mapped registers 
>> available */
>> +    s->MediaStatus = 0xd0; /* Power is present, TX/RX flow control 
>> enable */
>>       s->Config3 = 0x1; /* fast back-to-back compatible */
>>       s->Config5 = 0x0;
>>
>> @@ -3007,7 +3009,7 @@ static uint32_t rtl8139_io_readb(void *opaque, 
>> uint8_t addr)
>>               break;
>>
>>           case MediaStatus:
>> -            ret = 0xd0;
>> +            ret = s->MediaStatus;
>>               DPRINTF("MediaStatus read 0x%x\n", ret);
>>               break;
>>
>> @@ -3453,12 +3455,29 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
>>       qemu_del_net_client(&s->nic->nc);
>>   }
>>
>> +static void rtl8139_set_link_status(NetClientState *nc)
>> +{
>> +    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
>> +
>> +    if (nc->link_down) {
>> +        s->BasicModeStatus&= ~0x0004;
>> +        s->MediaStatus |= 0x0004;
>> +    } else {
>> +        s->BasicModeStatus |= 0x0004;
>> +        s->MediaStatus&= ~0x0004;
>> +    }
>> +
>> +    s->IntrStatus |= RxUnderrun;
>> +    rtl8139_update_irq(s);
>> +}
>> +
>>   static NetClientInfo net_rtl8139_info = {
>>       .type = NET_CLIENT_OPTIONS_KIND_NIC,
>>       .size = sizeof(NICState),
>>       .can_receive = rtl8139_can_receive,
>>       .receive = rtl8139_receive,
>>       .cleanup = rtl8139_cleanup,
>> +    .link_status_changed = rtl8139_set_link_status,
>>   };
>>
>>   static int pci_rtl8139_init(PCIDevice *dev)
>

[Qemu-devel] [PATCH v2] rtl8139: implement 8139cp link status

[Amos Kong]

From: Jason Wang <jasowang@redhat.com>

Add a link status chang callback and change the link status bit in BMSR
& MSR accordingly. Tested in Linux/Windows guests.

The link status bit of MediaStatus is infered from BasicModeStatus,
they are reverse. 

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
v2: don't add MediaState in RTL8139State to avoid migration trouble
---
 hw/rtl8139.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 844f1b8..fa949ca 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -167,7 +167,7 @@ enum IntrStatusBits {
     PCIErr = 0x8000,
     PCSTimeout = 0x4000,
     RxFIFOOver = 0x40,
-    RxUnderrun = 0x20,
+    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
     RxOverflow = 0x10,
     TxErr = 0x08,
     TxOK = 0x04,
@@ -3007,7 +3007,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
             break;
 
         case MediaStatus:
-            ret = 0xd0;
+            ret = 0xd0 | ~(s->BasicModeStatus & 0x0004);
             DPRINTF("MediaStatus read 0x%x\n", ret);
             break;
 
@@ -3453,12 +3453,27 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
     qemu_del_net_client(&s->nic->nc);
 }
 
+static void rtl8139_set_link_status(NetClientState *nc)
+{
+    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+
+    if (nc->link_down) {
+        s->BasicModeStatus &= ~0x0004;
+    } else {
+        s->BasicModeStatus |= 0x0004;
+    }
+
+    s->IntrStatus |= RxUnderrun;
+    rtl8139_update_irq(s);
+}
+
 static NetClientInfo net_rtl8139_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
     .can_receive = rtl8139_can_receive,
     .receive = rtl8139_receive,
     .cleanup = rtl8139_cleanup,
+    .link_status_changed = rtl8139_set_link_status,
 };
 
 static int pci_rtl8139_init(PCIDevice *dev)
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] rtl8139: implement 8139cp link status

[Stefan Hajnoczi]

On Thu, Sep 13, 2012 at 9:51 AM, Amos Kong <akong@redhat.com> wrote:
> From: Jason Wang <jasowang@redhat.com>
>
> Add a link status chang callback and change the link status bit in BMSR
> & MSR accordingly. Tested in Linux/Windows guests.
>
> The link status bit of MediaStatus is infered from BasicModeStatus,
> they are reverse.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> v2: don't add MediaState in RTL8139State to avoid migration trouble
> ---
>  hw/rtl8139.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 844f1b8..fa949ca 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -167,7 +167,7 @@ enum IntrStatusBits {
>      PCIErr = 0x8000,
>      PCSTimeout = 0x4000,
>      RxFIFOOver = 0x40,
> -    RxUnderrun = 0x20,
> +    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
>      RxOverflow = 0x10,
>      TxErr = 0x08,
>      TxOK = 0x04,
> @@ -3007,7 +3007,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
>              break;
>
>          case MediaStatus:
> -            ret = 0xd0;
> +            ret = 0xd0 | ~(s->BasicModeStatus & 0x0004);
>              DPRINTF("MediaStatus read 0x%x\n", ret);
>              break;

This does not give any hint that BMSR & 0x4 is the link status
(inverted).  I suggest adding an enum like the other constants at the
top of the file:

ret = 0xd0 | (nc->link_down ? MSRLinkDown : 0);

Regarding migration: do we migrate the NetClient->link_down field?  If
we only migrate the status register value then the link may actually
be up at the net.c level.

Stefan

Re: [Qemu-devel] [PATCH v2] rtl8139: implement 8139cp link status

[Amos Kong]

On 13/09/12 20:29, Stefan Hajnoczi wrote:
> On Thu, Sep 13, 2012 at 9:51 AM, Amos Kong <akong@redhat.com> wrote:
>> From: Jason Wang <jasowang@redhat.com>
>>
>> Add a link status chang callback and change the link status bit in BMSR
>> & MSR accordingly. Tested in Linux/Windows guests.
>>
>> The link status bit of MediaStatus is infered from BasicModeStatus,
>> they are reverse.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>> v2: don't add MediaState in RTL8139State to avoid migration trouble
>> ---
>>   hw/rtl8139.c |   19 +++++++++++++++++--
>>   1 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index 844f1b8..fa949ca 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -167,7 +167,7 @@ enum IntrStatusBits {
>>       PCIErr = 0x8000,
>>       PCSTimeout = 0x4000,
>>       RxFIFOOver = 0x40,
>> -    RxUnderrun = 0x20,
>> +    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
>>       RxOverflow = 0x10,
>>       TxErr = 0x08,
>>       TxOK = 0x04,
>> @@ -3007,7 +3007,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
>>               break;
>>
>>           case MediaStatus:
>> -            ret = 0xd0;
>> +            ret = 0xd0 | ~(s->BasicModeStatus & 0x0004);
>>               DPRINTF("MediaStatus read 0x%x\n", ret);
>>               break;
>
> This does not give any hint that BMSR & 0x4 is the link status
> (inverted).

only mentioned in the commitlog, will add a comment here.

> I suggest adding an enum like the other constants at the
> top of the file:
>
> ret = 0xd0 | (nc->link_down ? MSRLinkDown : 0);

Ok, I will update the patch.

> Regarding migration: do we migrate the NetClient->link_down field? If
> we only migrate the status register value then the link may actually
> be up at the net.c level.

I tried to add 'MediaStatus' to 'struct RTL8139State', and update
'VMStateDescription vmstate_rtl8139', then the value of MediaStatus
will be migrated.

But the idea in v2 is better.

> Stefan

Thanks.

-- 
			Amos.

[Qemu-devel] [PATCH v3] rtl8139: implement 8139cp link status

[Amos Kong]

From: Jason Wang <jasowang@redhat.com>

Add a link status chang callback and change the link status bit in BMSR
& MSR accordingly. Tested in Linux/Windows guests.

The LinkDown bit of MediaStatus is inverse with link status.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
v2: don't add MediaState in RTL8139State to avoid trouble
v3: adding an enum MediaStatusBits
---
 hw/rtl8139.c |   32 ++++++++++++++++++++++++++++++--
 1 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 844f1b8..02d571f 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -167,7 +167,7 @@ enum IntrStatusBits {
     PCIErr = 0x8000,
     PCSTimeout = 0x4000,
     RxFIFOOver = 0x40,
-    RxUnderrun = 0x20,
+    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
     RxOverflow = 0x10,
     TxErr = 0x08,
     TxOK = 0x04,
@@ -274,6 +274,18 @@ enum Config3Bits {
     Cfg3_GNTSel    = (1 << 7), /* 1 = delay 1 clock from PCI GNT signal */
 };
 
+/* Bits in MediaStatus */
+enum MediaStatusBits {
+    MSR_RXPF       = (1 << 0), /* Receive Pause Flag */
+    MSR_TXPF       = (1 << 1), /* Transmit Pause Flag */
+    MSR_LinkDown   = (1 << 2), /* Inverse of Link Status */
+    MSR_Speed_10   = (1 << 3), /* Media Speed */
+    MSR_Aux_Status = (1 << 4), /* Aux. Power Present Status */
+    MSR_Reserved   = (1 << 5), /* Reserved */
+    MSR_RXFCE      = (1 << 6), /* Rx Flow Control Enable */
+    MSR_TXFCE      = (1 << 7), /* Tx Flow Control Enable */
+};
+
 /* Bits in Config4 */
 enum Config4Bits {
     LWPTN = (1 << 2),    /* not on 8139, 8139A */
@@ -3007,7 +3019,8 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
             break;
 
         case MediaStatus:
-            ret = 0xd0;
+            /* The LinkDown bit of MediaStatus is inverse with link status */
+            ret = 0xd0 | (s->nic->nc.link_down ? MSR_LinkDown : 0);
             DPRINTF("MediaStatus read 0x%x\n", ret);
             break;
 
@@ -3453,12 +3466,27 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
     qemu_del_net_client(&s->nic->nc);
 }
 
+static void rtl8139_set_link_status(NetClientState *nc)
+{
+    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+
+    if (nc->link_down) {
+        s->BasicModeStatus &= ~0x0004;
+    } else {
+        s->BasicModeStatus |= 0x0004;
+    }
+
+    s->IntrStatus |= RxUnderrun;
+    rtl8139_update_irq(s);
+}
+
 static NetClientInfo net_rtl8139_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
     .can_receive = rtl8139_can_receive,
     .receive = rtl8139_receive,
     .cleanup = rtl8139_cleanup,
+    .link_status_changed = rtl8139_set_link_status,
 };
 
 static int pci_rtl8139_init(PCIDevice *dev)
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] rtl8139: implement 8139cp link status

[Stefan Hajnoczi]

On Fri, Sep 14, 2012 at 2:34 AM, Amos Kong <akong@redhat.com> wrote:
> On 13/09/12 20:29, Stefan Hajnoczi wrote:
>>
>> On Thu, Sep 13, 2012 at 9:51 AM, Amos Kong <akong@redhat.com> wrote:
>> Regarding migration: do we migrate the NetClient->link_down field? If
>> we only migrate the status register value then the link may actually
>> be up at the net.c level.
>
>
> I tried to add 'MediaStatus' to 'struct RTL8139State', and update
> 'VMStateDescription vmstate_rtl8139', then the value of MediaStatus
> will be migrated.
>
> But the idea in v2 is better.

Migrating the NIC's media status is not enough.  Above I asked about
migrating nc->link_down, which determines whether net.c delivers
packets or drops them.

Your patch migrates the NIC's media status but I believe nc->link_down
isn't being migrated and the guest will therefore receive packets from
the host!  This could lead to unexpected results since the guest
thinks the link is down.

It's not a bug in your patch, but a larger issue that needs to be
addressed for all NICs that support migration.  (Unless I missed the
code that will migrate link_down.)

Stefan

Re: [Qemu-devel] [PATCH v3] rtl8139: implement 8139cp link status

[Paolo Bonzini]

Il 14/09/2012 04:16, Amos Kong ha scritto:
> +            /* The LinkDown bit of MediaStatus is inverse with link status */
> +            ret = 0xd0 | (s->nic->nc.link_down ? MSR_LinkDown : 0);
>              DPRINTF("MediaStatus read 0x%x\n", ret);
>              break;
>  
> @@ -3453,12 +3466,27 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
>      qemu_del_net_client(&s->nic->nc);
>  }
>  
> +static void rtl8139_set_link_status(NetClientState *nc)
> +{
> +    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
> +
> +    if (nc->link_down) {
> +        s->BasicModeStatus &= ~0x0004;
> +    } else {
> +        s->BasicModeStatus |= 0x0004;
> +    }
> +
> +    s->IntrStatus |= RxUnderrun;
> +    rtl8139_update_irq(s);
> +}
> +

Actually, this is worse than v2 because then one bit is migrated and the
other is not.

I think v2 is correct and, on top of it, you have to check in post_load
whether nc->link_down matches the loaded BMSR value.  If not, you need
to either set the link status in NetClientState, or generate an
RxUnderrun interrupt.

An alternative is to add a get_link_status callback and call it after
migration for all NIC NetClientStates.

Paolo

Re: [Qemu-devel] [PATCH v2] rtl8139: implement 8139cp link status

[Jason Wang]

On 09/14/2012 03:30 PM, Stefan Hajnoczi wrote:
> On Fri, Sep 14, 2012 at 2:34 AM, Amos Kong<akong@redhat.com>  wrote:
>> On 13/09/12 20:29, Stefan Hajnoczi wrote:
>>> On Thu, Sep 13, 2012 at 9:51 AM, Amos Kong<akong@redhat.com>  wrote:
>>> Regarding migration: do we migrate the NetClient->link_down field? If
>>> we only migrate the status register value then the link may actually
>>> be up at the net.c level.
>>
>> I tried to add 'MediaStatus' to 'struct RTL8139State', and update
>> 'VMStateDescription vmstate_rtl8139', then the value of MediaStatus
>> will be migrated.
>>
>> But the idea in v2 is better.
> Migrating the NIC's media status is not enough.  Above I asked about
> migrating nc->link_down, which determines whether net.c delivers
> packets or drops them.
>
> Your patch migrates the NIC's media status but I believe nc->link_down
> isn't being migrated and the guest will therefore receive packets from
> the host!  This could lead to unexpected results since the guest
> thinks the link is down.
>
> It's not a bug in your patch, but a larger issue that needs to be
> addressed for all NICs that support migration.  (Unless I missed the
> code that will migrate link_down.)
>
> Stefan

A possible solution is to infer the nc->link_down from the media status 
register in the destination. It works without adding more codes net.c 
but need model specific callback functions.

Re: [Qemu-devel] [PATCH v3] rtl8139: implement 8139cp link status

[Amos Kong]

On 14/09/12 16:36, Paolo Bonzini wrote:
> Il 14/09/2012 04:16, Amos Kong ha scritto:
>> +            /* The LinkDown bit of MediaStatus is inverse with link status */
>> +            ret = 0xd0 | (s->nic->nc.link_down ? MSR_LinkDown : 0);
>>               DPRINTF("MediaStatus read 0x%x\n", ret);
>>               break;
>>
>> @@ -3453,12 +3466,27 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
>>       qemu_del_net_client(&s->nic->nc);
>>   }
>>
>> +static void rtl8139_set_link_status(NetClientState *nc)
>> +{
>> +    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
>> +
>> +    if (nc->link_down) {
>> +        s->BasicModeStatus &= ~0x0004;
>> +    } else {
>> +        s->BasicModeStatus |= 0x0004;
>> +    }
>> +
>> +    s->IntrStatus |= RxUnderrun;
>> +    rtl8139_update_irq(s);
>> +}
>> +
>
> Actually, this is worse than v2 because then one bit is migrated and the
> other is not.

> I think v2 is correct and, on top of it, you have to check in post_load
> whether nc->link_down matches the loaded BMSR value.  If not, you need
> to either set the link status in NetClientState, or generate an
> RxUnderrun interrupt.


If correct link_down in rtl8139_post_load(), both v2 and v3 will work.

   s->nic->nc.link_down = (s->BasicModeStatus & 0x04) == 0;

s->BasicModeStatus is really migrated, s->nic->nc.link_down is inferred.
so I will continually work on v2.


> An alternative is to add a get_link_status callback and call it after
> migration for all NIC NetClientStates.
>
> Paolo

Thanks.

-- 
			Amos.

[Qemu-devel] [PATCH v4 0/3] net: fix link status

[Amos Kong]

First patch implemeted link status setting of rtl8139, the rest patches
updated nc.link_down in post_load() of migration to keep it coincident
with real link status.

Amos Kong (2):
  e1000: update nc.link_down in e1000_post_load()
  update nc.link_down in virtio_net_load()

Jason Wang (1):
  rtl8139: implement 8139cp link status

 hw/e1000.c      |   11 +++++++++++
 hw/rtl8139.c    |   23 +++++++++++++++++++++--
 hw/virtio-net.c |    4 ++++
 3 files changed, 36 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH v4 1/3] rtl8139: implement 8139cp link status

[Amos Kong]

From: Jason Wang <jasowang@redhat.com>

Add a link status chang callback and change the link status bit in BMSR
& MSR accordingly. Tested in Linux/Windows guests.

The link status bit of MediaStatus is infered from BasicModeStatus,
they are inverse.

nc.link_down could not be migrated, this patch updates link_down in
rtl8139_post_load() to keep it coincident with real link status.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>

---
v2: don't add MediaState in RTL8139State to avoid trouble
v3: adding an enum MediaStatusBits
v4: correct nc.link_down in the end of migration
---
 hw/rtl8139.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 844f1b8..72c052e 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -167,7 +167,7 @@ enum IntrStatusBits {
     PCIErr = 0x8000,
     PCSTimeout = 0x4000,
     RxFIFOOver = 0x40,
-    RxUnderrun = 0x20,
+    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
     RxOverflow = 0x10,
     TxErr = 0x08,
     TxOK = 0x04,
@@ -3007,7 +3007,8 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
             break;
 
         case MediaStatus:
-            ret = 0xd0;
+            /* The LinkDown bit of MediaStatus is inverse with link status */
+            ret = 0xd0 | ~(s->BasicModeStatus & 0x04);
             DPRINTF("MediaStatus read 0x%x\n", ret);
             break;
 
@@ -3262,6 +3263,9 @@ static int rtl8139_post_load(void *opaque, int version_id)
         s->cplus_enabled = s->CpCmd != 0;
     }
 
+    /* infer link_down according to link status bit in BasicModeStatus */
+    s->nic->nc.link_down = (s->BasicModeStatus & 0x04) == 0;
+
     return 0;
 }
 
@@ -3453,12 +3457,27 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
     qemu_del_net_client(&s->nic->nc);
 }
 
+static void rtl8139_set_link_status(NetClientState *nc)
+{
+    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+
+    if (nc->link_down) {
+        s->BasicModeStatus &= ~0x04;
+    } else {
+        s->BasicModeStatus |= 0x04;
+    }
+
+    s->IntrStatus |= RxUnderrun;
+    rtl8139_update_irq(s);
+}
+
 static NetClientInfo net_rtl8139_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
     .can_receive = rtl8139_can_receive,
     .receive = rtl8139_receive,
     .cleanup = rtl8139_cleanup,
+    .link_status_changed = rtl8139_set_link_status,
 };
 
 static int pci_rtl8139_init(PCIDevice *dev)
-- 
1.7.1

[Qemu-devel] [PATCH v4 2/3] e1000: update nc.link_down in e1000_post_load()

[Amos Kong]

This patch introduced e1000_post_load(), it will be called in the end of
migration. nc.link_down could not be migrated, this patch updates
link_down in e1000_post_load() to keep it coincident with real link
status.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/e1000.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index ae8a6c5..2f05b7d 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1075,11 +1075,22 @@ static bool is_version_1(void *opaque, int version_id)
     return version_id == 1;
 }
 
+static int e1000_post_load(void *opaque, int version_id)
+{
+    E1000State *s = opaque;
+
+    /* infer link_down according to link status bit in mac_reg[STATUS] */
+    s->nic->nc.link_down = (s->mac_reg[STATUS] & E1000_STATUS_LU) == 0;
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_e1000 = {
     .name = "e1000",
     .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .post_load = e1000_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_PCI_DEVICE(dev, E1000State),
         VMSTATE_UNUSED_TEST(is_version_1, 4), /* was instance id */
-- 
1.7.1

[Qemu-devel] [PATCH v4 3/3] update nc.link_down in virtio_net_load()

[Amos Kong]

nc.link_down could not be migrated, this patch updates link_down in
virtio_post_load() to keep it coincident with real link status.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/virtio-net.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index b1998b2..e637619 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -977,6 +977,10 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
     n->mac_table.first_multi = i;
+
+    /* infer link_down according to link status bit in n->status */
+    n->nic->nc.link_down = (n->status & VIRTIO_NET_S_LINK_UP) == 0;
+
     return 0;
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH v4 0/3] net: fix link status

[Jason Wang]

On 09/17/2012 10:25 AM, Amos Kong wrote:
> First patch implemeted link status setting of rtl8139, the rest patches
> updated nc.link_down in post_load() of migration to keep it coincident
> with real link status.
>
> Amos Kong (2):
>    e1000: update nc.link_down in e1000_post_load()
>    update nc.link_down in virtio_net_load()
>
> Jason Wang (1):
>    rtl8139: implement 8139cp link status
>
>   hw/e1000.c      |   11 +++++++++++
>   hw/rtl8139.c    |   23 +++++++++++++++++++++--
>   hw/virtio-net.c |    4 ++++
>   3 files changed, 36 insertions(+), 2 deletions(-)
>
Reviewed-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH v4 1/3] rtl8139: implement 8139cp link status

[Stefan Hajnoczi]

On Mon, Sep 17, 2012 at 3:25 AM, Amos Kong <akong@redhat.com> wrote:
> From: Jason Wang <jasowang@redhat.com>
>
> Add a link status chang callback and change the link status bit in BMSR
> & MSR accordingly. Tested in Linux/Windows guests.
>
> The link status bit of MediaStatus is infered from BasicModeStatus,
> they are inverse.
>
> nc.link_down could not be migrated, this patch updates link_down in
> rtl8139_post_load() to keep it coincident with real link status.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
>
> ---
> v2: don't add MediaState in RTL8139State to avoid trouble
> v3: adding an enum MediaStatusBits
> v4: correct nc.link_down in the end of migration
> ---
>  hw/rtl8139.c |   23 +++++++++++++++++++++--
>  1 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 844f1b8..72c052e 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -167,7 +167,7 @@ enum IntrStatusBits {
>      PCIErr = 0x8000,
>      PCSTimeout = 0x4000,
>      RxFIFOOver = 0x40,
> -    RxUnderrun = 0x20,
> +    RxUnderrun = 0x20, /* Packet Underrun / Link Change */
>      RxOverflow = 0x10,
>      TxErr = 0x08,
>      TxOK = 0x04,
> @@ -3007,7 +3007,8 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
>              break;
>
>          case MediaStatus:
> -            ret = 0xd0;
> +            /* The LinkDown bit of MediaStatus is inverse with link status */
> +            ret = 0xd0 | ~(s->BasicModeStatus & 0x04);

We only want to OR negated bit 0x4, not all the other bits:
ret = 0xd0 | (~s->BasicModeStatus & 0x04);

>              DPRINTF("MediaStatus read 0x%x\n", ret);
>              break;
>
> @@ -3262,6 +3263,9 @@ static int rtl8139_post_load(void *opaque, int version_id)
>          s->cplus_enabled = s->CpCmd != 0;
>      }
>
> +    /* infer link_down according to link status bit in BasicModeStatus */
> +    s->nic->nc.link_down = (s->BasicModeStatus & 0x04) == 0;
> +
>      return 0;
>  }
>
> @@ -3453,12 +3457,27 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
>      qemu_del_net_client(&s->nic->nc);
>  }
>
> +static void rtl8139_set_link_status(NetClientState *nc)
> +{
> +    RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
> +
> +    if (nc->link_down) {
> +        s->BasicModeStatus &= ~0x04;
> +    } else {
> +        s->BasicModeStatus |= 0x04;
> +    }
> +
> +    s->IntrStatus |= RxUnderrun;
> +    rtl8139_update_irq(s);
> +}
> +
>  static NetClientInfo net_rtl8139_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_NIC,
>      .size = sizeof(NICState),
>      .can_receive = rtl8139_can_receive,
>      .receive = rtl8139_receive,
>      .cleanup = rtl8139_cleanup,
> +    .link_status_changed = rtl8139_set_link_status,
>  };
>
>  static int pci_rtl8139_init(PCIDevice *dev)
> --
> 1.7.1
>
>