Re: [Qemu-block] [Qemu-devel] [PATCH 06/10][TRIVIAL] macio-ide: add to storage category

[Thomas Huth]

On 26/09/15 18:22, Laurent Vivier wrote:
> macio-ide is an IDE controller, so add it
> to the storage category.
> 
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
>  hw/ide/macio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 66ac2ba..893c9b9 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -590,6 +590,7 @@ static void macio_ide_class_init(ObjectClass *oc, void 
> *data)
>  dc->realize = macio_ide_realizefn;
>  dc->reset = macio_ide_reset;
>  dc->vmsd = _pmac;
> +set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH 01/10][TRIVIAL] adb: add to input category

[Thomas Huth]

On 26/09/15 18:22, Laurent Vivier wrote:
> The Apple Desktop Bus is used to connect a keyboard and a mouse,
> so add it to the input category.
> 
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
>  hw/input/adb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/input/adb.c b/hw/input/adb.c
> index a18eea2..09eead9 100644
> --- a/hw/input/adb.c
> +++ b/hw/input/adb.c
> @@ -362,6 +362,7 @@ static void adb_kbd_class_init(ObjectClass *oc, void 
> *data)
>  
>  akc->parent_realize = dc->realize;
>  dc->realize = adb_kbd_realizefn;
> +set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  
>  adc->devreq = adb_kbd_request;
>  dc->reset = adb_kbd_reset;
> @@ -566,6 +567,7 @@ static void adb_mouse_class_init(ObjectClass *oc, void 
> *data)
>  
>  amc->parent_realize = dc->realize;
>  dc->realize = adb_mouse_realizefn;
> +set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  
>  adc->devreq = adb_mouse_request;
>  dc->reset = adb_mouse_reset;

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH 03/10][TRIVIAL] escc: add to input category

[Thomas Huth]

On 26/09/15 18:22, Laurent Vivier wrote:
> ESCC is a serial port controller, so add it
> to the input category.
> 
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
>  hw/char/escc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index ba653ef..9816154 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -1035,6 +1035,7 @@ static void escc_class_init(ObjectClass *klass, void 
> *data)
>  dc->reset = escc_reset;
>  dc->vmsd = _escc;
>  dc->props = escc_properties;
> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH 04/10][TRIVIAL] grackle: add to bridge category

[Thomas Huth]

On 26/09/15 18:22, Laurent Vivier wrote:
> Grackle is the PCI host controller of oldworld powermac,
> so add it to the bridge category.
> 
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
>  hw/pci-host/grackle.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index bfe707a..ea31b72 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -146,8 +146,10 @@ static const TypeInfo grackle_pci_info = {
>  static void pci_grackle_class_init(ObjectClass *klass, void *data)
>  {
>  SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +DeviceClass *dc = DEVICE_CLASS(klass);
>  
>  k->init = pci_grackle_init_device;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>  }

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH 10/10][TRIVIAL] openpic: add to misc category

[Thomas Huth]

On 26/09/15 18:22, Laurent Vivier wrote:
> openpic is a programmable interrupt controller, so
> add it to the misc category.
> 
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
>  hw/intc/openpic.c | 1 +
>  hw/intc/openpic_kvm.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 14ab0e3..bfcf155 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1643,6 +1643,7 @@ static void openpic_class_init(ObjectClass *oc, void 
> *data)
>  dc->props = openpic_properties;
>  dc->reset = openpic_reset;
>  dc->vmsd = _openpic;
> +set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo openpic_info = {
> diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
> index f7cac58..649f476 100644
> --- a/hw/intc/openpic_kvm.c
> +++ b/hw/intc/openpic_kvm.c
> @@ -275,6 +275,7 @@ static void kvm_openpic_class_init(ObjectClass *oc, void 
> *data)
>  dc->realize = kvm_openpic_realize;
>  dc->props = kvm_openpic_properties;
>  dc->reset = kvm_openpic_reset;
> +set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH 09/10][TRIVIAL] macio-nvram: add to misc category

[Thomas Huth]

On 26/09/15 18:22, Laurent Vivier wrote:
> The macio nvram is a non volatile RAM, so add it
> the misc category.
> 
> Signed-off-by: Laurent Vivier <laur...@vivier.eu>
> ---
>  hw/nvram/mac_nvram.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
> index d35f8a3..9f16566 100644
> --- a/hw/nvram/mac_nvram.c
> +++ b/hw/nvram/mac_nvram.c
> @@ -123,6 +123,7 @@ static void macio_nvram_class_init(ObjectClass *oc, void 
> *data)
>  dc->reset = macio_nvram_reset;
>  dc->vmsd = _macio_nvram;
>  dc->props = macio_nvram_properties;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }

Reviewed-by: Thomas Huth <th...@redhat.com>

[Qemu-block] [PATCH] doc: Fix mailing list address in tests/qemu-iotests/README

[Thomas Huth]

The address of the mailing list is qemu-de...@nongnu.org
instead of qemu-de...@savannah.nongnu.org. And while we're
at it, also mention the qemu-block mailing list here.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 tests/qemu-iotests/README | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
index 4ccfdd1..6079b40 100644
--- a/tests/qemu-iotests/README
+++ b/tests/qemu-iotests/README
@@ -17,4 +17,5 @@ additional options to test further image formats or I/O 
methods.
 * Feedback and patches
 
 Please send improvements to the test suite, general feedback or just
-reports of failing tests cases to qemu-de...@savannah.nongnu.org.
+reports of failing tests cases to qemu-de...@nongnu.org with a CC:
+to qemu-block@nongnu.org.
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> Block backends defined with -drive if=ide are meant to be picked up by
> machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=ide drives not picked up that way can still be used with -device as
> if they had if=none, but that's unclean and best avoided.  Unused ones
> produce an "Orphaned drive without device" warning.
> 
> -drive parameter "if" is optional, and the default depends on the
> machine type.  If a machine type doesn't specify a default, the
> default is "ide".
> 
> Many machine types implicitly default to if=ide that way, even though
> they don't actually have an IDE controller.  This makes no sense.
> 
> Change the implicit default to if=none.  Affected machines:
> 
> * all targets: none
> * aarch64/arm: akita ast2500 canon cheetah collie connex imx25
>   integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
>   netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
>   verdex z2
> * cris: axis-dev88
> * i386/x86_64: xenpv
> * lm32: lm32-evr lm32-uclinux milkymist
> * m68k: an5206 dummy mcf5208evb
> * microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
> * mips/mips64/mips64el/mipsel: mipssim
> * moxie: moxiesim
> * or32: or32-sim
> * ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
> * ppc/ppc64: mpc8544ds ppce500
> * sh4/sh4eb: shix
> * sparc: leon3_generic
> * sparc64: niagara
> * tricore: tricore_testboard
> * unicore32: puv3
> * xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim
> 
> None of these machines have an IDE controller, let alone code to
> honor if=ide.
> 
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: qemu-...@nongnu.org
> Cc: Edgar E. Iglesias <edgar.igles...@gmail.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>
> Cc: Anthony Perard <anthony.per...@citrix.com>
> Cc: xen-de...@lists.xensource.com
> Cc: Michael Walle <mich...@walle.cc>
> Cc: Laurent Vivier <laur...@vivier.eu>
> Cc: Anthony Green <gr...@moxielogic.com>
> Cc: Jia Liu <pro...@gmail.com>
> Cc: Alexander Graf <ag...@suse.de>
> Cc: qemu-...@nongnu.org
> Cc: Magnus Damm <magnus.d...@gmail.com>
> Cc: Fabien Chouteau <chout...@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4q...@gmail.com>
> Cc: Bastian Koppelmann <kbast...@mail.uni-paderborn.de>
> Cc: Guan Xuetao <g...@mprc.pku.edu.cn>
> Cc: Max Filippov <jcmvb...@gmail.com>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> Acked-By: Artyom Tarasenko <atar4q...@gmail.com>
> ---
>  include/sysemu/blockdev.h | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 16432f3..351a039 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
>  typedef enum {
>  IF_DEFAULT = -1,/* for use with drive_add() only */
>  /*
> - * IF_IDE must be zero, because we want MachineClass member
> - * block_default_type to default-initialize to IF_IDE
> + * IF_NONE must be zero, because we want MachineClass member
> + * block_default_type to default-initialize to IF_NONE
>   */
> -IF_IDE = 0,
> -IF_NONE,
> -IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +IF_NONE = 0,
> +IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
>  IF_COUNT
>  } BlockInterfaceType;
>  
> 

Makes sense.

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/8] hw: Default -drive to if=none instead of scsi when scsi cannot work

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> Block backends defined with -drive if=scsi are meant to be picked up
> by machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=scsi drives not picked up that way can still be used with -device
> as if they had if=none, but that's unclean and best avoided.  Unused
> ones produce an "Orphaned drive without device" warning.
> 
> A few machine types default to if=scsi, even though they don't
> actually have a SCSI HBA.  This makes no sense.  Change their default
> to if=none.  Affected machines:
> 
> * aarch64/arm: realview-pbx-a9 vexpress-a9 vexpress-a15 xilinx-zynq-a9
> 
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: "Edgar E. Iglesias" <edgar.igles...@gmail.com>
> Cc: Alistair Francis <alistair.fran...@xilinx.com>
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
>  hw/arm/realview.c| 1 -
>  hw/arm/vexpress.c| 1 -
>  hw/arm/xilinx_zynq.c | 1 -
>  3 files changed, 3 deletions(-)
> 
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 8eafcca..8c11c7a 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -443,7 +443,6 @@ static void realview_pbx_a9_class_init(ObjectClass *oc, 
> void *data)
>  
>  mc->desc = "ARM RealView Platform Baseboard Explore for Cortex-A9";
>  mc->init = realview_pbx_a9_init;
> -mc->block_default_type = IF_SCSI;
>  mc->max_cpus = 4;
>  }
>  
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 58760f4..5756470 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -751,7 +751,6 @@ static void vexpress_class_init(ObjectClass *oc, void 
> *data)
>  
>  mc->desc = "ARM Versatile Express";
>  mc->init = vexpress_common_init;
> -mc->block_default_type = IF_SCSI;
>  mc->max_cpus = 4;
>  }
>  
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 7dac20d..3985356 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -323,7 +323,6 @@ static void zynq_machine_init(MachineClass *mc)
>  {
>  mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
>  mc->init = zynq_init;
> -mc->block_default_type = IF_SCSI;
>  mc->max_cpus = 1;
>  mc->no_sdcard = 1;
>  }

Right, that looks like old copy-n-paste bugs from other machine types.

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH v2 5/8] hw/arm/highbank: Default -drive to if=ide instead of if=scsi

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> These machines have no onboard SCSI HBA, and no way to plug one.
> -drive if=scsi therefore cannot work.  They do have an onboard IDE
> controller (sysbus-ahci), but fail to honor if=ide.
> 
> Change their default to if=ide, and add a TODO comment on what needs
> to be done to actually honor -drive if=ide.
> 
> Cc: Rob Herring <r...@kernel.org>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: qemu-...@nongnu.org
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
>  hw/arm/highbank.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 80e5fd4..0a4508c 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -363,6 +363,8 @@ static void calxeda_init(MachineState *machine, enum 
> cxmachines machine_id)
>  sysbus_connect_irq(SYS_BUS_DEVICE(dev), 2, pic[82]);
>  }
>  
> +/* TODO create and connect IDE devices for ide_drive_get() */
> +
>  highbank_binfo.ram_size = ram_size;
>  highbank_binfo.kernel_filename = kernel_filename;
>  highbank_binfo.kernel_cmdline = kernel_cmdline;
> @@ -405,7 +407,8 @@ static void highbank_class_init(ObjectClass *oc, void 
> *data)
>  
>  mc->desc = "Calxeda Highbank (ECX-1000)";
>  mc->init = highbank_init;
> -mc->block_default_type = IF_SCSI;
> +mc->block_default_type = IF_IDE;
> +mc->units_per_default_bus = 1;
>  mc->max_cpus = 4;
>  }
>  
> @@ -421,7 +424,8 @@ static void midway_class_init(ObjectClass *oc, void *data)
>  
>  mc->desc = "Calxeda Midway (ECX-2000)";
>  mc->init = midway_init;
> -mc->block_default_type = IF_SCSI;
> +mc->block_default_type = IF_IDE;
> +mc->units_per_default_bus = 1;
>  mc->max_cpus = 4;
>  }
>  
> 

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] MIPS machines (was: [PATCH v2 1/8] hw: Default -drive to if=ide explicitly where it works)

[Thomas Huth]

On 27.01.2017 11:21, Yongbok Kim wrote:
> 
>>> Slightly off-topic, but: Is fulong2e still maintained? I did not spot an
>>> entry in MAINTAINERS...?
>>
>> It's covered by the general MIPS stanza:
>>
>> $ scripts/get_maintainer.pl -f hw/mips/mips_fulong2e.c 
>> Aurelien Jarno  (maintainer:MIPS)
>> Yongbok Kim  (maintainer:MIPS)
>> qemu-de...@nongnu.org (open list:All patches CC here)
>>
> 
> I'm not actively looking after the device at the moment but if it has any
> issues I love to handle that.

Great! Then could you maybe send a patch for the MAINTAINERS file to add
an entry for that machine?

Also it's a little bit confusing that "magnum" and "pica61" do not show
up in MAINTAINERS, but I guess that's what is meant by the "Jazz" entry?

 Thanks,
  Thomas

Re: [Qemu-block] [Qemu-devel] regarding bug name : -hda FAT:. limited to 504MBytes

[Thomas Huth]

 Hi,

On 14.02.2017 22:38, Shubham Kumar wrote:
> Since the problem seems like the used FAT-16 file system , 
> Will it solve the problem if I change the code of  vvfat.c for FAT-32 file 
> system to increase acceptable file size ?

As far as I know, FAT16 can already support up to 4GB file systems, so
that limitation to 504 MB must be something different.
It's maybe best if you put the qemu-block mailing list on CC:, to get
the attention of the block/disk expert, too. Maybe there's someone
around who knows the vvfat code and its limitations and can give a good
answer here...

 Thomas

[Qemu-block] [PATCH] qemu-options: Fix broken sheepdog URL

[Thomas Huth]

The sheepdog URL is broken twice: First it uses a duplicated
http:// prefix, second the website seems to have moved to
https://sheepdog.github.io/sheepdog/ instead.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 qemu-options.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index ad2f8fc..38113b9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2583,7 +2583,7 @@ Example
 qemu-system-i386 --drive file=sheepdog://192.0.2.1:3/MyVirtualMachine
 @end example
 
