Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+

[Gabriel L. Somlo]

On Thu, May 28, 2015 at 10:04:07PM +0200, Laszlo Ersek wrote:
> Version 2 of
> <http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05640.html>.
> 
> Changes are broken out per-patch; the cumulative changes are:
> - more granular structure (several patches in place of 1),
> - rename "force_fdctrl" parameter to "create_fdctrl",
> - drop the separate compat knob "force_fdctrl", use the "no_floppy"
>   machine class setting in its stead (in inverse meaning).
> 
> I didn't touch ACPI bits (raised by Gabriel) because I got the
> impression that they are
> - alright on PIIX4 (which sees no change in this series), and
> - already handled correctly / dynamically on Q35 (an independent issue
>   was discovered but Gerd took that on, thanks).

With your patches, I started an F21 guest like so:

bin/qemu-system-x86_64 -machine q35,accel=kvm -m 2048 -monitor stdio -device 
ide-drive,bus=ide.2,drive=CD -drive 
id=CD,if=none,snapshot=on,file=/home/somlo/Downloads/Iso/Fedora-Live-Workstation-x86_64-21-5.iso

I then installed acpica-tools, and dumped out the DSDT
(acpidump -b; iasl -d dsdt.dat)

Looking at dsdt.dsl, I found this:

    ...
    Scope (_SB.PCI0)
    {
        Device (ISA)
        {
            ...
            OperationRegion (LPCE, PCI_Config, 0x82, 0x02)
            Field (LPCE, AnyAcc, NoLock, Preserve)
            {
                ...
                FDEN,   1
            }
        }
    }

    Scope (_SB.PCI0.ISA)
    {
        ...
        Device (FDC0)
        {
            Name (_HID, EisaId ("PNP0700"))  // _HID: Hardware ID
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Local0 = FDEN /* \_SB_.PCI0.ISA_.FDEN */
                If ((Local0 == Zero))
                {
                    Return (Zero)
                }
                Else
                {
                    Return (0x0F)
                }
            }

            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x03F2,             // Range Minimum
                    0x03F2,             // Range Maximum
                    0x00,               // Alignment
                    0x04,               // Length
                    )
                IO (Decode16,
                    0x03F7,             // Range Minimum
                    0x03F7,             // Range Maximum
                    0x00,               // Alignment
                    0x01,               // Length
                    )
                IRQNoFlags ()
                    {6}
                DMA (Compatibility, NotBusMaster, Transfer8, )
                    {2}
            })
        }
        ...

I have no idea how big of a deal this is (i.e. how "wrong" it is for
this stuff to still be showing up when no FDC is provisioned on the
guest).

In any case, the patch below adds a FDC0 node (to the ssdt rather than
the dsdt -- I used the applesmc as a source of inspiration) to acpi
only if one is actually configured on the system.

I had to add an "aml_dma()" function first (that's the first two
diff's in the patch), then I'm programmatically and conditionally
adding AML for the FDC0 node (next two diff blocks), and lastly I'm
removing the hardcoded node from the .dsl files.

I can split these out properly and send real patches, but wanted to
give you all a chance to talk me down, in case I'm still missing the
point somehow :)

Thanks much,
--Gabriel


diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 3947201..93e4244 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -173,6 +173,7 @@ Aml *aml_io(AmlIODecode dec, uint16_t min_base, uint16_t 
max_base,
 Aml *aml_operation_region(const char *name, AmlRegionSpace rs,
                           uint32_t offset, uint32_t len);
 Aml *aml_irq_no_flags(uint8_t irq);
+Aml *aml_dma(uint8_t type, bool is_bus_master, uint8_t size, uint8_t channel);
 Aml *aml_named_field(const char *name, unsigned length);
 Aml *aml_reserved_field(unsigned length);
 Aml *aml_local(int num);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 77ce00b..f3380dd 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -542,6 +542,25 @@ Aml *aml_irq_no_flags(uint8_t irq)
     return var;
 }
 
