[PATCH 0/7] block: Use definitions instead of magic values

[PATCH 0/7] block: Use definitions instead of magic values

[Philippe Mathieu-Daudé]

Trivial block patches:
- Fix a typo
- Replace '1 << 30' by '1 * GiB' in null-co
- Replace 512 by BDRV_SECTOR_SIZE when appropriate.

Philippe Mathieu-Daudé (7):
  block/null: Make more explicit the driver default size is 1GiB
  hw/ide/core: Trivial typo fix
  hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE
  hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE

 block/null.c        |  4 +++-
 hw/ide/ahci.c       |  5 +++--
 hw/ide/atapi.c      |  8 ++++----
 hw/ide/core.c       | 25 +++++++++++++------------
 hw/ide/pci.c        |  2 +-
 hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
 6 files changed, 47 insertions(+), 41 deletions(-)

-- 
2.21.3

[PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB

[Philippe Mathieu-Daudé]

As it is not obvious the default size for the null block driver
is 1 GiB, replace the obfuscated '1 << 30' magic value by a
definition using IEC binary prefixes.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/null.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/null.c b/block/null.c
index 15e1d56746..8354def367 100644
--- a/block/null.c
+++ b/block/null.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
@@ -21,6 +22,7 @@
 
 #define NULL_OPT_LATENCY "latency-ns"
 #define NULL_OPT_ZEROES  "read-zeroes"
+#define NULL_OPT_SIZE    (1 * GiB)
 
 typedef struct {
     int64_t length;
@@ -86,7 +88,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &error_abort);
     s->length =
-        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
+        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, NULL_OPT_SIZE);
     s->latency_ns =
         qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
     if (s->latency_ns < 0) {
-- 
2.21.3

Re: [PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> As it is not obvious the default size for the null block driver
> is 1 GiB, replace the obfuscated '1 << 30' magic value by a
> definition using IEC binary prefixes.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  block/null.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:29写道：
>
> As it is not obvious the default size for the null block driver
> is 1 GiB, replace the obfuscated '1 << 30' magic value by a
> definition using IEC binary prefixes.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  block/null.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/null.c b/block/null.c
> index 15e1d56746..8354def367 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -11,6 +11,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> @@ -21,6 +22,7 @@
>
>  #define NULL_OPT_LATENCY "latency-ns"
>  #define NULL_OPT_ZEROES  "read-zeroes"
> +#define NULL_OPT_SIZE    (1 * GiB)
>
>  typedef struct {
>      int64_t length;
> @@ -86,7 +88,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &error_abort);
>      s->length =
> -        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> +        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, NULL_OPT_SIZE);
>      s->latency_ns =
>          qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
>      if (s->latency_ns < 0) {
> --
> 2.21.3
>
>

Re: [PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB

[Kevin Wolf]

Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
> As it is not obvious the default size for the null block driver
> is 1 GiB, replace the obfuscated '1 << 30' magic value by a
> definition using IEC binary prefixes.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  block/null.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/null.c b/block/null.c
> index 15e1d56746..8354def367 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> @@ -21,6 +22,7 @@
>  
>  #define NULL_OPT_LATENCY "latency-ns"
>  #define NULL_OPT_ZEROES  "read-zeroes"
> +#define NULL_OPT_SIZE    (1 * GiB)

Let's use a different naming schema for option names and option default
values, and an empty line between the definition for both. The way this
patch has it, it looks like another option name until you look at the
actual value.

Kevin

[PATCH 2/7] hw/ide/core: Trivial typo fix

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..f76f7e5234 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -709,7 +709,7 @@ void ide_cancel_dma_sync(IDEState *s)
     /*
      * We can't cancel Scatter Gather DMA in the middle of the
      * operation or a partial (not full) DMA transfer would reach
-     * the storage so we wait for completion instead (we beahve
+     * the storage so we wait for completion instead (we behave
      * like if the DMA was completed by the time the guest trying
      * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
      * set).
-- 
2.21.3

Re: [PATCH 2/7] hw/ide/core: Trivial typo fix

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ide/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/7] hw/ide/core: Trivial typo fix

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:29写道：
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/ide/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..f76f7e5234 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -709,7 +709,7 @@ void ide_cancel_dma_sync(IDEState *s)
>      /*
>       * We can't cancel Scatter Gather DMA in the middle of the
>       * operation or a partial (not full) DMA transfer would reach
> -     * the storage so we wait for completion instead (we beahve
> +     * the storage so we wait for completion instead (we behave
>       * like if the DMA was completed by the time the guest trying
>       * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
>       * set).
> --
> 2.21.3
>
>

Re: [PATCH 2/7] hw/ide/core: Trivial typo fix

[Kevin Wolf]

Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

Use self-explicit definitions instead of magic '512' value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/core.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index f76f7e5234..bcb2aa85fc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -121,7 +121,7 @@ static void ide_identify(IDEState *s)
     put_le16(p + 0, 0x0040);
     put_le16(p + 1, s->cylinders);
     put_le16(p + 3, s->heads);
-    put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
+    put_le16(p + 4, BDRV_SECTOR_SIZE * s->sectors); /* XXX: retired, remove ? */
     put_le16(p + 5, 512); /* XXX: retired, remove ? */
     put_le16(p + 6, s->sectors);
     padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
@@ -864,7 +864,7 @@ static void ide_dma_cb(void *opaque, int ret)
         }
     }
 
-    if (s->io_buffer_size > s->nsector * 512) {
+    if (s->io_buffer_size > s->nsector * BDRV_SECTOR_SIZE) {
         /*
          * The PRDs were longer than needed for this request.
          * The Active bit must remain set after the request completes.
@@ -877,7 +877,7 @@ static void ide_dma_cb(void *opaque, int ret)
 
     sector_num = ide_get_sector(s);
     if (n > 0) {
-        assert(n * 512 == s->sg.size);
+        assert(n * BDRV_SECTOR_SIZE == s->sg.size);
         dma_buf_commit(s, s->sg.size);
         sector_num += n;
         ide_set_sector(s, sector_num);
@@ -894,17 +894,17 @@ static void ide_dma_cb(void *opaque, int ret)
     /* launch next transfer */
     n = s->nsector;
     s->io_buffer_index = 0;
-    s->io_buffer_size = n * 512;
+    s->io_buffer_size = n * BDRV_SECTOR_SIZE;
     prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
     /* prepare_buf() must succeed and respect the limit */
-    assert(prep_size >= 0 && prep_size <= n * 512);
+    assert(prep_size >= 0 && prep_size <= n * BDRV_SECTOR_SIZE);
 
     /*
      * Now prep_size stores the number of bytes in the sglist, and
      * s->io_buffer_size stores the number of bytes described by the PRDs.
      */
 
-    if (prep_size < n * 512) {
+    if (prep_size < n * BDRV_SECTOR_SIZE) {
         /*
          * The PRDs are too short for this request. Error condition!
          * Reset the Active bit and don't raise the interrupt.
@@ -1412,7 +1412,8 @@ static bool cmd_identify(IDEState *s, uint8_t cmd)
             ide_cfata_identify(s);
         }
         s->status = READY_STAT | SEEK_STAT;
-        ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
+        ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE,
+                           ide_transfer_stop);
         ide_set_irq(s->bus);
         return false;
     } else {
@@ -1482,7 +1483,7 @@ static bool cmd_write_multiple(IDEState *s, uint8_t cmd)
     n = MIN(s->nsector, s->req_nb_sectors);
 
     s->status = SEEK_STAT | READY_STAT;
-    ide_transfer_start(s, s->io_buffer, 512 * n, ide_sector_write);
+    ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE * n, ide_sector_write);
 
     s->media_changed = 1;
 
@@ -1524,7 +1525,7 @@ static bool cmd_write_pio(IDEState *s, uint8_t cmd)
 
     s->req_nb_sectors = 1;
     s->status = SEEK_STAT | READY_STAT;
-    ide_transfer_start(s, s->io_buffer, 512, ide_sector_write);
+    ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE, ide_sector_write);
 
     s->media_changed = 1;
 
@@ -1678,7 +1679,7 @@ static bool cmd_identify_packet(IDEState *s, uint8_t cmd)
 {
     ide_atapi_identify(s);
     s->status = READY_STAT | SEEK_STAT;
-    ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
+    ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE, ide_transfer_stop);
     ide_set_irq(s->bus);
     return false;
 }
@@ -2559,7 +2560,7 @@ static void ide_init1(IDEBus *bus, int unit)
     s->unit = unit;
     s->drive_serial = drive_serial++;
     /* we need at least 2k alignment for accessing CDROMs using O_DIRECT */
-    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
+    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4;
     s->io_buffer = qemu_memalign(2048, s->io_buffer_total_len);
     memset(s->io_buffer, 0, s->io_buffer_total_len);
 
-- 
2.21.3

Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ide/core.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:31写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/ide/core.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f76f7e5234..bcb2aa85fc 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -121,7 +121,7 @@ static void ide_identify(IDEState *s)
>      put_le16(p + 0, 0x0040);
>      put_le16(p + 1, s->cylinders);
>      put_le16(p + 3, s->heads);
> -    put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
> +    put_le16(p + 4, BDRV_SECTOR_SIZE * s->sectors); /* XXX: retired, remove ? */
>      put_le16(p + 5, 512); /* XXX: retired, remove ? */
>      put_le16(p + 6, s->sectors);
>      padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
> @@ -864,7 +864,7 @@ static void ide_dma_cb(void *opaque, int ret)
>          }
>      }
>
> -    if (s->io_buffer_size > s->nsector * 512) {
> +    if (s->io_buffer_size > s->nsector * BDRV_SECTOR_SIZE) {
>          /*
>           * The PRDs were longer than needed for this request.
>           * The Active bit must remain set after the request completes.
> @@ -877,7 +877,7 @@ static void ide_dma_cb(void *opaque, int ret)
>
>      sector_num = ide_get_sector(s);
>      if (n > 0) {
> -        assert(n * 512 == s->sg.size);
> +        assert(n * BDRV_SECTOR_SIZE == s->sg.size);
>          dma_buf_commit(s, s->sg.size);
>          sector_num += n;
>          ide_set_sector(s, sector_num);
> @@ -894,17 +894,17 @@ static void ide_dma_cb(void *opaque, int ret)
>      /* launch next transfer */
>      n = s->nsector;
>      s->io_buffer_index = 0;
> -    s->io_buffer_size = n * 512;
> +    s->io_buffer_size = n * BDRV_SECTOR_SIZE;
>      prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
>      /* prepare_buf() must succeed and respect the limit */
> -    assert(prep_size >= 0 && prep_size <= n * 512);
> +    assert(prep_size >= 0 && prep_size <= n * BDRV_SECTOR_SIZE);
>
>      /*
>       * Now prep_size stores the number of bytes in the sglist, and
>       * s->io_buffer_size stores the number of bytes described by the PRDs.
>       */
>
> -    if (prep_size < n * 512) {
> +    if (prep_size < n * BDRV_SECTOR_SIZE) {
>          /*
>           * The PRDs are too short for this request. Error condition!
>           * Reset the Active bit and don't raise the interrupt.
> @@ -1412,7 +1412,8 @@ static bool cmd_identify(IDEState *s, uint8_t cmd)
>              ide_cfata_identify(s);
>          }
>          s->status = READY_STAT | SEEK_STAT;
> -        ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
> +        ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE,
> +                           ide_transfer_stop);
>          ide_set_irq(s->bus);
>          return false;
>      } else {
> @@ -1482,7 +1483,7 @@ static bool cmd_write_multiple(IDEState *s, uint8_t cmd)
>      n = MIN(s->nsector, s->req_nb_sectors);
>
>      s->status = SEEK_STAT | READY_STAT;
> -    ide_transfer_start(s, s->io_buffer, 512 * n, ide_sector_write);
> +    ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE * n, ide_sector_write);
>
>      s->media_changed = 1;
>
> @@ -1524,7 +1525,7 @@ static bool cmd_write_pio(IDEState *s, uint8_t cmd)
>
>      s->req_nb_sectors = 1;
>      s->status = SEEK_STAT | READY_STAT;
> -    ide_transfer_start(s, s->io_buffer, 512, ide_sector_write);
> +    ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE, ide_sector_write);
>
>      s->media_changed = 1;
>
> @@ -1678,7 +1679,7 @@ static bool cmd_identify_packet(IDEState *s, uint8_t cmd)
>  {
>      ide_atapi_identify(s);
>      s->status = READY_STAT | SEEK_STAT;
> -    ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
> +    ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE, ide_transfer_stop);
>      ide_set_irq(s->bus);
>      return false;
>  }
> @@ -2559,7 +2560,7 @@ static void ide_init1(IDEBus *bus, int unit)
>      s->unit = unit;
>      s->drive_serial = drive_serial++;
>      /* we need at least 2k alignment for accessing CDROMs using O_DIRECT */
> -    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
> +    s->io_buffer_total_len = IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4;
>      s->io_buffer = qemu_memalign(2048, s->io_buffer_total_len);
>      memset(s->io_buffer, 0, s->io_buffer_total_len);
>
> --
> 2.21.3
>
>

Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[Kevin Wolf]

Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

BDRV_SECTOR_SIZE is the arbitrary unit in which some block layer
functions and variables work (such as bs->total_sectors). It happens to
be 512.

IDE disks have a sector size, too. Actually, two of them, a physical and
a logical one. The more important one is the logical one. We happen to
emulate only IDE devices for which the logical block size is 512 bytes
(ide_dev_initfn() errors out otherwise).

But just because two constants both happen to be 512 in practice, they
are not the same thing.

So if we want to replace all magic 512 values, we should probably
introduce a new IDE_SECTOR_SIZE and then decide case by case whether
IDE_SECTOR_SIZE or BDRV_SECTOR_SIZE is meant. I think most (if not all)
of the places you converted in this patch need to be IDE_SECTOR_SIZE.

Kevin

Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[John Snow]

On 8/17/20 7:17 AM, Kevin Wolf wrote:
> Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
>> Use self-explicit definitions instead of magic '512' value.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> BDRV_SECTOR_SIZE is the arbitrary unit in which some block layer
> functions and variables work (such as bs->total_sectors). It happens to
> be 512.
> 
> IDE disks have a sector size, too. Actually, two of them, a physical and
> a logical one. The more important one is the logical one. We happen to
> emulate only IDE devices for which the logical block size is 512 bytes
> (ide_dev_initfn() errors out otherwise).
> 
> But just because two constants both happen to be 512 in practice, they
> are not the same thing.
> 
> So if we want to replace all magic 512 values, we should probably
> introduce a new IDE_SECTOR_SIZE and then decide case by case whether
> IDE_SECTOR_SIZE or BDRV_SECTOR_SIZE is meant. I think most (if not all)
> of the places you converted in this patch need to be IDE_SECTOR_SIZE.
> 
> Kevin
> 

I didn't audit the other patches, but be mindful of the distinction that 
Kevin is pointing out.

Luckily, I think we're low risk for deciding to change the 
BDRV_SECTOR_SIZE default any time soon, so it probably won't matter in 
the near future ...

--js

Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

On 9/23/20 4:53 PM, John Snow wrote:
> On 8/17/20 7:17 AM, Kevin Wolf wrote:
>> Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
>>> Use self-explicit definitions instead of magic '512' value.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> BDRV_SECTOR_SIZE is the arbitrary unit in which some block layer
>> functions and variables work (such as bs->total_sectors). It happens to
>> be 512.
>>
>> IDE disks have a sector size, too. Actually, two of them, a physical and
>> a logical one. The more important one is the logical one. We happen to
>> emulate only IDE devices for which the logical block size is 512 bytes
>> (ide_dev_initfn() errors out otherwise).
>>
>> But just because two constants both happen to be 512 in practice, they
>> are not the same thing.
>>
>> So if we want to replace all magic 512 values, we should probably
>> introduce a new IDE_SECTOR_SIZE and then decide case by case whether
>> IDE_SECTOR_SIZE or BDRV_SECTOR_SIZE is meant. I think most (if not all)
>> of the places you converted in this patch need to be IDE_SECTOR_SIZE.
>>
>> Kevin
>>
> 
> I didn't audit the other patches, but be mindful of the distinction that
> Kevin is pointing out.
> 
> Luckily, I think we're low risk for deciding to change the
> BDRV_SECTOR_SIZE default any time soon, so it probably won't matter in
> the near future ...

TBO my only concern was code readability while reviewing
(improve code readability).

I'll address Kevin's review comment at some point, but this is
a low priority.

Thanks both for having a look,

Phil.

> 
> --js
> 
> 

[PATCH 4/7] hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

Use self-explicit definitions instead of magic '512' value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/ahci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 009120f88b..b696c6291a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1151,7 +1151,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     if (!ncq_tfs->sector_count) {
         ncq_tfs->sector_count = 0x10000;
     }
-    size = ncq_tfs->sector_count * 512;
+    size = ncq_tfs->sector_count * BDRV_SECTOR_SIZE;
     ahci_populate_sglist(ad, &ncq_tfs->sglist, ncq_tfs->cmdh, size, 0);
 
     if (ncq_tfs->sglist.size < size) {
@@ -1703,7 +1703,8 @@ static int ahci_state_post_load(void *opaque, int version_id)
                 return -1;
             }
             ahci_populate_sglist(ncq_tfs->drive, &ncq_tfs->sglist,
-                                 ncq_tfs->cmdh, ncq_tfs->sector_count * 512,
+                                 ncq_tfs->cmdh,
+                                 ncq_tfs->sector_count * BDRV_SECTOR_SIZE,
                                  0);
             if (ncq_tfs->sector_count != ncq_tfs->sglist.size >> 9) {
                 return -1;
-- 
2.21.3

Re: [PATCH 4/7] hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ide/ahci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/7] hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:31写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/ide/ahci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 009120f88b..b696c6291a 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1151,7 +1151,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>      if (!ncq_tfs->sector_count) {
>          ncq_tfs->sector_count = 0x10000;
>      }
> -    size = ncq_tfs->sector_count * 512;
> +    size = ncq_tfs->sector_count * BDRV_SECTOR_SIZE;
>      ahci_populate_sglist(ad, &ncq_tfs->sglist, ncq_tfs->cmdh, size, 0);
>
>      if (ncq_tfs->sglist.size < size) {
> @@ -1703,7 +1703,8 @@ static int ahci_state_post_load(void *opaque, int version_id)
>                  return -1;
>              }
>              ahci_populate_sglist(ncq_tfs->drive, &ncq_tfs->sglist,
> -                                 ncq_tfs->cmdh, ncq_tfs->sector_count * 512,
> +                                 ncq_tfs->cmdh,
> +                                 ncq_tfs->sector_count * BDRV_SECTOR_SIZE,
>                                   0);
>              if (ncq_tfs->sector_count != ncq_tfs->sglist.size >> 9) {
>                  return -1;
> --
> 2.21.3
>
>

[PATCH 5/7] hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

Use self-explicit definitions instead of magic '512' value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/atapi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 17a9d635d8..14a2b0bb2f 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -824,9 +824,9 @@ static void cmd_get_configuration(IDEState *s, uint8_t *buf)
      *
      *      Only a problem if the feature/profiles grow.
      */
-    if (max_len > 512) {
+    if (max_len > BDRV_SECTOR_SIZE) {
         /* XXX: assume 1 sector */
-        max_len = 512;
+        max_len = BDRV_SECTOR_SIZE;
     }
 
     memset(buf, 0, max_len);
@@ -1186,8 +1186,8 @@ static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
         }
     }
 
-    memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * 512 + 4 ?
-           IDE_DMA_BUF_SECTORS * 512 + 4 : max_len);
+    memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 ?
+           IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 : max_len);
 
     switch (format) {
         case 0x00 ... 0x7f:
-- 
2.21.3

Re: [PATCH 5/7] hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ide/atapi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 17a9d635d8..14a2b0bb2f 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -824,9 +824,9 @@ static void cmd_get_configuration(IDEState *s, uint8_t *buf)
>       *
>       *      Only a problem if the feature/profiles grow.
>       */
> -    if (max_len > 512) {
> +    if (max_len > BDRV_SECTOR_SIZE) {
>          /* XXX: assume 1 sector */
> -        max_len = 512;
> +        max_len = BDRV_SECTOR_SIZE;
>      }
>  
>      memset(buf, 0, max_len);
> @@ -1186,8 +1186,8 @@ static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
>          }
>      }
>  
> -    memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * 512 + 4 ?
> -           IDE_DMA_BUF_SECTORS * 512 + 4 : max_len);
> +    memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 ?
> +           IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 : max_len);

If you're queuing other cleanups, both of these places could usefully use MIN().


r~

Re: [PATCH 5/7] hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:30写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/ide/atapi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 17a9d635d8..14a2b0bb2f 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -824,9 +824,9 @@ static void cmd_get_configuration(IDEState *s, uint8_t *buf)
>       *
>       *      Only a problem if the feature/profiles grow.
>       */
> -    if (max_len > 512) {
> +    if (max_len > BDRV_SECTOR_SIZE) {
>          /* XXX: assume 1 sector */
> -        max_len = 512;
> +        max_len = BDRV_SECTOR_SIZE;
>      }
>
>      memset(buf, 0, max_len);
> @@ -1186,8 +1186,8 @@ static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
>          }
>      }
>
> -    memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * 512 + 4 ?
> -           IDE_DMA_BUF_SECTORS * 512 + 4 : max_len);
> +    memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 ?
> +           IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 : max_len);
>
>      switch (format) {
>          case 0x00 ... 0x7f:
> --
> 2.21.3
>
>

[PATCH 6/7] hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

Use self-explicit definitions instead of magic '512' value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 5e85c4ad17..b50091b615 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -138,7 +138,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, int32_t limit)
     int l, len;
 
     pci_dma_sglist_init(&s->sg, pci_dev,
-                        s->nsector / (BMDMA_PAGE_SIZE / 512) + 1);
+                        s->nsector / (BMDMA_PAGE_SIZE / BDRV_SECTOR_SIZE) + 1);
     s->io_buffer_size = 0;
     for(;;) {
         if (bm->cur_prd_len == 0) {
-- 
2.21.3

Re: [PATCH 6/7] hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ide/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 6/7] hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:33写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/ide/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 5e85c4ad17..b50091b615 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -138,7 +138,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, int32_t limit)
>      int l, len;
>
>      pci_dma_sglist_init(&s->sg, pci_dev,
> -                        s->nsector / (BMDMA_PAGE_SIZE / 512) + 1);
> +                        s->nsector / (BMDMA_PAGE_SIZE / BDRV_SECTOR_SIZE) + 1);
>      s->io_buffer_size = 0;
>      for(;;) {
>          if (bm->cur_prd_len == 0) {
> --
> 2.21.3
>
>

[PATCH 7/7] hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE

[Philippe Mathieu-Daudé]

Use self-explicit definitions instead of magic '512' value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8ce68a9dd6..7612035a4e 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -71,7 +71,7 @@ typedef struct SCSIDiskClass {
 
 typedef struct SCSIDiskReq {
     SCSIRequest req;
-    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
+    /* Both sector and sector_count are in terms of BDRV_SECTOR_SIZE bytes.  */
     uint64_t sector;
     uint32_t sector_count;
     uint32_t buflen;
@@ -141,7 +141,7 @@ static void scsi_init_iovec(SCSIDiskReq *r, size_t size)
         r->buflen = size;
         r->iov.iov_base = blk_blockalign(s->qdev.conf.blk, r->buflen);
     }
-    r->iov.iov_len = MIN(r->sector_count * 512, r->buflen);
+    r->iov.iov_len = MIN(r->sector_count * BDRV_SECTOR_SIZE, r->buflen);
     qemu_iovec_init_external(&r->qiov, &r->iov, 1);
 }
 
@@ -311,7 +311,7 @@ static void scsi_read_complete_noio(SCSIDiskReq *r, int ret)
         goto done;
     }
 
-    n = r->qiov.size / 512;
+    n = r->qiov.size / BDRV_SECTOR_SIZE;
     r->sector += n;
     r->sector_count -= n;
     scsi_req_data(&r->req, r->qiov.size);
@@ -505,7 +505,7 @@ static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
         goto done;
     }
 