-See also @url{http://http://www.osrg.net/sheepdog/}.
+See also @url{https://sheepdog.github.io/sheepdog/}.
 
 @item GlusterFS
 GlusterFS is a user space distributed file system.
-- 
1.8.3.1

Re: [Qemu-block] [PATCH] option: Tweak invalid size error message and unbreak iotest 049

[Thomas Huth]

On 27.02.2017 13:55, Markus Armbruster wrote:
> Commit 75cdcd1 neglected to update tests/qemu-iotests/049.out, and
> made the error message for negative size worse.  Fix that.
> 
> Reported-by: Thomas Huth <th...@redhat.com>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
>  tests/qemu-iotests/049.out | 14 +-
>  util/qemu-option.c |  2 +-
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
> index 4673b67..34e66db 100644
> --- a/tests/qemu-iotests/049.out
> +++ b/tests/qemu-iotests/049.out
> @@ -95,14 +95,14 @@ qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
>  qemu-img: Image size must be less than 8 EiB!
>  
>  qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2
> -qemu-img: Parameter 'size' expects a non-negative number below 2^64
> +qemu-img: Value '-1024' is out of range for parameter 'size'
>  qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
>  
>  qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k
>  qemu-img: Image size must be less than 8 EiB!
>  
>  qemu-img create -f qcow2 -o size=-1k TEST_DIR/t.qcow2
> -qemu-img: Parameter 'size' expects a non-negative number below 2^64
> +qemu-img: Value '-1k' is out of range for parameter 'size'
>  qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
>  
>  qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
> @@ -110,15 +110,19 @@ qemu-img: Invalid image size specified! You may use k, 
> M, G, T, P or E suffixes
>  qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
>  
>  qemu-img create -f qcow2 -o size=1kilobyte TEST_DIR/t.qcow2
> -Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 encryption=off 
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +qemu-img: Parameter 'size' expects a non-negative number below 2^64
> +Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta-
> +and exabytes, respectively.
> +qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
>  
>  qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- foobar
>  qemu-img: Invalid image size specified! You may use k, M, G, T, P or E 
> suffixes for
>  qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
>  
>  qemu-img create -f qcow2 -o size=foobar TEST_DIR/t.qcow2
> -qemu-img: Parameter 'size' expects a size
> -You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and 
> terabytes.
> +qemu-img: Parameter 'size' expects a non-negative number below 2^64
> +Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta-
> +and exabytes, respectively.
>  qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
>  
>  == Check correct interpretation of suffixes for cluster size ==
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 419f252..5ce1b5c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -179,7 +179,7 @@ void parse_option_size(const char *name, const char 
> *value,
>  
>  err = qemu_strtosz(value, NULL, );
>  if (err == -ERANGE) {
> -error_setg(errp, "Value '%s' is too large for parameter '%s'",
> +error_setg(errp, "Value '%s' is out of range for parameter '%s'",
> value, name);
>  return;
>  }

Reviewed-by: Thomas Huth <th...@redhat.com>

[Qemu-block] [PATCH] virtio-blk: Remove hw/virtio/dataplane folder from MAINTAINERS file

[Thomas Huth]

The folder does not exist anymore, thus should be removed from the
MAINTAINERS file, too.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index b6fb84e..ff45f8c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -902,7 +902,6 @@ L: qemu-block@nongnu.org
 S: Supported
 F: hw/block/virtio-blk.c
 F: hw/block/dataplane/*
-F: hw/virtio/dataplane/*
 T: git git://github.com/stefanha/qemu.git block
 
 virtio-ccw
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: Remove hw/virtio/dataplane folder from MAINTAINERS file

[Thomas Huth]

On 05.09.2016 13:05, Markus Armbruster wrote:
> Thomas Huth <th...@redhat.com> writes:
> 
>> On 05.09.2016 10:22, Markus Armbruster wrote:
>>> Thomas Huth <th...@redhat.com> writes:
>>>
>>>> The folder does not exist anymore, thus should be removed from the
>>>> MAINTAINERS file, too.
>>>>
>>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>>> ---
>>>>  MAINTAINERS | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index b6fb84e..ff45f8c 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -902,7 +902,6 @@ L: qemu-block@nongnu.org
>>>>  S: Supported
>>>>  F: hw/block/virtio-blk.c
>>>>  F: hw/block/dataplane/*
>>>> -F: hw/virtio/dataplane/*
>>>>  T: git git://github.com/stefanha/qemu.git block
>>>>  
>>>
>>> Are there any other patterns in MAINTAINERS that don't match anything?
>>
>> for i in `grep "^[FX]: " MAINTAINERS | sed "s/^.: //"` ; do \
>>   if [ ! -e "$i" ]; then echo "$i" ; fi ; \
>> done
>>
>> include/hw/xilinx.h
> 
> Gone since commit d5001cf.
> 
>> include/hw/*/xlnx*.c
> 
> Bug, should be .h.
> 
>> include/hw/acpi/piix.h
> 
> Bug, should be piix4.h.
> 
>> hw/i386/*dsl
>> scripts/acpi*py
> 
> Gone since commit 9fc6502.
> 
>> hw/virtio/dataplane/*
> 
> Gone since commit fee089e.
> 
>> include/hw/cpu/icc_bus.h
>> hw/cpu/icc_bus.c
> 
> Gone since commit dfeb867.
> 
>> block/raw-aio.h
> 
> Moved to include/block/raw-aio.h in commit 0187f5c.
> 
>>  Thomas
> 
> Let's fix all this.  You or I?

I don't mind. I'm currently busy hunting some bugs ... So feel free to
send the patches for these issues. (and in case you also don't have time
for that now, I've also put an entry on my TODO list, so I could do that
later if you prefer that)

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: Remove hw/virtio/dataplane folder from MAINTAINERS file

[Thomas Huth]

On 05.09.2016 10:22, Markus Armbruster wrote:
> Thomas Huth <th...@redhat.com> writes:
> 
>> The folder does not exist anymore, thus should be removed from the
>> MAINTAINERS file, too.
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>> ---
>>  MAINTAINERS | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b6fb84e..ff45f8c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -902,7 +902,6 @@ L: qemu-block@nongnu.org
>>  S: Supported
>>  F: hw/block/virtio-blk.c
>>  F: hw/block/dataplane/*
>> -F: hw/virtio/dataplane/*
>>  T: git git://github.com/stefanha/qemu.git block
>>  
> 
> Are there any other patterns in MAINTAINERS that don't match anything?

for i in `grep "^[FX]: " MAINTAINERS | sed "s/^.: //"` ; do \
  if [ ! -e "$i" ]; then echo "$i" ; fi ; \
done

include/hw/xilinx.h
include/hw/*/xlnx*.c
include/hw/acpi/piix.h
hw/i386/*dsl
scripts/acpi*py
hw/virtio/dataplane/*
include/hw/cpu/icc_bus.h
hw/cpu/icc_bus.c
block/raw-aio.h

 Thomas

[Qemu-block] [PATCH] Put the copyright information on a separate line

[Thomas Huth]

The output string QEMU with "--version" is very long, it does
not fit into a normal line of a terminal window anymore. By
putting the copyright information on a separate line instead,
the output looks much nicer.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 Note: I'm not sure whether there is a technical or legal reason
 that the copyright information has to be on the same line as the
 version information, but if there is no such reason, I think
 a separate line looks much nicer here.

 bsd-user/main.c   | 2 +-
 linux-user/main.c | 2 +-
 qemu-img.c| 2 +-
 vl.c  | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index d803d3e..cd8ba64 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -651,7 +651,7 @@ void cpu_loop(CPUSPARCState *env)
 static void usage(void)
 {
 printf("qemu-" TARGET_NAME " version " QEMU_VERSION QEMU_PKGVERSION
-   ", " QEMU_COPYRIGHT "\n"
+   "\n" QEMU_COPYRIGHT "\n"
"usage: qemu-" TARGET_NAME " [options] program [arguments...]\n"
"BSD CPU emulator (compiled for %s emulation)\n"
"\n"
diff --git a/linux-user/main.c b/linux-user/main.c
index 9e4b430..1be24d9 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3936,7 +3936,7 @@ static void handle_arg_strace(const char *arg)
 static void handle_arg_version(const char *arg)
 {
 printf("qemu-" TARGET_NAME " version " QEMU_VERSION QEMU_PKGVERSION
-   ", " QEMU_COPYRIGHT "\n");
+   "\n" QEMU_COPYRIGHT "\n");
 exit(EXIT_SUCCESS);
 }
 
diff --git a/qemu-img.c b/qemu-img.c
index ceffefe..7e05349 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -44,7 +44,7 @@
 #include 
 
 #define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION QEMU_PKGVERSION \
-  ", " QEMU_COPYRIGHT "\n"
+  "\n" QEMU_COPYRIGHT "\n"
 
 typedef struct img_cmd_t {
 const char *name;
diff --git a/vl.c b/vl.c
index f3abd99..4dd6399 100644
--- a/vl.c
+++ b/vl.c
@@ -1954,7 +1954,7 @@ static void main_loop(void)
 
 static void version(void)
 {
-printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION ", "
+printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n"
QEMU_COPYRIGHT "\n");
 }
 
-- 
1.8.3.1

[Qemu-block] [PATCH] MAINTAINERS: Add some more headers to the IDE section

[Thomas Huth]

The folder include/hw/ide/ belongs to the IDE section.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d8a0cfc..acf6d6c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -791,6 +791,7 @@ M: John Snow <js...@redhat.com>
 L: qemu-block@nongnu.org
 S: Supported
 F: include/hw/ide.h
+F: include/hw/ide/
 F: hw/ide/
 F: hw/block/block.c
 F: hw/block/cdrom.c
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] MAINTAINERS: Add some more headers to the IDE section

[Thomas Huth]

On 26.09.2016 10:22, Kevin Wolf wrote:
> Am 23.09.2016 um 18:42 hat John Snow geschrieben:
>> On 09/23/2016 12:09 PM, Thomas Huth wrote:
>>> The folder include/hw/ide/ belongs to the IDE section.
>>>
>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>> ---
>>> MAINTAINERS | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index d8a0cfc..acf6d6c 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -791,6 +791,7 @@ M: John Snow <js...@redhat.com>
>>> L: qemu-block@nongnu.org
>>> S: Supported
>>> F: include/hw/ide.h
>>> +F: include/hw/ide/
>>> F: hw/ide/
>>> F: hw/block/block.c
>>> F: hw/block/cdrom.c
>>>
>>
>> Ah, yeah. These got missed when they were moved over. Thanks.
>>
>> Reviewed-by: John Snow <js...@redhat.com>
> 
> Who is supposed to merge this if you only give an R-b?

I've CC:ed this patch to qemu-trivial, so I hope it will get picked up
there if John does not want to apply this directly.

 Thomas




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] MAINTAINERS: Add Fam and Jsnow for Bitmap support

[Thomas Huth]

On 07.11.2016 17:40, Max Reitz wrote:
> On 04.08.2016 20:18, John Snow wrote:
>> These files are currently unmaintained.
>>
>> I'm proposing that Fam and I co-maintain them; under the model that
>> whomever between us isn't authoring a given series will be responsible
>> for reviewing it.
>>
>> Signed-off-by: John Snow 
>> ---
>>  MAINTAINERS | 14 ++
>>  1 file changed, 14 insertions(+)
> 
> Ping, anyone?

I'm currently gathering a set of my patches that updates the MAINTAINERS
file - and Paolo asked me to send a PULL request for that one, so if you
like, I can also include this patch there.

 Thomas




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] (no subject)

[Thomas Huth]

 Hi Christopher,

On 16.11.2016 20:41, Christopher Oliver wrote:
> This patch (hack?) works around the slowness in SEEK_HOLE for large dense 
> files
> on Linux tmpfs.  It may improve life elsewhere as well, and the penalty of 
> the checks
> should be vanishingly small where it is not needed.
> 
> If I'm subtly (or not so subtly) wrong, please fire back.

When submitting QEMU patches, there are some rules to be followed:
First, please have a look at
 http://qemu-project.org/Contribute/SubmitAPatch#Do_not_send_as_an_attachment
and the other paragraphs there.
Your mail should also have a proper subject, and the Signed-off-by line
should contain your e-mail address.

 Thanks,
  Thomas

[Qemu-block] [PATCH] hw/block/nvme: Simplify if-statements a little bit

[Thomas Huth]

The condition  '!A || (A && B)' is equivalent to '!A || B'.

Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
Signed-off-by: Thomas Huth <th...@redhat.com>
---
 hw/block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cef3bb4..53d9d2e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
 if (!cqid || nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
-if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
+if (!sqid || !nvme_check_sqid(n, sqid)) {
 return NVME_INVALID_QID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
@@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
 uint16_t qflags = le16_to_cpu(c->cq_flags);
 uint64_t prp1 = le64_to_cpu(c->prp1);
 
-if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
+if (!cqid || !nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.9 5/5] MAINTAINERS: Add myself for files I touched recently

[Thomas Huth]

On 21.03.2017 08:17, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> On 03/20/2017 07:55 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>>  MAINTAINERS | 11 +++
>>>  1 file changed, 11 insertions(+)
>>
>> Reviewed-by: Eric Blake 
>>
>> By the way, where do we stand on the idea of having checkpatch.pl reject
>> patches that introduce new files without mentioning a maintainer?
> 
> Stuck.  Thomas hasn't followed up on his RFC PATCH because he's afraid
> of false positives.  I encouraged him to rescue at least "[RFC PATCH
> 4/5] checkpatch: emit a reminder about MAINTAINERS on file
> add/move/delete", and I'm now encouraging him again.

Well, the patch series is out there, and there haven't been any (valid)
requests to rework it, so if you like, feel free to merge it:

https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05740.html

But as mentioned here:

 https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05753.html

there will be quite a bunch of false-positives. So we'd either have to
live with those or we have to come up with a smarter approach to handle
this issue (e.g. by running get_maintainers.pl on the affected files).

 Thomas

Re: [Qemu-block] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Thomas Huth]

On 04.04.2017 08:53, Alexander Graf wrote:
> 
> 
> On 03.04.17 23:00, Eduardo Habkost wrote:
>> On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 03.04.17 22:10, Eduardo Habkost wrote:
 On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> On 1 April 2017 at 01:46, Eduardo Habkost  wrote:
>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
>> all kinds of untested devices available to -device and
>> device_add.
>>
>> The problem with that is: setting has_dynamic_sysbus on a
>> machine-type lets it accept all the 288 sysbus device types we
>> have in QEMU, and most of them were never meant to be used with
>> -device. That's a lot of untested code.
>>
>> Fortunately today we have just a few has_dynamic_sysbus=1
>> machines: virt, pc-q35-*, ppce500, and spapr.
>>
>> virt, ppce500, and spapr have extra checks to ensure just a few
>> device types can be instantiated:
>>
>> * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
>> * ppce500 supports only TYPE_ETSEC_COMMON.
>> * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
>>
>> q35 has no code to block unsupported sysbus devices, however, and
>> accepts all device types. Fortunately, only the following 20
>> device types are compiled into the qemu-system-x86_64 and
>> qemu-system-i386 binaries:
>>
>> * allwinner-ahci
>> * amd-iommu
>> * cfi.pflash01
>> * esp
>> * fw_cfg_io
>> * fw_cfg_mem
>> * generic-sdhci
>> * hpet
>> * intel-iommu
>> * ioapic
>> * isabus-bridge
>> * kvmclock
>> * kvm-ioapic
>> * kvmvapic
>> * SUNW,fdtwo
>> * sysbus-ahci
>> * sysbus-fdc
>> * sysbus-ohci
>> * unimplemented-device
>> * virtio-mmio
>>
>> Instead of requiring each machine-type with has_dynamic_sysbus=1
>> to implement its own mechanism to block unsupported devices, we
>> can use the user_creatable flag to ensure we won't let the user
>> plug anything that will never work.
>
> How does this work? Which devices can be dynamically
> plugged is machine dependent. You can't dynamically-plug
> an intel-iommu on the ARM virt board, and you can't
> dynamically-plug the vfio-calxeda-xgmac on the spapr
> board, and so on. So I don't see how we can just have
> a flag on the device itself that controls whether
> it can be dynamically plugged.
>
> So I'm definitely coming around to the opinion that
> it's just a bug in the q35 board that it doesn't have
> any device whitelisting, and we should fix that.

 OK, let's assume q35 must implement a whitelist:

 To build that whitelist, we need to be able to know what should
 be in the whitelist, or not. And nobody knew for sure what was
 user-creatable in q35 by accident, and what was really supposed
 to be user-creatable in q35. See the "q35 and sysbus devices"
 thread I started ~2 weeks ago.

 Building a q35 whitelist will be much easier if make
 sys-bus-devices non-user-creatable by default.
>>>
>>> So why are they user creatable in the first place?
>>>
>>> We used to have boards that were dynamic sysbus aware (ppce500, virt)
>>> that
>>> allowed dynamic creation and every other board did not. I don't
>>> remember the
>>> exact mechanism behind it though.
>>>
>>> When did that behavior change? It sounds like a regression somewhere.
>>
>> See patch description:
>>
>> commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
>> cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
>>
>> Note that the commit above is not a regression[1] (because q35
>> didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
>> have cannot_instantiate_with_device_add_yet=false/user_creatable=true
>> by default makes it harder to build the whitelist for q35 (or
>> other machines that will have has_dynamic_sysbus=1 in the
>> future).
> 
> I seem to miss the bigger picture.
> 
> Why would anyone set has_dynamic_sysbus=1 in a board file without an
> explicit whitelist? The whitelist is *not* device specific. It's board
> specific, because your board needs to know how to wire up a device and
> how to expose the fact that it exists to the OS.
> 
> So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
> implementing all of the dynamic sysbus logic, no?

According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was
just introduced for allowing the "intel-iommu" device, so I guess this
is the device that we want to have in a whitelist there?

 Thomas

Re: [Qemu-block] [PATCH v2 05/21] fdc: Remove user_creatable flag from sysbus-fdc & SUNW, fdtwo

[Thomas Huth]

On 04.04.2017 22:24, Eduardo Habkost wrote:
> sysbus-fdc and SUNW,fdtwo devices need IRQs to be wired and mmio
> to be mapped, and can't be used with -device. Unset
> user_creatable on their device classes.
> 
> Cc: John Snow <js...@redhat.com>
> Cc: Kevin Wolf <kw...@redhat.com>
> Cc: Max Reitz <mre...@redhat.com>
> Cc: qemu-block@nongnu.org
> Cc: Thomas Huth <th...@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> ---
> Changes v1 -> v2:
> * Commit message rewrite only
> ---
>  hw/block/fdc.c | 10 --
>  1 file changed, 10 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 3d05565628..a328693d15 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2880,11 +2880,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
> void *data)
>  
>  dc->props = sysbus_fdc_properties;
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -/*
> - * FIXME: Set only because we are not sure yet if this device
> - * will be outside the q35 sysbus whitelist.
> - */
> -dc->user_creatable = true;
>  }
>  
>  static const TypeInfo sysbus_fdc_info = {
> @@ -2911,11 +2906,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, 
> void *data)
>  
>  dc->props = sun4m_fdc_properties;
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -/*
> - * FIXME: Set only because we are not sure yet if this device
> - * will be outside the q35 sysbus whitelist.
> - */
> -dc->user_creatable = true;
>  }
>  
>  static const TypeInfo sun4m_fdc_info = {
> 

Reviewed-by: Thomas Huth <th...@redhat.com>

[Qemu-block] [PATCH v2] Issue a deprecation warning if the user specifies the "-hdachs" option.

[Thomas Huth]

If the user needs to specify the disk geometry, the corresponding
parameters of the "-drive" option should be used instead. "-hdachs"
is considered as deprecated and might be removed soon.

Reviewed-by: Eric Blake <ebl...@redhat.com>
Signed-off-by: Thomas Huth <th...@redhat.com>
---
 v2: Removed the trailing dot from the string as suggested by Eric

 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index 0b4ed52..478cfdd 100644
--- a/vl.c
+++ b/vl.c
@@ -3230,6 +3230,8 @@ int main(int argc, char **argv, char **envp)
 }
 }
 }
+error_report("'-hdachs' is deprecated, please use '-drive"
+ " ...,cyls=c,heads=h,secs=s,trans=t' instead");
 break;
 case QEMU_OPTION_numa:
 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
-- 
1.8.3.1

Re: [Qemu-block] [PATCH] Issue a deprecation warning if the user specifies the "-hdachs" option.

[Thomas Huth]

On 26.04.2017 15:44, Paolo Bonzini wrote:
> 
> 
> On 25/04/2017 10:08, Thomas Huth wrote:
>> If the user needs to specify the disk geometry, the corresponding
>> parameters of the "-drive" option should be used instead. "-hdachs"
>> is considered as deprecated and might be removed soon.
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>> ---
>>  vl.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index 0b4ed52..0980213 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3230,6 +3230,8 @@ int main(int argc, char **argv, char **envp)
>>  }
>>  }
>>  }
>> +error_report("'-hdachs' is deprecated. Please use '-drive"
>> + " ...,cyls=c,heads=h,secs=s,trans=t' 
>> instead.");
>>  break;
>>  case QEMU_OPTION_numa:
>>  opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
>>
> 
> Not even this, which should also be deprecated; please suggest using
> "-device ide-hd" instead.

D'oh, you're right, of course ... I'll send a v3 ...

 Thomas

[Qemu-block] [PATCH v3] Issue a deprecation warning if the user specifies the "-hdachs" option.

[Thomas Huth]

If the user needs to specify the disk geometry, the corresponding
parameters of the "-device ide-hd" option should be used instead.
"-hdachs" is considered as deprecated and might be removed soon.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 v3:
 - Recommend to use the parameters of -device ide-hd instead
 - Also add the deprecation warning to the documentation

 qemu-options.hx | 4 ++--
 vl.c| 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 787b9c3..f68829f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -803,8 +803,8 @@ STEXI
 Force hard disk 0 physical geometry (1 <= @var{c} <= 16383, 1 <=
 @var{h} <= 16, 1 <= @var{s} <= 63) and optionally force the BIOS
 translation mode (@var{t}=none, lba or auto). Usually QEMU can guess
-all those parameters. This option is useful for old MS-DOS disk
-images.
+all those parameters. This option is deprecated, please use
+@code{-device ide-hd,cyls=c,heads=h,secs=s,...} instead.
 ETEXI
 
 DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
diff --git a/vl.c b/vl.c
index f46e070..42d4bce 100644
--- a/vl.c
+++ b/vl.c
@@ -3231,6 +3231,8 @@ int main(int argc, char **argv, char **envp)
 }
 }
 }
+error_report("'-hdachs' is deprecated, please use '-device"
+ " ide-hd,cyls=c,heads=h,secs=s,...' instead");
 break;
 case QEMU_OPTION_numa:
 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
-- 
1.8.3.1

[Qemu-block] [PATCH] Issue a deprecation warning if the user specifies the "-hdachs" option.

[Thomas Huth]

If the user needs to specify the disk geometry, the corresponding
parameters of the "-drive" option should be used instead. "-hdachs"
is considered as deprecated and might be removed soon.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index 0b4ed52..0980213 100644
--- a/vl.c
+++ b/vl.c
@@ -3230,6 +3230,8 @@ int main(int argc, char **argv, char **envp)
 }
 }
 }
+error_report("'-hdachs' is deprecated. Please use '-drive"
+ " ...,cyls=c,heads=h,secs=s,trans=t' instead.");
 break;
 case QEMU_OPTION_numa:
 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
-- 
1.8.3.1

[Qemu-block] Use after free problem somewhere in ahci.c or ich.c code

[Thomas Huth]

 Hi!

Looks like there is a use-after-free problem somewhere in
the ahci.c or ich.c code when trying to add the ich9-ahci
on a old PC machine. Using valgrind, I get:

$ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
==6604== Memcheck, a memory error detector
==6604== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==6604== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==6604== Command: x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
==6604== 
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add ich9-ahci,id=ich9-ahci
==6604== Invalid read of size 8
==6604==at 0x609AB0: object_unparent (object.c:445)
==6604==by 0x4C4478: device_unparent (qdev.c:1095)
==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604==by 0x364311: monitor_read (monitor.c:3905)
==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
free'd
==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==  Block was alloc'd at
==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50094F: ahci_realize (ahci.c:1468)
==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604== 
==6604== Invalid read of size 8
==6604==at 0x60A350: object_finalize_child_property (object.c:1395)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x4C4478: device_unparent (qdev.c:1095)
==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604==by 0x364311: monitor_read (monitor.c:3905)
==6604==  Address 0x15fc5428 is 30,296 bytes inside a block of size 36,288 
free'd
==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==  Block was alloc'd at
==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50094F: ahci_realize (ahci.c:1468)
==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
==6604==by 0x608DCD: property_set_bool 

Re: [Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Thomas Huth]

On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
>>> property
>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
>>> added the
>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>>> property.
>>>
>>> Check for use_acpi_pci_hotplug before calling
>>> acpi_pcihp_device_[un]plug_cb().
[...]
>>> Reported-by: Thomas Huth <th...@redhat.com>
>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>
>> Looks like this is a very old bug, isn't it?
>> Objections to merging this after the release?
> 
> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
> let him decide if it is worth crying wolf :) It's very likely no-one but
> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
> plugging AHCI devices :D

I'm fine if this gets included in 2.11 - it's quite unlikely that a user
tries hot-plug ahci on such an old machine type, I think. But we maybe
should include this in the 2.10.1 stable release, so I'm putting
qemu-stable on CC now.

Anyway, your patch seems to fix the issue for me, thanks!

Tested-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Thomas Huth]

On 23.08.2017 07:40, Thomas Huth wrote:
> On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
>> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
>>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
>>>> property
>>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
>>>> added the
>>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>>>> property.
>>>>
>>>> Check for use_acpi_pci_hotplug before calling
>>>> acpi_pcihp_device_[un]plug_cb().
> [...]
>>>> Reported-by: Thomas Huth <th...@redhat.com>
>>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>>>
>>> Looks like this is a very old bug, isn't it?
>>> Objections to merging this after the release?
>>
>> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
>> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
>> let him decide if it is worth crying wolf :) It's very likely no-one but
>> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
>> plugging AHCI devices :D
> 
> I'm fine if this gets included in 2.11 - it's quite unlikely that a user
> tries hot-plug ahci on such an old machine type, I think. But we maybe
> should include this in the 2.10.1 stable release, so I'm putting
> qemu-stable on CC now.
> 
> Anyway, your patch seems to fix the issue for me, thanks!
> 
> Tested-by: Thomas Huth <th...@redhat.com>

... No, I was too fast here. I'm afraid, it still crashes with mips64el:

$ valgrind mips64el-softmmu/qemu-system-mips64el -S -nographic -M 
malta,accel=qtest
==17935== Memcheck, a memory error detector
==17935== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==17935== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==17935== Command: mips64el-softmmu/qemu-system-mips64el -S -nographic -M 
malta,accel=qtest
==17935== 
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add ich9-ahci
==17935== Invalid read of size 8
==17935==at 0x5F6F10: object_unparent (object.c:445)
==17935==by 0x4BB2C8: device_unparent (qdev.c:1095)
==17935==by 0x5F77C4: object_finalize_child_property (object.c:1396)
==17935==by 0x5F6706: object_property_del_child.isra.7 (object.c:427)
==17935==by 0x448BC8: qdev_device_add (qdev-monitor.c:634)
==17935==by 0x449122: qmp_device_add (qdev-monitor.c:807)
==17935==by 0x462B29: hmp_device_add (hmp.c:1925)
==17935==by 0x370F83: handle_hmp_command (monitor.c:3119)
==17935==by 0x371E59: monitor_command_cb (monitor.c:3922)
==17935==by 0x6D3187: readline_handle_byte (readline.c:393)
==17935==by 0x371211: monitor_read (monitor.c:3905)
==17935==by 0x6699D3: mux_chr_read (char-mux.c:216)
==17935==  Address 0x21c549d8 is 30,328 bytes inside a block of size 36,288 
free'd
==17935==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==17935==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==17935==by 0x4FB94E: pci_ich9_uninit (ich.c:161)
==17935==by 0x5350FB: pci_qdev_unrealize (pci.c:1083)
==17935==by 0x4BCD39: device_set_realized (qdev.c:988)
==17935==by 0x5F622D: property_set_bool (object.c:1886)
==17935==by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27)
==17935==by 0x5F7FCF: object_property_set_bool (object.c:1162)
==17935==by 0x448B93: qdev_device_add (qdev-monitor.c:630)
==17935==by 0x449122: qmp_device_add (qdev-monitor.c:807)
==17935==by 0x462B29: hmp_device_add (hmp.c:1925)
==17935==by 0x370F83: handle_hmp_command (monitor.c:3119)
==17935==  Block was alloc'd at
==17935==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==17935==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==17935==by 0x4FB28F: ahci_realize (ahci.c:1468)
==17935==by 0x4FB9D8: pci_ich9_ahci_realize (ich.c:115)
==17935==by 0x5366BD: pci_qdev_realize (pci.c:2002)
==17935==by 0x4BCCB9: device_set_realized (qdev.c:914)
==17935==by 0x5F622D: property_set_bool (object.c:1886)
==17935==by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27)
==17935==by 0x5F7FCF: object_property_set_bool (object.c:1162)
==17935==by 0x448B93: qdev_device_add (qdev-monitor.c:630)
==17935==by 0x449122: qmp_device_add (qdev-monitor.c:807)
==17935==by 0x462B29: hmp_device_add (hmp.c:1925)

