[Qemu-devel] [PATCH 1.1 0/4] Xen: Fix PV-on-HVM

[Qemu-devel] [PATCH 1.1 0/4] Xen: Fix PV-on-HVM

[Anthony PERARD]

In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
unplug before the guest drivers are initialized. This mean that there must be
unplug without the consent of the guest.

Without this patch series, the guest end up with two nics with the same MAC,
the emulated nic and the PV nic.

I tried few other path before to submite these patches:
  - delayed the hot unplug in QEMU until the guest initialize the hotplug.
    => the guest unplug the nic only after the driver initialized it. That's a
      bit late.
  - delayed the call to unplug the emulated device until pci_acpi_init is called
    => this is worse, the pv disc does not show up and the guest does not boot.

In order to achive this fix, these patches introduce a new hotplug state only
used in acpi_piix4, and a new qdev callback force_unplug.

Would it be possible to have this fix in the next release?

Thanks,


Anthony PERARD (4):
  Introduce a new hotplug state: Force eject.
  qdev: Introduce qdev_force_unplug.
  pci: Add force_unplug callback.
  xen: Fix PV-on-HVM

 hw/acpi_piix4.c   |    5 +++++
 hw/pci.c          |   15 +++++++++++++--
 hw/pci.h          |    1 +
 hw/qdev.c         |   23 ++++++++++++++++++++---
 hw/qdev.h         |    3 +++
 hw/xen_platform.c |    2 +-
 6 files changed, 43 insertions(+), 6 deletions(-)

-- 
Anthony PERARD

[Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Anthony PERARD]

This hotplug state will be used to remove a device without the guest
cooperation.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/acpi_piix4.c |    5 +++++
 hw/pci.h        |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 585da4e..dfd5a9d 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -587,6 +587,11 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
         return 0;
     }
 
+    if (state == PCI_FORCE_EJECT) {
+        acpi_piix_eject_slot(s, 1 << slot);
+        return 0;
+    }
+
     if (state == PCI_HOTPLUG_ENABLED) {
         enable_device(s, slot);
     } else {
diff --git a/hw/pci.h b/hw/pci.h
index 8d0aa49..3b61e43 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -273,6 +273,7 @@ typedef enum {
     PCI_HOTPLUG_DISABLED,
     PCI_HOTPLUG_ENABLED,
     PCI_COLDPLUG_ENABLED,
+    PCI_FORCE_EJECT,
 } PCIHotplugState;
 
 typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
-- 
Anthony PERARD

[Qemu-devel] [PATCH 2/4] qdev: Introduce qdev_force_unplug.

[Anthony PERARD]

This function will be use to force a device to be ejected without the guest
cooperation.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/qdev.c |   23 ++++++++++++++++++++---
 hw/qdev.h |    3 +++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 6a8f6bd..c95d4c2 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -184,24 +184,41 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
-void qdev_unplug(DeviceState *dev, Error **errp)
+static void qdev_unplug_common(DeviceState *dev, Error **errp, bool force)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    qdev_event unplug;
 
     if (!dev->parent_bus->allow_hotplug) {
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
     }
-    assert(dc->unplug != NULL);
+
+    if (force) {
+        unplug = dc->force_unplug;
+    } else {
+        unplug = dc->unplug;
+    }
+    assert(unplug != NULL);
 
     qdev_hot_removed = true;
 
-    if (dc->unplug(dev) < 0) {
+    if (unplug(dev) < 0) {
         error_set(errp, QERR_UNDEFINED_ERROR);
         return;
     }
 }
 
+void qdev_force_unplug(DeviceState *dev, Error **errp)
+{
+    qdev_unplug_common(dev, errp, true);
+}
+
+void qdev_unplug(DeviceState *dev, Error **errp)
+{
+    qdev_unplug_common(dev, errp, false);
+}
+
 static int qdev_reset_one(DeviceState *dev, void *opaque)
 {
     device_reset(dev);
diff --git a/hw/qdev.h b/hw/qdev.h
index 4e90119..404c560 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -54,6 +54,7 @@ typedef struct DeviceClass {
     /* Private to qdev / bus.  */
     qdev_initfn init;
     qdev_event unplug;
+    qdev_event force_unplug;
     qdev_event exit;
     BusInfo *bus_info;
 } DeviceClass;
@@ -150,6 +151,8 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 void qdev_unplug(DeviceState *dev, Error **errp);
+/* Unplug a device without the guest cooperation. */
+void qdev_force_unplug(DeviceState *dev, Error **errp);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
-- 
Anthony PERARD

[Qemu-devel] [PATCH 3/4] pci: Add force_unplug callback.

[Anthony PERARD]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/pci.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index b706e69..c58bbc1 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1518,7 +1518,7 @@ static int pci_qdev_init(DeviceState *qdev)
     return 0;
 }
 
-static int pci_unplug_device(DeviceState *qdev)
+static int pci_unplug_device_common(DeviceState *qdev, PCIHotplugState state)
 {
     PCIDevice *dev = PCI_DEVICE(qdev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
@@ -1529,7 +1529,17 @@ static int pci_unplug_device(DeviceState *qdev)
     }
     object_unparent(OBJECT(dev));
     return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
-                             PCI_HOTPLUG_DISABLED);
+                             state);
+}
+
+static int pci_unplug_device(DeviceState *qdev)
+{
+    return pci_unplug_device_common(qdev, PCI_HOTPLUG_DISABLED);
+}
+
+static int pci_force_unplug_device(DeviceState *qdev)
+{
+    return pci_unplug_device_common(qdev, PCI_FORCE_EJECT);
 }
 
 PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
