[PATCH] plugins/core: add missing break in cb_to_tcg_flags

2020-01-04 Thread Emilio G. Cota
Reported-by: Robert Henry 
Signed-off-by: Emilio G. Cota 
---
 plugins/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/core.c b/plugins/core.c
index 9e1b9e7a91..ed863011ba 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -286,6 +286,7 @@ static inline uint32_t cb_to_tcg_flags(enum 
qemu_plugin_cb_flags flags)
 switch (flags) {
 case QEMU_PLUGIN_CB_RW_REGS:
 ret = 0;
+break;
 case QEMU_PLUGIN_CB_R_REGS:
 ret = TCG_CALL_NO_WG;
 break;
-- 
2.20.1




Re: [PATCH v2] Implement the Screamer sound chip for the mac99 machine type

2020-01-04 Thread Programmingkid
I found the patch that breaks Screamer sound support for qemu-system-ppc. It is 
this:

commit 2ceb8240fa4e4e30fb853565eb2bed3032d74f62
Author: Kővágó, Zoltán 
Date:   Thu Sep 19 23:24:11 2019 +0200

coreaudio: port to the new audio backend api

Signed-off-by: Kővágó, Zoltán 
Message-id: 
586a1e66de5cbc6c5234f9ae556d24befb6afada.1568927990.git.dirty.ice...@gmail.com
Signed-off-by: Gerd Hoffmann 


Reversing this patch should make the Screamer patch work with the current git 
version of QEMU.


Re: [Qemu-devel] What should a virtual board emulate?

2020-01-04 Thread Philippe Mathieu-Daudé

Hi Paolo,

On 3/20/19 11:35 AM, Paolo Bonzini wrote:

On 20/03/19 11:03, Philippe Mathieu-Daudé wrote:


-display/-vga options suffers same clarity problems than -net. Is it a
card device or a cable linking to a network? Here is it a card device or
a cable connecting a monitor display?


-display is a cable, -vga is a card ("-nic none" is a card, "-nic
anythingelse" is a card+cable; "-net nic" is a card, "-net anythingelse"
is a cable).


 Mind, I'm not demanding mips-fulong2e should continue to ignore -vga;
 that's for its maintainer to decide.  I don't demand, I ask: what should
 a virtual board emulate?  What should -nodefaults do?

IMHO -nodefaults contains soldered/mmio chipsets.
Whether you plug a display or not is a different story.


In principle you could also cut the copper tracks that connect the card
to the PCI bus...


But then you have a crippled machine... We are not trying to model that.

I went back to continue a Fuloong Avocado test I started a year ago, and 
it was failing. I remember I had something working, so I bisected and 
reached this commit...


78c37d88f1b8b0b3ebcc632c458f0c3779fe2951 is the first bad commit
commit 78c37d88f1b8b0b3ebcc632c458f0c3779fe2951
Author: Paolo Bonzini 
Date:   Tue Mar 19 15:37:19 2019 +0100

mips-fulong2e: obey -vga none

Do not create an ATI VGA if "-vga none" was passed on the command line.

Booting PMON 1.1.2:

console: PMON2000 MIPS Initializing. Standby...
console: ERRORPC= CONFIG=00030932
console: PRID=6302
console: DIMM read
console: 0080
console: read memory type
console: read number of rows
console: read memory size per side
console: read blocks per ddrram
console: read number of sides
console: read width
console: DIMM SIZE=1000
console: sdcfg=3d5043df
console: msize=1000
console: Init SDRAM Done!
console: Sizing caches...
console: Init caches...
console: godson2 caches found
console: Init caches done, cfg = 00030932
console: Copy PMON to execute location...
console: start = 0x8500
console: s0 = 0x3ac0
console: a500
console: a501
console: a502
console: a503
console: a504
console: copy text section done.
console: Copy PMON to execute location done.
console: sp=84ffc000Uncompressing BiosOK,Booting 
Bios

console: FREQ
console: FREI
console: DONE
console: TTYI
console: TTYD
console: ENVI
console: MAPV
console: Mfg  0, Id 60
console: STDV
console: 8010: heap is already above this point
console: SBDD
console: 686I
console: 0x3f8=ff
console: PPCIH
console: PCI bus 0 slot 5/0: reg 0x10 = 0x0
console: PCI bus 0 slot 5/0: reg 0x14 = 0x0
console: PCI bus 0 slot 5/0: reg 0x18 = 0x0
console: PCI bus 0 slot 5/0: reg 0x1c = 0x0
console: PCI bus 0 slot 5/0: reg 0x20 = 0x0
console: PCI bus 0 slot 5/0: reg 0x24 = 0x0
console: PCI bus 0 slot 5/1: reg 0x10 = 0xfff9
console: PCI bus 0 slot 5/1: reg 0x14 = 0xfffd
console: PCI bus 0 slot 5/1: reg 0x18 = 0xfff9
console: PCI bus 0 slot 5/1: reg 0x1c = 0xfffd
console: PCI bus 0 slot 5/1: reg 0x20 = 0xfff1
console: PCI bus 0 slot 5/1: reg 0x24 = 0x0
console: PCI bus 0 slot 5/2: reg 0x10 = 0x0
console: PCI bus 0 slot 5/2: reg 0x14 = 0x0
console: PCI bus 0 slot 5/2: reg 0x18 = 0x0
console: PCI bus 0 slot 5/2: reg 0x1c = 0x0
console: PCI bus 0 slot 5/2: reg 0x20 = 0xffe1
console: PCI bus 0 slot 5/2: reg 0x24 = 0x0
console: PCI bus 0 slot 5/3: reg 0x10 = 0x0
console: PCI bus 0 slot 5/3: reg 0x14 = 0x0
console: PCI bus 0 slot 5/3: reg 0x18 = 0x0
console: PCI bus 0 slot 5/3: reg 0x1c = 0x0
console: PCI bus 0 slot 5/3: reg 0x20 = 0xffe1
console: PCI bus 0 slot 5/3: reg 0x24 = 0x0
console: PCI bus 0 slot 5/4: reg 0x10 = 0x0
console: PCI bus 0 slot 5/4: reg 0x14 = 0x0
console: PCI bus 0 slot 5/4: reg 0x18 = 0x0
console: PCI bus 0 slot 5/4: reg 0x1c = 0x0
console: PCI bus 0 slot 5/4: reg 0x20 = 0x0
console: PCI bus 0 slot 5/4: reg 0x24 = 0x0
console: PCI bus 0 slot 5/5: reg 0x10 = 0x0
console: PCI bus 0 slot 5/5: reg 0x14 = 0x0
console: PCI bus 0 slot 5/5: reg 0x18 = 0x0
console: PCI bus 0 slot 5/5: reg 0x1c = 0x0
console: PCI bus 0 slot 5/5: reg 0x20 = 0x0
console: PCI bus 0 slot 5/5: reg 0x24 = 0x0
console: PCI bus 0 slot 5/6: reg 0x10 = 0x0
console: PCI bus 0 slot 5/6: reg 0x14 = 0x0
console: PCI bus 0 slot 5/6: reg 0x18 = 0x0
console: PCI bus 0 slot 5/6: reg 0x1c = 0x0
console: PCI bus 0 slot 5/6: reg 0x20 = 0x0
console: PCI bus 0 slot 5/6: reg 0x24 = 0x0
console: PCIS
console: PCIR
console: PCIW
console: NETI
console: RTCL
console: PCID
console: VGAI
console: Default MODE_ID 2
console: starting radeon init...
^
Current QEMU is stuck here.

Before it would continue:

console: iobase=bfd0a100,mmbase=b505
console: mc_status=5
console: mc_status=5
console: mc_status=5
console: mc_status=5
console: ppll_div_3 = 301f4
console: Wrote: 0x0043 0x000301f4 0x (0x)
console: Wrote: rd=67, fd=500, pd=3
console: VCLK_ECP_CNTL = 00C3
console: radeon init done
console: FRBI
console: cfb_c

Re: [RFC] Implementing vhost-user-blk device backend

2020-01-04 Thread Coiby Xu
Hi Stefan,

Thank you for reviewing my work! All the improvements have been
applied except for a small issue regarding object_add.

>  (qemu) object_add vhost-user-server,id=ID,chardev=CHARDEV,writable=on|off

Currently I implement object_add feature in the following syntax which use
unix_socket directly instead of chardev,

  (qemu) object_add
vhost-user-server,id=id=disk,unix_socket=/tmp/vhost-user-blk_vhost.socket,name=disk,writable=off

I know in QEMU we can create a socket server using chardev-add,
  (qemu) chardev-add socket,id=char1,path=/tmp/vhost-user-blk_vhost.socket

But it seems it's a bit cumbersome to utilize chardev. Take QMP over socket
as an example,

  $ x86_64-softmmu/qemu-system-x86_64 -drive
file=dpdk.img,format=raw,if=none,id=disk -device
ide-hd,drive=disk,bootindex=0 -m 128 -enable-kvm -chardev
socket,id=mon1,path=/tmp/mon.sock,server,nowait -mon
chardev=mon1,mode=control,pretty=on

It doesn't support multiple concurrent client connections because of the
limitation of chardev/char-socket.c.

On Thu, Dec 19, 2019 at 10:31 PM Stefan Hajnoczi  wrote:

> On Mon, Nov 18, 2019 at 10:27:28PM +0800, Coiby Xu wrote:
> > Hi all,
> >
> > This is an implementation of vhost-user-blk device backend by
> > following
> https://wiki.qemu.org/Google_Summer_of_Code_2019#vhost-user-blk_device_backend
> .
> > raw/qcow2 disk images can now be shared via vhost user protocol. In
> > this way, it could provide better performance than QEMU's existing NBD
> > support.
>
> Thank you for working on this feature!
>
> > +static size_t vub_iov_to_buf(const struct iovec *iov,
> > + const unsigned int iov_cnt, void *buf)
>
> Please take a look at utils/iov.c.  iov_to_buf_full() can be used
> instead of defining this function.
>
> > +{
> > +size_t len;
> > +unsigned int i;
> > +
> > +len = 0;
> > +for (i = 0; i < iov_cnt; i++) {
> > +memcpy(buf + len,  iov[i].iov_base, iov[i].iov_len);
> > +len += iov[i].iov_len;
> > +}
> > +return len;
> > +}
> > +
> > +static  VubDev *vub_device;
>
> If you switch to -object (see below) then this global pointer will go
> away so I won't comment on it throughout this patch.
>
> > +static void vub_accept(QIONetListener *listener, QIOChannelSocket *sioc,
> > +   gpointer opaque)
> > +{
> > +/* only one connection */
> > +if (vub_device->sioc) {
> > +return;
> > +}
> > +
> > +vub_device->sioc = sioc;
> > +vub_device->listener = listener;
> > +/*
> > + * increase the object reference, so cioc will not freeed by
> > + * qio_net_listener_channel_func which will call
> object_unref(OBJECT(sioc))
> > + */
> > +object_ref(OBJECT(sioc));
> > +
> > +qio_channel_set_name(QIO_CHANNEL(sioc), "vhost-server");
> > +if (!vug_init(&vub_device->parent, VHOST_USER_BLK_MAX_QUEUES,
> sioc->fd,
> > +  vub_panic_cb, &vub_iface)) {
> > +fprintf(stderr, "Failed to initialized libvhost-user-glib\n");
> > +}
>
> vug_init() uses the default GMainContext, which is bad for performance
> when there are many devices because it cannot take advantage of
> multi-core CPUs.  vhost-user-server should support IOThread so that
> devices can be run in dedicated threads.
>
> The nbd/server.c:NBDExport->ctx field serves this purpose in the NBD
> server.  It's a little trickier with libvhost-user-glib because the API
> currently doesn't allow passing in a GMainContext and will need to be
> extended.
>
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index cfcc044ce4..d8de179747 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1614,6 +1614,33 @@ STEXI
> >  @findex acl_reset
> >  Remove all matches from the access control list, and set the default
> >  policy back to @code{deny}.
> > +ETEXI
> > +
> > +{
> > +.name   = "vhost_user_server_stop",
> > +.args_type  = "",
> > +.params = "vhost_user_server_stop",
> > +.help   = "stop vhost-user-blk device backend",
> > +.cmd= hmp_vhost_user_server_stop,
> > +},
> > +STEXI
> > +@item vhost_user_server_stop
> > +@findex vhost_user_server_stop
> > +Stop the QEMU embedded vhost-user-blk device backend server.
> > +ETEXI
>
> The NBD server supports multiple client connections and exports
> (drives).  A vhost-user socket only supports one connection and one
> device.  I think it will be necessary to assign a unique identifier to
> every vhost-user server.
>
> By the way, I think the server should be a UserCreatable Object so the
> following syntax works:
>
>   $ qemu -object vhost-user-server,id=ID,chardev=CHARDEV,writable=on|off
>
> And existing HMP/QMP commands can be used:
>
>   (qemu) object_add vhost-user-server,id=ID,chardev=CHARDEV,writable=on|off
>   (qemu) object_del ID
>
> This way we don't need to define new HMP/QMP/command-line syntax for
> vhost-user-server.
>
> If you grep for UserCreatable you'll find examples like "i

Re: [PATCH v2] hppa: allow max ram size upto 4Gb

2020-01-04 Thread Philippe Mathieu-Daudé

On 1/3/20 10:54 AM, Igor Mammedov wrote:

On Thu, 2 Jan 2020 21:22:12 +0100
Helge Deller  wrote:


On 02.01.20 18:46, Igor Mammedov wrote:

Previous patch drops silent ram_size fixup and makes
QEMU error out with:

  "RAM size more than 3840m is not supported"

when user specified -m X more than supported value.

User shouldn't be bothered with starting QEMU with valid CLI,
so for the sake of user convenience allow using -m 4G vs -m 3840M.

Requested-by: Helge Deller 
Signed-off-by: Igor Mammedov 
---
v2:
   - make main ram -1 prio, so it wouldn't conflict with other regions
 starting from 0xf900

I dislike it but if you feel it's really necessary feel free to ack it.


Hard to find the v2 buried in the other series with my email client.



should be applied on top of:
   "hppa: drop RAM size fixup"


Hello Igor,
I appreciate that you are trying to make it more cleaner.
But, can't you merge both of your patches to one patch?
Right now you have one patch "hppa: drop RAM size fixup", which is
what I think is wrong. Then you add another one which somehow
fixes it up again and adds other stuff.

1st patch bring it in line with other boards adding
proper error check but without changing RAM size.
While 2nd is changing device model (mapped RAM size) and
clearly documents that it's a hack for user convenience,
Hence I'd prefer to keep both separated.


Having everything in one single patch makes your full change more
understandable.

Is it necessary to introduce clamped_ram_size and not continue with
ram_size (even if you would add it as static local variable)?

it's necessary since ram_size is global which should be kept == 
MachineState::ram_size.
Later on I plan to remove the former altogether and maybe
MachineState::ram_size aa well, since it could be read with
memory_region_size(MachineState::ram).


Why insist on clamping the ram? We recommend to model what the hardware 
does, and the hardware uses a full DIMM of DRAM, so 4GB, not less.


What are the new problem introduced by using 4GB? I only see advantages 
doing so. This doesn't break your series. This doesn't break the CLI.

Am I missing something?


---
  hw/hppa/machine.c | 21 +++--
  1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index ebbf44f..0302983 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -54,6 +54,7 @@ static uint64_t cpu_hppa_to_phys(void *opaque, uint64_t addr)

  static HPPACPU *cpu[HPPA_MAX_CPUS];
  static uint64_t firmware_entry;
+static ram_addr_t clamped_ram_size;

  static void machine_hppa_init(MachineState *machine)
  {
@@ -74,8 +75,6 @@ static void machine_hppa_init(MachineState *machine)
  long i;
  unsigned int smp_cpus = machine->smp.cpus;

-ram_size = machine->ram_size;
-
  /* Create CPUs.  */
  for (i = 0; i < smp_cpus; i++) {
  char *name = g_strdup_printf("cpu%ld-io-eir", i);
@@ -90,12 +89,14 @@ static void machine_hppa_init(MachineState *machine)
  }

  /* Limit main memory. */
-if (ram_size > FIRMWARE_START) {
-error_report("RAM size more than %d is not supported", FIRMWARE_START);
+if (machine->ram_size > 4 * GiB) {
+error_report("RAM size more than 4Gb is not supported");
  exit(EXIT_FAILURE);
  }
+clamped_ram_size = machine->ram_size > FIRMWARE_START ?
+FIRMWARE_START : machine->ram_size;

-memory_region_add_subregion(addr_space, 0, machine->ram);
+memory_region_add_subregion_overlap(addr_space, 0, machine->ram, -1);

  /* Init Dino (PCI host bus chip).  */
  pci_bus = dino_init(addr_space, &rtc_irq, &serial_irq);
@@ -151,7 +152,7 @@ static void machine_hppa_init(MachineState *machine)
  qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
"-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
firmware_low, firmware_high, firmware_entry);
-if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
+if (firmware_low < clamped_ram_size || firmware_high >= FIRMWARE_END) {
  error_report("Firmware overlaps with memory or IO space");
  exit(1);
  }
@@ -204,7 +205,7 @@ static void machine_hppa_init(MachineState *machine)
 (1) Due to sign-extension problems and PDC,
 put the initrd no higher than 1G.
 (2) Reserve 64k for stack.  */
-initrd_base = MIN(ram_size, 1 * GiB);
+initrd_base = MIN(clamped_ram_size, 1 * GiB);
  initrd_base = initrd_base - 64 * KiB;
  initrd_base = (initrd_base - initrd_size) & TARGET_PAGE_MASK;

@@ -232,7 +233,7 @@ static void machine_hppa_init(MachineState *machine)
   * various parameters in registers. After firmware initialization,
   * firmware will start the Linux kernel with ramdisk and cmdline.
   */
-cpu[0]->env.gr[26] = ram_size;
+cpu[0]->env.gr[26] = clamped_ram_size;


Helge, is this the code using this regis

Re: [PATCH v3 00/29] cputlb: Remove support for MMU_MODE*_SUFFIX

2020-01-04 Thread Philippe Mathieu-Daudé

On 12/29/19 12:10 AM, Richard Henderson wrote:

This is part of a project to raise the limit on NB_MMU_MODES.

One of those is in cpu_ldst.h, in support of MMU_MODE*_SUFFIX.
While this could be extended, it's not the best interface for
such things.  Better is a single interface that allows a variable
mmu_idx.  The best exemplars of that is the usage in target/mips
and target/ppc.

In the process, I tried to clean up the implementation of these
functions for softmmu and user-only.

Aleksander asked about code size changes.  They vary between
a minor size increase (e.g. for qemu-system-alpha, where there
are in fact no uses of the functions, which are now present as
out-of-line functions rather than eliminated inline functions),
to a minor size decrease (e.g. -79k/-1.6% for qemu-system-i386).
See below for details.

Changes for v3:
   * Add patch to avoid breaking build for --enable-plugins.
   * Rebase on master and resolve conflicts.

Changes for v2:
   * Significantly revised docs/devel/loads-stores.rst.
   * m68k and s390x dropped #defines and use *_mmuidx_ra directly.

Patches lacking review:
0009-plugins-Include-trace-mem.h-in-api.c.patch
0010-cputlb-Move-body-of-cpu_ldst_template.h-out-of-li.patch


r~


Cc: Aleksandar Markovic 
Cc: Aleksandar Rikalo 
Cc: Aurelien Jarno 
Cc: Chris Wulff 
Cc: David Gibson 
Cc: David Hildenbrand 
Cc: Edgar E. Iglesias 
Cc: Eduardo Habkost 
Cc: Guan Xuetao 
Cc: Laurent Vivier 
Cc: Marek Vasut 
Cc: Max Filippov 
Cc: Paolo Bonzini 
Cc: Peter Maydell 

Richard Henderson (29):
   target/xtensa: Use probe_access for itlb_hit_test
   cputlb: Use trace_mem_get_info instead of trace_mem_build_info
   trace: Remove trace_mem_build_info_no_se_[bl]e
   target/s390x: Include tcg.h in mem_helper.c
   target/arm: Include tcg.h in sve_helper.c
   accel/tcg: Include tcg.h in tcg-runtime.c
   linux-user: Include tcg.h in syscall.c
   linux-user: Include trace-root.h in syscall-trace.h
   plugins: Include trace/mem.h in api.c
   cputlb: Move body of cpu_ldst_template.h out of line
   translator: Use cpu_ld*_code instead of open-coding
   cputlb: Rename helper_ret_ld*_cmmu to cpu_ld*_code
   cputlb: Provide cpu_(ld,st}*_mmuidx_ra for user-only
   target/i386: Use cpu_*_mmuidx_ra instead of templates
   cputlb: Expand cpu_ldst_useronly_template.h in user-exec.c
   target/nios2: Remove MMU_MODE{0,1}_SUFFIX
   target/alpha: Remove MMU_MODE{0,1}_SUFFIX
   target/cris: Remove MMU_MODE{0,1}_SUFFIX
   target/i386: Remove MMU_MODE{0,1,2}_SUFFIX
   target/microblaze: Remove MMU_MODE{0,1,2}_SUFFIX
   target/sh4: Remove MMU_MODE{0,1}_SUFFIX
   target/unicore32: Remove MMU_MODE{0,1}_SUFFIX
   target/xtensa: Remove MMU_MODE{0,1,2,3}_SUFFIX
   target/m68k: Use cpu_*_mmuidx_ra instead of MMU_MODE{0,1}_SUFFIX
   target/mips: Use cpu_*_mmuidx_ra instead of MMU_MODE*_SUFFIX
   target/s390x: Use cpu_*_mmuidx_ra instead of MMU_MODE*_SUFFIX
   target/ppc: Use cpu_*_mmuidx_ra instead of MMU_MODE*_SUFFIX
   cputlb: Remove support for MMU_MODE*_SUFFIX
   cputlb: Expand cpu_ldst_template.h in cputlb.c


Series:
Tested-by: Philippe Mathieu-Daudé 




Re: [PATCH v3 09/29] plugins: Include trace/mem.h in api.c

2020-01-04 Thread Philippe Mathieu-Daudé

On 1/3/20 10:59 PM, Richard Henderson wrote:

On 1/3/20 5:22 PM, Philippe Mathieu-Daudé wrote:

Hi Richard,

On 12/29/19 12:11 AM, Richard Henderson wrote:

Code movement in an upcoming patch will show that this file
was implicitly depending on trace/mem.h being included beforehand.


Ah, it uses the TRACE_MEM_* macros from "trace/mem-internal.h", which is
include by "trace/mem.h". OK.

Which part requires "trace-root.h"? Isn't it "trace/mem-internal.h" that should
include "trace-root.h"?


I don't know -- perhaps it's not required at all.  I think I did a blind copy
of the trace related includes that were being removed.


I'v tested your series, removing "trace-root.h" from this patch and 
building next patches, and there is no problem, it is indeed not 
required at all.


Removing "trace-root.h":
Reviewed-by: Philippe Mathieu-Daudé 

Regardless:
Tested-by: Philippe Mathieu-Daudé 




Re: [PATCH 3/3] capstone: Add skipdata hook for s390x

2020-01-04 Thread Philippe Mathieu-Daudé

On 1/3/20 10:25 PM, Richard Henderson wrote:

Capstone assumes any s390x unknown instruction is 2 bytes.
Instead, use the ilen field in the first two bits of
the instruction to stay in sync with the insn stream.

Reviewed-by: Thomas Huth 
Signed-off-by: Richard Henderson 


Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 


---
  disas.c | 37 +
  1 file changed, 37 insertions(+)

diff --git a/disas.c b/disas.c
index 845c40fca8..1095bad049 100644
--- a/disas.c
+++ b/disas.c
@@ -178,6 +178,39 @@ static int print_insn_od_target(bfd_vma pc, 
disassemble_info *info)
 to share this across calls and across host vs target disassembly.  */
  static __thread cs_insn *cap_insn;
  
+/*

+ * The capstone library always skips 2 bytes for S390X.
+ * This is less than ideal, since we can tell from the first two bits
+ * the size of the insn and thus stay in sync with the insn stream.
+ */
+static size_t CAPSTONE_API
+cap_skipdata_s390x_cb(const uint8_t *code, size_t code_size,
+  size_t offset, void *user_data)
+{
+size_t ilen;
+
+/* See get_ilen() in target/s390x/internal.h.  */
+switch (code[offset] >> 6) {
+case 0:
+ilen = 2;
+break;
+case 1:
+case 2:
+ilen = 4;
+break;
+default:
+ilen = 6;
+break;
+}
+
+return ilen;
+}
+
+static const cs_opt_skipdata cap_skipdata_s390x = {
+.mnemonic = ".byte",
+.callback = cap_skipdata_s390x_cb
+};
+
  /* Initialize the Capstone library.  */
  /* ??? It would be nice to cache this.  We would need one handle for the
 host and one for the target.  For most targets we can reset specific
@@ -208,6 +241,10 @@ static cs_err cap_disas_start(disassemble_info *info, csh 
*handle)
  
  /* "Disassemble" unknown insns as ".byte W,X,Y,Z".  */

  cs_option(*handle, CS_OPT_SKIPDATA, CS_OPT_ON);
+if (info->cap_arch == CS_ARCH_SYSZ) {
+cs_option(*handle, CS_OPT_SKIPDATA_SETUP,
+  (uintptr_t)&cap_skipdata_s390x);
+}
  
  /* Allocate temp space for cs_disasm_iter.  */

  if (cap_insn == NULL) {






Re: [PATCH 2/3] capstone: Enable disassembly for s390x

2020-01-04 Thread Philippe Mathieu-Daudé

On 1/3/20 10:24 PM, Richard Henderson wrote:

Enable s390x, aka SYSZ, in the git submodule build.
Set the capstone parameters for both s390x host and guest.

Signed-off-by: Richard Henderson 


I'm fine with this patch because I don't use the s390 disas often.
For the S390 experts, my previous analysis is here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg667954.html

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 


---
  Makefile   | 1 +
  disas.c| 3 +++
  target/s390x/cpu.c | 4 
  3 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index 12e129ac9d..df1c692ccd 100644
--- a/Makefile
+++ b/Makefile
@@ -504,6 +504,7 @@ CAP_CFLAGS += -DCAPSTONE_USE_SYS_DYN_MEM
  CAP_CFLAGS += -DCAPSTONE_HAS_ARM
  CAP_CFLAGS += -DCAPSTONE_HAS_ARM64
  CAP_CFLAGS += -DCAPSTONE_HAS_POWERPC
+CAP_CFLAGS += -DCAPSTONE_HAS_SYSZ
  CAP_CFLAGS += -DCAPSTONE_HAS_X86
  
  .PHONY: capstone/all

diff --git a/disas.c b/disas.c
index 3937da6157..845c40fca8 100644
--- a/disas.c
+++ b/disas.c
@@ -660,6 +660,9 @@ void disas(FILE *out, void *code, unsigned long size)
  print_insn = print_insn_m68k;
  #elif defined(__s390__)
  print_insn = print_insn_s390;
+s.info.cap_arch = CS_ARCH_SYSZ;
+s.info.cap_insn_unit = 2;
+s.info.cap_insn_split = 6;
  #elif defined(__hppa__)
  print_insn = print_insn_hppa;
  #endif
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 625daeedd1..1734ad9c3a 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -43,6 +43,7 @@
  #include "sysemu/tcg.h"
  #endif
  #include "fpu/softfloat-helpers.h"
+#include "disas/capstone.h"
  
  #define CR0_RESET   0xE0UL

  #define CR14_RESET  0xC200UL;
@@ -162,6 +163,9 @@ static void s390_cpu_disas_set_info(CPUState *cpu, 
disassemble_info *info)
  {
  info->mach = bfd_mach_s390_64;
  info->print_insn = print_insn_s390;
+info->cap_arch = CS_ARCH_SYSZ;
+info->cap_insn_unit = 2;
+info->cap_insn_split = 6;
  }
  
  static void s390_cpu_realizefn(DeviceState *dev, Error **errp)







Re: [PATCH 1/3] capstone: Update to next

2020-01-04 Thread Philippe Mathieu-Daudé

On 1/3/20 10:24 PM, Richard Henderson wrote:

Update to aaffb38c44fa.  Choose this over the "current" 4.0.1 tag
because next now includes the s390x z13 vector opcodes, and also
the insn tables are now read-only.



As Thomas suggested,
Fixes: https://bugs.launchpad.net/qemu/+bug/1826175

With GCC on Linux:
Tested-by: Philippe Mathieu-Daudé 


Signed-off-by: Richard Henderson 
---
  Makefile  | 1 +
  capstone  | 2 +-
  configure | 2 +-
  3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 6b5ad1121b..12e129ac9d 100644
--- a/Makefile
+++ b/Makefile
@@ -499,6 +499,7 @@ dtc/%: .git-submodule-status
  # Remove all the extra -Warning flags that QEMU uses that Capstone doesn't;
  # no need to annoy QEMU developers with such things.
  CAP_CFLAGS = $(patsubst -W%,,$(CFLAGS) $(QEMU_CFLAGS))
+CAP_CFLAGS += -I$(SRC_PATH)/capstone/include
  CAP_CFLAGS += -DCAPSTONE_USE_SYS_DYN_MEM
  CAP_CFLAGS += -DCAPSTONE_HAS_ARM
  CAP_CFLAGS += -DCAPSTONE_HAS_ARM64
diff --git a/capstone b/capstone
index 22ead3e0bf..aaffb38c44 16
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
+Subproject commit aaffb38c44fa58f510ba9b6264f7079bfbba4c8e


Looking at https://github.com/aquynh/capstone/pull/1549, this is 
unfortunate the upstream project use the 'squash merge request' feature :(


Since I already reviewed various of the 1589 patches in the earlier 
version of this patch (22ead3e0bf..418d36d695) I'll only focus on the 
291 'squashed' commits added on top:


  $ git log --oneline 418d36d695..aaffb38c44|wc -l
  291

Most of the commits contains a single line of description, and various 
of them have hundreds of lines of changes.


Similarly to my previous review, I skipped the architecture we don't 
support and X86. Still to many ARM/Aarch64 patches to review, so I'm 
focusing on the buildsys and changes on the API we use:


Reviewed-by: Philippe Mathieu-Daudé 


diff --git a/configure b/configure
index 940bf9e87a..de96bfe017 100755
--- a/configure
+++ b/configure
@@ -5062,7 +5062,7 @@ case "$capstone" in
git_submodules="${git_submodules} capstone"
  fi
  mkdir -p capstone
-QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include"
+QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include/capstone"
  if test "$mingw32" = "yes"; then
LIBCAPSTONE=capstone.lib
  else






[PATCH v2 2/5] hda-codec: fix recording rate control

2020-01-04 Thread Volker Rümelin
Apply previous commit to hda_audio_input_cb for the same
reasons.

Signed-off-by: Volker Rümelin 
---
 hw/audio/hda-codec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 768ba31e79..e711a99a41 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -265,8 +265,6 @@ static void hda_audio_input_cb(void *opaque, int avail)
 
 int64_t to_transfer = MIN(B_SIZE - (wpos - rpos), avail);
 
-hda_timer_sync_adjust(st, -((wpos - rpos) + to_transfer - (B_SIZE >> 1)));
-
 while (to_transfer) {
 uint32_t start = (uint32_t) (wpos & B_MASK);
 uint32_t chunk = (uint32_t) MIN(B_SIZE - start, to_transfer);
@@ -278,6 +276,8 @@ static void hda_audio_input_cb(void *opaque, int avail)
 break;
 }
 }
+
+hda_timer_sync_adjust(st, -((wpos - rpos) - (B_SIZE >> 1)));
 }
 
 static void hda_audio_output_timer(void *opaque)
-- 
2.16.4




[PATCH v2 3/5] paaudio: drop recording stream in qpa_fini_in

2020-01-04 Thread Volker Rümelin
Every call to pa_stream_peek which returns a data length > 0
should have a corresponding pa_stream_drop. A call to qpa_read
does not necessarily call pa_stream_drop immediately after a
call to pa_stream_peek. Test in qpa_fini_in if a last
pa_stream_drop is needed.

This prevents following messages in the libvirt log file after
a recording stream gets closed and a new one opened.

pulseaudio: pa_stream_drop failed
pulseaudio: Reason: Bad state
pulseaudio: pa_stream_drop failed
pulseaudio: Reason: Bad state

To reproduce start qemu with
-audiodev pa,id=audio0,in.mixing-engine=off
and in the guest start and stop Audacity several times.

Signed-off-by: Volker Rümelin 
---
 audio/paaudio.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index 55a91f8980..7db1dc15f0 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -536,7 +536,6 @@ static void qpa_simple_disconnect(PAConnection *c, 
pa_stream *stream)
 {
 int err;
 
-pa_threaded_mainloop_lock(c->mainloop);
 /*
  * wait until actually connects. workaround pa bug #247
  * https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/247
@@ -550,7 +549,6 @@ static void qpa_simple_disconnect(PAConnection *c, 
pa_stream *stream)
 dolog("Failed to disconnect! err=%d\n", err);
 }
 pa_stream_unref(stream);
-pa_threaded_mainloop_unlock(c->mainloop);
 }
 
 static void qpa_fini_out (HWVoiceOut *hw)
@@ -558,8 +556,12 @@ static void qpa_fini_out (HWVoiceOut *hw)
 PAVoiceOut *pa = (PAVoiceOut *) hw;
 
 if (pa->stream) {
-qpa_simple_disconnect(pa->g->conn, pa->stream);
+PAConnection *c = pa->g->conn;
+
+pa_threaded_mainloop_lock(c->mainloop);
+qpa_simple_disconnect(c, pa->stream);
 pa->stream = NULL;
+pa_threaded_mainloop_unlock(c->mainloop);
 }
 }
 
@@ -568,8 +570,20 @@ static void qpa_fini_in (HWVoiceIn *hw)
 PAVoiceIn *pa = (PAVoiceIn *) hw;
 
 if (pa->stream) {
-qpa_simple_disconnect(pa->g->conn, pa->stream);
+PAConnection *c = pa->g->conn;
+
+pa_threaded_mainloop_lock(c->mainloop);
+if (pa->read_length) {
+int r = pa_stream_drop(pa->stream);
+if (r) {
+qpa_logerr(pa_context_errno(c->context),
+   "pa_stream_drop failed\n");
+}
+pa->read_length = 0;
+}
+qpa_simple_disconnect(c, pa->stream);
 pa->stream = NULL;
+pa_threaded_mainloop_unlock(c->mainloop);
 }
 }
 
-- 
2.16.4




[PATCH v2 5/5] paaudio: wait until the recording stream is ready

2020-01-04 Thread Volker Rümelin
Don't call pa_stream_peek before the recording stream is ready.

Information to reproduce the problem.

Start and stop Audacity in the guest several times because the
problem is racy.

libvirt log file:
-audiodev pa,id=audio0,server=localhost,out.latency=3,
 out.mixing-engine=off,in.mixing-engine=off \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,
 resourcecontrol=deny \
-msg timestamp=on
: Domain id=4 is tainted: custom-argv
char device redirected to /dev/pts/1 (label charserial0)
audio: Device pcspk: audiodev default parameter is deprecated,
 please specify audiodev=audio0
audio: Device hda: audiodev default parameter is deprecated,
 please specify audiodev=audio0
pulseaudio: pa_stream_peek failed
pulseaudio: Reason: Bad state
pulseaudio: pa_stream_peek failed
pulseaudio: Reason: Bad state

Signed-off-by: Volker Rümelin 
---
 audio/paaudio.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index b23274550e..dbfe48c03a 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -162,6 +162,10 @@ static size_t qpa_read(HWVoiceIn *hw, void *data, size_t 
length)
 
 CHECK_DEAD_GOTO(c, p->stream, unlock_and_fail,
 "pa_threaded_mainloop_lock failed\n");
+if (pa_stream_get_state(p->stream) != PA_STREAM_READY) {
+/* wait for stream to become ready */
+goto unlock;
+}
 
 while (total < length) {
 size_t l;
@@ -191,6 +195,7 @@ static size_t qpa_read(HWVoiceIn *hw, void *data, size_t 
length)
 }
 }
 
+unlock:
 pa_threaded_mainloop_unlock(c->mainloop);
 return total;
 
-- 
2.16.4




[PATCH v2 1/5] hda-codec: fix playback rate control

2020-01-04 Thread Volker Rümelin
Since commit 1930616b98 "audio: make mixeng optional" the
function hda_audio_output_cb can no longer assume the function
parameter avail contains the free buffer size. With the playback
mixing-engine turned off this leads to a broken playback rate
control and playback buffer drops in regular intervals.

This patch moves down the rate calculation, so the correct
buffer fill level is used for the calculation.

Signed-off-by: Volker Rümelin 
---
 hw/audio/hda-codec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index f17e8d8dce..768ba31e79 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -338,8 +338,6 @@ static void hda_audio_output_cb(void *opaque, int avail)
 return;
 }
 
-hda_timer_sync_adjust(st, (wpos - rpos) - to_transfer - (B_SIZE >> 1));
-
 while (to_transfer) {
 uint32_t start = (uint32_t) (rpos & B_MASK);
 uint32_t chunk = (uint32_t) MIN(B_SIZE - start, to_transfer);
@@ -351,6 +349,8 @@ static void hda_audio_output_cb(void *opaque, int avail)
 break;
 }
 }
