Re: [PATCH 1/3] hw/i386/pc: Allow to compile without CONFIG_FDC_ISA

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Thomas,

On 25/4/24 20:43, Thomas Huth wrote:
> The q35 machine can work without FDC. But to be able to also link
> a QEMU binary that does not include the FDC code, we have to make
> it possible to disable the spots that call into the FDC code.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/pc.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 08c7de416f..93c48f6747 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -439,16 +439,19 @@ static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
>   
>   static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice *floppy)
>   {
> -    int val, nb, i;
> +    int val, nb;
>       FloppyDriveType fd_type[2] = { FLOPPY_DRIVE_TYPE_NONE,
>                                      FLOPPY_DRIVE_TYPE_NONE };
>   
> +#ifdef CONFIG_FDC_ISA
>       /* floppy type */
>       if (floppy) {
> -        for (i = 0; i < 2; i++) {
> +        for (int i = 0; i < 2; i++) {
>               fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>           }
>       }
> +#endif
> +
>       val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
>           cmos_get_fd_drive_type(fd_type[1]);
>       mc146818rtc_set_cmos_data(rtc_state, 0x10, val);
> @@ -1132,7 +1135,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>       int i;
>       DriveInfo *fd[MAX_FD];
>       qemu_irq *a20_line;
> -    ISADevice *fdc, *i8042, *port92, *vmmouse;
> +    ISADevice *i8042, *port92, *vmmouse;
>   
>       serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
>       parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
> @@ -1142,11 +1145,13 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>           create_fdctrl |= !!fd[i];
>       }
>       if (create_fdctrl) {
> -        fdc = isa_new(TYPE_ISA_FDC);
> +#ifdef CONFIG_FDC_ISA
> +        ISADevice *fdc = isa_new(TYPE_ISA_FDC);
>           if (fdc) {
>               isa_realize_and_unref(fdc, isa_bus, &error_fatal);
>               isa_fdc_init_drives(fdc, fd);
>           }
> +#endif
>       }

So the problems are these 2 functions:

FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
{
     FDCtrlISABus *isa = ISA_FDC(fdc);

     return isa->state.drives[i].drive;
}

used by cmos_get_fd_drive_type() and

void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds)
{
     fdctrl_init_drives(&ISA_FDC(fdc)->state.bus, fds);
}

which expands to:

void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
{
     DeviceState *dev;
     int i;

     for (i = 0; i < MAX_FD; i++) {
         if (fds[i]) {
             dev = qdev_new("floppy");
             qdev_prop_set_uint32(dev, "unit", i);
             qdev_prop_set_enum(dev, "drive-type", FLOPPY_DRIVE_TYPE_AUTO);
             qdev_prop_set_drive_err(dev, "drive", 
blk_by_legacy_dinfo(fds[i]),
                                     &error_fatal);
             qdev_realize_and_unref(dev, &bus->bus, &error_fatal);
         }
     }
}

Pre-QOM code I suppose, since it is odd to create the floppy
drives out of the FDC realize() method. Somehow DriveInfo[]
should be passed to the FDC object before it is realized.

For cmos_get_fd_drive_type, we could expose the FloppyDriveType
as a QOM property. Then we only need the QOM API to access the
FDC, and it can be built as a generic modularized QDev object,
usable in a generic qemu-system binary.

This is why I was holding this series review, I needed to give
it some thoughts w.r.t. single binary. I'm gonna ack the series,
postponing the FDC rework.

Regards,

Phil.