@@ -2000,6 +2010,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = pci_qdev_init;
     k->unplug = pci_unplug_device;
+    k->force_unplug = pci_force_unplug_device;
     k->exit = pci_unregister_device;
     k->bus_info = &pci_bus_info;
 }
-- 
Anthony PERARD

[Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Anthony PERARD]

In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
unplug before the guest drivers are initialized. This mean that there must be
unplug without the consent of the guest.

Without this patch, the guest end up with two nics with the same MAC, the
emulated nic and the PV nic.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/xen_platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index a9c52a6..2e47129 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -87,7 +87,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
 {
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_NETWORK_ETHERNET) {
-        qdev_unplug(&(d->qdev), NULL);
+        qdev_force_unplug(&(d->qdev), NULL);
     }
 }
 
-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 1.1 0/4] Xen: Fix PV-on-HVM

[Anthony Liguori]

On 05/15/2012 10:26 AM, Anthony PERARD wrote:
> In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
> unplug before the guest drivers are initialized. This mean that there must be
> unplug without the consent of the guest.

Stefano,

Can you do a PULL for the various 1.1 fixes for Xen?  Please try to get any -rc3 
pull requests in by Friday.

Thanks,

Anthony Liguori

>
> Without this patch series, the guest end up with two nics with the same MAC,
> the emulated nic and the PV nic.
>
> I tried few other path before to submite these patches:
>    - delayed the hot unplug in QEMU until the guest initialize the hotplug.
>      =>  the guest unplug the nic only after the driver initialized it. That's a
>        bit late.
>    - delayed the call to unplug the emulated device until pci_acpi_init is called
>      =>  this is worse, the pv disc does not show up and the guest does not boot.
>
> In order to achive this fix, these patches introduce a new hotplug state only
> used in acpi_piix4, and a new qdev callback force_unplug.
>
> Would it be possible to have this fix in the next release?
>
> Thanks,
>
>
> Anthony PERARD (4):
>    Introduce a new hotplug state: Force eject.
>    qdev: Introduce qdev_force_unplug.
>    pci: Add force_unplug callback.
>    xen: Fix PV-on-HVM
>
>   hw/acpi_piix4.c   |    5 +++++
>   hw/pci.c          |   15 +++++++++++++--
>   hw/pci.h          |    1 +
>   hw/qdev.c         |   23 ++++++++++++++++++++---
>   hw/qdev.h         |    3 +++
>   hw/xen_platform.c |    2 +-
>   6 files changed, 43 insertions(+), 6 deletions(-)
>

Re: [Qemu-devel] [PATCH 1.1 0/4] Xen: Fix PV-on-HVM

[Stefano Stabellini]

On Tue, 15 May 2012, Anthony Liguori wrote:
> On 05/15/2012 10:26 AM, Anthony PERARD wrote:
> > In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
> > unplug before the guest drivers are initialized. This mean that there must be
> > unplug without the consent of the guest.
> 
> Stefano,
> 
> Can you do a PULL for the various 1.1 fixes for Xen?  Please try to get any -rc3 
> pull requests in by Friday.

Sure, I am going to issue a PULL request for yesterday's patches today.

Regarding Anthony's new fixes, I'll send them out on Thursday to get
people a chance to read them if for you is OK.

Re: [Qemu-devel] [PATCH 2/4] qdev: Introduce qdev_force_unplug.

[Stefano Stabellini]

