[Qemu-devel] [PATCH v7 0/3] i386: expose floppy-related objects in SSDT

[Qemu-devel] [PATCH v7 0/3] i386: expose floppy-related objects in SSDT

[Igor Mammedov]

v6->v7:
 - rebase on top of current master to resolve conflicts
   with the latest FDC refactoring

v5->v6:
 - rebased on top DSDT converted to AMP API
 - dropped intermediate structs for one time used floppy parameters
   which simplifies code a bit.

Windows on UEFI systems is only capable of detecting the presence and
the type of floppy drives via corresponding ACPI objects.

Those objects are added in patch 5; the preceding ones pave the way to
it, by making the necessary data public and by moving the whole
floppy drive controller description into runtime-generated SSDT.


Roman Kagan (3):
  i386/acpi: make floppy controller object dynamic
  expose floppy drive geometry and CMOS type
  i386: populate floppy drive information in DSDT

 hw/block/fdc.c         | 11 +++++++
 hw/i386/acpi-build.c   | 84 +++++++++++++++++++++++++++++++++++---------------
 hw/i386/pc.c           |  2 +-
 include/hw/block/fdc.h |  2 ++
 include/hw/i386/pc.h   |  1 +
 5 files changed, 74 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v7 1/3] i386/acpi: make floppy controller object dynamic

[Igor Mammedov]

From: Roman Kagan <rkagan@virtuozzo.com>

Instead of statically declaring the floppy controller in DSDT, with its
_STA method depending on some obscure bit in the parent ISA bridge, add
the object dynamically to DSDT via AML API only when the controller is
present.

The _STA method is no longer necessary and is therefore dropped.  So are
the declarations of the fields indicating whether the contoller is
enabled.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 78758e2..2f685fb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1231,29 +1231,10 @@ static Aml *build_fdc_device_aml(void)
 {
     Aml *dev;
     Aml *crs;
-    Aml *method;
-    Aml *if_ctx;
-    Aml *else_ctx;
-    Aml *zero = aml_int(0);
-    Aml *is_present = aml_local(0);
 
     dev = aml_device("FDC0");
     aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
 
-    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_store(aml_name("FDEN"), is_present));
-    if_ctx = aml_if(aml_equal(is_present, zero));
-    {
-        aml_append(if_ctx, aml_return(aml_int(0x00)));
-    }
-    aml_append(method, if_ctx);
-    else_ctx = aml_else();
-    {
-        aml_append(else_ctx, aml_return(aml_int(0x0f)));
-    }
-    aml_append(method, else_ctx);
-    aml_append(dev, method);
-
     crs = aml_resource_template();
     aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
     aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
@@ -1411,7 +1392,9 @@ static void build_isa_devices_aml(Aml *table)
     aml_append(scope, build_rtc_device_aml());
     aml_append(scope, build_kbd_device_aml());
     aml_append(scope, build_mouse_device_aml());
-    aml_append(scope, build_fdc_device_aml());
+    if (!!pc_find_fdc0()) {
+        aml_append(scope, build_fdc_device_aml());
+    }
     aml_append(scope, build_lpt_device_aml());
     aml_append(scope, build_com_device_aml(1));
     aml_append(scope, build_com_device_aml(2));
@@ -1780,8 +1763,6 @@ static void build_q35_isa_bridge(Aml *table)
     aml_append(field, aml_named_field("COMB", 3));
     aml_append(field, aml_reserved_field(1));
     aml_append(field, aml_named_field("LPTD", 2));
-    aml_append(field, aml_reserved_field(2));
-    aml_append(field, aml_named_field("FDCD", 2));
     aml_append(dev, field);
 
     aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
@@ -1791,7 +1772,6 @@ static void build_q35_isa_bridge(Aml *table)
     aml_append(field, aml_named_field("CAEN", 1));
     aml_append(field, aml_named_field("CBEN", 1));
     aml_append(field, aml_named_field("LPEN", 1));
