Re: [Qemu-devel] [PATCH v3] Add "cache" parameter to "-drive"

[andrzej zaborowski]

Hi,

On 18/12/2007, Laurent Vivier <address@hidden> wrote:
> This patch adds a new parameter to "-drive"
>
> Using "cache=off" with "-drive" will open the disk image file using
> "O_DIRECT".
>
> By default, "cache" is set to "on" to keep original behavior of qemu.
>
> v3 modify hw/sd.c to allocate buffer on init and not on each 
> blk_read()/blk_write()
> and add "cache=" in qemu-doc.texi.
>
>
> example:
>
> "-drive file=my_disk.qcow2,cache=off"
> ---
>  block-raw-posix.c |    8 +++++++
>  block-raw-win32.c |    4 +++
>  block.c           |    2 -
>  block.h           |    1
>  hw/fdc.c          |    7 +++++-
>  hw/ide.c          |   18 +++++++++++++----
>  hw/scsi-disk.c    |    3 +-
>  hw/sd.c           |   56 
> ++++++++++++++++++++++++++----------------------------
>  osdep.c           |   20 +++++++++++++++++++
>  osdep.h           |    1
>  qemu-doc.texi     |    2 +
>  vl.c              |   28 +++++++++++++++++++++++----
>  12 files changed, 110 insertions(+), 40 deletions(-)
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c   2007-12-18 10:58:14.000000000 +0100
> +++ qemu/block.c        2007-12-18 11:01:01.000000000 +0100
> @@ -380,7 +380,7 @@ int bdrv_open2(BlockDriverState *bs, con
>      /* Note: for compatibility, we open disk image files as RDWR, and
>         RDONLY as fallback */
>      if (!(flags & BDRV_O_FILE))
> -        open_flags = BDRV_O_RDWR;
> +        open_flags = BDRV_O_RDWR | (flags & BDRV_O_DIRECT);
>      else
>          open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
>      ret = drv->bdrv_open(bs, filename, open_flags);
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h   2007-12-18 10:58:14.000000000 +0100
> +++ qemu/block.h        2007-12-18 11:01:01.000000000 +0100
> @@ -44,6 +44,7 @@ typedef struct QEMUSnapshotInfo {
>                                       use a disk image format on top of
>                                       it (default for
>                                       bdrv_file_open()) */
> +#define BDRV_O_DIRECT      0x0020
>
>  #ifndef QEMU_IMG
>  void bdrv_info(void);
> Index: qemu/hw/fdc.c
> ===================================================================
> --- qemu.orig/hw/fdc.c  2007-12-18 10:58:14.000000000 +0100
> +++ qemu/hw/fdc.c       2007-12-18 11:01:01.000000000 +0100
> @@ -382,7 +382,7 @@ struct fdctrl_t {
>      uint8_t cur_drv;
>      uint8_t bootsel;
>      /* Command FIFO */
> -    uint8_t fifo[FD_SECTOR_LEN];
> +    uint8_t *fifo;
>      uint32_t data_pos;
>      uint32_t data_len;
>      uint8_t data_state;
> @@ -598,6 +598,11 @@ fdctrl_t *fdctrl_init (qemu_irq irq, int
>      fdctrl = qemu_mallocz(sizeof(fdctrl_t));
>      if (!fdctrl)
>          return NULL;
> +    fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
> +    if (fdctrl->fifo == NULL) {
> +        qemu_free(fdctrl);
> +        return NULL;
> +    }
>      fdctrl->result_timer = qemu_new_timer(vm_clock,
>                                            fdctrl_result_timer, fdctrl);
>
> Index: qemu/hw/ide.c
> ===================================================================
> --- qemu.orig/hw/ide.c  2007-12-18 10:58:14.000000000 +0100
> +++ qemu/hw/ide.c       2007-12-18 11:01:01.000000000 +0100
> @@ -365,7 +365,7 @@ typedef struct IDEState {
>      EndTransferFunc *end_transfer_func;
>      uint8_t *data_ptr;
>      uint8_t *data_end;
> -    uint8_t io_buffer[MAX_MULT_SECTORS*512 + 4];
> +    uint8_t *io_buffer;
>      QEMUTimer *sector_write_timer; /* only used for win2k install hack */
>      uint32_t irq_count; /* counts IRQs when using win2k install hack */
>      /* CF-ATA extended error */
> @@ -2377,17 +2377,24 @@ struct partition {
>  static int guess_disk_lchs(IDEState *s,
>                             int *pcylinders, int *pheads, int *psectors)
>  {
> -    uint8_t buf[512];
> +    uint8_t *buf;
>      int ret, i, heads, sectors, cylinders;
>      struct partition *p;
>      uint32_t nr_sects;
>
> +    buf = qemu_memalign(512, 512);
> +    if (buf == NULL)
> +        return -1;
>      ret = bdrv_read(s->bs, 0, buf, 1);
> -    if (ret < 0)
> +    if (ret < 0) {
> +        qemu_free(buf);
>          return -1;
> +    }
>      /* test msdos magic */
> -    if (buf[510] != 0x55 || buf[511] != 0xaa)
> +    if (buf[510] != 0x55 || buf[511] != 0xaa) {
> +        qemu_free(buf);
>          return -1;
> +    }
>      for(i = 0; i < 4; i++) {
>          p = ((struct partition *)(buf + 0x1be)) + i;
>          nr_sects = le32_to_cpu(p->nr_sects);
> @@ -2408,9 +2415,11 @@ static int guess_disk_lchs(IDEState *s,
>              printf("guessed geometry: LCHS=%d %d %d\n",
>                     cylinders, heads, sectors);
>  #endif
> +            qemu_free(buf);
>              return 0;
>          }
>      }
> +    qemu_free(buf);
>      return -1;
>  }
>
> @@ -2425,6 +2434,7 @@ static void ide_init2(IDEState *ide_stat
>
>      for(i = 0; i < 2; i++) {
>          s = ide_state + i;
> +        s->io_buffer = qemu_memalign(512, MAX_MULT_SECTORS*512 + 4);
>          if (i == 0)
>              s->bs = hd0;
>          else
> Index: qemu/hw/scsi-disk.c
> ===================================================================
> --- qemu.orig/hw/scsi-disk.c    2007-12-18 10:58:14.000000000 +0100
> +++ qemu/hw/scsi-disk.c 2007-12-18 11:01:01.000000000 +0100
> @@ -46,7 +46,7 @@ typedef struct SCSIRequest {
>      int sector_count;
>      /* The amounnt of data in the buffer.  */
>      int buf_len;
> -    uint8_t dma_buf[SCSI_DMA_BUF_SIZE];
> +    uint8_t *dma_buf;
>      BlockDriverAIOCB *aiocb;
>      struct SCSIRequest *next;
>  } SCSIRequest;
> @@ -78,6 +78,7 @@ static SCSIRequest *scsi_new_request(SCS
>          free_requests = r->next;
>      } else {
>          r = qemu_malloc(sizeof(SCSIRequest));
> +        r->dma_buf = qemu_memalign(512, SCSI_DMA_BUF_SIZE);
>      }
>      r->dev = s;
>      r->tag = tag;
> Index: qemu/hw/sd.c
> ===================================================================
> --- qemu.orig/hw/sd.c   2007-12-18 10:58:14.000000000 +0100
> +++ qemu/hw/sd.c        2007-12-18 11:19:19.000000000 +0100
> @@ -96,6 +96,7 @@ struct SDState {
>      qemu_irq readonly_cb;
>      qemu_irq inserted_cb;
>      BlockDriverState *bdrv;
> +    uint8_t *buf;
>  };
>
>  static void sd_set_status(SDState *sd)
> @@ -405,6 +406,7 @@ SDState *sd_init(BlockDriverState *bs, i
>      SDState *sd;
>
>      sd = (SDState *) qemu_mallocz(sizeof(SDState));
> +    sd->buf = qemu_memalign(512, 512);
>      sd->spi = is_spi;
>      sd_reset(sd, bs);
>      bdrv_set_change_cb(sd->bdrv, sd_cardchange, sd);
> @@ -1281,64 +1283,60 @@ int sd_do_command(SDState *sd, struct sd
>  }
>
>  /* No real need for 64 bit addresses here */
> -static void sd_blk_read(BlockDriverState *bdrv,
> -                void *data, uint32_t addr, uint32_t len)
> +static void sd_blk_read(SDState *sd)
>  {
> -    uint8_t buf[512];
> -    uint32_t end = addr + len;
> +    uint32_t addr = sd->data_start;
> +    uint32_t end = addr + sd->blk_len;
>
> -    if (!bdrv || bdrv_read(bdrv, addr >> 9, buf, 1) == -1) {
> +    if (!sd->bdrv || bdrv_read(sd->bdrv, addr >> 9, sd->buf, 1) == -1) {
>          printf("sd_blk_read: read error on host side\n");
>          return;
>      }
>
>      if (end > (addr & ~511) + 512) {
> -        memcpy(data, buf + (addr & 511), 512 - (addr & 511));
> +        memcpy(sd->data, sd->buf + (addr & 511), 512 - (addr & 511));
>
> -        if (bdrv_read(bdrv, end >> 9, buf, 1) == -1) {
> +        if (bdrv_read(sd->bdrv, end >> 9, sd->buf, 1) == -1) {
>              printf("sd_blk_read: read error on host side\n");
>              return;
>          }
> -        memcpy(data + 512 - (addr & 511), buf, end & 511);
> +        memcpy(sd->data + 512 - (addr & 511), sd->buf, end & 511);
>      } else
> -        memcpy(data, buf + (addr & 511), len);
> +        memcpy(sd->data, sd->buf + (addr & 511), sd->blk_len);
>  }
>
> -static void sd_blk_write(BlockDriverState *bdrv,
> -                void *data, uint32_t addr, uint32_t len)
> +static void sd_blk_write(SDState *sd)
>  {
> -    uint8_t buf[512];
> -    uint32_t end = addr + len;
> +    uint32_t addr = sd->data_start;
> +    uint32_t end = addr + sd->blk_len;
>
> -    if ((addr & 511) || len < 512)
> -        if (!bdrv || bdrv_read(bdrv, addr >> 9, buf, 1) == -1) {
> +    if ((addr & 511) || sd->blk_len < 512)
> +        if (!sd->bdrv || bdrv_read(sd->bdrv, addr >> 9, sd->buf, 1) == -1) {
>              printf("sd_blk_write: read error on host side\n");
>              return;
>          }
>
>      if (end > (addr & ~511) + 512) {
> -        memcpy(buf + (addr & 511), data, 512 - (addr & 511));
> -        if (bdrv_write(bdrv, addr >> 9, buf, 1) == -1) {
> +        memcpy(sd->buf + (addr & 511), sd->data, 512 - (addr & 511));
> +        if (bdrv_write(sd->bdrv, addr >> 9, sd->buf, 1) == -1) {
>              printf("sd_blk_write: write error on host side\n");
>              return;
>          }
>
> -        if (bdrv_read(bdrv, end >> 9, buf, 1) == -1) {
> +        if (bdrv_read(sd->bdrv, end >> 9, sd->buf, 1) == -1) {
>              printf("sd_blk_write: read error on host side\n");
>              return;
>          }
> -        memcpy(buf, data + 512 - (addr & 511), end & 511);
> -        if (bdrv_write(bdrv, end >> 9, buf, 1) == -1)
> +        memcpy(sd->buf, sd->data + 512 - (addr & 511), end & 511);
> +        if (bdrv_write(sd->bdrv, end >> 9, sd->buf, 1) == -1)
>              printf("sd_blk_write: write error on host side\n");
>      } else {
> -        memcpy(buf + (addr & 511), data, len);
> -        if (!bdrv || bdrv_write(bdrv, addr >> 9, buf, 1) == -1)
> +        memcpy(sd->buf + (addr & 511), sd->data, sd->blk_len);
> +        if (!sd->bdrv || bdrv_write(sd->bdrv, addr >> 9, sd->buf, 1) == -1)
>              printf("sd_blk_write: write error on host side\n");
>      }
>  }
>
> -#define BLK_READ_BLOCK(a, len) sd_blk_read(sd->bdrv, sd->data, a, len)
> -#define BLK_WRITE_BLOCK(a, len)        sd_blk_write(sd->bdrv, sd->data, a, 
> len)

I committed the patch but I retained the use of these macros in sd.c
because they make sense to me.

Thanks,
Andrew