Re: [PATCH 0/3] Add qemu-img checksum command using blkhash
ping Kevin, Hanna, I hope you have time to take a look. https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00021.html On Thu, Sep 1, 2022 at 5:32 PM Nir Soffer wrote: > > Since blkhash is available only via copr now, the new command is added as > optional feature, built only if blkhash-devel package is installed. > > Nir Soffer (3): > qemu-img: Add checksum command > iotests: Test qemu-img checksum > qemu-img: Speed up checksum > > docs/tools/qemu-img.rst | 22 + > meson.build | 10 +- > meson_options.txt | 2 + > qemu-img-cmds.hx | 8 + > qemu-img.c| 388 ++ > tests/qemu-iotests/tests/qemu-img-checksum| 149 +++ > .../qemu-iotests/tests/qemu-img-checksum.out | 74 > 7 files changed, 652 insertions(+), 1 deletion(-) > create mode 100755 tests/qemu-iotests/tests/qemu-img-checksum > create mode 100644 tests/qemu-iotests/tests/qemu-img-checksum.out > > -- > 2.37.2 >
Re: [PATCH 01/11] hw/ppc/meson: Allow e500 boards to be enabled separately
On 15/9/22 17:25, Bernhard Beschow wrote:
Gives users more fine-grained control over what should be compiled into
QEMU.
Signed-off-by: Bernhard Beschow
---
configs/devices/ppc-softmmu/default.mak | 3 ++-
hw/ppc/Kconfig | 8
hw/ppc/meson.build | 6 ++
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/configs/devices/ppc-softmmu/default.mak
b/configs/devices/ppc-softmmu/default.mak
index 658a454426..a887f5438b 100644
--- a/configs/devices/ppc-softmmu/default.mak
+++ b/configs/devices/ppc-softmmu/default.mak
@@ -1,7 +1,8 @@
# Default configuration for ppc-softmmu
# For embedded PPCs:
-CONFIG_E500=y
+CONFIG_E500PLAT=y
+CONFIG_MPC8544DS=y
CONFIG_PPC405=y
CONFIG_PPC440=y
CONFIG_VIRTEX=y
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 3a4418a69e..22a64745d4 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -132,6 +132,14 @@ config E500
select FDT_PPC
select DS1338
+config E500PLAT
+bool
+select E500
+
+config MPC8544DS
+bool
+select E500
+
config VIRTEX
bool
select PPC4XX
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 62801923f3..32babc9b48 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -71,12 +71,10 @@ ppc_ss.add(when: 'CONFIG_MAC_OLDWORLD', if_true:
files('mac_oldworld.c'))
# NewWorld PowerMac
ppc_ss.add(when: 'CONFIG_MAC_NEWWORLD', if_true: files('mac_newworld.c'))
# e500
+ppc_ss.add(when: 'CONFIG_E500PLAT', if_true: files('e500plat.c'))
+ppc_ss.add(when: 'CONFIG_MPC8544DS', if_true: files('mpc8544ds.c'))
ppc_ss.add(when: 'CONFIG_E500', if_true: files(
'e500.c',
- 'mpc8544ds.c',
- 'e500plat.c'
-))
-ppc_ss.add(when: 'CONFIG_E500', if_true: files(
'mpc8544_guts.c',
Reviewed-by: Philippe Mathieu-Daudé
'ppce500_spin.c'
))
Re: [PATCH 05/11] hw/ppc/e500: Remove if statement which is now always true
On 15/9/22 17:25, Bernhard Beschow wrote: Now that the MPC8544DS board also has a platform bus, the if statement was always true. s/was/is/. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Bernhard Beschow --- hw/ppc/e500.c | 30 ++ hw/ppc/e500.h | 1 - hw/ppc/e500plat.c | 1 - hw/ppc/mpc8544ds.c | 1 - 4 files changed, 14 insertions(+), 19 deletions(-)
Re: [PATCH 02/11] hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx
On 15/9/22 17:25, Bernhard Beschow wrote: Having a dedicated config switch makes dependency handling cleaner. Signed-off-by: Bernhard Beschow --- hw/gpio/Kconfig | 3 +++ hw/gpio/meson.build | 2 +- hw/ppc/Kconfig | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 08/11] hw/sd/sdhci-internal: Unexport ESDHC defines
On 15/9/22 17:25, Bernhard Beschow wrote: These defines aren't used outside of sdhci.c, so can be defined there. Signed-off-by: Bernhard Beschow --- hw/sd/sdhci-internal.h | 20 hw/sd/sdhci.c | 19 +++ 2 files changed, 19 insertions(+), 20 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 03/11] docs/system/ppc/ppce500: Add heading for networking chapter
On 15/9/22 17:25, Bernhard Beschow wrote: The sudden change of topics is slightly confusing and makes the networking information less visible. So separate the networking chapter to improve comprehensibility. Signed-off-by: Bernhard Beschow --- docs/system/ppc/ppce500.rst | 3 +++ 1 file changed, 3 insertions(+) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 00/11] ppc/e500: Add support for two types of flash, cleanup
On 15/9/22 17:25, Bernhard Beschow wrote:
This series adds support for -pflash and direct SD card access to the
PPC e500 boards. The idea is to increase compatibility with "real" firmware
images where only the bare minimum of drivers is compiled in.
The series is structured as follows:
Patches 1-3 perform some general cleanup which paves the way for the rest of
the series.
Patches 4-7 add -pflash handling where memory-mapped flash can be added on
user's behalf. That is, the flash memory region is only added if the -pflash
argument is supplied. Note that the cfi01 device model becomes stricter in
checking the size of the emulated flash space.
Patches 8-11 add a new device model - the Freescale eSDHC - to the e500
boards which was missing so far.
User documentation is also added as the new features become available.
Future possible cleanup: Looking at the e500 component, the MPC8544 GUTS
added in commit b0fb84236d ("PPC: E500: Implement reboot controller")
was used in hw/ppce500_mpc8544ds.c board, but in a later refactor
(commit 4a18e7c92a "PPC: e500: rename mpc8544ds into generic file")
this file got renamed as hw/ppc/e500.c; having a MPC8544 specific piece
in the common e500 seems odd.
Re: [PATCH v11 11/21] jobs: group together API calls under the same job lock
Am 14/09/2022 um 14:36 schrieb Vladimir Sementsov-Ogievskiy:
> On 8/26/22 16:20, Emanuele Giuseppe Esposito wrote:
>> Now that the API offers also _locked() functions, take advantage
>> of it and give also the caller control to take the lock and call
>> _locked functions.
>>
>> This makes sense especially when we have for loops, because it
>> makes no sense to have:
>>
>> for(job = job_next(); ...)
>>
>> where each job_next() takes the lock internally.
>> Instead we want
>>
>> JOB_LOCK_GUARD();
>> for(job = job_next_locked(); ...)
>>
>> In addition, protect also direct field accesses, by either creating a
>> new critical section or widening the existing ones.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block.c | 17 ++---
>> blockdev.c | 14 ++
>> blockjob.c | 35 ++-
>> job-qmp.c | 9 ++---
>> job.c | 7 +--
>> monitor/qmp-cmds.c | 7 +--
>> qemu-img.c | 17 +++--
>> 7 files changed, 69 insertions(+), 37 deletions(-)
>>
>
> [..]
>
>> diff --git a/job.c b/job.c
>> index dcd561fd46..e336af0c1c 100644
>> --- a/job.c
>> +++ b/job.c
>
> job.c hunks are unrelated in this patch, they should go to patch 05.
>
>> @@ -672,7 +672,7 @@ void coroutine_fn job_pause_point(Job *job)
>> job_pause_point_locked(job);
>> }
>> -void job_yield_locked(Job *job)
>> +static void job_yield_locked(Job *job)
>> {
>> assert(job->busy);
>> @@ -1046,11 +1046,14 @@ static void
>> job_completed_txn_abort_locked(Job *job)
>> /* Called with job_mutex held, but releases it temporarily */
>> static int job_prepare_locked(Job *job)
>> {
>> + int ret;
>> +
>> GLOBAL_STATE_CODE();
>> if (job->ret == 0 && job->driver->prepare) {
>> job_unlock();
>> - job->ret = job->driver->prepare(job);
>> + ret = job->driver->prepare(job);
>> job_lock();
>> + job->ret = ret;
>> job_update_rc_locked(job);
>> }
>> return job->ret;
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index 7314cd813d..81c8fdadf8 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -135,8 +135,11 @@ void qmp_cont(Error **errp)
>> blk_iostatus_reset(blk);
>> }
>> - for (job = block_job_next(NULL); job; job = block_job_next(job)) {
>> - block_job_iostatus_reset(job);
>> + WITH_JOB_LOCK_GUARD() {
>> + for (job = block_job_next_locked(NULL); job;
>> + job = block_job_next_locked(job)) {
>> + block_job_iostatus_reset_locked(job);
>> + }
>> }
>> /* Continuing after completed migration. Images have been
>> inactivated to
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 7d4b33b3da..c8ae704166 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -912,25 +912,30 @@ static void run_block_job(BlockJob *job, Error
>> **errp)
>> int ret = 0;
>> aio_context_acquire(aio_context);
>> - job_ref(&job->job);
>> + job_lock();
>> + job_ref_locked(&job->job);
>> do {
>> float progress = 0.0f;
>> + job_unlock();
>> aio_poll(aio_context, true);
>> progress_get_snapshot(&job->job.progress, &progress_current,
>> - &progress_total);
>> + &progress_total);
>
> broken indentation
>
>> if (progress_total) {
>> progress = (float)progress_current / progress_total *
>> 100.f;
>> }
>> qemu_progress_print(progress, 0);
>> - } while (!job_is_ready(&job->job) && !job_is_completed(&job->job));
>> + job_lock();
>> + } while (!job_is_ready_locked(&job->job) &&
>> + !job_is_completed_locked(&job->job));
>
> and here
Makes sense, I'll fix it.
Thank you,
Emanuele
>
>> - if (!job_is_completed(&job->job)) {
>> - ret = job_complete_sync(&job->job, errp);
>> + if (!job_is_completed_locked(&job->job)) {
>> + ret = job_complete_sync_locked(&job->job, errp);
>> } else {
>> ret = job->job.ret;
>> }
>> - job_unref(&job->job);
>> + job_unref_locked(&job->job);
>> + job_unlock();
>> aio_context_release(aio_context);
>> /* publish completion progress only when success */
>
>
Re: [PATCH v11 13/21] jobs: protect job.aio_context with BQL and job_mutex
Am 14/09/2022 um 15:25 schrieb Vladimir Sementsov-Ogievskiy:
> On 8/26/22 16:20, Emanuele Giuseppe Esposito wrote:
>> In order to make it thread safe, implement a "fake rwlock",
>> where we allow reads under BQL *or* job_mutex held, but
>> writes only under BQL *and* job_mutex.
>>
>> The only write we have is in child_job_set_aio_ctx, which always
>> happens under drain (so the job is paused).
>> For this reason, introduce job_set_aio_context and make sure that
>> the context is set under BQL, job_mutex and drain.
>> Also make sure all other places where the aiocontext is read
>> are protected.
>>
>> The reads in commit.c and mirror.c are actually safe, because always
>> done under BQL.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Suggested-by: Paolo Bonzini
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> block/replication.c | 7 +--
>> blockjob.c | 3 ++-
>> include/qemu/job.h | 23 ---
>> job.c | 12
>> 4 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/replication.c b/block/replication.c
>> index 55c8f894aa..6e02d98126 100644
>> --- a/block/replication.c
>> +++ b/block/replication.c
>> @@ -142,14 +142,17 @@ static void replication_close(BlockDriverState *bs)
>> {
>> BDRVReplicationState *s = bs->opaque;
>> Job *commit_job;
>> + GLOBAL_STATE_CODE();
>> if (s->stage == BLOCK_REPLICATION_RUNNING) {
>> replication_stop(s->rs, false, NULL);
>> }
>> if (s->stage == BLOCK_REPLICATION_FAILOVER) {
>> commit_job = &s->commit_job->job;
>> - assert(commit_job->aio_context ==
>> qemu_get_current_aio_context());
>> - job_cancel_sync(commit_job, false);
>> + WITH_JOB_LOCK_GUARD() {
>> + assert(commit_job->aio_context ==
>> qemu_get_current_aio_context());
>> + job_cancel_sync_locked(commit_job, false);
>> + }
>
> As Kevin said, this hunk seems not needed.. Why to add locking for
> reading aio_context, when we have GLOBAL_STATE_CODE()?
Ok, getting rid of it.
>
>> }
>> if (s->mode == REPLICATION_MODE_SECONDARY) {
>> diff --git a/blockjob.c b/blockjob.c
>> index 96fb9d9f73..c8919cef9b 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -162,12 +162,13 @@ static void child_job_set_aio_ctx(BdrvChild *c,
>> AioContext *ctx,
>> bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore);
>> }
>> - job->job.aio_context = ctx;
>> + job_set_aio_context(&job->job, ctx);
>> }
>> static AioContext *child_job_get_parent_aio_context(BdrvChild *c)
>> {
>> BlockJob *job = c->opaque;
>> + GLOBAL_STATE_CODE();
>> return job->job.aio_context;
>> }
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 5709e8d4a8..cede227e67 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -74,11 +74,17 @@ typedef struct Job {
>> /* ProgressMeter API is thread-safe */
>> ProgressMeter progress;
>> + /**
>> + * AioContext to run the job coroutine in.
>> + * The job Aiocontext can be read when holding *either*
>> + * the BQL (so we are in the main loop) or the job_mutex.
>> + * It can only be written when we hold *both* BQL
>> + * and the job_mutex.
>> + */
>> + AioContext *aio_context;
>> - /** Protected by AioContext lock */
>> - /** AioContext to run the job coroutine in */
>> - AioContext *aio_context;
>> + /** Protected by AioContext lock */
>> /** Reference count of the block job */
>> int refcnt;
>> @@ -741,4 +747,15 @@ int job_finish_sync(Job *job, void (*finish)(Job
>> *, Error **errp),
>> int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error
>> **errp),
>> Error **errp);
>> +/**
>> + * Sets the @job->aio_context.
>> + * Called with job_mutex *not* held.
>> + *
>> + * This function must run in the main thread to protect against
>> + * concurrent read in job_finish_sync_locked(), takes the job_mutex
>> + * lock to protect against the read in job_do_yield_locked(), and must
>> + * be called when the coroutine is quiescent.
>
> May be "job is quiscent" or "job is doing nothing", "no in-flight io
> operations in job".
>
> For example, backup has several running coroutines in contest of
> block_copy process, and main coroutine of the job
> is almost always "quescent"..
"job is quiescent" seems ok
>
>> + */
>> +void job_set_aio_context(Job *job, AioContext *ctx);
>> +
>> #endif
>> diff --git a/job.c b/job.c
>> index 85ae843f03..9f2fb2e73b 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -396,6 +396,17 @@ Job *job_get(const char *id)
>> return job_get_locked(id);
>> }
>> +void job_set_aio_context(Job *job, AioContext *ctx)
>> +{
>> + /* protect against read in job_finish_sync_locked and job_start */
>> + GLOBAL_STATE_CODE();
>> + /* protect against read in job_do_yield_locked
Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
Am 15/09/2022 um 16:52 schrieb Vladimir Sementsov-Ogievskiy:
> On 8/26/22 16:21, Emanuele Giuseppe Esposito wrote:
>> Change the job_{lock/unlock} and macros to use job_mutex.
>>
>> Now that they are not nop anymore, remove the aiocontext
>> to avoid deadlocks.
>>
>> Therefore:
>> - when possible, remove completely the aiocontext lock/unlock pair
>> - if it is used by some other function too, reduce the locking
>> section as much as possible, leaving the job API outside.
>> - change AIO_WAIT_WHILE in AIO_WAIT_WHILE_UNLOCKED, since we
>> are not using the aiocontext lock anymore
>>
>> The only functions that still need the aiocontext lock are the JobDriver
>> callbacks, already documented in job.h. Reduce the locking section to
>> only cover the callback invocation and document the functions that
>> take the AioContext lock, to avoid taking it twice.
>>
>> Also remove real_job_{lock/unlock}, as they are replaced by the
>> public functions.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> Reviewed-by: Stefan Hajnoczi
>> ---
>
> [..]
>
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -911,7 +911,6 @@ static void run_block_job(BlockJob *job, Error
>> **errp)
>> AioContext *aio_context = block_job_get_aio_context(job);
>> int ret = 0;
>> - aio_context_acquire(aio_context);
>> job_lock();
>> job_ref_locked(&job->job);
>> do {
>
> aio_poll() call here, doesn't require aio_context to be acquired?
On the contrary I think, if you see in AIO_WAIT_WHILE we explicitly
release it because we don't want to allow something else to run with the
aiocontext acquired.
>
>> @@ -936,7 +935,6 @@ static void run_block_job(BlockJob *job, Error
>> **errp)
>> }
>> job_unref_locked(&job->job);
>> job_unlock();
>> - aio_context_release(aio_context);
>> /* publish completion progress only when success */
>> if (!ret) {
>
> [..]
>
> In replication_stop, we call job_cancel_sync() inside
> aio_context_acquire - aio_context_release section. Should it be fixed?
I don't think it breaks anything. The question is: what is the
aiocontext there protecting? Do we need it? I have no idea.
>
> Another question, sometimes you move job_start out of
> aio-context-acquire critical section, sometimes not. As I understand,
> it's of for job_start to be called both with acquired aio-context or not
> acquired?
>
Same as above, job_start does not need the aiocontext to be taken
(otherwise job_lock would be useless).
>
> Otherwise looks good to me!
>
Thank you,
Emanuele
Re: [PATCH v11 21/21] job: remove unused functions
Am 14/09/2022 um 16:28 schrieb Vladimir Sementsov-Ogievskiy: > On 8/26/22 16:21, Emanuele Giuseppe Esposito wrote: >> These public functions are not used anywhere, thus can be dropped. >> Also, since this is the final job API that doesn't use AioContext >> lock and replaces it with job_lock, adjust all remaining function >> documentation to clearly specify if the job lock is taken or not. >> >> Also document the locking requirements for a few functions >> where the second version is not removed. >> >> Signed-off-by: Emanuele Giuseppe Esposito >> Reviewed-by: Stefan Hajnoczi >> Reviewed-by: Kevin Wolf >> --- >> include/qemu/job.h | 96 ++ >> job.c | 101 ++--- >> 2 files changed, 34 insertions(+), 163 deletions(-) >> >> diff --git a/include/qemu/job.h b/include/qemu/job.h >> index ad8b32b4ba..762d6a98a7 100644 >> --- a/include/qemu/job.h >> +++ b/include/qemu/job.h >> @@ -384,6 +384,8 @@ JobTxn *job_txn_new(void); >> /** >> * Release a reference that was previously acquired with >> job_txn_add_job or > > [..] > >> +/** >> + * Returns true if the job is user-paused. >> + * Called with job lock held. >> + */ >> bool job_user_paused_locked(Job *job); >> /** >> * Resume the specified @job. >> * Must be paired with a preceding job_user_pause. > > in comment: job_user_puase_locked > > > Please also fix other removed function mentioning in comments, for > example I see in comments mentioning of removed job_ref, job_unref, > job_user_pause... Ok >> @@ -725,9 +703,6 @@ void job_cancel_sync_all(void); >> * Returns the return value from the job. >> * Called with job_lock *not* held. > > in comment: with lock held. > No idea what you mean here. Thank you, Emanuele
Re: [PATCH v9 1/7] include: add zoned device structs
Stefan Hajnoczi 于2022年9月18日周日 04:17写道：
>
> On Thu, Sep 15, 2022 at 06:06:38PM +0800, Sam Li wrote:
> > Eric Blake 于2022年9月15日周四 16:05写道：
> > >
> > > On Sat, Sep 10, 2022 at 01:27:53PM +0800, Sam Li wrote:
> > > > Signed-off-by: Sam Li
> > > > Reviewed-by: Stefan Hajnoczi
> > > > Reviewed-by: Damien Le Moal
> > > > ---
> > > > include/block/block-common.h | 43
> > > > 1 file changed, 43 insertions(+)
> > > >
> > > > diff --git a/include/block/block-common.h b/include/block/block-common.h
> > > > index fdb7306e78..36bd0e480e 100644
> > > > --- a/include/block/block-common.h
> > > > +++ b/include/block/block-common.h
> > > > @@ -49,6 +49,49 @@ typedef struct BlockDriver BlockDriver;
> > > > typedef struct BdrvChild BdrvChild;
> > > > typedef struct BdrvChildClass BdrvChildClass;
> > > >
> > > > +typedef enum BlockZoneOp {
> > > > +BLK_ZO_OPEN,
> > > > +BLK_ZO_CLOSE,
> > > > +BLK_ZO_FINISH,
> > > > +BLK_ZO_RESET,
> > > > +} BlockZoneOp;
> > > > +
> > > > +typedef enum BlockZoneModel {
> > > > +BLK_Z_NONE = 0x0, /* Regular block device */
> > > > +BLK_Z_HM = 0x1, /* Host-managed zoned block device */
> > > > +BLK_Z_HA = 0x2, /* Host-aware zoned block device */
> > > > +} BlockZoneModel;
> > > > +
> > > > +typedef enum BlockZoneCondition {
> > > > +BLK_ZS_NOT_WP = 0x0,
> > > > +BLK_ZS_EMPTY = 0x1,
> > > > +BLK_ZS_IOPEN = 0x2,
> > > > +BLK_ZS_EOPEN = 0x3,
> > > > +BLK_ZS_CLOSED = 0x4,
> > > > +BLK_ZS_RDONLY = 0xD,
> > > > +BLK_ZS_FULL = 0xE,
> > > > +BLK_ZS_OFFLINE = 0xF,
> > > > +} BlockZoneCondition;
> > > > +
> > > > +typedef enum BlockZoneType {
> > > > +BLK_ZT_CONV = 0x1, /* Conventional random writes supported */
> > > > +BLK_ZT_SWR = 0x2, /* Sequential writes required */
> > > > +BLK_ZT_SWP = 0x3, /* Sequential writes preferred */
> > > > +} BlockZoneType;
> > > > +
> > > > +/*
> > > > + * Zone descriptor data structure.
> > > > + * Provides information on a zone with all position and size values in
> > > > bytes.
> > >
> > > I'm glad that you chose bytes here for use in qemu. But since the
> > > kernel struct blk_zone uses sectors instead of bytes, is it worth
> > > adding a sentence that we intentionally use bytes here, different from
> > > Linux, to make it easier for reviewers to realize that scaling when
> > > translating between qemu and kernel is necessary?
> >
> > Sorry about the unit mistake. The zone information is in sectors which
> > is the same as kernel struct blk_zone. I think adding a sentence to
> > inform the sector unit makes it clear what the zone descriptor is.
>
> I'd make the units bytes for consistency with the rest of the QEMU block
> layer. For example, the MapEntry structure that "qemu-img map" reports
> has names with similar fields and they are in bytes:
>
> struct MapEntry {
> int64_t start;
> int64_t length;
>
I think the zone descriptor uses sector units because ioctl() will
report zones in sector units. Making blk_zone.offset =
zone_descriptor.offset is more convenient than using byte units where
it needs make conversions twice(sector -> byte -> sector in zone
descriptors and offset argument in bdrv_co_zone_report). The MapEntry
uses byte units because lseek() in bdrv_co_block_status suggests the
file offset is set to bytes and I think it may be why the rest of the
block layer uses bytes(not sure).
I do not object to using bytes here but it would require some
compromises. If I was wrong about anything, please let me know.
Sam