On Tue, 15 May 2012, Anthony PERARD wrote:
> This function will be use to force a device to be ejected without the guest
> cooperation.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/qdev.c |   23 ++++++++++++++++++++---
>  hw/qdev.h |    3 +++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6a8f6bd..c95d4c2 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -184,24 +184,41 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>  
> -void qdev_unplug(DeviceState *dev, Error **errp)
> +static void qdev_unplug_common(DeviceState *dev, Error **errp, bool force)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    qdev_event unplug;
>  
>      if (!dev->parent_bus->allow_hotplug) {
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;
>      }
> -    assert(dc->unplug != NULL);
> +
> +    if (force) {
> +        unplug = dc->force_unplug;
> +    } else {
> +        unplug = dc->unplug;
> +    }
> +    assert(unplug != NULL);

unplug needs to be initialized to NULL above

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Stefano Stabellini]

On Tue, 15 May 2012, Anthony PERARD wrote:
> This hotplug state will be used to remove a device without the guest
> cooperation.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  hw/acpi_piix4.c |    5 +++++
>  hw/pci.h        |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 585da4e..dfd5a9d 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -587,6 +587,11 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>          return 0;
>      }
>  
> +    if (state == PCI_FORCE_EJECT) {
> +        acpi_piix_eject_slot(s, 1 << slot);
> +        return 0;
> +    }
> +
>      if (state == PCI_HOTPLUG_ENABLED) {
>          enable_device(s, slot);
>      } else {
> diff --git a/hw/pci.h b/hw/pci.h
> index 8d0aa49..3b61e43 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -273,6 +273,7 @@ typedef enum {
>      PCI_HOTPLUG_DISABLED,
>      PCI_HOTPLUG_ENABLED,
>      PCI_COLDPLUG_ENABLED,
> +    PCI_FORCE_EJECT,
>  } PCIHotplugState;

Given that it is supposed to be a state rather than an action, it should
be called PCI_FORCE_DISABLED or something like that.

Re: [Qemu-devel] [PATCH 1.1 0/4] Xen: Fix PV-on-HVM

[Stefano Stabellini]

On Tue, 15 May 2012, Anthony PERARD wrote:
> In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
> unplug before the guest drivers are initialized. This mean that there must be
> unplug without the consent of the guest.
> 
> Without this patch series, the guest end up with two nics with the same MAC,
> the emulated nic and the PV nic.
> 
> I tried few other path before to submite these patches:
>   - delayed the hot unplug in QEMU until the guest initialize the hotplug.
>     => the guest unplug the nic only after the driver initialized it. That's a
>       bit late.
>   - delayed the call to unplug the emulated device until pci_acpi_init is called
>     => this is worse, the pv disc does not show up and the guest does not boot.
> 
> In order to achive this fix, these patches introduce a new hotplug state only
> used in acpi_piix4, and a new qdev callback force_unplug.
> 
> Would it be possible to have this fix in the next release?

I would be in favor for having this fix in 1.1.

Apart from the two comments I sent, I think that the patch series looks
reasonable.

Re: [Qemu-devel] [PATCH 2/4] qdev: Introduce qdev_force_unplug.

[Anthony PERARD]

On Tue, May 15, 2012 at 7:15 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 15 May 2012, Anthony PERARD wrote:
>> This function will be use to force a device to be ejected without the guest
>> cooperation.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  hw/qdev.c |   23 ++++++++++++++++++++---
>>  hw/qdev.h |    3 +++
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 6a8f6bd..c95d4c2 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -184,24 +184,41 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>      dev->alias_required_for_version = required_for_version;
>>  }
>>
>> -void qdev_unplug(DeviceState *dev, Error **errp)
>> +static void qdev_unplug_common(DeviceState *dev, Error **errp, bool force)
>>  {
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> +    qdev_event unplug;
>>
>>      if (!dev->parent_bus->allow_hotplug) {
>>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>>          return;
>>      }
>> -    assert(dc->unplug != NULL);
>> +
>> +    if (force) {
>> +        unplug = dc->force_unplug;
>> +    } else {
>> +        unplug = dc->unplug;
>> +    }
>> +    assert(unplug != NULL);
>
> unplug needs to be initialized to NULL above

Why? unplug is not used before to be set.

But I can do that for the next version if there is one.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Anthony PERARD]

On Tue, May 15, 2012 at 7:19 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>
> Given that it is supposed to be a state rather than an action, it should
> be called PCI_FORCE_DISABLED or something like that.

Ok, I will change to that.

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Michael S. Tsirkin]

On Tue, May 15, 2012 at 04:26:36PM +0100, Anthony PERARD wrote:
> This hotplug state will be used to remove a device without the guest
> cooperation.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

This can crash guest, can't it? If you are fine with crashing guest,
we already let you do this:
- delete device
- reset guest
no need for new flags.

