Re: [Qemu-devel] [PATCH v3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 30.06.2011 16:16, schrieb Stefano Stabellini:
> On Thu, 30 Jun 2011, Kevin Wolf wrote:
>>> +static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>>> +{
>>> +    PCIDevice *pci_dev;
>>> +    PCIIDEState *pci_ide;
>>> +    DriveInfo *di;
>>> +    int i = 0;
>>> +
>>> +    pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
>>> +    pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev);
>>> +
>>> +    for (; i < 3; i++) {
>>> +        di = drive_get_by_index(IF_IDE, i);
>>> +        if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) {
>>> +            DeviceState *ds = bdrv_get_attached(di->bdrv);
>>> +            if (ds) {
>>> +                bdrv_detach(di->bdrv, ds);
>>> +            }
>>> +            bdrv_close(di->bdrv);
>>> +            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>>
>> Have you tested if this is enough if the guest tries to continue using
>> the device? I don't know of any case where it's not sufficient, just
>> trying to make sure that it's really true in practice.
> 
> The purpose of this is to "hide" the disk from the guest. The unplug is
> supposed to happen *before* the guest enumerates the IDE disks; it is
> responsibility of the guest to make sure of it.
> I tested it with Linux PV on HVM drivers, and Linux doesn't see the
> emulated disk after the unplug, as it should be.

Yeah. What I meant is that we should make sure that a misbehaving guest,
which just keeps on playing with the IDE ports anyway, can't crash qemu.
A quick review suggests that it is the case, but testing it anyway would
be better.

>>> +            drive_put_ref(di);
>>> +        }
>>> +    }
>>> +    qdev_reset_all(&(pci_ide->dev.qdev));
>>> +    return 0;
>>> +}
>>> +
>>> +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
>>> devfn)
>>> +{
>>> +    PCIDevice *dev;
>>> +
>>> +    dev = pci_create_simple(bus, devfn, "piix3-ide-xen");
>>> +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;
>>
>> Can't this be moved into the PCIDeviceInfo now that we have a separate
>> one for Xen?
>  
> No because it would be overridden by the default pci unplug function,
> that is not what we want in this case.

Okay. I'm not familiar with that code, so I'll just trust you there.

Kevin