Re: [Qemu-block] [PATCH v7 4/9] block/nbd: add cmdline and qapi parameter reconnect-delay

[Eric Blake]

On 6/18/19 6:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> Reconnect will be implemented in the following commit, so for now,
> in semantics below, disconnect itself is a "serious error".
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Eric Blake 
> ---
>  qapi/block-core.json | 11 ++-
>  block/nbd.c  | 16 +++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 61124431d8..17faf723e0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3858,13 +3858,22 @@
>  #  traditional "base:allocation" block status (see
>  #  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)
>  #
> +# @reconnect-delay: On an unexpected disconnect, the nbd client tries to
> +#   connect again until succeeding or encountering a serious
> +#   error.  During the first @reconnect-delay seconds, all
> +#   requests are paused and will be rerun on a successful
> +#   reconnect. After that time, any delayed requests and all
> +#   future requests before a successful reconnect will
> +#   immediately fail. Default 0 (Since 4.1)

4.2 now; sorry for the delays.  I can make that change; I'll definitely
stage at least through this patch for my first pull request as soon as
4.2 opens next week; while still looking at the rest of the series to
see if it is ready to go as well.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 3/9] block/nbd: move from quit to state

[Eric Blake]

On 6/18/19 6:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> To implement reconnect we need several states for the client:
> CONNECTED, QUIT and two different CONNECTING states. CONNECTING states
> will be added in the following patches. This patch implements CONNECTED
> and QUIT.
> 
> QUIT means, that we should close the connection and fail all current
> and further requests (like old quit = true).
> 
> CONNECTED means that connection is ok, we can send requests (like old
> quit = false).
> 
> For receiving loop we use a comparison of the current state with QUIT,
> because reconnect will be in the same loop, so it should be looping
> until the end.
> 
> Opposite, for requests we use a comparison of the current state with
> CONNECTED, as we don't want to send requests in future CONNECTING
> states.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Eric Blake 
> ---
>  block/nbd.c | 58 ++---
>  1 file changed, 37 insertions(+), 21 deletions(-)

> @@ -556,7 +572,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
>  s->requests[i].receiving = true;
>  qemu_coroutine_yield();
>  s->requests[i].receiving = false;
> -if (s->quit) {
> +if (s->state != NBD_CLIENT_CONNECTED) {
>  error_setg(errp, "Connection closed");
>  return -EIO;
>  }
> @@ -640,7 +656,7 @@ static coroutine_fn int nbd_co_receive_one_chunk(
>request_ret, qiov, payload, errp);
>  
>  if (ret < 0) {
> -s->quit = true;
> +nbd_channel_error(s, ret);

Minor merge conflict with changes in the meantime; easy enough to sort out.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v8] qemu-io: add pattern file for write command

[Eric Blake]

On 8/7/19 2:06 AM, Denis Plotnikov wrote:
> The patch allows to provide a pattern file for write
> command. There was no similar ability before.
> 
> Signed-off-by: Denis Plotnikov 
> ---

>  
> +static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
> + const char *file_name)
> +{

No comment on the usage of this function? Existing practice in this file
is not the best, but new code can do better.

> +char *buf, *buf_origin;
> +FILE *f = fopen(file_name, "r");
> +int pattern_len;
> +
> +if (!f) {
> +perror(file_name);
> +return NULL;
> +}
> +
> +if (qemuio_misalign) {
> +len += MISALIGN_OFFSET;
> +}
> +
> +buf_origin = buf = blk_blockalign(blk, len);
> +
> +if (qemuio_misalign) {
> +buf_origin += MISALIGN_OFFSET;

Here, you are changing where you point...

> +}
> +
> +pattern_len = fread(buf_origin, 1, len, f);
> +
> +if (ferror(f)) {
> +perror(file_name);
> +goto error;
> +}

...but if you fail here...


> +
> +error:
> +qemu_vfree(buf_origin);

...then you free the wrong pointer.  This MUST use
qemu_io_free(buf_origin) (the same as write_f correctly does with the
misaligned pointer that you return on success).

> @@ -1051,8 +1114,9 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>  return -EINVAL;
>  }
>  
> -if (zflag && Pflag) {
> -printf("-z and -P cannot be specified at the same time\n");
> +if ((int)zflag + (int)Pflag + (int)sflag > 1) {

The casts to int are not necessary.  Adding two bools promotes to int
naturally.

> +printf("Only one of -z, -P, and -s"
> +   "can be specified at the same time\n");

Missing a space; you don't want your user to see "and -scan be".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 3/3] qcow2: add zstd cluster compression

[Eric Blake]

On 7/4/19 8:09 AM, Denis Plotnikov wrote:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of compression ratio in comparison with
> zlib, which, by the moment, has been the only compression

s/by/at/

> method available.
> 
> The performance test results:
> Test compresses and decompresses qemu qcow2 image with just
> installed rhel-7.6 guest.
> Image cluster size: 64K. Image on disk size: 2.2G
> 
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
> 
> compress cmd:
>   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>   src.img [zlib|zstd]_compressed.img
> decompress cmd
>   time ./qemu-img convert -O qcow2
>   [zlib|zstd]_compressed.img uncompressed.img
> 
>compression   decompression
>  zlib   zstd   zlib zstd
> 
> real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
> user 65.0   15.85.3  2.5
> sys   3.30.22.0  2.0
> 
> Both ZLIB and ZSTD gave the same compression ratio: 1.57
> compressed image size in both cases: 1.4G
> 

Nice numbers.

> +++ b/docs/interop/qcow2.txt
> @@ -538,6 +538,9 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
> 8)):
>  Another compressed cluster may map to the tail of the 
> final
>  sector used by this compressed cluster.
>  
> +The layout of the compressed data depends on the 
> compression
> +type used for the image (see compressed cluster layout).
> +
>  If a cluster is unallocated, read requests shall read the data from the 
> backing
>  file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
>  no backing file or the backing file is smaller than the image, they shall 
> read
> @@ -790,3 +793,19 @@ In the image file the 'enabled' state is reflected by 
> the 'auto' flag. If this
>  flag is set, the software must consider the bitmap as 'enabled' and start
>  tracking virtual disk changes to this bitmap from the first write to the
>  virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +=== Compressed cluster layout ===
> +
> +The compressed cluster data may have a different layout depending on the
> +compression type used for the image, and store specific data for the 
> particular
> +compression type.
> +
> +Compressed data layout for the available compression types:
> +(x = data_space_length - 1)
> +
> +zlib:
> +Byte  0 -  x: the compressed data content
> +  all the space provided used for compressed data
> +zstd:
> +Byte  0 -  3: the length of compressed data
> +  4 -  x: the compressed data content

Missing a change to the header description at bytes 104-107 calling out
'1' as meaning zstd (it only calls out '0' or absent as meaning zlib).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 1/3] qcow2: introduce compression type feature

[Eric Blake]

On 7/4/19 8:09 AM, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
> 
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly 2x faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.

provides

> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov 

> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,12 @@ in the description of a field.
>  An External Data File Name header extension 
> may
>  be present if this bit is set.
>  
> -Bits 3-63:  Reserved (set to 0)
> +Bit 3:  Compression type bit. The bit must be set if
> +the compression type differs from default: 
> zlib.

I'd word this 'from the default of zlib.'

> +If the compression type is default the bit 
> must
> +be unset.

Why? I see no reason to forbid a qcow2 image that has the incompatible
bit set but still uses zlib compression.  True, such an image is not as
friendly to older clients, but it is not technically wrong, and newer
clients would still be able to use the image if not for this sentence
telling them they must not.  I'd drop this sentence.

> +
> +Bits 4-63:  Reserved (set to 0)
>  
>   80 -  87:  compatible_features
>  Bitmask of compatible features. An implementation can
> @@ -165,6 +170,20 @@ in the description of a field.
>  Length of the header structure in bytes. For version 2
>  images, the length is always assumed to be 72 bytes.
>  
> +104 - 107:  compression_type
> +Defines the compression method used for compressed 
> clusters.
> +A single compression type is applied to all compressed 
> image
> +clusters.
> +The compression type is set on image creation only.
> +The default compression type is zlib (value: 0).
> +When the compression type differs from the default
> +the compression type bit (incompatible feature bit 3)
> +must be set.

So far, so good.

> +Qemu versions older than 4.1 can use images created with
> +the default compression type without any additional
> +preparations and cannot use images created with any other
> +compression type.

I'm wondering whether we need to spell this out in the spec.  Yes, I
know we spell out other qemu limitations elsewhere, but with a version
number.  But the spec would not be any less correct if you omitted this
sentence.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 1/3] qcow2: introduce compression type feature

[Eric Blake]

On 8/7/19 6:12 PM, Max Reitz wrote:

>>  
>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> +switch (s->compression_type) {
>> +case QCOW2_COMPRESSION_TYPE_ZLIB:
>> +break;
>> +
>> +default:
>> +error_setg(errp, "qcow2: unknown compression type: %u",
>> +   s->compression_type);
>> +return -ENOTSUP;
>> +}
>> +
>> +/*
>> + * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
>> + * the incompatible feature flag must be set
>> + */
>> +
>> +if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
>> +!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>> +error_setg(errp, "qcow2: Invalid compression type setting");
>> +return -EINVAL;
> 
> (1) Why is this block indented twice?
> 
> (2) Do we need this at all?  Sure, it’s a corruption, but do we need to
> reject the image because of it?

Yes, because otherwise there is a high risk of some application
misinterpreting the contents (whether an older qemu that silently
ignores unrecognized headers, and so assumes it can decode compressed
clusters with zlib even though the decode will only succeed with zstd,
or can write a compressed cluster with zlib which then causes corruption
when the newer qemu tries to read it with zstd).  The whole point of an
incompatible bit is to reject opening an image that can't be interpreted
correctly, and where writing may break later readers.

> 
>> +}
>> +
>> +return 0;
>> +}
>> +
> 
> Overall, I don’t see the purpose of this function.  I don’t see any need
> to call it in qcow2_update_header().  And without “does non-zlib
> compression imply the respective incompatible flag?” check, you can just
> inline the rest (checking that we recognize the compression type) into
> qcow2_do_open().
> 

Inlining may indeed be possible; the real question is whether the
function expands later in the series to the point that inlining no
longer makes sense.

>>  /* Called with s->lock held.  */
>>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>int flags, Error **errp)
>> @@ -1318,6 +1344,35 @@ static int coroutine_fn 
>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>  s->compatible_features  = header.compatible_features;
>>  s->autoclear_features   = header.autoclear_features;
>>  
>> +/*
>> + * Handle compression type
>> + * Older qcow2 images don't contain the compression type header.
>> + * Distinguish them by the header length and use
>> + * the only valid (default) compression type in that case
>> + */
>> +if (header.header_length > offsetof(QCowHeader, compression_type)) {
>> +/* sanity check that we can read a compression type */
>> +size_t min_len = offsetof(QCowHeader, compression_type) +
>> + sizeof(header.compression_type);
>> +if (header.header_length < min_len) {
>> +error_setg(errp,
>> +   "Could not read compression type."
>> +   "qcow2 header is too short");
> 
> This will read as “Could not read compression type.qcow2 header is too
> short”.  There should be a space after the full stop (and the full stop
> should maybe be a comma instead).

Indeed, error_setg() should generally not contain '.'

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] tests/test-hbitmap: test next_zero and _next_dirty_area after truncate

[John Snow]

On 8/5/19 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> Test that hbitmap_next_zero and hbitmap_next_dirty_area can find things
> after old bitmap end.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> It's a follow-up for 
> 
> [PATCH for-4.1] util/hbitmap: update orig_size on truncate
> 
>  tests/test-hbitmap.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
> index 592d8219db..eed5d288cb 100644
> --- a/tests/test-hbitmap.c
> +++ b/tests/test-hbitmap.c
> @@ -1004,6 +1004,15 @@ static void test_hbitmap_next_zero_4(TestHBitmapData 
> *data, const void *unused)
>  test_hbitmap_next_zero_do(data, 4);
>  }
>  
> +static void test_hbitmap_next_zero_after_truncate(TestHBitmapData *data,
> +  const void *unused)
> +{
> +hbitmap_test_init(data, L1, 0);
> +hbitmap_test_truncate_impl(data, L1 * 2);
> +hbitmap_set(data->hb, 0, L1);
> +test_hbitmap_next_zero_check(data, 0);
> +}
> +
>  static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
> uint64_t offset,
> uint64_t count)
> @@ -1104,6 +1113,15 @@ static void 
> test_hbitmap_next_dirty_area_4(TestHBitmapData *data,
>  test_hbitmap_next_dirty_area_do(data, 4);
>  }
>  
> +static void test_hbitmap_next_dirty_area_after_truncate(TestHBitmapData 
> *data,
> +const void *unused)
> +{
> +hbitmap_test_init(data, L1, 0);
> +hbitmap_test_truncate_impl(data, L1 * 2);
> +hbitmap_set(data->hb, L1 + 1, 1);
> +test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  g_test_init(, , NULL);
> @@ -1169,6 +1187,8 @@ int main(int argc, char **argv)
>   test_hbitmap_next_zero_0);
>  hbitmap_test_add("/hbitmap/next_zero/next_zero_4",
>   test_hbitmap_next_zero_4);
> +hbitmap_test_add("/hbitmap/next_zero/next_zero_after_truncate",
> + test_hbitmap_next_zero_after_truncate);
>  
>  hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_0",
>   test_hbitmap_next_dirty_area_0);
> @@ -1176,6 +1196,8 @@ int main(int argc, char **argv)
>   test_hbitmap_next_dirty_area_1);
>  hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_4",
>   test_hbitmap_next_dirty_area_4);
> +
> hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_after_truncate",
> + test_hbitmap_next_dirty_area_after_truncate);
>  
>  g_test_run();
>  
> 

Tested-by: John Snow 
Reviewed-by: John Snow 

And staged:

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] backup fixes for 4.1?

[John Snow]

On 7/31/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:41, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here are two small fixes.
>>>
>>> 01 is not a degradation at all, so it's OK for 4.2
>>> 02 is degradation of 3.0, so it's possibly OK for 4.2 too,
>>> but it seems to be real bug and fix is very simple, so,
>>> may be 4.1 is better
>>>
>>> Or you may take the whole series to 4.1 if you want.
>>>
>>
>> I think (1) and (2) can go in for stable after review, but they're not
>> crucial for 4.1 especially at this late of a stage. Should be cataclysms
>> only right now.

You found a cataclysm :(

>>
>> --js
>>
> 
> I can rebase it than on your bitmaps branch. Or, if we want it for stable, 
> maybe,
> I shouldn't?
> 

I rebased these two patches (1 and 3) on top of bitmaps, on top of rc4.

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-block] [PATCH v2 1/3] qcow2: introduce compression type feature

[Max Reitz]

On 04.07.19 15:09, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
> 
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly 2x faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.
> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov 
> ---
>  block/qcow2.c | 95 +++
>  block/qcow2.h | 26 ---
>  docs/interop/qcow2.txt| 21 -
>  include/block/block_int.h |  1 +
>  qapi/block-core.json  | 22 -
>  5 files changed, 155 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3ace3b2209..8fa932a349 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1202,6 +1202,32 @@ static int qcow2_update_options(BlockDriverState *bs, 
> QDict *options,
>  return ret;
>  }
>  
> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> +switch (s->compression_type) {
> +case QCOW2_COMPRESSION_TYPE_ZLIB:
> +break;
> +
> +default:
> +error_setg(errp, "qcow2: unknown compression type: %u",
> +   s->compression_type);
> +return -ENOTSUP;
> +}
> +
> +/*
> + * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
> + * the incompatible feature flag must be set
> + */
> +
> +if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
> +!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> +error_setg(errp, "qcow2: Invalid compression type setting");
> +return -EINVAL;

(1) Why is this block indented twice?

(2) Do we need this at all?  Sure, it’s a corruption, but do we need to
reject the image because of it?

> +}
> +
> +return 0;
> +}
> +

Overall, I don’t see the purpose of this function.  I don’t see any need
to call it in qcow2_update_header().  And without “does non-zlib
compression imply the respective incompatible flag?” check, you can just
inline the rest (checking that we recognize the compression type) into
qcow2_do_open().

>  /* Called with s->lock held.  */
>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>int flags, Error **errp)
> @@ -1318,6 +1344,35 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>  s->compatible_features  = header.compatible_features;
>  s->autoclear_features   = header.autoclear_features;
>  
> +/*
> + * Handle compression type
> + * Older qcow2 images don't contain the compression type header.
> + * Distinguish them by the header length and use
> + * the only valid (default) compression type in that case
> + */
> +if (header.header_length > offsetof(QCowHeader, compression_type)) {
> +/* sanity check that we can read a compression type */
> +size_t min_len = offsetof(QCowHeader, compression_type) +
> + sizeof(header.compression_type);
> +if (header.header_length < min_len) {
> +error_setg(errp,
> +   "Could not read compression type."
> +   "qcow2 header is too short");

This will read as “Could not read compression type.qcow2 header is too
short”.  There should be a space after the full stop (and the full stop
should maybe be a comma instead).

> +   ret = -EINVAL;
> +   goto fail;

These two lines are incorrectly aligned.

> +}
> +
> +header.compression_type = be32_to_cpu(header.compression_type);
> +s->compression_type = header.compression_type;
> +} else {
> +s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> +}
> +
> +ret = check_compression_type(s, errp);
> +if (ret) {
> +goto fail;
> +}
> +
>  if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
>  void *feature_table = NULL;
>  qcow2_read_extensions(bs, header.header_length, ext_end,
> @@ -2434,6 +2489,13 @@ int qcow2_update_header(BlockDriverState *bs)
>  total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>  refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 
> 3);
>  
> +ret = check_compression_type(s, NULL);
> +
> +if (ret) {
> +goto fail;
> +}
> +
> +

Again, I don’t see why this 

[Qemu-block] bitmaps branch rebase

[John Snow]

FYI: I rebased jsnow/bitmaps on top of kwolf/block-next, itself based on
top of v4.1.0-rc4.

I'll post this along with the eventual pull request, but here's the
diffstat against the published patches:

011/33:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
016/33:[] [-C] 'iotests: Add virtio-scsi device helper'
017/33:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
030/33:[0001] [FC] 'block/backup: teach TOP to never copy unallocated
regions'
032/33:[0018] [FC] 'iotests/257: test traditional sync modes'

11: A new hbitmap call was added upstream, changed to
bdrv_dirty_bitmap_next_zero.
16: Context-only (self.has_quit is new context in 040)
17: Removed 'auto' to follow upstream trends in iotest fashion
30: Remove ret = -ECANCELED as agreed on-list;
Context changes for dirty_end patches
32: Fix capitalization in test, as mentioned on list.

I think the changes are actually fairly minimal and translate fairly
directly; let's review the rebase on-list in response to the PULL mails
when I send them.

Thanks,
--js

Re: [Qemu-block] [PATCH v2 3/3] block-backend: Queue requests while drained

[Max Reitz]

On 07.08.19 16:46, Kevin Wolf wrote:
> This fixes devices like IDE that can still start new requests from I/O
> handlers in the CPU thread while the block backend is drained.
> 
> The basic assumption is that in a drain section, no new requests should
> be allowed through a BlockBackend (blk_drained_begin/end don't exist,
> we get drain sections only on the node level). However, there are two
> special cases where requests should not be queued:
> 
> 1. Block jobs: We already make sure that block jobs are paused in a
>drain section, so they won't start new requests. However, if the
>drain_begin is called on the job's BlockBackend first, it can happen
>that we deadlock because the job stays busy until it reaches a pause
>point - which it can't if its requests aren't processed any more.
> 
>The proper solution here would be to make all requests through the
>job's filter node instead of using a BlockBackend. For now, just
>disabling request queuing on the job BlockBackend is simpler.
> 
> 2. In test cases where making requests through bdrv_* would be
>cumbersome because we'd need a BdrvChild. As we already got the
>functionality to disable request queuing from 1., use it in tests,
>too, for convenience.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/sysemu/block-backend.h |  1 +
>  block/backup.c |  1 +
>  block/block-backend.c  | 53 --
>  block/commit.c |  2 ++
>  block/mirror.c |  1 +
>  blockjob.c |  3 ++
>  tests/test-bdrv-drain.c|  1 +
>  7 files changed, 59 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 2/3] mirror: Keep mirror_top_bs drained after dropping permissions

[Max Reitz]