-    n = r->qiov.size / 512;
+    n = r->qiov.size / BDRV_SECTOR_SIZE;
     r->sector += n;
     r->sector_count -= n;
     if (r->sector_count == 0) {
@@ -1284,7 +1284,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
         } else { /* MODE_SENSE_10 */
             outbuf[7] = 8; /* Block descriptor length  */
         }
-        nb_sectors /= (s->qdev.blocksize / 512);
+        nb_sectors /= (s->qdev.blocksize / BDRV_SECTOR_SIZE);
         if (nb_sectors > 0xffffff) {
             nb_sectors = 0;
         }
@@ -1342,7 +1342,7 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
     start_track = req->cmd.buf[6];
     blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
     trace_scsi_disk_emulate_read_toc(start_track, format, msf >> 1);
-    nb_sectors /= s->qdev.blocksize / 512;
+    nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
     switch (format) {
     case 0:
         toclen = cdrom_read_toc(nb_sectors, outbuf, msf, start_track);
@@ -1738,9 +1738,10 @@ static void scsi_write_same_complete(void *opaque, int ret)
 
     block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
 
-    data->nb_sectors -= data->iov.iov_len / 512;
-    data->sector += data->iov.iov_len / 512;
-    data->iov.iov_len = MIN(data->nb_sectors * 512, data->iov.iov_len);
+    data->nb_sectors -= data->iov.iov_len / BDRV_SECTOR_SIZE;
+    data->sector += data->iov.iov_len / BDRV_SECTOR_SIZE;
+    data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
+                            data->iov.iov_len);
     if (data->iov.iov_len) {
         block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
                          data->iov.iov_len, BLOCK_ACCT_WRITE);