> ---
>  hw/acpi_piix4.c |    5 +++++
>  hw/pci.h        |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 585da4e..dfd5a9d 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -587,6 +587,11 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>          return 0;
>      }
>  
> +    if (state == PCI_FORCE_EJECT) {
> +        acpi_piix_eject_slot(s, 1 << slot);
> +        return 0;
> +    }
> +
>      if (state == PCI_HOTPLUG_ENABLED) {
>          enable_device(s, slot);
>      } else {
> diff --git a/hw/pci.h b/hw/pci.h
> index 8d0aa49..3b61e43 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -273,6 +273,7 @@ typedef enum {
>      PCI_HOTPLUG_DISABLED,
>      PCI_HOTPLUG_ENABLED,
>      PCI_COLDPLUG_ENABLED,
> +    PCI_FORCE_EJECT,
>  } PCIHotplugState;
>  
>  typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> -- 
> Anthony PERARD

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Michael S. Tsirkin]

On Tue, May 15, 2012 at 04:26:39PM +0100, Anthony PERARD wrote:
> In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
> unplug before the guest drivers are initialized. This mean that there must be
> unplug without the consent of the guest.
> 
> Without this patch, the guest end up with two nics with the same MAC, the
> emulated nic and the PV nic.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

OK, so on Xen there are special devices that can be safely removed
without telling the guest?  Does there need to be regular hotplug for
these devices too? Or can it be always surprise removal?

> ---
>  hw/xen_platform.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index a9c52a6..2e47129 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -87,7 +87,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
>  {
>      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
>              PCI_CLASS_NETWORK_ETHERNET) {
> -        qdev_unplug(&(d->qdev), NULL);
> +        qdev_force_unplug(&(d->qdev), NULL);
>      }
>  }
>  
> -- 
> Anthony PERARD

Re: [Qemu-devel] [PATCH 1.1 0/4] Xen: Fix PV-on-HVM

[Michael S. Tsirkin]

On Tue, May 15, 2012 at 04:26:35PM +0100, Anthony PERARD wrote:
> In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
> unplug before the guest drivers are initialized.

Guest drivers for which device?

> This mean that there must be unplug without the consent of the guest.
> Without this patch series, the guest end up with two nics with the
> same MAC, the emulated nic and the PV nic.

Shouldn't be a problem if one of them has link down state.
So maybe just be careful not to bring PV link up until after emulated
nic goes away?

> I tried few other path before to submite these patches:
>   - delayed the hot unplug in QEMU until the guest initialize the hotplug.
>     => the guest unplug the nic only after the driver initialized it. That's a
>       bit late.
>   - delayed the call to unplug the emulated device until pci_acpi_init is called
>     => this is worse, the pv disc does not show up and the guest does not boot.
> 
> In order to achive this fix, these patches introduce a new hotplug state only
> used in acpi_piix4, and a new qdev callback force_unplug.
> 
> Would it be possible to have this fix in the next release?
> 
> Thanks,
> 
> 
> Anthony PERARD (4):
>   Introduce a new hotplug state: Force eject.
>   qdev: Introduce qdev_force_unplug.
>   pci: Add force_unplug callback.
>   xen: Fix PV-on-HVM
> 
>  hw/acpi_piix4.c   |    5 +++++
>  hw/pci.c          |   15 +++++++++++++--
>  hw/pci.h          |    1 +
>  hw/qdev.c         |   23 ++++++++++++++++++++---
>  hw/qdev.h         |    3 +++
>  hw/xen_platform.c |    2 +-
>  6 files changed, 43 insertions(+), 6 deletions(-)
> 
> -- 
> Anthony PERARD

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Paolo Bonzini]

Il 15/05/2012 23:00, Michael S. Tsirkin ha scritto:
> On Tue, May 15, 2012 at 04:26:39PM +0100, Anthony PERARD wrote:
>> In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
>> unplug before the guest drivers are initialized. This mean that there must be
>> unplug without the consent of the guest.
>>
>> Without this patch, the guest end up with two nics with the same MAC, the
>> emulated nic and the PV nic.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> OK, so on Xen there are special devices that can be safely removed
> without telling the guest?  Does there need to be regular hotplug for
> these devices too? Or can it be always surprise removal?

On Xen the PV drivers can ask the firmware to surprise-remove the
emulated NICs.  Of course it has to do it early enough so that the guest
doesn't crash.

Paolo

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Michael S. Tsirkin]

On Wed, May 16, 2012 at 10:06:25AM +0200, Paolo Bonzini wrote:
> Il 15/05/2012 23:00, Michael S. Tsirkin ha scritto:
> > On Tue, May 15, 2012 at 04:26:39PM +0100, Anthony PERARD wrote:
> >> In the context of PV-on-HVM under Xen, the emulated nics are supposed to be
> >> unplug before the guest drivers are initialized. This mean that there must be
> >> unplug without the consent of the guest.
> >>
> >> Without this patch, the guest end up with two nics with the same MAC, the
> >> emulated nic and the PV nic.
> >>
> >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > OK, so on Xen there are special devices that can be safely removed
> > without telling the guest?  Does there need to be regular hotplug for
> > these devices too? Or can it be always surprise removal?
> 
> On Xen the PV drivers can ask the firmware to surprise-remove the
> emulated NICs.

