diff mbox

[RFC,v2] Add option to switch off FDC

Message ID 20140131190322.GL29329@ERROL.INI.CMU.EDU
State New
Headers show

Commit Message

Gabriel L. Somlo Jan. 31, 2014, 7:03 p.m. UTC
Allow guests to omit the configuration of a floppy disk controller.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---

New in this version:

  - "int have_fdc = 1;" instead of "int no_fdc = 0;"

  - use 8bit field for FDC0's _STA method

On Thu, Jan 09, 2014 at 11:24:42AM +0100, Paolo Bonzini wrote:
> Il 09/01/2014 09:46, Markus Armbruster ha scritto:
> >> > I think we need to provide a mechanism for machine-specific options.
> >> > Options like this one does not scale.
> > -machine?  It already has many keys that really apply to only a few
> > machines.  Sticking in one more won't kill us.
> 
> Yeah, -machine would work as a short-term solution.  I was thinking of
> making -machine set properties on the QOM /machine object.  Then you can
> add properties to that object, and they will show up in -machine
> automagically.

So, are we talking only about the "policy" part here, i.e., how and when
to turn "have_fdc" off or on, and whether to expose that option through
the command line ?

Are we OK with the "mechanism" part, i.e. introducing "have_fdc", and
programatically configuring the FDC in pc.c and patching the DSDT
based on its value ?

Thanks,
  Gabriel

 qemu-options.hx           |  8 ++++++++
 vl.c                      |  4 ++++
 include/hw/i386/pc.h      |  3 +++
 hw/i386/pc.c              | 10 +++++++---
 hw/i386/acpi-dsdt.dsl     |  2 +-
 hw/i386/q35-acpi-dsdt.dsl |  4 ++--
 hw/i386/acpi-dsdt-isa.dsl | 11 +++--------
 hw/i386/acpi-build.c      |  8 +++++++-
 8 files changed, 35 insertions(+), 15 deletions(-)

Comments

Eric Blake Jan. 31, 2014, 7:39 p.m. UTC | #1
On 01/31/2014 12:03 PM, Gabriel L. Somlo wrote:
> Allow guests to omit the configuration of a floppy disk controller.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
> 
> New in this version:
> 
>   - "int have_fdc = 1;" instead of "int no_fdc = 0;"
> 
>   - use 8bit field for FDC0's _STA method
> 

> 
> Are we OK with the "mechanism" part, i.e. introducing "have_fdc", and
> programatically configuring the FDC in pc.c and patching the DSDT
> based on its value ?
> 

>  
> +DEF("no-fdc", 0, QEMU_OPTION_no_fdc,
> +    "-no-fdc         disable FDC\n", QEMU_ARCH_I386)
> +STEXI
> +@item -no-fdc
> +@findex -no-fdc
> +Disable FDC support.
> +ETEXI

