| Message ID | 1347526299-27407-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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)