Xen HVM unplug changes

Backport from qemu-2.8:
Update unplug in Xen HVM guests to cover more cases. The patches are used since years in the SUSE xen-tools package.

Olaf

Olaf Hering (2):
xen_platform: unplug also SCSI disks
xen_platform: SUSE xenlinux unplug for emulated PCI

 hw/pci.c          | 41 +++++++++++++++++++++++++++++++++++++++++
 hw/xen_platform.c | 37 ++++++++++++++++++++++++++++++++++++-
 qemu-xen.h        |  1 +
 3 files changed, 78 insertions(+), 1 deletion(-)

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

From: Olaf Hering <[email protected]>

Using 'vdev=sd[a-o]' will create an emulated LSI controller, which can be used by the emulated BIOS to boot from disk. If the HVM domU has also PV driver the disk may appear twice in the guest. To avoid this an unplug of the emulated hardware is needed, similar to what is done for IDE and NIC drivers already.

Since the SCSI controller provides only disks the entire controller can be unplugged at once.

Impact of the change for classic and pvops based guest kernels:

vdev=sda:disk0
before: pvops: disk0=pv xvda + emulated sda
classic: disk0=pv sda + emulated sdq
after: pvops: disk0=pv xvda
classic: disk0=pv sda

vdev=hda:disk0, vdev=sda:disk1
before: pvops: disk0=pv xvda
disk1=emulated sda
classic: disk0=pv hda
disk1=pv sda + emulated sdq
after: pvops: disk0=pv xvda
disk1=not accessible by blkfront, index hda==index sda
classic: disk0=pv hda
disk1=pv sda

vdev=hda:disk0, vdev=sda:disk1, vdev=sdb:disk2
before: pvops: disk0=pv xvda
disk1=emulated sda
disk2=pv xvdb + emulated sdb
classic: disk0=pv hda
disk1=pv sda + emulated sdq
disk2=pv sdb + emulated sdr
after: pvops: disk0=pv xvda
disk1=not accessible by blkfront, index hda==index sda disk2=pv xvdb
classic: disk0=pv hda
disk1=pv sda
disk2=pv sda

Upstream commit 78f66897ddf58d1ffe5e0b95f7c1a1dad103a8da

---
 hw/pci.c          | 41 +++++++++++++++++++++++++++++++++++++++++
 hw/xen_platform.c |  4 +++-
 qemu-xen.h        |  1 +
 3 files changed, 45 insertions(+), 1 deletion(-)

index c423285..7d8e3b6 100644
@@ -871,6 +871,47 @@ void pci_unplug_netifs(void)
     }
 }
 
