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

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

[Roman Kagan]

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 4; 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 (4):
  i386/acpi: make floppy controller object dynamic
  i386: expose floppy drive CMOS type
  fdc: add function to determine drive chs limits
  i386: populate floppy drive information in DSDT

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
---
changes since v7:
 - rebased to latest master
 - use drive max c,h,s rather than the current diskette geometry

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

-- 
2.5.0

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

[Roman Kagan]

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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
---
changes since v7:
 - remove unnecessary double negation (cosmetic so keeping s-o-b)

 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 4554eb8..0213ccf 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);
-- 
2.5.0

[Qemu-devel] [PATCH v8 2/4] i386: expose floppy drive CMOS type

[Roman Kagan]

Make it possible to query the CMOS type of a floppy drive outside of the
source file where it's defined.

It will allow to properly populate the corresponding ACPI objects and
thus enable Windows on BIOS-less systems to access the floppy drives.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
---
changes since v7:
- split out from a bigger patch

 hw/i386/pc.c         | 2 +-
 include/hw/i386/pc.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0aeefd2..d7fea61 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -200,7 +200,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/i386/pc.h b/include/hw/i386/pc.h
index 8b3546e..472754c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -265,6 +265,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 */
 
-- 
2.5.0

[Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

[Roman Kagan]

When populating ACPI objects for floppy drives one needs to provide the
maximum values for cylinder, sector, and head number the drive supports.

This patch adds a function that iterates through the array of predefined
floppy drive formats and returns the maximum values of c, h, s, out of
those matching the given floppy drive type.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
---
changes since v7:
 - use drive max c,h,s rather than the current diskette geometry

 hw/block/fdc.c         | 23 +++++++++++++++++++++++
 include/hw/block/fdc.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9838d21..fc3aef9 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
     return isa->state.drives[i].drive;
 }
 
+void isa_fdc_get_drive_max_chs(FloppyDriveType type,
+                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
+{
+    const FDFormat *fdf;
+
+    *maxc = *maxh = *maxs = 0;
+    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
+        if (fdf->drive != type) {
+            continue;
+        }
+        if (*maxc < fdf->max_track) {
+            *maxc = fdf->max_track;
+        }
+        if (*maxh < fdf->max_head) {
+            *maxh = fdf->max_head;
+        }
+        if (*maxs < fdf->last_sect) {
+            *maxs = fdf->last_sect;
+        }
+    }
+    (*maxc)--;
+}
+
 static const VMStateDescription vmstate_isa_fdc ={
     .name = "fdc",
     .version_id = 2,
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index adce14f..1749dab 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_max_chs(FloppyDriveType type,
+                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs);
 
 #endif
-- 
2.5.0

[Qemu-devel] [PATCH v8 4/4] i386: populate floppy drive information in DSDT

[Roman Kagan]

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 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>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
---
changes since v7:
 - use drive max c,h,s rather than the current diskette geometry
 - better document the changes in the comments

 hw/i386/acpi-build.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0213ccf..02bffb2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -37,6 +37,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,11 +1228,60 @@ 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, FloppyDriveType type)
 {
+    Aml *dev, *fdi;
+    uint8_t maxc, maxh, maxs;
+
+    isa_fdc_get_drive_max_chs(type, &maxc, &maxh, &maxs);
+
+    dev = aml_device("FLP%c", 'A' + idx);
+
+    aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
+
+    fdi = aml_package(16);
+    aml_append(fdi, aml_int(idx));  /* Drive Number */
+    aml_append(fdi,
+        aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
+    /*
+     * the values below are the limits of the drive, and are thus independent
+     * of the inserted media
+     */
+    aml_append(fdi, aml_int(maxc));  /* Maximum Cylinder Number */
+    aml_append(fdi, aml_int(maxs));  /* Maximum Sector Number */
+    aml_append(fdi, aml_int(maxh));  /* 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;
 
+#define ACPI_FDE_MAX_FD 4
+    uint32_t fde_buf[5] = {
+        0, 0, 0, 0,     /* presence of floppy drives #0 - #3 */
+        cpu_to_le32(2)  /* tape presence (2 == never present) */
+    };
+
     dev = aml_device("FDC0");
     aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
 
@@ -1243,6 +1293,17 @@ 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 < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
+        FloppyDriveType type = isa_fdc_get_drive_type(fdc, i);
+
+        if (type < FLOPPY_DRIVE_TYPE_NONE) {
+            fde_buf[i] = cpu_to_le32(1);  /* drive present */
+            aml_append(dev, build_fdinfo_aml(i, type));
+        }
+    }
+    aml_append(dev, aml_name_decl("_FDE",
+               aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
+
     return dev;
 }
 
@@ -1387,13 +1448,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));
-- 
2.5.0