So driver tells firmware (meaning acpi? how?) that it's ok
to do surprize removal?

> Of course it has to do it early enough so that the guest
> doesn't crash.
> 
> Paolo

What does early enough mean and how do we ensure that?

-- 
MST

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Paolo Bonzini]

Il 16/05/2012 10:13, Michael S. Tsirkin ha scritto:
> On Wed, May 16, 2012 at 10:06:25AM +0200, Paolo Bonzini wrote:
>> On Xen the PV drivers can ask the firmware to surprise-remove the
>> emulated NICs.
> 
> So driver tells firmware (meaning acpi? how?) that it's ok
> to do surprize removal?

It writes something to some I/O port, and then QEMU surprise-removes the
NICs.

>> Of course it has to do it early enough so that the guest
>> doesn't crash.
> 
> What does early enough mean and how do we ensure that?

Early enough means that the I/O port is written very early in the boot
process, even before the PCI bus is scanned by the OS.

You don't ensure it, it's up to the OS.  The OS knows whether its
drivers can cope properly with surprise removal.  If they can, in
principle it could write the magic value whenever it wants to.

Paolo

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Stefano Stabellini]

On Wed, 16 May 2012, Paolo Bonzini wrote:
> Il 16/05/2012 10:13, Michael S. Tsirkin ha scritto:
> > On Wed, May 16, 2012 at 10:06:25AM +0200, Paolo Bonzini wrote:
> >> On Xen the PV drivers can ask the firmware to surprise-remove the
> >> emulated NICs.
> > 
> > So driver tells firmware (meaning acpi? how?) that it's ok
> > to do surprize removal?
> 
> It writes something to some I/O port, and then QEMU surprise-removes the
> NICs.

Yes, writing to a static I/O port provided by the Xen platform PCI
device, see hw/xen_platform.c:platform_fixed_ioport_writew.

The guest can ask to unplug emulated NICs and disks this way.
Surprise-removal is OK in these cases.


> >> Of course it has to do it early enough so that the guest
> >> doesn't crash.
> > 
> > What does early enough mean and how do we ensure that?
> 
> Early enough means that the I/O port is written very early in the boot
> process, even before the PCI bus is scanned by the OS.
> 
> You don't ensure it, it's up to the OS.  The OS knows whether its
> drivers can cope properly with surprise removal.  If they can, in
> principle it could write the magic value whenever it wants to.

Right, it is up to the OS, in general before the PCI bus is scanned.
In Linux we do it from hypervisor_x86->init_platform.

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Michael S. Tsirkin]

On Wed, May 16, 2012 at 11:19:53AM +0100, Stefano Stabellini wrote:
> On Wed, 16 May 2012, Paolo Bonzini wrote:
> > Il 16/05/2012 10:13, Michael S. Tsirkin ha scritto:
> > > On Wed, May 16, 2012 at 10:06:25AM +0200, Paolo Bonzini wrote:
> > >> On Xen the PV drivers can ask the firmware to surprise-remove the
> > >> emulated NICs.
> > > 
> > > So driver tells firmware (meaning acpi? how?) that it's ok
> > > to do surprize removal?
> > 
> > It writes something to some I/O port, and then QEMU surprise-removes the
> > NICs.
> 
> Yes, writing to a static I/O port provided by the Xen platform PCI
> device, see hw/xen_platform.c:platform_fixed_ioport_writew.
> 
> The guest can ask to unplug emulated NICs and disks this way.
> Surprise-removal is OK in these cases.

Confused.
Don't you want to just remove the device on unplug?
In fact the equivalent of guest calling _EJ0?

> > >> Of course it has to do it early enough so that the guest
> > >> doesn't crash.
> > > 
> > > What does early enough mean and how do we ensure that?
> > 
> > Early enough means that the I/O port is written very early in the boot
> > process, even before the PCI bus is scanned by the OS.
> > 
> > You don't ensure it, it's up to the OS.  The OS knows whether its
> > drivers can cope properly with surprise removal.  If they can, in
> > principle it could write the magic value whenever it wants to.
> 
> Right, it is up to the OS, in general before the PCI bus is scanned.
> In Linux we do it from hypervisor_x86->init_platform.

So early on boot you decide you want PV and so you unplug all emulated
devices?

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Stefano Stabellini]

