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

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

[Laszlo Ersek]

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).

Sorry if I misunderstood.

Thanks
Laszlo

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org

Laszlo Ersek (4):
  i386/pc: pc_basic_device_init(): delegate FDC creation request
  i386/pc: '-drive if=floppy' should imply a board-default FDC
  i386/pc_q35: don't insist on board FDC if there's no default floppy
  i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are
    wanted

 include/hw/i386/pc.h |  1 +
 hw/i386/pc.c         |  4 +++-
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     | 12 ++++++++----
 4 files changed, 13 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/4] i386/pc: pc_basic_device_init(): delegate FDC creation request

[Laszlo Ersek]

This patch introduces no observable change, but it allows the callers of
pc_basic_device_init(), ie. pc_init1() and pc_q35_init(), to request (or
not request) the creation of the FDC explicitly.

At the moment both callers pass constant create_fdctrl=true (hence no
observable change).

Assuming a board passes create_fdctrl=false, "floppy" will be NULL on
output, and (beyond the FDC not being created) that NULL will be passed on
to pc_cmos_init(). Luckily, pc_cmos_init() already handles that case.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - reduce scope of first patch
    - rename "force_fdctrl" to "create_fdctrl", so that it reflects more
      closely that it's a request from the board code [Markus, Michael]
    - split out "floppy drive implies FDC" magic to separate patch [Markus,
      Michael]
    - drop Paolo's A-b (move it to the last patch) and drop Markus's R-b.

 include/hw/i386/pc.h | 1 +
 hw/i386/pc.c         | 3 ++-
 hw/i386/pc_piix.c    | 2 +-
 hw/i386/pc_q35.c     | 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1b35168..58a92c1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -199,6 +199,7 @@ qemu_irq *pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
+                          bool create_fdctrl,
                           ISADevice **floppy,
                           bool no_vmport,
                           uint32 hpet_irqs);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 769eb25..a49323d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1395,6 +1395,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
 
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
+                          bool create_fdctrl,
                           ISADevice **floppy,
                           bool no_vmport,
                           uint32 hpet_irqs)
@@ -1490,7 +1491,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
     for(i = 0; i < MAX_FD; i++) {
         fd[i] = drive_get(IF_FLOPPY, 0, i);
     }
-    *floppy = fdctrl_init_isa(isa_bus, fd);
+    *floppy = create_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
 }
 
 void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 212e263..9f530c4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -242,7 +242,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
 
     pc_nic_init(isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e67f2de..2411349 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -250,7 +250,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
 
     /* connect pm stuff to lpc */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/4] i386/pc: '-drive if=floppy' should imply a board-default FDC

[Laszlo Ersek]

Even if board code decides not to request the creation of the FDC (keyed
off board-level factors, to be determined later), we should create the FDC
nevertheless if the user passes '-drive if=floppy' on the command line.

Otherwise '-drive if=floppy' would break without explicit '-device
isa-fdc' on such boards.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - separate this "magic" out to a standalone patch [Markus, Michael]

 hw/i386/pc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a49323d..90fb309 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1490,6 +1490,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
 
     for(i = 0; i < MAX_FD; i++) {
         fd[i] = drive_get(IF_FLOPPY, 0, i);
+        create_fdctrl |= !!fd[i];
     }
     *floppy = create_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/4] i386/pc_q35: don't insist on board FDC if there's no default floppy

[Laszlo Ersek]

The "no_floppy = 1" machine class setting causes "default_floppy" in
main() to become zero. Consequently, default_drive() will not call
drive_add() and drive_new() for IF_FLOPPY, index=0, meaning that no
default floppy drive will be created for the virtual machine. In that
case, board code should also not insist on the creation of the
board-default FDC.

The board-default FDC will still be created if the user requests a floppy
drive with "-drive if=floppy".

Additionally, separate FDCs can be specified manually with "-device
isa-fdc". They allow the

  -device isa-fdc,driveA=...