On 07.08.19 16:46, Kevin Wolf wrote:
> mirror_top_bs is currently implicitly drained through its connection to
> the source or the target node. However, the drain section for target_bs
> ends early after moving mirror_top_bs from src to target_bs, so that
> requests can already be restarted while mirror_top_bs is still present
> in the chain, but has dropped all permissions and therefore runs into an
> assertion failure like this:
> 
> qemu-system-x86_64: block/io.c:1634: bdrv_co_write_req_prepare:
> Assertion `child->perm & BLK_PERM_WRITE' failed.
> 
> Keep mirror_top_bs drained until all graph changes have completed.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 9f5c59ece1..642d6570cc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -656,7 +656,10 @@ static int mirror_exit_common(Job *job)
>  s->target = NULL;
>  
>  /* We don't access the source any more. Dropping any WRITE/RESIZE is
> - * required before it could become a backing file of target_bs. */
> + * required before it could become a backing file of target_bs. Not 
> having
> + * these permissions any more means that we can't allow any new requests 
> on
> + * mirror_top_bs from now on, so keep it drained. */

You’re lucky Patchew broke or it would have complained about multi-line
comments needing /* and */ on separate lines. :-)

I’m perfectly happy with this style, though:

Reviewed-by: Max Reitz 

> +bdrv_drained_begin(mirror_top_bs);
>  bs_opaque->stop = true;
>  bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
>   _abort);
> @@ -724,6 +727,7 @@ static int mirror_exit_common(Job *job)
>  bs_opaque->job = NULL;
>  
>  bdrv_drained_end(src);
> +bdrv_drained_end(mirror_top_bs);
>  s->in_drain = false;
>  bdrv_unref(mirror_top_bs);
>  bdrv_unref(src);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 8/8] block/backup: backup_do_cow: use bdrv_dirty_bitmap_next_dirty_area

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> Use effective bdrv_dirty_bitmap_next_dirty_area interface.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 56 ++
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index f19c9195fe..5ede0c8290 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -235,25 +235,28 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  {
>  CowRequest cow_request;
>  int ret = 0;
> -int64_t start, end; /* bytes */
> +uint64_t off, cur_bytes;
> +int64_t aligned_offset, aligned_bytes, aligned_end;
>  BdrvRequestFlags read_flags =
>  is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>  
>  qemu_co_rwlock_rdlock(>flush_rwlock);
>  
> -start = QEMU_ALIGN_DOWN(offset, job->cluster_size);
> -end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
> +aligned_offset = QEMU_ALIGN_DOWN(offset, job->cluster_size);
> +aligned_end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
> +aligned_bytes = aligned_end - aligned_offset;
>  
> -trace_backup_do_cow_enter(job, start, offset, bytes);
> +trace_backup_do_cow_enter(job, aligned_offset, offset, bytes);
>  
> -wait_for_overlapping_requests(job, start, end);
> -cow_request_begin(_request, job, start, end);
> +wait_for_overlapping_requests(job, aligned_offset, aligned_end);
> +cow_request_begin(_request, job, aligned_offset, aligned_end);
>  
>  if (job->initializing_bitmap) {
> -int64_t off, chunk;
> +int64_t chunk;
>  
> -for (off = offset; offset < end; offset += chunk) {
> -ret = backup_bitmap_reset_unallocated(job, off, end - off, 
> );
> +for (off = aligned_offset; off < aligned_end; off += chunk) {
> +ret = backup_bitmap_reset_unallocated(job, off, aligned_end - 
> off,
> +  );
>  if (ret < 0) {
>  chunk = job->cluster_size;
>  }
> @@ -261,47 +264,36 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  }
>  ret = 0;
>  
> -while (start < end) {
> -int64_t dirty_end;
> -int64_t cur_bytes;
> -
> -if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
> -trace_backup_do_cow_skip(job, start);
> -start += job->cluster_size;
> -continue; /* already copied */
> -}
> -
> -dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
> -end - start);
> -if (dirty_end < 0) {
> -dirty_end = end;
> -}
> -
> -trace_backup_do_cow_process(job, start);
> -cur_bytes = MIN(dirty_end - start, job->len - start);
> -bdrv_reset_dirty_bitmap(job->copy_bitmap, start, dirty_end - start);
> +off = aligned_offset;
> +cur_bytes = aligned_bytes;
> +while (bdrv_dirty_bitmap_next_dirty_area(job->copy_bitmap,
> + , _bytes))
> +{
> +trace_backup_do_cow_process(job, off);
> +bdrv_reset_dirty_bitmap(job->copy_bitmap, off, cur_bytes);
>  
>  if (job->use_copy_range) {
> -ret = backup_cow_with_offload(job, start, cur_bytes, read_flags);
> +ret = backup_cow_with_offload(job, off, cur_bytes, read_flags);
>  if (ret < 0) {
>  job->use_copy_range = false;
>  }
>  }
>  if (!job->use_copy_range) {
> -ret = backup_cow_with_bounce_buffer(job, start, cur_bytes,
> +ret = backup_cow_with_bounce_buffer(job, off, cur_bytes,
>  read_flags, error_is_read);
>  }
>  if (ret < 0) {
> -bdrv_set_dirty_bitmap(job->copy_bitmap, start, dirty_end - 
> start);
> +bdrv_set_dirty_bitmap(job->copy_bitmap, off, cur_bytes);
>  break;
>  }
>  
>  /* Publish progress, guest I/O counts as progress too.  Note that the
>   * offset field is an opaque progress value, it is not a disk offset.
>   */
> -start += cur_bytes;
> +off += cur_bytes;
>  job->bytes_read += cur_bytes;
>  job_progress_update(>common.job, cur_bytes);
> +cur_bytes = offset + bytes - off;

Hm, why not aligned_end - off?

(You could also drop aligned_bytes altogether and always set cur_bytes
to aligned_end - off.)

Max

>  }
>  
>  cow_request_end(_request);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 7/8] block/backup: merge duplicated logic into backup_do_cow

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> backup_cow_with_offload and backup_cow_with_bounce_buffer contains a
> lot of duplicated logic. Move it into backup_do_cow.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 83 +++---
>  1 file changed, 31 insertions(+), 52 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 6/8] block/backup: teach backup_cow_with_bounce_buffer to copy more at once

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> backup_cow_with_offload can transfer more than on cluster. Let
> backup_cow_with_bounce_buffer behave similarly. It reduces number
> of IO and there are no needs to copy cluster by cluster.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index eb41e4af4f..c765c073ad 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -104,22 +104,24 @@ static int coroutine_fn 
> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>int64_t start,
>int64_t end,
>bool is_write_notifier,
> -  bool *error_is_read,
> -  void **bounce_buffer)
> +  bool *error_is_read)
>  {
>  int ret;
>  BlockBackend *blk = job->common.blk;
>  int nbytes;
>  int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
> +void *bounce_buffer = blk_try_blockalign(blk, end);

s/end/end - start/ (or probably rather s/end/nbytes/ after that has been
set).

Rest looks good.

Max

>  
> -assert(QEMU_IS_ALIGNED(start, job->cluster_size));
> -bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
> -nbytes = MIN(job->cluster_size, job->len - start);
> -if (!*bounce_buffer) {
> -*bounce_buffer = blk_blockalign(blk, job->cluster_size);
> +if (!bounce_buffer) {
> +return -ENOMEM;
>  }



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

[Richard Henderson]

On 8/7/19 11:00 AM, Paolo Bonzini wrote:
> On 07/08/19 19:49, Richard Henderson wrote:
>> On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
>>> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>>> hwaddr addr,
>>>  /* As length is under guest control, handle illegal values. */
>>>  return;
>>>  }
>>> +/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>>   MEMTXATTRS_UNSPECIFIED);
>>>  }
>>
>> Here is an example of where Paolo is quite right -- you cannot simply add 
>> MO_TE
>> via size_memop().  In patch 22 we see
>>
>>> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy 
>>> *proxy, hwaddr addr,
>>>  val = pci_get_byte(buf);
>>>  break;
>>>  case 2:
>>> -val = cpu_to_le16(pci_get_word(buf));
>>> +val = pci_get_word(buf);
>>>  break;
>>>  case 4:
>>> -val = cpu_to_le32(pci_get_long(buf));
>>> +val = pci_get_long(buf);
>>>  break;
>>>  default:
>>>  /* As length is under guest control, handle illegal values. */
>>>  return;
>>>  }
>>> -/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>>   MEMTXATTRS_UNSPECIFIED);
>>
>> This is a little-endian store -- MO_LE not MO_TE.
> 
> Or leave the switch statement aside and request host endianness.  Either
> is fine.

The goal is to minimize the number of places and the number of times that
bswaps happen.  That's why I think pushing the cpu_to_le16 into
memory_region_dispatch_write is a good thing.

However, leaving a host-endian might be the easiest short-term solution for the
more complicated cases.  It also seems like a way to split the patch further if
we think that's desirable.


r~

Re: [Qemu-block] [PATCH 5/8] block/backup: fix backup_cow_with_offload for last cluster

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> We shouldn't try to copy bytes beyond EOF. Fix it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 4/8] block/backup: improve unallocated clusters skipping

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> Limit block_status querying to request bounds on write notifier to
> avoid extra seeking.

I don’t understand this reasoning.  Checking whether something is
allocated for qcow2 should just mean an L2 cache lookup.  Which we have
to do anyway when we try to copy data off the source.

I could understand saying this makes the code easier, but I actually
don’t think it does.  If you implemented checking the allocation status
only for areas where the bitmap is reset (which I think this patch
should), then it’d just duplicate the existing loop.

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 38 +-
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 11e27c844d..a4d37d2d62 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -267,6 +267,18 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  wait_for_overlapping_requests(job, start, end);
>  cow_request_begin(_request, job, start, end);
>  
> +if (job->initializing_bitmap) {
> +int64_t off, chunk;
> +
> +for (off = offset; offset < end; offset += chunk) {

This is what I’m referring to, I think this loop should skip areas that
are clean.

> +ret = backup_bitmap_reset_unallocated(job, off, end - off, 
> );
> +if (ret < 0) {
> +chunk = job->cluster_size;
> +}
> +}
> +}
> +ret = 0;
> +
>  while (start < end) {
>  int64_t dirty_end;
>  
> @@ -276,15 +288,6 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>  continue; /* already copied */
>  }
>  
> -if (job->initializing_bitmap) {
> -ret = backup_bitmap_reset_unallocated(job, start, _bytes);
> -if (ret == 0) {
> -trace_backup_do_cow_skip_range(job, start, skip_bytes);
> -start += skip_bytes;
> -continue;
> -}
> -}
> -
>  dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
>  end - start);
>  if (dirty_end < 0) {

Hm, you resolved that conflict differently from me.

I decided the bdrv_dirty_bitmap_next_zero() call should go before the
backup_bitmap_reset_unallocated() call so that we can then do a

  dirty_end = MIN(dirty_end, start + skip_bytes);

because we probably don’t want to copy anything past what
backup_bitmap_reset_unallocated() has inquired.


But then again this patch eliminates the difference anyway...

Max

> @@ -546,7 +549,8 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
>  goto out;
>  }
>  
> -ret = backup_bitmap_reset_unallocated(s, offset, );
> +ret = backup_bitmap_reset_unallocated(s, offset, s->len - offset,
> +  );
>  if (ret < 0) {
>  goto out;
>  }
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

[Paolo Bonzini]

On 07/08/19 19:49, Richard Henderson wrote:
> On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
>> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>> hwaddr addr,
>>  /* As length is under guest control, handle illegal values. */
>>  return;
>>  }
>> +/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>   MEMTXATTRS_UNSPECIFIED);
>>  }
> 
> Here is an example of where Paolo is quite right -- you cannot simply add 
> MO_TE
> via size_memop().  In patch 22 we see
> 
>> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
>> hwaddr addr,
>>  val = pci_get_byte(buf);
>>  break;
>>  case 2:
>> -val = cpu_to_le16(pci_get_word(buf));
>> +val = pci_get_word(buf);
>>  break;
>>  case 4:
>> -val = cpu_to_le32(pci_get_long(buf));
>> +val = pci_get_long(buf);
>>  break;
>>  default:
>>  /* As length is under guest control, handle illegal values. */
>>  return;
>>  }
>> -/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>>   MEMTXATTRS_UNSPECIFIED);
> 
> This is a little-endian store -- MO_LE not MO_TE.

Or leave the switch statement aside and request host endianness.  Either
is fine.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH v6 20/26] memory: Access MemoryRegion with endianness

[Richard Henderson]

On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
> @@ -551,6 +551,7 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
> hwaddr addr,
>  /* As length is under guest control, handle illegal values. */
>  return;
>  }
> +/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>   MEMTXATTRS_UNSPECIFIED);
>  }

Here is an example of where Paolo is quite right -- you cannot simply add MO_TE
via size_memop().  In patch 22 we see

> @@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
> hwaddr addr,
>  val = pci_get_byte(buf);
>  break;
>  case 2:
> -val = cpu_to_le16(pci_get_word(buf));
> +val = pci_get_word(buf);
>  break;
>  case 4:
> -val = cpu_to_le32(pci_get_long(buf));
> +val = pci_get_long(buf);
>  break;
>  default:
>  /* As length is under guest control, handle illegal values. */
>  return;
>  }
> -/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
>  memory_region_dispatch_write(mr, addr, val, size_memop(len),
>   MEMTXATTRS_UNSPECIFIED);

This is a little-endian store -- MO_LE not MO_TE.


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 21/26] cputlb: Replace size and endian operands for MemOp

[Richard Henderson]

On 8/7/19 1:33 AM, tony.ngu...@bt.com wrote:
> @@ -1246,7 +1246,7 @@ typedef uint64_t FullLoadHelper(CPUArchState *env, 
> target_ulong addr,
> 
>  static inline uint64_t __attribute__((always_inline))
>  load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
> -uintptr_t retaddr, size_t size, bool big_endian, bool code_read,
> +uintptr_t retaddr, MemOp op, bool code_read,

I assume the code generation is the same, or nearly so, for these functions?
It seems like it should be, since we're replacing one set of constant arguments
with another, and the compiler should be able to fold away the same set of 
tests.

But we should at least have a look...

> +switch (op) {
> +case MO_8:
>  res = ldub_p(haddr);
>  break;
> +case MO_BEUW:
> +res = lduw_be_p(haddr);

I don't like mixing a bare size with size+sign+endian.
I think you should go ahead and pass MO_UB.

> @@ -1605,30 +1605,27 @@ store_helper(CPUArchState *env, target_ulong addr, 
> uint64_t val,
> 
>   do_aligned_access:
>  haddr = (void *)((uintptr_t)addr + entry->addend);
> -switch (size) {
> -case 1:
> +switch (op) {
> +case MO_8:

Likewise.


r~

Re: [Qemu-block] [PATCH 3/8] block/io: handle alignment and max_transfer for copy_range

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> copy_range ignores these limitations, let's improve it. block/backup
> code handles max_transfer for copy_range by itself, now it's not needed
> more, drop it.

Shouldn’t this be two separate patches?

> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 11 ++-
>  block/io.c | 41 +
>  2 files changed, 35 insertions(+), 17 deletions(-)

[...]

> diff --git a/block/io.c b/block/io.c
> index 06305c6ea6..5abbd0fff2 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3005,6 +3005,12 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>  {
>  BdrvTrackedRequest req;
>  int ret;
> +uint32_t align = MAX(src->bs->bl.request_alignment,
> + dst->bs->bl.request_alignment);
> +uint32_t max_transfer =
> +
> QEMU_ALIGN_DOWN(MIN_NON_ZERO(MIN_NON_ZERO(src->bs->bl.max_transfer,
> +  
> dst->bs->bl.max_transfer),
> + INT_MAX), align);

I suppose the outer QEMU_ALIGN_DOWN() may result in @max_transfer of 0
(in theory, if one’s max_transfer is smaller than the other’s alignment).

Not likely, but should still be handled.

The rest looks good to me.

Max

>  /* TODO We can support BDRV_REQ_NO_FALLBACK here */
>  assert(!(read_flags & BDRV_REQ_NO_FALLBACK));



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices

[Damien Hedde]

On 8/7/19 5:07 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>>
>> This add the reset related sections for every QOM
>> device.
> 
> A bit more detail in the commit message would help, I think --
> this is adding extra machinery which has to copy and modify
> the VMStateDescription passed in by the device in order to
> add the subsection that handles reset.

Sorry for that, thought I've added some...

I've kept this patch separate from previous one because this it is
awkward. I'm not sure this is the right place (I mean in qdev files) do
this kind of stuff. It basically replaces every static vmsd by dynamic
ones, so it makes it harder to follow when debugging since there is no
symbol associated to them. But on the other hand, I don't see an
alternative.

I copy there what I've put in the cover-letter:
For devices, I've added a patch to automate the addition of reset
related subsection. In consequence it is not necessary to explicitly add
the reset subsection in every device specialization requiring it.
Right know this is kind of a hack into qdev to dynamically modify the
vmsd before the registration. There is probably a much cleaner way to do
this but I prefered to demonstrate it by keeping modification local to qdev.
As far as I can tell it's ok to dynamically add subsections at the end.
This does not prevent from further adding subsections in the orignal
vmsd: the subsections order in the array is irrelevant from migration
point-of-view. The loading part just lookup each subsection in the array
by name uppon reception.

> 
> I've added Dave Gilbert to the already long cc list since this
> is migration related.
> 
>> Signed-off-by: Damien Hedde 
>> ---
>>  hw/core/qdev-vmstate.c | 41 +
>>  hw/core/qdev.c | 12 +++-
>>  include/hw/qdev-core.h |  3 +++
>>  stubs/Makefile.objs|  1 +
>>  stubs/device.c |  7 +++
>>  5 files changed, 63 insertions(+), 1 deletion(-)
>>  create mode 100644 stubs/device.c
>>
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> index 07b010811f..24f8465c61 100644
>> --- a/hw/core/qdev-vmstate.c
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -43,3 +43,44 @@ const struct VMStateDescription device_vmstate_reset = {
>>  VMSTATE_END_OF_LIST()
>>  },
>>  };
>> +
>> +static VMStateDescription *vmsd_duplicate_and_append(
>> +const VMStateDescription *old_vmsd,
>> +const VMStateDescription *new_subsection)
>> +{
>> +VMStateDescription *vmsd;
>> +int n = 0;
>> +
>> +assert(old_vmsd && new_subsection);
>> +
>> +vmsd = (VMStateDescription *) g_memdup(old_vmsd, sizeof(*vmsd));
>> +
>> +if (old_vmsd->subsections) {
>> +while (old_vmsd->subsections[n]) {
>> +n += 1;
>> +}
>> +}
>> +vmsd->subsections = g_new(const VMStateDescription *, n + 2);
>> +
>> +if (old_vmsd->subsections) {
>> +memcpy(vmsd->subsections, old_vmsd->subsections,
>> +   sizeof(VMStateDescription *) * n);
>> +}
>> +vmsd->subsections[n] = new_subsection;
>> +vmsd->subsections[n + 1] = NULL;
>> +
>> +return vmsd;
>> +}
>> +
>> +void device_class_build_extended_vmsd(DeviceClass *dc)
>> +{
>> +assert(dc->vmsd);
>> +assert(!dc->vmsd_ext);
>> +
>> +/* forge a subsection with proper name */
>> +VMStateDescription *reset;
>> +reset = g_memdup(_vmstate_reset, sizeof(*reset));
>> +reset->name = g_strdup_printf("%s/device_reset", dc->vmsd->name);
>> +
>> +dc->vmsd_ext = vmsd_duplicate_and_append(dc->vmsd, reset);
>> +}
> 
> This will allocate memory, but there is no corresponding
> code which frees it. This means you'll have a memory leak
> across device realize->unrealize for hotplug devices.

Right. I'll handle this along with the existing vmsd_unregister
in realize/unrealize method.

> 
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e9e5f2d5f9..88387d3743 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -45,7 +45,17 @@ bool qdev_hot_removed = false;
>>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>>  {
>>  DeviceClass *dc = DEVICE_GET_CLASS(dev);
>> -return dc->vmsd;
>> +
>> +if (!dc->vmsd) {
>> +return NULL;
>> +}
>> +
>> +if (!dc->vmsd_ext) {
>> +/* build it first time we need it */
>> +device_class_build_extended_vmsd(dc);
>> +}
>> +
>> +return dc->vmsd_ext;
>>  }
> 
> Unfortunately not everything that wants the VMSD calls
> this function. migration/savevm.c:dump_vmstate_json_to_file()
> does a direct access to dc->vmsd, so we need to fix that first.
> 
> Devices which don't use dc->vmsd won't get this and so
> their reset state won't be migrated. That's OK for anything
> that's still not yet a QOM device, I guess -- it's not possible
> for them to be in a 'held in reset' state anyway, so the
> extra subsection would never be needed.
> 
> The one I'm less sure about is the 'virtio' 

Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs

[Damien Hedde]

On 8/7/19 5:18 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>>
>> It adds the possibility to add 2 gpios to control the warm and cold reset.
>> With theses ios, the reset can be maintained during some time.
>> Each io is associated with a state to detect level changes.
>>
>> Vmstate subsections are also added to the existsing device_reset
>> subsection.
>>
>> Signed-off-by: Damien Hedde 
>> ---
>>  hw/core/qdev-vmstate.c | 15 ++
>>  hw/core/qdev.c | 65 ++
>>  include/hw/qdev-core.h | 57 
>>  3 files changed, 137 insertions(+)
>>
>> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
>> index 24f8465c61..72f84c6cee 100644
>> --- a/hw/core/qdev-vmstate.c
>> +++ b/hw/core/qdev-vmstate.c
>> @@ -24,10 +24,23 @@ static int device_vmstate_reset_post_load(void *opaque, 
>> int version_id)
>>  {
>>  DeviceState *dev = (DeviceState *) opaque;
>>  BusState *bus;
>> +uint32_t io_count = 0;
>> +
>>  QLIST_FOREACH(bus, >child_bus, sibling) {
>>  bus->resetting = dev->resetting;
>>  bus->reset_is_cold = dev->reset_is_cold;
>>  }
>> +
>> +if (dev->cold_reset_input.state) {
>> +io_count += 1;
>> +}
>> +if (dev->warm_reset_input.state) {
>> +io_count += 1;
>> +}
>> +/* ensure resetting count is coherent with io states */
>> +if (dev->resetting < io_count) {
>> +return -1;
>> +}
>>  return 0;
>>  }
>>
>> @@ -40,6 +53,8 @@ const struct VMStateDescription device_vmstate_reset = {
>>  .fields = (VMStateField[]) {
>>  VMSTATE_UINT32(resetting, DeviceState),
>>  VMSTATE_BOOL(reset_is_cold, DeviceState),
>> +VMSTATE_BOOL(cold_reset_input.state, DeviceState),
>> +VMSTATE_BOOL(warm_reset_input.state, DeviceState),
> 
> If we're just adding these fields to this VMStateDescription
> then this patch should come earlier in the series than the
> patch where we create and start using the fields. Otherwise
> there's a migration compat break between a QEMU just
> before this patch and a QEMU with it. I think the simplest
> fix is to put this patch before patches 6/7 and have a note
> in the commit message that this functionality can't be used
> until after the patch which adds the migration support.

Independently of the compat break you mention, maybe it is better to
have 'conditional' subsection for each input ?

> 
>>  VMSTATE_END_OF_LIST()
>>  },
>>  };
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 88387d3743..11a4de55ea 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -450,6 +450,67 @@ void qdev_init_gpio_in(DeviceState *dev, 
>> qemu_irq_handler handler, int n)
>>  qdev_init_gpio_in_named(dev, handler, NULL, n);
>>  }
>>
>> +static DeviceResetInputState *device_get_reset_input_state(DeviceState *dev,
>> +bool cold)
>> +{
>> +return cold ? >cold_reset_input : >warm_reset_input;
>> +}
>> +
>> +static void device_reset_handler(DeviceState *dev, bool cold, bool level)
>> +{
>> +DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
>> +
>> +if (dris->type == DEVICE_RESET_ACTIVE_LOW) {
>> +level = !level;
>> +}
>> +
>> +if (dris->state == level) {
>> +/* io state has not changed */
>> +return;
>> +}
>> +
>> +dris->state = level;
>> +
>> +if (level) {
>> +resettable_assert_reset(OBJECT(dev), cold);
>> +} else {
>> +resettable_deassert_reset(OBJECT(dev));
>> +}
>> +}
>> +
>> +static void device_cold_reset_handler(void *opaque, int n, int level)
>> +{
>> +device_reset_handler((DeviceState *) opaque, true, level);
>> +}
>> +
>> +static void device_warm_reset_handler(void *opaque, int n, int level)
>> +{
>> +device_reset_handler((DeviceState *) opaque, false, level);
>> +}
>> +
>> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
>> +   bool cold, DeviceResetActiveType type)
>> +{
>> +DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
>> +qemu_irq_handler handler;
>> +
>> +switch (type) {
>> +case DEVICE_RESET_ACTIVE_LOW:
>> +case DEVICE_RESET_ACTIVE_HIGH:
>> +break;
>> +default:
>> +assert(false);
>> +break;
> 
> The usual way to write this is
> g_assert_not_reached();
> (and no following 'break').
> 
> 
> But the whole switch statement seems to be a complicated way
> of writing
>assert(type == DEVICE_RESET_ACTIVE_LOW || type == 
> DEVICE_RESET_ACTIVE_HIGH);
> 
>> +}
>> +
>> +assert(!dris->exists);
>> +dris->exists = true;
>> +dris->type = type;
>> +
>> +handler = cold ? device_cold_reset_handler : device_warm_reset_handler;
>> +qdev_init_gpio_in_named(dev, handler, name, 1);
>> +}
> 
> thanks
> -- PMM
> 

Re: [Qemu-block] [PATCH 2/8] block/backup: refactor write_flags

[Max Reitz]

On 07.08.19 10:07, Vladimir Sementsov-Ogievskiy wrote:
> write flags are constant, let's store it in BackupBlockJob instead of
> recalculating. It also makes two boolean fields to be unused, so,
> drop them.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: John Snow 
> ---
>  block/backup.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2] util/hbitmap: strict hbitmap_reset

[John Snow]

On 8/6/19 12:19 PM, Vladimir Sementsov-Ogievskiy wrote:
> 06.08.2019 19:09, Max Reitz wrote:
>> On 06.08.19 17:26, Vladimir Sementsov-Ogievskiy wrote:
>>> hbitmap_reset has an unobvious property: it rounds requested region up.
>>> It may provoke bugs, like in recently fixed write-blocking mode of
>>> mirror: user calls reset on unaligned region, not keeping in mind that
>>> there are possible unrelated dirty bytes, covered by rounded-up region
>>> and information of this unrelated "dirtiness" will be lost.
>>>
>>> Make hbitmap_reset strict: assert that arguments are aligned, allowing
>>> only one exception when @start + @count == hb->orig_size. It's needed
>>> to comfort users of hbitmap_next_dirty_area, which cares about
>>> hb->orig_size.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>
>>> v2 based on Max's https://github.com/XanClic/qemu.git block
>>> which will be merged soon to 4.1, and this patch goes to 4.2
>>> Based-on: https://github.com/XanClic/qemu.git block
>>>
>>> v1 was "[PATCH] util/hbitmap: fix unaligned reset", and as I understand
>>> we all agreed to just assert alignment instead of aligning down
>>> automatically.
>>>
>>>   include/qemu/hbitmap.h | 5 +
>>>   tests/test-hbitmap.c   | 2 +-
>>>   util/hbitmap.c | 4 
>>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>>> index 4afbe6292e..7865e819ca 100644
>>> --- a/include/qemu/hbitmap.h
>>> +++ b/include/qemu/hbitmap.h
>>> @@ -132,6 +132,11 @@ void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t 
>>> count);
>>>* @count: Number of bits to reset.
>>>*
>>>* Reset a consecutive range of bits in an HBitmap.
>>> + * @start and @count must be aligned to bitmap granularity. The only 
>>> exception
>>> + * is resetting the tail of the bitmap: @count may be equal to @start +
>>> + * hb->orig_size,
>>
>> s/@start + hb->orig_size/hb->orig_size - @start/, I think.
> 
> Ha, I wanted to say start + count equal to orig_size. Yours is OK too of 
> course.
> 
>>
>>>  in this case @count may be not aligned. @start + @count
>>
>> +are
>>
>> With those fixed:
>>
>> Reviewed-by: Max Reitz 
> 
> Thanks!
> 

I'll add this to the pile for 4.2, after I fix the rebase conflicts that
arose from 4.1-rc4.

--js

Re: [Qemu-block] [Qemu-devel] [PATCH v6 18/26] exec: Delete device_endian

[Richard Henderson]

On 8/7/19 1:32 AM, tony.ngu...@bt.com wrote:
> device_endian has been made redundant by MemOp.
> 
> Signed-off-by: Tony Nguyen 
> ---
>  include/exec/cpu-common.h | 8 
>  1 file changed, 8 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 17/26] exec: Replace device_endian with MemOp

[Richard Henderson]

On 8/7/19 1:31 AM, tony.ngu...@bt.com wrote:
> Simplify endianness comparisons with consistent use of the more
> expressive MemOp.
> 
> Suggested-by: Richard Henderson 
> Signed-off-by: Tony Nguyen  ---

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/26] exec: Map device_endian onto MemOp

[Richard Henderson]

On 8/7/19 8:59 AM, Richard Henderson wrote:
> On 8/7/19 1:31 AM, tony.ngu...@bt.com wrote:
>> +  _mm_ops[end == DEVICE_LITTLE_ENDIAN ? 0 : 
>> 1],
> 
> This is of course "end != DEVICE_LITTLE_ENDIAN".

And by that I mean drop the ?: operator.


r~

Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface

[Peter Maydell]

On Wed, 31 Jul 2019 at 07:33, David Gibson  wrote:
>
> On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote:
> > Signed-off-by: Damien Hedde 
> > +For Devices and Buses there is also the corresponding helpers:
> > +void device_reset(Device *dev, bool cold)
> > +void bus_reset(Device *dev, bool cold)
>
> What's the semantic difference between resetting a bus and resetting
> the bridge device which owns it?

We should definitely explain this in the documentation, but
consider for instance a SCSI controller. Resetting the
SCSI controller puts all its registers back into whatever
the reset state is for the device, as well as resetting
everything on the SCSI bus. Resetting just the SCSI bus
resets the disks and so on on the bus, but doesn't change
the state of the controller itself, which remains programmed
with whatever state the guest has set up.

PCI has a similar distinction between resetting the controller
and resetting the bus.

Note that we have this distinction in the current APIs too:
qbus_reset_all() vs qdev_reset_all().


thanks
-- PMM

Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:

> +For Devices and Buses there is also the corresponding helpers:
> +void device_reset(Device *dev, bool cold)
> +void bus_reset(Device *dev, bool cold

Just noticed, but the prototype here is wrong: bus_reset() takes
a BusState*, not a Device*.

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/26] exec: Map device_endian onto MemOp

[Richard Henderson]

On 8/7/19 1:31 AM, tony.ngu...@bt.com wrote:
> +  _mm_ops[end == DEVICE_LITTLE_ENDIAN ? 0 : 
> 1],

This is of course "end != DEVICE_LITTLE_ENDIAN".


r~

Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> Signed-off-by: Damien Hedde 
> ---
>  docs/devel/reset.txt | 165 +++
>  1 file changed, 165 insertions(+)
>  create mode 100644 docs/devel/reset.txt
>
> diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt
> new file mode 100644
> index 00..c7a1eb068f
> --- /dev/null
> +++ b/docs/devel/reset.txt
> @@ -0,0 +1,165 @@
> +
> +=
> +Reset
> +=
> +
> +The reset of qemu objects is handled using the Resettable interface declared
> +in *include/hw/resettable.h*.
> +As of now DeviceClass and BusClass implement this interface.
> +> +
> +Triggering reset
> +

I think this would be clearer if we documented it the other way around:

  This section documents the APIs which "users" of a resettable
  object should use to control it.

  A resettable object can be put into its "in reset" state and
  held there indefinitely.

  [documentation for resettable_assert/deassert_reset goes here]

  If you want to just trigger a reset event but not leave the
  object in reset for any period of time, you can use
  resettable_reset(), which is a convenience function identical
  in behaviour to calling resettable_assert() and then immediately
  calling resettable_deassert().

  [resettable_reset() docs here]

> +
> +The function *resettable_reset* is used to trigger a reset on a given
> +object.
> +void resettable_reset(Object *obj, bool cold)
> +
> +The parameter *obj* must implement the Resettable interface.
> +The parameter *cold* is a boolean specifying whether to do a cold or warm
> +reset
> +
> +For Devices and Buses there is also the corresponding helpers:
> +void device_reset(Device *dev, bool cold)
> +void bus_reset(Device *dev, bool cold)

We should explain what these do.

> +
> +If one wants to put an object into a reset state. There is the
> +*resettable_assert_reset* function.
> +void resettable_assert_reset(Object *obj, bool cold)
> +
> +One must eventually call the function *resettable_deassert_reset* to end the
> +reset state:
> +void resettable_deassert_reset(Object *obj, bool cold)
> +
> +Calling *resettable_assert_reset* then *resettable_deassert_reset* is the
> +same as calling *resettable_reset*.
> +
> +It is possible to interleave multiple calls to
> + - resettable_reset,
> + - resettable_assert_reset, and
> + - resettable_deassert_reset.
> +The only constraint is that *resettable_deassert_reset* must be called once
> +per *resettable_assert_reset* call so that the object leaves the reset state.
> +
> +Therefore there may be several reset sources/controllers of a given object.
> +The interface handle everything and the controllers do not need to know

"handles"

> +anything about each others. The object will leave reset state only when all

"each other"

> +controllers released their reset.

"release"

> +
> +All theses functions must called while holding the iothread lock.

"these"; "be called"

> +
> +
> +Implementing reset for a Resettable object : Multi-phase reset
> +--

  This section documents the APIs that an implementation of a
  resettable object must provide.

> +
> +The Resettable uses a multi-phase mechanism to handle some ordering 
> constraints
> +when resetting multiple object at the same time. For a given object the reset

"objects"

> +procedure is split into three different phases executed in order:
> + 1 INIT: This phase should set/reset the state of the Resettable it has when 
> is
> + in reset state. Side-effects to others object is forbidden (such as
> + setting IO level).
> + 2 HOLD: This phase corresponds to the external side-effects due to staying 
> into
> + the reset state.
> + 3 EXIT: This phase corresponds to leaving the reset state. It have both
> + local and external effects.

We should be a bit more detailed here. I had some text in
my review of patch 1 for the doc-comments which we can put here too.

> +
> +*resettable_assert_reset* does the INIT and HOLD phases. While
> +*resettable_deassert_reset* does the EXIT phase.

Delete this bit -- we don't want to muddle the reader up between
the 'user' APIs and the 'implementer' APIs.

> +
> +When resetting multiple object at the same time. The interface executes the

Comma, not full stop.

> +given phase of the objects before going to the next phase. This guarantee 
> that

"guarantees"

> +all INIT phases are done before any HOLD phase and so on.
> +
> +There is three methods in the interface so must be implemented in an object.

"are"; "that must"

> +The methods corresponds to the three phases:

"correspond"

> +```
> +typedef void (*ResettableInitPhase)(Object *obj);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
> +typedef struct ResettableClass {
> +InterfaceClass parent_class;
> +
> +struct ResettablePhases {
> +ResettableInitPhase init;

Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/26] exec: Map device_endian onto MemOp

[Richard Henderson]

On 8/7/19 1:31 AM, tony.ngu...@bt.com wrote:
> Preparation to replace device_endian with MemOp.
> 
> Mapping device_endian onto MemOp limits behaviour changes to this
> relatively smaller patch.
> 
> The next patch will replace all device_endian usages with the
> equivalent MemOp. That patch will be large but have no behaviour
> changes.
> 
> A subsequent patch will then delete unused device_endian.
> 
> Signed-off-by: Tony Nguyen 
> ---
>  hw/char/serial.c  | 18 ++
>  include/exec/cpu-common.h | 10 +++---
>  2 files changed, 13 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/26] exec: Hard code size with MO_{8|16|32|64}

