On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
> On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
>> <peter.crosthwa...@xilinx.com> wrote:
>> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <b.galv...@gmail.com>
>> > wrote:
>> >> + } else {
>> >> + mii->bmsr &= ~MII_BMSR_LINK_ST;
>> >> + mii->anlpar = MII_ANAR_TX;
>> >> + }
>> >> + mii->link_ok = link_ok;
>> >> +}
>> >> +
>> >> +static void mii_reset(AwEmacMii *mii)
>> >> +{
>> >> + mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
>> >> + mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
>> >> + MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
>> >> + mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
>> >> MII_ANAR_10 |
>> >> + MII_ANAR_CSMACD;
>> >> + mii->anlpar = MII_ANAR_TX;
>> >> + mii_set_link(mii, mii->link_ok);
>> >
>> > If this is actually needed, it smells like a bug in the net layer. Is
>> > the net layer not doing it's resets properly? It's quite odd that you
>> > are preserving old state across a reset.
>
> The net layer doesn't have a reset point for the link status. The net
> client must explicitly transition states itself.
>
>> >> +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
>> >> + size_t size)
>> >> +{
>> >> + AwEmacState *s = qemu_get_nic_opaque(nc);
>> >> + AwEmacFifo *fifo;
>> >> + uint32_t crc, *word;
>> >> + uint8_t *dest;
>> >> +
>> >> + if (s->num_rx >= NUM_RX_FIFOS) {
>> >
>> > Seems inconsistent that you check for fifo vacancy but you dont check
>> > for s->ctl & EMAC_CTL_RX_EN (as above in can_recieve). Then again,
>> > both conditions should be guarded by can_recieve, so I wonder whether
>> > you can just drop this.
>> >
>> > Stefan, if you are reading, can the recieve function rely on
>> > can_recieve success and drop such checks?
>
> It is safer to perform full checks in .receive() since
> qemu_net_queue_flush() calls .receive() repeatedly without
> .can_receive(). This means especially if the state can change between
> calls, you need to do a full check.
>
>> >> +static const VMStateDescription vmstate_mii = {
>> >> + .name = "allwinner_emac_mii",
>> >> + .version_id = 1,
>> >> + .minimum_version_id = 1,
>> >> + .fields = (VMStateField[]) {
>> >> + VMSTATE_UINT16(bmcr, AwEmacMii),
>> >> + VMSTATE_UINT16(bmsr, AwEmacMii),
>> >> + VMSTATE_UINT16(anar, AwEmacMii),
>> >> + VMSTATE_UINT16(anlpar, AwEmacMii),
>> >> + VMSTATE_BOOL(link_ok, AwEmacMii),
>> >
>> > The net layer should probably properly set link_ok on realize, so I
>> > doubt it's migratable state.
>
> The net layer is not part of live migration. It's up to the emulated
> device to migrate link state and restore it after migration. In other
> words, link state should be migrated as part of PHY vmstate.
>
But is the saved link state meaningful? The fact that net is not
migrating means that this link_ok migrated variable is potentially
incoherent with the net layer. E.G. What happens in the following
case:
Run QEMU
Link is Good
Save
Run Again
Link is bad
Load
The loaded emulation will have link_ok due to the vmsd load despite
the link being down in the new net layer environment. It will not
correct until net-layer activity causes a ->link_state_changed
callback.
Can this be just solved cleanly by calling the mii_set_link fn (added
in this patch) always on post load and removing this link_ok state?
This will mean that guest will see a link state transition immediately
on load if the link state changes from the saved-machine state to the
loaded-machine state.
Regards,
Peter