Do you've got an idea how to fix that, too?

 Thomas

[Qemu-block] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

We likely do not want to carry these legacy -drive options along forever.
Let's emit a deprecation warning for the -drive options that have a
replacement with the -device option, so that the (hopefully few) remaining
users are aware of this and can adapt their scripts / behaviour accordingly.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 v2:
 - Check for !qtest_enabled() since tests/hd-geo-test still uses these
 - Added "addr" to the list, too
 - Also mark the options as deprecated in the documentation

 blockdev.c  | 14 ++
 qemu-options.hx |  5 -
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 0b38c3d..aef38f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -50,6 +50,7 @@
 #include "qmp-commands.h"
 #include "block/trace.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/qtest.h"
 #include "qemu/cutils.h"
 #include "qemu/help_option.h"
 #include "qemu/throttle-options.h"
@@ -797,6 +798,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 const char *filename;
 Error *local_err = NULL;
 int i;
+const char *deprecated[] = {
+"serial", "trans", "secs", "heads", "cyls", "addr"
+};
 
 /* Change legacy command line options into QMP ones */
 static const struct {
@@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 "update your scripts.\n");
 }
 
+/* Other deprecated options */
+if (!qtest_enabled()) {
+for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
+if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
+error_report("'%s' is deprecated, please use the corresponding 
"
+ "option of '-device' instead", deprecated[i]);
+}
+}
+}
+
 /* Media type */
 value = qemu_opt_get(legacy_opts, "media");
 if (value) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 9d7964d..2f66f1a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -615,6 +615,8 @@ of available connectors of a given interface type.
 This option defines the type of the media: disk or cdrom.
 @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
 These options have the same definition as they have in @option{-hdachs}.
+These parameters are deprecated, use the corresponding parameters
+of @code{-device} instead.
 @item snapshot=@var{snapshot}
 @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive
 (see @option{-snapshot}).
@@ -631,7 +633,8 @@ an untrusted format header.
 @item serial=@var{serial}
 This option specifies the serial number to assign to the device.
 @item addr=@var{addr}
-Specify the controller's PCI address (if=virtio only).
+Specify the controller's PCI address (if=virtio only). This parameter is
+deprecated, use the corresponding parameter of @code{-device} instead.
 @item werror=@var{action},rerror=@var{action}
 Specify which @var{action} to take on write and read errors. Valid actions are:
 "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 11.05.2017 08:45, Markus Armbruster wrote:
> Thomas Huth <th...@redhat.com> writes:
> 
>> On 10.05.2017 17:55, Paolo Bonzini wrote:
>>>
>>>
>>> On 10/05/2017 17:50, Thomas Huth wrote:
>>>> We likely do not want to carry these legacy -drive options along forever.
>>>> Let's emit a deprecation warning for the -drive options that have a
>>>> replacement with the -device option, so that the (hopefully few) remaining
>>>> users are aware of this and can adapt their scripts / behaviour.
>>>>
>>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>>> ---
>>>>  blockdev.c | 11 +++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 4d8cded..87a025a 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -797,6 +797,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>>>> BlockInterfaceType block_default_type)
>>>>  const char *filename;
>>>>  Error *local_err = NULL;
>>>>  int i;
>>>> +const char *deprecated[] = {
>>>> +"serial", "trans", "secs", "heads", "cyls"
>>>> +};
>>>>  
>>>>  /* Change legacy command line options into QMP ones */
>>>>  static const struct {
>>>> @@ -880,6 +883,14 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>>>> BlockInterfaceType block_default_type)
>>>>  "update your scripts.\n");
>>>>  }
>>>>  
>>>> +/* Other deprecated options */
>>>> +for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
>>>> +if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
>>>> +error_report("'%s' is deprecated, please use the 
>>>> corresponding "
>>>> + "option of '-device' instead", deprecated[i]);
>>>> +}
>>>> +}
>>>> +
>>>>  /* Media type */
>>>>  value = qemu_opt_get(legacy_opts, "media");
>>>>  if (value) {
>>>
>>> This one should be deprecated too (separate patch if you prefer).
>>
>> I tried to add it, but then the deprecation message is always shown. I
>> think this happens because of the default CD-ROM drive is create in vl.c
>> with CDROM_OPTS (that is defined to "media=cdrom").
>> So no clue how to properly print a deprecation option for "media" here
>> right now (and it should likely be in a separate patch if there is a
>> solution).
> 
> The knee-jerk solution is to suppress the warning just for default
> drives, like we do in drive_check_orphaned().
> 
> The proper solution is to desugar default drives into non-legacy form.

"grep -r media= *" also shows that this option is heavily used in the
qemu-iotests ... so there is even more to be done here. Since I'm really
no block layer expert at all, I'd prefer if someone who is more
experienced in this area could clean up this legacy mess...

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 10.05.2017 17:50, Thomas Huth wrote:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour.
> 
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
>  blockdev.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4d8cded..87a025a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -797,6 +797,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  const char *filename;
>  Error *local_err = NULL;
>  int i;
> +const char *deprecated[] = {
> +"serial", "trans", "secs", "heads", "cyls"
> +};
>  
>  /* Change legacy command line options into QMP ones */
>  static const struct {
> @@ -880,6 +883,14 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  "update your scripts.\n");
>  }
>  
> +/* Other deprecated options */
> +for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
> +if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
> +error_report("'%s' is deprecated, please use the corresponding "
> + "option of '-device' instead", deprecated[i]);
> +}
> +}
> +
>  /* Media type */
>  value = qemu_opt_get(legacy_opts, "media");
>  if (value) {
> 

Self-NAK to this version of the patch ... looks like tests/hd-geo-test
is still using these parameters and now spills out these warning
messages. Need to clean up the test first...

 Thomas

[Qemu-block] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

We likely do not want to carry these legacy -drive options along forever.
Let's emit a deprecation warning for the -drive options that have a
replacement with the -device option, so that the (hopefully few) remaining
users are aware of this and can adapt their scripts / behaviour.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 blockdev.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 4d8cded..87a025a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -797,6 +797,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 const char *filename;
 Error *local_err = NULL;
 int i;
+const char *deprecated[] = {
+"serial", "trans", "secs", "heads", "cyls"
+};
 
 /* Change legacy command line options into QMP ones */
 static const struct {
@@ -880,6 +883,14 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 "update your scripts.\n");
 }
 
+/* Other deprecated options */
+for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
+if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
+error_report("'%s' is deprecated, please use the corresponding "
+ "option of '-device' instead", deprecated[i]);
+}
+}
+
 /* Media type */
 value = qemu_opt_get(legacy_opts, "media");
 if (value) {
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 10.05.2017 17:55, Paolo Bonzini wrote:
> 
> 
> On 10/05/2017 17:50, Thomas Huth wrote:
>> We likely do not want to carry these legacy -drive options along forever.
>> Let's emit a deprecation warning for the -drive options that have a
>> replacement with the -device option, so that the (hopefully few) remaining
>> users are aware of this and can adapt their scripts / behaviour.
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>> ---
>>  blockdev.c | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 4d8cded..87a025a 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -797,6 +797,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>> BlockInterfaceType block_default_type)
>>  const char *filename;
>>  Error *local_err = NULL;
>>  int i;
>> +const char *deprecated[] = {
>> +"serial", "trans", "secs", "heads", "cyls"
>> +};
>>  
>>  /* Change legacy command line options into QMP ones */
>>  static const struct {
>> @@ -880,6 +883,14 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>> BlockInterfaceType block_default_type)
>>  "update your scripts.\n");
>>  }
>>  
>> +/* Other deprecated options */
>> +for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
>> +if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
>> +error_report("'%s' is deprecated, please use the corresponding "
>> + "option of '-device' instead", deprecated[i]);
>> +}
>> +}
>> +
>>  /* Media type */
>>  value = qemu_opt_get(legacy_opts, "media");
>>  if (value) {
> 
> This one should be deprecated too (separate patch if you prefer).

I tried to add it, but then the deprecation message is always shown. I
think this happens because of the default CD-ROM drive is create in vl.c
with CDROM_OPTS (that is defined to "media=cdrom").
So no clue how to properly print a deprecation option for "media" here
right now (and it should likely be in a separate patch if there is a
solution).

 Thomas

[Qemu-block] Strange location of the "qemu-ga Invocation" chapter in the qemu-doc

[Thomas Huth]

 Hi all,

I noticed that the "qemu-ga Invocation" chapter in the QEMU doc is in a
strange location - it's a sub-chapter of the "Disk images" chapter.
Since the guest agent is not directly related to the handling of disk
images, that sounds somewhat wrong to me - or do I miss something?
Does anybody have a suggestion for a better location?

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 12.05.2017 12:33, Thomas Huth wrote:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour accordingly.
> 
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
>  v2:
>  - Check for !qtest_enabled() since tests/hd-geo-test still uses these
>  - Added "addr" to the list, too
>  - Also mark the options as deprecated in the documentation
> 
>  blockdev.c  | 14 ++
>  qemu-options.hx |  5 -
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0b38c3d..aef38f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -50,6 +50,7 @@
>  #include "qmp-commands.h"
>  #include "block/trace.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/qtest.h"
>  #include "qemu/cutils.h"
>  #include "qemu/help_option.h"
>  #include "qemu/throttle-options.h"
> @@ -797,6 +798,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  const char *filename;
>  Error *local_err = NULL;
>  int i;
> +const char *deprecated[] = {
> +"serial", "trans", "secs", "heads", "cyls", "addr"
> +};
>  
>  /* Change legacy command line options into QMP ones */
>  static const struct {
> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  "update your scripts.\n");
>  }
>  
> +/* Other deprecated options */
> +if (!qtest_enabled()) {
> +for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
> +if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
> +error_report("'%s' is deprecated, please use the 
> corresponding "
> + "option of '-device' instead", deprecated[i]);
> +}
> +}
> +}
> +
>  /* Media type */
>  value = qemu_opt_get(legacy_opts, "media");
>  if (value) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9d7964d..2f66f1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -615,6 +615,8 @@ of available connectors of a given interface type.
>  This option defines the type of the media: disk or cdrom.
>  @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
>  These options have the same definition as they have in @option{-hdachs}.
> +These parameters are deprecated, use the corresponding parameters
> +of @code{-device} instead.
>  @item snapshot=@var{snapshot}
>  @var{snapshot} is "on" or "off" and controls snapshot mode for the given 
> drive
>  (see @option{-snapshot}).
> @@ -631,7 +633,8 @@ an untrusted format header.
>  @item serial=@var{serial}
>  This option specifies the serial number to assign to the device.
>  @item addr=@var{addr}
> -Specify the controller's PCI address (if=virtio only).
> +Specify the controller's PCI address (if=virtio only). This parameter is
> +deprecated, use the corresponding parameter of @code{-device} instead.
>  @item werror=@var{action},rerror=@var{action}
>  Specify which @var{action} to take on write and read errors. Valid actions 
> are:
>  "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
> 

ping²

Any takers?

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 12.05.2017 12:33, Thomas Huth wrote:
> We likely do not want to carry these legacy -drive options along forever.
> Let's emit a deprecation warning for the -drive options that have a
> replacement with the -device option, so that the (hopefully few) remaining
> users are aware of this and can adapt their scripts / behaviour accordingly.
> 
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
>  v2:
>  - Check for !qtest_enabled() since tests/hd-geo-test still uses these
>  - Added "addr" to the list, too
>  - Also mark the options as deprecated in the documentation
> 
>  blockdev.c  | 14 ++
>  qemu-options.hx |  5 -
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0b38c3d..aef38f0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -50,6 +50,7 @@
>  #include "qmp-commands.h"
>  #include "block/trace.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/qtest.h"
>  #include "qemu/cutils.h"
>  #include "qemu/help_option.h"
>  #include "qemu/throttle-options.h"
> @@ -797,6 +798,9 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  const char *filename;
>  Error *local_err = NULL;
>  int i;
> +const char *deprecated[] = {
> +"serial", "trans", "secs", "heads", "cyls", "addr"
> +};
>  
>  /* Change legacy command line options into QMP ones */
>  static const struct {
> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  "update your scripts.\n");
>  }
>  
> +/* Other deprecated options */
> +if (!qtest_enabled()) {
> +for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
> +if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
> +error_report("'%s' is deprecated, please use the 
> corresponding "
> + "option of '-device' instead", deprecated[i]);
> +}
> +}
> +}
> +
>  /* Media type */
>  value = qemu_opt_get(legacy_opts, "media");
>  if (value) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9d7964d..2f66f1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -615,6 +615,8 @@ of available connectors of a given interface type.
>  This option defines the type of the media: disk or cdrom.
>  @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
>  These options have the same definition as they have in @option{-hdachs}.
> +These parameters are deprecated, use the corresponding parameters
> +of @code{-device} instead.
>  @item snapshot=@var{snapshot}
>  @var{snapshot} is "on" or "off" and controls snapshot mode for the given 
> drive
>  (see @option{-snapshot}).
> @@ -631,7 +633,8 @@ an untrusted format header.
>  @item serial=@var{serial}
>  This option specifies the serial number to assign to the device.
>  @item addr=@var{addr}
> -Specify the controller's PCI address (if=virtio only).
> +Specify the controller's PCI address (if=virtio only). This parameter is
> +deprecated, use the corresponding parameter of @code{-device} instead.
>  @item werror=@var{action},rerror=@var{action}
>  Specify which @var{action} to take on write and read errors. Valid actions 
> are:
>  "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
> 

Ping ... any comments on this version of my patch?

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v2] blockdev: Print a warning for legacy drive options that belong to -device

[Thomas Huth]

On 30.05.2017 07:20, Markus Armbruster wrote:
> Thomas Huth <th...@redhat.com> writes:
> 
>> We likely do not want to carry these legacy -drive options along forever.
>> Let's emit a deprecation warning for the -drive options that have a
>> replacement with the -device option, so that the (hopefully few) remaining
>> users are aware of this and can adapt their scripts / behaviour accordingly.
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>> ---
[...]
>>  /* Change legacy command line options into QMP ones */
>>  static const struct {
>> @@ -880,6 +884,16 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>> BlockInterfaceType block_default_type)
>if (qemu_opt_get(legacy_opts, "boot") != NULL) {
>fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
>"ignored. Future versions will reject this parameter. 
> Please "
>>  "update your scripts.\n");
> 
> Unrelated to this patch: this is ugly.  It's also almost three years
> old.  Can we bury the corpse already?

If you like to get rid of this now, feel free to send a patch ...
otherwise I'll make sure that it'll go away with QEMU v3.0 (it's on the
to-be-removed list on http://wiki.qemu.org/Features/LegacyRemoval already)

[...]
>> @@ -631,7 +633,8 @@ an untrusted format header.
>>  @item serial=@var{serial}
>>  This option specifies the serial number to assign to the device.
>>  @item addr=@var{addr}
>> -Specify the controller's PCI address (if=virtio only).
>> +Specify the controller's PCI address (if=virtio only). This parameter is
>> +deprecated, use the corresponding parameter of @code{-device} instead.
>>  @item werror=@var{action},rerror=@var{action}
>>  Specify which @var{action} to take on write and read errors. Valid actions 
>> are:
>>  "ignore" (ignore the error and try to continue), "stop" (pause QEMU),
> 
> Reviewed-by: Markus Armbruster <arm...@redhat.com>

 Thanks!
  Thomas

[Qemu-block] Dead code in cpu-models.h (was: block: Clean up some bad code in the vvfat driver)

[Thomas Huth]

On 19.09.2017 10:06, Paolo Bonzini wrote:
> On 13/09/2017 21:08, John Snow wrote:
[...]
>> Farewell, bitrot code.
>>
>> Reviewed-by: John Snow 
>>
>> Out of curiosity, I wonder ...
>>
>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>> 320
> 
> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
> ...
> hw/net/eepro100.c:21
> target/ppc/cpu-models.h:76
> 
> whoa :)

Igor recently already removed the dead definitions from cpu-models.c :

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff

I guess we could now remove the corresponding dead definitions from the
header, too...

Any volunteers?

 Thomas

Re: [Qemu-block] [Qemu-devel] Dead code in cpu-models.h

[Thomas Huth]

On 19.09.2017 20:54, John Snow wrote:
> 
> 
> On 09/19/2017 04:14 AM, Thomas Huth wrote:
>> On 19.09.2017 10:06, Paolo Bonzini wrote:
>>> On 13/09/2017 21:08, John Snow wrote:
>> [...]
>>>> Farewell, bitrot code.
>>>>
>>>> Reviewed-by: John Snow <js...@redhat.com>
>>>>
>>>> Out of curiosity, I wonder ...
>>>>
>>>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>>>> 320
>>>
>>> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
>>> ...
>>> hw/net/eepro100.c:21
>>> target/ppc/cpu-models.h:76
>>>
>>> whoa :)
>>
>> Igor recently already removed the dead definitions from cpu-models.c :
>>
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff
>>
>> I guess we could now remove the corresponding dead definitions from the
>> header, too...
>>
>> Any volunteers?
>>
>>  Thomas
> 
> Well, I can just take a chainsaw to it blindly if you want to critique it.
> 

I think it should at least be aligned with the changes that have been
done to cpu-models.c (e.g. the definition for CPU_POWERPC_401B3 has been
removed from the .c file, so the CPU_POWERPC_401B3 could be removed from
the .h file, too.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

[Thomas Huth]

On 19.09.2017 21:01, John Snow wrote:
> 
> 
> On 09/19/2017 04:06 AM, Paolo Bonzini wrote:
>> On 13/09/2017 21:08, John Snow wrote:
>>>
>>>
>>> On 09/13/2017 06:21 AM, Thomas Huth wrote:
>>>> Remove the unnecessary home-grown redefinition of the assert() macro here,
>>>> and remove the unusable debug code at the end of the checkpoint() function.
>>>> The code there uses assert() with side-effects (assignment to the "mapping"
>>>> variable), which should be avoided. Looking more closely, it seems as it is
>>>> apparently also only usable for one certain directory layout (with a file
>>>> named USB.H in it) and thus is of no use for the rest of the world.
>>>>
>>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>>
>>> Farewell, bitrot code.
>>>
>>> Reviewed-by: John Snow <js...@redhat.com>
>>>
>>> Out of curiosity, I wonder ...
>>>
>>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>>> 320
>>
>>
>> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
>> ...
>> hw/net/eepro100.c:21
>> target/ppc/cpu-models.h:76
>>
>> whoa :)
>>
> 
> Wonder if '#if 0' should be against the style guide / in checkpatch.

checkpatch already complains if you have a "#if 0" in your patch, so I
think we should be pretty fine here already - but of course you can
still add a paragraph to the CODING_STYLE if you like.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()

[Thomas Huth]

On 12.09.2017 15:35, Eric Blake wrote:
> On 09/12/2017 05:45 AM, Thomas Huth wrote:
>> On 11.09.2017 19:20, Eric Blake wrote:
>>> Maintaining two layers of libqtest APIs, one that takes an explicit
>>> QTestState object, and the other that uses the implicit global_qtest,
>>> is annoying.  In the interest of getting rid of global implicit
>>> state and having less code to maintain, merge:
>>>  qtest_clock_set()
>>>  qtest_clock_step()
>>>  qtest_clock_step_next()
>>> with their short counterparts.  All callers that previously
>>> used the short form now make it explicit that they are relying on
>>> global_qtest, and later patches can then clean things up to remove
>>> the global variable.
>>>
> 
>>> @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
>>>   *
>>>   * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
>>>   */
>>> -int64_t qtest_clock_set(QTestState *s, int64_t val);
>>> +int64_t clock_set(QTestState *s, int64_t val);
>>  Could we please keep the "qtest" prefix here and rather get rid of the
>> other ones? Even if it's more to type, I prefer to have a proper prefix
>> here so that it is clear at the first sight that the functions belong to
>> the qtest framework.
> 
> I suppose we can, although it makes more lines that are likely to bump
> up against 80 columns, and thus slightly more churn to reformat things
> to keep checkpatch happy.  I like the shorter name, because less typing
> is easier to remember.  I'd prefer a second opinion on naming before
> doing anything about it though - Markus or Paolo, do you have any
> preference?

IMHO you should at least keep the qtest prefix in patch 33/38 to avoid
confusion with the system definitions that have the same names (see "man
2 outb" for example).

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] iotests: use -ccw on s390x for 051

[Thomas Huth]

On 08.09.2017 13:54, Kevin Wolf wrote:
> Am 08.09.2017 um 13:24 hat Cornelia Huck geschrieben:
>> On Fri, 8 Sep 2017 13:04:25 +0200
>> Kevin Wolf  wrote:
>>
>>> Am 05.09.2017 um 17:16 hat Cornelia Huck geschrieben:
 The default cpu model on s390x does not provide zPCI, which is
 not yet wired up on tcg. Moreover, virtio-ccw is the standard
 on s390x, so use the -ccw instead of the -pci versions of virtio
 devices on s390x.

 Provide an output file for s390x.

 Signed-off-by: Cornelia Huck 
 ---
  tests/qemu-iotests/051 |   9 +-
  tests/qemu-iotests/051.s390-ccw-virtio.out | 434 
 +
  2 files changed, 442 insertions(+), 1 deletion(-)
  create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out  