[Richard Henderson]

On 8/7/19 1:30 AM, tony.ngu...@bt.com wrote:
> Temporarily no-op size_memop was introduced to aid the conversion of
> memory_region_dispatch_{read|write} operand "unsigned size" into
> "MemOp op".
> 
> Now size_memop is implemented, again hard coded size but with
> MO_{8|16|32|64}. This is more expressive and avoid size_memop calls.
> 
> Signed-off-by: Tony Nguyen 
> ---
>  memory_ldst.inc.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/26] hw/s390x: Hard code size with MO_{8|16|32|64}

[Richard Henderson]

On 8/7/19 1:30 AM, tony.ngu...@bt.com wrote:
> Temporarily no-op size_memop was introduced to aid the conversion of
> memory_region_dispatch_{read|write} operand "unsigned size" into
> "MemOp op".
> 
> Now size_memop is implemented, again hard coded size but with
> MO_{8|16|32|64}. This is more expressive and avoid size_memop calls.
> 
> Signed-off-by: Tony Nguyen 
> ---
>  hw/s390x/s390-pci-inst.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 13/26] target/mips: Hard code size with MO_{8|16|32|64}

[Richard Henderson]

On 8/7/19 1:30 AM, tony.ngu...@bt.com wrote:
> Temporarily no-op size_memop was introduced to aid the conversion of
> memory_region_dispatch_{read|write} operand "unsigned size" into
> "MemOp op".
> 
> Now size_memop is implemented, again hard coded size but with
> MO_{8|16|32|64}. This is more expressive and avoid size_memop calls.
> 
> Signed-off-by: Tony Nguyen 
> ---
>  target/mips/op_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 11/26] memory: Access MemoryRegion with MemOp

[Richard Henderson]

On 8/7/19 1:29 AM, tony.ngu...@bt.com wrote:
> Convert memory_region_dispatch_{read|write} operand "unsigned size"
> into a "MemOp op".
> 
> Signed-off-by: Tony Nguyen 
> ---
>  include/exec/memop.h  | 18 +-
>  include/exec/memory.h |  9 +
>  memory.c  |  7 +--
>  3 files changed, 23 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson 

> +/* MemOp to size in bytes.  */
> +static inline unsigned memop_size(MemOp op)
> +{
> +return 1 << ((op) & MO_SIZE);
> +}

s/(op)/op/

This is no longer a macro requiring such things.


r~

Re: [Qemu-block] [Qemu-devel] [PATCH v6 10/26] cputlb: Access MemoryRegion with MemOp

[Richard Henderson]

On 8/7/19 1:29 AM, tony.ngu...@bt.com wrote:
> The memory_region_dispatch_{read|write} operand "unsigned size" is
> being converted into a "MemOp op".
> 
> Convert interfaces by using no-op size_memop.
> 
> After all interfaces are converted, size_memop will be implemented
> and the memory_region_dispatch_{read|write} operand "unsigned size"
> will be converted into a "MemOp op".
> 
> As size_memop is a no-op, this patch does not change any behaviour.
> 
> Signed-off-by: Tony Nguyen 
> ---
>  accel/tcg/cputlb.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [PATCH v3 29/33] hw/misc/zynq_slcr: use standard register definition

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> Replace the zynq_slcr registers enum and macros using the
> hw/registerfields.h macros.
>
> Signed-off-by: Damien Hedde 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Alistair Francis 
> ---
>  hw/misc/zynq_slcr.c | 472 ++--
>  1 file changed, 236 insertions(+), 236 deletions(-)
>

Since this patch has been reviewed and it's unrelated to the
reset APIs, I'm going to pull it into target-arm.next, and
then you can stop carrying it around in your patchset
(well, once it's gone into master and you've rebased on that,
which will probably be in a week or two).

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 12/33] hw/pci/: remove qdev/qbus_reset_all call

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> Replace deprecated qdev/bus_reset_all by device/bus_reset_warm.
>
> This does not impact the behavior.
>
> Signed-off-by: Damien Hedde 

I'll come back to patches 12-28 later. They're all ok
in principle, we just need to check that in each individual
case:
 * we've made the right choice of cold vs warm reset
 * we're ok to switch to 'reset including children' from
   the legacy 'reset not including children' semantics

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH v6 03/26] memory: Introduce size_memop

[Richard Henderson]

On 8/7/19 1:26 AM, tony.ngu...@bt.com wrote:
> +/* Size in bytes to MemOp.  */
> +static inline MemOp size_memop(unsigned size)
> +{
> +/*
> + * FIXME: No-op to aid conversion of memory_region_dispatch_{read|write}
> + * "unsigned size" operand into a "MemOp op".
> + */
> +return size;
> +}
> +

Return type should remain unsigned until patch 11.
Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable

[Peter Maydell]

On Wed, 7 Aug 2019 at 16:23, Damien Hedde  wrote:
> On 8/7/19 4:41 PM, Peter Maydell wrote:
> > On Mon, 29 Jul 2019 at 15:58, Damien Hedde  
> > wrote:
> >> legacy resets are called in the "post" order (ie: children then parent)
> >> in hierarchical reset. That is the same order as legacy qdev_reset_all
> >> or qbus_reset_all were using.
> >
> > This is true, but on the other hand the semantics of most device
> > reset methods match "init", not "exit" -- they just set device
> > internal fields to the correct reset state.
>
> I changed from "init" to "exit" due to the change of the init phase call
> order to parent-then-children.
>
> This is a consequence of what I found about the raspi reset: it changes
> the reset hierarchy during reset. The only way I saw to have a chance
> allowing this kind of things cleanly is: do parent init first so that it
> can setup its children before they are considered for reset.

Changing the reset hierarchy during reset is a bit awkward;
I'll have to have a look at the email you sent about the raspi.

I can't decide whether there's an obvious "natural" order to
want the phases to be done in for parent vs children. I guess
it only matters for controller devices and the things on their
bus (eg pci controller vs pci devices, scsi controller vs scsi
devices). We should figure out what the right semantics for our
new multi phase setup are by looking at those, I suppose.

> I can put the legacy reset method to the hold phase which is part of the
> "enter reset state". Otherwise I need to change back the order of init
> phase.
>
> My other concern with putting it in init phase is that some device do
> things we forbid in it (like setting irq).

Yes, but those devices are broken today, because we forbid
setting IRQs in a 'legacy' device reset method as well!
The correct fix for those (eventually) would be to split the 'set irq'
part out into new-style 'init' and 'hold' methods.

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 28/33] qdev: Remove unused deprecated reset functions

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> Remove the functions now they are unused:
> + device_legacy_reset
> + qdev_reset_all[_fn]
> + qbus_reset_all[_fn]
>
> Signed-off-by: Damien Hedde 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state

[Damien Hedde]

On 8/7/19 4:54 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>>
>> It contains the resetting counter and cold flag status.
>>
>> At this point, migration of bus reset related state (counter and cold/warm
>> flag) is handled by parent device. This done using the post_load
>> function in the vmsd subsection.
>>
>> This is last point allow to add an initial support of migration with part of
>> qdev/qbus tree in reset state under the following condition:
>> + time-lasting reset are asserted on Device only
>>
>> Note that if this condition is not respected, migration will succeed and
>> no failure will occurs. The only impact is that the resetting counter
>> of a bus may lower afer a migration.
>>
>> Signed-off-by: Damien Hedde 
> 
> 
>> +const struct VMStateDescription device_vmstate_reset = {
>> +.name = "device_reset",
>> +.version_id = 0,
>> +.minimum_version_id = 0,
>> +.needed = device_vmstate_reset_needed,
>> +.post_load = device_vmstate_reset_post_load,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT32(resetting, DeviceState),
>> +VMSTATE_BOOL(reset_is_cold, DeviceState),
>> +VMSTATE_END_OF_LIST()
>> +},
>> +};
>> -
> 
> Forgot to ask -- why don't we migrate the hold_needed flags?

The flag is only used to keep the info between executing the init
and hold phases. We can't interrupt the code in between since this
mess is during resettable_assert_reset method which is atomic.
I can add a comment to explain that.

> 
> thanks
> -- PMM
> 

Re: [Qemu-block] [PATCH v3 11/33] hw/s390x/ipl.c: remove qbus_reset_all registration

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> Replace deprecated qbus_reset_all by resettable_reset_cold_fn for
> the ipl registration in the main reset handlers.
>
> This does not impact the behavior.
>
> Signed-off-by: Damien Hedde 
> ---
>  hw/s390x/ipl.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 60bd081d3e..402770a2c9 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -234,7 +234,11 @@ static void s390_ipl_realize(DeviceState *dev, Error 
> **errp)
>   */
>  ipl->compat_start_addr = ipl->start_addr;
>  ipl->compat_bios_start_addr = ipl->bios_start_addr;
> -qemu_register_reset(qdev_reset_all_fn, dev);
> +/*
> + * TODO: when we add some kind of main reset container / domain
> + * switch to it to really benefit from multi-phase.
> + */

I think this comment misses the mark a bit. Here's my suggestion:

/*
 * Because this Device is not on any bus in the qbus tree (it is
 * not a sysbus device and it's not on some other bus like a PCI
 * bus) it will not be automatically reset by the 'reset the
 * sysbus' hook registered by vl.c like most devices. So we must
 * manually register a reset hook for it.
 * TODO: there should be a better way to do this.
 */

> +qemu_register_reset(resettable_reset_cold_fn, dev);
>  error:
>  error_propagate(errp, err);
>  }
> --
> 2.22.0
>

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable

[Damien Hedde]

On 8/7/19 4:41 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>>
>> This add Resettable interface implementation for both Bus and Device.
>>
>> *resetting* counter and *reset_is_cold* flag are added in DeviceState
>> and BusState.
>>
>> Compatibility with existing code base is ensured.
>> The legacy bus or device reset method is called in the new exit phase
>> and the other 2 phases are let empty. Using the exit phase guarantee that
> 
> "left". "guarantees"
> 
>> legacy resets are called in the "post" order (ie: children then parent)
>> in hierarchical reset. That is the same order as legacy qdev_reset_all
>> or qbus_reset_all were using.
> 
> This is true, but on the other hand the semantics of most device
> reset methods match "init", not "exit" -- they just set device
> internal fields to the correct reset state.

I changed from "init" to "exit" due to the change of the init phase call
order to parent-then-children.

This is a consequence of what I found about the raspi reset: it changes
the reset hierarchy during reset. The only way I saw to have a chance
allowing this kind of things cleanly is: do parent init first so that it
can setup its children before they are considered for reset.

I can put the legacy reset method to the hold phase which is part of the
"enter reset state". Otherwise I need to change back the order of init
phase.

My other concern with putting it in init phase is that some device do
things we forbid in it (like setting irq).


