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

[Zhi Hui Li]

On 2012年05月15日 17:27, Paolo Bonzini wrote:
> Il 15/05/2012 11:17, Li Zhi Hui ha scritto:
> > Signed-off-by: Paolo Bonzini<address@hidden>
> > Signed-off-by: Li Zhi Hui<address@hidden>
> > ---
> >   hw/fdc.c |  313 +++++++++++++++++++++++++++++++++++++++++--------------------
> >   1 files changed, 210 insertions(+), 103 deletions(-)
>
> To reviewers, this is obviously missing migration support. :)

Yes, you are right.
I am sorry that I am a newer, I don't know more about migration, so I 
want to send some simple code first .
> 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.
Ok, I will add the commit message.
> > +            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").
ok.

Thank you very much for your feedback. :)