-    aml_append(field, aml_named_field("FDEN", 1));
     aml_append(dev, field);
 
     aml_append(scope, dev);
@@ -1839,7 +1819,6 @@ static void build_piix4_isa_bridge(Aml *table)
     aml_append(field, aml_reserved_field(3));
     aml_append(field, aml_named_field("CBEN", 1));
     aml_append(dev, field);
-    aml_append(dev, aml_name_decl("FDEN", aml_int(1)));
 
     aml_append(scope, dev);
     aml_append(table, scope);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v7 1/3] i386/acpi: make floppy controller object dynamic

[Marcel Apfelbaum]

On 01/26/2016 03:50 PM, Igor Mammedov wrote:
> From: Roman Kagan <rkagan@virtuozzo.com>
>
> Instead of statically declaring the floppy controller in DSDT, with its
> _STA method depending on some obscure bit in the parent ISA bridge, add
> the object dynamically to DSDT via AML API only when the controller is
> present.
>
> The _STA method is no longer necessary and is therefore dropped.  So are
> the declarations of the fields indicating whether the contoller is
> enabled.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/i386/acpi-build.c | 27 +++------------------------
>   1 file changed, 3 insertions(+), 24 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 78758e2..2f685fb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1231,29 +1231,10 @@ static Aml *build_fdc_device_aml(void)
>   {
>       Aml *dev;
>       Aml *crs;
> -    Aml *method;
> -    Aml *if_ctx;
> -    Aml *else_ctx;
> -    Aml *zero = aml_int(0);
> -    Aml *is_present = aml_local(0);
>
>       dev = aml_device("FDC0");
>       aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
>
> -    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -    aml_append(method, aml_store(aml_name("FDEN"), is_present));
> -    if_ctx = aml_if(aml_equal(is_present, zero));
> -    {
> -        aml_append(if_ctx, aml_return(aml_int(0x00)));
> -    }
> -    aml_append(method, if_ctx);
> -    else_ctx = aml_else();
> -    {
> -        aml_append(else_ctx, aml_return(aml_int(0x0f)));
> -    }
> -    aml_append(method, else_ctx);
> -    aml_append(dev, method);
> -
>       crs = aml_resource_template();
>       aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
>       aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
> @@ -1411,7 +1392,9 @@ static void build_isa_devices_aml(Aml *table)
>       aml_append(scope, build_rtc_device_aml());
>       aml_append(scope, build_kbd_device_aml());
>       aml_append(scope, build_mouse_device_aml());
> -    aml_append(scope, build_fdc_device_aml());
> +    if (!!pc_find_fdc0()) {

You don't really need the double negation, right?
Anyway, I agree with the patch.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

> +        aml_append(scope, build_fdc_device_aml());
> +    }
>       aml_append(scope, build_lpt_device_aml());
>       aml_append(scope, build_com_device_aml(1));
>       aml_append(scope, build_com_device_aml(2));
> @@ -1780,8 +1763,6 @@ static void build_q35_isa_bridge(Aml *table)
>       aml_append(field, aml_named_field("COMB", 3));
>       aml_append(field, aml_reserved_field(1));
>       aml_append(field, aml_named_field("LPTD", 2));
> -    aml_append(field, aml_reserved_field(2));
> -    aml_append(field, aml_named_field("FDCD", 2));
>       aml_append(dev, field);
>
>       aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
> @@ -1791,7 +1772,6 @@ static void build_q35_isa_bridge(Aml *table)
>       aml_append(field, aml_named_field("CAEN", 1));
>       aml_append(field, aml_named_field("CBEN", 1));
>       aml_append(field, aml_named_field("LPEN", 1));
> -    aml_append(field, aml_named_field("FDEN", 1));
>       aml_append(dev, field);
>
>       aml_append(scope, dev);
> @@ -1839,7 +1819,6 @@ static void build_piix4_isa_bridge(Aml *table)
>       aml_append(field, aml_reserved_field(3));
>       aml_append(field, aml_named_field("CBEN", 1));
>       aml_append(dev, field);
> -    aml_append(dev, aml_name_decl("FDEN", aml_int(1)));
>
>       aml_append(scope, dev);
>       aml_append(table, scope);
>

[Qemu-devel] [PATCH v7 2/3] expose floppy drive geometry and CMOS type

[Igor Mammedov]

From: Roman Kagan <rkagan@virtuozzo.com>

Make it possible to query the geometry and the CMOS type of a floppy
drive outside of the respective source files.

It will be useful, in particular, when dynamically building ACPI tables,
and will allow to properly populate the corresponding ACPI objects and
thus enable BIOS-less systems to access the floppy drives.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c         | 11 +++++++++++
 hw/i386/pc.c           |  2 +-
 include/hw/block/fdc.h |  2 ++
 include/hw/i386/pc.h   |  1 +
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e3b0e1e..c9d7001 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2518,6 +2518,17 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
     return isa->state.drives[i].drive;
 }
 
