Re: [PATCH 2/4] hw/block/fdc: Declare shared prototypes in fdc-internal.h

[John Snow <jsnow@redhat.com>]

On 4/15/21 6:23 AM, Philippe Mathieu-Daudé wrote:
> We want to extract ISA/SysBus code from the generic fdc.c file.
> First, declare the prototypes we will access from the new units
> into a new local header: "fdc-internal.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc-internal.h | 155 ++++++++++++++++++++++++++++++++++++++++
>   hw/block/fdc.c          | 126 +++-----------------------------
>   MAINTAINERS             |   1 +
>   3 files changed, 164 insertions(+), 118 deletions(-)
>   create mode 100644 hw/block/fdc-internal.h
> 

With our policy of not including osdep.h in headers, it's hard to verify 
that this header is otherwise self-sufficient.


I think the only thing it needs (not in osdep.h) happens to be MAX_FD. I 
added osdep.h just to test:

jsnow@scv ~/s/q/h/block (review)> gcc -I../../include/ -I../../bin/git 
-I/usr/lib64/glib-2.0/include -I/usr/include/glib-2.0 -c -o 
test_header.bin fdc-internal.h
fdc-internal.h:134:19: error: ‘MAX_FD’ undeclared here (not in a function)
   134 |     FDrive drives[MAX_FD];
       |                   ^~~~~~


Should we include the fdc header from the internal one?

> diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
> new file mode 100644
> index 00000000000..ddd41461ff3
> --- /dev/null
> +++ b/hw/block/fdc-internal.h
> @@ -0,0 +1,155 @@
> +/*
> + * QEMU Floppy disk emulator (Intel 82078)
> + *
> + * Copyright (c) 2003, 2007 Jocelyn Mayer
> + * Copyright (c) 2008 Hervé Poussineau
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#ifndef HW_BLOCK_FDC_INTERNAL_H
> +#define HW_BLOCK_FDC_INTERNAL_H
> +
> +#include "exec/memory.h"
> +#include "exec/ioport.h"
> +#include "hw/block/block.h"
> +#include "qapi/qapi-types-block.h"
> +
> +typedef struct FDCtrl FDCtrl;
> +
> +/* Floppy bus emulation */
> +
> +typedef struct FloppyBus {
> +    BusState bus;
> +    FDCtrl *fdc;
> +} FloppyBus;
> +
> +/* Floppy disk drive emulation */
> +
> +typedef enum FDriveRate {
> +    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> +    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> +    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> +    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> +} FDriveRate;
> +
> +typedef enum FDriveSize {
> +    FDRIVE_SIZE_UNKNOWN,
> +    FDRIVE_SIZE_350,
> +    FDRIVE_SIZE_525,
> +} FDriveSize;
> +
> +typedef struct FDFormat {
> +    FloppyDriveType drive;
> +    uint8_t last_sect;
> +    uint8_t max_track;
> +    uint8_t max_head;
> +    FDriveRate rate;
> +} FDFormat;
> +
> +typedef enum FDiskFlags {
> +    FDISK_DBL_SIDES  = 0x01,
> +} FDiskFlags;
> +
> +typedef struct FDrive {
> +    FDCtrl *fdctrl;
> +    BlockBackend *blk;
> +    BlockConf *conf;
> +    /* Drive status */
> +    FloppyDriveType drive;    /* CMOS drive type        */
> +    uint8_t perpendicular;    /* 2.88 MB access mode    */
> +    /* Position */
> +    uint8_t head;
> +    uint8_t track;
> +    uint8_t sect;
> +    /* Media */
> +    FloppyDriveType disk;     /* Current disk type      */
> +    FDiskFlags flags;
> +    uint8_t last_sect;        /* Nb sector per track    */
> +    uint8_t max_track;        /* Nb of tracks           */
> +    uint16_t bps;             /* Bytes per sector       */
> +    uint8_t ro;               /* Is read-only           */
> +    uint8_t media_changed;    /* Is media changed       */
> +    uint8_t media_rate;       /* Data rate of medium    */
> +
> +    bool media_validated;     /* Have we validated the media? */
> +} FDrive;
> +
> +struct FDCtrl {
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +    /* Controller state */
> +    QEMUTimer *result_timer;
> +    int dma_chann;
> +    uint8_t phase;
> +    IsaDma *dma;
> +    /* Controller's identification */
> +    uint8_t version;
> +    /* HW */
> +    uint8_t sra;
> +    uint8_t srb;
> +    uint8_t dor;
> +    uint8_t dor_vmstate; /* only used as temp during vmstate */
> +    uint8_t tdr;
> +    uint8_t dsr;
> +    uint8_t msr;
> +    uint8_t cur_drv;
> +    uint8_t status0;
> +    uint8_t status1;
> +    uint8_t status2;
> +    /* Command FIFO */
> +    uint8_t *fifo;
> +    int32_t fifo_size;
> +    uint32_t data_pos;
> +    uint32_t data_len;
> +    uint8_t data_state;
> +    uint8_t data_dir;
> +    uint8_t eot; /* last wanted sector */
> +    /* States kept only to be returned back */
> +    /* precompensation */
> +    uint8_t precomp_trk;
> +    uint8_t config;
> +    uint8_t lock;
> +    /* Power down config (also with status regB access mode */
> +    uint8_t pwrd;
> +    /* Floppy drives */
> +    FloppyBus bus;
> +    uint8_t num_floppies;
> +    FDrive drives[MAX_FD];
> +    struct {
> +        FloppyDriveType type;
> +    } qdev_for_drives[MAX_FD];
> +    int reset_sensei;
> +    FloppyDriveType fallback; /* type=auto failure fallback */
> +    /* Timers state */
> +    uint8_t timer0;
> +    uint8_t timer1;
> +    PortioList portio_list;
> +};
> +
> +extern const FDFormat fd_formats[];
> +extern const VMStateDescription vmstate_fdc;
> +
> +uint32_t fdctrl_read(void *opaque, uint32_t reg);
> +void fdctrl_write(void *opaque, uint32_t reg, uint32_t value);
> +void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
> +void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp);
> +
> +void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds);
> +
> +#endif
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 1d3a0473678..300f39672af 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -48,6 +48,7 @@
>   #include "qemu/module.h"
>   #include "trace.h"
>   #include "qom/object.h"
> +#include "fdc-internal.h"
>   
>   /********************************************************/
>   /* debug Floppy devices */
> @@ -68,15 +69,8 @@
>   #define TYPE_FLOPPY_BUS "floppy-bus"
>   OBJECT_DECLARE_SIMPLE_TYPE(FloppyBus, FLOPPY_BUS)
>   
> -typedef struct FDCtrl FDCtrl;
> -typedef struct FDrive FDrive;
>   static FDrive *get_drv(FDCtrl *fdctrl, int unit);
>   
> -struct FloppyBus {
> -    BusState bus;
> -    FDCtrl *fdc;
> -};
> -
>   static const TypeInfo floppy_bus_info = {
>       .name = TYPE_FLOPPY_BUS,
>       .parent = TYPE_BUS,
> @@ -93,32 +87,11 @@ static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
>   /********************************************************/
>   /* Floppy drive emulation                               */
>   
> -typedef enum FDriveRate {
> -    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> -    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> -    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> -    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> -} FDriveRate;
> -
> -typedef enum FDriveSize {
> -    FDRIVE_SIZE_UNKNOWN,
> -    FDRIVE_SIZE_350,
> -    FDRIVE_SIZE_525,
> -} FDriveSize;
> -
> -typedef struct FDFormat {
> -    FloppyDriveType drive;
> -    uint8_t last_sect;
> -    uint8_t max_track;
> -    uint8_t max_head;
> -    FDriveRate rate;
> -} FDFormat;
> -
>   /* In many cases, the total sector size of a format is enough to uniquely
>    * identify it. However, there are some total sector collisions between
>    * formats of different physical size, and these are noted below by
>    * highlighting the total sector size for entries with collisions. */
> -static const FDFormat fd_formats[] = {
> +const FDFormat fd_formats[] = {
>       /* First entry is default format */
>       /* 1.44 MB 3"1/2 floppy disks */
>       { FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 2880 */
> @@ -186,35 +159,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
>   #define FD_SECTOR_SC           2   /* Sector size code */
>   #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
>   
> -/* Floppy disk drive emulation */
> -typedef enum FDiskFlags {
> -    FDISK_DBL_SIDES  = 0x01,
> -} FDiskFlags;
> -
> -struct FDrive {
> -    FDCtrl *fdctrl;
> -    BlockBackend *blk;
> -    BlockConf *conf;
> -    /* Drive status */
> -    FloppyDriveType drive;    /* CMOS drive type        */
> -    uint8_t perpendicular;    /* 2.88 MB access mode    */
> -    /* Position */
> -    uint8_t head;
> -    uint8_t track;
> -    uint8_t sect;
> -    /* Media */
> -    FloppyDriveType disk;     /* Current disk type      */
> -    FDiskFlags flags;
> -    uint8_t last_sect;        /* Nb sector per track    */
> -    uint8_t max_track;        /* Nb of tracks           */
> -    uint16_t bps;             /* Bytes per sector       */
> -    uint8_t ro;               /* Is read-only           */
> -    uint8_t media_changed;    /* Is media changed       */
> -    uint8_t media_rate;       /* Data rate of medium    */
> -
> -    bool media_validated;     /* Have we validated the media? */
> -};
> -
>   
>   static FloppyDriveType get_fallback_drive_type(FDrive *drv);
>   
> @@ -626,7 +570,6 @@ static const TypeInfo floppy_drive_info = {
>   /********************************************************/
>   /* Intel 82078 floppy disk controller emulation          */
>   
> -static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
>   static void fdctrl_to_command_phase(FDCtrl *fdctrl);
>   static int fdctrl_transfer_handler (void *opaque, int nchan,
>                                       int dma_pos, int dma_len);
> @@ -828,58 +771,6 @@ enum {
>   #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
>   #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
>   
> -struct FDCtrl {
> -    MemoryRegion iomem;
> -    qemu_irq irq;
> -    /* Controller state */
> -    QEMUTimer *result_timer;
> -    int dma_chann;
> -    uint8_t phase;
> -    IsaDma *dma;
> -    /* Controller's identification */
> -    uint8_t version;
> -    /* HW */
> -    uint8_t sra;
> -    uint8_t srb;
> -    uint8_t dor;
> -    uint8_t dor_vmstate; /* only used as temp during vmstate */
> -    uint8_t tdr;
> -    uint8_t dsr;
> -    uint8_t msr;
> -    uint8_t cur_drv;
> -    uint8_t status0;
> -    uint8_t status1;
> -    uint8_t status2;
> -    /* Command FIFO */
> -    uint8_t *fifo;
> -    int32_t fifo_size;
> -    uint32_t data_pos;
> -    uint32_t data_len;
> -    uint8_t data_state;
> -    uint8_t data_dir;
> -    uint8_t eot; /* last wanted sector */
> -    /* States kept only to be returned back */
> -    /* precompensation */
> -    uint8_t precomp_trk;
> -    uint8_t config;
> -    uint8_t lock;
> -    /* Power down config (also with status regB access mode */
> -    uint8_t pwrd;
> -    /* Floppy drives */
> -    FloppyBus bus;
> -    uint8_t num_floppies;
> -    FDrive drives[MAX_FD];
> -    struct {
> -        FloppyDriveType type;
> -    } qdev_for_drives[MAX_FD];
> -    int reset_sensei;
> -    FloppyDriveType fallback; /* type=auto failure fallback */
> -    /* Timers state */
> -    uint8_t timer0;
> -    uint8_t timer1;
> -    PortioList portio_list;
> -};
> -
>   static FloppyDriveType get_fallback_drive_type(FDrive *drv)
>   {
>       return drv->fdctrl->fallback;
> @@ -909,7 +800,7 @@ struct FDCtrlISABus {
>       int32_t bootindexB;
>   };
>   
> -static uint32_t fdctrl_read (void *opaque, uint32_t reg)
> +uint32_t fdctrl_read(void *opaque, uint32_t reg)
>   {
>       FDCtrl *fdctrl = opaque;
>       uint32_t retval;
> @@ -946,7 +837,7 @@ static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>       return retval;
>   }
>   
> -static void fdctrl_write (void *opaque, uint32_t reg, uint32_t value)
> +void fdctrl_write(void *opaque, uint32_t reg, uint32_t value)
>   {
>       FDCtrl *fdctrl = opaque;
>   
> @@ -1178,7 +1069,7 @@ static const VMStateDescription vmstate_fdc_phase = {
>       }
>   };
>   
> -static const VMStateDescription vmstate_fdc = {
> +const VMStateDescription vmstate_fdc = {
>       .name = "fdc",
>       .version_id = 2,
>       .minimum_version_id = 2,
> @@ -1268,7 +1159,7 @@ static void fdctrl_raise_irq(FDCtrl *fdctrl)
>   }
>   
>   /* Reset controller */
> -static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
> +void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
>   {
>       int i;
>   
> @@ -2484,7 +2375,7 @@ static void fdctrl_result_timer(void *opaque)
>   
>   /* Init functions */
>   
> -static void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
> +void fdctrl_init_drives(FloppyBus *bus, DriveInfo **fds)
>   {
>       DeviceState *dev;
>       int i;
> @@ -2542,8 +2433,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
>       fdctrl_init_drives(&sys->state.bus, fds);
>   }
>   
> -static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
> -                                  Error **errp)
> +void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp)
>   {
>       int i, j;
>       FDrive *drive;
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 36055f14c59..20996f60e1f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1676,6 +1676,7 @@ M: John Snow <jsnow@redhat.com>
>   L: qemu-block@nongnu.org
>   S: Supported
>   F: hw/block/fdc.c
> +F: hw/block/fdc-internal.h
>   F: include/hw/block/fdc.h
>   F: tests/qtest/fdc-test.c
>   T: git https://gitlab.com/jsnow/qemu.git ide
>