syntax that is more flexible than the one required by the board-default
FDC:

  -global isa-fdc.driveA=...

This patch doesn't change the behavior observably, as all Q35 machine
types have "no_floppy = 0".

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - extend commit message with "-global isa-fdc.driveA=..." language
      [Markus]
    
    - set the "create_fdctrl" parameter based on MachineClass.no_floppy,
      rather than a separate compatibility knob [Markus, Paolo]
    
    - Hesitate a little bit if this patch should affect PIIX4 too, not just
      Q35. On one hand, leaving constant "true" in PIIX4 introduces a
      difference between the boards that is perhaps too "early" in a sense.
      On the other hand, wiring PIIX4 up to the machine class settings gives
      a false impression of dynamism -- the ACPI payload mentioned by
      Gabriel should be then made conditional too; plus  that extra
      flexibility won't be actually exercised in PIIX4. Ultimately, decide
      to go with Q35 only, since that's the ultimate target here, and the
      patch modifies board code after all.

 hw/i386/pc_q35.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 2411349..ad014e7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -89,6 +89,7 @@ static void pc_q35_init(MachineState *machine)
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
     DriveInfo *hd[MAX_SATA_PORTS];
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -163,7 +164,6 @@ static void pc_q35_init(MachineState *machine)
     guest_info->legacy_acpi_table_size = 0;
 
     if (smbios_defaults) {
-        MachineClass *mc = MACHINE_GET_CLASS(machine);
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                             mc->name, smbios_legacy_mode, smbios_uuid_encoded);
@@ -250,7 +250,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, !mc->no_floppy, &floppy,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
 
     /* connect pm stuff to lpc */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/4] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Laszlo Ersek]

It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
Machine type.

Hence, let us not instantiate the FDC when all of the following apply:
- the machine type is pc-q35-2.4 or later,
- "-device isa-fdc" is not passed on the command line (nor in the config
  file),
- no "-drive if=floppy,..." is requested.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---

Notes:
    v2:
    - flip no_floppy machine class setting in a separate patch

 hw/i386/pc_q35.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ad014e7..671ae69 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -424,7 +424,8 @@ static void pc_q35_init_1_4(MachineState *machine)
 #define PC_Q35_2_4_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
     .default_machine_opts = "firmware=bios-256k.bin",   \
-    .default_display = "std"
+    .default_display = "std",                           \
+    .no_floppy = 1
 
 static QEMUMachine pc_q35_machine_v2_4 = {
     PC_Q35_2_4_MACHINE_OPTIONS,
@@ -433,7 +434,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
     .init = pc_q35_init,
 };
 
-#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
+#define PC_Q35_2_3_MACHINE_OPTIONS                      \
+    PC_Q35_MACHINE_OPTIONS,                             \
+    .default_machine_opts = "firmware=bios-256k.bin",   \
+    .default_display = "std"
 
 static QEMUMachine pc_q35_machine_v2_3 = {
     PC_Q35_2_3_MACHINE_OPTIONS,
-- 
1.8.3.1

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
             }
         }
     }

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

[Laszlo Ersek]

On 05/28/15 23:08, Gabriel L. Somlo wrote:
> 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).

The _STA method will return 0 if the FDEN field is zero.

In the value returned by _STA (which is a bitmask), bit 0 is "Set if the
device is present.". Since FDEN==0 implies that this bit in the retval
of _STA will be zero, we can conclude that FDEN==0 implies that "the
FDC0 device is absent".

So presenting the payload is not a problem; when OSPM evaluates _STA, it
will see that the device doesn't exist.

The question is if FDEN is zero under these circumstances.

The LPCE operation region overlays the PCI config space of the "PCI
D31:f0 LPC ISA bridge" device -- see the _ADR object --, starting at
offset 0x82 in the config space, and continuing for 2 bytes.

Within this region, FDEN denotes bit#3 of the byte at the lowest address.