+void isa_fdc_get_drive_geometry(ISADevice *fdc, int i, uint8_t *cylinders,
+                                uint8_t *heads, uint8_t *sectors)
+{
+    FDCtrlISABus *isa = ISA_FDC(fdc);
+    FDrive *drv = &isa->state.drives[i];
+
+    *cylinders = drv->max_track;
+    *heads = (drv->flags & FDISK_DBL_SIDES) ? 2 : 1;
+    *sectors = drv->last_sect;
+}
+
 static const VMStateDescription vmstate_isa_fdc ={
     .name = "fdc",
     .version_id = 2,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 78cf8fa..ccd7f39 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -199,7 +199,7 @@ static void pic_irq_request(void *opaque, int irq, int level)
 
 #define REG_EQUIPMENT_BYTE          0x14
 
-static int cmos_get_fd_drive_type(FloppyDriveType fd0)
+int cmos_get_fd_drive_type(FloppyDriveType fd0)
 {
     int val;
 
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index adce14f..d87859e 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -15,5 +15,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
                        DriveInfo **fds, qemu_irq *fdc_tc);
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
+void isa_fdc_get_drive_geometry(ISADevice *fdc, int i, uint8_t *cylinders,
+                                uint8_t *heads, uint8_t *sectors);
 
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 65e8f24..ad59453 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -271,6 +271,7 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
 void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
 ISADevice *pc_find_fdc0(void);
+int cmos_get_fd_drive_type(FloppyDriveType fd0);
 
 /* acpi_piix.c */
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT

[Igor Mammedov]

From: Roman Kagan <rkagan@virtuozzo.com>

On x86-based systems Linux determines the presence
and the type of floppy drives via a query of a CMOS field.
So does SeaBIOS when populating the return data for
int 0x13 function 0x08.

However Windows doesn't do it. Instead, it requests
this information from BIOS via int 0x13/0x08 or through
ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
(Floppy Drive Information) of the floppy controller object.
On UEFI systems only ACPI-based detection is supported.

QEMU doesn't provide those objects in its ACPI tables
and as a result floppy drives aren't invisible to Windows
on UEFI/OVMF.

This patch adds those objects to the floppy controller
in DSDT, populating them with the information from
respective QEMU objects.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
fixup:
  - rebase on latest FDC refactoring
---
 hw/i386/acpi-build.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2f685fb..1d291d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/loader.h"
 #include "hw/isa/isa.h"
+#include "hw/block/fdc.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/mem/nvdimm.h"
 #include "sysemu/tpm.h"
@@ -1227,10 +1228,47 @@ static void build_hpet_aml(Aml *table)
     aml_append(table, scope);
 }
 
