Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Paolo Bonzini <pbonzini@redhat.com>]

Il 15/05/2012 11:17, Li Zhi Hui ha scritto:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
> ---
>  hw/fdc.c |  313 +++++++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 210 insertions(+), 103 deletions(-)

To reviewers, this is obviously missing migration support. :)

It is also missing a good commit message.  It should say that the
support in the existing code for scan commands has a lot of "weirdness",
for example:

-                if ((ret < 0 && fdctrl->data_dir == FD_DIR_SCANL) ||
-                    (ret > 0 && fdctrl->data_dir == FD_DIR_SCANH)) {
-                    status2 = 0x00;
-                    goto end_transfer;

...

- end_transfer:
-    len = fdctrl->data_pos - start_pos;
-    FLOPPY_DPRINTF("end transfer %d %d %d\n",
-                   fdctrl->data_pos, len, fdctrl->data_len);
-    if (fdctrl->data_dir == FD_DIR_SCANE ||
-        fdctrl->data_dir == FD_DIR_SCANL ||
-        fdctrl->data_dir == FD_DIR_SCANH)
-        status2 = FD_SR2_SEH;

which blindly overwrites status2.  Hence the new code was not written
based on it.  However, the new code is untested as far as I know.

> +            if (fdctrl->data_dir == FD_DIR_SCANE ||
> +                fdctrl->data_dir == FD_DIR_SCANL ||
> +                fdctrl->data_dir == FD_DIR_SCANH) {
> +                fdctrl->dma_status2 = FD_SR2_SEH;
> +            }

This should be FD_SR2_SNS (Spec for the FDC is at
http://www.cepece.info/amstrad/docs/i8272/8272sp.htm: "If the conditions
for scan are not met between the the starting sector (as specified by R)
and the last sector on the cylinder (EOT), then the FDC sets the SN
(Scan Not Satisfied) flag of Status Register 2 to a 1 (high), and
terminates the Scan Command").

I'm not sure whether scan commands should be kept at all though.  Hervé,
do you know anything that uses them?

Paolo