| Message ID | alpine.DEB.2.02.1505131826580.20496@kaball.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > Do not emulate a floppy drive if no drives are supposed to be present. > > This fixes the behavior of -nodefaults, that should remove the floppy > drive (see docs/qdev-device-use.txt:Default Devices), but actually > doesn't. Technically that doc is just refering to the disablement of the primary floppy drive, as opposed to the floppy controller. The floppy controller itself is really a built-in device that is defined as part of the machine type, along with the various other platform devices hanging off the PIIX and ISA brige. > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index a8e6be1..c9f50b3 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1410,6 +1410,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > qemu_irq *cpu_exit_irq; > MemoryRegion *ioport80_io = g_new(MemoryRegion, 1); > MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1); > + bool floppy_exist; > > memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1); > memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io); > @@ -1487,10 +1488,17 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); > DMA_init(0, cpu_exit_irq); > > + *floppy = NULL; > + floppy_exist = false; > for(i = 0; i < MAX_FD; i++) { > fd[i] = drive_get(IF_FLOPPY, 0, i); > + if (fd[i] != NULL) { > + floppy_exist = true; > + } > + } > + if (floppy_exist) { > + *floppy = fdctrl_init_isa(isa_bus, fd); > } > - *floppy = fdctrl_init_isa(isa_bus, fd); > } This results in a guest ABI change when updating QEMU, because the floppy controller will disappear for existing guests. This is liable to break guests upon migration. If we want to be able to disable the floppy controller itself, in addition to the floppy drives, then we'd likely need to define a property against the machine type to allow its existence to be toggled on/off. We'd then need to at least make sure the existing machine types defaulted to on, if we decided that new machine types should default to off. Libvirt would also need to gain the ability to represent the existance of the floppy controller and allow it to be turned on / off explicitly. Regards, Daniel
On Wed, 13 May 2015, Daniel P. Berrange wrote: > On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > > Do not emulate a floppy drive if no drives are supposed to be present. > > > > This fixes the behavior of -nodefaults, that should remove the floppy > > drive (see docs/qdev-device-use.txt:Default Devices), but actually > > doesn't. > > Technically that doc is just refering to the disablement of the > primary floppy drive, as opposed to the floppy controller. The > floppy controller itself is really a built-in device that is > defined as part of the machine type, along with the various other > platform devices hanging off the PIIX and ISA brige. I think you are right, this patch is a bit too harsh in fixing the problem: I just wanted to properly disable drive emulation, because from my tests the guest thinks that one drive is present even when is not. > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index a8e6be1..c9f50b3 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1410,6 +1410,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > > qemu_irq *cpu_exit_irq; > > MemoryRegion *ioport80_io = g_new(MemoryRegion, 1); > > MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1); > > + bool floppy_exist; > > > > memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1); > > memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io); > > @@ -1487,10 +1488,17 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > > cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); > > DMA_init(0, cpu_exit_irq); > > > > + *floppy = NULL; > > + floppy_exist = false; > > for(i = 0; i < MAX_FD; i++) { > > fd[i] = drive_get(IF_FLOPPY, 0, i); > > + if (fd[i] != NULL) { > > + floppy_exist = true; > > + } > > + } > > + if (floppy_exist) { > > + *floppy = fdctrl_init_isa(isa_bus, fd); > > } > > - *floppy = fdctrl_init_isa(isa_bus, fd); > > } > > This results in a guest ABI change when updating QEMU, because the floppy > controller will disappear for existing guests. This is liable to break > guests upon migration. > > If we want to be able to disable the floppy controller itself, in addition > to the floppy drives, then we'd likely need to define a property against > the machine type to allow its existence to be toggled on/off. We'd then > need to at least make sure the existing machine types defaulted to on, > if we decided that new machine types should default to off. > > Libvirt would also need to gain the ability to represent the existance > of the floppy controller and allow it to be turned on / off explicitly.
On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > Do not emulate a floppy drive if no drives are supposed to be present. > > This fixes the behavior of -nodefaults, that should remove the floppy > drive (see docs/qdev-device-use.txt:Default Devices), but actually > doesn't. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> No telling what this might break. I think a flag or a property to disable fdc would be safer. Default to enabled to make sure we don't break existing users. > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index a8e6be1..c9f50b3 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1410,6 +1410,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > qemu_irq *cpu_exit_irq; > MemoryRegion *ioport80_io = g_new(MemoryRegion, 1); > MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1); > + bool floppy_exist; > > memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1); > memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io); > @@ -1487,10 +1488,17 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); > DMA_init(0, cpu_exit_irq); > > + *floppy = NULL; > + floppy_exist = false; > for(i = 0; i < MAX_FD; i++) { > fd[i] = drive_get(IF_FLOPPY, 0, i); > + if (fd[i] != NULL) { > + floppy_exist = true; > + } > + } > + if (floppy_exist) { > + *floppy = fdctrl_init_isa(isa_bus, fd); > } > - *floppy = fdctrl_init_isa(isa_bus, fd); > } > > void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
On 05/13/2015 02:15 PM, Stefano Stabellini wrote: > On Wed, 13 May 2015, Daniel P. Berrange wrote: >> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: >>> Do not emulate a floppy drive if no drives are supposed to be present. >>> >>> This fixes the behavior of -nodefaults, that should remove the floppy >>> drive (see docs/qdev-device-use.txt:Default Devices), but actually >>> doesn't. >> >> Technically that doc is just refering to the disablement of the >> primary floppy drive, as opposed to the floppy controller. The >> floppy controller itself is really a built-in device that is >> defined as part of the machine type, along with the various other >> platform devices hanging off the PIIX and ISA brige. > > I think you are right, this patch is a bit too harsh in fixing the > problem: I just wanted to properly disable drive emulation, because from > my tests the guest thinks that one drive is present even when is not. > We should just be a little careful in explaining the difference between the controller, the drives, and what circumstances things show up and to whom. Currently the FDC is always present and always has two drive objects that are directly inlined to that device. Now, based on whether or not this line fires in vl.c: default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS); controls whether or not we find that drive in pc_basic_device_init as you've found, which controls (ultimately) whether or not fdctrl->drives[0].blk or fdctrl->drives[1].blk gets set. If the blk pointer is set, even if you have no media inserted, fd_revalidate (and pick_geometry) will *FORCIBLY PICK* a drive type for this floppy, currently a 1.44MB type, (20 sect, 80 track, 1 head) with a data rate of 500 Kbps. This is written to the rtc memory where seabios later reads it to discover if you have a guest-visible floppy drive there. Both QEMU and SeaBIOS confuse the concept of "A drive is present" with "A disk is present" which leads to these kinds of confusing scenarios. There's some work to do here, for sure. Anyway, maybe this patch is inappropriate: Just because we have no drives doesn't mean we don't have a controller. I have seen some discussion recently about allowing users to disable/remove the FDC altogether, though, and I think this is probably the way to go. > >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>> index a8e6be1..c9f50b3 100644 >>> --- a/hw/i386/pc.c >>> +++ b/hw/i386/pc.c >>> @@ -1410,6 +1410,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, >>> qemu_irq *cpu_exit_irq; >>> MemoryRegion *ioport80_io = g_new(MemoryRegion, 1); >>> MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1); >>> + bool floppy_exist; >>> >>> memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1); >>> memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io); >>> @@ -1487,10 +1488,17 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, >>> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); >>> DMA_init(0, cpu_exit_irq); >>> >>> + *floppy = NULL; >>> + floppy_exist = false; >>> for(i = 0; i < MAX_FD; i++) { >>> fd[i] = drive_get(IF_FLOPPY, 0, i); >>> + if (fd[i] != NULL) { >>> + floppy_exist = true; >>> + } >>> + } >>> + if (floppy_exist) { >>> + *floppy = fdctrl_init_isa(isa_bus, fd); >>> } >>> - *floppy = fdctrl_init_isa(isa_bus, fd); >>> } >> >> This results in a guest ABI change when updating QEMU, because the floppy >> controller will disappear for existing guests. This is liable to break >> guests upon migration. >> >> If we want to be able to disable the floppy controller itself, in addition >> to the floppy drives, then we'd likely need to define a property against >> the machine type to allow its existence to be toggled on/off. We'd then >> need to at least make sure the existing machine types defaulted to on, >> if we decided that new machine types should default to off. >> >> Libvirt would also need to gain the ability to represent the existance >> of the floppy controller and allow it to be turned on / off explicitly. > >
Am 13.05.2015 um 20:15 schrieb Stefano Stabellini: > On Wed, 13 May 2015, Daniel P. Berrange wrote: >> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: >>> Do not emulate a floppy drive if no drives are supposed to be present. >>> >>> This fixes the behavior of -nodefaults, that should remove the floppy >>> drive (see docs/qdev-device-use.txt:Default Devices), but actually >>> doesn't. >> Technically that doc is just refering to the disablement of the >> primary floppy drive, as opposed to the floppy controller. The >> floppy controller itself is really a built-in device that is >> defined as part of the machine type, along with the various other >> platform devices hanging off the PIIX and ISA brige. > I think you are right, this patch is a bit too harsh in fixing the > problem: I just wanted to properly disable drive emulation, because from > my tests the guest thinks that one drive is present even when is not. A short test on some of my physical machines shows that most of them don't have a floppy disk controller at all (dmesg | grep FDC). Only some older machines still have one. Therefore I think that QEMU must also be able to offer a virtual machine without an FDC, maybe as the default for the next version of QEMU. Stefan
On Thu, May 14, 2015 at 06:38:24AM +0200, Stefan Weil wrote: > Am 13.05.2015 um 20:15 schrieb Stefano Stabellini: > >On Wed, 13 May 2015, Daniel P. Berrange wrote: > >>On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > >>>Do not emulate a floppy drive if no drives are supposed to be present. > >>> > >>>This fixes the behavior of -nodefaults, that should remove the floppy > >>>drive (see docs/qdev-device-use.txt:Default Devices), but actually > >>>doesn't. > >>Technically that doc is just refering to the disablement of the > >>primary floppy drive, as opposed to the floppy controller. The > >>floppy controller itself is really a built-in device that is > >>defined as part of the machine type, along with the various other > >>platform devices hanging off the PIIX and ISA brige. > >I think you are right, this patch is a bit too harsh in fixing the > >problem: I just wanted to properly disable drive emulation, because from > >my tests the guest thinks that one drive is present even when is not. > > A short test on some of my physical machines shows that most > of them don't have a floppy disk controller at all (dmesg | grep FDC). > > Only some older machines still have one. > > Therefore I think that QEMU must also be able to offer a virtual > machine without an FDC, maybe as the default for the next > version of QEMU. > > Stefan Making it the default in QEMU might break a bunch of existing setups, but IMO it's very reasonable to add such a property, and teach libvirt to set it automatically.
On Wed, 13 May 2015, John Snow wrote: > On 05/13/2015 02:15 PM, Stefano Stabellini wrote: > > On Wed, 13 May 2015, Daniel P. Berrange wrote: > >> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > >>> Do not emulate a floppy drive if no drives are supposed to be present. > >>> > >>> This fixes the behavior of -nodefaults, that should remove the floppy > >>> drive (see docs/qdev-device-use.txt:Default Devices), but actually > >>> doesn't. > >> > >> Technically that doc is just refering to the disablement of the > >> primary floppy drive, as opposed to the floppy controller. The > >> floppy controller itself is really a built-in device that is > >> defined as part of the machine type, along with the various other > >> platform devices hanging off the PIIX and ISA brige. > > > > I think you are right, this patch is a bit too harsh in fixing the > > problem: I just wanted to properly disable drive emulation, because from > > my tests the guest thinks that one drive is present even when is not. > > > > We should just be a little careful in explaining the difference between > the controller, the drives, and what circumstances things show up and to > whom. > > Currently the FDC is always present and always has two drive objects > that are directly inlined to that device. > > Now, based on whether or not this line fires in vl.c: > default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS); > > controls whether or not we find that drive in pc_basic_device_init as > you've found, which controls (ultimately) whether or not > fdctrl->drives[0].blk or fdctrl->drives[1].blk gets set. > > If the blk pointer is set, even if you have no media inserted, > fd_revalidate (and pick_geometry) will *FORCIBLY PICK* a drive type for > this floppy, currently a 1.44MB type, (20 sect, 80 track, 1 head) with a > data rate of 500 Kbps. This is written to the rtc memory where seabios > later reads it to discover if you have a guest-visible floppy drive there. > > Both QEMU and SeaBIOS confuse the concept of "A drive is present" with > "A disk is present" which leads to these kinds of confusing scenarios. > > There's some work to do here, for sure. That was my impression too. On Thu, 14 May 2015, Stefan Weil wrote: > Am 13.05.2015 um 20:15 schrieb Stefano Stabellini: > > On Wed, 13 May 2015, Daniel P. Berrange wrote: > > > On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > > > > Do not emulate a floppy drive if no drives are supposed to be present. > > > > > > > > This fixes the behavior of -nodefaults, that should remove the floppy > > > > drive (see docs/qdev-device-use.txt:Default Devices), but actually > > > > doesn't. > > > Technically that doc is just refering to the disablement of the > > > primary floppy drive, as opposed to the floppy controller. The > > > floppy controller itself is really a built-in device that is > > > defined as part of the machine type, along with the various other > > > platform devices hanging off the PIIX and ISA brige. > > I think you are right, this patch is a bit too harsh in fixing the > > problem: I just wanted to properly disable drive emulation, because from > > my tests the guest thinks that one drive is present even when is not. > > A short test on some of my physical machines shows that most > of them don't have a floppy disk controller at all (dmesg | grep FDC). > > Only some older machines still have one. > > Therefore I think that QEMU must also be able to offer a virtual > machine without an FDC, maybe as the default for the next > version of QEMU. I think it would make sense to make it the default for -nodefaults machines. I would be OK with a new property too, as we could set it from libxl or libvirt. Anybody would be happy to pick this one up or should I do it?
On Thu, May 14, 2015 at 12:12:52PM +0100, Stefano Stabellini wrote: > On Wed, 13 May 2015, John Snow wrote: > > On 05/13/2015 02:15 PM, Stefano Stabellini wrote: > > > On Wed, 13 May 2015, Daniel P. Berrange wrote: > > >> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > > >>> Do not emulate a floppy drive if no drives are supposed to be present. > > >>> > > >>> This fixes the behavior of -nodefaults, that should remove the floppy > > >>> drive (see docs/qdev-device-use.txt:Default Devices), but actually > > >>> doesn't. > > >> > > >> Technically that doc is just refering to the disablement of the > > >> primary floppy drive, as opposed to the floppy controller. The > > >> floppy controller itself is really a built-in device that is > > >> defined as part of the machine type, along with the various other > > >> platform devices hanging off the PIIX and ISA brige. > > > > > > I think you are right, this patch is a bit too harsh in fixing the > > > problem: I just wanted to properly disable drive emulation, because from > > > my tests the guest thinks that one drive is present even when is not. > > > > > > > We should just be a little careful in explaining the difference between > > the controller, the drives, and what circumstances things show up and to > > whom. > > > > Currently the FDC is always present and always has two drive objects > > that are directly inlined to that device. > > > > Now, based on whether or not this line fires in vl.c: > > default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS); > > > > controls whether or not we find that drive in pc_basic_device_init as > > you've found, which controls (ultimately) whether or not > > fdctrl->drives[0].blk or fdctrl->drives[1].blk gets set. > > > > If the blk pointer is set, even if you have no media inserted, > > fd_revalidate (and pick_geometry) will *FORCIBLY PICK* a drive type for > > this floppy, currently a 1.44MB type, (20 sect, 80 track, 1 head) with a > > data rate of 500 Kbps. This is written to the rtc memory where seabios > > later reads it to discover if you have a guest-visible floppy drive there. > > > > Both QEMU and SeaBIOS confuse the concept of "A drive is present" with > > "A disk is present" which leads to these kinds of confusing scenarios. > > > > There's some work to do here, for sure. > > That was my impression too. > > > > On Thu, 14 May 2015, Stefan Weil wrote: > > Am 13.05.2015 um 20:15 schrieb Stefano Stabellini: > > > On Wed, 13 May 2015, Daniel P. Berrange wrote: > > > > On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > > > > > Do not emulate a floppy drive if no drives are supposed to be present. > > > > > > > > > > This fixes the behavior of -nodefaults, that should remove the floppy > > > > > drive (see docs/qdev-device-use.txt:Default Devices), but actually > > > > > doesn't. > > > > Technically that doc is just refering to the disablement of the > > > > primary floppy drive, as opposed to the floppy controller. The > > > > floppy controller itself is really a built-in device that is > > > > defined as part of the machine type, along with the various other > > > > platform devices hanging off the PIIX and ISA brige. > > > I think you are right, this patch is a bit too harsh in fixing the > > > problem: I just wanted to properly disable drive emulation, because from > > > my tests the guest thinks that one drive is present even when is not. > > > > A short test on some of my physical machines shows that most > > of them don't have a floppy disk controller at all (dmesg | grep FDC). > > > > Only some older machines still have one. > > > > Therefore I think that QEMU must also be able to offer a virtual > > machine without an FDC, maybe as the default for the next > > version of QEMU. > > I think it would make sense to make it the default for -nodefaults > machines. I would be OK with a new property too, as we could set it from > libxl or libvirt. Anybody would be happy to pick this one up or should I > do it? It isn't permissible to change the hardware exposed to guests for existing machine types, even when -nodefaults is used. Any change in defaults has to be for new machine types only. eg we can't change pc-2.3.0 machine type, but we could change defaults for the pc-2.4.0 machine type that will be in next release. That ensures migration upgrade compatibility for existing guests, while letting new guests get the new defaults. Regards, Daniel
On Thu, May 14, 2015 at 12:18:26PM +0100, Daniel P. Berrange wrote: > On Thu, May 14, 2015 at 12:12:52PM +0100, Stefano Stabellini wrote: > > On Wed, 13 May 2015, John Snow wrote: > > > On 05/13/2015 02:15 PM, Stefano Stabellini wrote: > > > > On Wed, 13 May 2015, Daniel P. Berrange wrote: > > > >> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > > > >>> Do not emulate a floppy drive if no drives are supposed to be present. > > > >>> > > > >>> This fixes the behavior of -nodefaults, that should remove the floppy > > > >>> drive (see docs/qdev-device-use.txt:Default Devices), but actually > > > >>> doesn't. > > > >> > > > >> Technically that doc is just refering to the disablement of the > > > >> primary floppy drive, as opposed to the floppy controller. The > > > >> floppy controller itself is really a built-in device that is > > > >> defined as part of the machine type, along with the various other > > > >> platform devices hanging off the PIIX and ISA brige. > > > > > > > > I think you are right, this patch is a bit too harsh in fixing the > > > > problem: I just wanted to properly disable drive emulation, because from > > > > my tests the guest thinks that one drive is present even when is not. > > > > > > > > > > We should just be a little careful in explaining the difference between > > > the controller, the drives, and what circumstances things show up and to > > > whom. > > > > > > Currently the FDC is always present and always has two drive objects > > > that are directly inlined to that device. > > > > > > Now, based on whether or not this line fires in vl.c: > > > default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS); > > > > > > controls whether or not we find that drive in pc_basic_device_init as > > > you've found, which controls (ultimately) whether or not > > > fdctrl->drives[0].blk or fdctrl->drives[1].blk gets set. > > > > > > If the blk pointer is set, even if you have no media inserted, > > > fd_revalidate (and pick_geometry) will *FORCIBLY PICK* a drive type for > > > this floppy, currently a 1.44MB type, (20 sect, 80 track, 1 head) with a > > > data rate of 500 Kbps. This is written to the rtc memory where seabios > > > later reads it to discover if you have a guest-visible floppy drive there. > > > > > > Both QEMU and SeaBIOS confuse the concept of "A drive is present" with > > > "A disk is present" which leads to these kinds of confusing scenarios. > > > > > > There's some work to do here, for sure. > > > > That was my impression too. > > > > > > > > On Thu, 14 May 2015, Stefan Weil wrote: > > > Am 13.05.2015 um 20:15 schrieb Stefano Stabellini: > > > > On Wed, 13 May 2015, Daniel P. Berrange wrote: > > > > > On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: > > > > > > Do not emulate a floppy drive if no drives are supposed to be present. > > > > > > > > > > > > This fixes the behavior of -nodefaults, that should remove the floppy > > > > > > drive (see docs/qdev-device-use.txt:Default Devices), but actually > > > > > > doesn't. > > > > > Technically that doc is just refering to the disablement of the > > > > > primary floppy drive, as opposed to the floppy controller. The > > > > > floppy controller itself is really a built-in device that is > > > > > defined as part of the machine type, along with the various other > > > > > platform devices hanging off the PIIX and ISA brige. > > > > I think you are right, this patch is a bit too harsh in fixing the > > > > problem: I just wanted to properly disable drive emulation, because from > > > > my tests the guest thinks that one drive is present even when is not. > > > > > > A short test on some of my physical machines shows that most > > > of them don't have a floppy disk controller at all (dmesg | grep FDC). > > > > > > Only some older machines still have one. > > > > > > Therefore I think that QEMU must also be able to offer a virtual > > > machine without an FDC, maybe as the default for the next > > > version of QEMU. > > > > I think it would make sense to make it the default for -nodefaults > > machines. I would be OK with a new property too, as we could set it from > > libxl or libvirt. Anybody would be happy to pick this one up or should I > > do it? > > It isn't permissible to change the hardware exposed to guests for existing > machine types, even when -nodefaults is used. Any change in defaults has > to be for new machine types only. eg we can't change pc-2.3.0 machine > type, but we could change defaults for the pc-2.4.0 machine type that > will be in next release. That ensures migration upgrade compatibility > for existing guests, while letting new guests get the new defaults. > > Regards, > Daniel For libvirt, yes. But command-line users might rely on the old behaviour with e.g. -M pc so we shouldn't change defaults lightly even for new machine types. > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, May 14, 2015 at 12:12:52PM +0100, Stefano Stabellini wrote: > I would be OK with a new property too, as we could set it from > libxl or libvirt. Anybody would be happy to pick this one up or should I > do it? Pls go ahead, I can merge it in the pc tree.
On 14/05/2015 13:47, Michael S. Tsirkin wrote: > > I would be OK with a new property too, as we could set it from > > libxl or libvirt. Anybody would be happy to pick this one up or should I > > do it? > > Pls go ahead, I can merge it in the pc tree. Note that because of the "-drive if=none,id=fdd1 -global isa-fdc.driveA=fdd1" trick to create a floppy drive without if=floppy, the default should remain on---at least for a few releases---even if -nodefaults is passed. Paolo
On Thu, May 14, 2015 at 01:54:22PM +0200, Paolo Bonzini wrote: > > > On 14/05/2015 13:47, Michael S. Tsirkin wrote: > > > I would be OK with a new property too, as we could set it from > > > libxl or libvirt. Anybody would be happy to pick this one up or should I > > > do it? > > > > Pls go ahead, I can merge it in the pc tree. > > Note that because of the "-drive if=none,id=fdd1 -global > isa-fdc.driveA=fdd1" trick to create a floppy drive without if=floppy, > the default should remain on---at least for a few releases---even if > -nodefaults is passed. > > Paolo Exactly.
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Thu, May 14, 2015 at 12:12:52PM +0100, Stefano Stabellini wrote: >> On Wed, 13 May 2015, John Snow wrote: >> > On 05/13/2015 02:15 PM, Stefano Stabellini wrote: >> > > On Wed, 13 May 2015, Daniel P. Berrange wrote: >> > >> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: >> > >>> Do not emulate a floppy drive if no drives are supposed to be present. >> > >>> >> > >>> This fixes the behavior of -nodefaults, that should remove the floppy >> > >>> drive (see docs/qdev-device-use.txt:Default Devices), but actually >> > >>> doesn't. >> > >> >> > >> Technically that doc is just refering to the disablement of the >> > >> primary floppy drive, as opposed to the floppy controller. The >> > >> floppy controller itself is really a built-in device that is >> > >> defined as part of the machine type, along with the various other >> > >> platform devices hanging off the PIIX and ISA brige. >> > > >> > > I think you are right, this patch is a bit too harsh in fixing the >> > > problem: I just wanted to properly disable drive emulation, because from >> > > my tests the guest thinks that one drive is present even when is not. >> > > >> > >> > We should just be a little careful in explaining the difference between >> > the controller, the drives, and what circumstances things show up and to >> > whom. >> > >> > Currently the FDC is always present and always has two drive objects >> > that are directly inlined to that device. >> > >> > Now, based on whether or not this line fires in vl.c: >> > default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS); >> > >> > controls whether or not we find that drive in pc_basic_device_init as >> > you've found, which controls (ultimately) whether or not >> > fdctrl->drives[0].blk or fdctrl->drives[1].blk gets set. >> > >> > If the blk pointer is set, even if you have no media inserted, >> > fd_revalidate (and pick_geometry) will *FORCIBLY PICK* a drive type for >> > this floppy, currently a 1.44MB type, (20 sect, 80 track, 1 head) with a >> > data rate of 500 Kbps. This is written to the rtc memory where seabios >> > later reads it to discover if you have a guest-visible floppy drive there. >> > >> > Both QEMU and SeaBIOS confuse the concept of "A drive is present" with >> > "A disk is present" which leads to these kinds of confusing scenarios. >> > >> > There's some work to do here, for sure. >> >> That was my impression too. >> >> >> >> On Thu, 14 May 2015, Stefan Weil wrote: >> > Am 13.05.2015 um 20:15 schrieb Stefano Stabellini: >> > > On Wed, 13 May 2015, Daniel P. Berrange wrote: >> > > > On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote: >> > > > > Do not emulate a floppy drive if no drives are supposed to be present. >> > > > > >> > > > > This fixes the behavior of -nodefaults, that should remove the floppy >> > > > > drive (see docs/qdev-device-use.txt:Default Devices), but actually >> > > > > doesn't. >> > > > Technically that doc is just refering to the disablement of the >> > > > primary floppy drive, as opposed to the floppy controller. The >> > > > floppy controller itself is really a built-in device that is >> > > > defined as part of the machine type, along with the various other >> > > > platform devices hanging off the PIIX and ISA brige. >> > > I think you are right, this patch is a bit too harsh in fixing the >> > > problem: I just wanted to properly disable drive emulation, because from >> > > my tests the guest thinks that one drive is present even when is not. >> > >> > A short test on some of my physical machines shows that most >> > of them don't have a floppy disk controller at all (dmesg | grep FDC). >> > >> > Only some older machines still have one. >> > >> > Therefore I think that QEMU must also be able to offer a virtual >> > machine without an FDC, maybe as the default for the next >> > version of QEMU. >> >> I think it would make sense to make it the default for -nodefaults >> machines. I would be OK with a new property too, as we could set it from >> libxl or libvirt. Anybody would be happy to pick this one up or should I >> do it? > > It isn't permissible to change the hardware exposed to guests for existing > machine types, even when -nodefaults is used. Any change in defaults has > to be for new machine types only. eg we can't change pc-2.3.0 machine > type, but we could change defaults for the pc-2.4.0 machine type that > will be in next release. That ensures migration upgrade compatibility > for existing guests, while letting new guests get the new defaults. Correct. Here's how I think it should be done: * Create a machine option to control the FDC This is a machine-specific option. It should only exist for machine types that have an optional FDC. Default must be "on" for old machine types. Default may be "off" for new machine types. It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards commonly don't have an FDC (depends on the Super I/O chip used). We may want to keep it off for pc-i440fx-2.4 and newer. I doubt there's a real i440FX without an FDC, but our virtual i440FX is quite unlike a real one in other ways already. * Create the FDC only if the option is "on". * Optional: make -drive if=floppy,... auto-enable it I wouldn't bother doing the same for -global isa-fdc.driveA=... and such. Stefano, if you're willing to tackle this, go right ahead!
On 14/05/2015 14:02, Markus Armbruster wrote: > It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards > commonly don't have an FDC (depends on the Super I/O chip used). > > We may want to keep it off for pc-i440fx-2.4 and newer. I doubt > there's a real i440FX without an FDC, but our virtual i440FX is quite > unlike a real one in other ways already. That would break libvirt for people upgrading from 2.3 to 2.4. So it's more like pc-i440fx-3.0 and pc-q35-3.0. Unless for q35 we decide to break everything and retroactively nuke the controller. (I'm still not sure why we have backwards-compatible machine types for q35). Paolo > * Create the FDC only if the option is "on". > > * Optional: make -drive if=floppy,... auto-enable it > > I wouldn't bother doing the same for -global isa-fdc.driveA=... and > such.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 14/05/2015 14:02, Markus Armbruster wrote: >> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards >> commonly don't have an FDC (depends on the Super I/O chip used). >> >> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt >> there's a real i440FX without an FDC, but our virtual i440FX is quite >> unlike a real one in other ways already. > > That would break libvirt for people upgrading from 2.3 to 2.4. So it's > more like pc-i440fx-3.0 and pc-q35-3.0. What exactly breaks when? > Unless for q35 we decide to > break everything and retroactively nuke the controller. > > (I'm still not sure why we have backwards-compatible machine types for q35). Beats me :) [...]
Paolo Bonzini <pbonzini@redhat.com> writes: > On 14/05/2015 13:47, Michael S. Tsirkin wrote: >> > I would be OK with a new property too, as we could set it from >> > libxl or libvirt. Anybody would be happy to pick this one up or should I >> > do it? >> >> Pls go ahead, I can merge it in the pc tree. > > Note that because of the "-drive if=none,id=fdd1 -global > isa-fdc.driveA=fdd1" trick to create a floppy drive without if=floppy, > the default should remain on---at least for a few releases---even if > -nodefaults is passed. Unless we bite the bullet and add the magic to make -global isa-fdc... auto-set the option to on.
On Thu, May 14, 2015 at 02:45:30PM +0200, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 14/05/2015 14:02, Markus Armbruster wrote: > >> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards > >> commonly don't have an FDC (depends on the Super I/O chip used). > >> > >> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt > >> there's a real i440FX without an FDC, but our virtual i440FX is quite > >> unlike a real one in other ways already. > > > > That would break libvirt for people upgrading from 2.3 to 2.4. So it's > > more like pc-i440fx-3.0 and pc-q35-3.0. > > What exactly breaks when? [quote] * Create the FDC only if the option is "on". * Optional: make -drive if=floppy,... auto-enable it I wouldn't bother doing the same for -global isa-fdc.driveA=... and such. [/quote] Libvirt uses -global when enabling floppy devices. So since current libvirt does not know about the new (to be created) machine type property to turn on FDC, it will get an error using -global isa-fdc.driveA= I'm not too bothered about this, as long as libvirt has enough advance notice to add support for the new machine type property to enable FDC before we change its default value to be "off". Perhaps one QEMU major release cycle before toggling the default, to give time for new libvirt to penetrate to distros Regards, Daniel
On 14/05/2015 14:45, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 14/05/2015 14:02, Markus Armbruster wrote: >>> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards >>> commonly don't have an FDC (depends on the Super I/O chip used). >>> >>> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt >>> there's a real i440FX without an FDC, but our virtual i440FX is quite >>> unlike a real one in other ways already. >> >> That would break libvirt for people upgrading from 2.3 to 2.4. So it's >> more like pc-i440fx-3.0 and pc-q35-3.0. > > What exactly breaks when? libvirt expects "-nodefaults -drive if=none,id=fdd0,... -global isa-fdc.driveA=fdd0" to result in a machine with a working FDD. It doesn't know that it has to add "-machine fdc=on". Besides, adding a new machine option is not the best we can do. If the default is "no FDC", all that is needed to add one back is -device. An FDC is yet another ISA device, it is possible to create one with -device. > add the magic to make -global isa-fdc... auto-set the option to on. That would be ugly magic. The more I think about this, the more I think this is just a kneejerk reaction to a sensationalist announcement. The effect of this vulnerability on properly configured data centers (running non-prehistoric versions of Xen or KVM and using stubdom/SELinux/AppArmor properly) should be really close to zero. It's a storm in a tea cup. Paolo >> Unless for q35 we decide to >> break everything and retroactively nuke the controller. >> >> (I'm still not sure why we have backwards-compatible machine types for q35). > > Beats me :) > > [...] >
Thursday, May 14, 2015, 2:53:17 PM, you wrote: > On 14/05/2015 14:45, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 14/05/2015 14:02, Markus Armbruster wrote: >>>> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards >>>> commonly don't have an FDC (depends on the Super I/O chip used). >>>> >>>> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt >>>> there's a real i440FX without an FDC, but our virtual i440FX is quite >>>> unlike a real one in other ways already. >>> >>> That would break libvirt for people upgrading from 2.3 to 2.4. So it's >>> more like pc-i440fx-3.0 and pc-q35-3.0. >> >> What exactly breaks when? > libvirt expects "-nodefaults -drive if=none,id=fdd0,... -global > isa-fdc.driveA=fdd0" to result in a machine with a working FDD. It > doesn't know that it has to add "-machine fdc=on". > Besides, adding a new machine option is not the best we can do. If the > default is "no FDC", all that is needed to add one back is -device. An > FDC is yet another ISA device, it is possible to create one with -device. >> add the magic to make -global isa-fdc... auto-set the option to on. > That would be ugly magic. > The more I think about this, the more I think this is just a kneejerk > reaction to a sensationalist announcement. The effect of this > vulnerability on properly configured data centers (running > non-prehistoric versions of Xen or KVM and using > stubdom/SELinux/AppArmor properly) should be really close to zero. > It's a storm in a tea cup. > Paolo I tend to kindly disagree if you look at the broader perspective, yes it's could be a storm in a tea cup, but there seems to be a pattern. From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of auto configuration / bundling in platforms is necessary (to limit the length of the cmdline to be supplied). But from a library / toolstack point of view it's quite nasty if *all* more or less obscure options have to actively disabled. It's quite undoable, not to mention what happens when new ones get added. From this PoV it would be better to have to actively enable all the stuff you want. At least Xen seemed to have taken the "no-defaults" as the best option to get there so far, but that doesn't seem to It's not the first CVE that has come from this "you have to actively disable all you don't want to happen" and probably won't be the last. So a "-no-defaults-now-for-real" option/mode for libraries / toolstacks, that requires them to be very verbose, explicit and specific in what they actually want, could be valuable. -- Sander >>> Unless for q35 we decide to >>> break everything and retroactively nuke the controller. >>> >>> (I'm still not sure why we have backwards-compatible machine types for q35). >> >> Beats me :) >> >> [...] >>
On Thu, May 14, 2015 at 03:25:39PM +0200, Sander Eikelenboom wrote: > > Thursday, May 14, 2015, 2:53:17 PM, you wrote: > > > > > On 14/05/2015 14:45, Markus Armbruster wrote: > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> > >>> On 14/05/2015 14:02, Markus Armbruster wrote: > >>>> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards > >>>> commonly don't have an FDC (depends on the Super I/O chip used). > >>>> > >>>> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt > >>>> there's a real i440FX without an FDC, but our virtual i440FX is quite > >>>> unlike a real one in other ways already. > >>> > >>> That would break libvirt for people upgrading from 2.3 to 2.4. So it's > >>> more like pc-i440fx-3.0 and pc-q35-3.0. > >> > >> What exactly breaks when? > > > libvirt expects "-nodefaults -drive if=none,id=fdd0,... -global > > isa-fdc.driveA=fdd0" to result in a machine with a working FDD. It > > doesn't know that it has to add "-machine fdc=on". > > > Besides, adding a new machine option is not the best we can do. If the > > default is "no FDC", all that is needed to add one back is -device. An > > FDC is yet another ISA device, it is possible to create one with -device. > > >> add the magic to make -global isa-fdc... auto-set the option to on. > > > That would be ugly magic. > > > The more I think about this, the more I think this is just a kneejerk > > reaction to a sensationalist announcement. The effect of this > > vulnerability on properly configured data centers (running > > non-prehistoric versions of Xen or KVM and using > > stubdom/SELinux/AppArmor properly) should be really close to zero. > > > It's a storm in a tea cup. > > > Paolo > > I tend to kindly disagree if you look at the broader perspective, yes it's could > be a storm in a tea cup, but there seems to be a pattern. > > From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of > auto configuration / bundling in platforms is necessary (to limit the length of > the cmdline to be supplied). > > But from a library / toolstack point of view it's quite nasty if *all* more or > less obscure options have to actively disabled. It's quite undoable, not to mention what > happens when new ones get added. > > From this PoV it would be better to have to actively enable all the stuff you want. > > At least Xen seemed to have taken the "no-defaults" as the best option to get > there so far, but that doesn't seem to > > It's not the first CVE that has come from this "you have to actively disable all > you don't want to happen" and probably won't be the last. > > So a "-no-defaults-now-for-real" option/mode for libraries / toolstacks, that > requires them to be very verbose, explicit and specific in what they actually > want, could be valuable. That doesn't really scale to be honest - a -no-defaults-now-for-real basically means deleting the entire concept of machine types, and requiring every single aspect of the base chipset to be explicitly configured. That's certainly possible but its a massive code job for QEMU and the management apps using QEMU. In fact the amount of code we'll have to write to achieve that in both QEMU and libvirt would probably introduce more bugs, so its questionable whether it would even be a net win in terms of reducing CVEs. Even if you disable some more default devices, the amount of code that'll be taken out of the attack surface is going to be tiny compared to the amount of code that will always remain due to the feature needs of the OS. So even with more default devices disabled, we have to assume that we will continue to see CVEs in stuff that remains and so must ensure we can mitigate the risks such that they have negligble impact on people who have deployed QEMU. ie the goal should be that a guest attack on QEMU ends up being nothing worse than a denial of service on attacker themselves. This is why using technologies such as SELinux, AppArmour, StubDomains and seccomp are so important when running QEMU. There is scope for applying more such as kernel namespaces to further isolate QEMU processes from each other. With these defences in place these kind of bugs really do become a storm in a teacup as Paolo suggests. That's not to say they are not important to fix, but we need to have some perspective about where efforts should be best focused to reduce our security risk. Regards, Daniel
On 14/05/2015 15:25, Sander Eikelenboom wrote: > I tend to kindly disagree if you look at the broader perspective, yes it's could > be a storm in a tea cup, but there seems to be a pattern. > > From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of > auto configuration / bundling in platforms is necessary (to limit the length of > the cmdline to be supplied). > > But from a library / toolstack point of view it's quite nasty if *all* more or > less obscure options have to actively disabled. It's quite undoable, not to mention what > happens when new ones get added. Where do you stop? Do you want to disable ACPI by default? It's a relatively large amount of code, but for most modern guests it is necessary. Besides, removing just the floppy drive makes little sense. The following devices remain with -nodefaults: - an HPET - the power management device, which includes an I2C controller - an IDE controller - a keyboard controller - the magic VMware I/O port - the PC speaker (!) - the legacy PIT - the real-time clock - the two PICs and IOAPIC Of all these, most guests will only use the PM device (maybe) and a small subset of the RTC. At the very least, the IDE controller, PC speaker and the HPET should be removed by -nodefaults-i-mean-it. If you're using UEFI firmware, probably you could remove everything except the PM device---maybe the RTC and the IOAPIC. At this point this is not -nodefaults-i-mean-it, it's a different machine type altogether and it's remarkably different from a PC. Reducing the attack surface is always nice, but hypervisor and device model bugs are going to exist forever. The amount of code for legacy devices is all in all not that big, and I can hardly recall other vulnerabilities in there. This is why I say it's a knee-jerk reaction. It is the intrinsic problem of virtualization mentioned in the famous quote of Theo de Raadt(*). Even if you do PV like Xen or Hyper-V, you have less or no QEMU code but you throw more hypervisor code in untrusted hands (hypervisor bugs exist, e.g. CVE-2012-4539, CVE-2012-5510 or CVE-2013-1964). (*) Which of course misses the point of virtualization altogether. Once you have established that you need more density, choosing containers vs. virtualization is a gamble on whether you prefer more moving parts and more layers that have to be broken (more isolation), or fewer moving parts and less isolation. Paolo > From this PoV it would be better to have to actively enable all the stuff you want. > > At least Xen seemed to have taken the "no-defaults" as the best option to get > there so far, but that doesn't seem to > > It's not the first CVE that has come from this "you have to actively disable all > you don't want to happen" and probably won't be the last. > > So a "-no-defaults-now-for-real" option/mode for libraries / toolstacks, that > requires them to be very verbose, explicit and specific in what they actually > want, could be valuable.
On Thu, May 14, 2015 at 03:25:39PM +0200, Sander Eikelenboom wrote: > > Thursday, May 14, 2015, 2:53:17 PM, you wrote: > > > > > On 14/05/2015 14:45, Markus Armbruster wrote: > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> > >>> On 14/05/2015 14:02, Markus Armbruster wrote: > >>>> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards > >>>> commonly don't have an FDC (depends on the Super I/O chip used). > >>>> > >>>> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt > >>>> there's a real i440FX without an FDC, but our virtual i440FX is quite > >>>> unlike a real one in other ways already. > >>> > >>> That would break libvirt for people upgrading from 2.3 to 2.4. So it's > >>> more like pc-i440fx-3.0 and pc-q35-3.0. > >> > >> What exactly breaks when? > > > libvirt expects "-nodefaults -drive if=none,id=fdd0,... -global > > isa-fdc.driveA=fdd0" to result in a machine with a working FDD. It > > doesn't know that it has to add "-machine fdc=on". > > > Besides, adding a new machine option is not the best we can do. If the > > default is "no FDC", all that is needed to add one back is -device. An > > FDC is yet another ISA device, it is possible to create one with -device. > > >> add the magic to make -global isa-fdc... auto-set the option to on. > > > That would be ugly magic. > > > The more I think about this, the more I think this is just a kneejerk > > reaction to a sensationalist announcement. The effect of this > > vulnerability on properly configured data centers (running > > non-prehistoric versions of Xen or KVM and using > > stubdom/SELinux/AppArmor properly) should be really close to zero. > > > It's a storm in a tea cup. > > > Paolo I agree. An option to disable fdc sounds like a reasonable thing to do reduce attack surface while keeping full compatibility. After all downstreams disable devices they don't want to support, too. Changing defaults just because a device had a bug does sound extreme. > I tend to kindly disagree if you look at the broader perspective, yes it's could > be a storm in a tea cup, but there seems to be a pattern. > > From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of > auto configuration / bundling in platforms is necessary (to limit the length of > the cmdline to be supplied). > > But from a library / toolstack point of view it's quite nasty if *all* more or > less obscure options have to actively disabled. It's quite undoable, not to mention what > happens when new ones get added. I support this statement for new features. Users should opt-in to them. E.g. we learned this the hard way with pvpanic. > From this PoV it would be better to have to actively enable all the stuff you want. > > At least Xen seemed to have taken the "no-defaults" as the best option to get > there so far, but that doesn't seem to > > It's not the first CVE that has come from this "you have to actively disable all > you don't want to happen" and probably won't be the last. > > So a "-no-defaults-now-for-real" option/mode for libraries / toolstacks, that > requires them to be very verbose, explicit and specific in what they actually > want, could be valuable. > > -- > Sander If you would change what this flag means in each release, that would achieve no useful purpose IMO. > >>> Unless for q35 we decide to > >>> break everything and retroactively nuke the controller. > >>> > >>> (I'm still not sure why we have backwards-compatible machine types for q35). > >> > >> Beats me :) > >> > >> [...] > >> > I'm fine with making them all identical.
On Thu, May 14, 2015 at 02:02:04PM +0200, Markus Armbruster wrote: > Correct. > > Here's how I think it should be done: > > * Create a machine option to control the FDC > > This is a machine-specific option. It should only exist for machine > types that have an optional FDC. > > Default must be "on" for old machine types. Default may be "off" for > new machine types. > > It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards > commonly don't have an FDC (depends on the Super I/O chip used). > > We may want to keep it off for pc-i440fx-2.4 and newer. I doubt > there's a real i440FX without an FDC, but our virtual i440FX is quite > unlike a real one in other ways already. I think making it off by default is a bad idea, it will break command-line users. > * Create the FDC only if the option is "on". > > * Optional: make -drive if=floppy,... auto-enable it Every time we do such auto hacks, we regret this later. Just do what we are told, fail if=floppy if disabled. > I wouldn't bother doing the same for -global isa-fdc.driveA=... and > such. > > Stefano, if you're willing to tackle this, go right ahead!
On Thu, 14 May 2015, Paolo Bonzini wrote: > On 14/05/2015 15:25, Sander Eikelenboom wrote: > > I tend to kindly disagree if you look at the broader perspective, yes it's could > > be a storm in a tea cup, but there seems to be a pattern. > > > > From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of > > auto configuration / bundling in platforms is necessary (to limit the length of > > the cmdline to be supplied). > > > > But from a library / toolstack point of view it's quite nasty if *all* more or > > less obscure options have to actively disabled. It's quite undoable, not to mention what > > happens when new ones get added. > > Where do you stop? At this stage I would be happy enough if no floppy drives were actually emulated when the user asks for none. > Do you want to disable ACPI by default? It's a relatively large amount > of code, but for most modern guests it is necessary. > > Besides, removing just the floppy drive makes little sense. The > following devices remain with -nodefaults: > > - an HPET > > - the power management device, which includes an I2C controller > > - an IDE controller > > - a keyboard controller > > - the magic VMware I/O port > > - the PC speaker (!) > > - the legacy PIT > > - the real-time clock > > - the two PICs and IOAPIC > > Of all these, most guests will only use the PM device (maybe) and a > small subset of the RTC. At the very least, the IDE controller, PC > speaker and the HPET should be removed by -nodefaults-i-mean-it. If > you're using UEFI firmware, probably you could remove everything except > the PM device---maybe the RTC and the IOAPIC. At this point this is not > -nodefaults-i-mean-it, it's a different machine type altogether and it's > remarkably different from a PC. > > Reducing the attack surface is always nice, but hypervisor and device > model bugs are going to exist forever. The amount of code for legacy > devices is all in all not that big, and I can hardly recall other > vulnerabilities in there. This is why I say it's a knee-jerk reaction. > > It is the intrinsic problem of virtualization mentioned in the famous > quote of Theo de Raadt(*). Even if you do PV like Xen or Hyper-V, you > have less or no QEMU code but you throw more hypervisor code in > untrusted hands (hypervisor bugs exist, e.g. CVE-2012-4539, > CVE-2012-5510 or CVE-2013-1964). > > (*) Which of course misses the point of virtualization > altogether. Once you have established that you need > more density, choosing containers vs. virtualization > is a gamble on whether you prefer more moving parts and > more layers that have to be broken (more isolation), or > fewer moving parts and less isolation. Sure, but it is harder to write a device emulator in a secure fashion than a brand new PV interface, that can be designed with security in mind from scratch. The number of VM escaping CVEs affecting Xen PV interfaces is extremely low, I think it was just two since the start of the project. > > From this PoV it would be better to have to actively enable all the stuff you want. > > > > At least Xen seemed to have taken the "no-defaults" as the best option to get > > there so far, but that doesn't seem to > > > > It's not the first CVE that has come from this "you have to actively disable all > > you don't want to happen" and probably won't be the last. > > > > So a "-no-defaults-now-for-real" option/mode for libraries / toolstacks, that > > requires them to be very verbose, explicit and specific in what they actually > > want, could be valuable. >
On 14/05/2015 16:39, Stefano Stabellini wrote: > On Thu, 14 May 2015, Paolo Bonzini wrote: >> On 14/05/2015 15:25, Sander Eikelenboom wrote: >>> I tend to kindly disagree if you look at the broader perspective, yes it's could >>> be a storm in a tea cup, but there seems to be a pattern. >>> >>> From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of >>> auto configuration / bundling in platforms is necessary (to limit the length of >>> the cmdline to be supplied). >>> >>> But from a library / toolstack point of view it's quite nasty if *all* more or >>> less obscure options have to actively disabled. It's quite undoable, not to mention what >>> happens when new ones get added. >> >> Where do you stop? > > At this stage I would be happy enough if no floppy drives were actually > emulated when the user asks for none. Floppy drives aren't being emulated; the controller is. Same for IDE, so why single out the FDD controller? > Sure, but it is harder to write a device emulator in a secure fashion > than a brand new PV interface, that can be designed with security in > mind from scratch. The number of VM escaping CVEs affecting Xen PV > interfaces is extremely low, I think it was just two since the start of > the project. Sure; OTOH, treating hypervisor and QEMU escapes would be very silly, if QEMU has been properly protected (through any combination of stubdoms, LSM, seccomp, ...). Paolo > > >>> From this PoV it would be better to have to actively enable all the stuff you want. >>> >>> At least Xen seemed to have taken the "no-defaults" as the best option to get >>> there so far, but that doesn't seem to >>> >>> It's not the first CVE that has come from this "you have to actively disable all >>> you don't want to happen" and probably won't be the last. >>> >>> So a "-no-defaults-now-for-real" option/mode for libraries / toolstacks, that >>> requires them to be very verbose, explicit and specific in what they actually >>> want, could be valuable. >>
On Thu, 14 May 2015, Paolo Bonzini wrote: > On 14/05/2015 16:39, Stefano Stabellini wrote: > > On Thu, 14 May 2015, Paolo Bonzini wrote: > >> On 14/05/2015 15:25, Sander Eikelenboom wrote: > >>> I tend to kindly disagree if you look at the broader perspective, yes it's could > >>> be a storm in a tea cup, but there seems to be a pattern. > >>> > >>> From a "cmdline user" / "platform emulation" point of view i can imagine that some sort of > >>> auto configuration / bundling in platforms is necessary (to limit the length of > >>> the cmdline to be supplied). > >>> > >>> But from a library / toolstack point of view it's quite nasty if *all* more or > >>> less obscure options have to actively disabled. It's quite undoable, not to mention what > >>> happens when new ones get added. > >> > >> Where do you stop? > > > > At this stage I would be happy enough if no floppy drives were actually > > emulated when the user asks for none. > > Floppy drives aren't being emulated; the controller is. Same for IDE, > so why single out the FDD controller? The problem is that floppy drive emulation code in the controller is not completely turned off even when no drives are available, see: http://marc.info/?l=xen-devel&m=143155839722714&w=2 Instead of disentangling the controller code, I am taking the easy route of removing the controller. > > Sure, but it is harder to write a device emulator in a secure fashion > > than a brand new PV interface, that can be designed with security in > > mind from scratch. The number of VM escaping CVEs affecting Xen PV > > interfaces is extremely low, I think it was just two since the start of > > the project. > > Sure; OTOH, treating hypervisor and QEMU escapes would be very silly, if > QEMU has been properly protected (through any combination of stubdoms, > LSM, seccomp, ...). That is true, but I don't know how many distros setup up selinux, stubdoms, etc properly by default, but I am guessing not that many.
On 05/14/2015 10:07 AM, Michael S. Tsirkin wrote: > On Thu, May 14, 2015 at 02:02:04PM +0200, Markus Armbruster wrote: >> Correct. >> >> Here's how I think it should be done: >> >> * Create a machine option to control the FDC >> >> This is a machine-specific option. It should only exist for machine >> types that have an optional FDC. >> >> Default must be "on" for old machine types. Default may be "off" for >> new machine types. >> >> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards >> commonly don't have an FDC (depends on the Super I/O chip used). >> >> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt >> there's a real i440FX without an FDC, but our virtual i440FX is quite >> unlike a real one in other ways already. > > I think making it off by default is a bad idea, it will break > command-line users. > > If we can add a flag to disable it, I still think I wouldn't mind that, if it could be worked out to not be hacky and gross. >> * Create the FDC only if the option is "on". >> >> * Optional: make -drive if=floppy,... auto-enable it > > Every time we do such auto hacks, we regret this later. > Just do what we are told, fail if=floppy if disabled. > I agree very much. Just because the current drive/device syntax is almost totally hosed doesn't mean we should put more wood on the fire. >> I wouldn't bother doing the same for -global isa-fdc.driveA=... and >> such. >> >> Stefano, if you're willing to tackle this, go right ahead! I'm definitely against a "--seriously-nothing" flag because the line for what is embedded or not is fuzzy. Paolo raises some good points against where you draw the line for what we decide to allow users to include/exclude that is otherwise considered part of the board. Still, given the hype train, if there is an API we could introduce that is likely not to make our code gross (or make us belly-ache about how dumb we were in 5 years) that disables the FDC, I don't think I would mind terribly. I'll leave that to minds more opinionated than mine to hash out, though. Maybe the best option here really is to carefully separate optional from non-optional components (FDC vs. Floppy Drive, Floppy Disk code) and just give the core FDC code a good scrubbing. --js
John Snow <jsnow@redhat.com> writes: > On 05/14/2015 10:07 AM, Michael S. Tsirkin wrote: >> On Thu, May 14, 2015 at 02:02:04PM +0200, Markus Armbruster wrote: >>> Correct. >>> >>> Here's how I think it should be done: >>> >>> * Create a machine option to control the FDC >>> >>> This is a machine-specific option. It should only exist for machine >>> types that have an optional FDC. >>> >>> Default must be "on" for old machine types. Default may be "off" for >>> new machine types. >>> >>> It should certainly be off for pc-q35-2.4 and newer. Real Q35 boards >>> commonly don't have an FDC (depends on the Super I/O chip used). >>> >>> We may want to keep it off for pc-i440fx-2.4 and newer. I doubt >>> there's a real i440FX without an FDC, but our virtual i440FX is quite >>> unlike a real one in other ways already. >> >> I think making it off by default is a bad idea, it will break >> command-line users. >> >> > > If we can add a flag to disable it, I still think I wouldn't mind that, > if it could be worked out to not be hacky and gross. > >>> * Create the FDC only if the option is "on". >>> >>> * Optional: make -drive if=floppy,... auto-enable it >> >> Every time we do such auto hacks, we regret this later. >> Just do what we are told, fail if=floppy if disabled. >> > > I agree very much. Just because the current drive/device syntax is > almost totally hosed doesn't mean we should put more wood on the fire. > >>> I wouldn't bother doing the same for -global isa-fdc.driveA=... and >>> such. >>> >>> Stefano, if you're willing to tackle this, go right ahead! > > > I'm definitely against a "--seriously-nothing" flag because the line for > what is embedded or not is fuzzy. Paolo raises some good points against > where you draw the line for what we decide to allow users to > include/exclude that is otherwise considered part of the board. --nodefaults must continue to disable all optional parts of the board. What exactly is optional is for the board / machine type to define. It can't be changed once the machine type is released. When in doubt, make it optional. Especially when the device has user-configurable properties, because optional devices are much nicer to configure than onboard devices. For an onboard device, you have to mess with -global, e.g. -global isa-fdc.driveA=fda Since -global applies to *all* devices of a kind, this has unwanted side effects when you have more than one. For instance: $ qemu-system-x86 -nodefaults -S -display none -drive if=none,id=fda -global isa-fdc.driveA=fda -device isa-fdc,iobase=370 qemu-system-x86: -device isa-fdc,iobase=370: Warning: global isa-fdc.driveA=fda ignored (Property 'isa-fdc.driveA' can't take value 'fda', it's in use) If it was optional, you'd do a perfectly regular -device isa-fdc,id=fdc0,driveA=floppyA which doesn't mess up any subsequent -device isa-fdc. Since Q35 is just starting to become migratable, the time to painlessly change its optional parts is *now*. For i440FX, we'll have to sacrifice some to the compatibility idols. > Still, given the hype train, if there is an API we could introduce that > is likely not to make our code gross (or make us belly-ache about how > dumb we were in 5 years) that disables the FDC, I don't think I would > mind terribly. I'll leave that to minds more opinionated than mine to > hash out, though. > > Maybe the best option here really is to carefully separate optional from > non-optional components (FDC vs. Floppy Drive, Floppy Disk code) and > just give the core FDC code a good scrubbing. In my not particularly humble opinion, time spent on FDC code is time stolen from more useful matters.
On 15/05/2015 09:50, Markus Armbruster wrote: > --nodefaults must continue to disable all optional parts of the board. > > What exactly is optional is for the board / machine type to define. It > can't be changed once the machine type is released. > > When in doubt, make it optional. I agree entirely. This unfortunately applies only to future boards. > Since Q35 is just starting to become migratable, the time to painlessly > change its optional parts is *now*. I can buy this. Let's just not divert attention from more important stuff. I may be more inclined to accept patches, if we add something to our security policy about disliking cute backronyms that attract bad journalism like magnets. And I'm only half joking; more like only 1% joking. Paolo
On Fri, 15 May 2015, Paolo Bonzini wrote: > On 15/05/2015 09:50, Markus Armbruster wrote: > > > --nodefaults must continue to disable all optional parts of the board. > > > > What exactly is optional is for the board / machine type to define. It > > can't be changed once the machine type is released. > > > > When in doubt, make it optional. > > I agree entirely. This unfortunately applies only to future boards. > > > Since Q35 is just starting to become migratable, the time to painlessly > > change its optional parts is *now*. > > I can buy this. > > Let's just not divert attention from more important stuff. I may be > more inclined to accept patches, if we add something to our security > policy about disliking cute backronyms that attract bad journalism like > magnets. And I'm only half joking; more like only 1% joking. +1
Am 14.05.2015 um 19:54 hat John Snow geschrieben: > On 05/14/2015 10:07 AM, Michael S. Tsirkin wrote: > > On Thu, May 14, 2015 at 02:02:04PM +0200, Markus Armbruster wrote: > >> * Create the FDC only if the option is "on". > >> > >> * Optional: make -drive if=floppy,... auto-enable it > > > > Every time we do such auto hacks, we regret this later. > > Just do what we are told, fail if=floppy if disabled. > > > > I agree very much. Just because the current drive/device syntax is > almost totally hosed doesn't mean we should put more wood on the fire. This is silly. Don't break our user interfaces just because you're panicking and everybody is scared of floppies now. The very purpose of if=foo (except if=none) is to add 'foo' hardware. if=virtio adds a virtio-blk controller. if=scsi adds a SCSI controller if no slot is available on an existing controller. Of course, if=floppy must create all the hardware that is necessary to have a floppy drive. If we ever add a -blockdev, it wouldn't have if=... at all, but for -drive, if=... is part of the interface (and as a convenience feature, I would always want to have options like this), so we shouldn't break it, but keep it consistent. > Maybe the best option here really is to carefully separate optional from > non-optional components (FDC vs. Floppy Drive, Floppy Disk code) and > just give the core FDC code a good scrubbing. That would be nice if someone could finally find the time to do so. Kevin
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index a8e6be1..c9f50b3 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1410,6 +1410,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, qemu_irq *cpu_exit_irq; MemoryRegion *ioport80_io = g_new(MemoryRegion, 1); MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1); + bool floppy_exist; memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1); memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io); @@ -1487,10 +1488,17 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1); DMA_init(0, cpu_exit_irq); + *floppy = NULL; + floppy_exist = false; for(i = 0; i < MAX_FD; i++) { fd[i] = drive_get(IF_FLOPPY, 0, i); + if (fd[i] != NULL) { + floppy_exist = true; + } + } + if (floppy_exist) { + *floppy = fdctrl_init_isa(isa_bus, fd); } - *floppy = fdctrl_init_isa(isa_bus, fd); } void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
Do not emulate a floppy drive if no drives are supposed to be present. This fixes the behavior of -nodefaults, that should remove the floppy drive (see docs/qdev-device-use.txt:Default Devices), but actually doesn't. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>