Re: -drive if=none: can't we make this the default?

2023-10-14 Thread BALATON Zoltan

On Sat, 14 Oct 2023, Michael Tokarev wrote:

Can't we make -drive if=none the default?

Yes, I know current default is ide, and whole world have to use if=none 
explicitly
to undo this.  I think at this point we can deprecate if=ide default and 
switch to

if=none in the next release.  I think it will be a welcome change.


I don't think that would be welcome by all people now using -drive 
media=disk,format-raw,file= shortcut (which they were forced to use 
instead of the previous -hda shortcut just to specify format=raw) and now 
they would need to change that to -drive lot,of,options,here -device 
ide-hd,drive=[what was it called and which drive is that?] instead. That's 
a lot more typing and much less convenient so please keep the convenience 
option at least somewhat convenient for command line users.


Also machine class has a member (possibly) set by board code: 
mc->block_default_type = IF_IDE; Isn't that the default used if no "if" 
property given?


Maybe I don't understand how this works but if it makes less convenient 
for users and breaks their scripts then I don't think it's a good idea.


Regards,
BALATON Zoltan



-drive if=none: can't we make this the default?

2023-10-14 Thread Michael Tokarev

Can't we make -drive if=none the default?

Yes, I know current default is ide, and whole world have to use if=none 
explicitly
to undo this.  I think at this point we can deprecate if=ide default and switch 
to
if=none in the next release.  I think it will be a welcome change.

/mjt



Re: [PATCH 1/1] block: improve alignment detection and fix 271 test

2023-10-14 Thread Nir Soffer
On Fri, Sep 8, 2023 at 12:54 AM Denis V. Lunev  wrote:

> Unfortunately 271 IO test is broken if started in non-cached mode.
>

Is this a real world issue? For example in oVirt you cannot create a disk
with
size < 4k so there is no way that 4k is not a good alignment.

Should we fix the test to reflect real world usage?

_reset_img 2083k

I guess it works with:

_reset_img 2084k