>>>
>>> It's already a pain to have two separate output files for 051, let's try
>>> to avoid adding a third one. Even more so since I think that the split
>>> between 051.out and 051.pc.out was already made for s390, so I'm not
>>> sure if anyone would actually still make use of the plain 051.out
>>> output if s390 got it's own one.
>>
>> Are there no non-pc and non-s390 machines for which this is run?
> 
> Who knows? But I'm not aware of anyone who is interested in something
> else and has contributed to the test cases until now.

FWIW, as far as I know, Lukáš is running this test also on ppc64 in our
weekly regression run. So it would be good to keep that working, please :-)

>> Another approach would be to drop the -pci postfix, but I don't want to
>> introduce more usage of aliases.
> 
> Maybe that would indeed be the easiest way. As long as we don't intend
> to remove the alias from qemu, there's no reason not to use it in tests.

Maybe we should even use it in a couple of places on purpose - so we get
some test coverage for them?

 Thomas

Re: [Qemu-block] [PATCH v7 13/38] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 11.09.2017 19:19, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all fw_cfg test
> functionality (invoked through alloc-pc) to pass in an explicit
> QTestState, adjusting all callers.  In particular, fw_cfg-test
> had to reorder things to create the test state prior to creating
> the fw_cfg (and drop a pointless strdup in the meantime), but that
> test now no longer depends on global_qtest.
> 
> Signed-off-by: Eric Blake <ebl...@redhat.com>

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [PATCH v7 12/38] libqos: Use explicit QTestState for virtio operations

[Thomas Huth]

On 11.09.2017 19:19, Eric Blake wrote:
> Now that QVirtioDevice and QVirtQueue point back to QVirtioBus,
> we can reuse the explicit QTestState stored there rather than
> relying on implicit global_qtest.  We also have to pass QTestState
> through a few functions that can't trace back through
> QVirtioDevice, and update those callers.
> 
> Drop some useless casts while touching things.
> 
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>  tests/libqos/virtio.h  |  6 ++--
>  tests/libqos/virtio-mmio.c | 57 ++-
>  tests/libqos/virtio-pci.c  |  8 ++---
>  tests/libqos/virtio.c  | 84 
> ++
>  tests/virtio-blk-test.c| 11 +++---
>  5 files changed, 94 insertions(+), 72 deletions(-)

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [PATCH v7 16/38] libqos: Use explicit QTestState for ahci operations

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all ahci test
> functionality to pass in an explicit QTestState.  The state was
> already available, so no callers had to be adjusted.
> 
> Signed-off-by: Eric Blake <ebl...@redhat.com>

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [PATCH v7 26/38] libqtest: Merge qtest_end() into qtest_quit()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> Rather than have two similar shutdown functions, where one requires
> the use of global_qtest in the header, it is better to have a single
> shutdown function that still takes care of cleaning up global_qtest
> if it is set.  All callers are updated.
> 
> Signed-off-by: Eric Blake <ebl...@redhat.com>

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [PATCH v2 3/3] iotests: use virtio aliases for 067

[Thomas Huth]

On 13.09.2017 11:10, Cornelia Huck wrote:
> The default cpu model on s390x does not provide zPCI, which is
> not yet wired up on tcg. Moreover, virtio-ccw is the standard
> on s390x.
> 
> Using virtio-scsi will implicitly pick the right device, so just
> switch to that for simplicity.
> 
> Signed-off-by: Cornelia Huck <coh...@redhat.com>
> ---
>  tests/qemu-iotests/067 | 3 ++-
>  tests/qemu-iotests/067.out | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
> index 5d4ca4bc61..cbb3da286a 100755
> --- a/tests/qemu-iotests/067
> +++ b/tests/qemu-iotests/067
> @@ -141,7 +141,7 @@ echo
>  echo === Empty drive with -device and device_del ===
>  echo
>  
> -run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0 < +run_qemu -device virtio-scsi -device scsi-cd,id=cd0 <  { "execute": "qmp_capabilities" }
>  { "execute": "query-block" }
>  { "execute": "device_del", "arguments": { "id": "cd0" } }
> @@ -150,6 +150,7 @@ run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0 
> <  { "execute": "quit" }
>  EOF
>  
> +

Superfluous white space change?

With that nit removed:

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [PATCH v2 2/3] iotests: use -ccw on s390x for 051

[Thomas Huth]

On 13.09.2017 11:10, Cornelia Huck wrote:
> The default cpu model on s390x does not provide zPCI, which is
> not yet wired up on tcg. Moreover, virtio-ccw is the standard
> on s390x, so use the -ccw instead of the -pci versions of virtio
> devices on s390x.
> 
> Signed-off-by: Cornelia Huck <coh...@redhat.com>
> ---
>  tests/qemu-iotests/051| 12 +++-
>  tests/qemu-iotests/051.out|  2 +-
>  tests/qemu-iotests/051.pc.out |  2 +-
>  3 files changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()

[Thomas Huth]

On 12.09.2017 15:32, Eric Blake wrote:
> On 09/12/2017 05:14 AM, Thomas Huth wrote:
>> On 11.09.2017 19:20, Eric Blake wrote:
>>> We have several callers that were formatting the argument strings
>>> themselves; consolidate this effort by adding new convenience
>>> functions directly in libqtest, and update all call-sites that
>>> can benefit from it.
[...]
>>>  static void test_mon_partial(const void *data)
>>>  {
>>>  char *s;
>>> -char *cli;
>>> +const char *args = data;
>>>
>>> -cli = make_cli(data, "-smp 8 "
>>> -   "-numa node,nodeid=0,cpus=0-1 "
>>> -   "-numa node,nodeid=1,cpus=4-5 ");
>>> -qtest_start(cli);
>>> +global_qtest = qtest_startf("%s -smp 8 "
>>> +"-numa node,nodeid=0,cpus=0-1 "
>>> +"-numa node,nodeid=1,cpus=4-5 ", args);
>>
>> Does GCC emit a warning if you'd used data here directly? Otherwise I
>> think you could simply do this without the local args variable...
> 
> Passing void* through varargs, with the intent of the receiver parsing
> it as char*, is technically undefined in C.  I don't know if gcc warns,
> but I'm also worried that clang might warn.  I prefer to err on the side
> of defined behavior in this case, even though it annoyingly requires a
> temporary variable.

OK, sounds reasonable, so let's keep it!

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v7 09/38] libqos: Track QTestState with QVirtioBus

[Thomas Huth]

On 12.09.2017 15:28, Eric Blake wrote:
> On 09/12/2017 02:21 AM, Thomas Huth wrote:
[...]
>> I fail to see why we need a separate bus object here for each device.
>> The bus is only available one time, not multiple times, isn't it? So
>> there should also only be one bus object floating around, not multiple
>> ones... or do I miss something?
> 
> You are right that there is only one bus for the machine - the problem
> is that the object representing the bus is dynamically created on the
> fly at the point where the device is first grabbed, rather than up front
> as part of creating the machine (in other words, the device search is
> not performed on a pre-existing bus object).
> 
> Contrast qpci_device_find() (lookup based on a pre-existing bus) with
> qvirtio_mmio_init_device() (lookup that did not use a pre-existing bus);
> also note that qvirtio_pci_device_find() takes a pre-existing QPCIBus
> (since on that architecture, the QVirtioBus is itself a device on the
> QPCIBus).
> 
> I suppose that we could indeed refactor the code to require callers to
> create the QVirtioBus object up front, and then make the device lookup
> (both qvirtio_mmio_init_device() and qvirtio_pci_device_find()) take a
> QVirtioBus parameter instead of QTestState or QPCIBus.  That's slightly
> more work, but I'm happy to attempt it if you think it will be better.

At least to me, that sounds like the right way to go. I guess we might
run into other problems later if we have multiple instances of the bus
object floating around ... so sorry if this is extra work, but I'd say
let's better do it properly now instead of having to rework this again
later.

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/10] cleanup qemu-iotests

[Thomas Huth]

On 12.09.2017 16:44, Paolo Bonzini wrote:
> The purpose of this series is to separate the "check" sources from
> the tests.  After these patches, common.config is reduced to simple
> shell initialization, and common.rc is only included by the tests.
> 
> Along the way, a lot of dead code is removed too.
> 
> In v2, the following patches:
> 
>   qemu-iotests: do not do useless search for QEMU_*_PROG
>   qemu-iotests: do not search for binaries in the current directory
>   qemu-iotests: include common.env and common.config early
> 
> have been replaced by "qemu-iotests: cleanup and fix search for programs",
> which also preserves the behavior of searching for programs as symlinks
> in the current directory.
> 
> Paolo
> 
> Paolo Bonzini (10):
>   qemu-iotests: remove dead code
>   qemu-iotests: get rid of AWK_PROG
>   qemu-iotests: move "check" code out of common.rc
>   qemu-iotests: cleanup and fix search for programs
>   qemu-iotests: limit non-_PROG-suffixed variables to common.rc
>   qemu-iotests: do not include common.rc in "check"
>   qemu-iotests: disintegrate more parts of common.config
>   qemu-iotests: fix uninitialized variable
>   qemu-iotests: get rid of $iam
>   qemu-iotests: merge "check" and "common"
> 
>  tests/qemu-iotests/039.out   |  10 +-
>  tests/qemu-iotests/061.out   |   4 +-
>  tests/qemu-iotests/137.out   |   2 +-
>  tests/qemu-iotests/check | 575 
> ++-
>  tests/qemu-iotests/common| 459 ---
>  tests/qemu-iotests/common.config | 206 +-
>  tests/qemu-iotests/common.qemu   |   1 +
>  tests/qemu-iotests/common.rc | 205 +++---

Meta comment: Could we maybe also rename "tests/qemu-iotests" to
"tests/iotests" ? The "qemu" prefix sounds always very superfluous to me
here...

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v7 10/38] libqos: Move/rename qpci_unplug_acpi_device_test() to pci.c

[Thomas Huth]

On 12.09.2017 15:28, Eric Blake wrote:
> On 09/12/2017 02:29 AM, Thomas Huth wrote:
>> On 11.09.2017 19:19, Eric Blake wrote:
>>> Commit 2f8b2767 originally added qpci_plug_device_test() and
>>> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c.
>>> Later, commit cf716b31 moved one half of the pair to pci.c
>>> when adding PPC64 support.  Keep the implementations of the
>>> two functions together, and shorten the name to
>>> qpci_unplug_device_test(), since all callers use the two
>>> functions in tandem.
>>>
> 
>>
>> No, that's a bad idea. ACPI and that outb() is clearly something
>> specific to x86, so this should not reside in pci.c but in pci-pc.c
>>
>> We might be able to unify this - I've had a similar patch here:
>>
>>  https://patchwork.kernel.org/patch/9905031/
>>
>> ... but I think this needs some more careful thinking and discussion, so
>> I'd suggest that you remove this from your already huge patch series for
>> now and we fix it later instead.
> 
> Okay, I'm fine dropping this patch, and can base my respin on top of
> your cleanup instead.

Note that my patches are currently on halt since I'm waiting for your
qemu_startf() reworks to get included first :-) ... so feel free to pick
up ideas or patches from my series into your series if that helps.

 Thomas



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] block: Clean up some bad code in the vvfat driver

[Thomas Huth]

Remove the unnecessary home-grown redefinition of the assert() macro here,
and remove the unusable debug code at the end of the checkpoint() function.
The code there uses assert() with side-effects (assignment to the "mapping"
variable), which should be avoided. Looking more closely, it seems as it is
apparently also only usable for one certain directory layout (with a file
named USB.H in it) and thus is of no use for the rest of the world.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 block/vvfat.c | 26 ++
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index c54fa94..777a8cd 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -56,15 +56,6 @@
 
 static void checkpoint(void);
 
-#ifdef __MINGW32__
-void nonono(const char* file, int line, const char* msg) {
-fprintf(stderr, "Nonono! %s:%d %s\n", file, line, msg);
-exit(-5);
-}
-#undef assert
-#define assert(a) do {if (!(a)) nonono(__FILE__, __LINE__, #a);}while(0)
-#endif
-
 #else
 
 #define DLOG(a)
@@ -3269,24 +3260,11 @@ static void bdrv_vvfat_init(void)
 block_init(bdrv_vvfat_init);
 
 #ifdef DEBUG
-static void checkpoint(void) {
+static void checkpoint(void)
+{
 assert(((mapping_t*)array_get(&(vvv->mapping), 0))->end == 2);
 check1(vvv);
 check2(vvv);
 assert(!vvv->current_mapping || vvv->current_fd || 
(vvv->current_mapping->mode & MODE_DIRECTORY));
-#if 0
-if (((direntry_t*)vvv->directory.pointer)[1].attributes != 0xf)
-fprintf(stderr, "Nonono!\n");
-mapping_t* mapping;
-direntry_t* direntry;
-assert(vvv->mapping.size >= vvv->mapping.item_size * vvv->mapping.next);
-assert(vvv->directory.size >= vvv->directory.item_size * 
vvv->directory.next);
-if (vvv->mapping.next<47)
-return;
-assert((mapping = array_get(&(vvv->mapping), 47)));
-assert(mapping->dir_index < vvv->directory.next);
-direntry = array_get(&(vvv->directory), mapping->dir_index);
-assert(!memcmp(direntry->name, "USB H  ", 11) || direntry->name[0]==0);
-#endif
 }
 #endif
-- 
1.8.3.1

Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> Maintaining two layers of libqtest APIs, one that takes an explicit
> QTestState object, and the other that uses the implicit global_qtest,
> is annoying.  In the interest of getting rid of global implicit
> state and having less code to maintain, merge:
>  qtest_clock_set()
>  qtest_clock_step()
>  qtest_clock_step_next()
> with their short counterparts.  All callers that previously
> used the short form now make it explicit that they are relying on
> global_qtest, and later patches can then clean things up to remove
> the global variable.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqtest.h | 50 
>  tests/libqtest.c |  6 ++--
>  tests/e1000e-test.c  |  2 +-
>  tests/fdc-test.c |  4 +--
>  tests/ide-test.c |  2 +-
>  tests/libqos/virtio.c|  8 +++---
>  tests/rtc-test.c | 74 
> 
>  tests/rtl8139-test.c | 10 +++
>  tests/tco-test.c | 22 +++---
>  tests/test-arm-mptimer.c | 25 +---
>  tests/wdt_ib700-test.c   | 12 
>  11 files changed, 90 insertions(+), 125 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 5651b77d2f..26d5f37bc9 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -417,17 +417,17 @@ void qtest_bufwrite(QTestState *s, uint64_t addr,
>  void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size);
> 
>  /**
> - * qtest_clock_step_next:
> + * clock_step_next:
>   * @s: #QTestState instance to operate on.
>   *
>   * Advance the QEMU_CLOCK_VIRTUAL to the next deadline.
>   *
>   * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
>   */
> -int64_t qtest_clock_step_next(QTestState *s);
> +int64_t clock_step_next(QTestState *s);
> 
>  /**
> - * qtest_clock_step:
> + * clock_step:
>   * @s: QTestState instance to operate on.
>   * @step: Number of nanoseconds to advance the clock by.
>   *
> @@ -435,10 +435,10 @@ int64_t qtest_clock_step_next(QTestState *s);
>   *
>   * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
>   */
> -int64_t qtest_clock_step(QTestState *s, int64_t step);
> +int64_t clock_step(QTestState *s, int64_t step);
> 
>  /**
> - * qtest_clock_set:
> + * clock_set:
>   * @s: QTestState instance to operate on.
>   * @val: Nanoseconds value to advance the clock to.
>   *
> @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
>   *
>   * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
>   */
> -int64_t qtest_clock_set(QTestState *s, int64_t val);
> +int64_t clock_set(QTestState *s, int64_t val);
 Could we please keep the "qtest" prefix here and rather get rid of the
other ones? Even if it's more to type, I prefer to have a proper prefix
here so that it is clear at the first sight that the functions belong to
the qtest framework.

 Thomas

Re: [Qemu-block] [PATCH v7 33/38] libqtest: Merge qtest_{in, out}[bwl]() with {in, out}[bwl]()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> Maintaining two layers of libqtest APIs, one that takes an explicit
> QTestState object, and the other that uses the implicit global_qtest,
> is annoying.  In the interest of getting rid of global implicit
> state and having less code to maintain, merge:
>  qtest_outb()
>  qtest_outw()
>  qtest_outl()
>  qtest_inb()
>  qtest_inw()
>  qtest_inl()
> with their short counterparts.  All callers that previously
> used the short form now make it explicit that they are relying on
> global_qtest, and later patches can then clean things up to remove
> the global variable.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqtest.h| 99 
> ++---
>  tests/multiboot/libc.h  |  2 +-
>  tests/libqtest.c| 14 +++
>  tests/boot-order-test.c |  4 +-
>  tests/endianness-test.c | 12 +++---
>  tests/fdc-test.c| 77 --
>  tests/hd-geo-test.c |  4 +-
>  tests/ipmi-bt-test.c| 12 +++---
>  tests/ipmi-kcs-test.c   |  8 ++--
>  tests/libqos/fw_cfg.c   |  4 +-
>  tests/libqos/pci-pc.c   | 44 +++---
>  tests/libqos/pci.c  |  2 +-
>  tests/m48t59-test.c |  8 ++--
>  tests/multiboot/libc.c  |  2 +-
>  tests/pvpanic-test.c|  4 +-
>  tests/rtc-test.c|  8 ++--
>  tests/wdt_ib700-test.c  |  8 ++--
>  17 files changed, 120 insertions(+), 192 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 8398c0fd07..520f745e7b 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -205,61 +205,61 @@ void irq_intercept_in(QTestState *s, const char 
> *string);
>  void irq_intercept_out(QTestState *s, const char *string);
> 
>  /**
> - * qtest_outb:
> + * outb:
>   * @s: #QTestState instance to operate on.
>   * @addr: I/O port to write to.
>   * @value: Value being written.
>   *
>   * Write an 8-bit value to an I/O port.
>   */
> -void qtest_outb(QTestState *s, uint16_t addr, uint8_t value);
> +void outb(QTestState *s, uint16_t addr, uint8_t value);

Could we please also keep the qtest prefix here? ... same applies for
all your other following "Merge ..." patches in this series...

 Thomas

Re: [Qemu-block] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> We have several callers that were formatting the argument strings
> themselves; consolidate this effort by adding new convenience
> functions directly in libqtest, and update all call-sites that
> can benefit from it.
[...]
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index e8c2e11817..b535d7768f 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args)
>  return global_qtest = s;
>  }
> 
> +QTestState *qtest_vstartf(const char *fmt, va_list ap)
> +{
> +char *args = g_strdup_vprintf(fmt, ap);
> +QTestState *s;
> +
> +s = qtest_start(args);
> +global_qtest = NULL;

Don't you need a g_free(args) here?

> +return s;
> +}
> +
> +QTestState *qtest_startf(const char *fmt, ...)
> +{
> +va_list ap;
> +QTestState *s;
> +
> +va_start(ap, fmt);
> +s = qtest_vstartf(fmt, ap);
> +va_end(ap);
> +return s;
> +}
[...]
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index 0c5fcdcc44..12bc526ad6 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -14,16 +14,8 @@
>  static void test_device(gconstpointer data)
>  {
>  const char *model = data;
> -QTestState *s;
> -char *args;
> 
> -args = g_strdup_printf("-device %s", model);
> -s = qtest_start(args);
> -
> -if (s) {
> -qtest_quit(s);
> -}
> -g_free(args);
> +qtest_quit(qtest_startf("-device %s", model));

Just my personal taste, but I think I'd be nicer to keep this on
separate lines:

QTestState *s;

s = qtest_startf("-device %s", model);
qtest_quit(s);

>  }
[...]
> diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
> index bdc8a67d57..fc9ea84d66 100644
> --- a/tests/eepro100-test.c
> +++ b/tests/eepro100-test.c
> @@ -13,18 +13,9 @@
>  static void test_device(gconstpointer data)
>  {
>  const char *model = data;
> -QTestState *s;
> -char *args;
> -
> -args = g_strdup_printf("-device %s", model);
> -s = qtest_start(args);
> 
>  /* Tests only initialization so far. TODO: Implement functional tests */
> -
> -if (s) {
> -qtest_quit(s);
> -}
> -g_free(args);
> +qtest_quit(qtest_startf("-device %s", model));
>  }

dito

