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
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~
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
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
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 > >
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
>
>
© 2016 - 2026 Red Hat, Inc.