> 
>> New *device_reset* and *bus_reset* function are proposed with an
>> additional boolean argument telling whether the reset is cold or warm.
>> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
>> are defined also as helpers.
>>
>> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
>> functions telling respectively whether the object is currently under reset 
>> and
>> if the current reset is cold or not.
> 
> I was expecting this patch to contain handling for migration
> of the new data fields. That's in a later patch, so the
> commit message here should say something like:
> 
> "This commit adds new fields to BusState and DeviceState, but
> does not set up migration handling for them; so the new functions
> can only be called after the subsequent commit which adds the
> migration support."
> 
>> Signed-off-by: Damien Hedde 
>> ---
>>  hw/core/bus.c  | 85 ++
>>  hw/core/qdev.c | 82 
>>  include/hw/qdev-core.h | 84 ++---
>>  tests/Makefile.include |  1 +
>>  4 files changed, 247 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/bus.c b/hw/core/bus.c
>> index 17bc1edcde..08a97addb6 100644
>> --- a/hw/core/bus.c
>> +++ b/hw/core/bus.c
>> @@ -22,6 +22,7 @@
>>  #include "qemu/module.h"
>>  #include "hw/qdev.h"
>>  #include "qapi/error.h"
>> +#include "hw/resettable.h"
>>
>>  void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
>>  {
>> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
>>  return 0;
>>  }
>>
>> +void bus_reset(BusState *bus, bool cold)
>> +{
>> +resettable_reset(OBJECT(bus), cold);
>> +}
>> +
>> +bool bus_is_resetting(BusState *bus)
>> +{
>> +return (bus->resetting != 0);
>> +}
>> +
>> +bool bus_is_reset_cold(BusState *bus)
>> +{
>> +return bus->reset_is_cold;
>> +}
>> +
>> +static uint32_t bus_get_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return bus->resetting;
>> +}
>> +
>> +static uint32_t bus_increment_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return ++bus->resetting;
>> +}
>> +
>> +static uint32_t bus_decrement_reset_count(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +return --bus->resetting;
>> +}
>> +
>> +static bool bus_set_reset_cold(Object *obj, bool cold)
>> +{
>> +BusState *bus = BUS(obj);
>> +bool old = bus->reset_is_cold;
>> +bus->reset_is_cold = cold;
>> +return old;
>> +}
>> +
>> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
>> +{
>> +BusState *bus = BUS(obj);
>> +bool old = bus->reset_hold_needed;
>> +bus->reset_hold_needed = hold_needed;
>> +return old;
>> +}
>> +
>> +static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
>> +{
>> +BusState *bus = BUS(obj);
>> +BusChild *kid;
>> +
>> +QTAILQ_FOREACH(kid, >children, sibling) {
>> +func(OBJECT(kid->child));
>> +}
>> +}
>> +
>> +static void bus_obj_legacy_reset(Object *obj)
>> +{
>> +BusState *bus = BUS(obj);
>> +BusClass *bc = BUS_GET_CLASS(obj);
>> +
>> +if (bc->reset) {
>> +bc->reset(bus);
>> +}
>> +}
>> +
>>  static void qbus_realize(BusState *bus, DeviceState *parent, const char 
>> *name)
>>  {
>>  const char *typename = object_get_typename(OBJECT(bus));
>> @@ -192,6 +262,8 @@ static void qbus_initfn(Object *obj)
>>

Re: [Qemu-block] [PATCH v3 10/33] vl.c: remove qbus_reset_all registration

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> Replace deprecated qbus_reset_all by resettable_reset_cold_fn for
> the sysbus reset registration.
> This does not impact the behavior.
>
> Signed-off-by: Damien Hedde 
> ---
>  vl.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index b426b32134..5a465c8236 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4421,7 +4421,11 @@ int main(int argc, char **argv, char **envp)
>
>  /* TODO: once all bus devices are qdevified, this should be done
>   * when bus is created by qdev.c */
> -qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
> +/*
> + * TODO: when we have a main reset container/domain object, use
> + * it to fully benefit from multi-phase reset
> + */

Let's be a bit more specific with the todo comment while the
detail is fresh in our minds:

/*
 * TODO: If we had a main 'reset container' that the whole system
 * lived in, we could reset that using the multi-phase reset
 * APIs. For the moment, we just reset the sysbus, which will cause
 * all devices hanging off it (and all their child buses, recursively)
 * to be reset. Note that this will *not* reset any Device objects
 * which are not attached to some part of the qbus tree!
 */

> +qemu_register_reset(resettable_reset_cold_fn, sysbus_get_default());
>  qemu_run_machine_init_done_notifiers();
>
>  if (rom_check_and_register_reset() != 0) {
> --
> 2.22.0

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> It adds the possibility to add 2 gpios to control the warm and cold reset.
> With theses ios, the reset can be maintained during some time.
> Each io is associated with a state to detect level changes.
>
> Vmstate subsections are also added to the existsing device_reset
> subsection.
>
> Signed-off-by: Damien Hedde 
> ---
>  hw/core/qdev-vmstate.c | 15 ++
>  hw/core/qdev.c | 65 ++
>  include/hw/qdev-core.h | 57 
>  3 files changed, 137 insertions(+)
>
> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
> index 24f8465c61..72f84c6cee 100644
> --- a/hw/core/qdev-vmstate.c
> +++ b/hw/core/qdev-vmstate.c
> @@ -24,10 +24,23 @@ static int device_vmstate_reset_post_load(void *opaque, 
> int version_id)
>  {
>  DeviceState *dev = (DeviceState *) opaque;
>  BusState *bus;
> +uint32_t io_count = 0;
> +
>  QLIST_FOREACH(bus, >child_bus, sibling) {
>  bus->resetting = dev->resetting;
>  bus->reset_is_cold = dev->reset_is_cold;
>  }
> +
> +if (dev->cold_reset_input.state) {
> +io_count += 1;
> +}
> +if (dev->warm_reset_input.state) {
> +io_count += 1;
> +}
> +/* ensure resetting count is coherent with io states */
> +if (dev->resetting < io_count) {
> +return -1;
> +}
>  return 0;
>  }
>
> @@ -40,6 +53,8 @@ const struct VMStateDescription device_vmstate_reset = {
>  .fields = (VMStateField[]) {
>  VMSTATE_UINT32(resetting, DeviceState),
>  VMSTATE_BOOL(reset_is_cold, DeviceState),
> +VMSTATE_BOOL(cold_reset_input.state, DeviceState),
> +VMSTATE_BOOL(warm_reset_input.state, DeviceState),

If we're just adding these fields to this VMStateDescription
then this patch should come earlier in the series than the
patch where we create and start using the fields. Otherwise
there's a migration compat break between a QEMU just
before this patch and a QEMU with it. I think the simplest
fix is to put this patch before patches 6/7 and have a note
in the commit message that this functionality can't be used
until after the patch which adds the migration support.

>  VMSTATE_END_OF_LIST()
>  },
>  };
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 88387d3743..11a4de55ea 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -450,6 +450,67 @@ void qdev_init_gpio_in(DeviceState *dev, 
> qemu_irq_handler handler, int n)
>  qdev_init_gpio_in_named(dev, handler, NULL, n);
>  }
>
> +static DeviceResetInputState *device_get_reset_input_state(DeviceState *dev,
> +bool cold)
> +{
> +return cold ? >cold_reset_input : >warm_reset_input;
> +}
> +
> +static void device_reset_handler(DeviceState *dev, bool cold, bool level)
> +{
> +DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
> +
> +if (dris->type == DEVICE_RESET_ACTIVE_LOW) {
> +level = !level;
> +}
> +
> +if (dris->state == level) {
> +/* io state has not changed */
> +return;
> +}
> +
> +dris->state = level;
> +
> +if (level) {
> +resettable_assert_reset(OBJECT(dev), cold);
> +} else {
> +resettable_deassert_reset(OBJECT(dev));
> +}
> +}
> +
> +static void device_cold_reset_handler(void *opaque, int n, int level)
> +{
> +device_reset_handler((DeviceState *) opaque, true, level);
> +}
> +
> +static void device_warm_reset_handler(void *opaque, int n, int level)
> +{
> +device_reset_handler((DeviceState *) opaque, false, level);
> +}
> +
> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
> +   bool cold, DeviceResetActiveType type)
> +{
> +DeviceResetInputState *dris = device_get_reset_input_state(dev, cold);
> +qemu_irq_handler handler;
> +
> +switch (type) {
> +case DEVICE_RESET_ACTIVE_LOW:
> +case DEVICE_RESET_ACTIVE_HIGH:
> +break;
> +default:
> +assert(false);
> +break;

The usual way to write this is
g_assert_not_reached();
(and no following 'break').


But the whole switch statement seems to be a complicated way
of writing
   assert(type == DEVICE_RESET_ACTIVE_LOW || type == DEVICE_RESET_ACTIVE_HIGH);

> +}
> +
> +assert(!dris->exists);
> +dris->exists = true;
> +dris->type = type;
> +
> +handler = cold ? device_cold_reset_handler : device_warm_reset_handler;
> +qdev_init_gpio_in_named(dev, handler, name, 1);
> +}

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/3] block-backend: Queue requests while drained

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190807144628.4988-1-kw...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v2 0/3] block-backend: Queue requests while drained
Message-id: 20190807144628.4988-1-kw...@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20190807144628.4988-1-kw...@redhat.com -> 
patchew/20190807144628.4988-1-kw...@redhat.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for 
path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) 
registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 
'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 
'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 
'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered 
for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) 
registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for 
path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) 
registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for 
path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) 
registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for 
path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for 
path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for 
path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) 
registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 
'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' 
(https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 
'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' 
(https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 
'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) 
registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out 
'22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out 
'90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 
'ba1ab360eebe6338bb8d7d83a9220ccf7e213af3'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out 
'20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) 
registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' 
(https://github.com/openssl/openssl) registered for path 
'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': 
checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out 
'50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered 
for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) 
registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': 
checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked 
out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 
'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out 
'09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 

Re: [Qemu-block] [PATCH v3 07/33] automatically add vmstate for reset support in devices

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:59, Damien Hedde  wrote:
>
> This add the reset related sections for every QOM
> device.

A bit more detail in the commit message would help, I think --
this is adding extra machinery which has to copy and modify
the VMStateDescription passed in by the device in order to
add the subsection that handles reset.

I've added Dave Gilbert to the already long cc list since this
is migration related.

> Signed-off-by: Damien Hedde 
> ---
>  hw/core/qdev-vmstate.c | 41 +
>  hw/core/qdev.c | 12 +++-
>  include/hw/qdev-core.h |  3 +++
>  stubs/Makefile.objs|  1 +
>  stubs/device.c |  7 +++
>  5 files changed, 63 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/device.c
>
> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
> index 07b010811f..24f8465c61 100644
> --- a/hw/core/qdev-vmstate.c
> +++ b/hw/core/qdev-vmstate.c
> @@ -43,3 +43,44 @@ const struct VMStateDescription device_vmstate_reset = {
>  VMSTATE_END_OF_LIST()
>  },
>  };
> +
> +static VMStateDescription *vmsd_duplicate_and_append(
> +const VMStateDescription *old_vmsd,
> +const VMStateDescription *new_subsection)
> +{
> +VMStateDescription *vmsd;
> +int n = 0;
> +
> +assert(old_vmsd && new_subsection);
> +
> +vmsd = (VMStateDescription *) g_memdup(old_vmsd, sizeof(*vmsd));
> +
> +if (old_vmsd->subsections) {
> +while (old_vmsd->subsections[n]) {
> +n += 1;
> +}
> +}
> +vmsd->subsections = g_new(const VMStateDescription *, n + 2);
> +
> +if (old_vmsd->subsections) {
> +memcpy(vmsd->subsections, old_vmsd->subsections,
> +   sizeof(VMStateDescription *) * n);
> +}
> +vmsd->subsections[n] = new_subsection;
> +vmsd->subsections[n + 1] = NULL;
> +
> +return vmsd;
> +}
> +
> +void device_class_build_extended_vmsd(DeviceClass *dc)
> +{
> +assert(dc->vmsd);
> +assert(!dc->vmsd_ext);
> +
> +/* forge a subsection with proper name */
> +VMStateDescription *reset;
> +reset = g_memdup(_vmstate_reset, sizeof(*reset));
> +reset->name = g_strdup_printf("%s/device_reset", dc->vmsd->name);
> +
> +dc->vmsd_ext = vmsd_duplicate_and_append(dc->vmsd, reset);
> +}

This will allocate memory, but there is no corresponding
code which frees it. This means you'll have a memory leak
across device realize->unrealize for hotplug devices.

> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e9e5f2d5f9..88387d3743 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -45,7 +45,17 @@ bool qdev_hot_removed = false;
>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>  {
>  DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -return dc->vmsd;
> +
> +if (!dc->vmsd) {
> +return NULL;
> +}
> +
> +if (!dc->vmsd_ext) {
> +/* build it first time we need it */
> +device_class_build_extended_vmsd(dc);
> +}
> +
> +return dc->vmsd_ext;
>  }

Unfortunately not everything that wants the VMSD calls
this function. migration/savevm.c:dump_vmstate_json_to_file()
does a direct access to dc->vmsd, so we need to fix that first.

Devices which don't use dc->vmsd won't get this and so
their reset state won't be migrated. That's OK for anything
that's still not yet a QOM device, I guess -- it's not possible
for them to be in a 'held in reset' state anyway, so the
extra subsection would never be needed.

The one I'm less sure about is the 'virtio' devices, which
have to do something odd with migration state for backwards
compat reasons. At the moment they can't be in a situation
where they're being held in reset when we do a migration,
but since they're PCI devices they might in future be possible
to put into new boards/pci controllers that would let them
be in that situation.

>  static void bus_remove_child(BusState *bus, DeviceState *child)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 1670ae41bb..926d4bbcb1 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -120,6 +120,7 @@ typedef struct DeviceClass {
>
>  /* device state */
>  const struct VMStateDescription *vmsd;
> +const struct VMStateDescription *vmsd_ext;
>
>  /* Private to qdev / bus.  */
>  const char *bus_type;
> @@ -520,6 +521,8 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
>
>  const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
>
> +void device_class_build_extended_vmsd(DeviceClass *dc);
> +
>  const char *qdev_fw_name(DeviceState *dev);
>
>  Object *qdev_get_machine(void);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9c7393b08c..432b56f290 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -40,4 +40,5 @@ stub-obj-y += pci-host-piix.o
>  stub-obj-y += ram-block.o
>  stub-obj-y += ramfb.o
>  stub-obj-y += fw_cfg.o
> +stub-obj-y += device.o
>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
> diff 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 19/26] exec: Delete DEVICE_HOST_ENDIAN

[Richard Henderson]

On 8/7/19 3:22 AM, Paolo Bonzini wrote:
> On 07/08/19 10:32, tony.ngu...@bt.com wrote:
>> +#if defined(HOST_WORDS_BIGENDIAN)
>> +    .endianness = MO_BE,
>> +#else
>> +    .endianness = MO_LE,
>> +#endif
> 
> Host endianness is just 0, isn't it?

Yes.  Just leaving a comment to that effect here for the one use is probably
sufficient.


r~

Re: [Qemu-block] [PATCH v3 01/33] Create Resettable QOM interface

[Damien Hedde]

On 8/7/19 4:20 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>>
>> This commit defines an interface allowing multi-phase reset.
>> The phases are INIT, HOLD and EXIT. Each phase has an associated method
>> in the class.
>>
>> The reset of a Resettable is controlled with 2 functions:
>>   - resettable_assert_reset which starts the reset operation.
>>   - resettable_deassert_reset which ends the reset operation.
>>
>> There is also a `resettable_reset` helper function which does assert then
>> deassert allowing to do a proper reset in one call.
>>
>> In addition, two functions, *resettable_reset_cold_fn* and
>> *resettable_reset_warm_fn*, are defined. They take only an opaque argument
>> (for the Resettable object) and can be used as callbacks.
>>
>> The Resettable interface is "reentrant", _assert_ can be called several
>> times and _deasert_ must be called the same number of times so that the
> 
> deassert
> 
>> Resettable leave reset state. It is supported by keeping a counter of
>> the current number of _assert_ calls. The counter maintainance is done
>> though 3 methods get/increment/decrement_count. A method set_cold is used
>> to set the cold flag of the reset. Another method set_host_needed is used
>> to ensure hold phase is executed only if required.
>>
>> Reset hierarchy is also supported. Each Resettable may have
>> sub-Resettable objects. When resetting a Resettable, it is propagated to
>> its children using the foreach_child method.
>>
>> When entering reset, init phases are executed parent-to-child order then
>> hold phases are executed child-parent order.
> 
> Why do we execute the hold phase in child-to-parent order? I would
> have expected this to follow the same order as the init phase.

No particular reason, I just thought it was better that way. But I don't
have a use case where it matters.

If we do only an assert_reset, then top-level phases are
called first and last:
parent's init
  child A's init
  child_B's init
  child_A's hold
  child_B's hold
parent's hold

I can switch it.

> 
>> When leaving reset, exit phases are executed in child-to-parent order.
>> This will allow to replace current qdev_reset mechanism by this interface
>> without side-effects on reset order.
> 
> It makes sense that the exit phase is child-to-parent.
> 
>> Note: I used an uint32 for the count. This match the type already used
>> in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the
>> PVSCSIState.
>>
>> Signed-off-by: Damien Hedde 
>> ---
>>  Makefile.objs   |   1 +
>>  hw/core/Makefile.objs   |   1 +
>>  hw/core/resettable.c| 220 
>>  hw/core/trace-events|  39 +++
>>  include/hw/resettable.h | 126 +++
>>  5 files changed, 387 insertions(+)
>>  create mode 100644 hw/core/resettable.c
>>  create mode 100644 hw/core/trace-events
>>  create mode 100644 include/hw/resettable.h
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 6a143dcd57..a723a47e14 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>  trace-events-subdirs += net
>>  trace-events-subdirs += ui
>>  endif
>> +trace-events-subdirs += hw/core
>>  trace-events-subdirs += hw/display
>>  trace-events-subdirs += qapi
>>  trace-events-subdirs += qom
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index f8481d959f..d9234aa98a 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -1,6 +1,7 @@
>>  # core qdev-related obj files, also used by *-user:
>>  common-obj-y += qdev.o qdev-properties.o
>>  common-obj-y += bus.o reset.o
>> +common-obj-y += resettable.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>>  # irq.o needed for qdev GPIO handling:
>> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
>> new file mode 100644
>> index 00..d7d631ce8b
>> --- /dev/null
>> +++ b/hw/core/resettable.c
>> @@ -0,0 +1,220 @@
>> +/*
>> + * Resettable interface.
>> + *
>> + * Copyright (c) 2019 GreenSocs SAS
>> + *
>> + * Authors:
>> + *   Damien Hedde
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/module.h"
>> +#include "hw/resettable.h"
>> +#include "trace.h"
>> +
>> +#define RESETTABLE_MAX_COUNT 50
>> +
>> +#define RESETTABLE_GET_CLASS(obj) \
>> +OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE)
> 
> Since this is an interface and not a complete class,
> we should name it TYPE_RESETTABLE_INTERFACE. We're rather
> inconsistent about naming of interfaces (in the codebase you
> can find "_INTERFACE suffix", "_IF suffix" and "no suffix").
> I think _INTERFACE suffix is best of these.
> (There's some discussion of this in this mailing list thread:
> 

Re: [Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>
> It contains the resetting counter and cold flag status.
>
> At this point, migration of bus reset related state (counter and cold/warm
> flag) is handled by parent device. This done using the post_load
> function in the vmsd subsection.
>
> This is last point allow to add an initial support of migration with part of
> qdev/qbus tree in reset state under the following condition:
> + time-lasting reset are asserted on Device only
>
> Note that if this condition is not respected, migration will succeed and
> no failure will occurs. The only impact is that the resetting counter
> of a bus may lower afer a migration.
>
> Signed-off-by: Damien Hedde 


> +const struct VMStateDescription device_vmstate_reset = {
> +.name = "device_reset",
> +.version_id = 0,
> +.minimum_version_id = 0,
> +.needed = device_vmstate_reset_needed,
> +.post_load = device_vmstate_reset_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(resetting, DeviceState),
> +VMSTATE_BOOL(reset_is_cold, DeviceState),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> -

Forgot to ask -- why don't we migrate the hold_needed flags?

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>
> It contains the resetting counter and cold flag status.
>
> At this point, migration of bus reset related state (counter and cold/warm
> flag) is handled by parent device. This done using the post_load

"is done"

> function in the vmsd subsection.
>
> This is last point allow to add an initial support of migration with part of
> qdev/qbus tree in reset state under the following condition:
> + time-lasting reset are asserted on Device only
>
> Note that if this condition is not respected, migration will succeed and
> no failure will occurs. The only impact is that the resetting counter
> of a bus may lower afer a migration.

We should just migrate the bus state correctly -- see below.

> Signed-off-by: Damien Hedde 
> ---
>  hw/core/Makefile.objs  |  1 +
>  hw/core/qdev-vmstate.c | 45 ++
>  2 files changed, 46 insertions(+)
>  create mode 100644 hw/core/qdev-vmstate.c
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index d9234aa98a..49e9be0228 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
>  common-obj-y += resettable.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> +common-obj-$(CONFIG_SOFTMMU) += qdev-vmstate.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  common-obj-y += hotplug.o
> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
> new file mode 100644
> index 00..07b010811f
> --- /dev/null
> +++ b/hw/core/qdev-vmstate.c
> @@ -0,0 +1,45 @@
> +/*
> + * Device vmstate
> + *
> + * Copyright (c) 2019 GreenSocs
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +#include "migration/vmstate.h"
> +
> +static bool device_vmstate_reset_needed(void *opaque)
> +{
> +DeviceState *dev = (DeviceState *) opaque;
> +return dev->resetting != 0;
> +}
> +
> +static int device_vmstate_reset_post_load(void *opaque, int version_id)
> +{
> +DeviceState *dev = (DeviceState *) opaque;
> +BusState *bus;
> +QLIST_FOREACH(bus, >child_bus, sibling) {
> +bus->resetting = dev->resetting;
> +bus->reset_is_cold = dev->reset_is_cold;
> +}

Bus reset state might not be the same as the parent device's
reset state, so we need to migrate them both separately.

The way to do this is that in a pre-save hook we iterate
through the child buses and capture their state into
an array. Then we can migrate the array. In the post-load
hook we can set the bus state fields from the array contents.
VMSTATE_WITH_TMP is useful for this kind of situation.

One thing I'm slightly wary of here is that this will mean
that the ordering of child buses within the child_bus
array becomes significant to avoid migration compat breaks.
I think this is OK, though.

> +return 0;
> +}
> +
> +const struct VMStateDescription device_vmstate_reset = {
> +.name = "device_reset",
> +.version_id = 0,
> +.minimum_version_id = 0,
> +.needed = device_vmstate_reset_needed,
> +.post_load = device_vmstate_reset_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(resetting, DeviceState),
> +VMSTATE_BOOL(reset_is_cold, DeviceState),
> +VMSTATE_END_OF_LIST()
> +},
> +};

This isn't used -- I think you should squash patch 7 in
with this one.

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>
> Deprecate old reset apis and make them use the new one while they
> are still used somewhere.
>
> Signed-off-by: Damien Hedde 
> ---
>  hw/core/qdev.c | 22 +++---
>  include/hw/qdev-core.h | 28 ++--
>  2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 559ced070d..e9e5f2d5f9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void 
> (*func)(Object *))
>  }
>  }
>
> -static int qdev_reset_one(DeviceState *dev, void *opaque)
> -{
> -device_legacy_reset(dev);
> -
> -return 0;
> -}
> -
> -static int qbus_reset_one(BusState *bus, void *opaque)
> -{
> -BusClass *bc = BUS_GET_CLASS(bus);
> -if (bc->reset) {
> -bc->reset(bus);
> -}
> -return 0;
> -}
> -
>  void qdev_reset_all(DeviceState *dev)
>  {
> -qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, 
> NULL);
> +device_reset(dev, false);
>  }
>
>  void qdev_reset_all_fn(void *opaque)
> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
>
>  void qbus_reset_all(BusState *bus)
>  {
> -qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, 
> NULL);
> +bus_reset(bus, false);
>  }
>
>  void qbus_reset_all_fn(void *opaque)
> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  }
>  }
>  if (dev->hotplugged) {
> -device_legacy_reset(dev);
> +device_reset(dev, true);
>  }
>  dev->pending_deleted_event = false;

The other changes here don't change the semantics, but this
one does -- previously we were only resetting this specific
device and now we're resetting both the device and its children.
I think it belongs as its own patch in with the other
"remove device_legacy_reset call" patches.

>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index eeb75611c8..1670ae41bb 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -109,6 +109,11 @@ typedef struct DeviceClass {
>  bool hotpluggable;
>
>  /* callbacks */
> +/*
> + * Reset method here is deprecated and replaced by methods in the
> + * resettable class interface to implement a multi-phase reset.
> + * TODO: remove once every reset callback is unused
> + */
>  DeviceReset reset;
>  DeviceRealize realize;
>  DeviceUnrealize unrealize;
> @@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus);
>   */
>  bool bus_is_reset_cold(BusState *bus);
>
> -void qdev_reset_all(DeviceState *dev);
> -void qdev_reset_all_fn(void *opaque);
> -
>  /**
> - * @qbus_reset_all:
> - * @bus: Bus to be reset.
> + * qbus/qdev_reset_all:
> + * @bus/dev: Bus/Device to be reset.
>   *
> - * Reset @bus and perform a bus-level ("hard") reset of all devices connected
> + * Reset @bus/dev and perform a bus-level reset of all devices/buses 
> connected
>   * to it, including recursive processing of all buses below @bus itself.  A
>   * hard reset means that qbus_reset_all will reset all state of the device.
>   * For PCI devices, for example, this will include the base address registers
>   * or configuration space.
> + *
> + * Theses functions are deprecated, please use device/bus_reset or

"these"

> + * resettable_reset_* instead
> + * TODO: remove them when all occurence are removed

"occurrences" (two 'r's, plural with 's'), here and below

>   */

The comment here says that qbus_reset_all() does a "hard" reset,
which presumably is equivalent to a 'cold' reset, but the
code in our new implementation passes cold = false.

> +void qdev_reset_all(DeviceState *dev);
> +void qdev_reset_all_fn(void *opaque);
>  void qbus_reset_all(BusState *bus);
>  void qbus_reset_all_fn(void *opaque);
>
> @@ -489,9 +497,17 @@ void qdev_machine_init(void);
>   * device_legacy_reset:
>   *
>   * Reset a single device (by calling the reset method).
> + *
> + * This function is deprecated, please use device_reset() instead.
> + * TODO: remove the function when all occurences are removed.
>   */
>  void device_legacy_reset(DeviceState *dev);
>
> +/**
> + * device_class_set_parent_reset:
> + * TODO: remove the function when DeviceClass's reset method
> + * is not used anymore.
> + */
>  void device_class_set_parent_reset(DeviceClass *dc,
> DeviceReset dev_reset,
> DeviceReset *parent_reset);
> --
> 2.22.0

thanks
-- PMM

[Qemu-block] [PATCH v2 3/3] block-backend: Queue requests while drained