[...]
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index fa21d26935..e2f6c68be8 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -12,20 +12,14 @@
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
> 
> -static char *make_cli(const char *generic_cli, const char *test_cli)
> -{
> -return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", 
> test_cli);
> -}
> -
>  static void test_mon_explicit(const void *data)
>  {
>  char *s;
> -char *cli;
> +const char *args = data;
> 
> -cli = make_cli(data, "-smp 8 "
> -   "-numa node,nodeid=0,cpus=0-3 "
> -   "-numa node,nodeid=1,cpus=4-7 ");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 "
> +"-numa node,nodeid=0,cpus=0-3 "
> +"-numa node,nodeid=1,cpus=4-7 ", args);
> 
>  s = hmp("info numa");
>  g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
> @@ -33,16 +27,14 @@ static void test_mon_explicit(const void *data)
>  g_free(s);
> 
>  qtest_quit(global_qtest);
> -g_free(cli);
>  }
> 
>  static void test_mon_default(const void *data)
>  {
>  char *s;
> -char *cli;
> +const char *args = data;
> 
> -cli = make_cli(data, "-smp 8 -numa node -numa node");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 -numa node -numa node", args);
> 
>  s = hmp("info numa");
>  g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
> @@ -50,18 +42,16 @@ static void test_mon_default(const void *data)
>  g_free(s);
> 
>  qtest_quit(global_qtest);
> -g_free(cli);
>  }
> 
>  static void test_mon_partial(const void *data)
>  {
>  char *s;
> -char *cli;
> +const char *args = data;
> 
> -cli = make_cli(data, "-smp 8 "
> -   "-numa node,nodeid=0,cpus=0-1 "
> -   "-numa node,nodeid=1,cpus=4-5 ");
> -qtest_start(cli);
> +global_qtest = qtest_startf("%s -smp 8 "
> +"-numa node,nodeid=0,cpus=0-1 "
> +"-numa node,nodeid=1,cpus=4-5 ", args);

Does GCC emit a warning if you'd used data here directly? Otherwise I
think you could simply do this without the local args variable...

 Thomas

Re: [Qemu-block] [PATCH v7 29/38] libqtest: Merge qtest_init() into qtest_start()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> Remove the trivial wrapper qtest_init(), and change qtest_start()
> to no longer implicitly set global_qtest, to make it obvious in the
> rest of the testsuite where we are still relying on global_qtest.
> Everything now uses qtest_start() (and friends) and qtest_quit(),
> and explicitly tracks the QTestState between the two (although in
> many cases, this tracking is still done through global_qtest).
> Doing this makes it easier to see what remaining cleanups will be
> needed if we don't want an implicit dependency on global state.
[...]
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 817e3a5580..2a21bf4605 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -27,7 +27,7 @@ extern QTestState *global_qtest;
>   * qtest_start_without_qmp_handshake:
>   * @extra_args: other arguments to pass to QEMU.
>   *
> - * Returns: #QTestState instance.  Does not affect #global_qtest.
> + * Returns: #QTestState instance, handshaking not yet completed.

I'd rather add a description a la:

  * Start QEMU, but do not execute the QMP handshake yet.
  *
  * Returns: #QTestState instance

>   */
>  QTestState *qtest_start_without_qmp_handshake(const char *extra_args);
> 
> @@ -35,10 +35,7 @@ QTestState *qtest_start_without_qmp_handshake(const char 
> *extra_args);
>   * qtest_start:
>   * @args: other arguments to pass to QEMU
>   *
> - * Start QEMU and assign the resulting #QTestState to #global_qtest.
> - * The global variable is used by "shortcut" functions documented below.
>   *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.

I'd rather change the description instead of removing it:

  * Start QEMU and execute the initial QMP handshake
  *
  * Returns: #QTestState instance.

>   */
>  QTestState *qtest_start(const char *args);
> 
> @@ -47,10 +44,7 @@ QTestState *qtest_start(const char *args);
>   * @fmt...: Format for creating other arguments to pass to QEMU, formatted
>   * like sprintf().
>   *
> - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(),
> - * #global_qtest is left at NULL).
> - *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.

dito

>   */
>  QTestState *qtest_startf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> 
> @@ -60,30 +54,11 @@ QTestState *qtest_startf(const char *fmt, ...) 
> GCC_FMT_ATTR(1, 2);
>   * like vsprintf().
>   * @ap: Format arguments.
>   *
> - * Start QEMU and return the resulting #QTestState (but unlike qtest_start(),
> - * #global_qtest is left at NULL).
> - *
> - * Returns: #QTestState instance.
> + * Returns: #QTestState instance, handshaking completed.

dito

>   */
>  QTestState *qtest_vstartf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
> 
>  /**
> - * qtest_init:
> - * @extra_args: other arguments to pass to QEMU.
> - *
> - * Returns: #QTestState instance.  Does not affect #global_qtest.
> - */
> -static inline QTestState *qtest_init(const char *extra_args)
> -{
> -QTestState *s;
> -
> -assert(!global_qtest);
> -s = qtest_start(extra_args);
> -global_qtest = NULL;
> -return s;
> -}
[...]
> diff --git a/tests/display-vga-test.c b/tests/display-vga-test.c
> index 8667330e3c..d638f15ec3 100644
> --- a/tests/display-vga-test.c
> +++ b/tests/display-vga-test.c
> @@ -12,39 +12,33 @@
> 
>  static void pci_cirrus(void)
>  {
> -qtest_start("-vga none -device cirrus-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device cirrus-vga"));

I'd prefer to keep this on separate lines ... but that's again just my
personal taste. (also for the othe changes below)

>  }
> 
>  static void pci_stdvga(void)
>  {
> -qtest_start("-vga none -device VGA");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device VGA"));
>  }
> 
>  static void pci_secondary(void)
>  {
> -qtest_start("-vga none -device secondary-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device secondary-vga"));
>  }
> 
>  static void pci_multihead(void)
>  {
> -qtest_start("-vga none -device VGA -device secondary-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device VGA -device secondary-vga"));
>  }
> 
>  static void pci_virtio_gpu(void)
>  {
> -qtest_start("-vga none -device virtio-gpu-pci");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device virtio-gpu-pci"));
>  }
> 
>  #ifdef CONFIG_VIRTIO_VGA
>  static void pci_virtio_vga(void)
>  {
> -qtest_start("-vga none -device virtio-vga");
> -qtest_quit(global_qtest);
> +qtest_quit(qtest_start("-vga none -device virtio-vga"));
>  }
>  #endif
[...]
> diff --git a/tests/ne2000-test.c b/tests/ne2000-test.c
> index cae83c5c4c..8e6f7b07c6 100644
> --- a/tests/ne2000-test.c
> +++ b/tests/ne2000-test.c
> @@ -22,7 +22,7 @@ int main(int argc, char **argv)
>  g_test_init(, , NULL);
>  

Re: [Qemu-block] [PATCH v7 32/38] libqtest: Merge qtest_irq*() with irq*()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> Maintaining two layers of libqtest APIs, one that takes an explicit
> QTestState object, and the other that uses the implicit global_qtest,
> is annoying.  In the interest of getting rid of global implicit
> state and having less code to maintain, merge:
>  qtest_get_irq()
>  qtest_irq_intercept_in()
>  qtest_irq_intercept_out()
> with their short counterparts.  All callers that previously
> used the short form now make it explicit that they are relying on
> global_qtest, and later patches can then clean things up to remove
> the global variable.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqtest.h | 47 ++-
>  tests/libqtest.c |  6 +++---
>  tests/fdc-test.c | 48 
> 
>  tests/ide-test.c | 17 +
>  tests/ipmi-bt-test.c |  6 +++---
>  tests/ipmi-kcs-test.c|  8 
>  tests/libqos/libqos-pc.c |  2 +-
>  tests/rtc-test.c | 10 +-
>  tests/tco-test.c |  2 +-
>  tests/wdt_ib700-test.c   |  8 
>  10 files changed, 60 insertions(+), 94 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 26d5f37bc9..8398c0fd07 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -176,33 +176,33 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...) 
> GCC_FMT_ATTR(2, 3);
>  char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap);
> 
>  /**
> - * qtest_get_irq:
> + * get_irq:
>   * @s: #QTestState instance to operate on.
>   * @num: Interrupt to observe.
>   *
>   * Returns: The level of the @num interrupt.
>   */
> -bool qtest_get_irq(QTestState *s, int num);
> +bool get_irq(QTestState *s, int num);
> 
>  /**
> - * qtest_irq_intercept_in:
> + * irq_intercept_in:
>   * @s: #QTestState instance to operate on.
>   * @string: QOM path of a device.
>   *
>   * Associate qtest irqs with the GPIO-in pins of the device
>   * whose path is specified by @string.
>   */
> -void qtest_irq_intercept_in(QTestState *s, const char *string);
> +void irq_intercept_in(QTestState *s, const char *string);
> 
>  /**
> - * qtest_irq_intercept_out:
> + * irq_intercept_out:
>   * @s: #QTestState instance to operate on.
>   * @string: QOM path of a device.
>   *
>   * Associate qtest irqs with the GPIO-out pins of the device
>   * whose path is specified by @string.
>   */
> -void qtest_irq_intercept_out(QTestState *s, const char *string);
> +void irq_intercept_out(QTestState *s, const char *string);

Could we please also keep the qtest prefix here?

 Thomas

Re: [Qemu-block] [PATCH v7 10/38] libqos: Move/rename qpci_unplug_acpi_device_test() to pci.c

[Thomas Huth]

On 11.09.2017 19:19, Eric Blake wrote:
> Commit 2f8b2767 originally added qpci_plug_device_test() and
> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c.
> Later, commit cf716b31 moved one half of the pair to pci.c
> when adding PPC64 support.  Keep the implementations of the
> two functions together, and shorten the name to
> qpci_unplug_device_test(), since all callers use the two
> functions in tandem.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/pci.h  |  2 +-
>  tests/e1000e-test.c |  2 +-
>  tests/ivshmem-test.c|  2 +-
>  tests/libqos/pci-pc.c   | 23 ---
>  tests/libqos/pci.c  | 23 +++
>  tests/virtio-blk-test.c |  2 +-
>  tests/virtio-net-test.c |  2 +-
>  tests/virtio-rng-test.c |  2 +-
>  8 files changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
> index 429c382282..fdda7eca6e 100644
> --- a/tests/libqos/pci.h
> +++ b/tests/libqos/pci.h
> @@ -111,5 +111,5 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
> 
>  void qpci_plug_device_test(const char *driver, const char *id,
> uint8_t slot, const char *opts);
> -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
> +void qpci_unplug_device_test(const char *id, uint8_t slot);
>  #endif
> diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> index d8085d944e..4c663a3019 100644
> --- a/tests/e1000e-test.c
> +++ b/tests/e1000e-test.c
> @@ -461,7 +461,7 @@ static void test_e1000e_hotplug(gconstpointer data)
>  qtest_start("-device e1000e");
> 
>  qpci_plug_device_test("e1000e", "e1000e_net", slot, NULL);
> -qpci_unplug_acpi_device_test("e1000e_net", slot);
> +qpci_unplug_device_test("e1000e_net", slot);
> 
>  qtest_end();
>  }
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index 37763425ee..8c9ed6a568 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -427,7 +427,7 @@ static void test_ivshmem_hotplug(void)
> 
>  qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
>  if (strcmp(arch, "ppc64") != 0) {
> -qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
> +qpci_unplug_device_test("iv1", PCI_SLOT_HP);
>  }
> 
>  qtest_end();
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index e267fd1a44..6305d142a5 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -19,9 +19,6 @@
>  #include "qemu-common.h"
> 
> 
> -#define ACPI_PCIHP_ADDR 0xae00
> -#define PCI_EJ_BASE 0x0008
> -
>  typedef struct QPCIBusPC
>  {
>  QPCIBus bus;
> @@ -156,23 +153,3 @@ void qpci_free_pc(QPCIBus *bus)
> 
>  g_free(s);
>  }
> -
> -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
> -{
> -QDict *response;
> -char *cmd;
> -
> -cmd = g_strdup_printf("{'execute': 'device_del',"
> -  " 'arguments': {"
> -  "   'id': '%s'"
> -  "}}", id);
> -response = qmp(cmd);
> -g_free(cmd);
> -g_assert(response);
> -g_assert(!qdict_haskey(response, "error"));
> -QDECREF(response);
> -
> -outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
> -
> -qmp_eventwait("DEVICE_DELETED");
> -}
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 2dcdeade2a..9f36ec73ef 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -16,6 +16,9 @@
>  #include "hw/pci/pci_regs.h"
>  #include "qemu/host-utils.h"
> 
> +#define ACPI_PCIHP_ADDR 0xae00
> +#define PCI_EJ_BASE 0x0008
> +
>  void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>   void (*func)(QPCIDevice *dev, int devfn, void 
> *data),
>   void *data)
> @@ -412,3 +415,23 @@ void qpci_plug_device_test(const char *driver, const 
> char *id,
>  g_assert(!qdict_haskey(response, "error"));
>  QDECREF(response);
>  }
> +
> +void qpci_unplug_device_test(const char *id, uint8_t slot)
> +{
> +QDict *response;
> +char *cmd;
> +
> +cmd = g_strdup_printf("{'execute': 'device_del',"
> +  " 'arguments': {"
> +  "   'id': '%s'"
> +  "}}", id);
> +response = qmp(cmd);
> +g_free(cmd);
> +g_assert(response);
> +g_assert(!qdict_haskey(response, "error"));
> +QDECREF(response);
> +
> +outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot);
> +
> +qmp_eventwait("DEVICE_DELETED");
> +}

No, that's a bad idea. ACPI and that outb() is clearly something
specific to x86, so this should not reside in pci.c but in pci-pc.c

We might be able to unify this - I've had a similar patch here:

 https://patchwork.kernel.org/patch/9905031/

... but I think this needs some more careful thinking and discussion, so
I'd suggest that you remove this from your already huge patch series for
now and we fix it later instead.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/29] libqos: Track QTestState with QPCIBus

[Thomas Huth]