Commits
> commit a6b257a08e3d72219f03e461a52152672fec0612
> Author: Nir Soffer 
> Date:   Tue Aug 13 21:21:03 2019 +0300
> file-posix: Handle undetectable alignment
> and
> commit 9c60a5d1978e6dcf85c0e01b50e6f7f54ca09104
> Author: Kevin Wolf 
> Date:   Thu Jul 16 16:26:00 2020 +0200
> block: Require aligned image size to avoid assertion failure
> have interesting side effect if used togather.
>
> If the image size is not multiple of 4k and that image falls under
> original constraints of Nil's patch, the image can not be opened
> due to the check in the bdrv_check_perm().
>
> The patch tries to satisfy the requirements of bdrv_check_perm()
> inside raw_probe_alignment(). This is at my opinion better that just
> disallowing to run that test in non-cached mode. The operation is legal
> by itself.
>
> Signed-off-by: Denis V. Lunev 
> CC: Nir Soffer 
> CC: Kevin Wolf 
> CC: Hanna Reitz 
> CC: Alberto Garcia 
> ---
>  block/file-posix.c | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index b16e9c21a1..988cfdc76c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -447,8 +447,21 @@ static void raw_probe_alignment(BlockDriverState *bs,
> int fd, Error **errp)
>  for (i = 0; i < ARRAY_SIZE(alignments); i++) {
>  align = alignments[i];
>  if (raw_is_io_aligned(fd, buf, align)) {
> -/* Fallback to safe value. */
> -bs->bl.request_alignment = (align != 1) ? align :
> max_align;
> +if (align != 1) {
> +bs->bl.request_alignment = align;
> +break;
> +}
> +/*
> + * Fallback to safe value. max_align is perfect, but the
> size of the device must be multiple of
> + * the virtual length of the device. In the other case we
> will get a error in
> + * bdrv_node_refresh_perm().
> + */
> +for (align = max_align; align > 1; align /= 2) {
> +if ((bs->total_sectors * BDRV_SECTOR_SIZE) % align ==
> 0) {
>

Moving image size calculation out of the loop would make the intent of the
code
more clear:

if (image_size % align == 0) {

Since qemu does not enforce image size alignment, I can see how you create
a 512 bytes
aligned image and in the case when qemu cannot detect the alignment, we end
with
align = 4k. In this case this loop would select align = 512, but with the
image aligned to
some strange value, this loop may select align = 2 or some other value that
does not
make sense.

So I can see using 4k or 512 bytes as a good fallback value, but anything
else should not
be possible, so maybe we should fix this in bdrv_check_perm()?

Nir


Re: [PATCH v2 1/2] qdev: add IOThreadVirtQueueMappingList property type

2023-10-14 Thread Markus Armbruster
Stefan Hajnoczi  writes:

> virtio-blk and virtio-scsi devices will need a way to specify the
> mapping between IOThreads and virtqueues. At the moment all virtqueues
> are assigned to a single IOThread or the main loop. This single thread
> can be a CPU bottleneck, so it is necessary to allow finer-grained
> assignment to spread the load.
>
> Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a
> parameter that maps virtqueues to IOThreads. The command-line syntax for
> this new property is as follows:
>
>   --device 
> '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}'
>
> IOThreads are specified by name and virtqueues are specified by 0-based
> index.
>
> It will be common to simply assign virtqueues round-robin across a set
> of IOThreads. A convenient syntax that does not require specifying
> individual virtqueue indices is available:
>
>   --device 
> '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}'
>
> Signed-off-by: Stefan Hajnoczi 
> ---
>  qapi/virtio.json| 30 ++
>  include/hw/qdev-properties-system.h |  4 +++
>  hw/core/qdev-properties-system.c| 47 +
>  3 files changed, 81 insertions(+)
>
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index e6dcee7b83..cb341ae596 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -928,3 +928,33 @@
>'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' },
>'returns': 'VirtioQueueElement',
>'features': [ 'unstable' ] }
> +
> +##
> +# @IOThreadVirtQueueMapping:
> +#
> +# Describes the subset of virtqueues assigned to an IOThread.
> +#
> +# @iothread: the id of IOThread object
> +# @vqs: an optional array of virtqueue indices that will be handled by this
> +#   IOThread. When absent, virtqueues are assigned round-robin across all
> +#   IOThreadVirtQueueMappings provided. Either all
> +#   IOThreadVirtQueueMappings must have @vqs or none of them must have 
> it.
> +#
> +# Since: 8.2
> +#
> +##

Please format like

   ##
   # @IOThreadVirtQueueMapping:
   #
   # Describes the subset of virtqueues assigned to an IOThread.
   #
   # @iothread: the id of IOThread object
   #
   # @vqs: an optional array of virtqueue indices that will be handled by
   # this IOThread.  When absent, virtqueues are assigned round-robin
   # across all IOThreadVirtQueueMappings provided.  Either all
   # IOThreadVirtQueueMappings must have @vqs or none of them must
   # have it.
   #
   # Since: 8.2
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

> +
> +{ 'struct': 'IOThreadVirtQueueMapping',
> +  'data': { 'iothread': 'str', '*vqs': ['uint16'] } }
> +
> +##
> +# @IOThreadVirtQueueMappings:
> +#
> +# IOThreadVirtQueueMapping list. This struct is not actually used but the
> +# IOThreadVirtQueueMappingList type it generates is!

Two spaces between sentences for consistency, please.

Doc comments are QMP reference documentation for users.  Does this
paragraph belong there?

> +#
> +# Since: 8.2
> +##
> +
> +{ 'struct': 'IOThreadVirtQueueMappings',
> +  'data': { 'mappings': ['IOThreadVirtQueueMapping'] } }
> diff --git a/include/hw/qdev-properties-system.h 
> b/include/hw/qdev-properties-system.h
> index 0ac327ae60..c526e502c8 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev;
>  extern const PropertyInfo qdev_prop_off_auto_pcibar;
>  extern const PropertyInfo qdev_prop_pcie_link_speed;
>  extern const PropertyInfo qdev_prop_pcie_link_width;
> +extern const PropertyInfo qdev_prop_iothread_vq_mapping_list;
>  
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)   \
>  DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
> @@ -73,5 +74,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>  #define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) \
>  DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID)
>  
> +#define DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST(_name, _state, _field) \
> +DEFINE_PROP(_name, _state, _field, qdev_prop_iothread_vq_mapping_list, \
> +IOThreadVirtQueueMappingList *)
>  
>  #endif
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 6d5d43eda2..831796e106 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -18,6 +18,7 @@
>  #include "qapi/qapi-types-block.h"
>  #include "qapi/qapi-types-machine.h"
>  #include "qapi/qapi-types-migration.h"
> +#include "qapi/qapi-visit-virtio.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ctype.h"
>  #include "qemu/cutils.h"
> @@ -1147,3 +1148,49 @@ const PropertyInfo qdev_prop_uuid = {
>  .set   = set_uuid,
>  .set_default_value = set_default_uuid_auto,
>  };
> +
> +/* --- IOT