[RFC] e1000: defer packets until BM enabled

Some guests seem to set BM for e1000 after
enabling RX.
If packets arrive in the window, device is wedged.
Probably works by luck on real hardware, work around
this by making can_receive depend on BM.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/e1000.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Hi Michael,

I had to make some small changes to get this patch to build successfully,
see inline below:

On Monday, December 01, 2014 11:50am, Michael S. Tsirkin [mst@redhat.com] wrote:
> 
> Some guests seem to set BM for e1000 after
> enabling RX.
> If packets arrive in the window, device is wedged.
> Probably works by luck on real hardware, work around
> this by making can_receive depend on BM.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/e1000.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index e33a4da..34625ac 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -923,7 +923,9 @@ e1000_can_receive(NetClientState *nc)
>      E1000State *s = qemu_get_nic_opaque(nc);
> 
>      return (s->mac_reg[STATUS] & E1000_STATUS_LU) &&
> -        (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1);
> +        (s->mac_reg[RCTL] & E1000_RCTL_EN) &&
> +        (s->parent_obj.config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
> +        e1000_has_rxbufs(s, 1);
>  }
> 
>  static uint64_t rx_desc_base(E1000State *s)
> @@ -1529,6 +1531,20 @@ static NetClientInfo net_e1000_info = {
>      .link_status_changed = e1000_set_link_status,
>  };
> 
> +static void e1000_write_config(PCIDevice *pci_dev, uint32_t address,
> +                                uint32_t val, int len)
> +{
> +    E1000State *d = E1000(dev);

s/dev/pci_dev/

> +
> +    pci_default_write_config(pci_dev, address, val, len);
> +
> +    if (range_covers_byte(address, len, PCI_COMMAND) &&

requires #include "qemu/range.h" at the top of e1000.c

> +        (pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> +        qemu_flush_queued_packets(qemu_get_queue(s->nic));

s/s->nic/d->nic/

> +    }
> +}
> +
> +
>  static int pci_e1000_init(PCIDevice *pci_dev)
>  {
>      DeviceState *dev = DEVICE(pci_dev);
> @@ -1539,6 +1555,8 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>      int i;
>      uint8_t *macaddr;
> 
> +    pci_dev->config_write = e1000_write_config;
> +
>      pci_conf = pci_dev->config;
> 
>      /* TODO: RST# value should be 0, PCI spec 6.2.4 */
> --

With this, I can confirm everything still works fine on both Mavericks and
F21-beta-live. So:

Tested-by: Gabriel Somlo <somlo@cmu.edu>

Regards,
--Gabriel

On Mon, Dec 01, 2014 at 06:01:18PM +0000, Gabriel Somlo wrote:
> Hi Michael,
> 
> I had to make some small changes to get this patch to build successfully,
> see inline below:

Ouch, looks like I sent out a stale version:
git commit
build+edit
git format-patch (without git commit)

Happens to me now and again :(

Sorry.

> On Monday, December 01, 2014 11:50am, Michael S. Tsirkin [mst@redhat.com] wrote:
> > 
> > Some guests seem to set BM for e1000 after
> > enabling RX.
> > If packets arrive in the window, device is wedged.
> > Probably works by luck on real hardware, work around
> > this by making can_receive depend on BM.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/net/e1000.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index e33a4da..34625ac 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -923,7 +923,9 @@ e1000_can_receive(NetClientState *nc)
> >      E1000State *s = qemu_get_nic_opaque(nc);
> > 
> >      return (s->mac_reg[STATUS] & E1000_STATUS_LU) &&
> > -        (s->mac_reg[RCTL] & E1000_RCTL_EN) && e1000_has_rxbufs(s, 1);
> > +        (s->mac_reg[RCTL] & E1000_RCTL_EN) &&
> > +        (s->parent_obj.config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
> > +        e1000_has_rxbufs(s, 1);
> >  }
> > 
> >  static uint64_t rx_desc_base(E1000State *s)
> > @@ -1529,6 +1531,20 @@ static NetClientInfo net_e1000_info = {
> >      .link_status_changed = e1000_set_link_status,
> >  };
> > 
> > +static void e1000_write_config(PCIDevice *pci_dev, uint32_t address,
> > +                                uint32_t val, int len)
> > +{
> > +    E1000State *d = E1000(dev);
> 
> s/dev/pci_dev/
> 
> > +
> > +    pci_default_write_config(pci_dev, address, val, len);
> > +
> > +    if (range_covers_byte(address, len, PCI_COMMAND) &&
> 
> requires #include "qemu/range.h" at the top of e1000.c
> 
> > +        (pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > +        qemu_flush_queued_packets(qemu_get_queue(s->nic));
> 
> s/s->nic/d->nic/
> 
> > +    }
> > +}
> > +
> > +
> >  static int pci_e1000_init(PCIDevice *pci_dev)
> >  {
> >      DeviceState *dev = DEVICE(pci_dev);
> > @@ -1539,6 +1555,8 @@ static int pci_e1000_init(PCIDevice *pci_dev)
> >      int i;
> >      uint8_t *macaddr;
> > 
> > +    pci_dev->config_write = e1000_write_config;
> > +
> >      pci_conf = pci_dev->config;
> > 
> >      /* TODO: RST# value should be 0, PCI spec 6.2.4 */
> > --
> 
> With this, I can confirm everything still works fine on both Mavericks and
> F21-beta-live. So:
> 
> Tested-by: Gabriel Somlo <somlo@cmu.edu>
> 
> Regards,
> --Gabriel

Thanks!