(The bit offset that FDEN corresponds to, and the _ADR object, are not
visible in the context that you quoted, but they can be seen in
"hw/i386/q35-acpi-dsdt.dsl".)

In "hw/isa/lpc_ich9.c", function ich9_lpc_machine_ready(), we have:

    if (memory_region_present(io_as, 0x3f0)) {
        /* floppy */
        pci_conf[0x82] |= 0x08;
    }

That is, the FDEN bit will evaluate to 1 in the _STA method only if the
above memory_region_present() call returned "true" at machine startup.

That IO port is set up in the following call chain:

pc_basic_device_init()                        [hw/i386/pc.c]
  fdctrl_init_isa()                           [hw/block/fdc.c]
    qdev_init_nofail()                        [hw/core/qdev.c]
      ...
        isabus_fdc_realize()                  [hw/block/fdc.c]
          // iobase = 0x3f0 comes from
          // "isa_fdc_properties"
          isa_register_portio_list()          [hw/isa/isa-bus.c]
            portio_list_add()                 [ioport.c]
              portio_list_add_1()             [ioport.c]
                memory_region_init_io()       [memory.c]
                memory_region_add_subregion() [memory.c]

This patch series prevents the

  pc_basic_device_init() --> fdctrl_init_isa()

call at the top, under the right circumstances.

Hence \_SB.PCI0.ISA.FDC0._STA() will report "device absent".

(I'm just repeating Gerd's earlier explanation, with more details.)

Thanks
Laszlo

> 
> 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
>              }
>          }
>      }
> 

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 11:50:28PM +0200, Laszlo Ersek wrote:
> >         ...
> > 
> > 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).
> 
> The _STA method will return 0 if the FDEN field is zero.
> 
> In the value returned by _STA (which is a bitmask), bit 0 is "Set if the
> device is present.". Since FDEN==0 implies that this bit in the retval
> of _STA will be zero, we can conclude that FDEN==0 implies that "the
> FDC0 device is absent".
> 
> So presenting the payload is not a problem; when OSPM evaluates _STA, it
> will see that the device doesn't exist.
> 
> The question is if FDEN is zero under these circumstances.
> 
> The LPCE operation region overlays the PCI config space of the "PCI
> D31:f0 LPC ISA bridge" device -- see the _ADR object --, starting at
> offset 0x82 in the config space, and continuing for 2 bytes.
> 
> Within this region, FDEN denotes bit#3 of the byte at the lowest address.
> 
> (The bit offset that FDEN corresponds to, and the _ADR object, are not
> visible in the context that you quoted, but they can be seen in
> "hw/i386/q35-acpi-dsdt.dsl".)
> 
> In "hw/isa/lpc_ich9.c", function ich9_lpc_machine_ready(), we have:
> 
>     if (memory_region_present(io_as, 0x3f0)) {
>         /* floppy */
>         pci_conf[0x82] |= 0x08;
>     }
> 
> That is, the FDEN bit will evaluate to 1 in the _STA method only if the
> above memory_region_present() call returned "true" at machine startup.
> 
> That IO port is set up in the following call chain:
> 
> pc_basic_device_init()                        [hw/i386/pc.c]
>   fdctrl_init_isa()                           [hw/block/fdc.c]
>     qdev_init_nofail()                        [hw/core/qdev.c]
>       ...
>         isabus_fdc_realize()                  [hw/block/fdc.c]
>           // iobase = 0x3f0 comes from
>           // "isa_fdc_properties"
>           isa_register_portio_list()          [hw/isa/isa-bus.c]
>             portio_list_add()                 [ioport.c]
>               portio_list_add_1()             [ioport.c]
>                 memory_region_init_io()       [memory.c]
>                 memory_region_add_subregion() [memory.c]
> 
> This patch series prevents the
> 
>   pc_basic_device_init() --> fdctrl_init_isa()
> 
> call at the top, under the right circumstances.
> 
> Hence \_SB.PCI0.ISA.FDC0._STA() will report "device absent".
> 
> (I'm just repeating Gerd's earlier explanation, with more details.)

