Re: [PATCH v1 20/23] xen platform: unplug ahci object

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Thu, 2023-06-22 at 05:40 +0000, Bernhard Beschow wrote:
> Am 20. Juni 2023 17:24:54 UTC schrieb Joel Upham <jupham125@xxxxxxxxx>:
> > This will unplug the ahci device when the Xen driver calls for an unplug.
> > This has been tested to work in linux and Windows guests.
> > When q35 is detected, we will remove the ahci controller
> > with the hard disks.  In the libxl config, cdrom devices
> > are put on a seperate ahci controller. This allows for 6 cdrom
> > devices to be added, and 6 qemu hard disks.
> 
> Does this also work with KVM Xen emulation? If so, the QEMU manual
> should be updated accordingly in this patch since it explicitly rules
> out Q35 due to missing AHCI unplug:
> https://gitlab.com/qemu-project/qemu/-/blob/stable-8.0/docs/system/i386/xen.rst?plain=1&ref_type=heads#L51

No, it doesn't work. Those assumptions about the topology and which
disk/cdrom devices are attached to which AHCI device on which PCI bus,
are not valid in the general case. So if I boot an HVM guest thus...

 $ qemu-system-x86_64 -M q35 -m 1g -display none -serial mon:stdio \
     --accel kvm,xen-version=0x40011,kernel-irqchip=split \
     -drive file=${GUEST_IMAGE},if=xen \
     -drive file=${GUEST_IMAGE},file.locking=off,if=ide

... it still sees the AHCI disk when it boots:

[root@localhost ~]# cat /proc/partitions 
major minor  #blocks  name

 202        0   20971520 xvda
 202        1   18874368 xvda1
 202        2    2096128 xvda2
   8        0   20971520 sda
   8        1   18874368 sda1
   8        2    2096128 sda2
  11        0    1048575 sr0
[root@localhost ~]# lspci
00:00.0 Host bridge: Intel Corporation 82G33/G31/P35/P31 Express DRAM Controller
00:01.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01)
00:02.0 VGA compatible controller: Device 1234:1111 (rev 02)
00:1f.0 ISA bridge: Intel Corporation 82801IB (ICH9) LPC Interface Controller 
(rev 02)
00:1f.2 SATA controller: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port 
SATA Controller [AHCI mode] (rev 02)
00:1f.3 SMBus: Intel Corporation 82801I (ICH9 Family) SMBus Controller (rev 02)

We probably do need to iterate over the children of the PCI device and
selectively destroy them. Which can be the *same* for IDE and AHCI.
Patch below.

It would be slightly more natural to do ide_dev_unplug() with an
'if (!idedev) return;' but I've done it this way to keep the
indentation the same, and thus highlight that it's just using the
existing blk unplug magic. I kind of hate that we *need* that magic,
shouldn't object_unparent() Just Work™ and unwire everything?
(It *doesn't*; qemu later crashes. But I think it *should*).

As I'm literally about to hit send on this, I realise I messed up the
'aux' logic. But as a proof of concept for this approach, this works OK
for both pc and q35 machines with Xen emulation tested as in the above
command line. Feel free to use it as you see fit, to which end:

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>

--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -169,37 +169,49 @@ static void pci_unplug_nics(PCIBus *bus)
  *
  * [1] 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
  */
-static void pci_xen_ide_unplug(PCIDevice *d, bool aux)
+static int ide_dev_unplug(DeviceState *dev, void *opaque)
 {
-    DeviceState *dev = DEVICE(d);
-    PCIIDEState *pci_ide;
-    int i;
     IDEDevice *idedev;
     IDEBus *idebus;
     BlockBackend *blk;
+    int unit;
 
-    pci_ide = PCI_IDE(dev);
-
-    for (i = aux ? 1 : 0; i < 4; i++) {
-        idebus = &pci_ide->bus[i / 2];
-        blk = idebus->ifs[i % 2].blk;
+    idedev = IDE_DEVICE(object_dynamic_cast(OBJECT(dev), "ide-hd"));
+    if (idedev) {
+        idebus = IDE_BUS(qdev_get_parent_bus(dev));
 
-        if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
-            if (!(i % 2)) {
-                idedev = idebus->master;
-            } else {
-                idedev = idebus->slave;
-            }
+        unit = (idedev == idebus->slave);
+        assert(unit || idedev == idebus->master);
 
+        blk = idebus->ifs[unit].blk;
+        if (blk) {
             blk_drain(blk);
             blk_flush(blk);
 
             blk_detach_dev(blk, DEVICE(idedev));
-            idebus->ifs[i % 2].blk = NULL;
+            idebus->ifs[unit].blk = NULL;
             idedev->conf.blk = NULL;
             monitor_remove_blk(blk);
             blk_unref(blk);
         }
+
+        object_unparent(OBJECT(dev));
+    }
+
+    return 0;
+}
+
+static void pci_xen_ide_unplug(PCIDevice *d, bool aux)
+{
+    DeviceState *dev = DEVICE(d);
+
+    if (!aux) {
+        IDEBus *idebus = IDE_BUS(qdev_get_child_bus(DEVICE(dev), "ide.0"));
+        if (idebus && idebus->master) {
+            ide_dev_unplug(DEVICE(idebus->master), NULL);
+        }
+    } else {
+        qdev_walk_children(dev, NULL, NULL, ide_dev_unplug, NULL, NULL);
     }
     pci_device_reset(d);
 }
@@ -216,6 +228,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void 
*opaque)
 
     switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
     case PCI_CLASS_STORAGE_IDE:
+    case PCI_CLASS_STORAGE_SATA:
         pci_xen_ide_unplug(d, aux);
         break;
 
-- 
2.34.1

Attachment: smime.p7s
Description: S/MIME cryptographic signature