@@ -1805,9 +1806,10 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
 
     data = g_new0(WriteSameCBData, 1);
     data->r = r;
-    data->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
-    data->nb_sectors = nb_sectors * (s->qdev.blocksize / 512);
-    data->iov.iov_len = MIN(data->nb_sectors * 512, SCSI_WRITE_SAME_MAX);
+    data->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+    data->nb_sectors = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+    data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
+                            SCSI_WRITE_SAME_MAX);
     data->iov.iov_base = buf = blk_blockalign(s->qdev.conf.blk,
                                               data->iov.iov_len);
     qemu_iovec_init_external(&data->qiov, &data->iov, 1);
@@ -1980,7 +1982,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
         if ((req->cmd.buf[8] & 1) == 0 && req->cmd.lba) {
             goto illegal_request;
         }
-        nb_sectors /= s->qdev.blocksize / 512;
+        nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
         /* Returned value is the address of the last sector.  */
         nb_sectors--;
         /* Remember the new size for read/write sanity checking. */
@@ -2049,7 +2051,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
             if ((req->cmd.buf[14] & 1) == 0 && req->cmd.lba) {
                 goto illegal_request;
             }
-            nb_sectors /= s->qdev.blocksize / 512;
+            nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
             /* Returned value is the address of the last sector.  */
             nb_sectors--;
             /* Remember the new size for read/write sanity checking. */