Re: [Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

[Michael S. Tsirkin]

On Wed, Feb 17, 2016 at 09:25:32PM +0300, Roman Kagan wrote:
> When populating ACPI objects for floppy drives one needs to provide the
> maximum values for cylinder, sector, and head number the drive supports.
> 
> This patch adds a function that iterates through the array of predefined
> floppy drive formats and returns the maximum values of c, h, s, out of
> those matching the given floppy drive type.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Kevin O'Connor <kevin@koconnor.net>
> ---
> changes since v7:
>  - use drive max c,h,s rather than the current diskette geometry
> 
>  hw/block/fdc.c         | 23 +++++++++++++++++++++++
>  include/hw/block/fdc.h |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 9838d21..fc3aef9 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
>      return isa->state.drives[i].drive;
>  }
>  
> +void isa_fdc_get_drive_max_chs(FloppyDriveType type,
> +                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
> +{
> +    const FDFormat *fdf;
> +
> +    *maxc = *maxh = *maxs = 0;
> +    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
> +        if (fdf->drive != type) {
> +            continue;
> +        }

Hmm. How does this interact with the fallback/autodetect thing?

I understand what it does rather vaguely.

I wonder whether we can just ignore the type and take
global maximum in all cases.

> +        if (*maxc < fdf->max_track) {
> +            *maxc = fdf->max_track;
> +        }
> +        if (*maxh < fdf->max_head) {
> +            *maxh = fdf->max_head;
> +        }
> +        if (*maxs < fdf->last_sect) {
> +            *maxs = fdf->last_sect;
> +        }
> +    }
> +    (*maxc)--;

Why not just *maxc = fdf->max_track - 1 above?

> +}
> +
>  static const VMStateDescription vmstate_isa_fdc ={
>      .name = "fdc",
>      .version_id = 2,
> diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
> index adce14f..1749dab 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_max_chs(FloppyDriveType type,
> +                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs);
>  
>  #endif
> -- 
> 2.5.0

Re: [Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

[Roman Kagan]

On Wed, Feb 17, 2016 at 10:15:32PM +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 17, 2016 at 09:25:32PM +0300, Roman Kagan wrote:
> > When populating ACPI objects for floppy drives one needs to provide the
> > maximum values for cylinder, sector, and head number the drive supports.
> > 
> > This patch adds a function that iterates through the array of predefined
> > floppy drive formats and returns the maximum values of c, h, s, out of
> > those matching the given floppy drive type.
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > Cc: John Snow <jsnow@redhat.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Kevin O'Connor <kevin@koconnor.net>
> > ---
> > changes since v7:
> >  - use drive max c,h,s rather than the current diskette geometry
> > 
> >  hw/block/fdc.c         | 23 +++++++++++++++++++++++
> >  include/hw/block/fdc.h |  2 ++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index 9838d21..fc3aef9 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
> >      return isa->state.drives[i].drive;
> >  }
> >  
> > +void isa_fdc_get_drive_max_chs(FloppyDriveType type,
> > +                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
> > +{
> > +    const FDFormat *fdf;
> > +
> > +    *maxc = *maxh = *maxs = 0;
> > +    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
> > +        if (fdf->drive != type) {
> > +            continue;
> > +        }
> 
> Hmm. How does this interact with the fallback/autodetect thing?

AFAICS the drive type is chosen at realize time once and for good, based
on the fdtype property, the size of the inserted image (autodetect), and
the fallback property.

So basically there's no interaction: this function only determines the
max c,h,s among all the formats of that type.

> I wonder whether we can just ignore the type and take
> global maximum in all cases.

There's non-zero chance that we can; however it's up to the
closed-source windows driver and can only be proven by testing relevant
image sizes across relevant windows versions.  OTOH the resulting code
wouldn't differ too much from this patch.  The patch also appears closer
to the definition in the ACPI spec, and I tested it already, so I'd be
reluctant to invest more effort into it.

> > +        if (*maxc < fdf->max_track) {
> > +            *maxc = fdf->max_track;
> > +        }
> > +        if (*maxh < fdf->max_head) {
> > +            *maxh = fdf->max_head;
> > +        }
> > +        if (*maxs < fdf->last_sect) {
> > +            *maxs = fdf->last_sect;
> > +        }
> > +    }
> > +    (*maxc)--;
> 
> Why not just *maxc = fdf->max_track - 1 above?

No particular reason, I just thought it was more readable to not mess
with - 1 when searching for the maximum.  I don't care either way, can
rewrite if you want me to.

Roman.

Re: [Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

[Michael S. Tsirkin]

On Thu, Feb 18, 2016 at 12:50:51PM +0300, Roman Kagan wrote:
> On Wed, Feb 17, 2016 at 10:15:32PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Feb 17, 2016 at 09:25:32PM +0300, Roman Kagan wrote:
> > > When populating ACPI objects for floppy drives one needs to provide the
> > > maximum values for cylinder, sector, and head number the drive supports.
> > > 
> > > This patch adds a function that iterates through the array of predefined
> > > floppy drive formats and returns the maximum values of c, h, s, out of
> > > those matching the given floppy drive type.
> > > 
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > > Cc: John Snow <jsnow@redhat.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Kevin O'Connor <kevin@koconnor.net>
> > > ---
> > > changes since v7:
> > >  - use drive max c,h,s rather than the current diskette geometry
> > > 
> > >  hw/block/fdc.c         | 23 +++++++++++++++++++++++
> > >  include/hw/block/fdc.h |  2 ++
> > >  2 files changed, 25 insertions(+)
> > > 
> > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > > index 9838d21..fc3aef9 100644
> > > --- a/hw/block/fdc.c
> > > +++ b/hw/block/fdc.c
> > > @@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
> > >      return isa->state.drives[i].drive;
> > >  }
> > >  
> > > +void isa_fdc_get_drive_max_chs(FloppyDriveType type,
> > > +                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
> > > +{
> > > +    const FDFormat *fdf;
> > > +
> > > +    *maxc = *maxh = *maxs = 0;
> > > +    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
> > > +        if (fdf->drive != type) {
> > > +            continue;
> > > +        }
> > 
> > Hmm. How does this interact with the fallback/autodetect thing?
> 
> AFAICS the drive type is chosen at realize time once and for good, based
> on the fdtype property, the size of the inserted image (autodetect), and
> the fallback property.
> 
> So basically there's no interaction: this function only determines the
> max c,h,s among all the formats of that type.
> 
> > I wonder whether we can just ignore the type and take
> > global maximum in all cases.
> 
> There's non-zero chance that we can; however it's up to the
> closed-source windows driver and can only be proven by testing relevant
> image sizes across relevant windows versions.  OTOH the resulting code
> wouldn't differ too much from this patch.  The patch also appears closer
> to the definition in the ACPI spec, and I tested it already, so I'd be
> reluctant to invest more effort into it.

Whatever jsnow says is right is fine by me.

> > > +        if (*maxc < fdf->max_track) {
> > > +            *maxc = fdf->max_track;
> > > +        }
> > > +        if (*maxh < fdf->max_head) {
> > > +            *maxh = fdf->max_head;
> > > +        }
> > > +        if (*maxs < fdf->last_sect) {
> > > +            *maxs = fdf->last_sect;
> > > +        }
> > > +    }
> > > +    (*maxc)--;
> > 
> > Why not just *maxc = fdf->max_track - 1 above?
> 
> No particular reason, I just thought it was more readable to not mess
> with - 1 when searching for the maximum.  I don't care either way, can
> rewrite if you want me to.
> 
> Roman.

It's confusing IMHO - also, if there's an error and you find nothing,
you get 0xFF instead of 0x0.

-- 
MST

Re: [Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

[John Snow]

On 02/18/2016 05:01 AM, Michael S. Tsirkin wrote:
> On Thu, Feb 18, 2016 at 12:50:51PM +0300, Roman Kagan wrote:
>> On Wed, Feb 17, 2016 at 10:15:32PM +0200, Michael S. Tsirkin wrote:
>>> On Wed, Feb 17, 2016 at 09:25:32PM +0300, Roman Kagan wrote:
>>>> When populating ACPI objects for floppy drives one needs to provide the
>>>> maximum values for cylinder, sector, and head number the drive supports.
>>>>
>>>> This patch adds a function that iterates through the array of predefined
>>>> floppy drive formats and returns the maximum values of c, h, s, out of
>>>> those matching the given floppy drive type.
>>>>
>>>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>> Cc: John Snow <jsnow@redhat.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Kevin O'Connor <kevin@koconnor.net>
>>>> ---
>>>> changes since v7:
>>>>  - use drive max c,h,s rather than the current diskette geometry
>>>>
>>>>  hw/block/fdc.c         | 23 +++++++++++++++++++++++
>>>>  include/hw/block/fdc.h |  2 ++
>>>>  2 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index 9838d21..fc3aef9 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
>>>>      return isa->state.drives[i].drive;
>>>>  }
>>>>  
>>>> +void isa_fdc_get_drive_max_chs(FloppyDriveType type,
>>>> +                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
>>>> +{
>>>> +    const FDFormat *fdf;
>>>> +
>>>> +    *maxc = *maxh = *maxs = 0;
>>>> +    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
>>>> +        if (fdf->drive != type) {
>>>> +            continue;
>>>> +        }
>>>
>>> Hmm. How does this interact with the fallback/autodetect thing?
>>
>> AFAICS the drive type is chosen at realize time once and for good, based
>> on the fdtype property, the size of the inserted image (autodetect), and
>> the fallback property.
>>
>> So basically there's no interaction: this function only determines the
>> max c,h,s among all the formats of that type.
>>
>>> I wonder whether we can just ignore the type and take
>>> global maximum in all cases.
>>
>> There's non-zero chance that we can; however it's up to the
>> closed-source windows driver and can only be proven by testing relevant
>> image sizes across relevant windows versions.  OTOH the resulting code
>> wouldn't differ too much from this patch.  The patch also appears closer
>> to the definition in the ACPI spec, and I tested it already, so I'd be
>> reluctant to invest more effort into it.
> 
> Whatever jsnow says is right is fine by me.
> 

What is right was not always clear :)

However, the interpretation that this should be the maximum supported
value seems correct, and the function looks mechanically correct.

Style shedding aside;

Reviewed-by: John Snow <jsnow@redhat.com>

>>>> +        if (*maxc < fdf->max_track) {
>>>> +            *maxc = fdf->max_track;
>>>> +        }
>>>> +        if (*maxh < fdf->max_head) {
>>>> +            *maxh = fdf->max_head;
>>>> +        }
>>>> +        if (*maxs < fdf->last_sect) {
>>>> +            *maxs = fdf->last_sect;
>>>> +        }
>>>> +    }
>>>> +    (*maxc)--;
>>>
>>> Why not just *maxc = fdf->max_track - 1 above?
>>
>> No particular reason, I just thought it was more readable to not mess
>> with - 1 when searching for the maximum.  I don't care either way, can
>> rewrite if you want me to.
>>
>> Roman.
> 
> It's confusing IMHO - also, if there's an error and you find nothing,
> you get 0xFF instead of 0x0.
> 

This function SHOULD always find something. If it doesn't, it means the
fdformat table has no entries for our given FDrive type, which should be
an assert during the boot process over in pick_drive_type and children,
so we should be safe.

--js

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

[Denis V. Lunev]

On 02/17/2016 09:25 PM, Roman Kagan wrote:
> 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 4; 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 (4):
>    i386/acpi: make floppy controller object dynamic
>    i386: expose floppy drive CMOS type
>    fdc: add function to determine drive chs limits
>    i386: populate floppy drive information in DSDT
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Kevin O'Connor <kevin@koconnor.net>
> ---
> changes since v7:
>   - rebased to latest master
>   - use drive max c,h,s rather than the current diskette geometry
>
>   hw/block/fdc.c         | 23 +++++++++++++
>   hw/i386/acpi-build.c   | 92 ++++++++++++++++++++++++++++++++++++--------------
>   hw/i386/pc.c           |  2 +-
>   include/hw/block/fdc.h |  2 ++
>   include/hw/i386/pc.h   |  1 +
>   5 files changed, 94 insertions(+), 26 deletions(-)
>
Michael, we have obtained Reviwed-by: from John.
Does this set is good to be accepted or your
last comment is mandatory?

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

[Michael S. Tsirkin]

On Wed, Mar 02, 2016 at 06:08:41PM +0300, Denis V. Lunev wrote:
> On 02/17/2016 09:25 PM, Roman Kagan wrote:
> >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 4; 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 (4):
> >   i386/acpi: make floppy controller object dynamic
> >   i386: expose floppy drive CMOS type
> >   fdc: add function to determine drive chs limits
> >   i386: populate floppy drive information in DSDT
> >
> >Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> >Cc: Igor Mammedov <imammedo@redhat.com>
> >Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >Cc: Marcel Apfelbaum <marcel@redhat.com>
> >Cc: John Snow <jsnow@redhat.com>
> >Cc: Laszlo Ersek <lersek@redhat.com>
> >Cc: Kevin O'Connor <kevin@koconnor.net>
> >---
> >changes since v7:
> >  - rebased to latest master
> >  - use drive max c,h,s rather than the current diskette geometry
> >
> >  hw/block/fdc.c         | 23 +++++++++++++
> >  hw/i386/acpi-build.c   | 92 ++++++++++++++++++++++++++++++++++++--------------
> >  hw/i386/pc.c           |  2 +-
> >  include/hw/block/fdc.h |  2 ++
> >  include/hw/i386/pc.h   |  1 +
> >  5 files changed, 94 insertions(+), 26 deletions(-)
> >
> Michael, we have obtained Reviwed-by: from John.
> Does this set is good to be accepted or your
> last comment is mandatory?

Pls do but you can make it a separate patch on top
if you prefer.

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

[Roman Kagan]

On Wed, Mar 02, 2016 at 05:10:58PM +0200, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2016 at 06:08:41PM +0300, Denis V. Lunev wrote:
> > On 02/17/2016 09:25 PM, Roman Kagan wrote:
> > >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 4; 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 (4):
> > >   i386/acpi: make floppy controller object dynamic
> > >   i386: expose floppy drive CMOS type
> > >   fdc: add function to determine drive chs limits
> > >   i386: populate floppy drive information in DSDT
> > >
> > >Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > >Cc: Igor Mammedov <imammedo@redhat.com>
> > >Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > >Cc: Marcel Apfelbaum <marcel@redhat.com>
> > >Cc: John Snow <jsnow@redhat.com>
> > >Cc: Laszlo Ersek <lersek@redhat.com>
> > >Cc: Kevin O'Connor <kevin@koconnor.net>
> > >---
> > >changes since v7:
> > >  - rebased to latest master
> > >  - use drive max c,h,s rather than the current diskette geometry
> > >
> > >  hw/block/fdc.c         | 23 +++++++++++++
> > >  hw/i386/acpi-build.c   | 92 ++++++++++++++++++++++++++++++++++++--------------
> > >  hw/i386/pc.c           |  2 +-
> > >  include/hw/block/fdc.h |  2 ++
> > >  include/hw/i386/pc.h   |  1 +
> > >  5 files changed, 94 insertions(+), 26 deletions(-)
> > >
> > Michael, we have obtained Reviwed-by: from John.
> > Does this set is good to be accepted or your
> > last comment is mandatory?
> 
> Pls do but you can make it a separate patch on top
> if you prefer.

Sorry I must have lost the track: I thought that all your concerns had
been addressed by John's comment.  Can you please point out what issues
still remain in this patchset that prevent it from being merged?

Thanks,
Roman.

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

[Michael S. Tsirkin]

On Thu, Mar 03, 2016 at 06:48:38PM +0300, Roman Kagan wrote:
> On Wed, Mar 02, 2016 at 05:10:58PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2016 at 06:08:41PM +0300, Denis V. Lunev wrote:
> > > On 02/17/2016 09:25 PM, Roman Kagan wrote:
> > > >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 4; 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 (4):
> > > >   i386/acpi: make floppy controller object dynamic
> > > >   i386: expose floppy drive CMOS type
> > > >   fdc: add function to determine drive chs limits
> > > >   i386: populate floppy drive information in DSDT
> > > >
> > > >Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > > >Cc: Igor Mammedov <imammedo@redhat.com>
> > > >Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > >Cc: Marcel Apfelbaum <marcel@redhat.com>
> > > >Cc: John Snow <jsnow@redhat.com>
> > > >Cc: Laszlo Ersek <lersek@redhat.com>
> > > >Cc: Kevin O'Connor <kevin@koconnor.net>
> > > >---
> > > >changes since v7:
> > > >  - rebased to latest master
> > > >  - use drive max c,h,s rather than the current diskette geometry
> > > >
> > > >  hw/block/fdc.c         | 23 +++++++++++++
> > > >  hw/i386/acpi-build.c   | 92 ++++++++++++++++++++++++++++++++++++--------------
> > > >  hw/i386/pc.c           |  2 +-
> > > >  include/hw/block/fdc.h |  2 ++
> > > >  include/hw/i386/pc.h   |  1 +
> > > >  5 files changed, 94 insertions(+), 26 deletions(-)
> > > >
> > > Michael, we have obtained Reviwed-by: from John.
> > > Does this set is good to be accepted or your
> > > last comment is mandatory?
> > 
> > Pls do but you can make it a separate patch on top
> > if you prefer.
> 
> Sorry I must have lost the track: I thought that all your concerns had
> been addressed by John's comment.  Can you please point out what issues
> still remain in this patchset that prevent it from being merged?
> 
> Thanks,
> Roman.

it's in my tree so nothing.
I prefer refactoring the loop slightly, by patch on top.