On Tue, 15 May 2012, Michael S. Tsirkin wrote:
> On Tue, May 15, 2012 at 04:26:36PM +0100, Anthony PERARD wrote:
> > This hotplug state will be used to remove a device without the guest
> > cooperation.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> This can crash guest, can't it? If you are fine with crashing guest,
> we already let you do this:
> - delete device
> - reset guest
> no need for new flags.

Given that the guest is not going to crash (if it knows what it is
doing), we could just:


diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index a9c52a6..a1e1a33 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -88,6 +88,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_NETWORK_ETHERNET) {
         qdev_unplug(&(d->qdev), NULL);
+        qdev_free(&(d->qdev));
     }
 }
 

Anthony, can you confirm that this solves the problem for you?

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Stefano Stabellini]

On Wed, 16 May 2012, Michael S. Tsirkin wrote:
> On Wed, May 16, 2012 at 11:19:53AM +0100, Stefano Stabellini wrote:
> > On Wed, 16 May 2012, Paolo Bonzini wrote:
> > > Il 16/05/2012 10:13, Michael S. Tsirkin ha scritto:
> > > > On Wed, May 16, 2012 at 10:06:25AM +0200, Paolo Bonzini wrote:
> > > >> On Xen the PV drivers can ask the firmware to surprise-remove the
> > > >> emulated NICs.
> > > > 
> > > > So driver tells firmware (meaning acpi? how?) that it's ok
> > > > to do surprize removal?
> > > 
> > > It writes something to some I/O port, and then QEMU surprise-removes the
> > > NICs.
> > 
> > Yes, writing to a static I/O port provided by the Xen platform PCI
> > device, see hw/xen_platform.c:platform_fixed_ioport_writew.
> > 
> > The guest can ask to unplug emulated NICs and disks this way.
> > Surprise-removal is OK in these cases.
> 
> Confused.
> Don't you want to just remove the device on unplug?

Yes, the NIC needs to "disappear" from the PCI bus.


> In fact the equivalent of guest calling _EJ0?

Except that _EJ0 can or cannot be implemented, while this doesn't have
to go through ACPI or PCI hotplug and it is supposed to always work.


> > > >> Of course it has to do it early enough so that the guest
> > > >> doesn't crash.
> > > > 
> > > > What does early enough mean and how do we ensure that?
> > > 
> > > Early enough means that the I/O port is written very early in the boot
> > > process, even before the PCI bus is scanned by the OS.
> > > 
> > > You don't ensure it, it's up to the OS.  The OS knows whether its
> > > drivers can cope properly with surprise removal.  If they can, in
> > > principle it could write the magic value whenever it wants to.
> > 
> > Right, it is up to the OS, in general before the PCI bus is scanned.
> > In Linux we do it from hypervisor_x86->init_platform.
> 
> So early on boot you decide you want PV and so you unplug all emulated
> devices?

Yes, but only the emulated devices that can collide with PV devices:
disk and network.

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Anthony PERARD]

On Wed, May 16, 2012 at 11:32 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 15 May 2012, Michael S. Tsirkin wrote:
>> On Tue, May 15, 2012 at 04:26:36PM +0100, Anthony PERARD wrote:
>> > This hotplug state will be used to remove a device without the guest
>> > cooperation.
>> >
>> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>
>> This can crash guest, can't it? If you are fine with crashing guest,
>> we already let you do this:
>> - delete device
>> - reset guest
>> no need for new flags.
>
> Given that the guest is not going to crash (if it knows what it is
> doing), we could just:
>
>
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index a9c52a6..a1e1a33 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -88,6 +88,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
>     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
>             PCI_CLASS_NETWORK_ETHERNET) {
>         qdev_unplug(&(d->qdev), NULL);
> +        qdev_free(&(d->qdev));
>     }
>  }
>
>
> Anthony, can you confirm that this solves the problem for you?

This work until I try to hotplug a new device to the guest at wish
point I have this:
ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete:
assertion failed: (obj->ref == 0)

This is because there is still a pending request of the hotunplug in
the acpi piix4.
If I call qdev_free without qdev_unplug, I hit the same assert, but
rigth away. This is way something new.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Paolo Bonzini]

Il 16/05/2012 13:15, Anthony PERARD ha scritto:
>> >         qdev_unplug(&(d->qdev), NULL);
>> > +        qdev_free(&(d->qdev));
>> >     }
>> >  }
>> >
>> >
>> > Anthony, can you confirm that this solves the problem for you?
> This work until I try to hotplug a new device to the guest at wish
> point I have this:
> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete:
> assertion failed: (obj->ref == 0)
> 
> This is because there is still a pending request of the hotunplug in
> the acpi piix4.
> If I call qdev_free without qdev_unplug, I hit the same assert, but
> rigth away. This is way something new.