@@ -2180,8 +2182,8 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         if (!check_lba_range(s, r->req.cmd.lba, len)) {
             goto illegal_lba;
         }
-        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
-        r->sector_count = len * (s->qdev.blocksize / 512);
+        r->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+        r->sector_count = len * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
         break;
     case WRITE_6:
     case WRITE_10:
@@ -2211,8 +2213,8 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         if (!check_lba_range(s, r->req.cmd.lba, len)) {
             goto illegal_lba;
         }
-        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
-        r->sector_count = len * (s->qdev.blocksize / 512);
+        r->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
+        r->sector_count = len * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
         break;
     default:
         abort();
@@ -2229,9 +2231,9 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
     }
     assert(r->iov.iov_len == 0);
     if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
-        return -r->sector_count * 512;
+        return -r->sector_count * BDRV_SECTOR_SIZE;
     } else {
-        return r->sector_count * 512;
+        return r->sector_count * BDRV_SECTOR_SIZE;
     }
 }
 
@@ -2243,7 +2245,7 @@ static void scsi_disk_reset(DeviceState *dev)
     scsi_device_purge_requests(&s->qdev, SENSE_CODE(RESET));
 
     blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
-    nb_sectors /= s->qdev.blocksize / 512;
+    nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
     if (nb_sectors) {
         nb_sectors--;
     }