+void pci_unplug_scsi(void)
+{
+    PCIBus *bus;
+    PCIDevice *dev;
+    PCIIORegion *region;
+    int x;
+    int i;
+
+    /* We only support one PCI bus */
+    for (bus = first_bus; bus; bus = NULL) {
+       for (x = 0; x < 256; x++) {
+           dev = bus->devices[x];
+           if (dev &&
+               dev->config[0xa] == 0 &&
+               dev->config[0xb] == 1
+#ifdef CONFIG_PASSTHROUGH
+               && test_pci_devfn(x) != 1
+#endif
+               ) {
+               /* Found a scsi disk.  Remove it from the bus.  Note that
+                  we don't free it here, since there could still be
+                  references to it floating around.  There are only
+                  ever one or two structures leaked, and it's not
+                  worth finding them all. */
+               bus->devices[x] = NULL;
+               for (i = 0; i < PCI_NUM_REGIONS; i++) {
+                   region = &dev->io_regions[i];
+                   if (region->addr == (uint32_t)-1 ||
+                       region->size == 0)
+                       continue;
+                   if (region->type == PCI_ADDRESS_SPACE_IO) {
+                       isa_unassign_ioport(region->addr, region->size);
+                   } else if (region->type == PCI_ADDRESS_SPACE_MEM) {
+                       unregister_iomem(region->addr);
+                   }
+               }
+           }
+       }
+    }
+}
+
 typedef struct {
     PCIDevice dev;
     PCIBus *bus;

index d282f8e..0a94e0d 100644
@@ -154,8 +154,10 @@ static void platform_fixed_ioport_write2(void *opaque, uint32_t addr, uint32_t v
         /* Unplug devices.  Value is a bitmask of which devices to
            unplug, with bit 0 the IDE devices, bit 1 the network
            devices, and bit 2 the non-primary-master IDE devices. */
-        if (val & UNPLUG_ALL_IDE_DISKS)
+        if (val & UNPLUG_ALL_IDE_DISKS) {
             ide_unplug_harddisks();
+            pci_unplug_scsi();
+        }
         if (val & UNPLUG_ALL_NICS) {
             pci_unplug_netifs();
             net_tap_shutdown_all();

index 0598668..d315623 100644
@@ -47,6 +47,7 @@ void unset_vram_mapping(void *opaque);
 #endif
 
 void pci_unplug_netifs(void);
+void pci_unplug_scsi(void);
 void destroy_hvm_domain(void);
 void unregister_iomem(target_phys_addr_t start);
 

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Olaf Hering writes ("[PATCH qemu-xen-traditional 1/2] xen_platform: unplug also SCSI disks"):

qemu-xen-traditional is in the deep freeze. I'm only fixes for quite serious problems (such as security problems).

While I think this change has a risk of breaking things, and certainly wouldn't warrant backporting to earlier Xen releases, I think there is an arguable case for this patch. At the very least it only affects users with scsi disks.

That the patch has been in use in SUSE for a long time is also a helpful datapoint.

I am inclined to accept this patch but would welcome opinions from others. Olaf, if no-one replies, please ping me about this again, and I will apply it.

Olaf Hering writes ("[PATCH qemu-xen-traditional 2/2] xen_platform: SUSE xenlinux unplug for emulated PCI"):

On the other hand, I am very reluctant to apply this.

I don't see a good reason for SUSE to have a custom unplug protocol. Why can't your guests use the standard one ? Why haven't they been updated to use the standard one some time in the last ?5-10 years ?

We had a similar question about certain Citrix XenServer behaviours. I forget the details. But if I remember that was also a case where a downstream had invented a private variant of an upstream protocol, failed to coordinate with upstream, and simply shipped a revised protocol. Again there we resisted incorporation of ad-hoc protocols.

Sorry.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

The protocol was introduced before upstream had one, xen-3.0 vs. xen-3.x. I have not digged into 10 year old emails why it was done that way, if it was ever proposed for upstream inclusion. Meanwhile the upstream unplug is used since a two years. This patch would allow to run old SUSE domUs on new non-SUSE dom0s with qemu-trad.

Olaf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

We make efforts to allow guest OSes that use Hyper-V and VMWare interfaces run properly, this seems small in comparison.

Given that SuSE kernels are now using the new unplug protocol, I think there's no longer any reason to stand on principle. We take a practical approach for emulating other hypervisor interfaces (Hyper-V and VMWare); making accomodation for "one of our own" seems pretty reasonable. (I haven't reviewed the patch itself.)

-George

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

So what should be done with the two patches?
Are they acceptable for staging, or will they be rejected?

Olaf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Olaf Hering writes ("Re: [Xen-devel] [PATCH qemu-xen-traditional 1/2] xen_platform: unplug also SCSI disks [and 1 more messages]"):

I am happy with the first patch, as I say. Would you like me to apply it while we discuss the 2nd ?

As for the 2nd patch: the last new feature was added to qemu-xen in late 2011 ("qemu-xen: add vkbd support for PV on HVM guests" 9b33a3e5603e) or maybe early 2012 ("xen: introduce an event channel for buffered io event notifications" a5af89a7d40d).

You are asking for a new feature to be accepted in a codebase which has been on receiving only security fixes and bitrot fixes for many years.

AFAICT it seems unlikely that this new feature will add new security bugs, but might it break some old guests which mistakenly write to this ioport ?

I think if you can reassure me about this point, I may be persuaded to accept the patch.

Very sorry to be so awkward.

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Yes. Will think about the 2nd. Thanks.

Olaf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

From: Olaf Hering <[email protected]>

Implement SUSE specific unplug protocol for emulated PCI devices in PVonHVM guests. Its a simple 'outl(1, (ioaddr + 4));'. This protocol was implemented and used since Xen 3.0.4. It is used in all SUSE/SLES/openSUSE releases up to SLES11SP3 and openSUSE 12.3.
In addition old (pre-2011) VMDP versions are handled as well.

Upstream commit 35132016dc1c27de2b1354b161df6cc22f3ac5bf

---
 hw/xen_platform.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

index 0a94e0d..3c2fc0e 100644
@@ -337,9 +337,42 @@ static void xen_platform_ioport_writeb(void *opaque, uint32_t addr, uint32_t val
     }
 }
 
+static void suse_platform_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
+{
+    addr &= 0x0f;
+    switch (addr) {
+    case 4:
+        /*
+         * SUSE unplug for Xenlinux
+         * xen-kmp used this since xen-3.0.4, instead the official protocol
+         * from xen-3.3+ It did an unconditional "outl(1, (ioaddr + 4));"
+         * Pre VMDP 1.7 used 4 and 8 depending on how VMDP was configured.
+         * If VMDP was to control both disk and LAN it would use 4.
+         * If it controlled just disk or just LAN, it would use 8 below.
+         * */
+        ide_unplug_harddisks();
+        pci_unplug_scsi();
+        pci_unplug_netifs();
+        net_tap_shutdown_all();
+        break;
+    case 8:
+        if (val ==1 ) {
+            ide_unplug_harddisks();
+            pci_unplug_scsi();
+        } else if (val == 2) {
+            pci_unplug_netifs();
+            net_tap_shutdown_all();
+        }
+        break;
+    default:
+        break;
+    }
+}
+
 static void platform_ioport_map(PCIDevice *pci_dev, int region_num, uint32_t addr, uint32_t size, int type)
 {
     PCIXenPlatformState *d = (PCIXenPlatformState *)pci_dev;
+    register_ioport_write(addr, 16, 4, suse_platform_ioport_writel, d);
     register_ioport_write(addr, size, 1, xen_platform_ioport_writeb, d);
     register_ioport_read(addr, size, 1, xen_platform_ioport_readb, d);
 }

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Are these patches in the review queue, or did they get dropped?

Olaf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Olaf Hering writes ("Re: [Xen-devel] [PATCH qemu-xen-traditional 0/2] Xen HVM unplug changes"):

Sorry, I was away in November and don't always manage to catch up on all my email. In future do feel free to ping after a couple of weeks rather than a couple of months...

I'll reply to the emails now.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel