Re: [PATCH v5 08/15] acpi: move aml builder code for floppy device

[John Snow]

On 5/7/20 10:05 AM, Philippe Mathieu-Daudé wrote:
> +Hervé
> 
> On 5/7/20 3:16 PM, Gerd Hoffmann wrote:
>> Signed-off-by: Gerd Hoffmann <address@hidden>
>> Reviewed-by: Igor Mammedov <address@hidden>
>> ---
>>   hw/block/fdc.c       | 83 ++++++++++++++++++++++++++++++++++++++++++++

oh no...

>>   hw/i386/acpi-build.c | 83 --------------------------------------------
>>   stubs/cmos.c         |  7 ++++
>>   stubs/Makefile.objs  |  1 +
>>   4 files changed, 91 insertions(+), 83 deletions(-)
>>   create mode 100644 stubs/cmos.c
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 9628cc171ef8..40faa088b5f7 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -32,6 +32,8 @@
>>   #include "qapi/error.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/timer.h"
>> +#include "hw/i386/pc.h"
> 
> I'd rather not see this target-specific header used in the generic
> device model... The culprit seems cmos_get_fd_drive_type(). Is the value
> really PC specific?
> 
> Hervé, do you know if such info & RTC nvram layout is used in other arch?
> 

do we need pc-only floppy code somewhere to keep the makefile pure?
that's fine too.

>> +#include "hw/acpi/aml-build.h"
>>   #include "hw/irq.h"
>>   #include "hw/isa/isa.h"
>>   #include "hw/qdev-properties.h"
>> @@ -2765,6 +2767,85 @@ void isa_fdc_get_drive_max_chs(FloppyDriveType
>> type,
>>       (*maxc)--;
>>   }
>>   +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 void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
>> +{
>> +    Aml *dev;
>> +    Aml *crs;
>> +    int i;
>> +
>> +#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) */
>> +    };
>> +
>> +    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));
>> +    aml_append(crs, aml_irq_no_flags(6));
>> +    aml_append(crs,
>> +        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
>> +
>> +    dev = aml_device("FDC0");
>> +    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
>> +    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(isadev, 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)));
>> +
>> +    aml_append(scope, dev);
>> +}
>> +
>>   static const VMStateDescription vmstate_isa_fdc ={
>>       .name = "fdc",
>>       .version_id = 2,
>> @@ -2798,11 +2879,13 @@ static Property isa_fdc_properties[] = {
>>   static void isabus_fdc_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>>         dc->realize = isabus_fdc_realize;
>>       dc->fw_name = "fdc";
>>       dc->reset = fdctrl_external_reset_isa;
>>       dc->vmsd = &vmstate_isa_fdc;
>> +    isa->build_aml = fdc_isa_build_aml;
>>       device_class_set_props(dc, isa_fdc_properties);
>>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>   }
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 443db94deb5b..775936e28b9a 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1058,85 +1058,6 @@ static void build_hpet_aml(Aml *table)
>>       aml_append(table, scope);
>>   }
>>   -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")));
>> -
>> -    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));
>> -    aml_append(crs, aml_irq_no_flags(6));
>> -    aml_append(crs,
>> -        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;
>> -}
>> -
>>   static Aml *build_kbd_device_aml(void)
>>   {
>>       Aml *dev;
>> @@ -1175,7 +1096,6 @@ static Aml *build_mouse_device_aml(void)
>>     static void build_isa_devices_aml(Aml *table)
>>   {
>> -    ISADevice *fdc = pc_find_fdc0();
>>       bool ambiguous;
>>         Aml *scope = aml_scope("_SB.PCI0.ISA");
>> @@ -1183,9 +1103,6 @@ static void build_isa_devices_aml(Aml *table)
>>         aml_append(scope, build_kbd_device_aml());
>>       aml_append(scope, build_mouse_device_aml());
>> -    if (fdc) {
>> -        aml_append(scope, build_fdc_device_aml(fdc));
>> -    }
>>         if (ambiguous) {
>>           error_report("Multiple ISA busses, unable to define IPMI
>> ACPI data");
>> diff --git a/stubs/cmos.c b/stubs/cmos.c
>> new file mode 100644
>> index 000000000000..416cbe4055ff
>> --- /dev/null
>> +++ b/stubs/cmos.c
>> @@ -0,0 +1,7 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/i386/pc.h"
>> +
>> +int cmos_get_fd_drive_type(FloppyDriveType fd0)
>> +{
>> +    return 0;
>> +}
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index 45be5dc0ed78..3cbe472d1c6c 100644
>> --- a/stubs/Makefile.objs
>> +++ b/stubs/Makefile.objs
>> @@ -3,6 +3,7 @@ stub-obj-y += bdrv-next-monitor-owned.o
>>   stub-obj-y += blk-commit-all.o
>>   stub-obj-y += blockdev-close-all-bdrv-states.o
>>   stub-obj-y += clock-warp.o
>> +stub-obj-y += cmos.o
>>   stub-obj-y += cpu-get-clock.o
>>   stub-obj-y += cpu-get-icount.o
>>   stub-obj-y += dump.o
>>
> 

-- 
—js