-- 
2.21.3

Re: [PATCH 7/7] hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE

[Richard Henderson]

On 8/14/20 1:28 AM, Philippe Mathieu-Daudé wrote:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 7/7] hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE

[Li Qiang]

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年8月14日周五 下午4:34写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8ce68a9dd6..7612035a4e 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -71,7 +71,7 @@ typedef struct SCSIDiskClass {
>
>  typedef struct SCSIDiskReq {
>      SCSIRequest req;
> -    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
> +    /* Both sector and sector_count are in terms of BDRV_SECTOR_SIZE bytes.  */
>      uint64_t sector;
>      uint32_t sector_count;
>      uint32_t buflen;
> @@ -141,7 +141,7 @@ static void scsi_init_iovec(SCSIDiskReq *r, size_t size)
>          r->buflen = size;
>          r->iov.iov_base = blk_blockalign(s->qdev.conf.blk, r->buflen);
>      }
> -    r->iov.iov_len = MIN(r->sector_count * 512, r->buflen);
> +    r->iov.iov_len = MIN(r->sector_count * BDRV_SECTOR_SIZE, r->buflen);
>      qemu_iovec_init_external(&r->qiov, &r->iov, 1);
>  }
>
> @@ -311,7 +311,7 @@ static void scsi_read_complete_noio(SCSIDiskReq *r, int ret)
>          goto done;
>      }
>
> -    n = r->qiov.size / 512;
> +    n = r->qiov.size / BDRV_SECTOR_SIZE;
>      r->sector += n;
>      r->sector_count -= n;
>      scsi_req_data(&r->req, r->qiov.size);
> @@ -505,7 +505,7 @@ static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
>          goto done;
>      }
>
> -    n = r->qiov.size / 512;
> +    n = r->qiov.size / BDRV_SECTOR_SIZE;
>      r->sector += n;
>      r->sector_count -= n;
>      if (r->sector_count == 0) {
> @@ -1284,7 +1284,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf)
>          } else { /* MODE_SENSE_10 */
>              outbuf[7] = 8; /* Block descriptor length  */
>          }
> -        nb_sectors /= (s->qdev.blocksize / 512);
> +        nb_sectors /= (s->qdev.blocksize / BDRV_SECTOR_SIZE);
>          if (nb_sectors > 0xffffff) {
>              nb_sectors = 0;
>          }
> @@ -1342,7 +1342,7 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
>      start_track = req->cmd.buf[6];
>      blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
>      trace_scsi_disk_emulate_read_toc(start_track, format, msf >> 1);
> -    nb_sectors /= s->qdev.blocksize / 512;
> +    nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
>      switch (format) {
>      case 0:
>          toclen = cdrom_read_toc(nb_sectors, outbuf, msf, start_track);
> @@ -1738,9 +1738,10 @@ static void scsi_write_same_complete(void *opaque, int ret)
>
>      block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
>
> -    data->nb_sectors -= data->iov.iov_len / 512;
> -    data->sector += data->iov.iov_len / 512;
> -    data->iov.iov_len = MIN(data->nb_sectors * 512, data->iov.iov_len);
> +    data->nb_sectors -= data->iov.iov_len / BDRV_SECTOR_SIZE;
> +    data->sector += data->iov.iov_len / BDRV_SECTOR_SIZE;
> +    data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
> +                            data->iov.iov_len);
>      if (data->iov.iov_len) {
>          block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>                           data->iov.iov_len, BLOCK_ACCT_WRITE);
> @@ -1805,9 +1806,10 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
>
>      data = g_new0(WriteSameCBData, 1);
>      data->r = r;
> -    data->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> -    data->nb_sectors = nb_sectors * (s->qdev.blocksize / 512);
> -    data->iov.iov_len = MIN(data->nb_sectors * 512, SCSI_WRITE_SAME_MAX);
> +    data->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +    data->nb_sectors = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +    data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
> +                            SCSI_WRITE_SAME_MAX);
>      data->iov.iov_base = buf = blk_blockalign(s->qdev.conf.blk,
>                                                data->iov.iov_len);
>      qemu_iovec_init_external(&data->qiov, &data->iov, 1);
> @@ -1980,7 +1982,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>          if ((req->cmd.buf[8] & 1) == 0 && req->cmd.lba) {
>              goto illegal_request;
>          }
> -        nb_sectors /= s->qdev.blocksize / 512;
> +        nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
>          /* Returned value is the address of the last sector.  */
>          nb_sectors--;
>          /* Remember the new size for read/write sanity checking. */
> @@ -2049,7 +2051,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>              if ((req->cmd.buf[14] & 1) == 0 && req->cmd.lba) {
>                  goto illegal_request;
>              }
> -            nb_sectors /= s->qdev.blocksize / 512;
> +            nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
>              /* Returned value is the address of the last sector.  */
>              nb_sectors--;
>              /* Remember the new size for read/write sanity checking. */
> @@ -2180,8 +2182,8 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
>          if (!check_lba_range(s, r->req.cmd.lba, len)) {
>              goto illegal_lba;
>          }
> -        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> -        r->sector_count = len * (s->qdev.blocksize / 512);
> +        r->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +        r->sector_count = len * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
>          break;
>      case WRITE_6:
>      case WRITE_10:
> @@ -2211,8 +2213,8 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
>          if (!check_lba_range(s, r->req.cmd.lba, len)) {
>              goto illegal_lba;
>          }
> -        r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> -        r->sector_count = len * (s->qdev.blocksize / 512);
> +        r->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +        r->sector_count = len * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
>          break;
>      default:
>          abort();
> @@ -2229,9 +2231,9 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
>      }
>      assert(r->iov.iov_len == 0);
>      if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
> -        return -r->sector_count * 512;
> +        return -r->sector_count * BDRV_SECTOR_SIZE;
>      } else {
> -        return r->sector_count * 512;
> +        return r->sector_count * BDRV_SECTOR_SIZE;
>      }
>  }
>
> @@ -2243,7 +2245,7 @@ static void scsi_disk_reset(DeviceState *dev)
>      scsi_device_purge_requests(&s->qdev, SENSE_CODE(RESET));
>
>      blk_get_geometry(s->qdev.conf.blk, &nb_sectors);
> -    nb_sectors /= s->qdev.blocksize / 512;
> +    nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
>      if (nb_sectors) {
>          nb_sectors--;
>      }
> --
> 2.21.3
>
>