This is not discoverable via the current state of QMP.  It also has the
annoyance of being a one-way switch with no way to revert to a default
state.  Can you make this instead use qemuOpts (maybe looking like '-fdc
yes' vs. '-fdc no')?
Michael S. Tsirkin Feb. 2, 2014, 1:47 p.m. UTC | #2
On Fri, Jan 31, 2014 at 12:39:01PM -0700, Eric Blake wrote:
> On 01/31/2014 12:03 PM, Gabriel L. Somlo wrote:
> > Allow guests to omit the configuration of a floppy disk controller.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> > 
> > New in this version:
> > 
> >   - "int have_fdc = 1;" instead of "int no_fdc = 0;"
> > 
> >   - use 8bit field for FDC0's _STA method
> > 
> 
> > 
> > Are we OK with the "mechanism" part, i.e. introducing "have_fdc", and
> > programatically configuring the FDC in pc.c and patching the DSDT
> > based on its value ?
> > 
> 
> >  
> > +DEF("no-fdc", 0, QEMU_OPTION_no_fdc,
> > +    "-no-fdc         disable FDC\n", QEMU_ARCH_I386)
> > +STEXI
> > +@item -no-fdc
> > +@findex -no-fdc
> > +Disable FDC support.
> > +ETEXI
> 
> This is not discoverable via the current state of QMP.  It also has the
> annoyance of being a one-way switch with no way to revert to a default
> state.  Can you make this instead use qemuOpts (maybe looking like '-fdc
> yes' vs. '-fdc no')?

I think Marcel said he's working on an infrastructure to add global
options as machine properties.

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Marcel Apfelbaum Feb. 10, 2014, 2:10 p.m. UTC | #3
On Sun, 2014-02-02 at 15:47 +0200, Michael S. Tsirkin wrote:
> On Fri, Jan 31, 2014 at 12:39:01PM -0700, Eric Blake wrote:
> > On 01/31/2014 12:03 PM, Gabriel L. Somlo wrote:
> > > Allow guests to omit the configuration of a floppy disk controller.
> > > 
> > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > > 
> > > New in this version:
> > > 
> > >   - "int have_fdc = 1;" instead of "int no_fdc = 0;"
> > > 
> > >   - use 8bit field for FDC0's _STA method
> > > 
> > 
> > > 
> > > Are we OK with the "mechanism" part, i.e. introducing "have_fdc", and
> > > programatically configuring the FDC in pc.c and patching the DSDT
> > > based on its value ?
> > > 
> > 
> > >  
> > > +DEF("no-fdc", 0, QEMU_OPTION_no_fdc,
> > > +    "-no-fdc         disable FDC\n", QEMU_ARCH_I386)
> > > +STEXI
> > > +@item -no-fdc
> > > +@findex -no-fdc
> > > +Disable FDC support.
> > > +ETEXI
> > 
> > This is not discoverable via the current state of QMP.  It also has the
> > annoyance of being a one-way switch with no way to revert to a default
> > state.  Can you make this instead use qemuOpts (maybe looking like '-fdc
> > yes' vs. '-fdc no')?
> 
> I think Marcel said he's working on an infrastructure to add global
> options as machine properties.
Yes, indeed, I am working on it. Maybe it worth waiting until it will be ready.

Thanks,
Marcel

> 
> > -- 
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
>
diff mbox

Patch

diff --git a/qemu-options.hx b/qemu-options.hx
index 56e5fdf..993d377 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1297,6 +1297,14 @@  STEXI
 Disable HPET support.
 ETEXI
 
+DEF("no-fdc", 0, QEMU_OPTION_no_fdc,
+    "-no-fdc         disable FDC\n", QEMU_ARCH_I386)
+STEXI
+@item -no-fdc
+@findex -no-fdc
+Disable FDC support.
+ETEXI
+
 DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
     "-acpitable [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=n][,{data|file}=file1[:file2]...]\n"
     "                ACPI table description\n", QEMU_ARCH_I386)
diff --git a/vl.c b/vl.c
index c73462e..ffaa7ae 100644
--- a/vl.c
+++ b/vl.c
@@ -214,6 +214,7 @@  const char *vnc_display;
 #endif
 int acpi_enabled = 1;
 int no_hpet = 0;
+int have_fdc = 1;
 int fd_bootchk = 1;
 static int no_reboot;
 int no_shutdown = 0;
@@ -3581,6 +3582,9 @@  int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_no_hpet:
                 no_hpet = 1;
                 break;
+            case QEMU_OPTION_no_fdc:
+                have_fdc = 0;
+                break;
             case QEMU_OPTION_balloon:
                 if (balloon_parse(optarg) < 0) {
                     fprintf(stderr, "Unknown -balloon argument %s\n", optarg);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3e1e81b..db4017b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -173,6 +173,9 @@  void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
 /* hpet.c */
 extern int no_hpet;
 
+/* fdc.c */
+extern int have_fdc;
+
 /* piix_pci.c */
 struct PCII440FXState;
 typedef struct PCII440FXState PCII440FXState;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 348b15f..d38e272 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1358,10 +1358,14 @@  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);
 
-    for(i = 0; i < MAX_FD; i++) {
-        fd[i] = drive_get(IF_FLOPPY, 0, i);
+    if (have_fdc) {
+        for (i = 0; i < MAX_FD; i++) {
+            fd[i] = drive_get(IF_FLOPPY, 0, i);
+        }
+        *floppy = fdctrl_init_isa(isa_bus, fd);
+    } else {
+        *floppy = NULL;
     }
-    *floppy = fdctrl_init_isa(isa_bus, fd);
 }
 
 void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index b23d5e0..16d3be5 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -149,11 +149,11 @@  DefinitionBlock (
                 , 3,
                 CBEN, 1,         // COM2
             }
-            Name(FDEN, 1)
         }
     }
 
 #define DSDT_APPLESMC_STA piix_dsdt_applesmc_sta
+#define DSDT_FDC_STA piix_dsdt_fdc_sta
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index d618e9e..0861414 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -181,13 +181,13 @@  DefinitionBlock (
             Field(LPCE, AnyAcc, NoLock, Preserve) {
                 CAEN,   1,
                 CBEN,   1,
-                LPEN,   1,
-                FDEN,   1
+                LPEN,   1
             }
         }
     }
 
 #define DSDT_APPLESMC_STA q35_dsdt_applesmc_sta
+#define DSDT_FDC_STA q35_dsdt_fdc_sta
 #include "acpi-dsdt-isa.dsl"
 
 
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index deb37de..ffe6770 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -60,14 +60,9 @@  Scope(\_SB.PCI0.ISA) {
 
     Device(FDC0) {
         Name(_HID, EisaId("PNP0700"))
-        Method(_STA, 0, NotSerialized) {
-            Store(FDEN, Local0)
-            If (LEqual(Local0, 0)) {
-                Return (0x00)
-            } Else {
-                Return (0x0F)
-            }
-        }
+        /* _STA will be patched to 0x0F if the FDC is present */
+        ACPI_EXTRACT_NAME_BYTE_CONST DSDT_FDC_STA
+        Name(_STA, 0xF0)
         Name(_CRS, ResourceTemplate() {
             IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
             IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 50e83f3..302a745 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -88,7 +88,7 @@  typedef struct AcpiBuildPciBusHotplugState {
 
 static void acpi_get_dsdt(AcpiMiscInfo *info)
 {
-    uint16_t *applesmc_sta;
+    uint16_t *applesmc_sta, *fdc_sta;
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     assert(!!piix != !!lpc);
@@ -97,16 +97,22 @@  static void acpi_get_dsdt(AcpiMiscInfo *info)
         info->dsdt_code = AcpiDsdtAmlCode;
         info->dsdt_size = sizeof AcpiDsdtAmlCode;
         applesmc_sta = piix_dsdt_applesmc_sta;
+        fdc_sta = piix_dsdt_fdc_sta;
     }
     if (lpc) {
         info->dsdt_code = Q35AcpiDsdtAmlCode;
         info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
         applesmc_sta = q35_dsdt_applesmc_sta;
+        fdc_sta = q35_dsdt_fdc_sta;
     }
 
     /* Patch in appropriate value for AppleSMC _STA */
     *(uint8_t *)(info->dsdt_code + *applesmc_sta) =
         applesmc_find() ? 0x0b : 0x00;
+
+    /* Patch in appropriate value for FDC _STA */
+    *(uint8_t *)(info->dsdt_code + *fdc_sta) =
+        have_fdc ? 0x0f : 0x00;
 }
 
 static