[Qemu-devel] Re: [PATCH] fdc: fix MAX_FD probelm

[TAKEDA, toshiya]

Dear Juan and members,

>address@hidden (武田 俊也) wrote:
>> Dear members,
>
>Hi
>>  static const VMStateDescription vmstate_fdc = {
>>      .name = "fdc",
>> -    .version_id = 2,
>> -    .minimum_version_id = 2,
>> -    .minimum_version_id_old = 2,
>> +    .version_id = 3,
>> +    .minimum_version_id = 3,
>> +    .minimum_version_id_old = 3,
>
>This is wrong.  You can't move to version 3 without a good reason.  And
>if you move, you have to retain backward compatibility).


>>      .pre_save = fdc_pre_save,
>>      .post_load = fdc_post_load,
>>      .fields      = (VMStateField []) {
>> @@ -692,7 +694,7 @@ static const VMStateDescription vmstate_fdc = {
>>          VMSTATE_UINT8(lock, fdctrl_t),
>>          VMSTATE_UINT8(pwrd, fdctrl_t),
>>          VMSTATE_UINT8_EQUAL(num_floppies, fdctrl_t),
>> -        VMSTATE_STRUCT_ARRAY(drives, fdctrl_t, MAX_FD, 1,
>> +        VMSTATE_STRUCT_ARRAY(drives, fdctrl_t, MAX_LOGICAL_FD, 1,
>
>Will send a patch with support for:
>
>VMSTATE_STRUCT_ARRAY(drives, fdctrl_t, num_floppies, 1, wmstate_fdrive,
>fdrive_t)

Well, it seems that we cannot specify the variable to WMSTATE_STRUCT_ARRAY.

And I found the problem that,
if I defined fdctrl_t->drives as fdrive_t drives[MAX_LOGICAL_FD];
and specify MAX_FD as VMSTATE_STRUCT_ARRAY(drives, fdctrl_t, MAX_FD, ...)
it cause the build error on MinGW.
It seems that we need to specify the same constant value.

So I added null_drives[] and fdctrl_get_drive(), though I feel it 
circumlocutory.


>to do what you need here.
>> diff --git a/qemu/hw/fdc.h b/qemu/hw/fdc.h
>> index 1b81ec1..9614e20 100644
>> --- a/qemu/hw/fdc.h
>> +++ b/qemu/hw/fdc.h
>> @@ -1,5 +1,5 @@
>>  /* fdc.c */
>> -#define MAX_FD 2
>> +#define MAX_PHYSICAL_FD 2
>
>This name change makes you change lots of places.  I think that just
>adding a comment that MAX_FD means MAX_PHYSICAL_FD will do the trick.
>
>Later, Juan.

This is new patch.

diff --git a/qemu/hw/fdc.c b/qemu/hw/fdc.c
index 389d9e6..6450fb5 100644
--- a/qemu/hw/fdc.c
+++ b/qemu/hw/fdc.c
@@ -83,6 +83,7 @@ typedef enum fdisk_flags_t {
 typedef struct fdrive_t {
     BlockDriverState *bs;
     /* Drive status */
+    uint8_t connected;
     fdrive_type_t drive;
     uint8_t perpendicular;    /* 2.88 MB access mode    */
     /* Position */
@@ -424,11 +425,7 @@ enum {
 };
 
 enum {
-#if MAX_FD == 4
     FD_DOR_SELMASK  = 0x03,
-#else
-    FD_DOR_SELMASK  = 0x01,
-#endif
     FD_DOR_nRESET   = 0x04,
     FD_DOR_DMAEN    = 0x08,
     FD_DOR_MOTEN0   = 0x10,
@@ -438,11 +435,7 @@ enum {
 };
 
 enum {
-#if MAX_FD == 4
     FD_TDR_BOOTSEL  = 0x0c,
-#else
-    FD_TDR_BOOTSEL  = 0x04,
-#endif
 };
 
 enum {
@@ -470,6 +463,14 @@ enum {
 #define FD_DID_SEEK(state) ((state) & FD_STATE_SEEK)
 #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
 
+/* MAX_LOGICAL_FD determines the max drive number that Intel 82078 can control,
+   and it should be 4.
+   fdctrl->num_floppies determines the number of physical drives that may be
+   connected and it is usually initialized to MAX_FD, but it may be initialized
+   to other value if any system requires */
+
+#define MAX_LOGICAL_FD 4
+
 struct fdctrl_t {
     /* Controller's identification */
     uint8_t version;
@@ -508,10 +509,13 @@ struct fdctrl_t {
     /* Power down config (also with status regB access mode */
     uint8_t pwrd;
     /* Sun4m quirks? */
-    int sun4m;
+    uint8_t sun4m;
     /* Floppy drives */
     uint8_t num_floppies;
     fdrive_t drives[MAX_FD];
+#if MAX_FD != MAX_LOGICAL_FD
+    fdrive_t null_drives[MAX_LOGICAL_FD - MAX_FD];
+#endif
     int reset_sensei;
 };
 
@@ -525,6 +529,19 @@ typedef struct fdctrl_isabus_t {
     struct fdctrl_t state;
 } fdctrl_isabus_t;
 
+static fdrive_t *fdctrl_get_drive (fdctrl_t *fdctrl, int drive_num)
+{
+    if (drive_num < MAX_FD) {
+        return &fdctrl->drives[drive_num];
+#if MAX_FD != MAX_LOGICAL_FD
+    } else if (drive_num < MAX_LOGICAL_FD) {
+        return &fdctrl->null_drives[drive_num - MAX_FD];
+#endif
+    } else {
+        return NULL;
+    }
+}
+
 static uint32_t fdctrl_read (void *opaque, uint32_t reg)
 {
     fdctrl_t *fdctrl = opaque;
@@ -718,7 +735,7 @@ static void fdctrl_handle_tc(void *opaque, int irq, int 
level)
 /* XXX: may change if moved to bdrv */
 int fdctrl_get_drive_type(fdctrl_t *fdctrl, int drive_num)
 {
-    return fdctrl->drives[drive_num].drive;
+    return fdctrl_get_drive(fdctrl, drive_num)->drive;
 }
 
 /* Change IRQ state */
@@ -760,7 +777,7 @@ static void fdctrl_reset (fdctrl_t *fdctrl, int do_irq)
     /* Initialise controller */
     fdctrl->sra = 0;
     fdctrl->srb = 0xc0;
-    if (!fdctrl->drives[1].bs)
+    if (!fdctrl_get_drive(fdctrl, 1)->bs)
         fdctrl->sra |= FD_SRA_nDRV2;
     fdctrl->cur_drv = 0;
     fdctrl->dor = FD_DOR_nRESET;
@@ -771,8 +788,8 @@ static void fdctrl_reset (fdctrl_t *fdctrl, int do_irq)
     fdctrl->data_len = 0;
     fdctrl->data_state = 0;
     fdctrl->data_dir = FD_DIR_WRITE;
-    for (i = 0; i < MAX_FD; i++)
-        fd_recalibrate(&fdctrl->drives[i]);
+    for (i = 0; i < MAX_LOGICAL_FD; i++)
+        fd_recalibrate(fdctrl_get_drive(fdctrl, i));
     fdctrl_reset_fifo(fdctrl);
     if (do_irq) {
         fdctrl_raise_irq(fdctrl, FD_SR0_RDYCHG);
@@ -782,44 +799,40 @@ static void fdctrl_reset (fdctrl_t *fdctrl, int do_irq)
 
 static inline fdrive_t *drv0 (fdctrl_t *fdctrl)
 {
-    return &fdctrl->drives[(fdctrl->tdr & FD_TDR_BOOTSEL) >> 2];
+    return fdctrl_get_drive(fdctrl, (fdctrl->tdr & FD_TDR_BOOTSEL) >> 2);
 }
 
 static inline fdrive_t *drv1 (fdctrl_t *fdctrl)
 {
     if ((fdctrl->tdr & FD_TDR_BOOTSEL) < (1 << 2))
-        return &fdctrl->drives[1];
+        return fdctrl_get_drive(fdctrl, 1);
     else
-        return &fdctrl->drives[0];
+        return fdctrl_get_drive(fdctrl, 0);
 }
 
-#if MAX_FD == 4
 static inline fdrive_t *drv2 (fdctrl_t *fdctrl)
 {
     if ((fdctrl->tdr & FD_TDR_BOOTSEL) < (2 << 2))
-        return &fdctrl->drives[2];
+        return fdctrl_get_drive(fdctrl, 2);
     else
-        return &fdctrl->drives[1];
+        return fdctrl_get_drive(fdctrl, 1);
 }
 
 static inline fdrive_t *drv3 (fdctrl_t *fdctrl)
 {
     if ((fdctrl->tdr & FD_TDR_BOOTSEL) < (3 << 2))
-        return &fdctrl->drives[3];
+        return fdctrl_get_drive(fdctrl, 3);
     else
-        return &fdctrl->drives[2];
+        return fdctrl_get_drive(fdctrl, 2);
 }
-#endif
 
 static fdrive_t *get_cur_drv (fdctrl_t *fdctrl)
 {
     switch (fdctrl->cur_drv) {
         case 0: return drv0(fdctrl);
         case 1: return drv1(fdctrl);
-#if MAX_FD == 4
         case 2: return drv2(fdctrl);
         case 3: return drv3(fdctrl);
-#endif
         default: return NULL;
     }
 }
@@ -969,12 +982,10 @@ static uint32_t fdctrl_read_dir (fdctrl_t *fdctrl)
 {
     uint32_t retval = 0;
 
-    if (fdctrl_media_changed(drv0(fdctrl))
-     || fdctrl_media_changed(drv1(fdctrl))
-#if MAX_FD == 4
-     || fdctrl_media_changed(drv2(fdctrl))
-     || fdctrl_media_changed(drv3(fdctrl))
-#endif
+    if ((drv0(fdctrl)->connected && fdctrl_media_changed(drv0(fdctrl)))
+     || (drv1(fdctrl)->connected && fdctrl_media_changed(drv1(fdctrl)))
+     || (drv2(fdctrl)->connected && fdctrl_media_changed(drv2(fdctrl)))
+     || (drv3(fdctrl)->connected && fdctrl_media_changed(drv3(fdctrl)))
         )
         retval |= FD_DIR_DSKCHG;
     if (retval != 0)
@@ -1416,15 +1427,10 @@ static void fdctrl_handle_dumpreg (fdctrl_t *fdctrl, 
int direction)
     fdrive_t *cur_drv = get_cur_drv(fdctrl);
 
     /* Drives position */
-    fdctrl->fifo[0] = drv0(fdctrl)->track;
-    fdctrl->fifo[1] = drv1(fdctrl)->track;
-#if MAX_FD == 4
-    fdctrl->fifo[2] = drv2(fdctrl)->track;
-    fdctrl->fifo[3] = drv3(fdctrl)->track;
-#else
-    fdctrl->fifo[2] = 0;
-    fdctrl->fifo[3] = 0;
-#endif
+    fdctrl->fifo[0] = drv0(fdctrl)->connected ? drv0(fdctrl)->track : 0;
+    fdctrl->fifo[1] = drv1(fdctrl)->connected ? drv1(fdctrl)->track : 0;
+    fdctrl->fifo[2] = drv2(fdctrl)->connected ? drv2(fdctrl)->track : 0;
+    fdctrl->fifo[3] = drv3(fdctrl)->connected ? drv3(fdctrl)->track : 0;
     /* timers */
     fdctrl->fifo[4] = fdctrl->timer0;
     fdctrl->fifo[5] = (fdctrl->timer1 << 1) | (fdctrl->dor & FD_DOR_DMAEN ? 1 
: 0);
@@ -1454,12 +1460,18 @@ static void fdctrl_handle_restore (fdctrl_t *fdctrl, 
int direction)
     fdrive_t *cur_drv = get_cur_drv(fdctrl);
 
     /* Drives position */
-    drv0(fdctrl)->track = fdctrl->fifo[3];
-    drv1(fdctrl)->track = fdctrl->fifo[4];
-#if MAX_FD == 4
-    drv2(fdctrl)->track = fdctrl->fifo[5];
-    drv3(fdctrl)->track = fdctrl->fifo[6];
-#endif
+    if(drv0(fdctrl)->connected) {
+        drv0(fdctrl)->track = fdctrl->fifo[3];
+    }
+    if(drv1(fdctrl)->connected) {
+        drv1(fdctrl)->track = fdctrl->fifo[4];
+    }
+    if(drv2(fdctrl)->connected) {
+        drv2(fdctrl)->track = fdctrl->fifo[5];
+    }
+    if(drv3(fdctrl)->connected) {
+        drv3(fdctrl)->track = fdctrl->fifo[6];
+    }
     /* timers */
     fdctrl->timer0 = fdctrl->fifo[7];
     fdctrl->timer1 = fdctrl->fifo[8];
@@ -1479,15 +1491,10 @@ static void fdctrl_handle_save (fdctrl_t *fdctrl, int 
direction)
     fdctrl->fifo[0] = 0;
     fdctrl->fifo[1] = 0;
     /* Drives position */
-    fdctrl->fifo[2] = drv0(fdctrl)->track;
-    fdctrl->fifo[3] = drv1(fdctrl)->track;
-#if MAX_FD == 4
-    fdctrl->fifo[4] = drv2(fdctrl)->track;
-    fdctrl->fifo[5] = drv3(fdctrl)->track;
-#else
-    fdctrl->fifo[4] = 0;
-    fdctrl->fifo[5] = 0;
-#endif
+    fdctrl->fifo[2] = drv0(fdctrl)->connected ? drv0(fdctrl)->track : 0;
+    fdctrl->fifo[3] = drv1(fdctrl)->connected ? drv1(fdctrl)->track : 0;
+    fdctrl->fifo[4] = drv2(fdctrl)->connected ? drv2(fdctrl)->track : 0;
+    fdctrl->fifo[5] = drv3(fdctrl)->connected ? drv3(fdctrl)->track : 0;
     /* timers */
     fdctrl->fifo[6] = fdctrl->timer0;
     fdctrl->fifo[7] = fdctrl->timer1;
@@ -1833,9 +1840,16 @@ static void fdctrl_connect_drives(fdctrl_t *fdctrl, 
BlockDriverState **fds)
 {
     unsigned int i;
 
-    for (i = 0; i < MAX_FD; i++) {
-        fd_init(&fdctrl->drives[i], fds[i]);
-        fd_revalidate(&fdctrl->drives[i]);
+    for (i = 0; i < MAX_LOGICAL_FD; i++) {
+        fdrive_t *drive = fdctrl_get_drive(fdctrl, i);
+        if (i < fdctrl->num_floppies) {
+            drive->connected = 1;
+            fd_init(drive, fds[i]);
+        } else {
+            drive->connected = 0;
+            fd_init(drive, NULL);
+        }
+        fd_revalidate(drive);
     }
 }
 
@@ -1851,6 +1865,7 @@ fdctrl_t *fdctrl_init_isa(BlockDriverState **fds)
     fdctrl->dma_chann = dma_chann;
     DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
 
+    fdctrl->num_floppies = MAX_FD;
     fdctrl_connect_drives(fdctrl, fds);
 
     return fdctrl;
@@ -1873,6 +1888,8 @@ fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
 
     fdctrl->dma_chann = dma_chann;
     DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
+
+    fdctrl->num_floppies = MAX_FD;
     fdctrl_connect_drives(fdctrl, fds);
 
     return fdctrl;
@@ -1895,6 +1912,7 @@ fdctrl_t *sun4m_fdctrl_init (qemu_irq irq, 
target_phys_addr_t io_base,
 
     fdctrl->dma_chann = -1;
 
+    fdctrl->num_floppies = MAX_FD;
     fdctrl_connect_drives(fdctrl, fds);
 
     return fdctrl;