Thanks (to both of you) for explaining. I was missing how FDEN was
being turned on/off, but now I get it.

I guess having FDC0 always present in the DSDT (with a conditional
_STA) vs. programmatically inserting it wholesale is just a matter of
aesthetics, and not worth worrying about (although being able to see
an explicit decision on whether or not to insert the entire aml blob
would arguably make it easier on inquisitive n00bs like myself) :P

Thanks again,
--Gabriel

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

[Laszlo Ersek]

On 05/29/15 01:53, Gabriel L. Somlo wrote:
> On Thu, May 28, 2015 at 11:50:28PM +0200, Laszlo Ersek wrote:
>>>         ...
>>>
>>> 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).
>>
>> The _STA method will return 0 if the FDEN field is zero.
>>
>> In the value returned by _STA (which is a bitmask), bit 0 is "Set if the
>> device is present.". Since FDEN==0 implies that this bit in the retval
>> of _STA will be zero, we can conclude that FDEN==0 implies that "the
>> FDC0 device is absent".
>>
>> So presenting the payload is not a problem; when OSPM evaluates _STA, it
>> will see that the device doesn't exist.
>>
>> The question is if FDEN is zero under these circumstances.
>>
>> The LPCE operation region overlays the PCI config space of the "PCI
>> D31:f0 LPC ISA bridge" device -- see the _ADR object --, starting at
>> offset 0x82 in the config space, and continuing for 2 bytes.
>>
>> Within this region, FDEN denotes bit#3 of the byte at the lowest address.
>>
>> (The bit offset that FDEN corresponds to, and the _ADR object, are not
>> visible in the context that you quoted, but they can be seen in
>> "hw/i386/q35-acpi-dsdt.dsl".)
>>
>> In "hw/isa/lpc_ich9.c", function ich9_lpc_machine_ready(), we have:
>>
>>     if (memory_region_present(io_as, 0x3f0)) {
>>         /* floppy */
>>         pci_conf[0x82] |= 0x08;
>>     }
>>
>> That is, the FDEN bit will evaluate to 1 in the _STA method only if the
>> above memory_region_present() call returned "true" at machine startup.
>>
>> That IO port is set up in the following call chain:
>>
>> pc_basic_device_init()                        [hw/i386/pc.c]
>>   fdctrl_init_isa()                           [hw/block/fdc.c]
>>     qdev_init_nofail()                        [hw/core/qdev.c]
>>       ...
>>         isabus_fdc_realize()                  [hw/block/fdc.c]
>>           // iobase = 0x3f0 comes from
>>           // "isa_fdc_properties"
>>           isa_register_portio_list()          [hw/isa/isa-bus.c]
>>             portio_list_add()                 [ioport.c]
>>               portio_list_add_1()             [ioport.c]
>>                 memory_region_init_io()       [memory.c]
>>                 memory_region_add_subregion() [memory.c]
>>
>> This patch series prevents the
>>
>>   pc_basic_device_init() --> fdctrl_init_isa()
>>
>> call at the top, under the right circumstances.
>>
>> Hence \_SB.PCI0.ISA.FDC0._STA() will report "device absent".
>>
>> (I'm just repeating Gerd's earlier explanation, with more details.)
> 
> Thanks (to both of you) for explaining. I was missing how FDEN was
> being turned on/off, but now I get it.
> 
> I guess having FDC0 always present in the DSDT (with a conditional
> _STA) vs. programmatically inserting it wholesale is just a matter of
> aesthetics, and not worth worrying about (although being able to see
> an explicit decision on whether or not to insert the entire aml blob
> would arguably make it easier on inquisitive n00bs like myself) :P

Well, it seems like pvpanic for example follows your preferred approach,
so as far as I'm concerned, feel free to champion this cause.
Preferably, *after* this series goes in. ;)

Cheers
Laszlo

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> 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).
>
> Sorry if I misunderstood.

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>