[Kevin Wolf]

This fixes devices like IDE that can still start new requests from I/O
handlers in the CPU thread while the block backend is drained.

The basic assumption is that in a drain section, no new requests should
be allowed through a BlockBackend (blk_drained_begin/end don't exist,
we get drain sections only on the node level). However, there are two
special cases where requests should not be queued:

1. Block jobs: We already make sure that block jobs are paused in a
   drain section, so they won't start new requests. However, if the
   drain_begin is called on the job's BlockBackend first, it can happen
   that we deadlock because the job stays busy until it reaches a pause
   point - which it can't if its requests aren't processed any more.

   The proper solution here would be to make all requests through the
   job's filter node instead of using a BlockBackend. For now, just
   disabling request queuing on the job BlockBackend is simpler.

2. In test cases where making requests through bdrv_* would be
   cumbersome because we'd need a BdrvChild. As we already got the
   functionality to disable request queuing from 1., use it in tests,
   too, for convenience.

Signed-off-by: Kevin Wolf 
---
 include/sysemu/block-backend.h |  1 +
 block/backup.c |  1 +
 block/block-backend.c  | 53 --
 block/commit.c |  2 ++
 block/mirror.c |  1 +
 blockjob.c |  3 ++
 tests/test-bdrv-drain.c|  1 +
 7 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 7320b58467..368d53af77 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -104,6 +104,7 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, 
uint64_t *shared_perm);
 
 void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow);
 void blk_set_allow_aio_context_change(BlockBackend *blk, bool allow);
+void blk_set_disable_request_queuing(BlockBackend *blk, bool disable);
 void blk_iostatus_enable(BlockBackend *blk);
 bool blk_iostatus_is_enabled(const BlockBackend *blk);
 BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
diff --git a/block/backup.c b/block/backup.c
index b26c22c4b8..4743c8f0bc 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -644,6 +644,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 if (ret < 0) {
 goto error;
 }
+blk_set_disable_request_queuing(job->target, true);
 
 job->on_source_error = on_source_error;
 job->on_target_error = on_target_error;
diff --git a/block/block-backend.c b/block/block-backend.c
index fdd6b01ecf..c13c5c83b0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -79,6 +79,9 @@ struct BlockBackend {
 QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
 
 int quiesce_counter;
+CoQueue queued_requests;
+bool disable_request_queuing;
+
 VMChangeStateEntry *vmsh;
 bool force_allow_inactivate;
 
@@ -339,6 +342,7 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, 
uint64_t shared_perm)
 
 block_acct_init(>stats);
 
+qemu_co_queue_init(>queued_requests);
 notifier_list_init(>remove_bs_notifiers);
 notifier_list_init(>insert_bs_notifiers);
 QLIST_INIT(>aio_notifiers);
@@ -1096,6 +1100,11 @@ void blk_set_allow_aio_context_change(BlockBackend *blk, 
bool allow)
 blk->allow_aio_context_change = allow;
 }
 