+
+hda_timer_sync_adjust(st, (wpos - rpos) - (B_SIZE >> 1));
 }
 
 static void hda_audio_compat_input_cb(void *opaque, int avail)
-- 
2.16.4




[PATCH v2 4/5] paaudio: try to drain the recording stream

2020-01-04 Thread Volker Rümelin
There is no guarantee a single call to pa_stream_peek every
timer_period microseconds can read a recording stream faster
than the data gets produced at the source. Let qpa_read try to
drain the recording stream.

To reproduce the problem:

Start qemu with -audiodev pa,id=audio0,in.mixing-engine=off

On the host connect the qemu recording stream to the monitor of
a hardware output device. While the problem can also be seen
with a hardware input device, it's obvious with the monitor of
a hardware output device.

In the guest start audio recording with audacity and notice the
slow recording data rate.

Signed-off-by: Volker Rümelin 
---
 audio/paaudio.c | 41 +
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index 7db1dc15f0..b23274550e 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -156,34 +156,43 @@ static size_t qpa_read(HWVoiceIn *hw, void *data, size_t 
length)
 {
 PAVoiceIn *p = (PAVoiceIn *) hw;
 PAConnection *c = p->g->conn;
-size_t l;
-int r;
+size_t total = 0;
 
 pa_threaded_mainloop_lock(c->mainloop);
 
 CHECK_DEAD_GOTO(c, p->stream, unlock_and_fail,
 "pa_threaded_mainloop_lock failed\n");
 
-if (!p->read_length) {
-r = pa_stream_peek(p->stream, &p->read_data, &p->read_length);
-CHECK_SUCCESS_GOTO(c, r == 0, unlock_and_fail,
-   "pa_stream_peek failed\n");
-}
+while (total < length) {
+size_t l;
+int r;
+
+if (!p->read_length) {
+r = pa_stream_peek(p->stream, &p->read_data, &p->read_length);
+CHECK_SUCCESS_GOTO(c, r == 0, unlock_and_fail,
+   "pa_stream_peek failed\n");
+if (!p->read_length) {
+/* buffer is empty */
+break;
+}
+}
 
-l = MIN(p->read_length, length);
-memcpy(data, p->read_data, l);
+l = MIN(p->read_length, length - total);
+memcpy((char *)data + total, p->read_data, l);
 
-p->read_data += l;
-p->read_length -= l;
+p->read_data += l;
+p->read_length -= l;
+total += l;
 
-if (!p->read_length) {
-r = pa_stream_drop(p->stream);
-CHECK_SUCCESS_GOTO(c, r == 0, unlock_and_fail,
-   "pa_stream_drop failed\n");
+if (!p->read_length) {
+r = pa_stream_drop(p->stream);
+CHECK_SUCCESS_GOTO(c, r == 0, unlock_and_fail,
+   "pa_stream_drop failed\n");
+}
 }
 
 pa_threaded_mainloop_unlock(c->mainloop);
-return l;
+return total;
 
 unlock_and_fail:
 pa_threaded_mainloop_unlock(c->mainloop);
-- 
2.16.4




[PATCH v2 0/5] audio fixes

2020-01-04 Thread Volker Rümelin
Here are five patches to fix PulseAudio playback/recording with
the mixing engine off.

v2:
- Patch 3/5: Corrected error log message.

Volker Rümelin (5):
  hda-codec: fix playback rate control
  hda-codec: fix recording rate control
  paaudio: drop recording stream in qpa_fini_in
  paaudio: try to drain the recording stream
  paaudio: wait until the recording stream is ready

 audio/paaudio.c  | 70 
 hw/audio/hda-codec.c |  8 +++---
 2 files changed, 53 insertions(+), 25 deletions(-)

-- 
2.16.4




Re: [PATCH 2/5] hda-codec: fix recording rate control

2020-01-04 Thread Volker Rümelin
> This mail is multipart text+html and "git am" can't process it (the
> others are text only).  Can you please resend the patches, preferably
> with "git send-email" to avoid them being sent as multipart?  They all
> look good to me (this series and the 6th patch sent as separate mail).
>
>
Sorry, I made a mistake here. I will send a version 2 patch series with
git send-email.

With best regards,
Volker