-static Aml *build_fdc_device_aml(void)
+static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
+                             uint8_t heads, uint8_t sectors)
+{
+    Aml *dev, *fdi;
+
+    dev = aml_device("FLP%c", 'A' + idx);
+
+    aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+    fdi = aml_package(0x10);
+    aml_append(fdi, aml_int(idx));  /* Drive Number */
+    aml_append(fdi,
+        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
+    aml_append(fdi,
+        aml_int(cylinders - 1));  /* Maximum Cylinder Number */
+    aml_append(fdi, aml_int(sectors));  /* Maximum Sector Number */
+    aml_append(fdi, aml_int(heads - 1));  /* Maximum Head Number */
+    /* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
+     * the drive type, so shall we */
+    aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
+    aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
+    aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
+    aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
+    aml_append(fdi, aml_int(0x12));  /* disk_eot */
+    aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
+    aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
+    aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
+    aml_append(fdi, aml_int(0xF6));  /* disk_fill */
+    aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
+    aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
+
+    aml_append(dev, aml_name_decl("_FDI", fdi));
+    return dev;
+}
+
+static Aml *build_fdc_device_aml(ISADevice *fdc)
 {
+    int i;
     Aml *dev;
     Aml *crs;
+    uint32_t fde_buf[5] = {0, 0, 0, 0, cpu_to_le32(0x2)};
 
     dev = aml_device("FDC0");
     aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
@@ -1243,6 +1281,21 @@ static Aml *build_fdc_device_aml(void)
         aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
     aml_append(dev, aml_name_decl("_CRS", crs));
 
+    for (i = 0; i < MAX_FD; i++) {
+        uint8_t type = isa_fdc_get_drive_type(fdc, i);
+
+        if (type < FLOPPY_DRIVE_TYPE_NONE) {
+            uint8_t cylinders, heads, sectors;
+
+            fde_buf[i] = cpu_to_le32(0x1);
+            isa_fdc_get_drive_geometry(fdc, i, &cylinders, &heads, &sectors);
+            aml_append(dev,
+                build_fdinfo_aml(i, type, cylinders, heads, sectors));
+        }
+    }
+    aml_append(dev, aml_name_decl("_FDE",
+               aml_buffer(sizeof(fde_buf), (uint8_t *) fde_buf)));
+
     return dev;
 }
 
@@ -1387,13 +1440,15 @@ static Aml *build_com_device_aml(uint8_t uid)
 
 static void build_isa_devices_aml(Aml *table)
 {
+    ISADevice *fdc = pc_find_fdc0();
+
     Aml *scope = aml_scope("_SB.PCI0.ISA");
 
     aml_append(scope, build_rtc_device_aml());
     aml_append(scope, build_kbd_device_aml());
     aml_append(scope, build_mouse_device_aml());
-    if (!!pc_find_fdc0()) {
-        aml_append(scope, build_fdc_device_aml());
+    if (!!fdc) {
+        aml_append(scope, build_fdc_device_aml(fdc));
     }
     aml_append(scope, build_lpt_device_aml());
     aml_append(scope, build_com_device_aml(1));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT

[Michael S. Tsirkin]

On Tue, Jan 26, 2016 at 02:50:25PM +0100, Igor Mammedov wrote:
> From: Roman Kagan <rkagan@virtuozzo.com>
> 
> On x86-based systems Linux determines the presence
> and the type of floppy drives via a query of a CMOS field.
> So does SeaBIOS when populating the return data for
> int 0x13 function 0x08.
> 
> However Windows doesn't do it. Instead, it requests
> this information from BIOS via int 0x13/0x08 or through
> ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
> (Floppy Drive Information) of the floppy controller object.
> On UEFI systems only ACPI-based detection is supported.
> 
> QEMU doesn't provide those objects in its ACPI tables
> and as a result floppy drives aren't invisible

are invisible

> to Windows
> on UEFI/OVMF.
> 
> This patch adds those objects to the floppy controller
> in DSDT, populating them with the information from
> respective QEMU objects.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> fixup:
>   - rebase on latest FDC refactoring
> ---
>  hw/i386/acpi-build.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2f685fb..1d291d8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,7 @@
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/loader.h"
>  #include "hw/isa/isa.h"
> +#include "hw/block/fdc.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/mem/nvdimm.h"
>  #include "sysemu/tpm.h"
> @@ -1227,10 +1228,47 @@ static void build_hpet_aml(Aml *table)
>      aml_append(table, scope);
>  }
>  
> -static Aml *build_fdc_device_aml(void)
> +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> +                             uint8_t heads, uint8_t sectors)

acpi spec says these are WORD values. Are they really uint8_t?

> +{
> +    Aml *dev, *fdi;
> +
> +    dev = aml_device("FLP%c", 'A' + idx);
> +
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
> +
> +    fdi = aml_package(0x10);

Why 0x10 and not 16?

> +    aml_append(fdi, aml_int(idx));  /* Drive Number */
> +    aml_append(fdi,
> +        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
> +    aml_append(fdi,
> +        aml_int(cylinders - 1));  /* Maximum Cylinder Number */
> +    aml_append(fdi, aml_int(sectors));  /* Maximum Sector Number */
> +    aml_append(fdi, aml_int(heads - 1));  /* Maximum Head Number */

Doesn't above change on media change?
If so this must be created dynamically, not hard-coded at machine
done type.

> +    /* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
> +     * the drive type, so shall we */

    /*
     * Format comments
     * Like this please
     */

    /* Not
     * Like this */


> +    aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
> +    aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
> +    aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
> +    aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
> +    aml_append(fdi, aml_int(0x12));  /* disk_eot */
> +    aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
> +    aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
> +    aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
> +    aml_append(fdi, aml_int(0xF6));  /* disk_fill */
> +    aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
> +    aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
> +
> +    aml_append(dev, aml_name_decl("_FDI", fdi));
> +    return dev;
> +}
> +
> +static Aml *build_fdc_device_aml(ISADevice *fdc)
>  {
> +    int i;
>      Aml *dev;
>      Aml *crs;
> +    uint32_t fde_buf[5] = {0, 0, 0, 0, cpu_to_le32(0x2)};

Pls document values.

>  
>      dev = aml_device("FDC0");
>      aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
> @@ -1243,6 +1281,21 @@ static Aml *build_fdc_device_aml(void)
>          aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
>      aml_append(dev, aml_name_decl("_CRS", crs));
>  
> +    for (i = 0; i < MAX_FD; i++) {
> +        uint8_t type = isa_fdc_get_drive_type(fdc, i);
> +
> +        if (type < FLOPPY_DRIVE_TYPE_NONE) {
> +            uint8_t cylinders, heads, sectors;
> +
> +            fde_buf[i] = cpu_to_le32(0x1);

Pls document values.

> +            isa_fdc_get_drive_geometry(fdc, i, &cylinders, &heads, &sectors);
> +            aml_append(dev,
> +                build_fdinfo_aml(i, type, cylinders, heads, sectors));
> +        }
> +    }
> +    aml_append(dev, aml_name_decl("_FDE",
> +               aml_buffer(sizeof(fde_buf), (uint8_t *) fde_buf)));

No space after cast please.

> +
>      return dev;
>  }
>  
> @@ -1387,13 +1440,15 @@ static Aml *build_com_device_aml(uint8_t uid)
>  
>  static void build_isa_devices_aml(Aml *table)
>  {
> +    ISADevice *fdc = pc_find_fdc0();
> +
>      Aml *scope = aml_scope("_SB.PCI0.ISA");
>  
>      aml_append(scope, build_rtc_device_aml());
>      aml_append(scope, build_kbd_device_aml());
>      aml_append(scope, build_mouse_device_aml());
> -    if (!!pc_find_fdc0()) {
> -        aml_append(scope, build_fdc_device_aml());
> +    if (!!fdc) {
> +        aml_append(scope, build_fdc_device_aml(fdc));
>      }
>      aml_append(scope, build_lpt_device_aml());
>      aml_append(scope, build_com_device_aml(1));
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT

[Roman Kagan]

On Sun, Feb 07, 2016 at 11:08:07AM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 26, 2016 at 02:50:25PM +0100, Igor Mammedov wrote:
> > From: Roman Kagan <rkagan@virtuozzo.com>
> > -static Aml *build_fdc_device_aml(void)
> > +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> > +                             uint8_t heads, uint8_t sectors)
> 
> acpi spec says these are WORD values. Are they really uint8_t?

Yes, see struct FDrive.

> > +    fdi = aml_package(0x10);
> 
> Why 0x10 and not 16?

I'm unaware of the difference...  If you have any preference I'll
adjust.  (Originally I stuck with hex because so did iasl -d and the
then hand-written ASL).

> > +    aml_append(fdi, aml_int(idx));  /* Drive Number */
> > +    aml_append(fdi,
> > +        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
> > +    aml_append(fdi,
> > +        aml_int(cylinders - 1));  /* Maximum Cylinder Number */
> > +    aml_append(fdi, aml_int(sectors));  /* Maximum Sector Number */
> > +    aml_append(fdi, aml_int(heads - 1));  /* Maximum Head Number */
> 
> Doesn't above change on media change?

I guess no, because this IMHO is supposed to describe the drive
properties, not the diskette properties.  But I'll double-check.

I'm confused about the status of this patchset: I saw you post a pull
request with this series last week, and now you review it.  What should
I do now in response to your comments: rework and resubmit it or post
incremental fixes on top of it?

Thanks,
Roman.

Re: [Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT

[Michael S. Tsirkin]

On Mon, Feb 08, 2016 at 04:00:37PM +0300, Roman Kagan wrote:
> On Sun, Feb 07, 2016 at 11:08:07AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 26, 2016 at 02:50:25PM +0100, Igor Mammedov wrote:
> > > From: Roman Kagan <rkagan@virtuozzo.com>
> > > -static Aml *build_fdc_device_aml(void)
> > > +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> > > +                             uint8_t heads, uint8_t sectors)
> > 
> > acpi spec says these are WORD values. Are they really uint8_t?
> 
> Yes, see struct FDrive.
> 
> > > +    fdi = aml_package(0x10);
> > 
> > Why 0x10 and not 16?
> 
> I'm unaware of the difference...  If you have any preference I'll
> adjust.  (Originally I stuck with hex because so did iasl -d and the
> then hand-written ASL).

Decimal as other packages please.

> > > +    aml_append(fdi, aml_int(idx));  /* Drive Number */
> > > +    aml_append(fdi,
> > > +        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
> > > +    aml_append(fdi,
> > > +        aml_int(cylinders - 1));  /* Maximum Cylinder Number */
> > > +    aml_append(fdi, aml_int(sectors));  /* Maximum Sector Number */
> > > +    aml_append(fdi, aml_int(heads - 1));  /* Maximum Head Number */
> > 
> > Doesn't above change on media change?
> 
> I guess no, because this IMHO is supposed to describe the drive
> properties, not the diskette properties.  But I'll double-check.

Where you take this from seems to be diskette properties though ...

> I'm confused about the status of this patchset: I saw you post a pull
> request with this series last week, and now you review it.  What should
> I do now in response to your comments: rework and resubmit it or post
> incremental fixes on top of it?
> 
> Thanks,
> Roman.

I dropped and re-did review because Igor reported XP crashing.
You should address comments and resubmit.

Thanks!

-- 
MST

Re: [Qemu-devel] [PATCH v7 3/3] i386: populate floppy drive information in DSDT

[Igor Mammedov]

On Tue, 26 Jan 2016 14:50:25 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> From: Roman Kagan <rkagan@virtuozzo.com>
> 
> On x86-based systems Linux determines the presence
> and the type of floppy drives via a query of a CMOS field.
> So does SeaBIOS when populating the return data for
> int 0x13 function 0x08.
> 
> However Windows doesn't do it. Instead, it requests
> this information from BIOS via int 0x13/0x08 or through
> ACPI objects _FDE (Floppy Drive Enumerate) and _FDI
> (Floppy Drive Information) of the floppy controller object.
> On UEFI systems only ACPI-based detection is supported.
> 
> QEMU doesn't provide those objects in its ACPI tables
> and as a result floppy drives aren't invisible to Windows
> on UEFI/OVMF.
> 
> This patch adds those objects to the floppy controller
> in DSDT, populating them with the information from
> respective QEMU objects.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
pls also test with booting XP guest with default SeaBIOS firmware.

> ---
> fixup:
>   - rebase on latest FDC refactoring
> ---
>  hw/i386/acpi-build.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2f685fb..1d291d8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -38,6 +38,7 @@
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/loader.h"
>  #include "hw/isa/isa.h"
> +#include "hw/block/fdc.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/mem/nvdimm.h"
>  #include "sysemu/tpm.h"
> @@ -1227,10 +1228,47 @@ static void build_hpet_aml(Aml *table)
>      aml_append(table, scope);
>  }
>  
> -static Aml *build_fdc_device_aml(void)
> +static Aml *build_fdinfo_aml(int idx, uint8_t type, uint8_t cylinders,
> +                             uint8_t heads, uint8_t sectors)
> +{
> +    Aml *dev, *fdi;
> +
> +    dev = aml_device("FLP%c", 'A' + idx);
> +
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
> +
> +    fdi = aml_package(0x10);
> +    aml_append(fdi, aml_int(idx));  /* Drive Number */
> +    aml_append(fdi,
> +        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
> +    aml_append(fdi,
> +        aml_int(cylinders - 1));  /* Maximum Cylinder Number */
int overflow shouldn't happen here, XP crashes here
because overflows causes 64bit number here which has not
supported AML opcode per ACPI1.0b spec,
Also per ACPI spec it's uint16_t type max.


> +    aml_append(fdi, aml_int(sectors));  /* Maximum Sector Number */
> +    aml_append(fdi, aml_int(heads - 1));  /* Maximum Head Number */
isa_fdc_get_drive_geometry() returns number of 'heads' either 1 or 2,
so are you sure that (heads - 1) is correct here?
The same question applies to 'cylinders'.


> +    /* SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
> +     * the drive type, so shall we */
> +    aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
> +    aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
> +    aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
> +    aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
> +    aml_append(fdi, aml_int(0x12));  /* disk_eot */
> +    aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
> +    aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
> +    aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
> +    aml_append(fdi, aml_int(0xF6));  /* disk_fill */
> +    aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
> +    aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
> +
> +    aml_append(dev, aml_name_decl("_FDI", fdi));
> +    return dev;
> +}
> +
> +static Aml *build_fdc_device_aml(ISADevice *fdc)
>  {
> +    int i;
>      Aml *dev;
>      Aml *crs;
> +    uint32_t fde_buf[5] = {0, 0, 0, 0, cpu_to_le32(0x2)};
>  
>      dev = aml_device("FDC0");
>      aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
> @@ -1243,6 +1281,21 @@ static Aml *build_fdc_device_aml(void)
>          aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
>      aml_append(dev, aml_name_decl("_CRS", crs));
>  
> +    for (i = 0; i < MAX_FD; i++) {
> +        uint8_t type = isa_fdc_get_drive_type(fdc, i);
> +
> +        if (type < FLOPPY_DRIVE_TYPE_NONE) {
> +            uint8_t cylinders, heads, sectors;
> +
> +            fde_buf[i] = cpu_to_le32(0x1);
> +            isa_fdc_get_drive_geometry(fdc, i, &cylinders, &heads, &sectors);
> +            aml_append(dev,
> +                build_fdinfo_aml(i, type, cylinders, heads, sectors));
> +        }
> +    }
> +    aml_append(dev, aml_name_decl("_FDE",
> +               aml_buffer(sizeof(fde_buf), (uint8_t *) fde_buf)));
> +
>      return dev;
>  }
>  
> @@ -1387,13 +1440,15 @@ static Aml *build_com_device_aml(uint8_t uid)
>  
>  static void build_isa_devices_aml(Aml *table)
>  {
> +    ISADevice *fdc = pc_find_fdc0();
> +
>      Aml *scope = aml_scope("_SB.PCI0.ISA");
>  
>      aml_append(scope, build_rtc_device_aml());
>      aml_append(scope, build_kbd_device_aml());
>      aml_append(scope, build_mouse_device_aml());
> -    if (!!pc_find_fdc0()) {
> -        aml_append(scope, build_fdc_device_aml());
> +    if (!!fdc) {
> +        aml_append(scope, build_fdc_device_aml(fdc));
>      }
>      aml_append(scope, build_lpt_device_aml());
>      aml_append(scope, build_com_device_aml(1));

Re: [Qemu-devel] [PATCH v7 0/3] i386: expose floppy-related objects in SSDT

[John Snow]

On 01/26/2016 08:50 AM, Igor Mammedov wrote:
> v6->v7:
>  - rebase on top of current master to resolve conflicts
>    with the latest FDC refactoring
> 
> v5->v6:
>  - rebased on top DSDT converted to AMP API
>  - dropped intermediate structs for one time used floppy parameters
>    which simplifies code a bit.
> 
> Windows on UEFI systems is only capable of detecting the presence and
> the type of floppy drives via corresponding ACPI objects.
> 
> Those objects are added in patch 5; the preceding ones pave the way to
> it, by making the necessary data public and by moving the whole
> floppy drive controller description into runtime-generated SSDT.
> 
> 
> Roman Kagan (3):
>   i386/acpi: make floppy controller object dynamic
>   expose floppy drive geometry and CMOS type
>   i386: populate floppy drive information in DSDT
> 
>  hw/block/fdc.c         | 11 +++++++
>  hw/i386/acpi-build.c   | 84 +++++++++++++++++++++++++++++++++++---------------
>  hw/i386/pc.c           |  2 +-
>  include/hw/block/fdc.h |  2 ++
>  include/hw/i386/pc.h   |  1 +
>  5 files changed, 74 insertions(+), 26 deletions(-)
> 

Who is going to be sending the PR for this series? Igor, mst, bonzini,
ehabkost?

(Sorry for perturbing the bits with the FDC series.)

--js

Re: [Qemu-devel] [PATCH v7 0/3] i386: expose floppy-related objects in SSDT

[Michael S. Tsirkin]

On Tue, Jan 26, 2016 at 02:15:26PM -0500, John Snow wrote:
> 
> 
> On 01/26/2016 08:50 AM, Igor Mammedov wrote:
> > v6->v7:
> >  - rebase on top of current master to resolve conflicts
> >    with the latest FDC refactoring
> > 
> > v5->v6:
> >  - rebased on top DSDT converted to AMP API
> >  - dropped intermediate structs for one time used floppy parameters
> >    which simplifies code a bit.
> > 
> > Windows on UEFI systems is only capable of detecting the presence and
> > the type of floppy drives via corresponding ACPI objects.
> > 
> > Those objects are added in patch 5; the preceding ones pave the way to
> > it, by making the necessary data public and by moving the whole
> > floppy drive controller description into runtime-generated SSDT.
> > 
> > 
> > Roman Kagan (3):
> >   i386/acpi: make floppy controller object dynamic
> >   expose floppy drive geometry and CMOS type
> >   i386: populate floppy drive information in DSDT
> > 
> >  hw/block/fdc.c         | 11 +++++++
> >  hw/i386/acpi-build.c   | 84 +++++++++++++++++++++++++++++++++++---------------
> >  hw/i386/pc.c           |  2 +-
> >  include/hw/block/fdc.h |  2 ++
> >  include/hw/i386/pc.h   |  1 +
> >  5 files changed, 74 insertions(+), 26 deletions(-)
> > 
> 
> Who is going to be sending the PR for this series? Igor, mst, bonzini,
> ehabkost?
> 
> (Sorry for perturbing the bits with the FDC series.)
> 
> --js

I'll merge this.

-- 
MST