Bug 698732

Created attachment 493909 [details]
#1 Backport xen-emul-unplug support from upstream

Created attachment 493910 [details]
#2 add XEN defines to pci IDs

Created attachment 493911 [details]
#3 skip xen-vnif registration if 8139 registered with same MAC address

Created attachment 493912 [details]
#4 netfront: add xennet to module alias

Happily to see this feature get into rhel5.

sanity testing hints from Don:

(a) PV guests are not affected
(b) ide=disable doesn't affect the above xen pv connections
(c) attach/dettach of blk & net devices still works
(d) device assignment/pci-passthrough still works
(e) mutiple ide devices assigned to a guest get multiple xvd's connected to those block backends
(f) HVM guests that were re-built & configured to use pv-on-hvm still work
    (1) guest xml's spec xvda & vnif for block & net
    (2) guest has ide=disable set
    (3) (new) initrd (due to kernel update) still works
      i.e., regression testing for existing xen hvm guests using pv-on-hvm

What about the following case:

Suppose the sysadmin originally configured two disk devices for an HVM domain:
- "tap:aio:/image/file,xvda,w" as hard disk (PV-on-HVM driver for performance),
- "file:/another/image/file.iso,hdc:cdrom,r" as CD-ROM (paravirt CD-ROM is not
  supported)

1. The sysadmin doesn't change the VM config file

  1.1. #ifdef avoid_rhel5_regression (though this macro is not defined by the
       patch)
    - xen_unplug_emulated_devices() will unplug the CD-ROM, and set
      XEN_UNPLUG_ALL_IDE_DISKS in "xen_platform_pci_unplug"
    - blkfront will refuse to drive the CD-ROM
    - the guest loses the CD-ROM

  1.2. #ifndef avoid_rhel5_regression (currently)
    - xen_unplug_emulated_devices() will unplug the CD-ROM
    - blkfront will try to drive th CD-ROM and probably break
    - the guest loses the CD-ROM in addition

2. The sysadmin decides to add "xen_emul_unplug=never" (either to keep his
   CD-ROM, or just so)
  - xen_unplug_emulated_devices() will return early and leave
    "xen_platform_pci_unplug" initialized to 0
  - boot_wait_for_devices() won't call wait_for_devices()
  - the kernel might try to mount the root fs on /dev/xvda1 before blkfront
    initializes xvda fully.

Additionally, the added "xen_ide_unplug_unsupported" variable is never read.

I believe the patch needs further work.

xen_unplug_emulated_devices() will not unplug the CD-ROM:

    static void _ide_unplug_harddisks(int start)
    {
        IDEState *s;
        int i, j;
    
        if (!principal_ide_controller) {
            fprintf(stderr, "No principal controller?\n");
            return;
        }
        /* wait for outstanding aio requests */
        qemu_aio_flush();
        for (i = start; i < 4; i++) {
            s = principal_ide_controller->ide_if + i;
            if (!s->bs)
                continue; /* drive not present */
            if (s->is_cdrom)
                continue; /* cdrom */
            bdrv_close(s->bs);
            s->bs = NULL;
            ide_reset(s);
        }
    }

wrt c#8: 
Paolo,  I forgot about that skip-cdrom-unplug check, so you are correct.
So what I believe that means is that you can't connect to the cdrom via pv-block driver, but the cdrom can be connected via the emulated ide.

So, the patch set would be an improvement, since ide=disable would prevent cdrom use, where the xen_unplug=all would enable cdrom use if ide=disable is removed from the kernel cmdline.

Closing as wontfix for risks introduces by this patch.