Re: [PATCH 7/7] hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE

[Kevin Wolf]

Am 14.08.2020 um 10:28 hat Philippe Mathieu-Daudé geschrieben:
> Use self-explicit definitions instead of magic '512' value.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

In this patch, BDRV_SECTOR_SIZE actually looks correct to me. The values
have already been converted from s->qdev.blocksize in these instances,
so it's not about the sector size of the virtual disk, but purely about
QEMU's internal handling.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 0/7] block: Use definitions instead of magic values

[Stefano Garzarella]

On Fri, Aug 14, 2020 at 10:28:34AM +0200, Philippe Mathieu-Daudé wrote:
> Trivial block patches:
> - Fix a typo
> - Replace '1 << 30' by '1 * GiB' in null-co
> - Replace 512 by BDRV_SECTOR_SIZE when appropriate.
> 
> Philippe Mathieu-Daudé (7):
>   block/null: Make more explicit the driver default size is 1GiB
>   hw/ide/core: Trivial typo fix
>   hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE
> 
>  block/null.c        |  4 +++-
>  hw/ide/ahci.c       |  5 +++--
>  hw/ide/atapi.c      |  8 ++++----
>  hw/ide/core.c       | 25 +++++++++++++------------
>  hw/ide/pci.c        |  2 +-
>  hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
>  6 files changed, 47 insertions(+), 41 deletions(-)