Because it's missing the object_unparent done by qdev_unplug.  Does
object_unparent+qdev_free work?  (I believe object_unparent should be
done by qdev_free rather than qdev_unplug, but that's something for 1.2).

Paolo

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Anthony PERARD]

On Wed, May 16, 2012 at 12:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 16/05/2012 13:15, Anthony PERARD ha scritto:
>>> >         qdev_unplug(&(d->qdev), NULL);
>>> > +        qdev_free(&(d->qdev));
>>> >     }
>>> >  }
>>> >
>>> >
>>> > Anthony, can you confirm that this solves the problem for you?
>> This work until I try to hotplug a new device to the guest at wish
>> point I have this:
>> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete:
>> assertion failed: (obj->ref == 0)
>>
>> This is because there is still a pending request of the hotunplug in
>> the acpi piix4.
>> If I call qdev_free without qdev_unplug, I hit the same assert, but
>> rigth away. This is way something new.
>
> Because it's missing the object_unparent done by qdev_unplug.  Does
> object_unparent+qdev_free work?  (I believe object_unparent should be
> done by qdev_free rather than qdev_unplug, but that's something for 1.2).

Cool, this seems to work fine. Thanks.

I'll test a bit more and resend a patch with only object_unparent+qdev_free.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Michael S. Tsirkin]

On Wed, May 16, 2012 at 12:37:54PM +0100, Anthony PERARD wrote:
> On Wed, May 16, 2012 at 12:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 16/05/2012 13:15, Anthony PERARD ha scritto:
> >>> >         qdev_unplug(&(d->qdev), NULL);
> >>> > +        qdev_free(&(d->qdev));
> >>> >     }
> >>> >  }
> >>> >
> >>> >
> >>> > Anthony, can you confirm that this solves the problem for you?
> >> This work until I try to hotplug a new device to the guest at wish
> >> point I have this:
> >> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete:
> >> assertion failed: (obj->ref == 0)
> >>
> >> This is because there is still a pending request of the hotunplug in
> >> the acpi piix4.
> >> If I call qdev_free without qdev_unplug, I hit the same assert, but
> >> rigth away. This is way something new.
> >
> > Because it's missing the object_unparent done by qdev_unplug.  Does
> > object_unparent+qdev_free work?  (I believe object_unparent should be
> > done by qdev_free rather than qdev_unplug, but that's something for 1.2).
> 
> Cool, this seems to work fine. Thanks.
> 
> I'll test a bit more and resend a patch with only object_unparent+qdev_free.

Separately it was suggested to make qdev_free do object_unparent
automatically. Anthony is yet to respond.

> -- 
> Anthony PERARD

Re: [Qemu-devel] [PATCH 4/4] xen: Fix PV-on-HVM

[Michael S. Tsirkin]

On Wed, May 16, 2012 at 11:37:02AM +0100, Stefano Stabellini wrote:
> On Wed, 16 May 2012, Michael S. Tsirkin wrote:
> > On Wed, May 16, 2012 at 11:19:53AM +0100, Stefano Stabellini wrote:
> > > On Wed, 16 May 2012, Paolo Bonzini wrote:
> > > > Il 16/05/2012 10:13, Michael S. Tsirkin ha scritto:
> > > > > On Wed, May 16, 2012 at 10:06:25AM +0200, Paolo Bonzini wrote:
> > > > >> On Xen the PV drivers can ask the firmware to surprise-remove the
> > > > >> emulated NICs.
> > > > > 
> > > > > So driver tells firmware (meaning acpi? how?) that it's ok
> > > > > to do surprize removal?
> > > > 
> > > > It writes something to some I/O port, and then QEMU surprise-removes the
> > > > NICs.
> > > 
> > > Yes, writing to a static I/O port provided by the Xen platform PCI
> > > device, see hw/xen_platform.c:platform_fixed_ioport_writew.
> > > 
> > > The guest can ask to unplug emulated NICs and disks this way.
> > > Surprise-removal is OK in these cases.
> > 
> > Confused.
> > Don't you want to just remove the device on unplug?
> 
> Yes, the NIC needs to "disappear" from the PCI bus.
> 
> 
> > In fact the equivalent of guest calling _EJ0?
> 
> Except that _EJ0 can or cannot be implemented, while this doesn't have
> to go through ACPI or PCI hotplug and it is supposed to always work.

So the answer is to simply free on unplug exactly
like _EJ0 does. There's no forcing and no surprise removal here.

-- 
MST

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Paolo Bonzini]

