Re: [Qemu-devel] [PATCH] Add option to disable FDC from ISA bus and ACPI on i386

[Michael S. Tsirkin]

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 <address@hidden>
> ---
> 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
> address@hidden -no-fdc
> address@hidden -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