+void blk_set_disable_request_queuing(BlockBackend *blk, bool disable)
+{
+blk->disable_request_queuing = disable;
+}
+
 static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
   size_t size)
 {
@@ -1127,13 +1136,24 @@ static int blk_check_byte_request(BlockBackend *blk, 
int64_t offset,
 return 0;
 }
 
+static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
+{
+if (blk->quiesce_counter && !blk->disable_request_queuing) {
+qemu_co_queue_wait(>queued_requests, NULL);
+}
+}
+
 int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
unsigned int bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags)
 {
 int ret;
-BlockDriverState *bs = blk_bs(blk);
+BlockDriverState *bs;
 
+blk_wait_while_drained(blk);
+
+/* Call blk_bs() only after waiting, the graph may have changed */
+bs = blk_bs(blk);
 trace_blk_co_preadv(blk, bs, offset, bytes, flags);
 
 ret = blk_check_byte_request(blk, offset, bytes);
@@ -1159,8 +1179,12 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, 
int64_t offset,
 BdrvRequestFlags flags)
 {
 int ret;
-BlockDriverState *bs = blk_bs(blk);
+BlockDriverState *bs;
 
+blk_wait_while_drained(blk);
+
+/* Call blk_bs() only after waiting, the graph may have changed */
+bs = blk_bs(blk);
 trace_blk_co_pwritev(blk, bs, offset, 

[Qemu-block] [PATCH v2 2/3] mirror: Keep mirror_top_bs drained after dropping permissions

[Kevin Wolf]

mirror_top_bs is currently implicitly drained through its connection to
the source or the target node. However, the drain section for target_bs
ends early after moving mirror_top_bs from src to target_bs, so that
requests can already be restarted while mirror_top_bs is still present
in the chain, but has dropped all permissions and therefore runs into an
assertion failure like this:

qemu-system-x86_64: block/io.c:1634: bdrv_co_write_req_prepare:
Assertion `child->perm & BLK_PERM_WRITE' failed.

Keep mirror_top_bs drained until all graph changes have completed.

Signed-off-by: Kevin Wolf 
---
 block/mirror.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9f5c59ece1..642d6570cc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -656,7 +656,10 @@ static int mirror_exit_common(Job *job)
 s->target = NULL;
 
 /* We don't access the source any more. Dropping any WRITE/RESIZE is
- * required before it could become a backing file of target_bs. */
+ * required before it could become a backing file of target_bs. Not having
+ * these permissions any more means that we can't allow any new requests on
+ * mirror_top_bs from now on, so keep it drained. */
+bdrv_drained_begin(mirror_top_bs);
 bs_opaque->stop = true;
 bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
  _abort);
@@ -724,6 +727,7 @@ static int mirror_exit_common(Job *job)
 bs_opaque->job = NULL;
 
 bdrv_drained_end(src);
+bdrv_drained_end(mirror_top_bs);
 s->in_drain = false;
 bdrv_unref(mirror_top_bs);
 bdrv_unref(src);
-- 
2.20.1

[Qemu-block] [PATCH v2 1/3] block: Remove blk_pread_unthrottled()

[Kevin Wolf]

The functionality offered by blk_pread_unthrottled() goes back to commit
498e386c584. Then, we couldn't perform I/O throttling with synchronous
requests because timers wouldn't be executed in polling loops. So the
commit automatically disabled I/O throttling as soon as a synchronous
request was issued.

However, for geometry detection during disk initialisation, we always
used (and still use) synchronous requests even if guest requests use AIO
later. Geometry detection was not wanted to disable I/O throttling, so
bdrv_pread_unthrottled() was introduced which disabled throttling only
temporarily.

All of this isn't necessary any more because we do run timers in polling
loop and even synchronous requests are now using coroutine
infrastructure internally. For this reason, commit 90c78624f already
removed the automatic disabling of I/O throttling.

It's time to get rid of the workaround for the removed code, and its
abuse of blk_root_drained_begin()/end(), as well.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 include/sysemu/block-backend.h |  2 --
 block/block-backend.c  | 16 
 hw/block/hd-geometry.c |  7 +--
 3 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 733c4957eb..7320b58467 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -117,8 +117,6 @@ char *blk_get_attached_dev_id(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
 BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
-int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
-  int bytes);
 int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
unsigned int bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
diff --git a/block/block-backend.c b/block/block-backend.c
index 0056b526b8..fdd6b01ecf 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1237,22 +1237,6 @@ static int blk_prw(BlockBackend *blk, int64_t offset, 
uint8_t *buf,
 return rwco.ret;
 }
 
-int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
-  int count)
-{
-int ret;
-
-ret = blk_check_byte_request(blk, offset, count);
-if (ret < 0) {
-return ret;
-}
-
-blk_root_drained_begin(blk->root);
-ret = blk_pread(blk, offset, buf, count);
-blk_root_drained_end(blk->root, NULL);
-return ret;
-}
-
 int blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
   int bytes, BdrvRequestFlags flags)
 {
diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index 79384a2b0a..dcbccee294 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -63,12 +63,7 @@ static int guess_disk_lchs(BlockBackend *blk,
 
 blk_get_geometry(blk, _sectors);
 
-/**
- * The function will be invoked during startup not only in sync I/O mode,
- * but also in async I/O mode. So the I/O throttling function has to
- * be disabled temporarily here, not permanently.
- */
-if (blk_pread_unthrottled(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
+if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
 return -1;
 }
 /* test msdos magic */
-- 
2.20.1

[Qemu-block] [PATCH v2 0/3] block-backend: Queue requests while drained

[Kevin Wolf]

This series fixes the problem that devices like IDE, which submit
requests as a direct result of I/O from the CPU thread, can continue to
submit new requests even in a drained section.

v2:
- Rebased on top of block-next
- Replaced patch 2 with draining mirror_top_bs instead [Max]
- Removed wait_while_drained parameter from patch 3 [Max]
- Covered blk_co_flush() [Max]
- Fixed some typos [Eric]

Kevin Wolf (3):
  block: Remove blk_pread_unthrottled()
  mirror: Keep mirror_top_bs drained after dropping permissions
  block-backend: Queue requests while drained

 include/sysemu/block-backend.h |  3 +-
 block/backup.c |  1 +
 block/block-backend.c  | 69 --
 block/commit.c |  2 +
 block/mirror.c |  7 +++-
 blockjob.c |  3 ++
 hw/block/hd-geometry.c |  7 +---
 tests/test-bdrv-drain.c|  1 +
 8 files changed, 65 insertions(+), 28 deletions(-)

-- 
2.20.1

Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>
> This add Resettable interface implementation for both Bus and Device.
>
> *resetting* counter and *reset_is_cold* flag are added in DeviceState
> and BusState.
>
> Compatibility with existing code base is ensured.
> The legacy bus or device reset method is called in the new exit phase
> and the other 2 phases are let empty. Using the exit phase guarantee that

"left". "guarantees"

> legacy resets are called in the "post" order (ie: children then parent)
> in hierarchical reset. That is the same order as legacy qdev_reset_all
> or qbus_reset_all were using.

This is true, but on the other hand the semantics of most device
reset methods match "init", not "exit" -- they just set device
internal fields to the correct reset state.

> New *device_reset* and *bus_reset* function are proposed with an
> additional boolean argument telling whether the reset is cold or warm.
> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
> are defined also as helpers.
>
> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
> functions telling respectively whether the object is currently under reset and
> if the current reset is cold or not.

I was expecting this patch to contain handling for migration
of the new data fields. That's in a later patch, so the
commit message here should say something like:

"This commit adds new fields to BusState and DeviceState, but
does not set up migration handling for them; so the new functions
can only be called after the subsequent commit which adds the
migration support."

> Signed-off-by: Damien Hedde 
> ---
>  hw/core/bus.c  | 85 ++
>  hw/core/qdev.c | 82 
>  include/hw/qdev-core.h | 84 ++---
>  tests/Makefile.include |  1 +
>  4 files changed, 247 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 17bc1edcde..08a97addb6 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -22,6 +22,7 @@
>  #include "qemu/module.h"
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
> +#include "hw/resettable.h"
>
>  void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
>  {
> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
>  return 0;
>  }
>
> +void bus_reset(BusState *bus, bool cold)
> +{
> +resettable_reset(OBJECT(bus), cold);
> +}
> +
> +bool bus_is_resetting(BusState *bus)
> +{
> +return (bus->resetting != 0);
> +}
> +
> +bool bus_is_reset_cold(BusState *bus)
> +{
> +return bus->reset_is_cold;
> +}
> +
> +static uint32_t bus_get_reset_count(Object *obj)
> +{
> +BusState *bus = BUS(obj);
> +return bus->resetting;
> +}
> +
> +static uint32_t bus_increment_reset_count(Object *obj)
> +{
> +BusState *bus = BUS(obj);
> +return ++bus->resetting;
> +}
> +
> +static uint32_t bus_decrement_reset_count(Object *obj)
> +{
> +BusState *bus = BUS(obj);
> +return --bus->resetting;
> +}
> +
> +static bool bus_set_reset_cold(Object *obj, bool cold)
> +{
> +BusState *bus = BUS(obj);
> +bool old = bus->reset_is_cold;
> +bus->reset_is_cold = cold;
> +return old;
> +}
> +
> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
> +{
> +BusState *bus = BUS(obj);
> +bool old = bus->reset_hold_needed;
> +bus->reset_hold_needed = hold_needed;
> +return old;
> +}
> +
> +static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
> +{
> +BusState *bus = BUS(obj);
> +BusChild *kid;
> +
> +QTAILQ_FOREACH(kid, >children, sibling) {
> +func(OBJECT(kid->child));
> +}
> +}
> +
> +static void bus_obj_legacy_reset(Object *obj)
> +{
> +BusState *bus = BUS(obj);
> +BusClass *bc = BUS_GET_CLASS(obj);
> +
> +if (bc->reset) {
> +bc->reset(bus);
> +}
> +}
> +
>  static void qbus_realize(BusState *bus, DeviceState *parent, const char 
> *name)
>  {
>  const char *typename = object_get_typename(OBJECT(bus));
> @@ -192,6 +262,8 @@ static void qbus_initfn(Object *obj)
>   NULL);
>  object_property_add_bool(obj, "realized",
>   bus_get_realized, bus_set_realized, NULL);
> +
> +bus->resetting = 0;
>  }
>
>  static char *default_bus_get_fw_dev_path(DeviceState *dev)
> @@ -202,9 +274,18 @@ static char *default_bus_get_fw_dev_path(DeviceState 
> *dev)
>  static void bus_class_init(ObjectClass *class, void *data)
>  {
>  BusClass *bc = BUS_CLASS(class);
> +ResettableClass *rc = RESETTABLE_CLASS(class);
>
>  class->unparent = bus_unparent;
>  bc->get_fw_dev_path = default_bus_get_fw_dev_path;
> +
> +rc->phases.exit = bus_obj_legacy_reset;
> +rc->get_count = bus_get_reset_count;
> +rc->increment_count = bus_increment_reset_count;
> +rc->decrement_count = bus_decrement_reset_count;
> +rc->foreach_child = 

Re: [Qemu-block] [PATCH v3 02/33] add temporary device_legacy_reset function to replace device_reset

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>
> Provide a temporary function doing what device_reset does to do the
> transition with Resettable API which will trigger a prototype change
> of device_reset.

The other point here is that device_legacy_reset() resets
only that device, not any of its qbus children, right?
So the new function which we eventually replace the callsites
with also has different semantics, which is why we do the
changes one by one in patches 10-28.

So you could add:

The new resettable API function also has different semantics
(resetting child buses as well as the specified device).
Subsequent commits will make the changeover for each callsite
individually; once that is complete device_legacy_reset() will be
removed.

> Signed-off-by: Damien Hedde 

I agree with David that patch 3 could be squashed into this one.

If you do that and tweak the commit message you can have
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-block] [PATCH v3 01/33] Create Resettable QOM interface

[Peter Maydell]

On Mon, 29 Jul 2019 at 15:58, Damien Hedde  wrote:
>
> This commit defines an interface allowing multi-phase reset.
> The phases are INIT, HOLD and EXIT. Each phase has an associated method
> in the class.
>
> The reset of a Resettable is controlled with 2 functions:
>   - resettable_assert_reset which starts the reset operation.
>   - resettable_deassert_reset which ends the reset operation.
>
> There is also a `resettable_reset` helper function which does assert then
> deassert allowing to do a proper reset in one call.
>
> In addition, two functions, *resettable_reset_cold_fn* and
> *resettable_reset_warm_fn*, are defined. They take only an opaque argument
> (for the Resettable object) and can be used as callbacks.
>
> The Resettable interface is "reentrant", _assert_ can be called several
> times and _deasert_ must be called the same number of times so that the

deassert

> Resettable leave reset state. It is supported by keeping a counter of
> the current number of _assert_ calls. The counter maintainance is done
> though 3 methods get/increment/decrement_count. A method set_cold is used
> to set the cold flag of the reset. Another method set_host_needed is used
> to ensure hold phase is executed only if required.
>
> Reset hierarchy is also supported. Each Resettable may have
> sub-Resettable objects. When resetting a Resettable, it is propagated to
> its children using the foreach_child method.
>
> When entering reset, init phases are executed parent-to-child order then
> hold phases are executed child-parent order.

Why do we execute the hold phase in child-to-parent order? I would
have expected this to follow the same order as the init phase.

> When leaving reset, exit phases are executed in child-to-parent order.
> This will allow to replace current qdev_reset mechanism by this interface
> without side-effects on reset order.

It makes sense that the exit phase is child-to-parent.

> Note: I used an uint32 for the count. This match the type already used
> in the existing resetting counter in hw/scsi/vmw_pvscsi.c for the
> PVSCSIState.
>
> Signed-off-by: Damien Hedde 
> ---
>  Makefile.objs   |   1 +
>  hw/core/Makefile.objs   |   1 +
>  hw/core/resettable.c| 220 
>  hw/core/trace-events|  39 +++
>  include/hw/resettable.h | 126 +++
>  5 files changed, 387 insertions(+)
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 hw/core/trace-events
>  create mode 100644 include/hw/resettable.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 6a143dcd57..a723a47e14 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>  trace-events-subdirs += net
>  trace-events-subdirs += ui
>  endif
> +trace-events-subdirs += hw/core
>  trace-events-subdirs += hw/display
>  trace-events-subdirs += qapi
>  trace-events-subdirs += qom
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index f8481d959f..d9234aa98a 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,6 +1,7 @@
>  # core qdev-related obj files, also used by *-user:
>  common-obj-y += qdev.o qdev-properties.o
>  common-obj-y += bus.o reset.o
> +common-obj-y += resettable.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>  # irq.o needed for qdev GPIO handling:
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> new file mode 100644
> index 00..d7d631ce8b
> --- /dev/null
> +++ b/hw/core/resettable.c
> @@ -0,0 +1,220 @@
> +/*
> + * Resettable interface.
> + *
> + * Copyright (c) 2019 GreenSocs SAS
> + *
> + * Authors:
> + *   Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "hw/resettable.h"
> +#include "trace.h"
> +
> +#define RESETTABLE_MAX_COUNT 50
> +
> +#define RESETTABLE_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE)

Since this is an interface and not a complete class,
we should name it TYPE_RESETTABLE_INTERFACE. We're rather
inconsistent about naming of interfaces (in the codebase you
can find "_INTERFACE suffix", "_IF suffix" and "no suffix").
I think _INTERFACE suffix is best of these.
(There's some discussion of this in this mailing list thread:
https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03840.html
 -- the idea is that it avoids confusion between whether an
implementation of the type needs to be a subclass of it or
if it has to be added as an interface to some other type.)

> +
> +static void resettable_init_reset(Object *obj, bool cold);
> +
> +static bool resettable_class_check(ResettableClass *rc)
> +{
> +if (!rc->set_cold) {
> +return false;
> +}
> +if (!rc->set_hold_needed) {
> +return false;
> +}
> +if (!rc->increment_count) {

[Qemu-block] [PATCH v4 02/10] block: reverse order for reopen commits

[Vladimir Sementsov-Ogievskiy]

It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
can't work, as qcow2 needs write access to file child, to mark bitmaps
in-image with IN_USE flag. But usually children goes after parents in
reopen queue and file child is still RO on qcow2 reopen commit. Reverse
reopen order to fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 696162cd7a..d59f9f97cb 100644
--- a/block.c
+++ b/block.c
@@ -3476,10 +3476,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 bs_entry->perms_checked = true;
 }
 
-/* If we reach this point, we have success and just need to apply the
- * changes
+/*
+ * If we reach this point, we have success and just need to apply the
+ * changes.
+ *
+ * Reverse order is used to comfort qcow2 driver: on commit it need to 
write
+ * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
+ * children are usually goes after parents in reopen-queue, so go from last
+ * to first element.
  */
-QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
 bdrv_reopen_commit(_entry->state);
 }
 
-- 
2.18.0

[Qemu-block] [PATCH v4 08/10] iotests: add test 260 to check bitmap life after snapshot + commit

[Vladimir Sementsov-Ogievskiy]

Two testcases with persistent bitmaps are not added here, as there are
bugs to be fixed soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/260 | 87 ++
 tests/qemu-iotests/260.out | 52 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 140 insertions(+)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
new file mode 100755
index 00..d8fcf4567a
--- /dev/null
+++ b/tests/qemu-iotests/260
@@ -0,0 +1,87 @@
+#!/usr/bin/env python
+#
+# Tests for temporary external snapshot when we have bitmaps.
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import qemu_img_create, file_path, log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+base, top = file_path('base', 'top')
+size = 64 * 1024 * 3
+
+
+def print_bitmap(msg, vm):
+result = vm.qmp('query-block')['return'][0]
+if 'dirty-bitmaps' in result:
+bitmap = result['dirty-bitmaps'][0]
+log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
+bitmap['count'] // 64 // 1024))
+else:
+log(msg + ': not found')
+
+
+def test(persistent, restart):
+assert persistent or not restart
+log("\nTestcase {}persistent {} restart\n".format(
+'' if persistent else 'non-', 'with' if restart else 'without'))
+
+qemu_img_create('-f', iotests.imgfmt, base, str(size))
+
+vm = iotests.VM().add_drive(base)
+vm.launch()
+
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
+   persistent=persistent)
+vm.hmp_qemu_io('drive0', 'write 0 64K')
+print_bitmap('initial bitmap', vm)
+
+vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
+   format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
+vm.hmp_qemu_io('drive0', 'write 64K 512')
+print_bitmap('check that no bitmaps are in snapshot', vm)
+
+if restart:
+log("... Restart ...")
+vm.shutdown()
+vm = iotests.VM().add_drive(top)
+vm.launch()
+
+vm.qmp_log('block-commit', device='drive0', top=top,
+   filters=[iotests.filter_qmp_testfiles])
+ev = vm.events_wait_log((('BLOCK_JOB_READY', None),
+ ('BLOCK_JOB_COMPLETED', None)))
+if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
+vm.shutdown()
+log(vm.get_log())
+exit()
+
+vm.qmp_log('block-job-complete', device='drive0')
+vm.event_wait_log('BLOCK_JOB_COMPLETED')
+print_bitmap('check bitmap after commit', vm)
+
+vm.hmp_qemu_io('drive0', 'write 128K 64K')
+print_bitmap('check updated bitmap', vm)
+
+vm.shutdown()
+
+
+test(persistent=False, restart=False)
+test(persistent=True, restart=False)
+test(persistent=True, restart=True)
diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
new file mode 100644
index 00..2f0d98d036
--- /dev/null
+++ b/tests/qemu-iotests/260.out
@@ -0,0 +1,52 @@
+
+Testcase non-persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": 
"drive0", "persistent": false}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", 
"format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": 
"TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
"USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
+
+Testcase persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", 

[Qemu-block] [PATCH v4 10/10] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[Vladimir Sementsov-Ogievskiy]

The only reason I can imagine for this strange code at the very-end of
bdrv_reopen_commit is the fact that bs->read_only updated after
calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
check for being writable, when actually it only need writable file
child not self.

So, as it's fixed, let's move things to correct place.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block_int.h |  6 --
 block.c   | 19 ---
 block/qcow2.c | 15 ++-
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3aa1e832a8..18a1e81194 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -531,12 +531,6 @@ struct BlockDriver {
  uint64_t parent_perm, uint64_t parent_shared,
  uint64_t *nperm, uint64_t *nshared);
 
-/**
- * Bitmaps should be marked as 'IN_USE' in the image on reopening image
- * as rw. This handler should realize it. It also should unset readonly
- * field of BlockDirtyBitmap's in case of success.
- */
-int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
 bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
 const char *name,
 uint32_t granularity,
diff --git a/block.c b/block.c
index d59f9f97cb..395bc88045 100644
--- a/block.c
+++ b/block.c
@@ -3925,16 +3925,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 BlockDriver *drv;
 BlockDriverState *bs;
 BdrvChild *child;
-bool old_can_write, new_can_write;
 
 assert(reopen_state != NULL);
 bs = reopen_state->bs;
 drv = bs->drv;
 assert(drv != NULL);
 
-old_can_write =
-!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-
 /* If there are any driver level actions to take */
 if (drv->bdrv_reopen_commit) {
 drv->bdrv_reopen_commit(reopen_state);
@@ -3978,21 +3974,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 }
 
 bdrv_refresh_limits(bs, NULL);
-
-new_can_write =
-!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
-Error *local_err = NULL;
-if (drv->bdrv_reopen_bitmaps_rw(bs, _err) < 0) {
-/* This is not fatal, bitmaps just left read-only, so all following
- * writes will fail. User can remove read-only bitmaps to unblock
- * writes.
- */
-error_reportf_err(local_err,
-  "%s: Failed to make dirty bitmaps writable: ",
-  bdrv_get_node_name(bs));
-}
-}
 }
 
 /*
diff --git a/block/qcow2.c b/block/qcow2.c
index 5c1187e2f9..9e6210c282 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1828,6 +1828,20 @@ fail:
 static void qcow2_reopen_commit(BDRVReopenState *state)
 {
 qcow2_update_options_commit(state->bs, state->opaque);
+if (state->flags & BDRV_O_RDWR) {
+Error *local_err = NULL;
+
+if (qcow2_reopen_bitmaps_rw(state->bs, _err) < 0) {
+/*
+ * This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes or retry reopen.
+ */
+error_reportf_err(local_err,
+  "%s: Failed to make dirty bitmaps writable: ",
+  bdrv_get_node_name(state->bs));
+}
+}
 g_free(state->opaque);
 }
 
@@ -5229,7 +5243,6 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
-.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
 .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
 .bdrv_remove_persistent_dirty_bitmap = 
qcow2_remove_persistent_dirty_bitmap,
 };
-- 
2.18.0

[Qemu-block] [PATCH v4 07/10] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[Vladimir Sementsov-Ogievskiy]

qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
readonly. But the latter don't work, as
qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
It's OK for inactivation but bad idea for reopen-ro. And this leads to
the following bug:

Assume we have persistent bitmap 'bitmap0'.
Create external snapshot
  bitmap0 is stored and therefore removed
Commit snapshot
  now we have no bitmaps
Do some writes from guest (*)
  they are not marked in bitmap
Shutdown
Start
  bitmap0 is loaded as valid, but it is actually broken! It misses
  writes (*)
Incremental backup
  it will be inconsistent

So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
reopening bitmaps to rw is broken too, so the whole scenario will not
work after this patch and we can't enable corresponding test cases in
260 iotests still. Reopening bitmaps rw will be fixed in the following
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 block/qcow2.h|  3 ++-
 block/qcow2-bitmap.c | 49 ++--
 block/qcow2.c|  2 +-
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index a5b24f4eec..a67e6a7d7c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -738,7 +738,8 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+  bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fbeee37243..a636dc50ca 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1432,7 +1432,32 @@ fail:
 bitmap_list_free(bm_list);
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/*
+ * qcow2_store_persistent_dirty_bitmaps
+ *
+ * Stores persistent BdrvDirtyBitmap objects.
+ *
+ * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
+ * image. This is used in two cases, both via qcow2_inactivate:
+ * 1. bdrv_close: It's correct to remove bitmaps on close.
+ * 2. migration: If bitmaps are migrated through migration channel via
+ *'dirty-bitmaps' migration capability they are not handled by this code.
+ *Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
+ *invalidation.
+ *
+ * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
+ * inactivation means that we lose control on disk, and therefore on bitmaps,
+ * we should sync them and do not touch more.
+ *
+ * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
+ * when we need to store them, as image is still under our control, and it's
+ * good to keep all the bitmaps in read-only mode. Moreover, keeping them
+ * read-only is correct because this is what would happen if we opened the node
+ * readonly to begin with, and whether we opened directly or reopened to that
+ * state shouldn't matter for the state we get afterward.
+ */
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+  bool release_stored, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
 BDRVQcow2State *s = bs->opaque;
@@ -1545,20 +1570,14 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 g_free(tb);
 }
 
-QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-/* For safety, we remove bitmap after storing.
- * We may be here in two cases:
- * 1. bdrv_close. It's ok to drop bitmap.
- * 2. inactivation. It means migration without 'dirty-bitmaps'
- *capability, so bitmaps are not marked with
- *BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
- *and reload on invalidation.
- */
-if (bm->dirty_bitmap == NULL) {
-continue;
-}
+if (release_stored) {
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+if (bm->dirty_bitmap == NULL) {
+continue;
+}
 
-bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+}
 }
 
 success:
@@ -1586,7 +1605,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 BdrvDirtyBitmap *bitmap;
 Error *local_err = NULL;
 
-qcow2_store_persistent_dirty_bitmaps(bs, _err);
+qcow2_store_persistent_dirty_bitmaps(bs, false, _err);
 if (local_err != NULL) {
 

[Qemu-block] [PATCH v4 05/10] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

[Vladimir Sementsov-Ogievskiy]

Firstly, no reason to optimize failure path. Then, function name is
ambiguous: it checks for readonly and similar things, but someone may
think that it will ignore normal bitmaps which was just unchanged, and
this is in bad relation with the fact that we should drop IN_USE flag
for unchanged bitmaps in the image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c | 12 
 block/qcow2-bitmap.c | 23 +--
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 62682eb865..acca86d0fc 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -104,7 +104,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
 BdrvDirtyBitmap *bitmap);
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 95a9c2a5d8..2e0cecf5ff 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -774,18 +774,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap 
*bitmap)
 return bitmap->inconsistent;
 }
 
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
-{
-BdrvDirtyBitmap *bm;
-QLIST_FOREACH(bm, >dirty_bitmaps, list) {
-if (bm->persistent && !bm->readonly && !bm->migration) {
-return true;
-}
-}
-
-return false;
-}
-
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
 BdrvDirtyBitmap *bitmap)
 {
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..60b79f1dac 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1456,16 +1456,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 Qcow2Bitmap *bm;
 QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
 Qcow2BitmapTable *tb, *tb_next;
-
-if (!bdrv_has_changed_persistent_bitmaps(bs)) {
-/* nothing to do */
-return;
-}
-
-if (!can_write(bs)) {
-error_setg(errp, "No write access");
-return;
-}
+bool need_write = false;
 
 QSIMPLEQ_INIT(_tables);
 
@@ -1493,6 +1484,8 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 continue;
 }
 
+need_write = true;
+
 if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
 error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: 
",
   name);
@@ -1531,6 +1524,15 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bm->dirty_bitmap = bitmap;
 }
 
+if (!need_write) {
+goto success;
+}
+
+if (!can_write(bs)) {
+error_setg(errp, "No write access");
+goto fail;
+}
+
 /* allocate clusters and store bitmaps */
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 if (bm->dirty_bitmap == NULL) {
@@ -1572,6 +1574,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
 }
 
+success:
 bitmap_list_free(bm_list);
 return;
 
-- 
2.18.0

[Qemu-block] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

- Correct check for write access to file child, and in correct place
  (only if we want to write).
- Support reopen rw -> rw (which will be used in following commit),
  for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
  bitmap is marked IN_USE in the image.
- Consider unexpected bitmap as a corruption and check other
  combinations of in-image and in-RAM bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 56 +---
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a636dc50ca..e276a95154 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 Qcow2BitmapList *bm_list;
 Qcow2Bitmap *bm;
 GSList *ro_dirty_bitmaps = NULL;
-int ret = 0;
+int ret = -EINVAL;
+bool need_header_update = false;
 
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
 }
 
-if (!can_write(bs)) {
-error_setg(errp, "Can't write to the image on reopening bitmaps rw");
-return -EINVAL;
-}
-
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
 if (bm_list == NULL) {
@@ -1128,32 +1124,54 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-if (bitmap == NULL) {
-continue;
-}
 
-if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was 
"
-   "not marked as readonly. This is a bug, something went "
-   "wrong. All of the bitmaps may be corrupted", bm->name);
-ret = -EINVAL;
+if (!bitmap) {
+error_setg(errp, "Unexpected bitmap '%s' in the image '%s'",
+   bm->name, bs->filename);
 goto out;
 }
 
-bm->flags |= BME_FLAG_IN_USE;
-ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+if (!(bm->flags & BME_FLAG_IN_USE)) {
+if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE 
"
+   "in the image '%s' and not marked readonly in RAM",
+   bm->name, bs->filename);
+goto out;
+}
+if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+error_setg(errp, "Corruption: bitmap '%s' is inconsistent but "
+   "is not marked IN_USE in the image '%s'", bm->name,
+   bs->filename);
+goto out;
+}
+
+bm->flags |= BME_FLAG_IN_USE;
+need_header_update = true;
+}
+
+if (bdrv_dirty_bitmap_readonly(bitmap)) {
+ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+}
 }
 
-if (ro_dirty_bitmaps != NULL) {
+if (need_header_update) {
+if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) {
+error_setg(errp, "Failed to reopen bitmaps rw: no write access "
+   "the protocol file");
+goto out;
+}
+
 /* in_use flags must be updated */
 ret = update_ext_header_and_dir_in_place(bs, bm_list);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Can't update bitmap directory");
+error_setg_errno(errp, -ret, "Cannot update bitmap directory");
 goto out;
 }
-g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
+g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
+ret = 0;
+
 out:
 g_slist_free(ro_dirty_bitmaps);
 bitmap_list_free(bm_list);
-- 
2.18.0

[Qemu-block] [PATCH v4 01/10] block: switch reopen queue from QSIMPLEQ to QTAILQ

[Vladimir Sementsov-Ogievskiy]

We'll need reverse-foreach in the following commit, QTAILQ support it,
so move to QTAILQ.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block.h |  2 +-
 block.c   | 22 +++---
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 50a07c1c33..3d84b2a2c9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -192,7 +192,7 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_RECURSE  0x40
 #define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
 
-typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) 
BlockReopenQueue;
+typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 
 typedef struct BDRVReopenState {
 BlockDriverState *bs;
diff --git a/block.c b/block.c
index cbd8da5f3b..696162cd7a 100644
--- a/block.c
+++ b/block.c
@@ -1718,7 +1718,7 @@ typedef struct BlockReopenQueueEntry {
  bool prepared;
  bool perms_checked;
  BDRVReopenState state;
- QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+ QTAILQ_ENTRY(BlockReopenQueueEntry) entry;
 } BlockReopenQueueEntry;
 
 /*
@@ -1731,7 +1731,7 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, 
BlockDriverState *bs)
 BlockReopenQueueEntry *entry;
 
 if (q != NULL) {
-QSIMPLEQ_FOREACH(entry, q, entry) {
+QTAILQ_FOREACH(entry, q, entry) {
 if (entry->state.bs == bs) {
 return entry->state.flags;
 }
@@ -3280,7 +3280,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 if (bs_queue == NULL) {
 bs_queue = g_new0(BlockReopenQueue, 1);
-QSIMPLEQ_INIT(bs_queue);
+QTAILQ_INIT(bs_queue);
 }
 
 if (!options) {
@@ -3288,7 +3288,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 }
 
 /* Check if this BlockDriverState is already in the queue */
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 if (bs == bs_entry->state.bs) {
 break;
 }
@@ -3344,7 +3344,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 if (!bs_entry) {
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
-QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+QTAILQ_INSERT_TAIL(bs_queue, bs_entry, entry);
 } else {
 qobject_unref(bs_entry->state.options);
 qobject_unref(bs_entry->state.explicit_options);
@@ -3445,7 +3445,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 
 assert(bs_queue != NULL);
 
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 assert(bs_entry->state.bs->quiesce_counter > 0);
 if (bdrv_reopen_prepare(_entry->state, bs_queue, errp)) {
 goto cleanup;
@@ -3453,7 +3453,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 bs_entry->prepared = true;
 }
 
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 BDRVReopenState *state = _entry->state;
 ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
   state->shared_perm, NULL, NULL, errp);
@@ -3479,13 +3479,13 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 /* If we reach this point, we have success and just need to apply the
  * changes
  */
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 bdrv_reopen_commit(_entry->state);
 }
 
 ret = 0;
 cleanup_perm:
-QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 BDRVReopenState *state = _entry->state;
 
 if (!bs_entry->perms_checked) {
@@ -3502,7 +3502,7 @@ cleanup_perm:
 }
 }
 cleanup:
-QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 if (ret) {
 if (bs_entry->prepared) {
 bdrv_reopen_abort(_entry->state);
@@ -3542,7 +3542,7 @@ static BlockReopenQueueEntry 
*find_parent_in_reopen_queue(BlockReopenQueue *q,
 {
 BlockReopenQueueEntry *entry;
 
-QSIMPLEQ_FOREACH(entry, q, entry) {
+QTAILQ_FOREACH(entry, q, entry) {
 BlockDriverState *bs = entry->state.bs;
 BdrvChild *child;
 
-- 
2.18.0

[Qemu-block] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

Reopening bitmaps to RW was broken prior to previous commit. Check that
it works now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/165 | 46 --
 tests/qemu-iotests/165.out |  4 ++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 88f62d3c6d..dd93b5a2d0 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 os.remove(disk)
 
 def mkVm(self):
-return iotests.VM().add_drive(disk)
+return iotests.VM().add_drive(disk, opts='node-name=node0')
 
 def mkVmRo(self):
-return iotests.VM().add_drive(disk, opts='readonly=on')
+return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
 
 def getSha256(self):
 result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
@@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
 self.vm.shutdown()
 
+def test_reopen_rw(self):
+self.vm = self.mkVm()
+self.vm.launch()
+self.qmpAddBitmap()
+
+# Calculate sha256 corresponding to regions1
+self.writeRegions(regions1)
+sha256 = self.getSha256()
+result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
+ name='bitmap0')
+self.assert_qmp(result, 'return', {})
+
+self.vm.shutdown()
+
+self.vm = self.mkVmRo()
+self.vm.launch()
+
+# We've loaded empty bitmap
+assert sha256 != self.getSha256()
+
+# Check that we are in RO mode
+self.writeRegions(regions1)
+assert sha256 != self.getSha256()
+
+result = self.vm.qmp('x-blockdev-reopen', **{
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk
+},
+'read-only': False
+})
+self.assert_qmp(result, 'return', {})
+
+# Check that bitmap is reopened to RW and we can write to it.
+self.writeRegions(regions1)
+assert sha256 == self.getSha256()
+
+self.vm.shutdown()
+
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
index ae1213e6f8..fbc63e62f8 100644
--- a/tests/qemu-iotests/165.out
+++ b/tests/qemu-iotests/165.out
@@ -1,5 +1,5 @@
-.
+..
 --
-Ran 1 tests
+Ran 2 tests
 
 OK
-- 
2.18.0

[Qemu-block] [PATCH v4 04/10] iotests.py: add event_wait_log and events_wait_log helpers

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce74177ab1..4ad265f140 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -540,6 +540,16 @@ class VM(qtest.QEMUQtestMachine):
 log(result, filters, indent=indent)
 return result
 
+def event_wait_log(self, name, **kwargs):
+event = self.event_wait(name, **kwargs)
+log(event, filters=[filter_qmp_event])
+return event
+
+def events_wait_log(self, events, **kwargs):
+event = self.events_wait(events, **kwargs)
+log(event, filters=[filter_qmp_event])
+return event
+
 # Returns None on success, and an error string on failure
 def run_job(self, job, auto_finalize=True, auto_dismiss=False,
 pre_finalize=None, use_log=True, wait=60.0):
-- 
2.18.0

[Qemu-block] [PATCH v4 06/10] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()

[Vladimir Sementsov-Ogievskiy]

The function is unused, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 block/qcow2.h|  2 --
 block/qcow2-bitmap.c | 15 +--
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index fc1b0d3c1e..a5b24f4eec 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -736,8 +736,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Error **errp);
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 60b79f1dac..fbeee37243 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1102,8 +1102,7 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 return list;
 }
 
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp)
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 Qcow2BitmapList *bm_list;
@@ -,10 +1110,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 GSList *ro_dirty_bitmaps = NULL;
 int ret = 0;
 
-if (header_updated != NULL) {
-*header_updated = false;
-}
-
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
@@ -1156,9 +1151,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 error_setg_errno(errp, -ret, "Can't update bitmap directory");
 goto out;
 }
-if (header_updated != NULL) {
-*header_updated = true;
-}
 g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
@@ -1169,11 +1161,6 @@ out:
 return ret;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
-{
-return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
-}
-
 /* Checks to see if it's safe to resize bitmaps */
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
 {
-- 
2.18.0

[Qemu-block] [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

Hi all!

Bitmaps reopening is buggy, reopening-rw just not working at all and
reopening-ro may lead to producing broken incremental
backup if we do temporary snapshot in a meantime.

v4: Drop complicated solution around reopening logic [Kevin], fix
the existing bug in a simplest way

Structure:

02: fix reopen to RW
03: test reopen to RW

07: fix reopen to RO
08: test reopen to RO

Others are less significant improvements and refactoring

Changelog:

01-03: new patches, to fix reopening bitmaps to RW and personal test for
   this bug
08: merged test from v3, it covers both bugs (reopen to RW and reopen to RO)
10: instead of moving bitmap-reopening to prepare(as in 09 in v3) we now keep it
in commit, but in right place
others: unchanged

v3:
02: John's events_wait already merged in, so my 02 from v2 is not needed.
Instead, add two simple logging wrappers here
03: rebase on 02 - use new wrappers, move to 260
05: add John's r-b
06: improve function docs [John], add John's r-b

v2:
01: new
02-03: test: splat into two patches, some wording
   improvements and event_wait improved
04: add John's r-b
05: new
06-09: fixes: changed, splat, use patch 01

Vladimir Sementsov-Ogievskiy (10):
  block: switch reopen queue from QSIMPLEQ to QTAILQ
  block: reverse order for reopen commits
  iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
  iotests.py: add event_wait_log and events_wait_log helpers
  block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
  block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
  block/qcow2-bitmap: do not remove bitmaps on reopen-ro
  iotests: add test 260 to check bitmap life after snapshot + commit
  block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
  qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

 block/qcow2.h |   5 +-
 include/block/block.h |   2 +-
 include/block/block_int.h |   6 --
 include/block/dirty-bitmap.h  |   1 -
 block.c   |  51 +---
 block/dirty-bitmap.c  |  12 ---
 block/qcow2-bitmap.c  | 143 --
 block/qcow2.c |  17 +++-
 tests/qemu-iotests/165|  46 ++-
 tests/qemu-iotests/165.out|   4 +-
 tests/qemu-iotests/260|  87 +
 tests/qemu-iotests/260.out|  52 +
 tests/qemu-iotests/group  |   1 +
 tests/qemu-iotests/iotests.py |  10 +++
 14 files changed, 318 insertions(+), 119 deletions(-)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

-- 
2.18.0

Re: [Qemu-block] [PATCH v3 0/5] block: Fixes for concurrent block jobs

[Kevin Wolf]

Am 22.07.2019 um 15:33 hat Max Reitz geschrieben:
> I think the patches speak for themselves now.
> 
> (The title of this series alludes to what the iotest added in the final
> patch tests.)
> 
> v3:
> - Rebased on master
> - Added two tests to test-bdrv-drain [Kevin]
> - Removed new iotest from auto [Thomas]

Thanks, applied to block-next.

Kevin

[Qemu-block] [Qemu-devel] [PATCH v6 23/26] cpu: TLB_FLAGS_MASK bit to force memory slow path

[tony.nguyen]

The fast path is taken when TLB_FLAGS_MASK is all zero.

TLB_FORCE_SLOW is simply a TLB_FLAGS_MASK bit to force the slow path,
there are no other side effects.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 include/exec/cpu-all.h | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 536ea58..e496f99 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -331,12 +331,18 @@ CPUArchState *cpu_copy(CPUArchState *env);
 #define TLB_MMIO(1 << (TARGET_PAGE_BITS - 3))
 /* Set if TLB entry must have MMU lookup repeated for every access */
 #define TLB_RECHECK (1 << (TARGET_PAGE_BITS - 4))
+/* Set if TLB entry must take the slow path.  */
+#define TLB_FORCE_SLOW  (1 << (TARGET_PAGE_BITS - 5))

 /* Use this mask to check interception with an alignment mask
  * in a TCG backend.
  */
-#define TLB_FLAGS_MASK  (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO \
- | TLB_RECHECK)
+#define TLB_FLAGS_MASK \
+(TLB_INVALID_MASK  \
+ | TLB_NOTDIRTY\
+ | TLB_MMIO\
+ | TLB_RECHECK \
+ | TLB_FORCE_SLOW)

 /**
  * tlb_hit_page: return true if page aligned @addr is a hit against the
--
1.8.3.1

?

Re: [Qemu-block] [Qemu-devel] [PATCH v6 00/26] Invert Endian bit in SPARCv9 MMU TTE

[tony.nguyen]

On 8/7/19 8:37 PM, Philippe Mathieu-Daudé wrote:

> I'm confused I think I already reviewed various patches of your previous
?> series but don't see my Reviewed-by tags.?


Apologies Philippe! I am the confused one here =/


Will append.


Thank you very much for the reviews and qemu-devel newbie tips so far. I have 
felt very welcome.


Tony

Re: [Qemu-block] [Qemu-devel] [PATCH v6 04/26] target/mips: Access MemoryRegion with MemOp

[tony.nguyen]

Sorry, I missed a tag.


Reviewed-by: Philippe Mathieu-Daudé ?

Re: [Qemu-block] [Qemu-devel] [PATCH v6 26/26] target/sparc: sun4u Invert Endian TTE bit

[tony.nguyen]

Sorry, I missed a tag.?


Tested-by: Mark Cave-Ayland 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 09/26] exec: Access MemoryRegion with MemOp

[tony.nguyen]

Sorry, I missed a tag.


Reviewed-by: Philippe Mathieu-Daudé ?

[Qemu-block] [Qemu-devel] [PATCH v6 10/26] cputlb: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
---
 accel/tcg/cputlb.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 523be4c..6c83878 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -906,8 +906,8 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 qemu_mutex_lock_iothread();
 locked = true;
 }
-r = memory_region_dispatch_read(mr, mr_offset,
-, size, iotlbentry->attrs);
+r = memory_region_dispatch_read(mr, mr_offset, , size_memop(size),
+iotlbentry->attrs);
 if (r != MEMTX_OK) {
 hwaddr physaddr = mr_offset +
 section->offset_within_address_space -
@@ -947,8 +947,8 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 qemu_mutex_lock_iothread();
 locked = true;
 }
-r = memory_region_dispatch_write(mr, mr_offset,
- val, size, iotlbentry->attrs);
+r = memory_region_dispatch_write(mr, mr_offset, val, size_memop(size),
+ iotlbentry->attrs);
 if (r != MEMTX_OK) {
 hwaddr physaddr = mr_offset +
 section->offset_within_address_space -
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 13/26] target/mips: Hard code size with MO_{8|16|32|64}

[tony.nguyen]

Temporarily no-op size_memop was introduced to aid the conversion of
memory_region_dispatch_{read|write} operand "unsigned size" into
"MemOp op".

Now size_memop is implemented, again hard coded size but with
MO_{8|16|32|64}. This is more expressive and avoid size_memop calls.

Signed-off-by: Tony Nguyen 
---
 target/mips/op_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 1c72a00..e79f99d 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -4741,11 +4741,11 @@ void helper_cache(CPUMIPSState *env, target_ulong addr, 
uint32_t op)
 if (op == 9) {
 /* Index Store Tag */
 memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
- size_memop(8), MEMTXATTRS_UNSPECIFIED);
+ MO_64, MEMTXATTRS_UNSPECIFIED);
 } else if (op == 5) {
 /* Index Load Tag */
 memory_region_dispatch_read(env->itc_tag, index, >CP0_TagLo,
-size_memop(8), MEMTXATTRS_UNSPECIFIED);
+MO_64, MEMTXATTRS_UNSPECIFIED);
 }
 #endif
 }
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 22/26] memory: Single byte swap along the I/O path

[tony.nguyen]

Now that MemOp has been pushed down into the memory API, and
callers are encoding endianness, we can collapse byte swaps
along the I/O path into the accelerator and target independent
adjust_endianness.

Collapsing byte swaps along the I/O path enables additional endian
inversion logic, e.g. SPARC64 Invert Endian TTE bit, with redundant
byte swaps cancelling out.

Suggested-by: Richard Henderson 
Signed-off-by: Tony Nguyen 
---
 accel/tcg/cputlb.c | 42 +++--
 hw/virtio/virtio-pci.c | 10 
 memory.c   | 33 ++
 memory_ldst.inc.c  | 64 --
 4 files changed, 19 insertions(+), 130 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 86d85cc..473b8e6 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1200,38 +1200,6 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
target_ulong addr,
 cpu_loop_exit_atomic(env_cpu(env), retaddr);
 }

-#ifdef TARGET_WORDS_BIGENDIAN
-#define NEED_BE_BSWAP 0
-#define NEED_LE_BSWAP 1
-#else
-#define NEED_BE_BSWAP 1
-#define NEED_LE_BSWAP 0
-#endif
-
-/*
- * Byte Swap Helper
- *
- * This should all dead code away depending on the build host and
- * access type.
- */
-
-static inline uint64_t handle_bswap(uint64_t val, MemOp op)
-{
-if ((memop_big_endian(op) && NEED_BE_BSWAP) ||
-(!memop_big_endian(op) && NEED_LE_BSWAP)) {
-switch (op & MO_SIZE) {
-case MO_8: return val;
-case MO_16: return bswap16(val);
-case MO_32: return bswap32(val);
-case MO_64: return bswap64(val);
-default:
-g_assert_not_reached();
-}
-} else {
-return val;
-}
-}
-
 /*
  * Load Helpers
  *
@@ -1306,10 +1274,8 @@ load_helper(CPUArchState *env, target_ulong addr, 
TCGMemOpIdx oi,
 }
 }

-/* FIXME: io_readx ignores MO_BSWAP.  */
-res = io_readx(env, _tlb(env)->d[mmu_idx].iotlb[index],
-   mmu_idx, addr, retaddr, access_type, op);
-return handle_bswap(res, op);
+return io_readx(env, _tlb(env)->d[mmu_idx].iotlb[index],
+mmu_idx, addr, retaddr, access_type, op);
 }

 /* Handle slow unaligned access (it spans two pages or IO).  */
@@ -1552,10 +1518,8 @@ store_helper(CPUArchState *env, target_ulong addr, 
uint64_t val,
 }
 }

-/* FIXME: io_writex ignores MO_BSWAP.  */
 io_writex(env, _tlb(env)->d[mmu_idx].iotlb[index], mmu_idx,
-  handle_bswap(val, op),
-  addr, retaddr, op);
+  val, addr, retaddr, op);
 return;
 }

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 70eb161..f3fe6ca 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -542,16 +542,15 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
hwaddr addr,
 val = pci_get_byte(buf);
 break;
 case 2:
-val = cpu_to_le16(pci_get_word(buf));
+val = pci_get_word(buf);
 break;
 case 4:
-val = cpu_to_le32(pci_get_long(buf));
+val = pci_get_long(buf);
 break;
 default:
 /* As length is under guest control, handle illegal values. */
 return;
 }
-/* FIXME: memory_region_dispatch_write ignores MO_BSWAP.  */
 memory_region_dispatch_write(mr, addr, val, size_memop(len),
  MEMTXATTRS_UNSPECIFIED);
 }
@@ -576,7 +575,6 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr 
addr,
 /* Make sure caller aligned buf properly */
 assert(!(((uintptr_t)buf) & (len - 1)));

-/* FIXME: memory_region_dispatch_read ignores MO_BSWAP.  */
 memory_region_dispatch_read(mr, addr, , size_memop(len),
 MEMTXATTRS_UNSPECIFIED);
 switch (len) {
@@ -584,10 +582,10 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr 
addr,
 pci_set_byte(buf, val);
 break;
 case 2:
-pci_set_word(buf, le16_to_cpu(val));
+pci_set_word(buf, val);
 break;
 case 4:
-pci_set_long(buf, le32_to_cpu(val));
+pci_set_long(buf, val);
 break;
 default:
 /* As length is under guest control, handle illegal values. */
diff --git a/memory.c b/memory.c
index 264c624..9d3c3a6 100644
--- a/memory.c
+++ b/memory.c
@@ -343,32 +343,23 @@ static void flatview_simplify(FlatView *view)
 }
 }

-static bool memory_region_wrong_endianness(MemoryRegion *mr)
+static void adjust_endianness(MemoryRegion *mr, uint64_t *data, MemOp op)
 {
-#ifdef TARGET_WORDS_BIGENDIAN
-return mr->ops->endianness == MO_LE;
-#else
-return mr->ops->endianness == MO_BE;
-#endif
-}
-
-static void adjust_endianness(MemoryRegion *mr, uint64_t *data, unsigned size)
-{
-if (memory_region_wrong_endianness(mr)) {
-switch (size) {
-case 1:
+if ((op & MO_BSWAP) != 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 10/26] cputlb: Access MemoryRegion with MemOp

[tony.nguyen]

Sorry, I missed a tag.


Reviewed-by: Philippe Mathieu-Daudé 

[Qemu-block] [Qemu-devel] [PATCH v6 26/26] target/sparc: sun4u Invert Endian TTE bit

[tony.nguyen]

This bit configures endianness of PCI MMIO devices. It is used by
Solaris and OpenBSD sunhme drivers.

Tested working on OpenBSD.

Unfortunately Solaris 10 had a unrelated keyboard issue blocking
testing... another inch towards Solaris 10 on SPARC64 =)

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 target/sparc/cpu.h| 2 ++
 target/sparc/mmu_helper.c | 8 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 1406f0b..c6bafa8 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -275,6 +275,7 @@ enum {

 #define TTE_VALID_BIT   (1ULL << 63)
 #define TTE_NFO_BIT (1ULL << 60)
+#define TTE_IE_BIT  (1ULL << 59)
 #define TTE_USED_BIT(1ULL << 41)
 #define TTE_LOCKED_BIT  (1ULL <<  6)
 #define TTE_SIDEEFFECT_BIT  (1ULL <<  3)
@@ -291,6 +292,7 @@ enum {

 #define TTE_IS_VALID(tte)   ((tte) & TTE_VALID_BIT)
 #define TTE_IS_NFO(tte) ((tte) & TTE_NFO_BIT)
+#define TTE_IS_IE(tte)  ((tte) & TTE_IE_BIT)
 #define TTE_IS_USED(tte)((tte) & TTE_USED_BIT)
 #define TTE_IS_LOCKED(tte)  ((tte) & TTE_LOCKED_BIT)
 #define TTE_IS_SIDEEFFECT(tte) ((tte) & TTE_SIDEEFFECT_BIT)
diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 826e14b..77dc86a 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -537,6 +537,10 @@ static int get_physical_address_data(CPUSPARCState *env, 
hwaddr *physical,
 if (ultrasparc_tag_match(>dtlb[i], address, context, physical)) {
 int do_fault = 0;

+if (TTE_IS_IE(env->dtlb[i].tte)) {
+attrs->byte_swap = true;
+}
+
 /* access ok? */
 /* multiple bits in SFSR.FT may be set on TT_DFAULT */
 if (TTE_IS_PRIV(env->dtlb[i].tte) && is_user) {
@@ -792,7 +796,7 @@ void dump_mmu(CPUSPARCState *env)
 }
 if (TTE_IS_VALID(env->dtlb[i].tte)) {
 qemu_printf("[%02u] VA: %" PRIx64 ", PA: %llx"
-", %s, %s, %s, %s, ctx %" PRId64 " %s\n",
+", %s, %s, %s, %s, ie %s, ctx %" PRId64 " %s\n",
 i,
 env->dtlb[i].tag & (uint64_t)~0x1fffULL,
 TTE_PA(env->dtlb[i].tte),
@@ -801,6 +805,8 @@ void dump_mmu(CPUSPARCState *env)
 TTE_IS_W_OK(env->dtlb[i].tte) ? "RW" : "RO",
 TTE_IS_LOCKED(env->dtlb[i].tte) ?
 "locked" : "unlocked",
+TTE_IS_IE(env->dtlb[i].tte) ?
+"yes" : "no",
 env->dtlb[i].tag & (uint64_t)0x1fffULL,
 TTE_IS_GLOBAL(env->dtlb[i].tte) ?
 "global" : "local");
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 14/26] exec: Hard code size with MO_{8|16|32|64}

[tony.nguyen]

Temporarily no-op size_memop was introduced to aid the conversion of
memory_region_dispatch_{read|write} operand "unsigned size" into
"MemOp op".

Now size_memop is implemented, again hard coded size but with
MO_{8|16|32|64}. This is more expressive and avoid size_memop calls.

Signed-off-by: Tony Nguyen 
---
 memory_ldst.inc.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/memory_ldst.inc.c b/memory_ldst.inc.c
index 1e8a2fc..de658c4 100644
--- a/memory_ldst.inc.c
+++ b/memory_ldst.inc.c
@@ -38,7 +38,7 @@ static inline uint32_t glue(address_space_ldl_internal, 
SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , size_memop(4), attrs);
+r = memory_region_dispatch_read(mr, addr1, , MO_32, attrs);
 #if defined(TARGET_WORDS_BIGENDIAN)
 if (endian == DEVICE_LITTLE_ENDIAN) {
 val = bswap32(val);
@@ -114,7 +114,7 @@ static inline uint64_t glue(address_space_ldq_internal, 
SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , size_memop(8), attrs);
+r = memory_region_dispatch_read(mr, addr1, , MO_64, attrs);
 #if defined(TARGET_WORDS_BIGENDIAN)
 if (endian == DEVICE_LITTLE_ENDIAN) {
 val = bswap64(val);
@@ -188,7 +188,7 @@ uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , size_memop(1), attrs);
+r = memory_region_dispatch_read(mr, addr1, , MO_8, attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -224,7 +224,7 @@ static inline uint32_t glue(address_space_lduw_internal, 
SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , size_memop(2), attrs);
+r = memory_region_dispatch_read(mr, addr1, , MO_16, attrs);
 #if defined(TARGET_WORDS_BIGENDIAN)
 if (endian == DEVICE_LITTLE_ENDIAN) {
 val = bswap16(val);
@@ -300,7 +300,7 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
 if (l < 4 || !memory_access_is_direct(mr, true)) {
 release_lock |= prepare_mmio_access(mr);

-r = memory_region_dispatch_write(mr, addr1, val, size_memop(4), attrs);
+r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs);
 } else {
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
 stl_p(ptr, val);
@@ -346,7 +346,7 @@ static inline void glue(address_space_stl_internal, 
SUFFIX)(ARG1_DECL,
 val = bswap32(val);
 }
 #endif
-r = memory_region_dispatch_write(mr, addr1, val, size_memop(4), attrs);
+r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -408,7 +408,7 @@ void glue(address_space_stb, SUFFIX)(ARG1_DECL,
 mr = TRANSLATE(addr, , , true, attrs);
 if (!memory_access_is_direct(mr, true)) {
 release_lock |= prepare_mmio_access(mr);
-r = memory_region_dispatch_write(mr, addr1, val, size_memop(1), attrs);
+r = memory_region_dispatch_write(mr, addr1, val, MO_8, attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -451,7 +451,7 @@ static inline void glue(address_space_stw_internal, 
SUFFIX)(ARG1_DECL,
 val = bswap16(val);
 }
 #endif
-r = memory_region_dispatch_write(mr, addr1, val, size_memop(2), attrs);
+r = memory_region_dispatch_write(mr, addr1, val, MO_16, attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -524,7 +524,7 @@ static void glue(address_space_stq_internal, 
SUFFIX)(ARG1_DECL,
 val = bswap64(val);
 }
 #endif
-r = memory_region_dispatch_write(mr, addr1, val, size_memop(8), attrs);
+r = memory_region_dispatch_write(mr, addr1, val, MO_64, attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 19/26] exec: Delete DEVICE_HOST_ENDIAN

[tony.nguyen]

DEVICE_HOST_ENDIAN is conditional upon HOST_WORDS_BIGENDIAN.

Code is cleaner if the single use of DEVICE_HOST_ENDIAN is instead
directly conditional upon HOST_WORDS_BIGENDIAN.

Signed-off-by: Tony Nguyen 
---
 include/exec/cpu-common.h | 8 
 memory.c  | 6 +-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 7eeb78c..b33dc0c 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -16,14 +16,6 @@ void tcg_flush_softmmu_tlb(CPUState *cs);

 #if !defined(CONFIG_USER_ONLY)

-#include "exec/memop.h"
-
-#if defined(HOST_WORDS_BIGENDIAN)
-#define DEVICE_HOST_ENDIAN MO_BE
-#else
-#define DEVICE_HOST_ENDIAN MO_LE
-#endif
-
 /* address in the RAM (different from a physical address) */
 #if defined(CONFIG_XEN_BACKEND)
 typedef uint64_t ram_addr_t;
diff --git a/memory.c b/memory.c
index 3cabb52..11db6ec 100644
--- a/memory.c
+++ b/memory.c
@@ -1362,7 +1362,11 @@ static void memory_region_ram_device_write(void *opaque, 
hwaddr addr,
 static const MemoryRegionOps ram_device_mem_ops = {
 .read = memory_region_ram_device_read,
 .write = memory_region_ram_device_write,
-.endianness = DEVICE_HOST_ENDIAN,
+#if defined(HOST_WORDS_BIGENDIAN)
+.endianness = MO_BE,
+#else
+.endianness = MO_LE,
+#endif
 .valid = {
 .min_access_size = 1,
 .max_access_size = 8,
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 08/26] hw/vfio: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 hw/vfio/pci-quirks.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index b35a640..fb3cc33 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -11,6 +11,7 @@
  */

 #include "qemu/osdep.h"
+#include "exec/memop.h"
 #include "qemu/units.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
@@ -1071,7 +1072,7 @@ static void vfio_rtl8168_quirk_address_write(void 
*opaque, hwaddr addr,

 /* Write to the proper guest MSI-X table instead */
 memory_region_dispatch_write(>pdev.msix_table_mmio,
- offset, val, size,
+ offset, val, size_memop(size),
  MEMTXATTRS_UNSPECIFIED);
 }
 return; /* Do not write guest MSI-X data to hardware */
@@ -1102,7 +1103,8 @@ static uint64_t vfio_rtl8168_quirk_data_read(void *opaque,
 if (rtl->enabled && (vdev->pdev.cap_present & QEMU_PCI_CAP_MSIX)) {
 hwaddr offset = rtl->addr & 0xfff;
 memory_region_dispatch_read(>pdev.msix_table_mmio, offset,
-, size, MEMTXATTRS_UNSPECIFIED);
+, size_memop(size),
+MEMTXATTRS_UNSPECIFIED);
 trace_vfio_quirk_rtl8168_msix_read(vdev->vbasedev.name, offset, data);
 }

--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 25/26] target/sparc: Add TLB entry with attributes

[tony.nguyen]

Append MemTxAttrs to interfaces so we can pass along up coming Invert
Endian TTE bit on SPARC64.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 target/sparc/mmu_helper.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index cbd1e91..826e14b 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -88,7 +88,7 @@ static const int perm_table[2][8] = {
 };

 static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
-int *prot, int *access_index,
+int *prot, int *access_index, MemTxAttrs 
*attrs,
 target_ulong address, int rw, int mmu_idx,
 target_ulong *page_size)
 {
@@ -219,6 +219,7 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 target_ulong vaddr;
 target_ulong page_size;
 int error_code = 0, prot, access_index;
+MemTxAttrs attrs = {};

 /*
  * TODO: If we ever need tlb_vaddr_to_host for this target,
@@ -229,7 +230,7 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 assert(!probe);

 address &= TARGET_PAGE_MASK;
-error_code = get_physical_address(env, , , _index,
+error_code = get_physical_address(env, , , _index, 
,
   address, access_type,
   mmu_idx, _size);
 vaddr = address;
@@ -490,8 +491,8 @@ static inline int ultrasparc_tag_match(SparcTLBEntry *tlb,
 return 0;
 }

-static int get_physical_address_data(CPUSPARCState *env,
- hwaddr *physical, int *prot,
+static int get_physical_address_data(CPUSPARCState *env, hwaddr *physical,
+ int *prot, MemTxAttrs *attrs,
  target_ulong address, int rw, int mmu_idx)
 {
 CPUState *cs = env_cpu(env);
@@ -608,8 +609,8 @@ static int get_physical_address_data(CPUSPARCState *env,
 return 1;
 }

-static int get_physical_address_code(CPUSPARCState *env,
- hwaddr *physical, int *prot,
+static int get_physical_address_code(CPUSPARCState *env, hwaddr *physical,
+ int *prot, MemTxAttrs *attrs,
  target_ulong address, int mmu_idx)
 {
 CPUState *cs = env_cpu(env);
@@ -686,7 +687,7 @@ static int get_physical_address_code(CPUSPARCState *env,
 }

 static int get_physical_address(CPUSPARCState *env, hwaddr *physical,
-int *prot, int *access_index,
+int *prot, int *access_index, MemTxAttrs 
*attrs,
 target_ulong address, int rw, int mmu_idx,
 target_ulong *page_size)
 {
@@ -716,11 +717,11 @@ static int get_physical_address(CPUSPARCState *env, 
hwaddr *physical,
 }

 if (rw == 2) {
-return get_physical_address_code(env, physical, prot, address,
+return get_physical_address_code(env, physical, prot, attrs, address,
  mmu_idx);
 } else {
-return get_physical_address_data(env, physical, prot, address, rw,
- mmu_idx);
+return get_physical_address_data(env, physical, prot, attrs, address,
+ rw, mmu_idx);
 }
 }

@@ -734,10 +735,11 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 target_ulong vaddr;
 hwaddr paddr;
 target_ulong page_size;
+MemTxAttrs attrs = {};
 int error_code = 0, prot, access_index;

 address &= TARGET_PAGE_MASK;
-error_code = get_physical_address(env, , , _index,
+error_code = get_physical_address(env, , , _index, 
,
   address, access_type,
   mmu_idx, _size);
 if (likely(error_code == 0)) {
@@ -747,7 +749,8 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
env->dmmu.mmu_primary_context,
env->dmmu.mmu_secondary_context);

-tlb_set_page(cs, vaddr, paddr, prot, mmu_idx, page_size);
+tlb_set_page_with_attrs(cs, vaddr, paddr, attrs, prot, mmu_idx,
+page_size);
 return true;
 }
 if (probe) {
@@ -849,9 +852,10 @@ static int cpu_sparc_get_phys_page(CPUSPARCState *env, 
hwaddr *phys,
 {
 target_ulong page_size;
 int prot, access_index;
+MemTxAttrs attrs = {};

-return get_physical_address(env, phys, , _index, addr, rw,
-mmu_idx, _size);
+return get_physical_address(env, phys, , _index, , addr,
+rw, mmu_idx, _size);
 }

 #if defined(TARGET_SPARC64)
--
1.8.3.1

?

Re: [Qemu-block] [Qemu-devel] [PATCH v6 06/26] hw/intc/armv7m_nic: Access MemoryRegion with MemOp

[tony.nguyen]

Sorry, I missed a tag.


Reviewed-by: Philippe Mathieu-Daudé ?

[Qemu-block] [Qemu-devel] [PATCH v6 05/26] hw/s390x: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 hw/s390x/s390-pci-inst.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 0023514..0c958fc 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -15,6 +15,7 @@
 #include "cpu.h"
 #include "s390-pci-inst.h"
 #include "s390-pci-bus.h"
+#include "exec/memop.h"
 #include "exec/memory-internal.h"
 #include "qemu/error-report.h"
 #include "sysemu/hw_accel.h"
@@ -372,7 +373,7 @@ static MemTxResult zpci_read_bar(S390PCIBusDevice *pbdev, 
uint8_t pcias,
 mr = pbdev->pdev->io_regions[pcias].memory;
 mr = s390_get_subregion(mr, offset, len);
 offset -= mr->addr;
-return memory_region_dispatch_read(mr, offset, data, len,
+return memory_region_dispatch_read(mr, offset, data, size_memop(len),
MEMTXATTRS_UNSPECIFIED);
 }

@@ -471,7 +472,7 @@ static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, 
uint8_t pcias,
 mr = pbdev->pdev->io_regions[pcias].memory;
 mr = s390_get_subregion(mr, offset, len);
 offset -= mr->addr;
-return memory_region_dispatch_write(mr, offset, data, len,
+return memory_region_dispatch_write(mr, offset, data, size_memop(len),
 MEMTXATTRS_UNSPECIFIED);
 }

@@ -780,7 +781,8 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r3, uint64_t gaddr,

 for (i = 0; i < len / 8; i++) {
 result = memory_region_dispatch_write(mr, offset + i * 8,
-  ldq_p(buffer + i * 8), 8,
+  ldq_p(buffer + i * 8),
+  size_memop(8),
   MEMTXATTRS_UNSPECIFIED);
 if (result != MEMTX_OK) {
 s390_program_interrupt(env, PGM_OPERAND, 6, ra);
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 21/26] cputlb: Replace size and endian operands for MemOp

[tony.nguyen]

Preparation for collapsing the two byte swaps adjust_endianness and
handle_bswap into the former.

Signed-off-by: Tony Nguyen 
---
 accel/tcg/cputlb.c   | 170 +--
 include/exec/memop.h |   6 ++
 memory.c |  11 +---
 3 files changed, 90 insertions(+), 97 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 6c83878..86d85cc 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -881,7 +881,7 @@ static void tlb_fill(CPUState *cpu, target_ulong addr, int 
size,

 static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
  int mmu_idx, target_ulong addr, uintptr_t retaddr,
- MMUAccessType access_type, int size)
+ MMUAccessType access_type, MemOp op)
 {
 CPUState *cpu = env_cpu(env);
 hwaddr mr_offset;
@@ -906,14 +906,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 qemu_mutex_lock_iothread();
 locked = true;
 }
-r = memory_region_dispatch_read(mr, mr_offset, , size_memop(size),
-iotlbentry->attrs);
+r = memory_region_dispatch_read(mr, mr_offset, , op, 
iotlbentry->attrs);
 if (r != MEMTX_OK) {
 hwaddr physaddr = mr_offset +
 section->offset_within_address_space -
 section->offset_within_region;

-cpu_transaction_failed(cpu, physaddr, addr, size, access_type,
+cpu_transaction_failed(cpu, physaddr, addr, memop_size(op), 
access_type,
mmu_idx, iotlbentry->attrs, r, retaddr);
 }
 if (locked) {
@@ -925,7 +924,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,

 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
   int mmu_idx, uint64_t val, target_ulong addr,
-  uintptr_t retaddr, int size)
+  uintptr_t retaddr, MemOp op)
 {
 CPUState *cpu = env_cpu(env);
 hwaddr mr_offset;
@@ -947,15 +946,15 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 qemu_mutex_lock_iothread();
 locked = true;
 }
-r = memory_region_dispatch_write(mr, mr_offset, val, size_memop(size),
- iotlbentry->attrs);
+r = memory_region_dispatch_write(mr, mr_offset, val, op, 
iotlbentry->attrs);
 if (r != MEMTX_OK) {
 hwaddr physaddr = mr_offset +
 section->offset_within_address_space -
 section->offset_within_region;

-cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
-   mmu_idx, iotlbentry->attrs, r, retaddr);
+cpu_transaction_failed(cpu, physaddr, addr, memop_size(op),
+   MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
+   retaddr);
 }
 if (locked) {
 qemu_mutex_unlock_iothread();
@@ -1216,14 +1215,15 @@ static void *atomic_mmu_lookup(CPUArchState *env, 
target_ulong addr,
  * access type.
  */

-static inline uint64_t handle_bswap(uint64_t val, int size, bool big_endian)
+static inline uint64_t handle_bswap(uint64_t val, MemOp op)
 {
-if ((big_endian && NEED_BE_BSWAP) || (!big_endian && NEED_LE_BSWAP)) {
-switch (size) {
-case 1: return val;
-case 2: return bswap16(val);
-case 4: return bswap32(val);
-case 8: return bswap64(val);
+if ((memop_big_endian(op) && NEED_BE_BSWAP) ||
+(!memop_big_endian(op) && NEED_LE_BSWAP)) {
+switch (op & MO_SIZE) {
+case MO_8: return val;
+case MO_16: return bswap16(val);
+case MO_32: return bswap32(val);
+case MO_64: return bswap64(val);
 default:
 g_assert_not_reached();
 }
@@ -1246,7 +1246,7 @@ typedef uint64_t FullLoadHelper(CPUArchState *env, 
target_ulong addr,

 static inline uint64_t __attribute__((always_inline))
 load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
-uintptr_t retaddr, size_t size, bool big_endian, bool code_read,
+uintptr_t retaddr, MemOp op, bool code_read,
 FullLoadHelper *full_load)
 {
 uintptr_t mmu_idx = get_mmuidx(oi);
@@ -1260,6 +1260,7 @@ load_helper(CPUArchState *env, target_ulong addr, 
TCGMemOpIdx oi,
 unsigned a_bits = get_alignment_bits(get_memop(oi));
 void *haddr;
 uint64_t res;
+size_t size = memop_size(op);

 /* Handle CPU specific unaligned behaviour */
 if (addr & ((1 << a_bits) - 1)) {
@@ -1305,9 +1306,10 @@ load_helper(CPUArchState *env, target_ulong addr, 
TCGMemOpIdx oi,
 }
 }

+/* FIXME: io_readx ignores MO_BSWAP.  */
 res = io_readx(env, _tlb(env)->d[mmu_idx].iotlb[index],
-   mmu_idx, addr, retaddr, access_type, size);
-return handle_bswap(res, size, big_endian);
+   

[Qemu-block] [Qemu-devel] [PATCH v6 07/26] hw/virtio: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 hw/virtio/virtio-pci.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f6d2223..25875c8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -17,6 +17,7 @@

 #include "qemu/osdep.h"

+#include "exec/memop.h"
 #include "standard-headers/linux/virtio_pci.h"
 #include "hw/virtio/virtio.h"
 #include "hw/pci/pci.h"
@@ -550,7 +551,8 @@ void virtio_address_space_write(VirtIOPCIProxy *proxy, 
hwaddr addr,
 /* As length is under guest control, handle illegal values. */
 return;
 }
-memory_region_dispatch_write(mr, addr, val, len, MEMTXATTRS_UNSPECIFIED);
+memory_region_dispatch_write(mr, addr, val, size_memop(len),
+ MEMTXATTRS_UNSPECIFIED);
 }

 static void
@@ -573,7 +575,8 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr 
addr,
 /* Make sure caller aligned buf properly */
 assert(!(((uintptr_t)buf) & (len - 1)));

-memory_region_dispatch_read(mr, addr, , len, MEMTXATTRS_UNSPECIFIED);
+memory_region_dispatch_read(mr, addr, , size_memop(len),
+MEMTXATTRS_UNSPECIFIED);
 switch (len) {
 case 1:
 pci_set_byte(buf, val);
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 04/26] target/mips: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 target/mips/op_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 9e2e02f..1c72a00 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -24,6 +24,7 @@
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
+#include "exec/memop.h"
 #include "sysemu/kvm.h"

 /*/
@@ -4740,11 +4741,11 @@ void helper_cache(CPUMIPSState *env, target_ulong addr, 
uint32_t op)
 if (op == 9) {
 /* Index Store Tag */
 memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
- 8, MEMTXATTRS_UNSPECIFIED);
+ size_memop(8), MEMTXATTRS_UNSPECIFIED);
 } else if (op == 5) {
 /* Index Load Tag */
 memory_region_dispatch_read(env->itc_tag, index, >CP0_TagLo,
-8, MEMTXATTRS_UNSPECIFIED);
+size_memop(8), MEMTXATTRS_UNSPECIFIED);
 }
 #endif
 }
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 24/26] cputlb: Byte swap memory transaction attribute

[tony.nguyen]

Notice new attribute, byte swap, and force the transaction through the
memory slow path.

Required by architectures that can invert endianness of memory
transaction, e.g. SPARC64 has the Invert Endian TTE bit.

Suggested-by: Richard Henderson 
Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 accel/tcg/cputlb.c  | 11 +++
 include/exec/memattrs.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 473b8e6..f6f4dd5 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -738,6 +738,9 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong 
vaddr,
  */
 address |= TLB_RECHECK;
 }
+if (attrs.byte_swap) {
+address |= TLB_FORCE_SLOW;
+}
 if (!memory_region_is_ram(section->mr) &&
 !memory_region_is_romd(section->mr)) {
 /* IO memory case */
@@ -891,6 +894,10 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 bool locked = false;
 MemTxResult r;

+if (iotlbentry->attrs.byte_swap) {
+op ^= MO_BSWAP;
+}
+
 section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
 mr = section->mr;
 mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
@@ -933,6 +940,10 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry 
*iotlbentry,
 bool locked = false;
 MemTxResult r;

+if (iotlbentry->attrs.byte_swap) {
+op ^= MO_BSWAP;
+}
+
 section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
 mr = section->mr;
 mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d4a3477..95f2d20 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -37,6 +37,8 @@ typedef struct MemTxAttrs {
 unsigned int user:1;
 /* Requester ID (for MSI for example) */
 unsigned int requester_id:16;
+/* Invert endianness for this page */
+unsigned int byte_swap:1;
 /*
  * The following are target-specific page-table bits.  These are not
  * related to actual memory transactions at all.  However, this structure
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 15/26] build: Correct non-common common-obj-* to obj-*

[tony.nguyen]

Preparation for replacing device_endian with MemOp.

Device realizing code with MemorRegionOps endianness as
DEVICE_NATIVE_ENDIAN is not common code.

Corrected devices were identified by making the declaration of
DEVICE_NATIVE_ENDIAN conditional upon NEED_CPU_H and then listing
what failed to compile.

Signed-off-by: Tony Nguyen 
---
 hw/audio/Makefile.objs| 11 ++-
 hw/block/Makefile.objs|  8 
 hw/char/Makefile.objs | 22 +++---
 hw/core/Makefile.objs |  2 +-
 hw/display/Makefile.objs  | 10 +-
 hw/dma/Makefile.objs  | 18 +-
 hw/gpio/Makefile.objs |  6 +++---
 hw/i2c/Makefile.objs  |  8 
 hw/input/Makefile.objs|  4 ++--
 hw/intc/Makefile.objs | 19 ++-
 hw/ipack/Makefile.objs|  2 +-
 hw/isa/Makefile.objs  |  2 +-
 hw/misc/Makefile.objs | 18 +-
 hw/net/Makefile.objs  | 14 +++---
 hw/pci-host/Makefile.objs |  6 +++---
 hw/scsi/Makefile.objs |  2 +-
 hw/sd/Makefile.objs   |  2 +-
 hw/ssi/Makefile.objs  | 10 +-
 hw/timer/Makefile.objs| 39 ---
 hw/virtio/Makefile.objs   |  2 +-
 20 files changed, 104 insertions(+), 101 deletions(-)

diff --git a/hw/audio/Makefile.objs b/hw/audio/Makefile.objs
index 63db383..40b26c6 100644
--- a/hw/audio/Makefile.objs
+++ b/hw/audio/Makefile.objs
@@ -5,14 +5,15 @@ common-obj-$(CONFIG_AC97) += ac97.o
 common-obj-$(CONFIG_ADLIB) += fmopl.o adlib.o
 common-obj-$(CONFIG_GUS) += gus.o gusemu_hal.o gusemu_mixer.o
 common-obj-$(CONFIG_CS4231A) += cs4231a.o
-common-obj-$(CONFIG_HDA) += intel-hda.o hda-codec.o
+common-obj-$(CONFIG_HDA) += hda-codec.o
+obj-$(CONFIG_HDA) += intel-hda.o

 common-obj-$(CONFIG_PCSPK) += pcspk.o
 common-obj-$(CONFIG_WM8750) += wm8750.o
-common-obj-$(CONFIG_PL041) += pl041.o lm4549.o
+obj-$(CONFIG_PL041) += pl041.o lm4549.o

-common-obj-$(CONFIG_CS4231) += cs4231.o
-common-obj-$(CONFIG_MARVELL_88W8618) += marvell_88w8618.o
-common-obj-$(CONFIG_MILKYMIST) += milkymist-ac97.o
+obj-$(CONFIG_CS4231) += cs4231.o
+obj-$(CONFIG_MARVELL_88W8618) += marvell_88w8618.o
+obj-$(CONFIG_MILKYMIST) += milkymist-ac97.o

 common-obj-y += soundhw.o
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index f5f643f..9098cda 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -1,12 +1,12 @@
 common-obj-y += block.o cdrom.o hd-geometry.o
-common-obj-$(CONFIG_FDC) += fdc.o
+obj-$(CONFIG_FDC) += fdc.o
 common-obj-$(CONFIG_SSI_M25P80) += m25p80.o
 common-obj-$(CONFIG_NAND) += nand.o
-common-obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o
-common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
+obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o
+obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
 common-obj-$(CONFIG_XEN) += xen-block.o
 common-obj-$(CONFIG_ECC) += ecc.o
-common-obj-$(CONFIG_ONENAND) += onenand.o
+obj-$(CONFIG_ONENAND) += onenand.o
 common-obj-$(CONFIG_NVME_PCI) += nvme.o

 obj-$(CONFIG_SH4) += tc58128.o
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 02d8a66..af3e76a 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -1,17 +1,17 @@
 common-obj-$(CONFIG_IPACK) += ipoctal232.o
-common-obj-$(CONFIG_ESCC) += escc.o
+obj-$(CONFIG_ESCC) += escc.o
 common-obj-$(CONFIG_NRF51_SOC) += nrf51_uart.o
-common-obj-$(CONFIG_PARALLEL) += parallel.o
+obj-$(CONFIG_PARALLEL) += parallel.o
 common-obj-$(CONFIG_ISA_BUS) += parallel-isa.o
-common-obj-$(CONFIG_PL011) += pl011.o
-common-obj-$(CONFIG_SERIAL) += serial.o
+obj-$(CONFIG_PL011) += pl011.o
+obj-$(CONFIG_SERIAL) += serial.o
 common-obj-$(CONFIG_SERIAL_ISA) += serial-isa.o
 common-obj-$(CONFIG_SERIAL_PCI) += serial-pci.o
 common-obj-$(CONFIG_SERIAL_PCI_MULTI) += serial-pci-multi.o
 common-obj-$(CONFIG_VIRTIO_SERIAL) += virtio-console.o
-common-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
+obj-$(CONFIG_XILINX) += xilinx_uartlite.o
 common-obj-$(CONFIG_XEN) += xen_console.o
-common-obj-$(CONFIG_CADENCE) += cadence_uart.o
+obj-$(CONFIG_CADENCE) += cadence_uart.o

 obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
 obj-$(CONFIG_COLDFIRE) += mcf_uart.o
@@ -23,13 +23,13 @@ obj-$(CONFIG_STM32F2XX_USART) += stm32f2xx_usart.o
 obj-$(CONFIG_RASPI) += bcm2835_aux.o

 common-obj-$(CONFIG_CMSDK_APB_UART) += cmsdk-apb-uart.o
-common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
+obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
 common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
-common-obj-$(CONFIG_GRLIB) += grlib_apbuart.o
-common-obj-$(CONFIG_IMX) += imx_serial.o
+obj-$(CONFIG_GRLIB) += grlib_apbuart.o
+obj-$(CONFIG_IMX) += imx_serial.o
 common-obj-$(CONFIG_LM32) += lm32_juart.o
-common-obj-$(CONFIG_LM32) += lm32_uart.o
-common-obj-$(CONFIG_MILKYMIST) += milkymist-uart.o
+obj-$(CONFIG_LM32) += lm32_uart.o
+obj-$(CONFIG_MILKYMIST) += milkymist-uart.o
 common-obj-$(CONFIG_SCLPCONSOLE) += sclpconsole.o sclpconsole-lm.o

 obj-$(CONFIG_VIRTIO) += virtio-serial-bus.o
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 

[Qemu-block] [Qemu-devel] [PATCH v6 16/26] exec: Map device_endian onto MemOp

[tony.nguyen]

Preparation to replace device_endian with MemOp.

Mapping device_endian onto MemOp limits behaviour changes to this
relatively smaller patch.

The next patch will replace all device_endian usages with the
equivalent MemOp. That patch will be large but have no behaviour
changes.

A subsequent patch will then delete unused device_endian.

Signed-off-by: Tony Nguyen 
---
 hw/char/serial.c  | 18 ++
 include/exec/cpu-common.h | 10 +++---
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 7c42a2a..7345f69 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1012,22 +1012,15 @@ static void serial_mm_write(void *opaque, hwaddr addr,
 serial_ioport_write(s, addr >> s->it_shift, value, 1);
 }

-static const MemoryRegionOps serial_mm_ops[3] = {
-[DEVICE_NATIVE_ENDIAN] = {
-.read = serial_mm_read,
-.write = serial_mm_write,
-.endianness = DEVICE_NATIVE_ENDIAN,
-.valid.max_access_size = 8,
-.impl.max_access_size = 8,
-},
-[DEVICE_LITTLE_ENDIAN] = {
+static const MemoryRegionOps serial_mm_ops[2] = {
+[0] = {
 .read = serial_mm_read,
 .write = serial_mm_write,
 .endianness = DEVICE_LITTLE_ENDIAN,
 .valid.max_access_size = 8,
 .impl.max_access_size = 8,
 },
-[DEVICE_BIG_ENDIAN] = {
+[1] = {
 .read = serial_mm_read,
 .write = serial_mm_write,
 .endianness = DEVICE_BIG_ENDIAN,
@@ -1053,8 +1046,9 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
 serial_realize_core(s, _fatal);
 vmstate_register(NULL, base, _serial, s);

-memory_region_init_io(>io, NULL, _mm_ops[end], s,
-  "serial", 8 << it_shift);
+memory_region_init_io(>io, NULL,
+  _mm_ops[end == DEVICE_LITTLE_ENDIAN ? 0 : 1],
+  s, "serial", 8 << it_shift);
 memory_region_add_subregion(address_space, base, >io);
 return s;
 }
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index f7dbe75..c388453 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -16,10 +16,14 @@ void tcg_flush_softmmu_tlb(CPUState *cs);

 #if !defined(CONFIG_USER_ONLY)

+#include "exec/memop.h"
+
 enum device_endian {
-DEVICE_NATIVE_ENDIAN,
-DEVICE_BIG_ENDIAN,
-DEVICE_LITTLE_ENDIAN,
+#ifdef NEED_CPU_H
+DEVICE_NATIVE_ENDIAN = MO_TE,
+#endif
+DEVICE_BIG_ENDIAN = MO_BE,
+DEVICE_LITTLE_ENDIAN = MO_LE,
 };

 #if defined(HOST_WORDS_BIGENDIAN)
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 18/26] exec: Delete device_endian

[tony.nguyen]

device_endian has been made redundant by MemOp.

Signed-off-by: Tony Nguyen 
---
 include/exec/cpu-common.h | 8 
 1 file changed, 8 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 01a29ba..7eeb78c 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -18,14 +18,6 @@ void tcg_flush_softmmu_tlb(CPUState *cs);

 #include "exec/memop.h"

-enum device_endian {
-#ifdef NEED_CPU_H
-DEVICE_NATIVE_ENDIAN = MO_TE,
-#endif
-DEVICE_BIG_ENDIAN = MO_BE,
-DEVICE_LITTLE_ENDIAN = MO_LE,
-};
-
 #if defined(HOST_WORDS_BIGENDIAN)
 #define DEVICE_HOST_ENDIAN MO_BE
 #else
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 06/26] hw/intc/armv7m_nic: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 hw/intc/armv7m_nvic.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 9f8f0d3..237ccef 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -18,6 +18,7 @@
 #include "hw/intc/armv7m_nvic.h"
 #include "target/arm/cpu.h"
 #include "exec/exec-all.h"
+#include "exec/memop.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "trace.h"
@@ -2345,7 +2346,8 @@ static MemTxResult nvic_sysreg_ns_write(void *opaque, 
hwaddr addr,
 if (attrs.secure) {
 /* S accesses to the alias act like NS accesses to the real region */
 attrs.secure = 0;
-return memory_region_dispatch_write(mr, addr, value, size, attrs);
+return memory_region_dispatch_write(mr, addr, value, size_memop(size),
+attrs);
 } else {
 /* NS attrs are RAZ/WI for privileged, and BusFault for user */
 if (attrs.user) {
@@ -2364,7 +2366,8 @@ static MemTxResult nvic_sysreg_ns_read(void *opaque, 
hwaddr addr,
 if (attrs.secure) {
 /* S accesses to the alias act like NS accesses to the real region */
 attrs.secure = 0;
-return memory_region_dispatch_read(mr, addr, data, size, attrs);
+return memory_region_dispatch_read(mr, addr, data, size_memop(size),
+   attrs);
 } else {
 /* NS attrs are RAZ/WI for privileged, and BusFault for user */
 if (attrs.user) {
@@ -2390,7 +2393,8 @@ static MemTxResult nvic_systick_write(void *opaque, 
hwaddr addr,

 /* Direct the access to the correct systick */
 mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(>systick[attrs.secure]), 0);
-return memory_region_dispatch_write(mr, addr, value, size, attrs);
+return memory_region_dispatch_write(mr, addr, value, size_memop(size),
+attrs);
 }

 static MemTxResult nvic_systick_read(void *opaque, hwaddr addr,
@@ -2402,7 +2406,7 @@ static MemTxResult nvic_systick_read(void *opaque, hwaddr 
addr,

 /* Direct the access to the correct systick */
 mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(>systick[attrs.secure]), 0);
-return memory_region_dispatch_read(mr, addr, data, size, attrs);
+return memory_region_dispatch_read(mr, addr, data, size_memop(size), 
attrs);
 }

 static const MemoryRegionOps nvic_systick_ops = {
--
1.8.3.1

?

[Qemu-block] [Qemu-devel] [PATCH v6 09/26] exec: Access MemoryRegion with MemOp

[tony.nguyen]

The memory_region_dispatch_{read|write} operand "unsigned size" is
being converted into a "MemOp op".

Convert interfaces by using no-op size_memop.

After all interfaces are converted, size_memop will be implemented
and the memory_region_dispatch_{read|write} operand "unsigned size"
will be converted into a "MemOp op".

As size_memop is a no-op, this patch does not change any behaviour.

Signed-off-by: Tony Nguyen 
Reviewed-by: Richard Henderson 
---
 exec.c|  6 --
 memory_ldst.inc.c | 18 +-
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/exec.c b/exec.c
index 3e78de3..9f69197 100644
--- a/exec.c
+++ b/exec.c
@@ -3334,7 +3334,8 @@ static MemTxResult flatview_write_continue(FlatView *fv, 
hwaddr addr,
 /* XXX: could force current_cpu to NULL to avoid
potential bugs */
 val = ldn_p(buf, l);
-result |= memory_region_dispatch_write(mr, addr1, val, l, attrs);
+result |= memory_region_dispatch_write(mr, addr1, val,
+   size_memop(l), attrs);
 } else {
 /* RAM case */
 ptr = qemu_ram_ptr_length(mr->ram_block, addr1, , false);
@@ -3395,7 +3396,8 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr 
addr,
 /* I/O case */
 release_lock |= prepare_mmio_access(mr);
 l = memory_access_size(mr, l, addr1);
-result |= memory_region_dispatch_read(mr, addr1, , l, attrs);
+result |= memory_region_dispatch_read(mr, addr1, ,
+  size_memop(l), attrs);
 stn_p(buf, l, val);
 } else {
 /* RAM case */
diff --git a/memory_ldst.inc.c b/memory_ldst.inc.c
index acf865b..1e8a2fc 100644
--- a/memory_ldst.inc.c
+++ b/memory_ldst.inc.c
@@ -38,7 +38,7 @@ static inline uint32_t glue(address_space_ldl_internal, 
SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , 4, attrs);
+r = memory_region_dispatch_read(mr, addr1, , size_memop(4), attrs);
 #if defined(TARGET_WORDS_BIGENDIAN)
 if (endian == DEVICE_LITTLE_ENDIAN) {
 val = bswap32(val);
@@ -114,7 +114,7 @@ static inline uint64_t glue(address_space_ldq_internal, 
SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , 8, attrs);
+r = memory_region_dispatch_read(mr, addr1, , size_memop(8), attrs);
 #if defined(TARGET_WORDS_BIGENDIAN)
 if (endian == DEVICE_LITTLE_ENDIAN) {
 val = bswap64(val);
@@ -188,7 +188,7 @@ uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , 1, attrs);
+r = memory_region_dispatch_read(mr, addr1, , size_memop(1), attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -224,7 +224,7 @@ static inline uint32_t glue(address_space_lduw_internal, 
SUFFIX)(ARG1_DECL,
 release_lock |= prepare_mmio_access(mr);

 /* I/O case */
-r = memory_region_dispatch_read(mr, addr1, , 2, attrs);
+r = memory_region_dispatch_read(mr, addr1, , size_memop(2), attrs);
 #if defined(TARGET_WORDS_BIGENDIAN)
 if (endian == DEVICE_LITTLE_ENDIAN) {
 val = bswap16(val);
@@ -300,7 +300,7 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
 if (l < 4 || !memory_access_is_direct(mr, true)) {
 release_lock |= prepare_mmio_access(mr);

-r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
+r = memory_region_dispatch_write(mr, addr1, val, size_memop(4), attrs);
 } else {
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
 stl_p(ptr, val);
@@ -346,7 +346,7 @@ static inline void glue(address_space_stl_internal, 
SUFFIX)(ARG1_DECL,
 val = bswap32(val);
 }
 #endif
-r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
+r = memory_region_dispatch_write(mr, addr1, val, size_memop(4), attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -408,7 +408,7 @@ void glue(address_space_stb, SUFFIX)(ARG1_DECL,
 mr = TRANSLATE(addr, , , true, attrs);
 if (!memory_access_is_direct(mr, true)) {
 release_lock |= prepare_mmio_access(mr);
-r = memory_region_dispatch_write(mr, addr1, val, 1, attrs);
+r = memory_region_dispatch_write(mr, addr1, val, size_memop(1), attrs);
 } else {
 /* RAM case */
 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
@@ -451,7 +451,7 @@ static inline void glue(address_space_stw_internal, 
SUFFIX)(ARG1_DECL,
 val = bswap16(val);
 }
 #endif
-r = memory_region_dispatch_write(mr,