Series:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>


Thanks for the cleaning that makes the code more readable!
Stefano

Re: [PATCH 0/7] block: Use definitions instead of magic values

[Laurent Vivier]

Le 14/08/2020 à 10:28, Philippe Mathieu-Daudé a écrit :
> Trivial block patches:
> - Fix a typo
> - Replace '1 << 30' by '1 * GiB' in null-co
> - Replace 512 by BDRV_SECTOR_SIZE when appropriate.
> 
> Philippe Mathieu-Daudé (7):
>   block/null: Make more explicit the driver default size is 1GiB
>   hw/ide/core: Trivial typo fix
>   hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE
>   hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE
> 
>  block/null.c        |  4 +++-
>  hw/ide/ahci.c       |  5 +++--
>  hw/ide/atapi.c      |  8 ++++----
>  hw/ide/core.c       | 25 +++++++++++++------------
>  hw/ide/pci.c        |  2 +-
>  hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
>  6 files changed, 47 insertions(+), 41 deletions(-)
> 

Applied to my trivial-patches branch.

Except the following ones that have comment from Kevin:

[PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB
[PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

Thanks,
Laurent

Re: [PATCH 0/7] block: Use definitions instead of magic values

[Philippe Mathieu-Daudé]

On 9/1/20 11:27 AM, Laurent Vivier wrote:
> Le 14/08/2020 à 10:28, Philippe Mathieu-Daudé a écrit :
>> Trivial block patches:
>> - Fix a typo
>> - Replace '1 << 30' by '1 * GiB' in null-co
>> - Replace 512 by BDRV_SECTOR_SIZE when appropriate.
>>
>> Philippe Mathieu-Daudé (7):
>>   block/null: Make more explicit the driver default size is 1GiB
>>   hw/ide/core: Trivial typo fix
>>   hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE
>>   hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE
>>   hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE
>>   hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE
>>   hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE
>>
>>  block/null.c        |  4 +++-
>>  hw/ide/ahci.c       |  5 +++--
>>  hw/ide/atapi.c      |  8 ++++----
>>  hw/ide/core.c       | 25 +++++++++++++------------
>>  hw/ide/pci.c        |  2 +-
>>  hw/scsi/scsi-disk.c | 44 +++++++++++++++++++++++---------------------
>>  6 files changed, 47 insertions(+), 41 deletions(-)
>>
> 
> Applied to my trivial-patches branch.
> 
> Except the following ones that have comment from Kevin:
> 
> [PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB
> [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE

Thanks!

Phil.