| Message ID | 20140108200217.GA4094@ERROL.INI.CMU.EDU |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 08, 2014 at 03:02:17PM -0500, Gabriel L. Somlo wrote: > Add i386 command line option "-no-fdc", which allows guests to omit the > configuration of a floppy controller. Applies on top of my previous patch > titled "Add DSDT node for AppleSMC" > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > --- > On Sun, Dec 22, 2013 at 11:21:00PM +0100, Laszlo Ersek wrote: > >I guess the "by the book" solution would be to really stop the FDC from > >being emulated when the AppleSMC is present > > On Wed, Dec 25, 2013 at 09:20:55PM +0200, Michael S. Tsirkin wrote: > >On Wed, Dec 25, 2013 at 08:11:56PM +0100, Alexander Graf wrote: > >>Speaking of which, does the q35 even have an fdc? > >I don't think it does but this device seems to be supported with piix as well > > This patch should be the first step to resolve all these issues. > If/when we make it possible to turn off the FDC, we can then choose > to leave it out altogether on Q35, and/or to throw an error if both > it and the AppleSMC are turned on, or have the presence of the > AppleSMC automatically force the FDC to be turned off, etc. > > Please let me know what you all think. > > Thanks, > Gabriel > > include/hw/i386/pc.h | 3 +++ > qemu-options.hx | 8 ++++++++ > vl.c | 4 ++++ > 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, 36 insertions(+), 14 deletions(-) > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 24eb3de..5226a79 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 no_fdc; > + > /* piix_pci.c */ > struct PCII440FXState; > typedef struct PCII440FXState PCII440FXState; > diff --git a/qemu-options.hx b/qemu-options.hx > index bcfe9ea..396a028 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 7511e70..4a461a2 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 no_fdc = 0; > int fd_bootchk = 1; > static int no_reboot; > int no_shutdown = 0; > @@ -3576,6 +3577,9 @@ int main(int argc, char **argv, char **envp) > case QEMU_OPTION_no_hpet: > no_hpet = 1; > break; > + case QEMU_OPTION_no_fdc: > + no_fdc = 1; > + break; > case QEMU_OPTION_balloon: > if (balloon_parse(optarg) < 0) { > fprintf(stderr, "Unknown -balloon argument %s\n", optarg); > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 3cd8f38..ba3bd3d 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1353,10 +1353,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 (!no_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 b87c6e0..b608abf 100644 > --- a/hw/i386/acpi-dsdt.dsl > +++ b/hw/i386/acpi-dsdt.dsl > @@ -110,11 +110,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 12ff544..55756d8 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -165,13 +165,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 46942c1..747c3df 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_WORD_CONST DSDT_FDC_STA > + Name(_STA, 0xFF00) I'm not sure why this is WORD. Spec says bits 0-4 have meaning bits up to 31 are cleared. So should not this be either dword (to make all 32 bits explicit) or byte (to make it small)? > 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 30bfcd2..ff0f93e 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -82,6 +82,7 @@ typedef struct AcpiMiscInfo { > static void acpi_get_dsdt(AcpiMiscInfo *info) > { > unsigned short applesmc_sta_val, *applesmc_sta_off; > + unsigned short fdc_sta_val, *fdc_sta_off; > Object *piix = piix4_pm_find(); > Object *lpc = ich9_lpc_find(); > assert(!!piix != !!lpc); > @@ -90,13 +91,20 @@ static void acpi_get_dsdt(AcpiMiscInfo *info) > info->dsdt_code = AcpiDsdtAmlCode; > info->dsdt_size = sizeof AcpiDsdtAmlCode; > applesmc_sta_off = piix_dsdt_applesmc_sta; > + fdc_sta_off = piix_dsdt_fdc_sta; > } > if (lpc) { > info->dsdt_code = Q35AcpiDsdtAmlCode; > info->dsdt_size = sizeof Q35AcpiDsdtAmlCode; > applesmc_sta_off = q35_dsdt_applesmc_sta; > + fdc_sta_off = q35_dsdt_fdc_sta; > } > > + /* Patch in appropriate value for FDC _STA */ > + fdc_sta_val = no_fdc ? 0x00 : 0x0F; > + *(uint16_t *)(info->dsdt_code + *fdc_sta_off) = > + cpu_to_le16(fdc_sta_val); > + > /* Patch in appropriate value for AppleSMC _STA */ > applesmc_sta_val = applesmc_find() ? 0x0b : 0x00; > *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) = > -- > 1.8.1.4
On Wed, Jan 08, 2014 at 10:38:21PM +0200, Michael S. Tsirkin wrote: > > + /* _STA will be patched to 0x0F if the FDC is present */ > > + ACPI_EXTRACT_NAME_WORD_CONST DSDT_FDC_STA > > + Name(_STA, 0xFF00) > > I'm not sure why this is WORD. Spec says bits 0-4 have meaning bits up > to 31 are cleared. So should not this be either dword (to make all > 32 bits explicit) or byte (to make it small)? You're right of course, _STA returns 32 bits, 5 of which are defined, and the rest are reserved and "must be cleared". As such, I think it should be DWORD (and I'm not confident enough to be sure no breakage would occur if we go "BYTE" instead :) ) Speaking of, I messed this one up in my AppleSMC patch as well. Should I resend that one in its entirety too, with DWORD, or just an incremental patch setting AppleSMC _STA to DWORD ? Thanks, --Gabriel
Il 08/01/2014 21:02, Gabriel L. Somlo ha scritto: > Add i386 command line option "-no-fdc", which allows guests to omit the > configuration of a floppy controller. Applies on top of my previous patch > titled "Add DSDT node for AppleSMC" I think we need to provide a mechanism for machine-specific options. Options like this one does not scale. Still, thanks for writing this patch which is great at least as a proof of concept and for the ACPI side of things! Have you played with the HPET device? It would be nice to have Mac OS X support in QEMU 2.0. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 08/01/2014 21:02, Gabriel L. Somlo ha scritto: >> Add i386 command line option "-no-fdc", which allows guests to omit the >> configuration of a floppy controller. Applies on top of my previous patch >> titled "Add DSDT node for AppleSMC" > > 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. It sure would be nice if -machine accepted exactly the keys that actually work. [...]
Il 09/01/2014 09:46, Markus Armbruster ha scritto: >>> >> Add i386 command line option "-no-fdc", which allows guests to omit the >>> >> configuration of a floppy controller. Applies on top of my previous patch >>> >> titled "Add DSDT node for AppleSMC" >> > >> > 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. Paolo > It sure would be nice if -machine accepted exactly the keys that > actually work.
Il 08/01/2014 22:09, Gabriel L. Somlo ha scritto: > On Wed, Jan 08, 2014 at 10:38:21PM +0200, Michael S. Tsirkin wrote: >>> > > + /* _STA will be patched to 0x0F if the FDC is present */ >>> > > + ACPI_EXTRACT_NAME_WORD_CONST DSDT_FDC_STA >>> > > + Name(_STA, 0xFF00) >> > >> > I'm not sure why this is WORD. Spec says bits 0-4 have meaning bits up >> > to 31 are cleared. So should not this be either dword (to make all >> > 32 bits explicit) or byte (to make it small)? > You're right of course, _STA returns 32 bits, 5 of which are defined, > and the rest are reserved and "must be cleared". > > As such, I think it should be DWORD (and I'm not confident enough to > be sure no breakage would occur if we go "BYTE" instead :) ) Byte should work. Internally, the AML interpreter treats all integers as either 32-bit or 64-bit depending on the version. Paolo
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 24eb3de..5226a79 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 no_fdc; + /* piix_pci.c */ struct PCII440FXState; typedef struct PCII440FXState PCII440FXState; diff --git a/qemu-options.hx b/qemu-options.hx index bcfe9ea..396a028 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 7511e70..4a461a2 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 no_fdc = 0; int fd_bootchk = 1; static int no_reboot; int no_shutdown = 0; @@ -3576,6 +3577,9 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_no_hpet: no_hpet = 1; break; + case QEMU_OPTION_no_fdc: + no_fdc = 1; + break; case QEMU_OPTION_balloon: if (balloon_parse(optarg) < 0) { fprintf(stderr, "Unknown -balloon argument %s\n", optarg); diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 3cd8f38..ba3bd3d 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1353,10 +1353,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 (!no_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 b87c6e0..b608abf 100644 --- a/hw/i386/acpi-dsdt.dsl +++ b/hw/i386/acpi-dsdt.dsl @@ -110,11 +110,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 12ff544..55756d8 100644 --- a/hw/i386/q35-acpi-dsdt.dsl +++ b/hw/i386/q35-acpi-dsdt.dsl @@ -165,13 +165,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 46942c1..747c3df 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_WORD_CONST DSDT_FDC_STA + Name(_STA, 0xFF00) 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 30bfcd2..ff0f93e 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -82,6 +82,7 @@ typedef struct AcpiMiscInfo { static void acpi_get_dsdt(AcpiMiscInfo *info) { unsigned short applesmc_sta_val, *applesmc_sta_off; + unsigned short fdc_sta_val, *fdc_sta_off; Object *piix = piix4_pm_find(); Object *lpc = ich9_lpc_find(); assert(!!piix != !!lpc); @@ -90,13 +91,20 @@ static void acpi_get_dsdt(AcpiMiscInfo *info) info->dsdt_code = AcpiDsdtAmlCode; info->dsdt_size = sizeof AcpiDsdtAmlCode; applesmc_sta_off = piix_dsdt_applesmc_sta; + fdc_sta_off = piix_dsdt_fdc_sta; } if (lpc) { info->dsdt_code = Q35AcpiDsdtAmlCode; info->dsdt_size = sizeof Q35AcpiDsdtAmlCode; applesmc_sta_off = q35_dsdt_applesmc_sta; + fdc_sta_off = q35_dsdt_fdc_sta; } + /* Patch in appropriate value for FDC _STA */ + fdc_sta_val = no_fdc ? 0x00 : 0x0F; + *(uint16_t *)(info->dsdt_code + *fdc_sta_off) = + cpu_to_le16(fdc_sta_val); + /* Patch in appropriate value for AppleSMC _STA */ applesmc_sta_val = applesmc_find() ? 0x0b : 0x00; *(uint16_t *)(info->dsdt_code + *applesmc_sta_off) =