On 06.09.2017 23:00, Eric Blake wrote:
> On 09/05/2017 04:36 AM, Thomas Huth wrote:
>> On 01.09.2017 20:03, Eric Blake wrote:
>>> When initializing a QPCIBus, track which QTestState the bus is
>>> associated with (so that a later patch can then explicitly use
>>> that test state for all communication on the bus, rather than
>>> blindly relying on global_qtest).  Update the initialization
>>> functions to take another parameter, and update all callers to
>>> pass in state (for now, most callers get away with passing the
>>> current global_qtest as the current state, although this required
>>> fixing the order of initialization to ensure qtest_start() is
>>> called before qpci_init*() in rtl8139-test, and provided an
>>> opportunity to pass in the allocator in e1000e-test).
>>>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>> ---
>> [...]
>>> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
>>> index 6226546c28..c95428e1cb 100644
>>> --- a/tests/libqos/libqos.c
>>> +++ b/tests/libqos/libqos.c
>>> @@ -26,8 +26,8 @@ QOSState *qtest_vboot(QOSOps *ops, const char 
>>> *cmdline_fmt, va_list ap)
>>>  if (ops->init_allocator) {
>>>  qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
>>>  }
>>> -if (ops->qpci_init && qs->alloc) {
>>> -qs->pcibus = ops->qpci_init(qs->alloc);
>>> +if (ops->qpci_init) {
>>
>> Why did you remove the check for qs->alloc?
>>
>>> +qs->pcibus = ops->qpci_init(qs->qts, qs->alloc);
> 
> Because we want to ensure qpci_init() is called to set qs->qts
> (presumably, whether or not qs->alloc is set).  Furthermore, only two
> files declare a 'static QOSOps' structure in the first place
> (libqos-pc.c and libqos-spapr.c); where both files set both the
> .init_allocator and .qpci_init callbacks; a little bit of auditing shows
> that the .init_allocator() never returns NULL (although that requires
> browsing yet more files for malloc-{pc,spapr}.c).

OK, thanks for the explanation! ... but maybe we should
g_assert(gs->alloc) somewhere instead? ... just an idea, I'm also fine
if you leave it away.

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v1 3/8] hw: Replace fprintf(stderr, "*\n" with error_report()

[Thomas Huth]

On 26.09.2017 02:08, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
> 
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' 
> \
> {} +
> find ./* -type f -exec sed -i \
> 'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> find ./* -type f -exec sed -i \
> 'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
> {} +
> 
> Some lines where then manually tweaked to pass checkpatch.
> 
> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
> Cc: Andrzej Zaborowski <balr...@gmail.com>
> Cc: Jan Kiszka <jan.kis...@web.de>
> Cc: Stefan Hajnoczi <stefa...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Thomas Huth <h...@tuxfamily.org>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> Cc: Richard Henderson <r...@twiddle.net>
> Cc: Eduardo Habkost <ehabk...@redhat.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>
> Cc: Anthony Perard <anthony.per...@citrix.com>
> Cc: John Snow <js...@redhat.com>
> Cc: Christian Borntraeger <borntrae...@de.ibm.com>
> Cc: Cornelia Huck <coh...@redhat.com>
> Cc: Alexander Graf <ag...@suse.de>
> Cc: Michael Walle <mich...@walle.cc>
> Cc: Paul Burton <paul.bur...@imgtec.com>
> Cc: Aurelien Jarno <aurel...@aurel32.net>
> Cc: Yongbok Kim <yongbok@imgtec.com>
> Cc: "Hervé Poussineau" <hpous...@reactos.org>
> Cc: Anthony Green <gr...@moxielogic.com>
> Cc: Jason Wang <jasow...@redhat.com>
> Cc: Chris Wulff <crwu...@gmail.com>
> Cc: Marek Vasut <ma...@denx.de>
> Cc: Jia Liu <pro...@gmail.com>
> Cc: Stafford Horne <sho...@gmail.com>
> Cc: Marcel Apfelbaum <mar...@redhat.com>
> Cc: Magnus Damm <magnus.d...@gmail.com>
> Cc: Fabien Chouteau <chout...@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4q...@gmail.com>
> Cc: qemu-...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-...@nongnu.org
> ---
> 
>  hw/arm/armv7m.c |  2 +-
>  hw/arm/boot.c   | 34 +--
>  hw/arm/gumstix.c| 13 
>  hw/arm/mainstone.c  |  7 ++--
>  hw/arm/musicpal.c   |  2 +-
>  hw/arm/omap1.c  |  5 +--
>  hw/arm/omap2.c  | 21 ++--
>  hw/arm/omap_sx1.c   |  6 ++--
>  hw/arm/palm.c   | 10 +++---
>  hw/arm/pxa2xx.c |  7 ++--
>  hw/arm/stellaris.c  |  3 +-
>  hw/arm/tosa.c   | 17 +-
>  hw/arm/versatilepb.c|  2 +-
>  hw/arm/vexpress.c   |  8 ++---
>  hw/arm/z2.c |  6 ++--
>  hw/block/dataplane/virtio-blk.c |  6 ++--
>  hw/block/onenand.c  |  8 ++---
>  hw/block/tc58128.c  | 44 -
>  hw/bt/core.c| 15 +
>  hw/bt/hci-csr.c

[Qemu-block] [PATCH] hw/block/onenand: Remove dead code block

[Thomas Huth]

The condition of the for-loop makes sure that b is always smaller
than s->blocks, so the "if (b >= s->blocks)" statement is completely
superfluous here.

Buglink: https://bugs.launchpad.net/qemu/+bug/1715007
Signed-off-by: Thomas Huth <th...@redhat.com>
---
 hw/block/onenand.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 30e40f3..de65c9e 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -520,10 +520,6 @@ static void onenand_command(OneNANDState *s)
 s->intstatus |= ONEN_INT;
 
 for (b = 0; b < s->blocks; b ++) {
-if (b >= s->blocks) {
-s->status |= ONEN_ERR_CMD;
-break;
-}
 if (s->blockwp[b] == ONEN_LOCK_LOCKTIGHTEN)
 break;
 
-- 
1.8.3.1

Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory

[Thomas Huth]

On 28.08.2017 18:34, Igor Mammedov wrote:
> Fixes read after freeing error reported
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
>   Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
> 
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
> 
>qdev_device_add()
>  object_property_set_bool('realized', true)
>device_set_realized()
>   ...
>   pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
>   AHCIDevice *ad = >dev[i];
>   ide_bus_new(>port, sizeof(ad->port), qdev, i, 1);
>   ^^^ creates bus in memory allocated by above gnew()
>   and adds it as child propety to ahci device
>   ...
>   hotplug_handler_plug(); -> goto post_realize_fail;
>   pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
>   ...
>g_free(s->dev);
>^^^ free memory that holds children busses
> 
>   return with error from device_set_realized()
> 
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
> 
> Reported-by: Thomas Huth <th...@redhat.com>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
>  hw/ide/ahci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>  
>      ide_exit(s);
>  }
> +object_unparent(OBJECT(>port));
>  }
>  
>  g_free(s->dev);
> 

Thanks, this fixes the problem for me with both, x86_64 and mips64el!

Tested-by: Thomas Huth <th...@redhat.com>

[Qemu-block] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false

[Thomas Huth]

QEMU currently aborts with an assertion message when the user is trying
to remove a dscm1 again:

$ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add dscm1,id=xyz
(qemu) device_del xyz
**
ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
Aborted (core dumped)

Looks like this device has to be wired up in code and is not meant
to be hot-pluggable, so let's mark it with user_creatable = false.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 hw/ide/microdrive.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index e3fd30e..17917c0 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -575,12 +575,15 @@ PCMCIACardState *dscm1_init(DriveInfo *dinfo)
 static void dscm1_class_init(ObjectClass *oc, void *data)
 {
 PCMCIACardClass *pcc = PCMCIA_CARD_CLASS(oc);
+DeviceClass *dc = DEVICE_CLASS(oc);
 
 pcc->cis = dscm1_cis;
 pcc->cis_len = sizeof(dscm1_cis);
 
 pcc->attach = dscm1_attach;
 pcc->detach = dscm1_detach;
+/* Reason: Needs to be wired-up in code, see dscm1_init() */
+dc->user_creatable = false;
 }
 
 static const TypeInfo dscm1_type_info = {
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] isa-fdc: assert replaced by proper error exit

[Thomas Huth]

On 01.09.2017 13:07, Eduardo Otubo wrote:
> When not available, isa-fdc falls into assert instead of proper error
> exit. This patch fixes this behavior.
> 
> Signed-off-by: Eduardo Otubo <ot...@redhat.com>
> ---
>  hw/block/fdc.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 401129073b..0b6def4e1d 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2699,11 +2699,15 @@ static void isabus_fdc_realize(DeviceState *dev, 
> Error **errp)
>  fdctrl->dma_chann = isa->dma;
>  if (fdctrl->dma_chann != -1) {
>  fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
> -assert(fdctrl->dma);
> +if (!fdctrl->dma) {
> +error_setg(errp, "isa-fdc not supported");
> +goto error;
> +}
>  }
>  
>  qdev_set_legacy_instance_id(dev, isa->iobase, 2);
>  fdctrl_realize_common(dev, fdctrl, );
> +error:
>  if (err != NULL) {
>  error_propagate(errp, err);
>      return;

Maybe add the reproducer to the commit message:

 qemu-system-ppc64 -S -machine powernv -device isa-fdc

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all virtio test
> functionality to pass in an explicit QTestState in constructors,
> where it is then reused for later access.  Adjust all callers.
> This gets us one step closer to eliminating implicit use of
> global_qtest.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/virtio-mmio.h |  3 +-
>  tests/libqos/virtio.h  |  5 ++-
>  tests/libqos/virtio-mmio.c | 54 +---
>  tests/libqos/virtio-pci.c  |  3 ++
>  tests/libqos/virtio.c  | 77 
> ++
>  tests/virtio-blk-test.c|  3 +-
>  6 files changed, 84 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/libqos/virtio-mmio.h b/tests/libqos/virtio-mmio.h
> index e3e52b9ce1..bd01386054 100644
> --- a/tests/libqos/virtio-mmio.h
> +++ b/tests/libqos/virtio-mmio.h
> @@ -41,6 +41,7 @@ typedef struct QVirtioMMIODevice {
> 
>  extern const QVirtioBus qvirtio_mmio;
> 
> -QVirtioMMIODevice *qvirtio_mmio_init_device(uint64_t addr, uint32_t 
> page_size);
> +QVirtioMMIODevice *qvirtio_mmio_init_device(QTestState *qts, uint64_t addr,
> +uint32_t page_size);
> 
>  #endif
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> index 8fbcd1869c..d180d54fc4 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -19,6 +19,7 @@ typedef struct QVirtioBus QVirtioBus;
> 
>  typedef struct QVirtioDevice {
>  const QVirtioBus *bus;
> +QTestState *qts;
>  /* Device type */
>  uint16_t device_type;
>  } QVirtioDevice;
> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
>  uint16_t last_used_idx;
>  bool indirect;
>  bool event;
> +QTestState *qts;
>  } QVirtQueue;
> 
>  typedef struct QVRingIndirectDesc {
>  uint64_t desc; /* This points to an array fo struct vring_desc */
>  uint16_t index;
>  uint16_t elem;
> +QTestState *qts;
>  } QVRingIndirectDesc;

Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
ugly to me. I think they should either rather have a pointer to the
associated QVirtioDevice, or the functions where this is needed
(qvring_init() for example) should get a "QTestState *" parameter instead.

Just my 0.02 €.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 17/29] ahci-test: Drop dependence on global_qtest

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Managing parallel connections to two different monitors via
> the implicit global_qtest makes it hard to copy-and-paste code
> to tests that are not aware of the implicit state; the
> management of global_qtest is even harder to follow because
> it was masked behind set_context().
> 
> Instead, explicitly pass QTestState* around (generally, by
> reusing the member already present in ahci->parent QOSState),
> and call explicit qtest_* functions on all places that
> interact with a monitor.
> 
> We can assert that the conversion is correct by checking that
> global_qtest remains NULL throughout the test (a later patch
> that changes global_qtest to not be a public global variable
> will drop the assertions).
> 
> Bonus: there were several spots that were constructing a JSON
> string, then passing that through qmp() as the format, rather
> than directly using qmp() to construct the JSON.  Fixing that
> gets us one step closer to enabling -Wformat checking on
> constructed JSON.
> 
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>  tests/libqos/libqos.h|  1 -
>  tests/ahci-test.c| 83 
> +++-
>  tests/libqos/ahci.c  | 45 +-
>  tests/libqos/libqos-pc.c |  2 +-
>  tests/libqos/libqos.c| 37 ++---
>  5 files changed, 73 insertions(+), 95 deletions(-)

Might be easier to review if you'd split the changes to libqos.c into a
separate patch. But anyway:

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/29] libqos: Track QTestState with QPCIBus

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> When initializing a QPCIBus, track which QTestState the bus is
> associated with (so that a later patch can then explicitly use
> that test state for all communication on the bus, rather than
> blindly relying on global_qtest).  Update the initialization
> functions to take another parameter, and update all callers to
> pass in state (for now, most callers get away with passing the
> current global_qtest as the current state, although this required
> fixing the order of initialization to ensure qtest_start() is
> called before qpci_init*() in rtl8139-test, and provided an
> opportunity to pass in the allocator in e1000e-test).
> 
> Signed-off-by: Eric Blake 
> ---
[...]
> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
> index 6226546c28..c95428e1cb 100644
> --- a/tests/libqos/libqos.c
> +++ b/tests/libqos/libqos.c
> @@ -26,8 +26,8 @@ QOSState *qtest_vboot(QOSOps *ops, const char *cmdline_fmt, 
> va_list ap)
>  if (ops->init_allocator) {
>  qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
>  }
> -if (ops->qpci_init && qs->alloc) {
> -qs->pcibus = ops->qpci_init(qs->alloc);
> +if (ops->qpci_init) {

Why did you remove the check for qs->alloc?

> +qs->pcibus = ops->qpci_init(qs->qts, qs->alloc);
>  }
>  }
> 
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 02ce49927a..85b34c6d13 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -115,11 +115,14 @@ static void qpci_pc_config_writel(QPCIBus *bus, int 
> devfn, uint8_t offset, uint3
>  outl(0xcfc, value);
>  }
> 
> -QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>  {
>  QPCIBusPC *ret;
> 
> +assert(qts);
> +
>  ret = g_malloc(sizeof(*ret));
> +ret->bus.qts = qts;
> 
>  ret->bus.pio_readb = qpci_pc_pio_readb;
>  ret->bus.pio_readw = qpci_pc_pio_readw;
> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> index 2043f1e123..cd9b8f52d2 100644
> --- a/tests/libqos/pci-spapr.c
> +++ b/tests/libqos/pci-spapr.c
> @@ -154,11 +154,14 @@ static void qpci_spapr_config_writel(QPCIBus *bus, int 
> devfn, uint8_t offset,
>  #define SPAPR_PCI_MMIO32_WIN_SIZE0x8000 /* 2 GiB */
>  #define SPAPR_PCI_IO_WIN_SIZE0x1
> 
> -QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
> +QPCIBus *qpci_init_spapr(QTestState *qts, QGuestAllocator *alloc)
>  {
>  QPCIBusSPAPR *ret;
> 
> +assert(qts);
> +
>  ret = g_malloc(sizeof(*ret));

+1 for using g_malloc0 here instead.

> +ret->bus.qts = qts;
> 
>  ret->alloc = alloc;
> 
> @@ -201,6 +204,7 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
>  ret->bus.mmio_alloc_ptr = ret->mmio32.pci_base;
>  ret->bus.mmio_limit = ret->mmio32.pci_base + ret->mmio32.size;
> 
> +

Superfluous white space change.

>  return >bus;
>  }

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all fw_cfg test
> functionality (invoked through alloc-pc) to pass in an explicit
> QTestState, adjusting all callers.  In particular, fw_cfg-test
> had to reorder things to create the test state prior to creating
> the fw_cfg.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/fw_cfg.h   | 10 ++
>  tests/libqos/libqos.h   |  2 +-
>  tests/libqos/malloc-pc.h|  4 ++--
>  tests/libqos/malloc-spapr.h |  2 +-
>  tests/libqos/malloc.h   |  1 +
>  tests/boot-order-test.c |  6 +++---
>  tests/e1000e-test.c |  2 +-
>  tests/fw_cfg-test.c | 14 ++
>  tests/ide-test.c|  2 +-
>  tests/libqos/fw_cfg.c   | 14 --
>  tests/libqos/libqos.c   |  2 +-
>  tests/libqos/malloc-pc.c|  8 
>  tests/libqos/malloc-spapr.c |  4 ++--
>  tests/vhost-user-test.c |  2 +-
>  14 files changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index e8371b2317..396dd4ee1e 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -15,10 +15,12 @@
> 
> 
>  typedef struct QFWCFG QFWCFG;
> +typedef struct QTestState QTestState;

Not sure, but I slightly remember that typedeffing a struct like this in
multiple places can cause compiler warnings or errors with certain
versions of GCC or clang? So a file that includes both, fw_cfg.h and
libqtest.h will then fail to compile?

I think it would be better to change the include order in the .c files
instead, so that libqtest.h is always included before fw_cfg.h.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 05.09.2017 12:12, Thomas Huth wrote:
> On 01.09.2017 20:03, Eric Blake wrote:
>> Drop one more client of global_qtest by teaching all fw_cfg test
>> functionality (invoked through alloc-pc) to pass in an explicit
>> QTestState, adjusting all callers.  In particular, fw_cfg-test
>> had to reorder things to create the test state prior to creating
>> the fw_cfg.
>>
>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>> ---
>>  tests/libqos/fw_cfg.h   | 10 ++
>>  tests/libqos/libqos.h   |  2 +-
>>  tests/libqos/malloc-pc.h|  4 ++--
>>  tests/libqos/malloc-spapr.h |  2 +-
>>  tests/libqos/malloc.h   |  1 +
>>  tests/boot-order-test.c |  6 +++---
>>  tests/e1000e-test.c |  2 +-
>>  tests/fw_cfg-test.c | 14 ++
>>  tests/ide-test.c|  2 +-
>>  tests/libqos/fw_cfg.c   | 14 --
>>  tests/libqos/libqos.c   |  2 +-
>>  tests/libqos/malloc-pc.c|  8 
>>  tests/libqos/malloc-spapr.c |  4 ++--
>>  tests/vhost-user-test.c |  2 +-
>>  14 files changed, 38 insertions(+), 35 deletions(-)
>>
>> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
>> index e8371b2317..396dd4ee1e 100644
>> --- a/tests/libqos/fw_cfg.h
>> +++ b/tests/libqos/fw_cfg.h
>> @@ -15,10 +15,12 @@
>>
>>
>>  typedef struct QFWCFG QFWCFG;
>> +typedef struct QTestState QTestState;
> 
> Not sure, but I slightly remember that typedeffing a struct like this in
> multiple places can cause compiler warnings or errors with certain
> versions of GCC or clang? So a file that includes both, fw_cfg.h and
> libqtest.h will then fail to compile?
> 
> I think it would be better to change the include order in the .c files
> instead, so that libqtest.h is always included before fw_cfg.h.

Ah, well, I just saw that you also sent a fixup patch for this. Anyway,
I'm not a fan of including header files from other header files, so
changing the include order in the .c files sounds like the better
solution to me.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 28/29] libqtest: Remove qtest_start() and qtest_end() shortcuts

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Remove the trivial wrappers qtest_start() and qtest_end(), to make
> it obvious in the rest of the testsuite where we are still relying on
> global_qtest.  Doing this makes it easier to see what remaining
> cleanups will be needed if we don't want an implicit dependency
> on global state.  Many tests can also take advantage of qtest_init()
> doing formatting of args, avoiding a temporary local variable.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqtest.h   | 26 --
>  tests/libqtest.c   |  4 +--
>  tests/ac97-test.c  |  4 +--
>  tests/boot-order-test.c| 11 +++-
>  tests/boot-serial-test.c   | 12 +++--
>  tests/device-introspect-test.c | 24 -
>  tests/display-vga-test.c   | 18 +
>  tests/drive_del-test.c | 17 ++--
>  tests/ds1338-test.c|  2 +-
>  tests/e1000-test.c | 10 ++-
>  tests/e1000e-test.c| 14 +++---
>  tests/eepro100-test.c  | 11 ++--
>  tests/endianness-test.c| 33 +--
>  tests/es1370-test.c|  4 +--
>  tests/fdc-test.c   |  4 +--
>  tests/hd-geo-test.c| 16 +--
>  tests/i440fx-test.c| 16 +--
>  tests/i82801b11-test.c |  5 ++--
>  tests/ide-test.c   |  4 +--
>  tests/intel-hda-test.c | 11 
>  tests/ioh3420-test.c   |  7 ++---
>  tests/ipmi-bt-test.c   | 11 +++-
>  tests/ipmi-kcs-test.c  |  5 +---
>  tests/ipoctal232-test.c|  5 ++--
>  tests/ivshmem-test.c   |  4 +--
>  tests/libqos/libqos.c  |  2 +-
>  tests/m25p80-test.c|  9 +++
>  tests/m48t59-test.c|  2 +-
>  tests/ne2000-test.c|  4 +--
>  tests/numa-test.c  | 28 ++--
>  tests/nvme-test.c  |  7 ++---
>  tests/pc-cpu-test.c| 26 +++---
>  tests/pcnet-test.c |  4 +--
>  tests/pnv-xscom-test.c | 14 ++
>  tests/prom-env-test.c  | 13 -
>  tests/pvpanic-test.c   |  4 +--
>  tests/pxe-test.c   | 14 --
>  tests/q35-test.c   |  8 +++---
>  tests/qom-test.c   |  7 ++---
>  tests/rtc-test.c   |  2 +-
>  tests/rtl8139-test.c   |  4 +--
>  tests/spapr-phb-test.c |  5 ++--
>  tests/tco-test.c   | 12 -
>  tests/test-arm-mptimer.c   |  4 +--
>  tests/test-filter-mirror.c | 16 +--
>  tests/test-filter-redirector.c | 60 
> --
>  tests/test-hmp.c   |  7 ++---
>  tests/test-netfilter.c |  9 +++
>  tests/test-x86-cpuid-compat.c  | 13 -
>  tests/tmp105-test.c|  2 +-
>  tests/tpci200-test.c   |  4 +--
>  tests/usb-hcd-ehci-test.c  | 25 +-
>  tests/usb-hcd-ohci-test.c  |  4 +--
>  tests/usb-hcd-xhci-test.c  |  4 +--
>  tests/virtio-balloon-test.c|  4 +--
>  tests/virtio-blk-test.c| 13 -
>  tests/virtio-console-test.c|  8 +++---
>  tests/virtio-net-test.c|  4 +--
>  tests/virtio-rng-test.c|  4 +--
>  tests/virtio-serial-test.c |  4 +--
>  tests/vmgenid-test.c   | 29 ++--
>  tests/vmxnet3-test.c   |  4 +--
>  62 files changed, 267 insertions(+), 394 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index d338de3e22..0459526187 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -509,32 +509,6 @@ void qtest_add_data_func_full(const char *str, void 
> *data,
>  void qtest_add_abrt_handler(GHookFunc fn, const void *data);
> 
>  /**
> - * qtest_start:
> - * @args: other arguments to pass to QEMU
> - *
> - * Start QEMU and assign the resulting #QTestState to a global variable.
> - * The global variable is used by "shortcut" functions documented below.
> - *
> - * Returns: #QTestState instance.
> - */
> -static inline QTestState *qtest_start(const char *args)
> -{
> -global_qtest = qtest_init("%s", args);
> -return global_qtest;
> -}
> -
> -/**
> - * qtest_end:
> - *
> - * Shut down the QEMU process started by qtest_start().
> - */
> -static inline void qtest_end(void)
> -{
> -qtest_quit(global_qtest);
> -global_qtest = NULL;
> -}
> -
> -/**
>   * qmp:
>   * @fmt...: QMP message to send to qemu
>   *
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 18facbf130..fa4e47c137 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -970,7 +970,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
> *machine))
>  QString *qstr;
>  const char *mname;
> 
> -qtest_start("-machine none");
> +global_qtest = qtest_init("-machine none");
>  response = qmp("{ 'execute': 'query-machines' }");
>  g_assert(response);
>  list = 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 29/29] libqtest: Rename qtest_init() to qtest_start()

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> We already have another qtest_init() in the tree, for the
> top-level qtest.c device; having two functions with different
> signatures is confusing.  Rename the libqtest version to
> qtest_start() to eliminate the duplication.

This is too much code churn for my taste, and I also do not like the
idea of naming the function qtest_start() - since this was a function
with different semantics before your patch 28/29, so this will cause
confusion for all the people who are used to the old qtest_start()
function or who want to backport patches that have done after this
change to a code level before this change.

If you are really bugged by the qtest_init() name clash, I think it's
way easier if you rename the qtest_init() in the qtest.c file instead.

 Thomas

Re: [Qemu-block] [PATCH 61/88] tests: use g_new() family of functions

[Thomas Huth]

On 07.10.2017 01:49, Philippe Mathieu-Daudé wrote:
> From: Marc-André Lureau <marcandre.lur...@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> [PMD: split of some files in other commits of the same series, add libqtest.c]
> ---
>  tests/ahci-test.c | 4 ++--
>  tests/fw_cfg-test.c   | 4 ++--
>  tests/libqos/ahci.c   | 2 +-
>  tests/libqos/libqos.c | 2 +-
>  tests/libqos/malloc.c | 6 +++---
>  tests/libqtest.c  | 2 +-
>  tests/pc-cpu-test.c   | 2 +-
>  7 files changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [Qemu-block] [Qemu-devel] [PATCH v3 42/46] util: Replace fprintf(stderr, "*\n" with error_report()

[Thomas Huth]

On 19.10.2017 18:18, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
[...]
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 5946ac09f0..29fff51fcf 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -15,6 +15,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +#include "qemu/error-report.h"
>  #include "block/block.h"
>  #include "qemu/rcu_queue.h"
>  #include "qemu/sockets.h"
> @@ -703,8 +704,8 @@ void aio_context_setup(AioContext *ctx)
>  {
>  /* TODO remove this in final patch submission */
>  if (getenv("QEMU_AIO_POLL_MAX_NS")) {
> -fprintf(stderr, "The QEMU_AIO_POLL_MAX_NS environment variable has "
> -"been replaced with -object iothread,poll-max-ns=NUM\n");
> +error_report("The QEMU_AIO_POLL_MAX_NS environment variable has "
> +"been replaced with -object iothread,poll-max-ns=NUM");
>  exit(1);
>  }

The comment in front of this code block indicates that this should
rather be removed completely. Stefan, do you agree?

> diff --git a/util/error.c b/util/error.c
> index 3efdd69162..e423368ca0 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -32,7 +32,7 @@ Error *error_fatal;
>  static void error_handle_fatal(Error **errp, Error *err)
>  {
>  if (errp == _abort) {
> -fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> +error_report("Unexpected error in %s() at %s:%d:",
>  err->func, err->src, err->line);

Indentation is still wrong if the statement spans more than one line :-(

 Thomas

[Qemu-block] [PATCH] hw/ide/ahci: Move allwinner code into a separate file

[Thomas Huth]

The allwinner code is only needed for the allwinner board (for which
we also have a separate CONFIG_ALLWINNER_A10 config switch), so it
does not make sense that we compile this for all the other boards
that need AHCI, too. Let's move it to a separate file that is only
compiled when CONFIG_ALLWINNER_A10 is set.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 hw/ide/Makefile.objs|   1 +
 hw/ide/ahci-allwinner.c | 127 
 hw/ide/ahci.c   |  95 
 3 files changed, 128 insertions(+), 95 deletions(-)
 create mode 100644 hw/ide/ahci-allwinner.c

diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs
index 729e9bd..f0edca3 100644
--- a/hw/ide/Makefile.objs
+++ b/hw/ide/Makefile.objs
@@ -10,3 +10,4 @@ common-obj-$(CONFIG_IDE_VIA) += via.o
 common-obj-$(CONFIG_MICRODRIVE) += microdrive.o
 common-obj-$(CONFIG_AHCI) += ahci.o
 common-obj-$(CONFIG_AHCI) += ich.o
+common-obj-$(CONFIG_ALLWINNER_A10) += ahci-allwinner.o
diff --git a/hw/ide/ahci-allwinner.c b/hw/ide/ahci-allwinner.c
new file mode 100644
index 000..c3f1604
--- /dev/null
+++ b/hw/ide/ahci-allwinner.c
@@ -0,0 +1,127 @@
+/*
+ * QEMU Allwinner AHCI Emulation
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "qemu/error-report.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/dma.h"
+#include "hw/ide/internal.h"
+#include "hw/ide/ahci_internal.h"
+
+#include "trace.h"
+
+#define ALLWINNER_AHCI_BISTAFR((0xa0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTCR ((0xa4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTFCTR   ((0xa8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTSR ((0xac - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_BISTDECR   ((0xb0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_DIAGNR0((0xb4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_DIAGNR1((0xb8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_OOBR   ((0xbc - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PHYCS0R((0xc0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PHYCS1R((0xc4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PHYCS2R((0xc8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_TIMER1MS   ((0xe0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_GPARAM1R   ((0xe8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_GPARAM2R   ((0xec - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_PPARAMR((0xf0 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_TESTR  ((0xf4 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_VERSIONR   ((0xf8 - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_IDR((0xfc - ALLWINNER_AHCI_MMIO_OFF) / 4)
+#define ALLWINNER_AHCI_RWCR   ((0xfc - ALLWINNER_AHCI_MMIO_OFF) / 4)
+
+static uint64_t allwinner_ahci_mem_read(void *opaque, hwaddr addr,
+unsigned size)
+{
+AllwinnerAHCIState *a = opaque;
+AHCIState *s = &(SYSBUS_AHCI(a)->ahci);
+uint64_t val = a->regs[addr / 4];
+
+switch (addr / 4) {
+case ALLWINNER_AHCI_PHYCS0R:
+val |= 0x2 << 28;
+break;
+case ALLWINNER_AHCI_PHYCS2R:
+val &= ~(0x1 << 24);
+break;
+}
+trace_allwinner_ahci_mem_read(s, a, addr, val, size);
+return  val;
+}
+
+static void allwinner_ahci_mem_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+AllwinnerAHCIState *a = opaque;
+AHCIState *s = &(SYSBUS_AHCI(a)->ahci);
+
+trace_allwinner_ahci_mem_write(s, a, addr, val, size);
+a->regs[addr / 4] = val;
+}
+
+static const MemoryRegionOps allwinner_ahci_mem_ops = {
+.read = allwinner_ahci_mem_read,
+.write = allwinner_ahci_mem_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void allwinner_ahci_init(Object *obj)
+{
+SysbusAHCIState *s = SYSBUS_AHCI(obj);
+AllwinnerAHCIState *a = ALLWINNER_AHCI(obj);
+
+memory_region_init_io(>mmio, OBJECT(obj), _ahci_mem_ops, a,
+  "allwinner-ahci&

Re: [Qemu-block] [Qemu-devel] [PATCH] scripts/make-release: No need to delete pixman/.git anymore

[Thomas Huth]

On 13.11.2017 10:43, no-re...@patchew.org wrote:
> Hi,
> 
> This series failed automatic build test. Please find the testing commands and
> their output below. If you have docker installed, you can probably reproduce 
> it
> locally.
> 
> Subject: [Qemu-devel] [PATCH] scripts/make-release: No need to delete 
> pixman/.git anymore
> Type: series
> Message-id: 1510564905-6185-1-git-send-email-th...@redhat.com
[...]
> QEMU  -- "/tmp/qemu-test/build/x86_64-softmmu/qemu-system-x86_64" 
> -nodefaults -machine accel=qtest
> QEMU_IMG  -- "/tmp/qemu-test/build/qemu-img" 
> QEMU_IO   -- "/tmp/qemu-test/build/qemu-io"  --cache writeback -f raw
> QEMU_NBD  -- "/tmp/qemu-test/build/qemu-nbd" 
> IMGFMT-- raw
> IMGPROTO  -- file
> PLATFORM  -- Linux/x86_64 fe9889ba94a0 4.11.10-300.fc26.x86_64
> TEST_DIR  -- /tmp/qemu-test
> SOCKET_SCM_HELPER -- /tmp/qemu-test/build/tests/qemu-iotests/socket_scm_helper
> 
> 001
> 002
> 004
> 005
> 008
> 009
> 010
> 011
> 012
> 017 [not run] not suitable for this image format: raw
> 018 [not run] not suitable for this image format: raw
> 019 [not run] not suitable for this image format: raw
> 020 [not run] not suitable for this image format: raw
> 021
> 024 [not run] not suitable for this image format: raw
> 025
> 027 [not run] not suitable for this image format: raw
> 028 [not run] not suitable for this image format: raw
> 029 [not run] not suitable for this image format: raw
> 031 [not run] not suitable for this image format: raw
> 032
> 033
> 034 [not run] not suitable for this image format: raw
> 035 [not run] not suitable for this image format: raw
> 036 [not run] not suitable for this image format: raw
> 037 [not run] not suitable for this image format: raw
> 038 [not run] not suitable for this image format: raw
> 039 [not run] not suitable for this image format: raw
> 042 [not run] not suitable for this image format: raw
> 045
> 046 [not run] not suitable for this image format: raw
> 047 [not run] not suitable for this image format: raw
> 048
> 050 [not run] not suitable for this image format: raw
> 052
> 053 [not run] not suitable for this image format: raw
> 054 [not run] not suitable for this image format: raw
> 058 [not run] not suitable for this image format: raw
> 059 [not run] not suitable for this image format: raw
> 060 [not run] not suitable for this image format: raw
> 062 [not run] not suitable for this image format: raw
> 063
> 064 [not run] not suitable for this image format: raw
> 065 [not run] not suitable for this image format: raw
> 066 [not run] not suitable for this image format: raw
> 067 [not run] not suitable for this image format: raw
> 068 [not run] not suitable for this image format: raw
> 069 [not run] not suitable for this image format: raw
> 070 [not run] not suitable for this image format: raw
> 071 [not run] not suitable for this image format: raw
> 072 [not run] not suitable for this image format: raw
> 073 [not run] not suitable for this image format: raw
> 074 [not run] not suitable for this image format: raw
> 075 [not run] not suitable for this image format: raw
> 077 - output mismatch (see 077.out.bad)
> --- /tmp/qemu-test/src/tests/qemu-iotests/077.out 2017-11-13 
> 09:38:59.0 +
> +++ /tmp/qemu-test/build/tests/qemu-iotests/077.out.bad   2017-11-13 
> 09:41:20.601278693 +
> @@ -56,9 +56,9 @@
>  wrote XXX/XXX bytes at offset XXX
>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  blkdebug: Resuming request 'B'
> +blkdebug: Resuming request 'A'
>  wrote XXX/XXX bytes at offset XXX
>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -blkdebug: Resuming request 'A'
>  wrote XXX/XXX bytes at offset XXX
>  XXX bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  wrote XXX/XXX bytes at offset XXX
> 078 [not run] not suitable for this image format: raw
> 081
> 082 [not run] not suitable for this image format: raw
> 084 [not run] not suitable for this image format: raw
> 086
> 087 [not run] not suitable for this image format: raw
> 088 [not run] not suitable for this image format: raw
> 089 [not run] not suitable for this image format: raw
> 090 [not run] not suitable for this image format: raw
> 092 [not run] not suitable for this image format: raw
> 094 [not run] not suitable for this image protocol: file
> 095 [not run] not suitable for this image format: raw
> 096 [not run] not suitable for this image format: raw
> 098  

[Qemu-block] [PATCH v2] hw/ide: Remove duplicated definitions from ahci_internal.h

[Thomas Huth]

The same definitions can also be found in include/hw/ide/ahci.h
so let's remove these #defines from ahci_internal.h.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 v2: Also remove TYPE_ICH9_AHCI as suggested by John

 hw/ide/ahci_internal.h | 12 
 1 file changed, 12 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index ce2e818..e3e3ed2 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -311,11 +311,6 @@ struct AHCIPCIState {
 AHCIState ahci;
 };
 
-#define TYPE_ICH9_AHCI "ich9-ahci"
-
-#define ICH_AHCI(obj) \
-OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH9_AHCI)
-
 extern const VMStateDescription vmstate_ahci;
 
 #define VMSTATE_AHCI(_field, _state) {   \
@@ -375,11 +370,4 @@ void ahci_uninit(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
-#define TYPE_SYSBUS_AHCI "sysbus-ahci"
-#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), TYPE_SYSBUS_AHCI)
-
-#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
-#define ALLWINNER_AHCI(obj) OBJECT_CHECK(AllwinnerAHCIState, (obj), \
-   TYPE_ALLWINNER_AHCI)
-
 #endif /* HW_IDE_AHCI_H */
-- 
1.8.3.1

[Qemu-block] [PATCH 2/3] block: Remove the deprecated -hdachs option

[Thomas Huth]

It's been marked as deprecated since QEMU v2.10.0, and so far nobody
complained that we should keep it, so let's remove this legacy option
now to simplify the code quite a bit.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 qemu-doc.texi   |  8 --
 qemu-options.hx | 19 ++---
 vl.c| 86 ++---
 3 files changed, 4 insertions(+), 109 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index e313bf1..af495ad 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2475,14 +2475,6 @@ The ``--net dump'' argument is now replaced with the
 ``-object filter-dump'' argument which works in combination
 with the modern ``-netdev`` backends instead.
 
-@subsection -hdachs (since 2.10.0)
-
-The ``-hdachs'' argument is now a synonym for setting
-the ``cyls'', ``heads'', ``secs'', and ``trans'' properties
-on the ``ide-hd'' device using the ``-device'' argument.
-The new syntax allows different settings to be provided
-per disk.
-
 @subsection -usbdevice (since 2.10.0)
 
 The ``-usbdevice DEV'' argument is now a synonym for setting
diff --git a/qemu-options.hx b/qemu-options.hx
index 32d9378..828546c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -846,8 +846,8 @@ of available connectors of a given interface type.
 @item media=@var{media}
 This option defines the type of the media: disk or cdrom.
 @item cyls=@var{c},heads=@var{h},secs=@var{s}[,trans=@var{t}]
-These options have the same definition as they have in @option{-hdachs}.
-These parameters are deprecated, use the corresponding parameters
+Force disk physical geometry and the optional BIOS translation (trans=none or
+lba). These parameters are deprecated, use the corresponding parameters
 of @code{-device} instead.
 @item snapshot=@var{snapshot}
 @var{snapshot} is "on" or "off" and controls snapshot mode for the given drive
@@ -1027,21 +1027,6 @@ the raw disk image you use is not written back. You can 
however force
 the write back by pressing @key{C-a s} (@pxref{disk_images}).
 ETEXI
 
-DEF("hdachs", HAS_ARG, QEMU_OPTION_hdachs, \
-"-hdachs c,h,s[,t]\n" \
-"force hard disk 0 physical geometry and the optional 
BIOS\n" \
-"translation (t=none or lba) (usually QEMU can guess 
them)\n",
-QEMU_ARCH_ALL)
-STEXI
-@item -hdachs @var{c},@var{h},@var{s},[,@var{t}]
-@findex -hdachs
-Force hard disk 0 physical geometry (1 <= @var{c} <= 16383, 1 <=
-@var{h} <= 16, 1 <= @var{s} <= 63) and optionally force the BIOS
-translation mode (@var{t}=none, lba or auto). Usually QEMU can guess
-all those parameters. This option is deprecated, please use
-@code{-device ide-hd,cyls=c,heads=h,secs=s,...} instead.
-ETEXI
-
 DEF("fsdev", HAS_ARG, QEMU_OPTION_fsdev,
 "-fsdev 
fsdriver,id=id[,path=path,][security_model={mapped-xattr|mapped-file|passthrough|none}]\n"
 " 
[,writeout=immediate][,readonly][,socket=socket|sock_fd=sock_fd][,fmode=fmode][,dmode=dmode]\n"
diff --git a/vl.c b/vl.c
index e9012bb..a797243 100644
--- a/vl.c
+++ b/vl.c
@@ -3052,9 +3052,8 @@ int main(int argc, char **argv, char **envp)
 const char *boot_order = NULL;
 const char *boot_once = NULL;
 DisplayState *ds;
-int cyls, heads, secs, translation;
 QemuOpts *opts, *machine_opts;
-QemuOpts *hda_opts = NULL, *icount_opts = NULL, *accel_opts = NULL;
+QemuOpts *icount_opts = NULL, *accel_opts = NULL;
 QemuOptsList *olist;
 int optind;
 const char *optarg;
@@ -3146,8 +3145,6 @@ int main(int argc, char **argv, char **envp)
 
 cpu_model = NULL;
 snapshot = 0;
-cyls = heads = secs = 0;
-translation = BIOS_ATA_TRANSLATION_AUTO;
 
 nb_nics = 0;
 
@@ -3186,7 +3183,7 @@ int main(int argc, char **argv, char **envp)
 if (optind >= argc)
 break;
 if (argv[optind][0] != '-') {
-hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS);
+drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS);
 } else {
 const QEMUOption *popt;
 
@@ -3206,21 +3203,6 @@ int main(int argc, char **argv, char **envp)
 cpu_model = optarg;
 break;
 case QEMU_OPTION_hda:
-{
-char buf[256];
-if (cyls == 0)
-snprintf(buf, sizeof(buf), "%s", HD_OPTS);
-else
-snprintf(buf, sizeof(buf),
- "%s,cyls=%d,heads=%d,secs=%d%s",
- HD_OPTS , cyls, heads, secs,
- translation == BIOS_ATA_TRANSLATION_LBA ?
- ",trans=lba" :
- translation == BIOS_ATA_TRANSLATION_NONE ?
- ",trans=none" : "&

[Qemu-block] [PATCH 1/3] block: Remove the obsolete -drive boot=on|off parameter

[Thomas Huth]

It's not working anymore since QEMU v1.3.0 - time to remove it now.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 blockdev.c| 11 ---
 qemu-doc.texi |  6 --
 2 files changed, 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 56a6b24..c21ba27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -734,10 +734,6 @@ QemuOptsList qemu_legacy_drive_opts = {
 .type = QEMU_OPT_STRING,
 .help = "chs translation (auto, lba, none)",
 },{
-.name = "boot",
-.type = QEMU_OPT_BOOL,
-.help = "(deprecated, ignored)",
-},{
 .name = "addr",
 .type = QEMU_OPT_STRING,
 .help = "pci address (virtio only)",
@@ -872,13 +868,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 goto fail;
 }
 
-/* Deprecated option boot=[on|off] */
-if (qemu_opt_get(legacy_opts, "boot") != NULL) {
-fprintf(stderr, "qemu-kvm: boot=on|off is deprecated and will be "
-"ignored. Future versions will reject this parameter. Please "
-"update your scripts.\n");
-}
-
 /* Other deprecated options */
 if (!qtest_enabled()) {
 for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
diff --git a/qemu-doc.texi b/qemu-doc.texi
index f7317df..e313bf1 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2373,12 +2373,6 @@ deprecated.
 
 @section System emulator command line arguments
 
-@subsection -drive boot=on|off (since 1.3.0)
-
-The ``boot=on|off'' option to the ``-drive'' argument is
-ignored. Applications should use the ``bootindex=N'' parameter
-to set an absolute ordering between devices instead.
-
 @subsection -tdf (since 1.3.0)
 
 The ``-tdf'' argument is ignored. The behaviour implemented
-- 
1.8.3.1

[Qemu-block] [PATCH 3/3] block: Mention -drive cyls/heads/secs/trans/serial/addr in deprecation chapter

[Thomas Huth]

Looks like we forgot to announce the deprecation of these options in
the corresponding chapter of the qemu-doc text, so let's do that now.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 qemu-doc.texi | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index af495ad..f6afe01 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2469,6 +2469,21 @@ longer be directly supported in QEMU.
 The ``-drive if=scsi'' argument is replaced by the the
 ``-device BUS-TYPE'' argument combined with ``-drive if=none''.
 
+@subsection -drive cyls=...,heads=...,secs=...,trans=... (since 2.10.0)
+
+The drive geometry arguments are replaced by the the geometry arguments
+that can be specified with the ``-device'' parameter.
+
+@subsection -drive serial=... (since 2.10.0)
+
+The drive serial argument is replaced by the the serial argument
+that can be specified with the ``-device'' parameter.
+
+@subsection -drive addr=... (since 2.10.0)
+
+The drive addr argument is replaced by the the addr argument
+that can be specified with the ``-device'' parameter.
+
 @subsection -net dump (since 2.10.0)
 
 The ``--net dump'' argument is now replaced with the
-- 
1.8.3.1

[Qemu-block] [PATCH 0/3] block: Deprecated options

[Thomas Huth]

Remove the deprecated "-drive boot" and "-hdachs" options and properly
mark some other deprecated options in the deprecation chapter.

Thomas Huth (3):
  block: Remove the obsolete -drive boot=on|off parameter
  block: Remove the deprecated -hdachs option
  block: Mention -drive cyls/heads/secs/trans/serial/addr in deprecation
chapter

 blockdev.c  | 11 
 qemu-doc.texi   | 29 +--
 qemu-options.hx | 19 ++---
 vl.c| 86 ++---
 4 files changed, 19 insertions(+), 126 deletions(-)

-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] using "qemu-img convert -O qcow2" to convert qcow v1 to v2 creates a qcow v3 file?

[Thomas Huth]

On 14.11.2017 14:32, Max Reitz wrote:
[...]
> Well, do you want to document it?  I'd rather deprecate it altogether.

Maybe a first step could be to change qemu-img so that it refuses to
create new qcow1 images (but still can convert them into other formats).
So basically make qcow1 read-only?

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2] hw/ide: Remove duplicated definitions from ahci_internal.h

[Thomas Huth]

On 06.12.2017 23:16, John Snow wrote:
> I tweaked this again, sorry:
> 
> The names need to stay public, but the wrappers to manipulate the
> objects can stay internal. Minor difference.
> 
> If that's okay, I'll just merge this in.
> OK?

Sure. Feel also free to replace my "Signed-off-by" with "Reported-by" in
that case if you like.

 Thomas

[Qemu-block] [PATCH for-2.12] hw/ide: Remove duplicated definitions from ahci_internal.h

[Thomas Huth]

The same definitions can also be found in include/hw/ide/ahci.h
so let's remove these #defines from ahci_internal.h.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 hw/ide/ahci_internal.h | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index ce2e818..1080a34 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -375,11 +375,4 @@ void ahci_uninit(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
-#define TYPE_SYSBUS_AHCI "sysbus-ahci"
-#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), TYPE_SYSBUS_AHCI)
-
-#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
-#define ALLWINNER_AHCI(obj) OBJECT_CHECK(AllwinnerAHCIState, (obj), \
-   TYPE_ALLWINNER_AHCI)
-
 #endif /* HW_IDE_AHCI_H */
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] qemu-img crash when resize a qcow2 file created with preallocation=full/falloc .

[Thomas Huth]

On 24.10.2017 05:28, Changlimin wrote:
> Hi,
> I am glad to see that qcow2 file created with preallocation=full/falloc can 
> be resized. But when I test it, qemu-img crashs.
> qemu-img: block/qcow2-refcount.c:530: qcow2_refcount_area: Assertion 
> `!(start_offset % s->cluster_size)' failed.
> 
> These are commands:
> qemu-img create -f qcow2 -o preallocation=full full.img 1G
> qemu-img resize --preallocation=full full.img +1G

Which version of qemu-img are you using? v2.10 or the latest git master
branch?

 Thomas

Re: [Qemu-block] [Qemu-devel] [Bug 1728615] [NEW] qemu-io crashes with SIGABRT and Assertion `c->entries[i].offset != 0' failed

[Thomas Huth]

On 30.10.2017 15:43, R.Nageswara Sastry wrote:
> Public bug reported:
> 
> git is at HEAD a93ece47fd9edbd4558db24300056c9a57d3bcd4
> This is on ppc64le architecture.
> 
> Re-production steps:
> 
> 1. Copy the attached files named backing_img.file and test.img to a directory
> 2. And customize the following command to point to the above directory and 
> run the same.
> /usr/bin/qemu-io /test.img -c "write 1352192 1707520"
> 
> 3.Output of the above command.
> qemu-io: block/qcow2-cache.c:411: qcow2_cache_entry_mark_dirty: Assertion 
> `c->entries[i].offset != 0' failed.
> Aborted (core dumped)
> 
> from gdb:
> (gdb) bt
> #0  0x3fff833eeff0 in raise () from /lib64/libc.so.6
> #1  0x3fff833f136c in abort () from /lib64/libc.so.6
> #2  0x3fff833e4c44 in __assert_fail_base () from /lib64/libc.so.6
> #3  0x3fff833e4d34 in __assert_fail () from /lib64/libc.so.6
> #4  0x1006a594 in qcow2_cache_entry_mark_dirty (bs=0x2e886f60, 
> c=0x2e879700, table=0x3fff8120) at block/qcow2-cache.c:411
> #5  0x10056154 in alloc_refcount_block (bs=0x2e886f60, 
> cluster_index=2, refcount_block=0x3fff80cff808) at block/qcow2-refcount.c:417
> #6  0x10057520 in update_refcount (bs=0x2e886f60, offset=1048576, 
> length=524288, addend=1, decrease=false, type=QCOW2_DISCARD_NEVER)
> at block/qcow2-refcount.c:834
> #7  0x10057dc8 in qcow2_alloc_clusters_at (bs=0x2e886f60, 
> offset=1048576, nb_clusters=1) at block/qcow2-refcount.c:1032
> #8  0x100636d8 in do_alloc_cluster_offset (bs=0x2e886f60, 
> guest_offset=2097152, host_offset=0x3fff80cff9e0, nb_clusters=0x3fff80cff9d8)
> at block/qcow2-cluster.c:1221
> #9  0x10063afc in handle_alloc (bs=0x2e886f60, guest_offset=2097152, 
> host_offset=0x3fff80cffab0, bytes=0x3fff80cffab8, m=0x3fff80cffb60)
> at block/qcow2-cluster.c:1324
> #10 0x10064178 in qcow2_alloc_cluster_offset (bs=0x2e886f60, 
> offset=1572864, bytes=0x3fff80cffb4c, host_offset=0x3fff80cffb58, 
> m=0x3fff80cffb60)
> at block/qcow2-cluster.c:1511
> #11 0x1004d3f4 in qcow2_co_pwritev (bs=0x2e886f60, offset=1572864, 
> bytes=1486848, qiov=0x3fffc85f0bf0, flags=0) at block/qcow2.c:1919
> #12 0x100a9648 in bdrv_driver_pwritev (bs=0x2e886f60, offset=1352192, 
> bytes=1707520, qiov=0x3fffc85f0bf0, flags=16) at block/io.c:898
> #13 0x100ab630 in bdrv_aligned_pwritev (child=0x2e8927f0, 
> req=0x3fff80cffdd8, offset=1352192, bytes=1707520, align=1, 
> qiov=0x3fffc85f0bf0, flags=16)
> at block/io.c:1440
> #14 0x100ac4ac in bdrv_co_pwritev (child=0x2e8927f0, offset=1352192, 
> bytes=1707520, qiov=0x3fffc85f0bf0, flags=BDRV_REQ_FUA) at block/io.c:1691
> #15 0x1008da0c in blk_co_pwritev (blk=0x2e879410, offset=1352192, 
> bytes=1707520, qiov=0x3fffc85f0bf0, flags=BDRV_REQ_FUA) at 
> block/block-backend.c:1085
> #16 0x1008db68 in blk_write_entry (opaque=0x3fffc85f0c08) at 
> block/block-backend.c:1110
> #17 0x101aa444 in coroutine_trampoline (i0=780753552, i1=0) at 
> util/coroutine-ucontext.c:79
> #18 0x3fff83402b9c in makecontext () from /lib64/libc.so.6
> #19 0x in ?? ()
> (gdb) bt full
> #0  0x3fff833eeff0 in raise () from /lib64/libc.so.6
> No symbol table info available.
> #1  0x3fff833f136c in abort () from /lib64/libc.so.6
> No symbol table info available.
> #2  0x3fff833e4c44 in __assert_fail_base () from /lib64/libc.so.6
> No symbol table info available.
> #3  0x3fff833e4d34 in __assert_fail () from /lib64/libc.so.6
> No symbol table info available.
> #4  0x1006a594 in qcow2_cache_entry_mark_dirty (bs=0x2e886f60, 
> c=0x2e879700, table=0x3fff8120) at block/qcow2-cache.c:411
> i = 0
> __PRETTY_FUNCTION__ = "qcow2_cache_entry_mark_dirty"
> #5  0x10056154 in alloc_refcount_block (bs=0x2e886f60, 
> cluster_index=2, refcount_block=0x3fff80cff808) at block/qcow2-refcount.c:417
> s = 0x2e893210
> refcount_table_index = 0
> ret = 0
> new_block = 0
> blocks_used = 72057594818669408
> meta_offset = 1572863
> #6  0x10057520 in update_refcount (bs=0x2e886f60, offset=1048576, 
> length=524288, addend=1, decrease=false, type=QCOW2_DISCARD_NEVER)
> at block/qcow2-refcount.c:834
> block_index = 268870408
> refcount = 780741808
> cluster_index = 2
> table_index = 0
> s = 0x2e893210
> start = 1048576
> last = 1048576
> cluster_offset = 1048576
> refcount_block = 0x3fff8120
> old_table_index = -1
> ret = 16383
> #7  0x10057dc8 in qcow2_alloc_clusters_at (bs=0x2e886f60, 
> offset=1048576, nb_clusters=1) at block/qcow2-refcount.c:1032
> s = 0x2e893210
> cluster_index = 3
> refcount = 0
> i = 1
> ret = 0
> __PRETTY_FUNCTION__ = "qcow2_alloc_clusters_at"
> #8  0x100636d8 in do_alloc_cluster_offset (bs=0x2e886f60, 
> 

Re: [Qemu-block] [PATCH 0/3] block: Deprecated options

[Thomas Huth]

On 20.12.2017 22:40, John Snow wrote:
> 
> 
> On 12/18/2017 12:14 PM, Thomas Huth wrote:
>> Remove the deprecated "-drive boot" and "-hdachs" options and properly
>> mark some other deprecated options in the deprecation chapter.
>>
>> Thomas Huth (3):
>>   block: Remove the obsolete -drive boot=on|off parameter
>>   block: Remove the deprecated -hdachs option
>>   block: Mention -drive cyls/heads/secs/trans/serial/addr in deprecation
>> chapter
>>
>>  blockdev.c  | 11 
>>  qemu-doc.texi   | 29 +--
>>  qemu-options.hx | 19 ++---
>>  vl.c| 86 
>> ++---
>>  4 files changed, 19 insertions(+), 126 deletions(-)
>>
> 
> Reviewed-by: John Snow <js...@redhat.com>

Thanks!

> So long, farewell: however, since we forgot to document them, can we
> remove them already?

The are documented in
https://qemu.weilnetz.de/doc/qemu-doc.html#Block-device-options so I
doubt that we can simply drop them without announcing it first?

 Thomas

[Qemu-block] [PATCH v3 3/4] tests/cdrom-test: Test that -cdrom parameter is working

[Thomas Huth]

Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter
on a couple of boards without us noticing it immediately. Thus let's
add a test which checks that "-cdrom" can at least be used to start
QEMU with certain machine types.

Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Reviewed-by: Hervé Poussineau <hpous...@reactos.org>
Acked-By: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Thomas Huth <th...@redhat.com>
---
 tests/cdrom-test.c | 58 ++
 1 file changed, 58 insertions(+)

diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
index 5bbf322..7a1fce5 100644
--- a/tests/cdrom-test.c
+++ b/tests/cdrom-test.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 #include "boot-sector.h"
+#include "qapi/qmp/qdict.h"
 
 static char isoimage[] = "cdrom-boot-iso-XX";
 
@@ -89,6 +90,32 @@ cleanup:
 return ret;
 }
 
+/**
+ * Check that at least the -cdrom parameter is basically working, i.e. we can
+ * see the filename of the ISO image in the output of "info block" afterwards
+ */
+static void test_cdrom_param(gconstpointer data)
+{
+QTestState *qts;
+char *resp;
+
+qts = qtest_startf("-M %s -cdrom %s", (const char *)data, isoimage);
+resp = qtest_hmp(qts, "info block");
+g_assert(strstr(resp, isoimage) != 0);
+g_free(resp);
+qtest_quit(qts);
+}
+
+static void add_cdrom_param_tests(const char **machines)
+{
+while (*machines) {
+char *testname = g_strdup_printf("cdrom/param/%s", *machines);
+qtest_add_data_func(testname, *machines, test_cdrom_param);
+g_free(testname);
+machines++;
+}
+}
+
 static void test_cdboot(gconstpointer data)
 {
 QTestState *qts;
@@ -154,6 +181,37 @@ int main(int argc, char **argv)
 add_x86_tests();
 } else if (g_str_equal(arch, "s390x")) {
 add_s390x_tests();
+} else if (g_str_equal(arch, "ppc64")) {
+const char *ppcmachines[] = {
+"pseries", "mac99", "g3beige", "40p", "prep", NULL
+};
+add_cdrom_param_tests(ppcmachines);
+} else if (g_str_equal(arch, "sparc")) {
+const char *sparcmachines[] = {
+"LX", "SPARCClassic", "SPARCbook", "SS-10", "SS-20", "SS-4",
+"SS-5", "SS-600MP", "Voyager", "leon3_generic", NULL
+};
+add_cdrom_param_tests(sparcmachines);
+} else if (g_str_equal(arch, "sparc64")) {
+const char *sparc64machines[] = {
+"niagara", "sun4u", "sun4v", NULL
+};
+add_cdrom_param_tests(sparc64machines);
+} else if (!strncmp(arch, "mips64", 6)) {
+const char *mips64machines[] = {
+"magnum", "malta", "mips", "pica61", NULL
+};
+add_cdrom_param_tests(mips64machines);
+} else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
+const char *armmachines[] = {
+"realview-eb", "realview-eb-mpcore", "realview-pb-a8",
+"realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
+"vexpress-a9", "virt", NULL
+};
+add_cdrom_param_tests(armmachines);
+} else {
+const char *nonemachine[] = { "none", NULL };
+add_cdrom_param_tests(nonemachine);
 }
 
 ret = g_test_run();
-- 
1.8.3.1

[Qemu-block] [PATCH v3 4/4] MAINTAINERS: Add the cdrom-test to John's section

[Thomas Huth]

The cdrom-test checks various block types - IDE, SCSI and
virtio, so it's a little bit hard to decide where this should
belong to in the MAINTAINERS file. But John volunteered to take
it, so let's put it into the IDE section for now.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 24b7016..6cceb10 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -998,6 +998,7 @@ F: hw/block/cdrom.c
 F: hw/block/hd-geometry.c
 F: tests/ide-test.c
 F: tests/ahci-test.c
+F: tests/cdrom-test.c
 F: tests/libqos/ahci*
 T: git git://github.com/jnsnow/qemu.git ide
 
-- 
1.8.3.1

[Qemu-block] [PATCH v3 0/4] Add new CD-ROM related qtests

[Thomas Huth]

With one of my clean-up patches (see commit 1454509726719e0933c800), I
recently accidentially broke the "-cdrom" parameter (more precisely
"-drive if=scsi") on a couple of boards, since there was no error
detected during the "make check" regression testing. This is clearly an
indication that we are lacking tests in this area.
So this small patch series now introduces some tests for CD-ROM drives:
The first two patches introduce the possibility to check that booting
from CD-ROM drives still works fine for x86 and s390x, and the third
patch adds a test that certain machines can at least still be started
with the "-cdrom" parameter (i.e. that test would have catched the
mistake that I did with my SCSI cleanup patch).

v3:
 - Rebased to current master branch
 - Add a final patch that adds an entry to the MAINTAINERS file

v2:
 - Use g_spawn_sync() instead of execlp() to run genisoimage
 - The "-cdrom" parameter test is now run on all architectures (with
   machine "none" for the machines that are not explicitly checked)
 - Some rewordings and improved comments here and there

Thomas Huth (4):
  tests/boot-sector: Add magic bytes to s390x boot code header
  tests/cdrom-test: Test booting from CD-ROM ISO image file
  tests/cdrom-test: Test that -cdrom parameter is working
  MAINTAINERS: Add the cdrom-test to John's section

 MAINTAINERS|   1 +
 tests/Makefile.include |   2 +
 tests/boot-sector.c|   9 +-
 tests/cdrom-test.c | 222 +
 4 files changed, 231 insertions(+), 3 deletions(-)
 create mode 100644 tests/cdrom-test.c

-- 
1.8.3.1

[Qemu-block] [PATCH v3 2/4] tests/cdrom-test: Test booting from CD-ROM ISO image file

[Thomas Huth]

We already have the code for a boot file in tests/boot-sector.c,
so if the genisoimage program is available, we can easily create
a bootable CD ISO image that we can use for testing whether our
CD-ROM emulation and the BIOS CD-ROM boot works correctly.

Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Reviewed-by: Hervé Poussineau <hpous...@reactos.org>
Acked-By: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Thomas Huth <th...@redhat.com>
---
 tests/Makefile.include |   2 +
 tests/cdrom-test.c | 164 +
 2 files changed, 166 insertions(+)
 create mode 100644 tests/cdrom-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e3..41abda0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -179,6 +179,7 @@ check-qtest-generic-y = tests/qmp-test$(EXESUF)
 gcov-files-generic-y = monitor.c qapi/qmp-dispatch.c
 check-qtest-generic-y += tests/device-introspect-test$(EXESUF)
 gcov-files-generic-y = qdev-monitor.c qmp.c
+check-qtest-generic-y += tests/cdrom-test$(EXESUF)
 
 gcov-files-ipack-y += hw/ipack/ipack.c
 check-qtest-ipack-y += tests/ipoctal232-test$(EXESUF)
@@ -835,6 +836,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o 
$(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o 
tests/acpi-utils.o
 tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
+tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o 
$(libqos-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< 
,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
new file mode 100644
index 000..5bbf322
--- /dev/null
+++ b/tests/cdrom-test.c
@@ -0,0 +1,164 @@
+/*
+ * Various tests for emulated CD-ROM drives.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author:
+ *Thomas Huth <th...@redhat.com>
+ *
+ * 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 "libqtest.h"
+#include "boot-sector.h"
+
+static char isoimage[] = "cdrom-boot-iso-XX";
+
+static int exec_genisoimg(const char **args)
+{
+gchar *out_err = NULL;
+gint exit_status = -1;
+bool success;
+
+success = g_spawn_sync(NULL, (gchar **)args, NULL,
+   G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL,
+   NULL, NULL, NULL, _err, _status, NULL);
+if (!success) {
+return -ENOENT;
+}
+if (out_err) {
+fputs(out_err, stderr);
+g_free(out_err);
+}
+
+return exit_status;
+}
+
+static int prepare_image(const char *arch, char *isoimage)
+{
+char srcdir[] = "cdrom-test-dir-XX";
+char *codefile = NULL;
+int ifh, ret = -1;
+const char *args[] = {
+"genisoimage", "-quiet", "-l", "-no-emul-boot",
+"-b", NULL, "-o", isoimage, srcdir, NULL
+};
+
+ifh = mkstemp(isoimage);
+if (ifh < 0) {
+perror("Error creating temporary iso image file");
+return -1;
+}
+if (!mkdtemp(srcdir)) {
+perror("Error creating temporary directory");
+goto cleanup;
+}
+
+if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64") ||
+g_str_equal(arch, "s390x")) {
+codefile = g_strdup_printf("%s/bootcode-XX", srcdir);
+ret = boot_sector_init(codefile);
+if (ret) {
+goto cleanup;
+}
+} else {
+/* Just create a dummy file */
+char txt[] = "empty disc";
+codefile = g_strdup_printf("%s/readme.txt", srcdir);
+if (!g_file_set_contents(codefile, txt, sizeof(txt) - 1, NULL)) {
+fprintf(stderr, "Failed to create '%s'\n", codefile);
+goto cleanup;
+}
+}
+
+args[5] = strchr(codefile, '/') + 1;
+ret = exec_genisoimg(args);
+if (ret) {
+fprintf(stderr, "genisoimage failed: %i\n", ret);
+}
+
+unlink(codefile);
+
+cleanup:
+g_free(codefile);
+rmdir(srcdir);
+close(ifh);
+
+return ret;
+}
+
+static void test_cdboot(gconstpointer data)
+{
+QTestState *qts;
+
+qts = qtest_startf("-accel kvm:tcg -no-shutdown %s%s", (const char *)data,
+   isoimage);
+boot_sector_test(qts);
+qtest_quit(qts);
+}
+
+static void add_x86_tests(void)
+{
+qtest_add_data_func("cdrom/boot/default", "-cdrom ", test_cdboot);
+qtest_add_data_func("c

[Qemu-block] [PATCH v3 1/4] tests/boot-sector: Add magic bytes to s390x boot code header

[Thomas Huth]

We're going to use the s390x boot code for testing CD-ROM booting.
But the ISO loader of the s390-ccw bios is a little bit more picky
than the network loader and expects some magic bytes in the header
of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so
we've got to add them in our boot code here, too.

Reviewed-by: Christian Borntraeger <borntrae...@de.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Reviewed-by: Hervé Poussineau <hpous...@reactos.org>
Acked-By: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Signed-off-by: Thomas Huth <th...@redhat.com>
---
 tests/boot-sector.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/boot-sector.c b/tests/boot-sector.c
index c373f0e..7824286 100644
--- a/tests/boot-sector.c
+++ b/tests/boot-sector.c
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
 
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {
+0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,  /* Program status word  */
+0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,  /* Magic:   */
+0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,  /* see linux_s390_magic */
+0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40   /* in the s390-ccw bios */
 };
 static const uint8_t s390x_code[] = {
 0xa7, 0xf4, 0x00, 0x0a,/* j 0x10010 */
@@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
 } else if (g_str_equal(arch, "s390x")) {
 len = 0x1 + sizeof(s390x_code);
 boot_code = g_malloc0(len);
-memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
+memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
 memcpy(_code[0x1], s390x_code, sizeof(s390x_code));
 } else {
 g_assert_not_reached();
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [RFC 11/13] dp8393x: manage big endian bus

[Thomas Huth]

On 08.06.2018 22:05, Laurent Vivier wrote:
> This is needed by Quadra 800, this card can run on little-endian
> or big-endian bus.
> 
> Signed-off-by: Laurent Vivier 
> ---
>  hw/net/dp8393x.c | 101 
> ++-
>  1 file changed, 70 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index ef5f1eb94f..5061474e6b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -150,6 +150,7 @@ typedef struct dp8393xState {
>  
>  /* Hardware */
>  uint8_t it_shift;
> +bool big_endian;
>  qemu_irq irq;
>  #ifdef DEBUG_SONIC
>  int irq_level;
> @@ -174,6 +175,12 @@ typedef struct dp8393xState {
>  AddressSpace as;
>  } dp8393xState;
>  
> +#ifdef HOST_WORDS_BIGENDIAN
> +static const bool host_big_endian = true;
> +#else
> +static const bool host_big_endian = false;
> +#endif
> +
>  /* Accessor functions for values which are formed by
>   * concatenating two 16 bit device registers. By putting these
>   * in their own functions with a uint32_t return type we avoid the
> @@ -220,6 +227,36 @@ static uint32_t dp8393x_wt(dp8393xState *s)
>  return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
>  }
>  
> +static uint16_t dp8393x_get(dp8393xState *s, int width, uint16_t *base,
> +int offset)
> +{
> +uint16_t val;
> +
> +if (s->big_endian) {
> +val = base[offset * width + width - 1];
> +} else {
> +val = base[offset * width];
> +}
> +if (s->big_endian != host_big_endian) {
> +val = bswap16(val);
> +}
> +return val;
> +}

Could you maybe write that like this instead:

{
uint16_t val;

if (s->big_endian) {
val = base[offset * width + width - 1];
val = be16_to_cpu(val);
} else {
val = base[offset * width];
val = le16_to_cpu(val);
}
return val;
}

?
... then you don't need that ugly host_big_endian variable anymore.

 Thomas

Re: [Qemu-block] [Qemu-devel] [RFC 00/13] hw/m68k: add Apple Machintosh Quadra 800 machine

[Thomas Huth]

On 09.06.2018 16:25, Philippe Mathieu-Daudé wrote:
> Hi Laurent,
> 
> On 06/08/2018 05:05 PM, Laurent Vivier wrote:
>> if you want to test the machine, I'm sorry, it doesn't boot
>> a MacROM, but you can boot a linux kernel from the command line.
>>
>> You can install your own disk using debian-installer, with:
>>
>> ...
>> -M q800 \
>> -serial none -serial mon:stdio \
>> -m 1000M -drive file=m68k.qcow2,format=qcow2 \
>> -net nic,model=dp83932,addr=09:00:07:12:34:57 \
>> -append "console=ttyS0 vga=off" \
>> -kernel vmlinux-4.15.0-2-m68k \
>> -initrd initrd.gz \
>> -drive file=debian-9.0-m68k-NETINST-1.iso \
>> -drive file=m68k.qcow2,format=qcow2 \
>> -nographic
>>
>> If you use a graphic adapter instead of "-nographic", you can use "-g" to 
>> set the
>> size of the display (I use "-g 1600x800x24").
>>
>> You can get the ISO from:
>>
>> https://cdimage.debian.org/mirror/cdimage/ports/9.0/m68k/iso-cd/debian-9.0-m68k-NETINST-1.iso
>>
>> and extract the kernel and initrd.gz:
>>
>> guestfish --add debian-9.0-m68k-NETINST-1.iso --ro \
>>   --mount /dev/sda:/ <<_EOF_
>> copy-out /install/cdrom/initrd.gz .
>> copy-out /install/kernels/vmlinux-4.15.0-2-m68k .
>> _EOF_
> 
> Running with -d in_asm,int I get:
> 
> 
> IN: nf_get_id
> 0xd432:  movel %a3,%d0
> 0xd434:  addil #0,%d0
> 0xd43a:  movel %d0,%sp@-
> 0xd43c:  jsr 0xd404
> 
> 
> IN:
> 0xd404:  071400
> 
> INT  1: Unassigned(0xf4) pc=d404 sp=00393e60 sr=2700
> INT  2: Access Fault(0x8) pc= sp=00393e58 sr=2700
> ssw:  0506 ea:    sfc:  5dfc: 5
> 
> 
> IN:
> 0x280c:  clrl %sp@-
> 0x280e:  pea 0x
> 0x2812:  movel %d0,%sp@-
> 0x2814:  moveml %d1-%d5/%a0-%a2,%sp@-
> 0x2818:  movel %sp,%d0
> 0x281a:  andil #-8192,%d0
> 0x2820:  moveal %d0,%a2
> 0x2822:  moveal %a2@,%a2
> 0x2824:  movel %sp,%sp@-
> 0x2826:  bsrl 0x557c
> 
> 
> IN: buserr_c
> 0x557c:  subql #4,%sp
> 0x557e:  moveml %d2-%d7/%a3-%fp,%sp@-
> 0x5582:  moveal %sp@(48),%a3
> 0x5586:  btst #5,%a3@(44)
> 0x558c:  bnes 0x5592
> 
> ...
> 
> 
> IN: panic
> 0x0002c956:  moveal 0x39503c,%a0
> 0x0002c95c:  moveq #101,%d1
> 0x0002c95e:  subql #1,%d1
> 0x0002c960:  bnes 0x2c9c6
> 
> objdump -S gives:
> 
> d404 :
> d404:   7300mvsb %d0,%d1
> d406:   4e75rts
> 
> Instruction which exists in the disas code, but doesn't seem
> tcg-implemented:
> 
> disas/m68k.c:3654:{"mvsb", 2,   one(0070400),   one(0170700), "*bDd",
> mcfisa_b },

0x7300 is the illegal opcode that is used by the Aranym emulator for its
"Native Feature" (some kind of Hypercall) interface:

https://github.com/aranym/aranym/wiki/natfeats-proposal#special-opcodes

It's also a valid opcode in the ColdFire ISA (that's why the
disassembler detects this as valid instruction), but the name of the
function (nf_get_id_phys) clearly indicates that Linux is trying to use
the Natfeats opcode here.

So it's normal that this opcode is not implemented in QEMU 680x0 mode.
If Linux correctly catches the illegal opcode exception afterwards,
everything is fine and you don't need to worry about this anymore.
However, if Linux fails to catch it correctly, there is certainly
something wrong here...

 Thomas

Re: [Qemu-block] [Qemu-devel] [RFC 12/13] dp8393x: put DMA temp buffer in the state, not in the stack

[Thomas Huth]

On 08.06.2018 22:05, Laurent Vivier wrote:
> It's only 32 bytes, and this simplifies the dp8393x_get()/
> dp8393x_put() interface.

Maybe not worth the effort ... or do you need this in a later patch,
too? If so, please mention it in the patch description here.

> Signed-off-by: Laurent Vivier 
> ---
>  hw/net/dp8393x.c | 107 
> ++-
>  1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 5061474e6b..40e5f8257b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -168,6 +168,7 @@ typedef struct dp8393xState {
>  
>  /* Temporaries */
>  uint8_t tx_buffer[0x1];
> +uint16_t data[16];

Why 16? The biggest array that you replaced has only 12 entries...

Also, while you're at it, maybe change the name of the variable
("dma_data"?) or add a comment with a short explanation ?

 Thomas