[Qemu-devel] [PATCH] e1000: Fix hotplug

[Qemu-devel] [PATCH] e1000: Fix hotplug

[Alex Williamson]

When we removed the call to e1000_reset() back in cset c1699988, we
left some register state uninitialized.  When we hotplug the device,
we don't go through a reset cycle, which means a hot added e1000 is
useless until the VM reboots.  Duplicate the bits we need from
e1000_reset().

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 0.13 candidate?

 hw/e1000.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 80b78bc..eb323d2 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1129,6 +1129,10 @@ static int pci_e1000_init(PCIDevice *pci_dev)
     checksum = (uint16_t) EEPROM_SUM - checksum;
     d->eeprom_data[EEPROM_CHECKSUM_REG] = checksum;
 
+    memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
+    memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init);
+    d->rxbuf_min_shift = 1;
+
     d->nic = qemu_new_nic(&net_e1000_info, &d->conf,
                           d->dev.qdev.info->name, d->dev.qdev.id, d);
 

[Qemu-devel] Re: [PATCH] e1000: Fix hotplug

[Alex Williamson]

rtl8139 has the same problem, except there's a much bigger pile of code
in rtl8139_reset()?  I think maybe we need to revisit this wholesale
remove of reset calls from init functions, unless I'm missing how
hotplug is supposed to work.  Thanks,

Alex

On Mon, 2010-08-02 at 15:15 -0600, Alex Williamson wrote:
> When we removed the call to e1000_reset() back in cset c1699988, we
> left some register state uninitialized.  When we hotplug the device,
> we don't go through a reset cycle, which means a hot added e1000 is
> useless until the VM reboots.  Duplicate the bits we need from
> e1000_reset().
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  0.13 candidate?
> 
>  hw/e1000.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 80b78bc..eb323d2 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -1129,6 +1129,10 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>      checksum = (uint16_t) EEPROM_SUM - checksum;
>      d->eeprom_data[EEPROM_CHECKSUM_REG] = checksum;
>  
> +    memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
> +    memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init);
> +    d->rxbuf_min_shift = 1;
> +
>      d->nic = qemu_new_nic(&net_e1000_info, &d->conf,
>                            d->dev.qdev.info->name, d->dev.qdev.id, d);
>  
> 

[Qemu-devel] Re: [PATCH] e1000: Fix hotplug

[Glauber Costa]

On Mon, Aug 02, 2010 at 03:15:17PM -0600, Alex Williamson wrote:
> When we hotplug the device,
> we don't go through a reset cycle, which means a hot added e1000 is
> useless until the VM reboots.  

I do guess, however, that this is true for any device, right?

Wouldn't it be better to just call the newly added reset function at
hotplug? One way to do that, would be to store a value indicated qemu
has already started. If you add a reset handler after that, the function
is called before being placed on the list.

[Qemu-devel] Re: [PATCH] e1000: Fix hotplug

[Alex Williamson]

On Tue, 2010-08-03 at 08:17 -0400, Glauber Costa wrote:
> On Mon, Aug 02, 2010 at 03:15:17PM -0600, Alex Williamson wrote:
> > When we hotplug the device,
> > we don't go through a reset cycle, which means a hot added e1000 is
> > useless until the VM reboots.  
> 
> I do guess, however, that this is true for any device, right?
> 
> Wouldn't it be better to just call the newly added reset function at
> hotplug? One way to do that, would be to store a value indicated qemu
> has already started. If you add a reset handler after that, the function
> is called before being placed on the list.

Yeah, that sounds like a better idea.  We can actually do it quite
easily from qdev_init using the hotplugged flag on the DeviceState.
Drop this e1000 specific one, I'll send a new patch in a minute.
Thanks,

Alex