+Aml *aml_dma(uint8_t type, bool is_bus_master, uint8_t size, uint8_t channel)
+{
+    uint8_t dma_mask;
+    Aml *var = aml_alloc();
+
+    assert(type < 4);
+    assert(size < 4);
+    assert(channel < 8);
+    build_append_byte(var->buf, 0x2A); /* DMA descriptor */
+
+    dma_mask = 1U << channel;
+    build_append_byte(var->buf, dma_mask);
+
+    build_append_byte(var->buf, (type << 5) |
+                                is_bus_master ? 1U << 2 : 0 |
+                                size);
+    return var;
+}
+
 /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLEqual */
 Aml *aml_equal(Aml *arg1, Aml *arg2)
 {
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index d48b2f8..f82d3e6 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -14,6 +14,17 @@ typedef enum FDriveType {
 } FDriveType;
 
 #define TYPE_ISA_FDC "isa-fdc"
+#define ISA_FDC_PROP_IO_BASE "iobase"
+
+static inline uint16_t isafdc_port(void)
+{
+    Object *obj = object_resolve_path_type("", TYPE_ISA_FDC, NULL);
+
+    if (obj) {
+        return object_property_get_int(obj, ISA_FDC_PROP_IO_BASE, NULL);
+    }
+    return 0;
+}
 
 ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
 void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 73259e7..cfa275f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -111,6 +111,7 @@ typedef struct AcpiMiscInfo {
     unsigned dsdt_size;
     uint16_t pvpanic_port;
     uint16_t applesmc_io_base;
+    uint16_t isafdc_io_base;
 } AcpiMiscInfo;
 
 typedef struct AcpiBuildPciBusHotplugState {
@@ -237,6 +238,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
     info->has_tpm = tpm_find();
     info->pvpanic_port = pvpanic_port();
     info->applesmc_io_base = applesmc_port();
+    info->isafdc_io_base = isafdc_port();
 }
 
 static void acpi_get_pci_info(PcPciInfo *info)
@@ -730,6 +732,30 @@ build_ssdt(GArray *table_data, GArray *linker,
         aml_append(ssdt, scope);
     }
 
+    if (misc->isafdc_io_base) {
+        scope = aml_scope("\\_SB.PCI0.ISA");
+        dev = aml_device("FDC0");
+
+        aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
+        /* device present, functioning, decoding, shown in UI */
+        aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+
+        crs = aml_resource_template();
+        aml_append(crs,
+            aml_io(aml_decode16, misc->isafdc_io_base, misc->isafdc_io_base,
+                   0x00, 0x04)
+        );
+        aml_append(crs,
+            aml_io(aml_decode16, 0x03F7, 0x03F7, 0x00, 0x01)
+        );
+        aml_append(crs, aml_irq_no_flags(6));
+        aml_append(crs, aml_dma(0, false, 0, 2));
+        aml_append(dev, aml_name_decl("_CRS", crs));
+
+        aml_append(scope, dev);
+        aml_append(ssdt, scope);
+    }
+
     if (misc->pvpanic_port) {
         scope = aml_scope("\\_SB.PCI0.ISA");
 
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 89caa16..061507d 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -47,24 +47,6 @@ 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)
-            }
-        }
-        Name(_CRS, ResourceTemplate() {
-            IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
-            IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
-            IRQNoFlags() { 6 }
-            DMA(Compatibility, NotBusMaster, Transfer8) { 2 }
-        })
-    }
-
     Device(LPT) {
         Name(_HID, EisaId("PNP0400"))
         Method(_STA, 0, NotSerialized) {
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a2d84ec..81864d5 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -81,7 +81,6 @@ DefinitionBlock (
                 , 3,
                 CBEN, 1,         // COM2
             }
-            Name(FDEN, 1)
         }
     }
 
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 16eaca3..1645a16 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -144,8 +144,7 @@ DefinitionBlock (
             Field(LPCE, AnyAcc, NoLock, Preserve) {
                 CAEN,   1,
                 CBEN,   1,
-                LPEN,   1,
-                FDEN,   1
+                LPEN,   1
             }
         }
     }