Il 16/05/2012 15:20, Michael S. Tsirkin ha scritto:
>>> Because it's missing the object_unparent done by qdev_unplug.  Does
>>> object_unparent+qdev_free work?  (I believe object_unparent should be
>>> done by qdev_free rather than qdev_unplug, but that's something for 1.2).
>>
>> Cool, this seems to work fine. Thanks.
>>
>> I'll test a bit more and resend a patch with only object_unparent+qdev_free.
> 
> Separately it was suggested to make qdev_free do object_unparent
> automatically. Anthony is yet to respond.

Yes, but for 1.1 this Xen-only patch would be nice.

Paolo

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Anthony Liguori]

On 05/16/2012 06:23 AM, Paolo Bonzini wrote:
> Il 16/05/2012 13:15, Anthony PERARD ha scritto:
>>>>          qdev_unplug(&(d->qdev), NULL);
>>>> +        qdev_free(&(d->qdev));
>>>>      }
>>>>   }
>>>>
>>>>
>>>> Anthony, can you confirm that this solves the problem for you?
>> This work until I try to hotplug a new device to the guest at wish
>> point I have this:
>> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete:
>> assertion failed: (obj->ref == 0)
>>
>> This is because there is still a pending request of the hotunplug in
>> the acpi piix4.
>> If I call qdev_free without qdev_unplug, I hit the same assert, but
>> rigth away. This is way something new.
>
> Because it's missing the object_unparent done by qdev_unplug.  Does
> object_unparent+qdev_free work?  (I believe object_unparent should be
> done by qdev_free rather than qdev_unplug, but that's something for 1.2).

qdev_free() is trivially object_delete today.

What we should do is make an object_destroy() which emits a destroy event and 
then decrements the reference count.  When ref == 0, we should emit a delete event.

We could then register a slot to object_unparent in the destroy event handler, 
and then object_new() could register a free handler in the delete event.

Then object_delete()/qdev_free() just become trivial invocations of object_unref().

But for 1.1, we definitely should just do an explicit object_unparent().

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Michael S. Tsirkin]

On Wed, May 16, 2012 at 11:02:30AM -0500, Anthony Liguori wrote:
> On 05/16/2012 06:23 AM, Paolo Bonzini wrote:
> >Il 16/05/2012 13:15, Anthony PERARD ha scritto:
> >>>>         qdev_unplug(&(d->qdev), NULL);
> >>>>+        qdev_free(&(d->qdev));
> >>>>     }
> >>>>  }
> >>>>
> >>>>
> >>>>Anthony, can you confirm that this solves the problem for you?
> >>This work until I try to hotplug a new device to the guest at wish
> >>point I have this:
> >>ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete:
> >>assertion failed: (obj->ref == 0)
> >>
> >>This is because there is still a pending request of the hotunplug in
> >>the acpi piix4.
> >>If I call qdev_free without qdev_unplug, I hit the same assert, but
> >>rigth away. This is way something new.
> >
> >Because it's missing the object_unparent done by qdev_unplug.  Does
> >object_unparent+qdev_free work?  (I believe object_unparent should be
> >done by qdev_free rather than qdev_unplug, but that's something for 1.2).
> 
> qdev_free() is trivially object_delete today.
> 
> What we should do is make an object_destroy() which emits a destroy
> event and then decrements the reference count.  When ref == 0, we
> should emit a delete event.
> 
> We could then register a slot to object_unparent in the destroy
> event handler, and then object_new() could register a free handler
> in the delete event.
> 
> Then object_delete()/qdev_free() just become trivial invocations of object_unref().
> 
> But for 1.1, we definitely should just do an explicit object_unparent().
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >Paolo

Okay. Can you fix the bug Amos reported this way too
to avoid confusion?  Or prefer Amos to do it?

-- 
MST

Re: [Qemu-devel] [PATCH 1/4] Introduce a new hotplug state: Force eject.

[Michael S. Tsirkin]

On Wed, May 16, 2012 at 05:52:17PM +0200, Paolo Bonzini wrote:
> Il 16/05/2012 15:20, Michael S. Tsirkin ha scritto:
> >>> Because it's missing the object_unparent done by qdev_unplug.  Does
> >>> object_unparent+qdev_free work?  (I believe object_unparent should be
> >>> done by qdev_free rather than qdev_unplug, but that's something for 1.2).
> >>
> >> Cool, this seems to work fine. Thanks.
> >>
> >> I'll test a bit more and resend a patch with only object_unparent+qdev_free.
> > 
> > Separately it was suggested to make qdev_free do object_unparent
> > automatically. Anthony is yet to respond.
> 
> Yes, but for 1.1 this Xen-only patch would be nice.
> 
> Paolo

No sure which this patch is meant, the force eject is not a good idea.
Freeing directly from Xen is fine.

-- 
MST