| Message ID | 20161124201130.16558-2-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Olaf Hering writes ("[PATCH qemu-xen-traditional 1/2] xen_platform: unplug also SCSI disks"):
> From: Olaf Hering <ohering@suse.de>
>
> 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.
qemu-xen-traditional is in the deep freeze. I'm only fixes for quite
serious problems (such as security problems).
> Impact of the change for classic and pvops based guest kernels:
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"):
> From: Olaf Hering <ohering@suse.de>
>
> 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.
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.
On Mon, Jan 09, Ian Jackson wrote: > Olaf Hering writes ("[PATCH qemu-xen-traditional 2/2] xen_platform: SUSE xenlinux unplug for emulated PCI"): > > From: Olaf Hering <ohering@suse.de> > > > > 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. > > 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 ? 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
On Mon, Jan 9, 2017 at 4:39 PM, Olaf Hering <olaf@aepfle.de> wrote: > On Mon, Jan 09, Ian Jackson wrote: > >> Olaf Hering writes ("[PATCH qemu-xen-traditional 2/2] xen_platform: SUSE xenlinux unplug for emulated PCI"): >> > From: Olaf Hering <ohering@suse.de> >> > >> > 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. >> >> 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 ? > > 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. 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
On Tue, Jan 10, George Dunlap wrote: > On Mon, Jan 9, 2017 at 4:39 PM, Olaf Hering <olaf@aepfle.de> wrote: > > 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. > 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.) So what should be done with the two patches? Are they acceptable for staging, or will they be rejected? Olaf
Olaf Hering writes ("Re: [Xen-devel] [PATCH qemu-xen-traditional 1/2] xen_platform: unplug also SCSI disks [and 1 more messages]"):
> So what should be done with the two patches?
> Are they acceptable for staging, or will they be rejected?
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.
On Tue, Jan 31, Ian Jackson wrote: > Olaf Hering writes ("Re: [Xen-devel] [PATCH qemu-xen-traditional 1/2] xen_platform: unplug also SCSI disks [and 1 more messages]"): > > So what should be done with the two patches? > > Are they acceptable for staging, or will they be rejected? > > I am happy with the first patch, as I say. Would you like me to apply > it while we discuss the 2nd ? Yes. Will think about the 2nd. Thanks. Olaf
diff --git a/hw/pci.c b/hw/pci.c index c423285..7d8e3b6 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -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; diff --git a/hw/xen_platform.c b/hw/xen_platform.c index d282f8e..0a94e0d 100644 --- a/hw/xen_platform.c +++ b/hw/xen_platform.c @@ -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(); diff --git a/qemu-xen.h b/qemu-xen.h index 0598668..d315623 100644 --- a/qemu-xen.h +++ b/qemu-xen.h @@ -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);