Re: [Qemu-devel] [PATCH RFC v3 00/11] Add RX archtecture support

[Yoshinori Sato]

On Sun, 03 Mar 2019 03:51:14 +0900,
Philippe Mathieu-Daudé wrote:
> 
> Hi Yoshinori,
> 
> On 3/2/19 7:21 AM, Yoshinori Sato wrote:
> > Hello.
> > This patch series is added Renesas RX target emulation.
> > 
> > My git repository is bellow.
> > git://git.pf.osdn.net/gitroot/y/ys/ysato/qemu.git
> > 
> > Since my understanding is not enough,
> > I want many comments to make this a good one.
> 
> OK :)
> 
> Can you provide notes about how to test your port?
> Such: links to toolchains, how to build, what firmware/OS we can run...

OK.
toolchain - rx-unknown-linux
binutils-2.32 include rx-unknown-linux support.
$ configure --target=rx-unknown-linux
$ make

gcc - please use my git repo.
git://git.pf.osdn.net/gitroot/y/ys/ysato/gcc.git rx-trunk
$ configure --target=rx-unknown-linux --enable-languages=c --disable-shared \
--disable-threads --with-uclibc --disable-libssp --disable-libquadmath \
--disable-libgomp --disable-libatomic
$ make

This toolchain can build u-boot / linux.

target program.
u-boot
git://git.pf.osdn.net/gitroot/y/ys/ysato/uboot.git rx
pre build binary in bellow.
https://osdn.net/users/ysato/pf/qemu/dl/u-boot.bin

linux
git://git.osdn.net/gitroot/uclinux-h8/linux.git rx
https://osdn.net/users/ysato/pf/qemu/dl/zImage

Since linux is still incomplete, it may be problematic.

> > 
> > Thanks.
> > 
> > Changes v2
> > Rewrite translate. using decodetree.py
> > 
> > Yoshinori Sato (11):
> >   target/rx: TCG Translation
> >   target/rx: TCG helper
> >   target/rx: CPU definition
> >   target/rx: RX disassembler
> >   target/rx: miscellaneous functions
> >   RX62N interrupt contorol uint
> >   RX62N internal timer modules
> >   RX62N internal serial communication interface
> >   RX Target hardware definition
> >   Add rx-softmmu
> >   MAINTAINERS: Add RX entry.
> > 
> >  MAINTAINERS|   20 +
> >  arch_init.c|2 +
> >  configure  |8 +
> >  default-configs/rx-softmmu.mak |7 +
> >  hw/char/Makefile.objs  |2 +-
> >  hw/char/renesas_sci.c  |  288 ++
> >  hw/intc/Makefile.objs  |1 +
> >  hw/intc/rx_icu.c   |  323 ++
> >  hw/rx/Makefile.objs|1 +
> >  hw/rx/rx62n.c  |  227 
> >  hw/rx/rxqemu.c |  100 ++
> >  hw/timer/Makefile.objs |2 +
> >  hw/timer/renesas_cmt.c |  235 +
> >  hw/timer/renesas_tmr.c |  412 
> >  include/disas/bfd.h|5 +
> >  include/hw/char/renesas_sci.h  |   42 +
> >  include/hw/intc/rx_icu.h   |   49 +
> >  include/hw/rx/rx.h |7 +
> >  include/hw/rx/rx62n.h  |   54 +
> >  include/hw/timer/renesas_cmt.h |   33 +
> >  include/hw/timer/renesas_tmr.h |   42 +
> >  include/sysemu/arch_init.h |1 +
> >  target/rx/Makefile.objs|   11 +
> >  target/rx/cpu-qom.h|   52 +
> >  target/rx/cpu.c|  224 
> >  target/rx/cpu.h|  214 
> >  target/rx/disas.c  | 1570 
> >  target/rx/gdbstub.c|  113 ++
> >  target/rx/helper.c |  252 +
> >  target/rx/helper.h |   39 +
> >  target/rx/insns.decode |  336 ++
> >  target/rx/monitor.c|   38 +
> >  target/rx/op_helper.c  |  602 +++
> >  target/rx/translate.c  | 2220 
> > 
> >  34 files changed, 7531 insertions(+), 1 deletion(-)
> >  create mode 100644 default-configs/rx-softmmu.mak
> >  create mode 100644 hw/char/renesas_sci.c
> >  create mode 100644 hw/intc/rx_icu.c
> >  create mode 100644 hw/rx/Makefile.objs
> >  create mode 100644 hw/rx/rx62n.c
> >  create mode 100644 hw/rx/rxqemu.c
> >  create mode 100644 hw/timer/renesas_cmt.c
> >  create mode 100644 hw/timer/renesas_tmr.c
> >  create mode 100644 include/hw/char/renesas_sci.h
> >  create mode 100644 include/hw/intc/rx_icu.h
> >  create mode 100644 include/hw/rx/rx.h
> >  create mode 100644 include/hw/rx/rx62n.h
> >  create mode 100644 include/hw/timer/renesas_cmt.h
> >  create mode 100644 include/hw/timer/renesas_tmr.h
> >  create mode 100644 target/rx/Makefile.objs
> >  create mode 100644 target/rx/cpu-qom.h
> >  create mode 100644 target/rx/cpu.c
> >  create mode 100644 target/rx/cpu.h
> >  create mode 100644 target/rx/disas.c
> >  create mode 100644 target/rx/gdbstub.c
> >  create mode 100644 target/rx/helper.c
> >  create mode 100644 target/rx/helper.h
> >  create mode 100644 target/rx/insns.decode
> >  create mode 100644 target/rx/monitor.c
> >  create mode 100644 target/rx/op_helper.c
> >  create mode 100644 target/rx/translate.c
> > 
> 

-- 
Yosinori Sato

Re: [Qemu-devel] [PATCH v3 4/9] {monitor, hw/pvrdma}: Expose device internals via monitor interface

[Marcel Apfelbaum]

On 3/1/19 2:28 PM, Yuval Shaia wrote:

On Fri, Mar 01, 2019 at 08:17:02AM +0100, Markus Armbruster wrote:

Marcel Apfelbaum  writes:


Hi Yuval,

On 2/27/19 4:06 PM, Yuval Shaia wrote:

Allow interrogating device internals through HMP interface.
The exposed indicators can be used for troubleshooting by developers or
sysadmin.
There is no need to expose these attributes to a management system (e.x.
libvirt) because (1) most of them are not "device-management' related
info and (2) there is no guarantee the interface is stable.

Signed-off-by: Yuval Shaia 
---
   hmp-commands-info.hx  | 16 
   hw/rdma/rdma_backend.c| 70 ++-
   hw/rdma/rdma_rm.c |  7 
   hw/rdma/rdma_rm_defs.h| 27 +-
   hw/rdma/vmw/pvrdma.h  |  5 +++
   hw/rdma/vmw/pvrdma_hmp.h  | 21 +++
   hw/rdma/vmw/pvrdma_main.c | 77 +++
   monitor.c | 10 +
   8 files changed, 215 insertions(+), 18 deletions(-)
   create mode 100644 hw/rdma/vmw/pvrdma_hmp.h

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cbee8b944d..9153c33974 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -524,6 +524,22 @@ STEXI
   Show CPU statistics.
   ETEXI
   +#if defined(CONFIG_PVRDMA)
+{
+.name   = "pvrdmacounters",
+.args_type  = "",
+.params = "",
+.help   = "show pvrdma device counters",
+.cmd= hmp_info_pvrdmacounters,
+},
+
+STEXI
+@item info pvrdmacounters
+@findex info pvrdmacounters
+Show pvrdma device counters.
+ETEXI
+#endif
+
   #if defined(CONFIG_SLIRP)
   {
   .name   = "usernet",

[...]

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index b6061f4b6e..85101368c5 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -14,6 +14,7 @@
*/
 #include "qemu/osdep.h"
+#include "qemu/units.h"
   #include "qapi/error.h"
   #include "hw/hw.h"
   #include "hw/pci/pci.h"
@@ -25,6 +26,7 @@
   #include "cpu.h"
   #include "trace.h"
   #include "sysemu/sysemu.h"
+#include "monitor/monitor.h"
 #include "../rdma_rm.h"
   #include "../rdma_backend.h"
@@ -32,10 +34,13 @@
 #include 
   #include "pvrdma.h"
+#include "pvrdma_hmp.h"
   #include "standard-headers/rdma/vmw_pvrdma-abi.h"
   #include "standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h"
   #include "pvrdma_qp_ops.h"
   +GSList *devices;

"devices" is far too generic for an external identifier.  Are you
missing a 'static' here?  Even if static, I'd recommend "rdma_devices".

Yep, thanks.


+
   static Property pvrdma_dev_properties[] = {
   DEFINE_PROP_STRING("netdev", PVRDMADev, backend_eth_device_name),
   DEFINE_PROP_STRING("ibdev", PVRDMADev, backend_device_name),
@@ -55,6 +60,71 @@ static Property pvrdma_dev_properties[] = {

[...]

+}
+
+void pvrdma_dump_counters(Monitor *mon)
+{
+g_slist_foreach(devices, pvrdma_dump_device_counters, mon);
+}

Note for later: does nothing when @devices is empty.

But that is fine, isn't it?


+
   static void free_dev_ring(PCIDevice *pci_dev, PvrdmaRing *ring,
 void *ring_state)
   {
@@ -304,6 +374,8 @@ static void pvrdma_fini(PCIDevice *pdev)
 rdma_info_report("Device %s %x.%x is down", pdev->name,
PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+devices = g_slist_remove(devices, pdev);
   }
 static void pvrdma_stop(PVRDMADev *dev)
@@ -394,6 +466,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr, 
uint64_t val,
   if (val == 0) {
   trace_pvrdma_regs_write(addr, val, "REQUEST", "");
   pvrdma_exec_cmd(dev);
+dev->stats.commands++;
   }
   break;
   default:
@@ -612,9 +685,13 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
   goto out;
   }
   +memset(&dev->stats, 0, sizeof(dev->stats));
+
   dev->shutdown_notifier.notify = pvrdma_shutdown_notifier;
   qemu_register_shutdown_notifier(&dev->shutdown_notifier);
   +devices = g_slist_append(devices, pdev);
+
   out:
   if (rc) {
   pvrdma_fini(pdev);

Note for later: @devices contains the realized "pvrdma" devices.

This happens only when rc indicates an error and we jumped here before
adding the device to the list.


You could find these devices with object_child_foreach_recursive()
instead of maintaining a separate list.

Hmmm...interesting.
I will check if it fits my needs and will change accordingly if yes.


diff --git a/monitor.c b/monitor.c
index defa129319..53ecb5bc70 100644
--- a/monitor.c
+++ b/monitor.c
@@ -85,6 +85,9 @@
   #include "sysemu/iothread.h"
   #include "qemu/cutils.h"
   #include "tcg/tcg.h"
+#ifdef CONFIG_PVRDMA
+#include "hw/rdma/vmw/pvrdma_hmp.h"
+#endif
 #if defined(TARGET_S390X)
   #include "hw/s390x/storage-keys.h"
@@ -1362,6 +1365,13 @@ static void hmp_info_cpustats(Monitor *mon

Re: [Qemu-devel] [PATCH v2 5/5] contrib: gitdm: add a mapping for Janus Technologies

[Marcel Apfelbaum]

On 3/1/19 12:03 PM, Alex Bennée wrote:

Currently this just includes Marcel who is a fairly prolific
contributor.

Cc: Marcel Apfelbaum 
Signed-off-by: Alex Bennée 
---
  contrib/gitdm/group-map-janustech | 5 +
  gitdm.config  | 1 +
  2 files changed, 6 insertions(+)
  create mode 100644 contrib/gitdm/group-map-janustech

diff --git a/contrib/gitdm/group-map-janustech 
b/contrib/gitdm/group-map-janustech
new file mode 100644
index 00..4ae7cc24f2
--- /dev/null
+++ b/contrib/gitdm/group-map-janustech
@@ -0,0 +1,5 @@
+#
+# Janus Technologies contributors using non-corporate email
+#
+
+marcel.apfelb...@gmail.com
diff --git a/gitdm.config b/gitdm.config
index 7472d4b8be..c01c219078 100644
--- a/gitdm.config
+++ b/gitdm.config
@@ -36,6 +36,7 @@ GroupMap contrib/gitdm/group-map-wavecomp Wave Computing
  GroupMap contrib/gitdm/group-map-cadence Cadence Design Systems
  GroupMap contrib/gitdm/group-map-codeweavers CodeWeavers
  GroupMap contrib/gitdm/group-map-ibm IBM
+GroupMap contrib/gitdm/group-map-janustech Janus Technologies
  
  # Also group together our prolific individual contributors

  # and those working under academic auspices


Acked-by: Marcel Apfelbaum

Thanks,
Marcel

[Qemu-devel] [PATCH 1/2] iconv: detect and make curses depend on it

[Samuel Thibault]

curses will use it for proper wide output support.

Signed-off-by: Samuel Thibault 
---
 configure | 40 
 vl.c  |  2 +-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 540bee19ba..9979ca708d 100755
--- a/configure
+++ b/configure
@@ -1217,6 +1217,10 @@ for opt do
   ;;
   --enable-curses) curses="yes"
   ;;
+  --disable-iconv) iconv="no"
+  ;;
+  --enable-iconv) iconv="yes"
+  ;;
   --disable-curl) curl="no"
   ;;
   --enable-curl) curl="yes"
@@ -1718,6 +1722,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   gtk gtk UI
   vte vte support for the gtk UI
   curses  curses UI
+  iconv   font glyph conversion support
   vnc VNC UI support
   vnc-saslSASL encryption for VNC server
   vnc-jpegJPEG lossy compression for VNC server
@@ -3398,8 +3403,39 @@ EOF
   fi
 fi
 
+##
+# iconv probe
+if test "$iconv" != "no" ; then
+  cat > $TMPC << EOF
+#include 
+int main(void) {
+  iconv_t conv = iconv_open("WCHAR_T", "UCS-2");
+  return conv != (iconv_t) -1;
+}
+EOF
+  for iconv_lib in '' -liconv; do
+if compile_prog "" "$iconv_lib" ; then
+  iconv_found=yes
+  libs_softmmu="$iconv_lib $libs_softmmu"
+  break
+fi
+  done
+  if test "$iconv_found" = "yes" ; then
+iconv=yes
+  else
+if test "$iconv" = "yes" ; then
+  feature_not_found "iconv" "Install iconv devel"
+fi
+iconv=no
+  fi
+fi
+
 ##
 # curses probe
+if test "$iconv" = "no" ; then
+  # curses will need iconv
+  curses=no
+fi
 if test "$curses" != "no" ; then
   if test "$mingw32" = "yes" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
@@ -6111,6 +6147,7 @@ echo "libgcrypt $gcrypt"
 echo "nettle$nettle $(echo_version $nettle $nettle_version)"
 echo "libtasn1  $tasn1"
 echo "PAM   $auth_pam"
+echo "iconv support $iconv"
 echo "curses support$curses"
 echo "virgl support $virglrenderer $(echo_version $virglrenderer 
$virgl_version)"
 echo "curl support  $curl"
@@ -6435,6 +6472,9 @@ fi
 if test "$cocoa" = "yes" ; then
   echo "CONFIG_COCOA=y" >> $config_host_mak
 fi
+if test "$iconv" = "yes" ; then
+  echo "CONFIG_ICONV=y" >> $config_host_mak
+fi
 if test "$curses" = "yes" ; then
   echo "CONFIG_CURSES=m" >> $config_host_mak
   echo "CURSES_CFLAGS=$curses_inc" >> $config_host_mak
diff --git a/vl.c b/vl.c
index 502857a176..c8594fc6d5 100644
--- a/vl.c
+++ b/vl.c
@@ -3171,7 +3171,7 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_CURSES
 dpy.type = DISPLAY_TYPE_CURSES;
 #else
-error_report("curses support is disabled");
+error_report("curses or iconv support is disabled");
 exit(1);
 #endif
 break;
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] curses: add option to specify VGA font encoding

[Samuel Thibault]

This uses iconv to convert glyphs from the specified VGA font encoding to
unicode, and makes use of cchar_t instead of chtype when using ncursesw,
which allows to store all wide char as well as the WACS values.

Signed-off-by: Samuel Thibault 
Cc: Eddie Kohler 
---
 configure   |   5 +-
 qapi/ui.json|   4 +-
 qemu-options.hx |   5 +-
 ui/curses.c | 315 
 4 files changed, 279 insertions(+), 50 deletions(-)

diff --git a/configure b/configure
index 9979ca708d..1270dc8dc0 100755
--- a/configure
+++ b/configure
@@ -3449,14 +3449,17 @@ if test "$curses" != "no" ; then
 #include 
 #include 
 #include 
+#include 
 int main(void) {
+  const char *codeset;
   wchar_t wch = L'w';
   setlocale(LC_ALL, "");
   resize_term(0, 0);
   addwstr(L"wide chars\n");
   addnwstr(&wch, 1);
   add_wch(WACS_DEGREE);
-  return 0;
+  codeset = nl_langinfo(CODESET);
+  return codeset != 0;
 }
 EOF
   IFS=:
diff --git a/qapi/ui.json b/qapi/ui.json
index c5d1d7f099..12d3a2c751 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1131,6 +1131,7 @@
 # @full-screen:   Start user interface in fullscreen mode (default: off).
 # @window-close:  Allow to quit qemu with window close button (default: on).
 # @gl:Enable OpenGL support (default: off).
+# @charset:   Font charset used by guest (default: CP437).
 #
 # Since: 2.12
 #
@@ -1139,7 +1140,8 @@
   'base': { 'type'   : 'DisplayType',
 '*full-screen'   : 'bool',
 '*window-close'  : 'bool',
-'*gl': 'DisplayGLMode' },
+'*gl': 'DisplayGLMode',
+'*charset'   : 'str' },
   'discriminator' : 'type',
   'data': { 'gtk': 'DisplayGTK',
 'egl-headless'   : 'DisplayEGLHeadless'} }
diff --git a/qemu-options.hx b/qemu-options.hx
index 1cf9aac1fe..4bc4e736bb 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1216,7 +1216,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
 "[,window_close=on|off][,gl=on|core|es|off]\n"
 "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
 "-display vnc=[,]\n"
-"-display curses\n"
+"-display curses[,charset=]\n"
 "-display none\n"
 "-display egl-headless[,rendernode=]"
 "select display type\n"
@@ -1248,6 +1248,9 @@ support a text mode, QEMU can display this output using a
 curses/ncurses interface. Nothing is displayed when the graphics
 device is in graphical mode or if the graphics device does not support
 a text mode. Generally only the VGA device models support text mode.
+The font charset used by the guest can be specified with the
+@code{charset} option, for example @code{charset=CP850} for IBM CP850
+encoding. The default is @code{CP437}.
 @item none
 Do not display video output. The guest will still see an emulated
 graphics card, but its output will not be displayed to the QEMU
diff --git a/ui/curses.c b/ui/curses.c
index 1724dd57d4..203cc075d0 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -27,6 +27,10 @@
 #include 
 #include 
 #endif
+#include 
+#include 
+#include 
+#include 
 
 #include "qapi/error.h"
 #include "qemu-common.h"
@@ -54,7 +58,8 @@ static WINDOW *screenpad = NULL;
 static int width, height, gwidth, gheight, invalidate;
 static int px, py, sminx, sminy, smaxx, smaxy;
 
-static chtype vga_to_curses[256];
+static const char *font_charset = "CP437";
+static cchar_t vga_to_curses[256];
 
 static wint_t console_getch(enum maybe_keycode *maybe_keycode)
 {
@@ -77,19 +82,23 @@ static void curses_update(DisplayChangeListener *dcl,
   int x, int y, int w, int h)
 {
 console_ch_t *line;
-chtype curses_line[width];
+cchar_t curses_line[width];
 
 line = screen + y * width;
 for (h += y; y < h; y ++, line += width) {
 for (x = 0; x < width; x++) {
 chtype ch = line[x] & 0xff;
 chtype at = line[x] & ~0xff;
-if (vga_to_curses[ch]) {
-ch = vga_to_curses[ch];
+if (vga_to_curses[ch].chars[0]) {
+curses_line[x] = vga_to_curses[ch];
+} else {
+curses_line[x].chars[0] = ch;
+curses_line[x].chars[1] = 0;
+curses_line[x].attr = 0;
 }
-curses_line[x] = ch | at;
+curses_line[x].attr |=  at;
 }
-mvwaddchnstr(screenpad, y, 0, curses_line, width);
+mvwadd_wchnstr(screenpad, y, 0, curses_line, width);
 }
 
 pnoutrefresh(screenpad, py, px, sminy, sminx, smaxy - 1, smaxx - 1);
@@ -391,6 +400,254 @@ static void curses_atexit(void)
 endwin();
 }
 
+/* Setup wchar glyph for one UCS-2 char */
+static void convert_ucs(int glyph, uint16_t ch, iconv_t conv)
+{
+wchar_t wch;
+char *pch, *pwch;
+size_t sch, swch;
+
+pch = (char *) &ch;
+pwch = (char *) &wch;
+sch = sizeof(ch);
+swch = sizeof(wch);
+
+if (iconv(conv,

[Qemu-devel] [PATCH 0/2] curses: Add support for wide output

[Samuel Thibault]

Hello,

This adds support for wide output in the curses frontend

Samuel Thibault (2):
  iconv: detect and make curses depend on it
  curses: add option to specify VGA font encoding

 configure   |  45 ++-
 qapi/ui.json|   4 +-
 qemu-options.hx |   5 +-
 ui/curses.c | 315 
 vl.c|   2 +-
 5 files changed, 320 insertions(+), 51 deletions(-)

-- 
2.20.1

Re: [Qemu-devel] [RFC] multi phase reset

[Peter Maydell]

On Sat, 2 Mar 2019 at 19:41, Philippe Mathieu-Daudé  wrote:
>
> Hi Damien,
>
> On 3/1/19 5:52 PM, Peter Maydell wrote:
> > On Fri, 1 Mar 2019 at 15:34, Damien Hedde  
> > wrote:
> >> On 3/1/19 12:43 PM, Peter Maydell wrote:
> >>> In my design the only thing that I thought would happen in phase 3
> >>> was the "clear the resetting flag", but you've moved that to RELEASE.
> >>> What's left ? Do you have a concrete example where we'd need this?
> >>
> >> I hesitated to remove this phase (would be easy to add it after if it is
> >> really needed). I see 2 cases where it might be useful.
>
> If I RELEASE a PLL which need some time to warm up and stabilize, once
> stabilized it moves the device to the POST phase where it is ready?

No, I think that things like that where the device is not ready
for some period of time after reset should be handled by
the device itself. Typically we just ignore this and have
PLLs become stable instantaneously. If you really needed to
model it you'd just have a timer of some kind inside the
device model.

(This matches h/w behaviour -- a PLL which is not yet stable
is not still in reset, it's out of reset but has different
behaviour for an initial period of time before it stabilizes.)

thanks
-- PMM

[Qemu-devel] [Bug 1818398] [NEW] No evdev mouse passthrough with virtio-vga or kvm

[Yohann Agrebbe]

Public bug reported:

Hi,

Using qemu version 3.1.0-1 on a host with the latest Archlinux 64-bit
distribution, and running the same OS as guest, the mouse doesn't work
when using both evdev passthrough and virtio-vga, or when using both
evdev passthrough and kvm.

The following command line runs a machine that does not receive any mouse event:
qemu-system-x86_64 -machine type=q35,accel=kvm -cpu host -accel kvm -boot 
order=dc,menu=on -m size=2048 -net nic -device virtio-vga -device intel-hda 
-name Linux -drive file=/mnt/data/nobody/linux/arch.img,if=virtio -display 
sdl,gl=on -full-screen -net user -D /dev/null -rtc 
base=utc,clock=host,driftfix=slew -nodefaults -object 
input-linux,id=kbd1,evdev=/dev/input/event6,grab_all=on,repeat=on -object 
input-linux,id=mouse1,evdev=/dev/input/event7

But with this command line, removing virtio-vga and kvm, the mouse works as 
expected:
qemu-system-x86_64 -machine type=q35 -boot order=dc,menu=on -m size=2048 -net 
nic -device cirrus-vga -device intel-hda -name Linux -drive 
file=/mnt/data/nobody/linux/arch.img,if=virtio -display sdl,gl=on -full-screen 
-net user -D /dev/null -rtc base=utc,clock=host,driftfix=slew -nodefaults 
-object input-linux,id=kbd1,evdev=/dev/input/event6,grab_all=on,repeat=on 
-object input-linux,id=mouse1,evdev=/dev/input/event7

Note: Passing a keyboard by evdev in the same way always works, the
problem is mouse specific.

Thanks in advance for the analysis,
gatestallman

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: evdev kvm mouse virtio-vga

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1818398

Title:
  No evdev mouse passthrough with virtio-vga or kvm

Status in QEMU:
  New

Bug description:
  Hi,

  Using qemu version 3.1.0-1 on a host with the latest Archlinux 64-bit
  distribution, and running the same OS as guest, the mouse doesn't work
  when using both evdev passthrough and virtio-vga, or when using both
  evdev passthrough and kvm.

  The following command line runs a machine that does not receive any mouse 
event:
  qemu-system-x86_64 -machine type=q35,accel=kvm -cpu host -accel kvm -boot 
order=dc,menu=on -m size=2048 -net nic -device virtio-vga -device intel-hda 
-name Linux -drive file=/mnt/data/nobody/linux/arch.img,if=virtio -display 
sdl,gl=on -full-screen -net user -D /dev/null -rtc 
base=utc,clock=host,driftfix=slew -nodefaults -object 
input-linux,id=kbd1,evdev=/dev/input/event6,grab_all=on,repeat=on -object 
input-linux,id=mouse1,evdev=/dev/input/event7

  But with this command line, removing virtio-vga and kvm, the mouse works as 
expected:
  qemu-system-x86_64 -machine type=q35 -boot order=dc,menu=on -m size=2048 -net 
nic -device cirrus-vga -device intel-hda -name Linux -drive 
file=/mnt/data/nobody/linux/arch.img,if=virtio -display sdl,gl=on -full-screen 
-net user -D /dev/null -rtc base=utc,clock=host,driftfix=slew -nodefaults 
-object input-linux,id=kbd1,evdev=/dev/input/event6,grab_all=on,repeat=on 
-object input-linux,id=mouse1,evdev=/dev/input/event7

  Note: Passing a keyboard by evdev in the same way always works, the
  problem is mouse specific.

  Thanks in advance for the analysis,
  gatestallman

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1818398/+subscriptions

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[BALATON Zoltan]

On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:

Hi Zoltan,

On 3/3/19 12:34 AM, BALATON Zoltan wrote:

At least two machines, the PPC mac99 and MIPS fulong2e, have an ATI
gfx chip by default (Rage 128 Pro and M6/RV100 respectively) and
guests running on these and the PMON2000 firmware of the fulong2e
expect this to be available. Fortunately these are very similar chips
so they can be mostly emulated in the same device model. This patch
adds basic emulation of these ATI VGA chips.

While this is incomplete and currently only enough to run the MIPS
firmware and get framebuffer output with Linux, it allows the fulong2e
board to work more like the real hardware and having it in QEMU in
this state provides a way to experiment with it and allows others to
contribute to improve it. It is compiled for all archs but only the
fulong2e (which currently has no display output at all) is set to use
it by default (in a patch sent separately).


Patch looks good, trivial comment inlined.


Hello,
Thanks for yhe review. I took what I liked, replies for the rest below.


 default-configs/pci.mak  |   1 +
 hw/display/Makefile.objs |   2 +
 hw/display/ati.c | 686 +++
 hw/display/ati_2d.c  | 134 +
 hw/display/ati_dbg.c | 254 ++
 hw/display/ati_int.h |  87 ++
 hw/display/ati_regs.h| 456 +++


Please have a look at scripts/git.orderfile :)


Actually in this case I think that would make it less readable by putting 
headers with a lot of defines before actual code.



+qemu_log_mask(LOG_UNIMP, "Unsupported bpp value");


qemu_log_mask() requires trailing '\n' :(


Yes, it's hard to remember which of these QEMU functions need '\n' and 
which don't. Fixed.



+static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
+ unsigned int size)


I doubt inlining help here.


Should not hurt either and it shows this is supposed to be a light helper 
function that's only split off for readability. I could also turn it to a 
macro with a ( ? : ) conditional expression now but it's probably more 
readable this way.



+case RBBM_STATUS:


  /* fall through */


+case GUI_STAT:
+val = 64; /* free CMDFIFO entries */
+break;


Obviously fall through because there are no statements between case 
labels. I only use /* fall thorugh */ comment where there are some 
statements and a missing break but not in this case which should be clear 
without comment.



+default:


  qemu_log(UNIMP)?


+break;
+}
+trace_ati_mm_read(size, addr, ati_reg_name(addr & ~3ULL), val);


No log yet as most of the registers are currently unimplemented so that 
would generate too much logs. The trace should log all register accesses 
including unimplemented ones, that's enough currently.



+if (s->dev_id == PCI_DEVICE_ID_ATI_RADEON_QY &&
+s->vga.vram_size_mb < 16) {
+warn_report("Too small video memory for device id");
+s->vga.vram_size_mb = 16;


I'd rather use error_setg() and return.


Correcting the error if possible is friendlier IMO but this can be debated 
what's the preferred behaviour. I'll leave it as it is for now.



diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
new file mode 100644
index 00..5b6fdb299d
--- /dev/null
+++ b/hw/display/ati_dbg.c
@@ -0,0 +1,254 @@
+#include "ati_int.h"
+
+#ifdef DEBUG_ATI
+struct ati_regdesc {
+const char *name;
+int num;


uint16_t?


uint16_t would be enough but int should be simpler for current CPUs and 
won't need possible padding to struct so I did not bother with restricting 
type here.



I'd have inverted the structure member for nicer align ;)


It was easier to generate it with a simple sed command from ati_regs.h and 
update when new regs are added this way. I guess reversing the values 
could be done the same way. Maybe I should automate this so it will be 
updated automatically when new reg definitions are added but for now I 
left it to do by hand.



+};
+
+static struct ati_regdesc ati_reg_names[] = {


static 'const' struct ...


+{"MM_INDEX", 0x},

[...]

+{"RAGE128_MPP_TB_CONFIG", 0x01c0},
+{NULL, -1}
+};
+
+const char *ati_reg_name(int num)


I'd use reg_addr instead of num.


+{
+int i;
+
+num &= ~3;
+for (i = 0; ati_reg_names[i].name; i++) {


Well since it is const you can iterate until ARRAY_SIZE(ati_reg_names)
and drop the {NULL, -1} entry.


I could do any of the above but all this file is temporary for development 
and meant to be removed once enough functions of the device are 
implemented so for now I don't bother unless there's a strong argument 
other than style preference to change it.



diff --git a/hw/display/trace-events b/hw/display/trace-events
index 37d3264bb2..6aed33eeaa 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -138,3 +138,7 @@ vga_

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[Philippe Mathieu-Daudé]

On 3/3/19 1:46 PM, BALATON Zoltan wrote:
> On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
>> Hi Zoltan,
>>
>> On 3/3/19 12:34 AM, BALATON Zoltan wrote:
>>> At least two machines, the PPC mac99 and MIPS fulong2e, have an ATI
>>> gfx chip by default (Rage 128 Pro and M6/RV100 respectively) and
>>> guests running on these and the PMON2000 firmware of the fulong2e
>>> expect this to be available. Fortunately these are very similar chips
>>> so they can be mostly emulated in the same device model. This patch
>>> adds basic emulation of these ATI VGA chips.
>>>
>>> While this is incomplete and currently only enough to run the MIPS
>>> firmware and get framebuffer output with Linux, it allows the fulong2e
>>> board to work more like the real hardware and having it in QEMU in
>>> this state provides a way to experiment with it and allows others to
>>> contribute to improve it. It is compiled for all archs but only the
>>> fulong2e (which currently has no display output at all) is set to use
>>> it by default (in a patch sent separately).
>>
>> Patch looks good, trivial comment inlined.
> 
> Hello,
> Thanks for yhe review. I took what I liked, replies for the rest below.
> 
>>>  default-configs/pci.mak  |   1 +
>>>  hw/display/Makefile.objs |   2 +
>>>  hw/display/ati.c | 686
>>> +++
>>>  hw/display/ati_2d.c  | 134 +
>>>  hw/display/ati_dbg.c | 254 ++
>>>  hw/display/ati_int.h |  87 ++
>>>  hw/display/ati_regs.h    | 456 +++
>>
>> Please have a look at scripts/git.orderfile :)
> 
> Actually in this case I think that would make it less readable by
> putting headers with a lot of defines before actual code.
> 
>>> +    qemu_log_mask(LOG_UNIMP, "Unsupported bpp value");
>>
>> qemu_log_mask() requires trailing '\n' :(
> 
> Yes, it's hard to remember which of these QEMU functions need '\n' and
> which don't. Fixed.
> 
>>> +static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
>>> + unsigned int size)
>>
>> I doubt inlining help here.
> 
> Should not hurt either and it shows this is supposed to be a light
> helper function that's only split off for readability. I could also turn
> it to a macro with a ( ? : ) conditional expression now but it's
> probably more readable this way.
> 
>>> +    case RBBM_STATUS:
>>
>>   /* fall through */
>>
>>> +    case GUI_STAT:
>>> +    val = 64; /* free CMDFIFO entries */
>>> +    break;
> 
> Obviously fall through because there are no statements between case
> labels. I only use /* fall thorugh */ comment where there are some
> statements and a missing break but not in this case which should be
> clear without comment.

Apparently not enough obvious to me since I stopped here, since I don't
know the difference/relation between RBBM_STATUS/GUI_STAT registers.

> 
>>> +    default:
>>
>>   qemu_log(UNIMP)?
>>
>>> +    break;
>>> +    }
>>> +    trace_ati_mm_read(size, addr, ati_reg_name(addr & ~3ULL), val);
> 
> No log yet as most of the registers are currently unimplemented so that
> would generate too much logs. The trace should log all register accesses
> including unimplemented ones, that's enough currently.

As you wish :)

> 
>>> +    if (s->dev_id == PCI_DEVICE_ID_ATI_RADEON_QY &&
>>> +    s->vga.vram_size_mb < 16) {
>>> +    warn_report("Too small video memory for device id");
>>> +    s->vga.vram_size_mb = 16;
>>
>> I'd rather use error_setg() and return.
> 
> Correcting the error if possible is friendlier IMO but this can be
> debated what's the preferred behaviour. I'll leave it as it is for now.

Well we are somehow lying to the user... modifying his hardware without
his consent.

> 
>>> diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
>>> new file mode 100644
>>> index 00..5b6fdb299d
>>> --- /dev/null
>>> +++ b/hw/display/ati_dbg.c
>>> @@ -0,0 +1,254 @@
>>> +#include "ati_int.h"
>>> +
>>> +#ifdef DEBUG_ATI
>>> +struct ati_regdesc {
>>> +    const char *name;
>>> +    int num;
>>
>> uint16_t?
> 
> uint16_t would be enough but int should be simpler for current CPUs and
> won't need possible padding to struct so I did not bother with
> restricting type here.
> 
>> I'd have inverted the structure member for nicer align ;)
> 
> It was easier to generate it with a simple sed command from ati_regs.h
> and update when new regs are added this way. I guess reversing the
> values could be done the same way. Maybe I should automate this so it
> will be updated automatically when new reg definitions are added but for
> now I left it to do by hand.

Fair enough.

> 
>>> +};
>>> +
>>> +static struct ati_regdesc ati_reg_names[] = {
>>
>> static 'const' struct ...
>>
>>> +    {"MM_INDEX", 0x},
> [...]
>>> +    {"RAGE128_MPP_TB_CONFIG", 0x01c0},
>>> +    {NULL, -1}
>>> +};
>>> +
>>> +const char *ati_reg_name(int num)
>>
>> I'd use reg_addr instead of 

Re: [Qemu-devel] [PATCH RFC v3 11/11] MAINTAINERS: Add RX entry.

[Yoshinori Sato]

On Sun, 03 Mar 2019 04:03:19 +0900,
Philippe Mathieu-Daudé wrote:
> 
> On 3/2/19 7:21 AM, Yoshinori Sato wrote:
> > Signed-off-by: Yoshinori Sato 
> > ---
> >  MAINTAINERS | 20 
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5040d9dfb1..141c4994b9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -270,6 +270,14 @@ F: include/hw/riscv/
> >  F: linux-user/host/riscv32/
> >  F: linux-user/host/riscv64/
> >  
> > +RX
> 
> RX sounds very generic, what about:
> 
>   Renesas Extreme (RX)
> 
> Or
> 
>   RX (Renesas Extreme)
> 
> > +M: Yoshinori Sato 
> > +S: Maintained
> > +F: target/rx/
> > +F: hw/rx/
> > +F: include/hw/rx/
> > +F: disas/rx.c
> > +
> >  S390
> >  M: Richard Henderson 
> >  M: David Hildenbrand 
> > @@ -1084,6 +1092,18 @@ F: pc-bios/canyonlands.dt[sb]
> >  F: pc-bios/u-boot-sam460ex-20100605.bin
> >  F: roms/u-boot-sam460ex
> >  
> > +RX Machines
> > +---
> > +RX-QEMU
> > +M: Yoshinori Sato 
> > +S: Maintained
> > +F: hw/rx/rxqemu.c
> > +F: hw/intc/rx_icu.c
> > +F: hw/timer/renesas_*.c
> > +F: hw/char/renesas_sci.c
> > +F: include/hw/timer/renesas_*.h
> > +F: include/hw/char/renesas_sci.h
> > +
> >  SH4 Machines
> >  
> >  R2D
> > 
> 

OK.
I will update entry.
RENESAS RX

Thanks.

-- 
Yosinori Sato

Re: [Qemu-devel] [PATCH RFC v3 08/11] RX62N internal serial communication interface

[Yoshinori Sato]

On Sun, 03 Mar 2019 04:21:10 +0900,
Philippe Mathieu-Daudé wrote:
> 
> Hi Yoshinori,
> 
> On 3/2/19 7:21 AM, Yoshinori Sato wrote:
> > This module supported only non FIFO type.
> > Hardware manual.
> > https://www.renesas.com/us/en/doc/products/mpumcu/doc/rx_family/r01uh0033ej0140_rx62n.pdf?key=086621e01bd70347c18ea7f794aa9cc3
> 
> This link also works without the trailing
> "?key=086621e01bd70347c18ea7f794aa9cc3".

OK.
It is probably a parameter for searching. remove it.

> > 
> > Signed-off-by: Yoshinori Sato 
> > ---
> >  hw/char/Makefile.objs |   2 +-
> >  hw/char/renesas_sci.c | 288 
> > ++
> >  include/hw/char/renesas_sci.h |  42 ++
> 
> QEMU provides a git config helper to improve git diff by showing headers
> first and C sources after: scripts/git.orderfile
> I recommend you to look at it.

OK.

> I'll review the header, then back to source.
> 
> >  3 files changed, 331 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/char/renesas_sci.c
> >  create mode 100644 include/hw/char/renesas_sci.h
> > 
> > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> > index c4947d7ae7..68eae7b9a5 100644
> > --- a/hw/char/Makefile.objs
> > +++ b/hw/char/Makefile.objs
> > @@ -15,7 +15,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> >  obj-$(CONFIG_EXYNOS4) += exynos4210_uart.o
> >  obj-$(CONFIG_COLDFIRE) += mcf_uart.o
> >  obj-$(CONFIG_OMAP) += omap_uart.o
> > -obj-$(CONFIG_SH4) += sh_serial.o
> > +obj-$(CONFIG_RENESAS_SCI) += renesas_sci.o
> >  obj-$(CONFIG_PSERIES) += spapr_vty.o
> >  obj-$(CONFIG_DIGIC) += digic-uart.o
> >  obj-$(CONFIG_STM32F2XX_USART) += stm32f2xx_usart.o
> > diff --git a/hw/char/renesas_sci.c b/hw/char/renesas_sci.c
> > new file mode 100644
> > index 00..56d070a329
> > --- /dev/null
> > +++ b/hw/char/renesas_sci.c
> > @@ -0,0 +1,288 @@
> > +/*
> > + * Renesas Serial Communication Interface
> 
> I'd add here:
> 
> Datasheet: RX62N Group, RX621 Group User's Manual: Hardware
> (Rev.1.40 R01UH0033EJ0140)

OK.

> > + *
> > + * Copyright (c) 2019 Yoshinori Sato
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2 or later, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope 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 .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +#include "cpu.h"
> > +#include "hw/hw.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/char/renesas_sci.h"
> > +#include "qemu/error-report.h"
> > +
> > +#define freq_to_ns(freq) (10LL / freq)
> 
> You can directly use NANOSECONDS_PER_SECOND in update_trtime().

OK.

> > +
> > +static int can_receive(void *opaque)
> > +{
> > +RSCIState *sci = RSCI(opaque);
> > +if (sci->rx_next > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> > +return 0;
> > +} else {
> > +return sci->scr & 0x10;
> 
> It is way easier to review a code using definitions generated by the
> "hw/registerfields.h" API, a recent example is hw/char/cmsdk-apb-uart.c.

OK.
Other part have same code. I will update it as well.

> > +}
> > +}
> > +
> > +static void receive(void *opaque, const uint8_t *buf, int size)
> > +{
> > +RSCIState *sci = RSCI(opaque);
> > +sci->rdr = buf[0];
> > +if (sci->ssr & 0x40 || size > 1) {
> > +sci->ssr |= 0x20;
> > +if (sci->scr & 0x40) {
> > +qemu_set_irq(sci->irq[ERI], 1);
> > +}
> > +} else {
> > +sci->ssr |= 0x40;
> > +sci->rx_next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + sci->trtime;
> > +if (sci->scr & 0x40) {
> > +qemu_set_irq(sci->irq[RXI], 1);
> > +qemu_set_irq(sci->irq[RXI], 0);
> 
> Both lines are equivalent to:
> 
>qemu_irq_pulse(sci->irq[RXI]);

OK. 

> 
> > +}
> > +}
> > +}
> > +
> > +static void send_byte(RSCIState *sci)
> > +{
> > +if (qemu_chr_fe_backend_connected(&sci->chr)) {
> > +qemu_chr_fe_write_all(&sci->chr, &sci->tdr, 1);
> > +}
> > +timer_mod(sci->timer,
> > +  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + sci->trtime);
> > +sci->ssr &= ~0x04;
> > +sci->ssr |= 0x80;
> > +qemu_set_irq(sci->irq[TEI], 0);
> > +if (sci->scr & 0x80) {
> > +qemu_set_irq(sci->irq[TXI], 1);
> > +qemu_set_irq(sci->irq[TXI], 0);
> > +}
> > +}
> > +
> > +static void txend(void *opaque)
> > +{
> > +RSCIState *sci = RSCI(opaque);
> > +if ((sci->ssr & 0x80) == 0) {
> > +send_byte(sci);
> > +   

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[BALATON Zoltan]

On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:

On 3/3/19 1:46 PM, BALATON Zoltan wrote:

On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:

+??? case RBBM_STATUS:


? /* fall through */


+??? case GUI_STAT:
+??? val = 64; /* free CMDFIFO entries */
+??? break;


Obviously fall through because there are no statements between case
labels. I only use /* fall thorugh */ comment where there are some
statements and a missing break but not in this case which should be
clear without comment.


Apparently not enough obvious to me since I stopped here, since I don't
know the difference/relation between RBBM_STATUS/GUI_STAT registers.


I did not mean obvious from the registers but obvious from having no 
statements between case labels. Multiple case labels for common cases in 
switch without fall through comment between them is used in several other 
places in QEMU as well. The fall through comment is only needed when there 
are some statements but no break at the end and code falls through to the 
next case. Then the comment tells that break was not forgotten but fall 
through is intended but no such comment is needed when multiple cases go 
to same place like here. It may look odd in a big switch where every other 
case has statements only these two are common ones but it's correct and 
consistent with other places and should be clear without comment what is 
meant. (I could add a comment here anyway to make it look more regular but 
I prefer code that's less verbose vertically so more lines fit on the 
screen.)



+??? if (s->dev_id == PCI_DEVICE_ID_ATI_RADEON_QY &&
+??? s->vga.vram_size_mb < 16) {
+??? warn_report("Too small video memory for device id");
+??? s->vga.vram_size_mb = 16;


I'd rather use error_setg() and return.


Correcting the error if possible is friendlier IMO but this can be
debated what's the preferred behaviour. I'll leave it as it is for now.


Well we are somehow lying to the user... modifying his hardware without
his consent.


We do warn them though so not silently changing hardware. There are 
arguments for both ways, I can't decide and this can be changed anytime 
later so I go with what I like unless more votes for another way.



I could do any of the above but all this file is temporary for
development and meant to be removed once enough functions of the device
are implemented so for now I don't bother unless there's a strong
argument other than style preference to change it.


I'd definitively keep it, and I now believe you should drop the
DEBUG_ATI around. It is only used by tracing right? And tracing is
already optimized. If it becomes someday unuseful to you, it can still
be useful for the next one wanting to improve/fix your device.


We've discussed before why trace points are not convenient for debugging 
during development so for these I keep DPRINTF and only convert to traces 
parts that are mostly finished. This way it's also clear which messages 
are to be removed later and which are meant to be kept for tracing. If we 
choose to keep register names also after development for more detailed 
traces then this could be cleaned up but this probably would just add 
unused code for most of the time. So after development is finished I don't 
think it's useful to include reg names.



diff --git a/hw/display/trace-events b/hw/display/trace-events
index 37d3264bb2..6aed33eeaa 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -138,3 +138,7 @@ vga_cirrus_write_blt(uint32_t offset, uint32_t
val) "offset 0x%x, val 0x%x"
?sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
?sii9022_write_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
?sii9022_switch_mode(const char *mode) "mode: %s"
+
+# hw/display/ati*.c
+ati_mm_read(unsigned int size, uint64_t addr, const char *name,
uint64_t val) "%d 0x%"HWADDR_PRIx " %s -> 0x%"PRIx64
+ati_mm_write(unsigned int size, uint64_t addr, const char *name,
uint64_t val) "%d 0x%"HWADDR_PRIx " %s <- 0x%"PRIx64


"%u ...


Should not matter as size that can only be 1 to 4 but %u is more correct
to print unsigned variable so I'll change this.


The argument is unsigned, so the correct format is %u.

This help reducing the thousands of warnings in QEMU about incorrect
formatstring.


Yes, I've corrected this. I did not get warnings for it but maybe some 
compilers are pickier about this.


Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH V2 4/7] Migration/colo.c: Add new COLOExitReason to handle all failover state

[Zhang, Chen]

-Original Message-
From: Eric Blake [mailto:ebl...@redhat.com] 
Sent: Friday, March 1, 2019 1:05 AM
To: Zhang, Chen ; Li Zhijian ; 
Zhang Chen ; Dr. David Alan Gilbert ; 
Juan Quintela ; zhanghailiang 
; Markus Armbruster ; 
qemu-dev 
Subject: Re: [PATCH V2 4/7] Migration/colo.c: Add new COLOExitReason to handle 
all failover state

On 2/28/19 10:55 AM, Zhang Chen wrote:
> From: Zhang Chen 
> 
> In this patch we add the processing state for COLOExitReason, because 
> we have to identify COLO in the failover processing state or failover 
> error state. In the way, we can handle all the failover state.
> We have improved the description of the COLOExitReason by the way.
> 
> Signed-off-by: Zhang Chen 
> ---
>  migration/colo.c| 24 +---
>  qapi/migration.json | 15 +--
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 

> +++ b/qapi/migration.json
> @@ -983,19 +983,22 @@
>  ##
>  # @COLOExitReason:
>  #
> -# The reason for a COLO exit
> +# Describe the reason for COLO exit.
>  #
> -# @none: no failover has ever happened. This can't occur in the -# 
> COLO_EXIT event, only in the result of query-colo-status.
> +# @none: failover has never happened. This state does not occurred # 
> +in the COLO_EXIT event, only happened in the result of # 
> +query-colo-status.

Grammar suggestion:

This state does not occur in the COLO_EXIT event, and is only visible in the 
result of query-colo-status.

OK~ I will fix it in next version.

>  #
> -# @request: COLO exit is due to an external request
> +# @request: COLO exit caused by an external request.
>  #
> -# @error: COLO exit is due to an internal error
> +# @error: COLO exit caused by an internal error.
> +#
> +# @processing: COLO in failover handling state.

Missing a (since 4.0) tag.  Maybe:

@processing: COLO is currently handling a failover (since 4.0).

Sure, thank you for your comments~


Thanks
Zhang Chen

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

Re: [Qemu-devel] [PATCH V2 5/7] qapi/migration.json: Remove a variable that doesn't exist in example

[Zhang, Chen]

-Original Message-
From: Eric Blake [mailto:ebl...@redhat.com] 
Sent: Friday, March 1, 2019 1:07 AM
To: Zhang, Chen ; Li Zhijian ; 
Zhang Chen ; Dr. David Alan Gilbert ; 
Juan Quintela ; zhanghailiang 
; Markus Armbruster ; 
qemu-dev 
Subject: Re: [PATCH V2 5/7] qapi/migration.json: Remove a variable that doesn't 
exist in example

On 2/28/19 10:55 AM, Zhang Chen wrote:
> From: Zhang Chen 
> 
> Remove the "active" variable in example for query-colo-status.
> 
> Signed-off-by: Zhang Chen 
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

Might be nice to mention that the doc bug has been present since the command's 
introduction in commit f56c0065.

Sure, will write in next version.

Thanks
Zhang Chen

> 
> diff --git a/qapi/migration.json b/qapi/migration.json index 
> 48e21880a3..f4c1762dfc 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1342,7 +1342,7 @@
>  # Example:
>  #
>  # -> { "execute": "query-colo-status" } -# <- { "return": { "mode": 
> "primary", "active": true, "reason": "request" } }
> +# <- { "return": { "mode": "primary", "reason": "request" } }
>  #
>  # Since: 3.1
>  ##
> 

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

Re: [Qemu-devel] [PATCH] tests: Remove (mostly) useless architecture checks

[Thomas Huth]

On 01/03/2019 18.57, John Snow wrote:
> 
> 
> On 3/1/19 11:16 AM, Thomas Huth wrote:
>> These checks at the beginning of some of the tests are mostly useless:
>> We only run the tests on x86 anyway, and g_test_message() does not
>> print anything unless you call g_test_init() first.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  tests/fdc-test.c  | 7 ---
>>  tests/ide-test.c  | 7 ---
>>  tests/ipmi-bt-test.c  | 7 ---
>>  tests/ipmi-kcs-test.c | 7 ---
>>  4 files changed, 28 deletions(-)
>>
>> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
>> index 88f1abf..31cd329 100644
>> --- a/tests/fdc-test.c
>> +++ b/tests/fdc-test.c
>> @@ -548,16 +548,9 @@ static void fuzz_registers(void)
>>  
>>  int main(int argc, char **argv)
>>  {
>> -const char *arch = qtest_get_arch();
>>  int fd;
>>  int ret;
>>  
>> -/* Check architecture */
>> -if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>> -g_test_message("Skipping test for non-x86\n");
>> -return 0;
>> -}
>> -
>>  /* Create a temporary raw image */
>>  fd = mkstemp(test_image);
>>  g_assert(fd >= 0);
>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>> index f0280e6..300d64e 100644
>> --- a/tests/ide-test.c
>> +++ b/tests/ide-test.c
>> @@ -1009,16 +1009,9 @@ static void test_cdrom_dma(void)
>>  
>>  int main(int argc, char **argv)
>>  {
>> -const char *arch = qtest_get_arch();
>>  int fd;
>>  int ret;
>>  
>> -/* Check architecture */
>> -if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>> -g_test_message("Skipping test for non-x86\n");
>> -return 0;
>> -}
>> -
>>  /* Create temporary blkdebug instructions */
>>  fd = mkstemp(debug_path);
>>  g_assert(fd >= 0);
>> diff --git a/tests/ipmi-bt-test.c b/tests/ipmi-bt-test.c
>> index f4a81b5..fc4c83b 100644
>> --- a/tests/ipmi-bt-test.c
>> +++ b/tests/ipmi-bt-test.c
>> @@ -400,15 +400,8 @@ static void open_socket(void)
>>  
>>  int main(int argc, char **argv)
>>  {
>> -const char *arch = qtest_get_arch();
>>  int ret;
>>  
>> -/* Check architecture */
>> -if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>> -g_test_message("Skipping test for non-x86\n");
>> -return 0;
>> -}
>> -
>>  open_socket();
>>  
>>  /* Run the tests */
>> diff --git a/tests/ipmi-kcs-test.c b/tests/ipmi-kcs-test.c
>> index 178ffc1..a2354c1 100644
>> --- a/tests/ipmi-kcs-test.c
>> +++ b/tests/ipmi-kcs-test.c
>> @@ -263,16 +263,9 @@ static void test_enable_irq(void)
>>  
>>  int main(int argc, char **argv)
>>  {
>> -const char *arch = qtest_get_arch();
>>  char *cmdline;
>>  int ret;
>>  
>> -/* Check architecture */
>> -if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>> -g_test_message("Skipping test for non-x86\n");
>> -return 0;
>> -}
>> -
>>  /* Run the tests */
>>  g_test_init(&argc, &argv, NULL);
>>  
>>
> 
> Hm, if you insist. I have no strong feelings... Do we plan to split
> tests out by architecture eventually? Clearly x86 only tests don't
> really need to each individually check the arch, but I'm not sure what
> the vision is.

We could also fix the g_test_message() output by moving it after the
g_test_init() ... I don't mind too much which way we go, but at least
the current state is bad.

Looking at other tests, we seem to be pretty inconsistent in checking
the architecture at the beginning. For example q35-test.c,
pvpanic-test.c and test-x86-cpuid.c do not check for x86, while
rtas-test.c has a check for ppc64...

> Either way, since I have no horse in the race:
> 
> Acked-by: John Snow 

Thanks!

 Thomas

[Qemu-devel] [PATCH V3 5/7] qapi/migration.json: Remove a variable that doesn't exist in example

[Zhang Chen]

From: Zhang Chen 

Remove the "active" variable in example for query-colo-status.
It is a doc bug from commit f56c0065

Signed-off-by: Zhang Chen 
Reviewed-by: Eric Blake 
---
 qapi/migration.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 089ed67807..18929f1154 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1342,7 +1342,7 @@
 # Example:
 #
 # -> { "execute": "query-colo-status" }
-# <- { "return": { "mode": "primary", "active": true, "reason": "request" } }
+# <- { "return": { "mode": "primary", "reason": "request" } }
 #
 # Since: 3.1
 ##
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 6/7] Migration/colo.c: Add the necessary checks for colo_do_failover

[Zhang Chen]

From: Zhang Chen 

Signed-off-by: Zhang Chen 
---
 migration/colo.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index dbe2b88807..d1ae2e6d11 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -197,10 +197,16 @@ void colo_do_failover(MigrationState *s)
 vm_stop_force_state(RUN_STATE_COLO);
 }
 
-if (get_colo_mode() == COLO_MODE_PRIMARY) {
+switch (get_colo_mode()) {
+case COLO_MODE_PRIMARY:
 primary_vm_do_failover();
-} else {
+break;
+case COLO_MODE_SECONDARY:
 secondary_vm_do_failover();
+break;
+default:
+error_report("colo_do_failover failed because the colo mode"
+ " could not be obtained");
 }
 }
 
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 4/7] Migration/colo.c: Add new COLOExitReason to handle all failover state

[Zhang Chen]

From: Zhang Chen 

In this patch we add the processing state for COLOExitReason,
because we have to identify COLO in the failover processing state or
failover error state. In the way, we can handle all the failover state.
We have improved the description of the COLOExitReason by the way.

Signed-off-by: Zhang Chen 
---
 migration/colo.c| 24 +---
 qapi/migration.json | 15 +--
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 89325952c7..dbe2b88807 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -267,7 +267,11 @@ COLOStatus *qmp_query_colo_status(Error **errp)
 s->reason = COLO_EXIT_REASON_REQUEST;
 break;
 default:
-s->reason = COLO_EXIT_REASON_ERROR;
+if (migration_in_colo_state()) {
+s->reason = COLO_EXIT_REASON_PROCESSING;
+} else {
+s->reason = COLO_EXIT_REASON_ERROR;
+}
 }
 
 return s;
@@ -579,16 +583,13 @@ out:
  * or the user triggered failover.
  */
 switch (failover_get_state()) {
-case FAILOVER_STATUS_NONE:
-qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
-  COLO_EXIT_REASON_ERROR);
-break;
 case FAILOVER_STATUS_COMPLETED:
 qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
   COLO_EXIT_REASON_REQUEST);
 break;
 default:
-abort();
+qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
+  COLO_EXIT_REASON_ERROR);
 }
 
 /* Hope this not to be too long to wait here */
@@ -850,17 +851,18 @@ out:
 error_report_err(local_err);
 }
 
+/*
+ * There are only two reasons we can get here, some error happened
+ * or the user triggered failover.
+ */
 switch (failover_get_state()) {
-case FAILOVER_STATUS_NONE:
-qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
-  COLO_EXIT_REASON_ERROR);
-break;
 case FAILOVER_STATUS_COMPLETED:
 qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
   COLO_EXIT_REASON_REQUEST);
 break;
 default:
-abort();
+qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
+  COLO_EXIT_REASON_ERROR);
 }
 
 if (fb) {
diff --git a/qapi/migration.json b/qapi/migration.json
index 7a795ecc16..089ed67807 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -983,19 +983,22 @@
 ##
 # @COLOExitReason:
 #
-# The reason for a COLO exit
+# Describe the reason for COLO exit.
 #
-# @none: no failover has ever happened. This can't occur in the
-# COLO_EXIT event, only in the result of query-colo-status.
+# @none: failover has never happened. This state does not occur
+# in the COLO_EXIT event, and is only visible in the result of
+# query-colo-status.
 #
-# @request: COLO exit is due to an external request
+# @request: COLO exit caused by an external request.
 #
-# @error: COLO exit is due to an internal error
+# @error: COLO exit caused by an internal error.
+#
+# @processing: COLO is currently handling a failover (since 4.0).
 #
 # Since: 3.1
 ##
 { 'enum': 'COLOExitReason',
-  'data': [ 'none', 'request', 'error' ] }
+  'data': [ 'none', 'request', 'error' , 'processing' ] }
 
 ##
 # @x-colo-lost-heartbeat:
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 3/7] Migration/colo.c: Make COLO node running after failover

[Zhang Chen]

From: Zhang Chen 

Delay to close COLO for auto start VM after failover.

Signed-off-by: Zhang Chen 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/colo.c  | 1 -
 migration/migration.c | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index a13acac192..89325952c7 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -875,7 +875,6 @@ out:
 qemu_fclose(mis->to_src_file);
 mis->to_src_file = NULL;
 }
-migration_incoming_disable_colo();
 
 rcu_unregister_thread();
 return NULL;
diff --git a/migration/migration.c b/migration/migration.c
index 37e06b76dc..cec5f529c3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -383,6 +383,9 @@ static void process_incoming_migration_bh(void *opaque)
 } else {
 runstate_set(RUN_STATE_PAUSED);
 }
+} else if (migration_incoming_colo_enabled()) {
+migration_incoming_disable_colo();
+vm_start();
 } else {
 runstate_set(global_state_get_runstate());
 }
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 7/7] Migration/colo.c: Make user obtain the COLO mode info after failover

[Zhang Chen]

From: Zhang Chen 

Add the last_colo_mode to save the status after failover.
This patch can solve the issue that user got nothing to call
query_colo_status after failover.

Signed-off-by: Zhang Chen 
---
 migration/colo.c | 28 +++-
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index d1ae2e6d11..6eba8e06f2 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -38,6 +38,9 @@
 static bool vmstate_loading;
 static Notifier packets_compare_notifier;
 
+/* User need to know colo mode after COLO failover */
+static COLOMode last_colo_mode;
+
 #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
 
 bool migration_in_colo_state(void)
@@ -197,7 +200,10 @@ void colo_do_failover(MigrationState *s)
 vm_stop_force_state(RUN_STATE_COLO);
 }
 
-switch (get_colo_mode()) {
+/* Update last_COLO_mode to avoid unexpectedly exit COLO status */
+last_colo_mode = get_colo_mode();
+
+switch (last_colo_mode) {
 case COLO_MODE_PRIMARY:
 primary_vm_do_failover();
 break;
@@ -263,7 +269,7 @@ COLOStatus *qmp_query_colo_status(Error **errp)
 {
 COLOStatus *s = g_new0(COLOStatus, 1);
 
-s->mode = get_colo_mode();
+s->mode = last_colo_mode;
 
 switch (failover_get_state()) {
 case FAILOVER_STATUS_NONE:
@@ -515,6 +521,12 @@ static void colo_process_checkpoint(MigrationState *s)
 Error *local_err = NULL;
 int ret;
 
+last_colo_mode = get_colo_mode();
+if (last_colo_mode != COLO_MODE_PRIMARY) {
+error_report("COLO mode must be COLO_MODE_PRIMARY");
+return;
+}
+
 failover_init_state();
 
 s->rp_state.from_dst_file = qemu_file_get_return_path(s->to_dst_file);
@@ -682,12 +694,18 @@ void *colo_process_incoming_thread(void *opaque)
 Error *local_err = NULL;
 int ret;
 
-rcu_register_thread();
-qemu_sem_init(&mis->colo_incoming_sem, 0);
-
 migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
   MIGRATION_STATUS_COLO);
 
+last_colo_mode = get_colo_mode();
+if (last_colo_mode != COLO_MODE_SECONDARY) {
+error_report("COLO mode must be COLO_MODE_SECONDARY");
+return NULL;
+}
+
+rcu_register_thread();
+qemu_sem_init(&mis->colo_incoming_sem, 0);
+
 failover_init_state();
 
 mis->to_src_file = qemu_file_get_return_path(mis->from_src_file);
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 2/7] Migration/colo.c: Fix COLO failover status error

[Zhang Chen]

From: Zhang Chen 

When finished COLO failover, the status is FAILOVER_STATUS_COMPLETED.
The origin codes misunderstand the FAILOVER_STATUS_REQUIRE.

Signed-off-by: Zhang Chen 
---
 migration/colo.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index a916dc178c..a13acac192 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -121,6 +121,7 @@ static void secondary_vm_do_failover(void)
 }
 /* Notify COLO incoming thread that failover work is finished */
 qemu_sem_post(&mis->colo_incoming_sem);
+
 /* For Secondary VM, jump to incoming co */
 if (mis->migration_incoming_co) {
 qemu_coroutine_enter(mis->migration_incoming_co);
@@ -262,7 +263,7 @@ COLOStatus *qmp_query_colo_status(Error **errp)
 case FAILOVER_STATUS_NONE:
 s->reason = COLO_EXIT_REASON_NONE;
 break;
-case FAILOVER_STATUS_REQUIRE:
+case FAILOVER_STATUS_COMPLETED:
 s->reason = COLO_EXIT_REASON_REQUEST;
 break;
 default:
@@ -582,7 +583,7 @@ out:
 qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
   COLO_EXIT_REASON_ERROR);
 break;
-case FAILOVER_STATUS_REQUIRE:
+case FAILOVER_STATUS_COMPLETED:
 qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
   COLO_EXIT_REASON_REQUEST);
 break;
@@ -854,7 +855,7 @@ out:
 qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
   COLO_EXIT_REASON_ERROR);
 break;
-case FAILOVER_STATUS_REQUIRE:
+case FAILOVER_STATUS_COMPLETED:
 qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
   COLO_EXIT_REASON_REQUEST);
 break;
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 1/7] Migration/colo.c: Fix double close bug when occur COLO failover

[Zhang Chen]

From: Zhang Chen 

In migration_incoming_state_destroy(void) will check the mis->to_src_file
to double close the mis->to_src_file when occur COLO failover.

Signed-off-by: Zhang Chen 
Reviewed-by: Dr. David Alan Gilbert 
---
 migration/colo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/colo.c b/migration/colo.c
index 398b239d1c..a916dc178c 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -872,6 +872,7 @@ out:
 /* Must be called after failover BH is completed */
 if (mis->to_src_file) {
 qemu_fclose(mis->to_src_file);
+mis->to_src_file = NULL;
 }
 migration_incoming_disable_colo();
 
-- 
2.17.GIT

[Qemu-devel] [PATCH V3 0/7] Migration/colo: Fix upstream bugs when occur failover

[Zhang Chen]

From: Zhang Chen 

This series focus on COLO failover bug fix and optimization.

V3:
 - Fix grammer issues in patch 4/7.
 - Add more information in commit log of patch 5/7.

V2:
 - Add patch 4/7 to handle failover state.
 - Add new patches to optimize failover status.

V1:
 - Init patch.


Zhang Chen (7):
  Migration/colo.c: Fix double close bug when occur COLO failover
  Migration/colo.c: Fix COLO failover status error
  Migration/colo.c: Make COLO node running after failover
  Migration/colo.c: Add new COLOExitReason to handle all failover state
  qapi/migration.json: Remove a variable that doesn't exist in example
  Migration/colo.c: Add the necessary checks for colo_do_failover
  Migration/colo.c: Make user obtain the COLO mode info after failover

 migration/colo.c  | 69 ++-
 migration/migration.c |  3 ++
 qapi/migration.json   | 17 ++-
 3 files changed, 61 insertions(+), 28 deletions(-)

-- 
2.17.GIT

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[Philippe Mathieu-Daudé]

On 3/3/19 3:04 PM, BALATON Zoltan wrote:
> On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
>> On 3/3/19 1:46 PM, BALATON Zoltan wrote:
>>> On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 37d3264bb2..6aed33eeaa 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -138,3 +138,7 @@ vga_cirrus_write_blt(uint32_t offset, uint32_t
> val) "offset 0x%x, val 0x%x"
> ?sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> ?sii9022_write_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val
> 0x%02x"
> ?sii9022_switch_mode(const char *mode) "mode: %s"
> +
> +# hw/display/ati*.c
> +ati_mm_read(unsigned int size, uint64_t addr, const char *name,
> uint64_t val) "%d 0x%"HWADDR_PRIx " %s -> 0x%"PRIx64
> +ati_mm_write(unsigned int size, uint64_t addr, const char *name,
> uint64_t val) "%d 0x%"HWADDR_PRIx " %s <- 0x%"PRIx64

 "%u ...
>>>
>>> Should not matter as size that can only be 1 to 4 but %u is more correct
>>> to print unsigned variable so I'll change this.
>>
>> The argument is unsigned, so the correct format is %u.
>>
>> This help reducing the thousands of warnings in QEMU about incorrect
>> formatstring.
> 
> Yes, I've corrected this. I did not get warnings for it but maybe some
> compilers are pickier about this.

We currently don't use this warning (yet) because there are plenty of
incorrect formatstring, simply because we never cared.
One day we might care. Or we could enable these warnings on a subsystem
basis (but we'd still need to clean the headers first).

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[Aleksandar Markovic]

On Sunday, March 3, 2019, BALATON Zoltan  wrote:

> At least two machines, the PPC mac99 and MIPS fulong2e, have an ATI
> gfx chip by default (Rage 128 Pro and M6/RV100 respectively) and
> guests running on these and the PMON2000 firmware of the fulong2e
> expect this to be available. Fortunately these are very similar chips
> so they can be mostly emulated in the same device model. This patch
> adds basic emulation of these ATI VGA chips.


I am not familiar enough with display/graphics code in QEMU to give this
patch an official "reviewed-by", but from the standpoint of MIPS (Fulong 2E
is a MIPS-based board), this patch is a desirable one, and we want it, if
possible, even before March 12th (the planned soft freeze date). Though, I
am not rushing anyone in any way. So, FWIW:

Acked-by: Aleksandar Markovic 


> While this is incomplete and currently only enough to run the MIPS
> firmware and get framebuffer output with Linux, it allows the fulong2e
> board to work more like the real hardware and having it in QEMU in
> this state provides a way to experiment with it and allows others to
> contribute to improve it. It is compiled for all archs but only the
> fulong2e (which currently has no display output at all) is set to use
> it by default (in a patch sent separately).
>
> Signed-off-by: BALATON Zoltan 
> ---
> v4:
> - fix mingw build (from Gerd)
> - set dev_id in realize to allow pci_patch_ids to change bios rom
> - add model aliases to select device variant by name instead of id
> - misc mode switch and 2d fixes (better but still not quite right)
>
> v3:
> - add to default-configs/pci.mak instead of mips64el and ppc only
> - rename device_id property to x-device-id
> - use extract32/deposit32 in *_offs functions
> - add ati-vga to vl.c default_list[]
>
> v2:
> - Extended debug logs
> - Fix mode switching and some registers
> - Fixes to 2D functions
>
>  default-configs/pci.mak  |   1 +
>  hw/display/Makefile.objs |   2 +
>  hw/display/ati.c | 686 ++
> +
>  hw/display/ati_2d.c  | 134 +
>  hw/display/ati_dbg.c | 254 ++
>  hw/display/ati_int.h |  87 ++
>  hw/display/ati_regs.h| 456 +++
>  hw/display/trace-events  |   4 +
>  vl.c |   1 +
>  9 files changed, 1625 insertions(+)
>  create mode 100644 hw/display/ati.c
>  create mode 100644 hw/display/ati_2d.c
>  create mode 100644 hw/display/ati_dbg.c
>  create mode 100644 hw/display/ati_int.h
>  create mode 100644 hw/display/ati_regs.h
>
> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> index 037636fa33..e59e2fa7b6 100644
> --- a/default-configs/pci.mak
> +++ b/default-configs/pci.mak
> @@ -49,3 +49,4 @@ CONFIG_IVSHMEM_DEVICE=$(CONFIG_IVSHMEM)
>  CONFIG_ROCKER=y
>  CONFIG_VFIO=$(CONFIG_LINUX)
>  CONFIG_VFIO_PCI=y
> +CONFIG_ATI_VGA=y
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 7c4ae9a0fd..963c23f3c8 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -53,3 +53,5 @@ virtio-gpu-3d.o-cflags := $(VIRGL_CFLAGS)
>  virtio-gpu-3d.o-libs += $(VIRGL_LIBS)
>  obj-$(CONFIG_DPCD) += dpcd.o
>  obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx_dp.o
> +
> +obj-$(CONFIG_ATI_VGA) += ati.o ati_2d.o ati_dbg.o
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> new file mode 100644
> index 00..72dd9b4953
> --- /dev/null
> +++ b/hw/display/ati.c
> @@ -0,0 +1,686 @@
> +/*
> + * QEMU ATI SVGA emulation
> + *
> + * Copyright (c) 2019 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + */
> +
> +/*
> + * WARNING:
> + * This is very incomplete and only enough for Linux console and some
> + * unaccelerated X output at the moment.
> + * Currently it's little more than a frame buffer with minimal functions,
> + * other more advanced features of the hardware are yet to be implemented.
> + * We only aim for Rage 128 Pro (and some RV100) and 2D only at first,
> + * No 3D at all yet (maybe after 2D works, but feel free to improve it)
> + */
> +
> +#include "ati_int.h"
> +#include "ati_regs.h"
> +#include "vga_regs.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "hw/hw.h"
> +#include "ui/console.h"
> +#include "trace.h"
> +
> +static struct {
> +const char *name;
> +uint16_t dev_id;
> +} ati_model_aliases[] = {
> +{ "rage128p", PCI_DEVICE_ID_ATI_RAGE128_PF },
> +{ "rv100", PCI_DEVICE_ID_ATI_RADEON_QY },
> +};
> +
> +enum { VGA_MODE, EXT_MODE };
> +
> +static void ati_vga_switch_mode(ATIVGAState *s)
> +{
> +DPRINTF("%d -> %d\n",
> +s->mode, !!(s->regs.crtc_gen_cntl & CRTC2_EXT_DISP_EN));
> +if (s->regs.crtc_gen_cntl & CRTC2_EXT_DISP_EN) {
> +/* Extended mode enabled */
> +s->mode = EXT_MODE;
> +if (s->regs.crtc_gen_cntl & CRTC2_EN) {
> +/* CRT controller enabled, use CRTC values */
> +uint32_t offs = s-

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[Philippe Mathieu-Daudé]

On 3/3/19 1:46 PM, BALATON Zoltan wrote:
> On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
>> Hi Zoltan,
>>
>> On 3/3/19 12:34 AM, BALATON Zoltan wrote:
>>> At least two machines, the PPC mac99 and MIPS fulong2e, have an ATI
>>> gfx chip by default (Rage 128 Pro and M6/RV100 respectively) and
>>> guests running on these and the PMON2000 firmware of the fulong2e
>>> expect this to be available. Fortunately these are very similar chips
>>> so they can be mostly emulated in the same device model. This patch
>>> adds basic emulation of these ATI VGA chips.
>>>
>>> While this is incomplete and currently only enough to run the MIPS
>>> firmware and get framebuffer output with Linux, it allows the fulong2e
>>> board to work more like the real hardware and having it in QEMU in
>>> this state provides a way to experiment with it and allows others to
>>> contribute to improve it. It is compiled for all archs but only the
>>> fulong2e (which currently has no display output at all) is set to use
>>> it by default (in a patch sent separately).
>>
>> Patch looks good, trivial comment inlined.
> 
> Hello,
> Thanks for yhe review. I took what I liked, [...]

So for your next version:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[Philippe Mathieu-Daudé]

On 3/3/19 4:40 PM, Aleksandar Markovic wrote:
> On Sunday, March 3, 2019, BALATON Zoltan  wrote:
> 
>> At least two machines, the PPC mac99 and MIPS fulong2e, have an ATI
>> gfx chip by default (Rage 128 Pro and M6/RV100 respectively) and
>> guests running on these and the PMON2000 firmware of the fulong2e
>> expect this to be available. Fortunately these are very similar chips
>> so they can be mostly emulated in the same device model. This patch
>> adds basic emulation of these ATI VGA chips.
> 
> 
> I am not familiar enough with display/graphics code in QEMU to give this
> patch an official "reviewed-by", but from the standpoint of MIPS (Fulong 2E
> is a MIPS-based board), this patch is a desirable one, and we want it, if
> possible, even before March 12th (the planned soft freeze date). Though, I
> am not rushing anyone in any way. So, FWIW:
> 
> Acked-by: Aleksandar Markovic 

Tested-by: Philippe Mathieu-Daudé 

Using
$ qemu-system-mips64el -M fulong2e -bios pmon_2e.bin -device ati-vga

Regards,

Phil.

>> While this is incomplete and currently only enough to run the MIPS
>> firmware and get framebuffer output with Linux, it allows the fulong2e
>> board to work more like the real hardware and having it in QEMU in
>> this state provides a way to experiment with it and allows others to
>> contribute to improve it. It is compiled for all archs but only the
>> fulong2e (which currently has no display output at all) is set to use
>> it by default (in a patch sent separately).
>>
>> Signed-off-by: BALATON Zoltan 

Re: [Qemu-devel] [PATCH] Reduce curses escdelay from 1s to 0.2s

[Warner Losh]

On Sun, Mar 3, 2019, 12:45 AM Samuel Thibault 
wrote:

> By default, curses will only report single ESC key event after 1s delay,
> since ESC is also used for keypad escape sequences. This however makes
> users
> believe that ESC is not working. Reducing to 0.2s provides good enough user
> experience, while still allowing 200ms for keypad sequences to get in,
> which
> should be more than enough.
>

How did you come up with this number? Still seems a bit long for the ESC
ESC case where the user hits ESC twice in quick succession. Even back in
the day, terminals would send the characters back to back at 1200 baud,
which is 8ms per character. 32ms is 4x that, 32x 9600 baud rates. 25 or
50ms is suggested from these figures.

Warner

Signed-off-by: Samuel Thibault 
> ---
>  ui/curses.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/curses.c b/ui/curses.c
> index 6e0091c3b2..700315bc09 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -231,7 +231,7 @@ static void curses_refresh(DisplayChangeListener *dcl)
>  keycode = curses2keycode[chr];
>  keycode_alt = 0;
>
> -/* alt key */
> +/* alt or esc key */
>  if (keycode == 1) {
>  int nextchr = getch();
>
> @@ -361,6 +361,7 @@ static void curses_setup(void)
>  initscr(); noecho(); intrflush(stdscr, FALSE);
>  nodelay(stdscr, TRUE); nonl(); keypad(stdscr, TRUE);
>  start_color(); raw(); scrollok(stdscr, FALSE);
> +set_escdelay(200);
>
>  /* Make color pair to match color format (3bits bg:3bits fg) */
>  for (i = 0; i < 64; i++) {
> --
> 2.20.1
>
>
>

[Qemu-devel] [PATCH 1/8] target/ppc: introduce single fpr_offset() function

[Mark Cave-Ayland]

Instead of having multiple copies of the offset calculation logic, move it to a
single fpr_offset() function.

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h   | 7 ++-
 target/ppc/translate.c | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 26604ddf98..4bb4e42670 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2563,9 +2563,14 @@ static inline bool lsw_reg_in_range(int start, int 
nregs, int rx)
 }
 
 /* Accessors for FP, VMX and VSX registers */
+static inline int fpr_offset(int i)
+{
+return offsetof(CPUPPCState, vsr[i].u64[0]);
+}
+
 static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
 {
-return &env->vsr[i].u64[0];
+return (uint64_t *)((uintptr_t)env + fpr_offset(i));
 }
 
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 819221f246..3b1992faf1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6677,12 +6677,12 @@ GEN_TM_PRIV_NOOP(trechkpt);
 
 static inline void get_fpr(TCGv_i64 dst, int regno)
 {
-tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
+tcg_gen_ld_i64(dst, cpu_env, fpr_offset(regno));
 }
 
 static inline void set_fpr(int regno, TCGv_i64 src)
 {
-tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
+tcg_gen_st_i64(src, cpu_env, fpr_offset(regno));
 }
 
 static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
-- 
2.11.0

[Qemu-devel] [PATCH 4/8] target/ppc: introduce avrh_offset() and avrl_offset() functions

[Mark Cave-Ayland]

These will become more useful later, but initially use this as an aid to
simplify the offset calculation by replacing the HOST_TARGET_BIGENDIAN
sections with the VsrD macro.

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h   | 10 ++
 target/ppc/translate.c | 24 ++--
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index d0580c6b6d..326593e0e7 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2603,6 +2603,16 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, 
int i)
 return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
 }
 
+static inline int avrh_offset(int i)
+{
+return offsetof(CPUPPCState, vsr[32 + i].VsrD(0));
+}
+
+static inline int avrl_offset(int i)
+{
+return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
+}
+
 static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
 {
 return &env->vsr[32 + i];
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 3b1992faf1..f646f359e7 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6687,24 +6687,20 @@ static inline void set_fpr(int regno, TCGv_i64 src)
 
 static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
-  vsr[32 + regno].u64[(high ? 0 : 
1)]));
-#else
-tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
-  vsr[32 + regno].u64[(high ? 1 : 
0)]));
-#endif
+if (high) {
+tcg_gen_ld_i64(dst, cpu_env, avrh_offset(regno));
+} else {
+tcg_gen_ld_i64(dst, cpu_env, avrl_offset(regno));
+}
 }
 
 static inline void set_avr64(int regno, TCGv_i64 src, bool high)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState,
-  vsr[32 + regno].u64[(high ? 0 : 
1)]));
-#else
-tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState,
-  vsr[32 + regno].u64[(high ? 1 : 
0)]));
-#endif
+if (high) {
+tcg_gen_st_i64(src, cpu_env, avrh_offset(regno));
+} else {
+tcg_gen_st_i64(src, cpu_env, avrl_offset(regno));
+}
 }
 
 #include "translate/fp-impl.inc.c"
-- 
2.11.0

[Qemu-devel] [PATCH 6/8] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Mark Cave-Ayland]

When VSX support was initially added, the fpr registers were added at
offset 0 of the VSR register and the vsrl registers were added at offset
1. This is in contrast to the VMX registers (the last 32 VSX registers) which
are stored in host-endian order.

Switch the fpr/vsrl registers so that the lower 32 VSX registers are now also
stored in host endian order to match the VMX registers. This ensures that TCG
vector operations involving mixed VMX and VSX registers will function
correctly.

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h  | 4 ++--
 target/ppc/internal.h | 8 
 target/ppc/machine.c  | 8 
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 89651988ab..faae25a566 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2585,7 +2585,7 @@ static inline bool lsw_reg_in_range(int start, int nregs, 
int rx)
 
 static inline int fpr_offset(int i)
 {
-return offsetof(CPUPPCState, vsr[i].u64[0]);
+return offsetof(CPUPPCState, vsr[i].VsrD(0));
 }
 
 static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
@@ -2595,7 +2595,7 @@ static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int 
i)
 
 static inline int vsrl_offset(int i)
 {
-return offsetof(CPUPPCState, vsr[i].u64[1]);
+return offsetof(CPUPPCState, vsr[i].VsrD(1));
 }
 
 static inline int vsr_full_offset(int i)
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 3ebbdf4da4..fb6f64ed1e 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -206,14 +206,14 @@ EXTRACT_HELPER_SPLIT_3(DCMX_XV, 5, 16, 0, 1, 2, 5, 1, 6, 
6);
 
 static inline void getVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
 {
-vsr->VsrD(0) = env->vsr[n].u64[0];
-vsr->VsrD(1) = env->vsr[n].u64[1];
+vsr->VsrD(0) = env->vsr[n].VsrD(0);
+vsr->VsrD(1) = env->vsr[n].VsrD(1);
 }
 
 static inline void putVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
 {
-env->vsr[n].u64[0] = vsr->VsrD(0);
-env->vsr[n].u64[1] = vsr->VsrD(1);
+env->vsr[n].VsrD(0) = vsr->VsrD(0);
+env->vsr[n].VsrD(1) = vsr->VsrD(1);
 }
 
 void helper_compute_fprf_float16(CPUPPCState *env, float16 arg);
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 756b6d2971..a92d0ad3a3 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -150,7 +150,7 @@ static int get_fpr(QEMUFile *f, void *pv, size_t size,
 {
 ppc_vsr_t *v = pv;
 
-v->u64[0] = qemu_get_be64(f);
+v->VsrD(0) = qemu_get_be64(f);
 
 return 0;
 }
@@ -160,7 +160,7 @@ static int put_fpr(QEMUFile *f, void *pv, size_t size,
 {
 ppc_vsr_t *v = pv;
 
-qemu_put_be64(f, v->u64[0]);
+qemu_put_be64(f, v->VsrD(0));
 return 0;
 }
 
@@ -181,7 +181,7 @@ static int get_vsr(QEMUFile *f, void *pv, size_t size,
 {
 ppc_vsr_t *v = pv;
 
-v->u64[1] = qemu_get_be64(f);
+v->VsrD(1) = qemu_get_be64(f);
 
 return 0;
 }
@@ -191,7 +191,7 @@ static int put_vsr(QEMUFile *f, void *pv, size_t size,
 {
 ppc_vsr_t *v = pv;
 
-qemu_put_be64(f, v->u64[1]);
+qemu_put_be64(f, v->VsrD(1));
 return 0;
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH 2/8] target/ppc: introduce single vsrl_offset() function

[Mark Cave-Ayland]

Instead of having multiple copies of the offset calculation logic, move it to a
single vsrl_offset() function.

This commit also renames the existing get_vsr()/set_vsr() functions to
get_vsrl()/set_vsrl() which better describes their purpose.

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h|  7 ++-
 target/ppc/translate/vsx-impl.inc.c | 12 ++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 4bb4e42670..4a7df13c2d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2573,9 +2573,14 @@ static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, 
int i)
 return (uint64_t *)((uintptr_t)env + fpr_offset(i));
 }
 
+static inline int vsrl_offset(int i)
+{
+return offsetof(CPUPPCState, vsr[i].u64[1]);
+}
+
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
 {
-return &env->vsr[i].u64[1];
+return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
 }
 
 static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
diff --git a/target/ppc/translate/vsx-impl.inc.c 
b/target/ppc/translate/vsx-impl.inc.c
index e73197e717..381ae0f2e9 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1,13 +1,13 @@
 /***   VSX extension   ***/
 
-static inline void get_vsr(TCGv_i64 dst, int n)
+static inline void get_vsrl(TCGv_i64 dst, int n)
 {
-tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
+tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
 }
 
-static inline void set_vsr(int n, TCGv_i64 src)
+static inline void set_vsrl(int n, TCGv_i64 src)
 {
-tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
+tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
 }
 
 static inline int vsr_full_offset(int n)
@@ -27,7 +27,7 @@ static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
 static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
 {
 if (n < 32) {
-get_vsr(dst, n);
+get_vsrl(dst, n);
 } else {
 get_avr64(dst, n - 32, false);
 }
@@ -45,7 +45,7 @@ static inline void set_cpu_vsrh(int n, TCGv_i64 src)
 static inline void set_cpu_vsrl(int n, TCGv_i64 src)
 {
 if (n < 32) {
-set_vsr(n, src);
+set_vsrl(n, src);
 } else {
 set_avr64(n - 32, src, false);
 }
-- 
2.11.0

Re: [Qemu-devel] [PATCH] Reduce curses escdelay from 1s to 0.2s

[Samuel Thibault]

Warner Losh, le dim. 03 mars 2019 10:11:52 -0700, a ecrit:
> On Sun, Mar 3, 2019, 12:45 AM Samuel Thibault 
> <[1]samuel.thiba...@ens-lyon.org>
> wrote:
> 
> By default, curses will only report single ESC key event after 1s delay,
> since ESC is also used for keypad escape sequences. This however makes
> users
> believe that ESC is not working. Reducing to 0.2s provides good enough 
> user
> experience, while still allowing 200ms for keypad sequences to get in,
> which
> should be more than enough.
> 
> How did you come up with this number?

Since the default was very long, I chose a value that felt fast enough
for the user.

> Still seems a bit long for the ESC ESC case where the user hits ESC
> twice in quick succession.

Right, there might be such double-press.

> Even back in the day, terminals would send the characters back to back
> at 1200 baud, which is 8ms per character. 32ms is 4x that, 32x 9600
> baud rates. 25 or 50ms is suggested from these figures.

Alright, let's try 25 then.

Samuel

[Qemu-devel] [PATCH 5/8] target/ppc: introduce avr_offset() function

[Mark Cave-Ayland]

All TCG vector operations require pointers to the base address of the vector
rather than separate access to the top and bottom 64-bits. Convert
the VMX TCG instructions to use a new avr_offset() function instead of
avr64_offset(), which can itself be written as a simple wrapper onto
vsr_full_offset().

After the conversion is complete then avr64_offset() can be removed since its
functionality is now completely within get_avr64()/set_avr64().

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h| 12 +++-
 target/ppc/translate/vmx-impl.inc.c | 27 +++
 target/ppc/translate/vsx-impl.inc.c |  5 -
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 326593e0e7..89651988ab 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2598,6 +2598,11 @@ static inline int vsrl_offset(int i)
 return offsetof(CPUPPCState, vsr[i].u64[1]);
 }
 
+static inline int vsr_full_offset(int i)
+{
+return offsetof(CPUPPCState, vsr[i].u64[0]);
+}
+
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
 {
 return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
@@ -2613,9 +2618,14 @@ static inline int avrl_offset(int i)
 return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
 }
 
+static inline int avr_offset(int i)
+{
+return vsr_full_offset(i + 32);
+}
+
 static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
 {
-return &env->vsr[32 + i];
+return (ppc_avr_t *)((uintptr_t)env + avr_offset(i));
 }
 
 void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env);
diff --git a/target/ppc/translate/vmx-impl.inc.c 
b/target/ppc/translate/vmx-impl.inc.c
index f1b15ae2cb..5f0c96a5e9 100644
--- a/target/ppc/translate/vmx-impl.inc.c
+++ b/target/ppc/translate/vmx-impl.inc.c
@@ -10,15 +10,10 @@
 static inline TCGv_ptr gen_avr_ptr(int reg)
 {
 TCGv_ptr r = tcg_temp_new_ptr();
-tcg_gen_addi_ptr(r, cpu_env, offsetof(CPUPPCState, vsr[32 + reg].u64[0]));
+tcg_gen_addi_ptr(r, cpu_env, avr_offset(reg));
 return r;
 }
 
-static inline long avr64_offset(int reg, bool high)
-{
-return offsetof(CPUPPCState, vsr[32 + reg].u64[(high ? 0 : 1)]);
-}
-
 #define GEN_VR_LDX(name, opc2, opc3)  \
 static void glue(gen_, name)(DisasContext *ctx)
   \
 { \
@@ -205,7 +200,7 @@ static void gen_mtvscr(DisasContext *ctx)
 }
 
 val = tcg_temp_new_i32();
-bofs = avr64_offset(rB(ctx->opcode), true);
+bofs = avr_offset(rB(ctx->opcode));
 #ifdef HOST_WORDS_BIGENDIAN
 bofs += 3 * 4;
 #endif
@@ -284,9 +279,9 @@ static void glue(gen_, name)(DisasContext *ctx) 
\
 }   \
 \
 tcg_op(vece,\
-   avr64_offset(rD(ctx->opcode), true), \
-   avr64_offset(rA(ctx->opcode), true), \
-   avr64_offset(rB(ctx->opcode), true), \
+   avr_offset(rD(ctx->opcode)), \
+   avr_offset(rA(ctx->opcode)), \
+   avr_offset(rB(ctx->opcode)), \
16, 16); \
 }
 
@@ -578,10 +573,10 @@ static void glue(gen_, NAME)(DisasContext *ctx)   
  \
 gen_exception(ctx, POWERPC_EXCP_VPU);   \
 return; \
 }   \
-tcg_gen_gvec_4(avr64_offset(rD(ctx->opcode), true), \
+tcg_gen_gvec_4(avr_offset(rD(ctx->opcode)), \
offsetof(CPUPPCState, vscr_sat), \
-   avr64_offset(rA(ctx->opcode), true), \
-   avr64_offset(rB(ctx->opcode), true), \
+   avr_offset(rA(ctx->opcode)), \
+   avr_offset(rB(ctx->opcode)), \
16, 16, &g); \
 }
 
@@ -755,7 +750,7 @@ static void glue(gen_, name)(DisasContext *ctx) 
\
 return; \
 }   \
 simm = SIMM5(ctx->opcode);  \
-tcg_op(avr64_offset(rD(ctx->opcode), true), 16, 16, simm);  \
+tcg_op(avr_offset(rD(ctx->opcode)), 16, 16, simm);  \
 }
 
 GEN_VXFORM_DUPI(vspltisb, 

[Qemu-devel] [PATCH 0/8] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Mark Cave-Ayland]

After some investigation into Andrew's report of corruption in his ppc64le
tests at https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07234.html, I
discovered the underlying cause was that the first 32 VSX registers are not
stored in host endian order.

This is something that Richard and I had discussed before, but missed that with
VSX if you have source registers from different register sets then even logical
operations will give you the wrong result.

Rather than revert 7b8fe477e1 "target/ppc: convert VSX logical operations to
vector operations" let's keep the use of the accelerated vector instructions,
and instead fix the real problem which is to switch the first 32 VSX registers
to host endian order matching the VMX registers.

Patches 1-5 aim to consolidate the offset calculations for both CPUPPCState
and the associated _ptr() functions into one single place.

With this preliminary work complete, patch 6 switches the first 32 registers
into host endian order without too much difficulty.

Finally now that all VSX registers are stored in the same way, the vsr offset
functions and get_cpu_vsrh()/get_cpu_vsrl() can be simplified accordingly.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (8):
  target/ppc: introduce single fpr_offset() function
  target/ppc: introduce single vsrl_offset() function
  target/ppc: move Vsr* macros from internal.h to cpu.h
  target/ppc: introduce avrh_offset() and avrl_offset() functions
  target/ppc: introduce avr_offset() function
  target/ppc: switch fpr/vsrl registers so all VSX registers are in host
endian order
  target/ppc: introduce vsrh_offset() function
  target/ppc: simplify get_cpu_vsrh() and get_cpu_vsrl() functions

 target/ppc/cpu.h| 56 +++--
 target/ppc/internal.h   | 27 +++---
 target/ppc/machine.c|  8 +++---
 target/ppc/translate.c  | 28 ---
 target/ppc/translate/vmx-impl.inc.c | 27 --
 target/ppc/translate/vsx-impl.inc.c | 39 +++---
 6 files changed, 88 insertions(+), 97 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 7/8] target/ppc: introduce vsrh_offset() function

[Mark Cave-Ayland]

Now that both VSX and VMX registers are in host-endian order we can introduce
a vsrh_offset() function as a replacement for fpr_offset().

In addition the avrh_offset() and avrl_offset() functions can be simplified in
terms of vsrh_offset() and vsrl_offset().

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h   | 16 
 target/ppc/translate.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index faae25a566..9f8eb0bdc0 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2583,16 +2583,11 @@ static inline bool lsw_reg_in_range(int start, int 
nregs, int rx)
 #define VsrSD(i) s64[1 - (i)]
 #endif
 
-static inline int fpr_offset(int i)
+static inline int vsrh_offset(int i)
 {
 return offsetof(CPUPPCState, vsr[i].VsrD(0));
 }
 
-static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
-{
-return (uint64_t *)((uintptr_t)env + fpr_offset(i));
-}
-
 static inline int vsrl_offset(int i)
 {
 return offsetof(CPUPPCState, vsr[i].VsrD(1));
@@ -2603,6 +2598,11 @@ static inline int vsr_full_offset(int i)
 return offsetof(CPUPPCState, vsr[i].u64[0]);
 }
 
+static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
+{
+return (uint64_t *)((uintptr_t)env + vsrh_offset(i));
+}
+
 static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
 {
 return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
@@ -2610,12 +2610,12 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, 
int i)
 
 static inline int avrh_offset(int i)
 {
-return offsetof(CPUPPCState, vsr[32 + i].VsrD(0));
+return vsrh_offset(i + 32);
 }
 
 static inline int avrl_offset(int i)
 {
-return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
+return vsrl_offset(i + 32);
 }
 
 static inline int avr_offset(int i)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index f646f359e7..1c7377d588 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6677,12 +6677,12 @@ GEN_TM_PRIV_NOOP(trechkpt);
 
 static inline void get_fpr(TCGv_i64 dst, int regno)
 {
-tcg_gen_ld_i64(dst, cpu_env, fpr_offset(regno));
+tcg_gen_ld_i64(dst, cpu_env, vsrh_offset(regno));
 }
 
 static inline void set_fpr(int regno, TCGv_i64 src)
 {
-tcg_gen_st_i64(src, cpu_env, fpr_offset(regno));
+tcg_gen_st_i64(src, cpu_env, vsrh_offset(regno));
 }
 
 static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
-- 
2.11.0

[Qemu-devel] [PATCH 3/8] target/ppc: move Vsr* macros from internal.h to cpu.h

[Mark Cave-Ayland]

It isn't possible to include internal.h from cpu.h so move the Vsr* macros
into cpu.h alongside the other VMX/VSX register access functions.

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/cpu.h  | 20 
 target/ppc/internal.h | 19 ---
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 4a7df13c2d..d0580c6b6d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2563,6 +2563,26 @@ static inline bool lsw_reg_in_range(int start, int 
nregs, int rx)
 }
 
 /* Accessors for FP, VMX and VSX registers */
+#if defined(HOST_WORDS_BIGENDIAN)
+#define VsrB(i) u8[i]
+#define VsrSB(i) s8[i]
+#define VsrH(i) u16[i]
+#define VsrSH(i) s16[i]
+#define VsrW(i) u32[i]
+#define VsrSW(i) s32[i]
+#define VsrD(i) u64[i]
+#define VsrSD(i) s64[i]
+#else
+#define VsrB(i) u8[15 - (i)]
+#define VsrSB(i) s8[15 - (i)]
+#define VsrH(i) u16[7 - (i)]
+#define VsrSH(i) s16[7 - (i)]
+#define VsrW(i) u32[3 - (i)]
+#define VsrSW(i) s32[3 - (i)]
+#define VsrD(i) u64[1 - (i)]
+#define VsrSD(i) s64[1 - (i)]
+#endif
+
 static inline int fpr_offset(int i)
 {
 return offsetof(CPUPPCState, vsr[i].u64[0]);
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index f26a71ffcf..3ebbdf4da4 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -204,25 +204,6 @@ EXTRACT_HELPER(IMM8, 11, 8);
 EXTRACT_HELPER(DCMX, 16, 7);
 EXTRACT_HELPER_SPLIT_3(DCMX_XV, 5, 16, 0, 1, 2, 5, 1, 6, 6);
 
-#if defined(HOST_WORDS_BIGENDIAN)
-#define VsrB(i) u8[i]
-#define VsrSB(i) s8[i]
-#define VsrH(i) u16[i]
-#define VsrSH(i) s16[i]
-#define VsrW(i) u32[i]
-#define VsrSW(i) s32[i]
-#define VsrD(i) u64[i]
-#define VsrSD(i) s64[i]
-#else
-#define VsrB(i) u8[15 - (i)]
-#define VsrSB(i) s8[15 - (i)]
-#define VsrH(i) u16[7 - (i)]
-#define VsrSH(i) s16[7 - (i)]
-#define VsrW(i) u32[3 - (i)]
-#define VsrSW(i) s32[3 - (i)]
-#define VsrD(i) u64[1 - (i)]
-#define VsrSD(i) s64[1 - (i)]
-#endif
 static inline void getVSR(int n, ppc_vsr_t *vsr, CPUPPCState *env)
 {
 vsr->VsrD(0) = env->vsr[n].u64[0];
-- 
2.11.0

[Qemu-devel] [PATCH 8/8] target/ppc: simplify get_cpu_vsrh() and get_cpu_vsrl() functions

[Mark Cave-Ayland]

Now that the VSX registers are all in host endian order, there is no need to
go via different accessors depending upon the register number. Instead the
high and low parts can be accessed directly via vsrh_offset() and vsrl_offset()
accordingly.

Signed-off-by: Mark Cave-Ayland 
---
 target/ppc/translate/vsx-impl.inc.c | 34 --
 1 file changed, 4 insertions(+), 30 deletions(-)

diff --git a/target/ppc/translate/vsx-impl.inc.c 
b/target/ppc/translate/vsx-impl.inc.c
index 7d02a235e7..43e97756a2 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -1,49 +1,23 @@
 /***   VSX extension   ***/
 
-static inline void get_vsrl(TCGv_i64 dst, int n)
-{
-tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
-}
-
-static inline void set_vsrl(int n, TCGv_i64 src)
-{
-tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
-}
-
 static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
 {
-if (n < 32) {
-get_fpr(dst, n);
-} else {
-get_avr64(dst, n - 32, true);
-}
+tcg_gen_ld_i64(dst, cpu_env, vsrh_offset(n));
 }
 
 static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
 {
-if (n < 32) {
-get_vsrl(dst, n);
-} else {
-get_avr64(dst, n - 32, false);
-}
+tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
 }
 
 static inline void set_cpu_vsrh(int n, TCGv_i64 src)
 {
-if (n < 32) {
-set_fpr(n, src);
-} else {
-set_avr64(n - 32, src, true);
-}
+tcg_gen_st_i64(src, cpu_env, vsrh_offset(n));
 }
 
 static inline void set_cpu_vsrl(int n, TCGv_i64 src)
 {
-if (n < 32) {
-set_vsrl(n, src);
-} else {
-set_avr64(n - 32, src, false);
-}
+tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
 }
 
 #define VSX_LOAD_SCALAR(name, operation)  \
-- 
2.11.0

[Qemu-devel] [PULLv2] Reduce curses escdelay from 1s to 25ms

[Samuel Thibault]

By default, curses will only report single ESC key event after 1s delay,
since ESC is also used for keypad escape sequences. This however makes
users believe that ESC is not working. Reducing to 25ms provides good user
experience, while still allowing 25ms for keypad sequences to get in, which
should be enough.

Signed-off-by: Samuel Thibault 
---
 ui/curses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/curses.c b/ui/curses.c
index 6e0091c3b2..870273de51 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -231,7 +231,7 @@ static void curses_refresh(DisplayChangeListener *dcl)
 keycode = curses2keycode[chr];
 keycode_alt = 0;
 
-/* alt key */
+/* alt or esc key */
 if (keycode == 1) {
 int nextchr = getch();
 
@@ -361,6 +361,7 @@ static void curses_setup(void)
 initscr(); noecho(); intrflush(stdscr, FALSE);
 nodelay(stdscr, TRUE); nonl(); keypad(stdscr, TRUE);
 start_color(); raw(); scrollok(stdscr, FALSE);
+set_escdelay(25);
 
 /* Make color pair to match color format (3bits bg:3bits fg) */
 for (i = 0; i < 64; i++) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v6 62/73] cpu: introduce cpu_has_work_with_iothread_lock

[Emilio G. Cota]

On Fri, Feb 08, 2019 at 11:33:32 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > It will gain some users soon.
> >
> > Suggested-by: Paolo Bonzini 
> > Reviewed-by: Richard Henderson 
> > Signed-off-by: Emilio G. Cota 
> > ---
> >  include/qom/cpu.h | 36 +---
(snip)
> >  static inline bool cpu_has_work(CPUState *cpu)
> >  {
> >  CPUClass *cc = CPU_GET_CLASS(cpu);
> > +bool has_cpu_lock = cpu_mutex_locked(cpu);
> > +bool (*func)(CPUState *cpu);
> >  bool ret;
> >
> > +if (cc->has_work_with_iothread_lock) {
> > +if (qemu_mutex_iothread_locked()) {
> > +func = cc->has_work_with_iothread_lock;
> > +goto call_func;
> > +}
> > +
> > +if (has_cpu_lock) {
> > +/* avoid deadlock by acquiring the locks in order */
> 
> This is fine here but can we expand the comment above:
> 
>  * cpu_has_work:
>  * @cpu: The vCPU to check.
>  *
>  * Checks whether the CPU has work to do. If the vCPU helper needs to
>  * check it's work status with the BQL held ensure we hold the BQL
>  * before taking the CPU lock.

I added a comment to the body of the function:

+/* some targets require us to hold the BQL when checking for work */
 if (cc->has_work_with_iothread_lock) {

> Where is our canonical description of the locking interaction between
> BQL and CPU locks?

It's in a few places, for instance cpu_mutex_lock's documentation
in include/qom/cpu.h.

I've added a comment about the locking order to @lock's documentation,
also in cpu.h:

- * @lock: Lock to prevent multiple access to per-CPU fields.
+ * @lock: Lock to prevent multiple access to per-CPU fields. Must be acqrd
+ *after the BQL.

> Otherwise:
> 
> Reviewed-by: Alex Bennée 

Thanks!

Emilio

[Qemu-devel] [PATCH v4 0/9] Misc fixes to pvrdma device

[Yuval Shaia]

Hi,
Please review the following patch-set which consist of cosmetics fixes to
device's user interface (traces, error_report and monitor) and some bug
fixes.

Thanks Markus, Eric, Marcel and David for reviewing v0.

David, please see version notes below, since i restructured the HMP part
your second review is needed, thanks in advance.

v0 -> v1:
* Explain why device attributes are exposed only in HMP interface.
* Squash the 3 patches related to HMP interface into one.
* Make monitor dump function simple.
* Make HMP interface available only if pvrdma is included (detected by
  build robot).
* Remove patch 03/10 ("Warn when too many consecutive poll CQ triggered
  on an empty CQ) and add the two counters to patch 0/7 (monitor).
* Add Marcel's R-Bs.
* Add mutex protection to cqe_ctx list.
* Add two new patches.

v1 -> v2:
* Rename locked-lists to protected-lists in patch 2 and patch 6.
* Add Marcel's R-Bs.

v2 -> v3:
* Address some 32 bit host compilation issues.

v3 -> v4:
* Per suggestion from Markus rebase HMP report on
  object_child_foreach_recursive().
* Strip off David's r-b from HMP patch (#4) because of the above.

Yuval Shaia (9):
  hw/rdma: Switch to generic error reporting way
  hw/rdma: Introduce protected qlist
  hw/rdma: Protect against concurrent execution of poll_cq
  {hmp,hw/pvrdma}: Expose device internals via monitor interface
  hw/rdma: Free all MAD receive buffers when device is closed
  hw/rdma: Free all receive buffers when QP is destroyed
  hw/pvrdma: Delete unneeded function argument
  hw/pvrdma: Delete pvrdma_exit function
  hw/pvrdma: Unregister from shutdown notifier when device goes down

 hmp-commands-info.hx  |  14 +
 hmp.c |  27 ++
 hmp.h |   1 +
 hw/rdma/Makefile.objs |   2 +-
 hw/rdma/rdma_backend.c| 483 +-
 hw/rdma/rdma_backend.h|   3 +-
 hw/rdma/rdma_backend_defs.h   |  10 +-
 hw/rdma/rdma_hmp.c|  30 +++
 hw/rdma/rdma_rm.c | 193 --
 hw/rdma/rdma_rm.h |   7 +-
 hw/rdma/rdma_rm_defs.h|  28 +-
 hw/rdma/rdma_utils.c  |  83 +-
 hw/rdma/rdma_utils.h  |  61 ++---
 hw/rdma/trace-events  |  32 ++-
 hw/rdma/vmw/pvrdma.h  |   7 +-
 hw/rdma/vmw/pvrdma_cmd.c  | 113 ++--
 hw/rdma/vmw/pvrdma_dev_ring.c |  26 +-
 hw/rdma/vmw/pvrdma_main.c | 161 +---
 hw/rdma/vmw/pvrdma_qp_ops.c   |  52 +---
 hw/rdma/vmw/trace-events  |  16 +-
 include/hw/rdma/rdma_hmp.h|  40 +++
 21 files changed, 779 insertions(+), 610 deletions(-)
 create mode 100644 hw/rdma/rdma_hmp.c
 create mode 100644 include/hw/rdma/rdma_hmp.h

-- 
2.17.2

[Qemu-devel] [PATCH v4 5/9] hw/rdma: Free all MAD receive buffers when device is closed

[Yuval Shaia]

When device is going down free all saved MAD buffers.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c| 34 +-
 hw/rdma/vmw/pvrdma_main.c |  2 ++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index bc2fefcf93..a65f5737e4 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -64,6 +64,33 @@ static inline void complete_work(enum ibv_wc_status status, 
uint32_t vendor_err,
 comp_handler(ctx, &wc);
 }
 
+static void free_cqe_ctx(gpointer data, gpointer user_data)
+{
+BackendCtx *bctx;
+RdmaDeviceResources *rdma_dev_res = user_data;
+unsigned long cqe_ctx_id = GPOINTER_TO_INT(data);
+
+bctx = rdma_rm_get_cqe_ctx(rdma_dev_res, cqe_ctx_id);
+if (bctx) {
+rdma_rm_dealloc_cqe_ctx(rdma_dev_res, cqe_ctx_id);
+}
+g_free(bctx);
+}
+
+static void clean_recv_mads(RdmaBackendDev *backend_dev)
+{
+unsigned long cqe_ctx_id;
+
+do {
+cqe_ctx_id = rdma_protected_qlist_pop_int64(&backend_dev->
+recv_mads_list);
+if (cqe_ctx_id != -ENOENT) {
+free_cqe_ctx(GINT_TO_POINTER(cqe_ctx_id),
+ backend_dev->rdma_dev_res);
+}
+} while (cqe_ctx_id != -ENOENT);
+}
+
 static int rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq *ibcq)
 {
 int i, ne, total_ne = 0;
@@ -1037,6 +1064,11 @@ static int mad_init(RdmaBackendDev *backend_dev, 
CharBackend *mad_chr_be)
 return 0;
 }
 
+static void mad_stop(RdmaBackendDev *backend_dev)
+{
+clean_recv_mads(backend_dev);
+}
+
 static void mad_fini(RdmaBackendDev *backend_dev)
 {
 disable_rdmacm_mux_async(backend_dev);
@@ -1224,12 +1256,12 @@ void rdma_backend_start(RdmaBackendDev *backend_dev)
 
 void rdma_backend_stop(RdmaBackendDev *backend_dev)
 {
+mad_stop(backend_dev);
 stop_backend_thread(&backend_dev->comp_thread);
 }
 
 void rdma_backend_fini(RdmaBackendDev *backend_dev)
 {
-rdma_backend_stop(backend_dev);
 mad_fini(backend_dev);
 g_hash_table_destroy(ah_hash);
 ibv_destroy_comp_channel(backend_dev->channel);
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 659331ac93..b795f80666 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -305,6 +305,8 @@ static void pvrdma_fini(PCIDevice *pdev)
 
 pvrdma_qp_ops_fini();
 
+rdma_backend_stop(&dev->backend_dev);
+
 rdma_rm_fini(&dev->rdma_dev_res, &dev->backend_dev,
  dev->backend_eth_device_name);
 
-- 
2.17.2

[Qemu-devel] [PATCH v4 2/9] hw/rdma: Introduce protected qlist

[Yuval Shaia]

To make code more readable move handling of protected list to a
rdma_utils

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c  | 20 +--
 hw/rdma/rdma_backend_defs.h |  8 ++--
 hw/rdma/rdma_utils.c| 39 +
 hw/rdma/rdma_utils.h|  9 +
 4 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 24bac00a20..0ed14751be 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -527,9 +527,7 @@ static unsigned int save_mad_recv_buffer(RdmaBackendDev 
*backend_dev,
 bctx->up_ctx = ctx;
 bctx->sge = *sge;
 
-qemu_mutex_lock(&backend_dev->recv_mads_list.lock);
-qlist_append_int(backend_dev->recv_mads_list.list, bctx_id);
-qemu_mutex_unlock(&backend_dev->recv_mads_list.lock);
+rdma_protected_qlist_append_int64(&backend_dev->recv_mads_list, bctx_id);
 
 return 0;
 }
@@ -913,23 +911,19 @@ static inline void build_mad_hdr(struct ibv_grh *grh, 
union ibv_gid *sgid,
 static void process_incoming_mad_req(RdmaBackendDev *backend_dev,
  RdmaCmMuxMsg *msg)
 {
-QObject *o_ctx_id;
 unsigned long cqe_ctx_id;
 BackendCtx *bctx;
 char *mad;
 
 trace_mad_message("recv", msg->umad.mad, msg->umad_len);
 
-qemu_mutex_lock(&backend_dev->recv_mads_list.lock);
-o_ctx_id = qlist_pop(backend_dev->recv_mads_list.list);
-qemu_mutex_unlock(&backend_dev->recv_mads_list.lock);
-if (!o_ctx_id) {
+cqe_ctx_id = rdma_protected_qlist_pop_int64(&backend_dev->recv_mads_list);
+if (cqe_ctx_id == -ENOENT) {
 rdma_warn_report("No more free MADs buffers, waiting for a while");
 sleep(THR_POLL_TO);
 return;
 }
 
-cqe_ctx_id = qnum_get_uint(qobject_to(QNum, o_ctx_id));
 bctx = rdma_rm_get_cqe_ctx(backend_dev->rdma_dev_res, cqe_ctx_id);
 if (unlikely(!bctx)) {
 rdma_error_report("No matching ctx for req %ld", cqe_ctx_id);
@@ -994,8 +988,7 @@ static int mad_init(RdmaBackendDev *backend_dev, 
CharBackend *mad_chr_be)
 return -EIO;
 }
 
-qemu_mutex_init(&backend_dev->recv_mads_list.lock);
-backend_dev->recv_mads_list.list = qlist_new();
+rdma_protected_qlist_init(&backend_dev->recv_mads_list);
 
 enable_rdmacm_mux_async(backend_dev);
 
@@ -1010,10 +1003,7 @@ static void mad_fini(RdmaBackendDev *backend_dev)
 {
 disable_rdmacm_mux_async(backend_dev);
 qemu_chr_fe_disconnect(backend_dev->rdmacm_mux.chr_be);
-if (backend_dev->recv_mads_list.list) {
-qlist_destroy_obj(QOBJECT(backend_dev->recv_mads_list.list));
-qemu_mutex_destroy(&backend_dev->recv_mads_list.lock);
-}
+rdma_protected_qlist_destroy(&backend_dev->recv_mads_list);
 }
 
 int rdma_backend_get_gid_index(RdmaBackendDev *backend_dev,
diff --git a/hw/rdma/rdma_backend_defs.h b/hw/rdma/rdma_backend_defs.h
index 15ae8b970e..a8c15b09ab 100644
--- a/hw/rdma/rdma_backend_defs.h
+++ b/hw/rdma/rdma_backend_defs.h
@@ -20,6 +20,7 @@
 #include "chardev/char-fe.h"
 #include 
 #include "contrib/rdmacm-mux/rdmacm-mux.h"
+#include "rdma_utils.h"
 
 typedef struct RdmaDeviceResources RdmaDeviceResources;
 
@@ -30,11 +31,6 @@ typedef struct RdmaBackendThread {
 bool is_running; /* Set by the thread to report its status */
 } RdmaBackendThread;
 
-typedef struct RecvMadList {
-QemuMutex lock;
-QList *list;
-} RecvMadList;
-
 typedef struct RdmaCmMux {
 CharBackend *chr_be;
 int can_receive;
@@ -48,7 +44,7 @@ typedef struct RdmaBackendDev {
 struct ibv_context *context;
 struct ibv_comp_channel *channel;
 uint8_t port_num;
-RecvMadList recv_mads_list;
+RdmaProtectedQList recv_mads_list;
 RdmaCmMux rdmacm_mux;
 } RdmaBackendDev;
 
diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
index b9f07fcda7..0a8abe572d 100644
--- a/hw/rdma/rdma_utils.c
+++ b/hw/rdma/rdma_utils.c
@@ -14,6 +14,8 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnum.h"
 #include "trace.h"
 #include "rdma_utils.h"
 
@@ -51,3 +53,40 @@ void rdma_pci_dma_unmap(PCIDevice *dev, void *buffer, 
dma_addr_t len)
 pci_dma_unmap(dev, buffer, len, DMA_DIRECTION_TO_DEVICE, 0);
 }
 }
+
+void rdma_protected_qlist_init(RdmaProtectedQList *list)
+{
+qemu_mutex_init(&list->lock);
+list->list = qlist_new();
+}
+
+void rdma_protected_qlist_destroy(RdmaProtectedQList *list)
+{
+if (list->list) {
+qlist_destroy_obj(QOBJECT(list->list));
+qemu_mutex_destroy(&list->lock);
+list->list = NULL;
+}
+}
+
+void rdma_protected_qlist_append_int64(RdmaProtectedQList *list, int64_t value)
+{
+qemu_mutex_lock(&list->lock);
+qlist_append_int(list->list, value);
+qemu_mutex_unlock(&list->lock);
+}
+
+int64_t rdma_protected_qlist_pop_int64(RdmaProtectedQList *list)
+{
+QObject *obj;
+
+qemu_mutex_lock(&list->lock);
+obj = qlist_pop(

[Qemu-devel] [PATCH v4 7/9] hw/pvrdma: Delete unneeded function argument

[Yuval Shaia]

The function's argument rdma_dev_res is not needed as it is stored in
the backend_dev object at init.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c  | 13 ++---
 hw/rdma/rdma_backend.h  |  1 -
 hw/rdma/vmw/pvrdma_qp_ops.c |  3 +--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index d511ca282b..66185bd487 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -594,7 +594,6 @@ static unsigned int save_mad_recv_buffer(RdmaBackendDev 
*backend_dev,
 }
 
 void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
-RdmaDeviceResources *rdma_dev_res,
 RdmaBackendQP *qp, uint8_t qp_type,
 struct ibv_sge *sge, uint32_t num_sge, void *ctx)
 {
@@ -613,9 +612,9 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 rc = save_mad_recv_buffer(backend_dev, sge, num_sge, ctx);
 if (rc) {
 complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
-rdma_dev_res->stats.mad_rx_bufs_err++;
+backend_dev->rdma_dev_res->stats.mad_rx_bufs_err++;
 } else {
-rdma_dev_res->stats.mad_rx_bufs++;
+backend_dev->rdma_dev_res->stats.mad_rx_bufs++;
 }
 }
 return;
@@ -625,7 +624,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 bctx->up_ctx = ctx;
 bctx->backend_qp = qp;
 
-rc = rdma_rm_alloc_cqe_ctx(rdma_dev_res, &bctx_id, bctx);
+rc = rdma_rm_alloc_cqe_ctx(backend_dev->rdma_dev_res, &bctx_id, bctx);
 if (unlikely(rc)) {
 complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_NOMEM, ctx);
 goto err_free_bctx;
@@ -633,7 +632,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 
 rdma_protected_gslist_append_int32(&qp->cqe_ctx_list, bctx_id);
 
-rc = build_host_sge_array(rdma_dev_res, new_sge, sge, num_sge,
+rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,
   &backend_dev->rdma_dev_res->stats.rx_bufs_len);
 if (rc) {
 complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
@@ -652,13 +651,13 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 }
 
 atomic_inc(&backend_dev->rdma_dev_res->stats.missing_cqe);
-rdma_dev_res->stats.rx_bufs++;
+backend_dev->rdma_dev_res->stats.rx_bufs++;
 
 return;
 
 err_dealloc_cqe_ctx:
 backend_dev->rdma_dev_res->stats.rx_bufs_err++;
-rdma_rm_dealloc_cqe_ctx(rdma_dev_res, bctx_id);
+rdma_rm_dealloc_cqe_ctx(backend_dev->rdma_dev_res, bctx_id);
 
 err_free_bctx:
 g_free(bctx);
diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index cb5efa2a3a..5d507a1c41 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -111,7 +111,6 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
 union ibv_gid *dgid, uint32_t dqpn, uint32_t dqkey,
 void *ctx);
 void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
-RdmaDeviceResources *rdma_dev_res,
 RdmaBackendQP *qp, uint8_t qp_type,
 struct ibv_sge *sge, uint32_t num_sge, void *ctx);
 
diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
index 16db726dac..508d8fca3c 100644
--- a/hw/rdma/vmw/pvrdma_qp_ops.c
+++ b/hw/rdma/vmw/pvrdma_qp_ops.c
@@ -231,8 +231,7 @@ void pvrdma_qp_recv(PVRDMADev *dev, uint32_t qp_handle)
 continue;
 }
 
-rdma_backend_post_recv(&dev->backend_dev, &dev->rdma_dev_res,
-   &qp->backend_qp, qp->qp_type,
+rdma_backend_post_recv(&dev->backend_dev, &qp->backend_qp, qp->qp_type,
(struct ibv_sge *)&wqe->sge[0], 
wqe->hdr.num_sge,
comp_ctx);
 
-- 
2.17.2

[Qemu-devel] [PATCH v4 3/9] hw/rdma: Protect against concurrent execution of poll_cq

[Yuval Shaia]

The function rdma_poll_cq is called from two contexts - completion
handler thread which sense new completion on backend channel and
explicitly as result of guest issuing poll_cq command.

Add lock to protect against concurrent executions.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c | 2 ++
 hw/rdma/rdma_rm.c  | 4 
 hw/rdma/rdma_rm_defs.h | 1 +
 3 files changed, 7 insertions(+)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 0ed14751be..9679b842d1 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -70,6 +70,7 @@ static void rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, 
struct ibv_cq *ibcq)
 BackendCtx *bctx;
 struct ibv_wc wc[2];
 
+qemu_mutex_lock(&rdma_dev_res->lock);
 do {
 ne = ibv_poll_cq(ibcq, ARRAY_SIZE(wc), wc);
 
@@ -89,6 +90,7 @@ static void rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, 
struct ibv_cq *ibcq)
 g_free(bctx);
 }
 } while (ne > 0);
+qemu_mutex_unlock(&rdma_dev_res->lock);
 
 if (ne < 0) {
 rdma_error_report("ibv_poll_cq fail, rc=%d, errno=%d", ne, errno);
diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 5dab4a2189..14580ca379 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -618,12 +618,16 @@ int rdma_rm_init(RdmaDeviceResources *dev_res, struct 
ibv_device_attr *dev_attr,
 
 init_ports(dev_res);
 
+qemu_mutex_init(&dev_res->lock);
+
 return 0;
 }
 
 void rdma_rm_fini(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev,
   const char *ifname)
 {
+qemu_mutex_destroy(&dev_res->lock);
+
 fini_ports(dev_res, backend_dev, ifname);
 
 res_tbl_free(&dev_res->uc_tbl);
diff --git a/hw/rdma/rdma_rm_defs.h b/hw/rdma/rdma_rm_defs.h
index 0ba61d1838..f0ee1f3072 100644
--- a/hw/rdma/rdma_rm_defs.h
+++ b/hw/rdma/rdma_rm_defs.h
@@ -105,6 +105,7 @@ typedef struct RdmaDeviceResources {
 RdmaRmResTbl cq_tbl;
 RdmaRmResTbl cqe_ctx_tbl;
 GHashTable *qp_hash; /* Keeps mapping between real and emulated */
+QemuMutex lock;
 } RdmaDeviceResources;
 
 #endif
-- 
2.17.2

[Qemu-devel] [PATCH v4 8/9] hw/pvrdma: Delete pvrdma_exit function

[Yuval Shaia]

This hook is not called and was implemented by mistake.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/vmw/pvrdma_main.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index b795f80666..4e4a43eac4 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -641,11 +641,6 @@ out:
 }
 }
 
-static void pvrdma_exit(PCIDevice *pdev)
-{
-pvrdma_fini(pdev);
-}
-
 static void pvrdma_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -653,7 +648,6 @@ static void pvrdma_class_init(ObjectClass *klass, void 
*data)
 RdmaStatsProviderClass *ir = RDMA_STATS_PROVIDER_CLASS(klass);
 
 k->realize = pvrdma_realize;
-k->exit = pvrdma_exit;
 k->vendor_id = PCI_VENDOR_ID_VMWARE;
 k->device_id = PCI_DEVICE_ID_VMWARE_PVRDMA;
 k->revision = 0x00;
-- 
2.17.2

[Qemu-devel] [PATCH v4 6/9] hw/rdma: Free all receive buffers when QP is destroyed

[Yuval Shaia]

When QP is destroyed the backend QP is destroyed as well. This ensures
we clean all received buffer we posted to it.
However, a contexts of these buffers are still remain in the device.
Fix it by maintaining a list of buffer's context and free them when QP
is destroyed.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c  | 26 --
 hw/rdma/rdma_backend.h  |  2 +-
 hw/rdma/rdma_backend_defs.h |  2 +-
 hw/rdma/rdma_rm.c   |  2 +-
 hw/rdma/rdma_utils.c| 29 +
 hw/rdma/rdma_utils.h| 11 +++
 6 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index a65f5737e4..d511ca282b 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -39,6 +39,7 @@
 typedef struct BackendCtx {
 void *up_ctx;
 struct ibv_sge sge; /* Used to save MAD recv buffer */
+RdmaBackendQP *backend_qp; /* To maintain recv buffers */
 } BackendCtx;
 
 struct backend_umad {
@@ -73,6 +74,7 @@ static void free_cqe_ctx(gpointer data, gpointer user_data)
 bctx = rdma_rm_get_cqe_ctx(rdma_dev_res, cqe_ctx_id);
 if (bctx) {
 rdma_rm_dealloc_cqe_ctx(rdma_dev_res, cqe_ctx_id);
+atomic_dec(&rdma_dev_res->stats.missing_cqe);
 }
 g_free(bctx);
 }
@@ -85,13 +87,15 @@ static void clean_recv_mads(RdmaBackendDev *backend_dev)
 cqe_ctx_id = rdma_protected_qlist_pop_int64(&backend_dev->
 recv_mads_list);
 if (cqe_ctx_id != -ENOENT) {
+atomic_inc(&backend_dev->rdma_dev_res->stats.missing_cqe);
 free_cqe_ctx(GINT_TO_POINTER(cqe_ctx_id),
  backend_dev->rdma_dev_res);
 }
 } while (cqe_ctx_id != -ENOENT);
 }
 
-static int rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq *ibcq)
+static int rdma_poll_cq(RdmaBackendDev *backend_dev,
+RdmaDeviceResources *rdma_dev_res, struct ibv_cq *ibcq)
 {
 int i, ne, total_ne = 0;
 BackendCtx *bctx;
@@ -113,6 +117,8 @@ static int rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, 
struct ibv_cq *ibcq)
 
 comp_handler(bctx->up_ctx, &wc[i]);
 
+rdma_protected_gslist_remove_int32(&bctx->backend_qp->cqe_ctx_list,
+   wc[i].wr_id);
 rdma_rm_dealloc_cqe_ctx(rdma_dev_res, wc[i].wr_id);
 g_free(bctx);
 }
@@ -175,14 +181,12 @@ static void *comp_handler_thread(void *arg)
 }
 
 backend_dev->rdma_dev_res->stats.poll_cq_from_bk++;
-rdma_poll_cq(backend_dev->rdma_dev_res, ev_cq);
+rdma_poll_cq(backend_dev, backend_dev->rdma_dev_res, ev_cq);
 
 ibv_ack_cq_events(ev_cq, 1);
 }
 }
 
-/* TODO: Post cqe for all remaining buffs that were posted */
-
 backend_dev->comp_thread.is_running = false;
 
 qemu_thread_exit(0);
@@ -311,7 +315,7 @@ void rdma_backend_poll_cq(RdmaDeviceResources 
*rdma_dev_res, RdmaBackendCQ *cq)
 int polled;
 
 rdma_dev_res->stats.poll_cq_from_guest++;
-polled = rdma_poll_cq(rdma_dev_res, cq->ibcq);
+polled = rdma_poll_cq(cq->backend_dev, rdma_dev_res, cq->ibcq);
 if (!polled) {
 rdma_dev_res->stats.poll_cq_from_guest_empty++;
 }
@@ -501,6 +505,7 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
 
 bctx = g_malloc0(sizeof(*bctx));
 bctx->up_ctx = ctx;
+bctx->backend_qp = qp;
 
 rc = rdma_rm_alloc_cqe_ctx(backend_dev->rdma_dev_res, &bctx_id, bctx);
 if (unlikely(rc)) {
@@ -508,6 +513,8 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
 goto err_free_bctx;
 }
 
+rdma_protected_gslist_append_int32(&qp->cqe_ctx_list, bctx_id);
+
 rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge, num_sge,
   &backend_dev->rdma_dev_res->stats.tx_len);
 if (rc) {
@@ -616,6 +623,7 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 
 bctx = g_malloc0(sizeof(*bctx));
 bctx->up_ctx = ctx;
+bctx->backend_qp = qp;
 
 rc = rdma_rm_alloc_cqe_ctx(rdma_dev_res, &bctx_id, bctx);
 if (unlikely(rc)) {
@@ -623,6 +631,8 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
 goto err_free_bctx;
 }
 
+rdma_protected_gslist_append_int32(&qp->cqe_ctx_list, bctx_id);
+
 rc = build_host_sge_array(rdma_dev_res, new_sge, sge, num_sge,
   &backend_dev->rdma_dev_res->stats.rx_bufs_len);
 if (rc) {
@@ -762,6 +772,8 @@ int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t 
qp_type,
 return -EIO;
 }
 
+rdma_protected_gslist_init(&qp->cqe_ctx_list);
+
 qp->ibpd = pd->ibpd;
 
 /* TODO: Query QP to get max_inline_data and save it to be used in send */
@@ -919,11 +931,13 @@ int rdma_backend_query_qp(RdmaBackendQP *qp, struct 
ibv_qp_attr *attr,
 return ib

[Qemu-devel] [PATCH v4 4/9] {hmp, hw/pvrdma}: Expose device internals via monitor interface

[Yuval Shaia]

Allow interrogating device internals through HMP interface.
The exposed indicators can be used for troubleshooting by developers or
sysadmin.
There is no need to expose these attributes to a management system (e.x.
libvirt) because (1) most of them are not "device-management' related
info and (2) there is no guarantee the interface is stable.

Signed-off-by: Yuval Shaia 
---
 hmp-commands-info.hx   | 14 
 hmp.c  | 27 +++
 hmp.h  |  1 +
 hw/rdma/Makefile.objs  |  2 +-
 hw/rdma/rdma_backend.c | 70 +-
 hw/rdma/rdma_hmp.c | 30 
 hw/rdma/rdma_rm.c  | 60 
 hw/rdma/rdma_rm.h  |  1 +
 hw/rdma/rdma_rm_defs.h | 27 ++-
 hw/rdma/vmw/pvrdma.h   |  5 +++
 hw/rdma/vmw/pvrdma_main.c  | 21 
 include/hw/rdma/rdma_hmp.h | 40 ++
 12 files changed, 279 insertions(+), 19 deletions(-)
 create mode 100644 hw/rdma/rdma_hmp.c
 create mode 100644 include/hw/rdma/rdma_hmp.h

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cbee8b944d..c59444c461 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -202,6 +202,20 @@ STEXI
 @item info pic
 @findex info pic
 Show PIC state.
+ETEXI
+
+{
+.name   = "rdma",
+.args_type  = "",
+.params = "",
+.help   = "show RDMA state",
+.cmd= hmp_info_rdma,
+},
+
+STEXI
+@item info rdma
+@findex info rdma
+Show RDMA state.
 ETEXI
 
 {
diff --git a/hmp.c b/hmp.c
index 1e006eeb49..68511b2441 100644
--- a/hmp.c
+++ b/hmp.c
@@ -51,6 +51,7 @@
 #include "qemu/error-report.h"
 #include "exec/ramlist.h"
 #include "hw/intc/intc.h"
+#include "hw/rdma/rdma_hmp.h"
 #include "migration/snapshot.h"
 #include "migration/misc.h"
 
@@ -968,6 +969,32 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict)
hmp_info_pic_foreach, mon);
 }
 
+static int hmp_info_rdma_foreach(Object *obj, void *opaque)
+{
+RdmaStatsProvider *rdma;
+RdmaStatsProviderClass *k;
+Monitor *mon = opaque;
+
+if (object_dynamic_cast(obj, TYPE_RDMA_STATS_PROVIDER)) {
+rdma = RDMA_STATS_PROVIDER(obj);
+k = RDMA_STATS_PROVIDER_GET_CLASS(obj);
+if (k->print_statistics) {
+k->print_statistics(mon, rdma);
+} else {
+monitor_printf(mon, "RDMA statistics not available for %s.\n",
+   object_get_typename(obj));
+}
+}
+
+return 0;
+}
+
+void hmp_info_rdma(Monitor *mon, const QDict *qdict)
+{
+object_child_foreach_recursive(object_get_root(),
+   hmp_info_rdma_foreach, mon);
+}
+
 void hmp_info_pci(Monitor *mon, const QDict *qdict)
 {
 PciInfoList *info_list, *info;
diff --git a/hmp.h b/hmp.h
index 5f1addcca2..666949afc3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict);
 void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_irq(Monitor *mon, const QDict *qdict);
 void hmp_info_pic(Monitor *mon, const QDict *qdict);
+void hmp_info_rdma(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
diff --git a/hw/rdma/Makefile.objs b/hw/rdma/Makefile.objs
index bd36cbf51c..dd59faff51 100644
--- a/hw/rdma/Makefile.objs
+++ b/hw/rdma/Makefile.objs
@@ -1,5 +1,5 @@
 ifeq ($(CONFIG_PVRDMA),y)
-obj-$(CONFIG_PCI) += rdma_utils.o rdma_backend.o rdma_rm.o
+obj-$(CONFIG_PCI) += rdma_utils.o rdma_backend.o rdma_rm.o rdma_hmp.o
 obj-$(CONFIG_PCI) += vmw/pvrdma_dev_ring.o vmw/pvrdma_cmd.o \
  vmw/pvrdma_qp_ops.o vmw/pvrdma_main.o
 endif
diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 9679b842d1..bc2fefcf93 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -64,9 +64,9 @@ static inline void complete_work(enum ibv_wc_status status, 
uint32_t vendor_err,
 comp_handler(ctx, &wc);
 }
 
-static void rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq 
*ibcq)
+static int rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq *ibcq)
 {
-int i, ne;
+int i, ne, total_ne = 0;
 BackendCtx *bctx;
 struct ibv_wc wc[2];
 
@@ -89,12 +89,18 @@ static void rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, 
struct ibv_cq *ibcq)
 rdma_rm_dealloc_cqe_ctx(rdma_dev_res, wc[i].wr_id);
 g_free(bctx);
 }
+total_ne += ne;
 } while (ne > 0);
+atomic_sub(&rdma_dev_res->stats.missing_cqe, total_ne);
 qemu_mutex_unlock(&rdma_dev_res->lock);
 
 if (ne < 0) {
 rdma_error_report("ibv_poll_cq fail, rc=%d, errno=%d", ne, errno);
 }
+
+rdma_dev_res->stats.completions += total_ne;
+
+return total_ne;
 }
 
 static void *comp_handler_thr

[Qemu-devel] [PATCH v4 1/9] hw/rdma: Switch to generic error reporting way

[Yuval Shaia]

Utilize error_report for all pr_err calls and some pr_dbg that are
considered as errors.
For the remaining pr_dbg calls, the important ones were replaced by
trace points while other deleted.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/rdma_backend.c| 336 ++
 hw/rdma/rdma_rm.c | 127 ++---
 hw/rdma/rdma_rm.h |   6 +-
 hw/rdma/rdma_utils.c  |  15 +-
 hw/rdma/rdma_utils.h  |  45 +
 hw/rdma/trace-events  |  32 +++-
 hw/rdma/vmw/pvrdma.h  |   2 +-
 hw/rdma/vmw/pvrdma_cmd.c  | 113 +++-
 hw/rdma/vmw/pvrdma_dev_ring.c |  26 +--
 hw/rdma/vmw/pvrdma_main.c | 132 +
 hw/rdma/vmw/pvrdma_qp_ops.c   |  49 ++---
 hw/rdma/vmw/trace-events  |  16 +-
 12 files changed, 343 insertions(+), 556 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index fd571f21e5..24bac00a20 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -14,7 +14,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qlist.h"
@@ -39,7 +38,6 @@
 
 typedef struct BackendCtx {
 void *up_ctx;
-bool is_tx_req;
 struct ibv_sge sge; /* Used to save MAD recv buffer */
 } BackendCtx;
 
@@ -52,7 +50,7 @@ static void (*comp_handler)(void *ctx, struct ibv_wc *wc);
 
 static void dummy_comp_handler(void *ctx, struct ibv_wc *wc)
 {
-pr_err("No completion handler is registered\n");
+rdma_error_report("No completion handler is registered");
 }
 
 static inline void complete_work(enum ibv_wc_status status, uint32_t 
vendor_err,
@@ -66,29 +64,24 @@ static inline void complete_work(enum ibv_wc_status status, 
uint32_t vendor_err,
 comp_handler(ctx, &wc);
 }
 
-static void poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq *ibcq)
+static void rdma_poll_cq(RdmaDeviceResources *rdma_dev_res, struct ibv_cq 
*ibcq)
 {
 int i, ne;
 BackendCtx *bctx;
 struct ibv_wc wc[2];
 
-pr_dbg("Entering poll_cq loop on cq %p\n", ibcq);
 do {
 ne = ibv_poll_cq(ibcq, ARRAY_SIZE(wc), wc);
 
-pr_dbg("Got %d completion(s) from cq %p\n", ne, ibcq);
+trace_rdma_poll_cq(ne, ibcq);
 
 for (i = 0; i < ne; i++) {
-pr_dbg("wr_id=0x%" PRIx64 "\n", wc[i].wr_id);
-pr_dbg("status=%d\n", wc[i].status);
-
 bctx = rdma_rm_get_cqe_ctx(rdma_dev_res, wc[i].wr_id);
 if (unlikely(!bctx)) {
-pr_dbg("Error: Failed to find ctx for req %" PRId64 "\n",
-   wc[i].wr_id);
+rdma_error_report("No matching ctx for req %"PRId64,
+  wc[i].wr_id);
 continue;
 }
-pr_dbg("Processing %s CQE\n", bctx->is_tx_req ? "send" : "recv");
 
 comp_handler(bctx->up_ctx, &wc[i]);
 
@@ -98,7 +91,7 @@ static void poll_cq(RdmaDeviceResources *rdma_dev_res, struct 
ibv_cq *ibcq)
 } while (ne > 0);
 
 if (ne < 0) {
-pr_dbg("Got error %d from ibv_poll_cq\n", ne);
+rdma_error_report("ibv_poll_cq fail, rc=%d, errno=%d", ne, errno);
 }
 }
 
@@ -115,12 +108,10 @@ static void *comp_handler_thread(void *arg)
 flags = fcntl(backend_dev->channel->fd, F_GETFL);
 rc = fcntl(backend_dev->channel->fd, F_SETFL, flags | O_NONBLOCK);
 if (rc < 0) {
-pr_dbg("Fail to change to non-blocking mode\n");
+rdma_error_report("Failed to change backend channel FD to 
non-blocking");
 return NULL;
 }
 
-pr_dbg("Starting\n");
-
 pfds[0].fd = backend_dev->channel->fd;
 pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
 
@@ -132,27 +123,25 @@ static void *comp_handler_thread(void *arg)
 } while (!rc && backend_dev->comp_thread.run);
 
 if (backend_dev->comp_thread.run) {
-pr_dbg("Waiting for completion on channel %p\n", 
backend_dev->channel);
 rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
-pr_dbg("ibv_get_cq_event=%d\n", rc);
 if (unlikely(rc)) {
-pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
+rdma_error_report("ibv_get_cq_event fail, rc=%d, errno=%d", rc,
+  errno);
 continue;
 }
 
 rc = ibv_req_notify_cq(ev_cq, 0);
 if (unlikely(rc)) {
-pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
+rdma_error_report("ibv_req_notify_cq fail, rc=%d, errno=%d", 
rc,
+  errno);
 }
 
-poll_cq(backend_dev->rdma_dev_res, ev_cq);
+rdma_poll_cq(backend_dev->rdma_dev_res, ev_cq);
 
 ibv_ack_cq_events(ev_cq, 1);
 }
 }
 
-pr_dbg("Going down\n");
-
 /* TODO: Post cqe for all remaining buffs that were posted */
 
 backend_dev->comp_thread.is_running = fa

[Qemu-devel] [PATCH v4 9/9] hw/pvrdma: Unregister from shutdown notifier when device goes down

[Yuval Shaia]

This hook was installed to close the device when VM is going down.
After the device is closed there is no need to be informed on VM
shutdown.

Signed-off-by: Yuval Shaia 
Reviewed-by: Marcel Apfelbaum 
---
 hw/rdma/vmw/pvrdma_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 4e4a43eac4..cb6eefe029 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -303,6 +303,8 @@ static void pvrdma_fini(PCIDevice *pdev)
 {
 PVRDMADev *dev = PVRDMA_DEV(pdev);
 
+notifier_remove(&dev->shutdown_notifier);
+
 pvrdma_qp_ops_fini();
 
 rdma_backend_stop(&dev->backend_dev);
-- 
2.17.2

Re: [Qemu-devel] [PATCH v6 72/73] cpu: add async_run_on_cpu_no_bql

[Emilio G. Cota]

On Fri, Feb 08, 2019 at 14:58:40 +, Alex Bennée wrote:
> 
> Emilio G. Cota  writes:
> 
> > Some async jobs do not need the BQL.
> >
> > Reviewed-by: Richard Henderson 
> > Signed-off-by: Emilio G. Cota 
(snip)
> So we now have a locking/scheduling hierarchy that goes:
> 
>   - run_on_cpu - synchronously wait until target cpu has done the thing
>   - async_run_on_cpu - schedule work on cpu at some point (soon) resources 
> protected by BQL
>   - async_run_on_cpu_no_bql - as above but only protected by cpu_lock

This one doesn't hold any locks when calling the passed function,
though. The CPU lock is just to serialise the queueing/dequeueing.

>   - async_safe_run_on_cpu - as above but locking (probably) not required as 
> everything else asleep
> 
> So the BQL is only really needed to manipulate data that is shared
> across multiple vCPUs like device emulation or other state shared across
> multiple vCPUS. For all "just do it over there" cases we should be able
> to stick to cpu locks.
> 
> It would be nice if we could expand the documentation in
> multi-thread-tcg.txt to cover this in long form for people trying to
> work out the best thing to use.

I think the documentation in the functions is probably enough -- they
point to each other, so figuring out what to use should be pretty easy.

Besides, these functions aren't MTTCG-only, they're QEMU-wide, so
perhaps their documentation should go in a different file?

Since this can be done later, I'll post a v7 with the other
changes you suggested.

> Reviewed-by: Alex Bennée 

Thanks!

Emilio

Re: [Qemu-devel] [PATCH 1/8] target/ppc: introduce single fpr_offset() function

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> Instead of having multiple copies of the offset calculation logic, move it to 
> a
> single fpr_offset() function.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  target/ppc/cpu.h   | 7 ++-
>  target/ppc/translate.c | 4 ++--
>  2 files changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 2/8] target/ppc: introduce single vsrl_offset() function

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> Instead of having multiple copies of the offset calculation logic, move it to 
> a
> single vsrl_offset() function.
> 
> This commit also renames the existing get_vsr()/set_vsr() functions to
> get_vsrl()/set_vsrl() which better describes their purpose.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  target/ppc/cpu.h|  7 ++-
>  target/ppc/translate/vsx-impl.inc.c | 12 ++--
>  2 files changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 5/8] target/ppc: introduce avr_offset() function

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> All TCG vector operations require pointers to the base address of the vector
> rather than separate access to the top and bottom 64-bits. Convert
> the VMX TCG instructions to use a new avr_offset() function instead of
> avr64_offset(), which can itself be written as a simple wrapper onto
> vsr_full_offset().
> 
> After the conversion is complete then avr64_offset() can be removed since its
> functionality is now completely within get_avr64()/set_avr64().
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  target/ppc/cpu.h| 12 +++-
>  target/ppc/translate/vmx-impl.inc.c | 27 +++
>  target/ppc/translate/vsx-impl.inc.c |  5 -
>  3 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 326593e0e7..89651988ab 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2598,6 +2598,11 @@ static inline int vsrl_offset(int i)
>  return offsetof(CPUPPCState, vsr[i].u64[1]);
>  }
>  
> +static inline int vsr_full_offset(int i)
> +{
> +return offsetof(CPUPPCState, vsr[i].u64[0]);
> +}
> +
>  static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
>  {
>  return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
> @@ -2613,9 +2618,14 @@ static inline int avrl_offset(int i)
>  return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
>  }
>  
> +static inline int avr_offset(int i)
> +{
> +return vsr_full_offset(i + 32);
> +}

avr_full_offset?


r~

Re: [Qemu-devel] [PATCH 4/8] target/ppc: introduce avrh_offset() and avrl_offset() functions

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> These will become more useful later, but initially use this as an aid to
> simplify the offset calculation by replacing the HOST_TARGET_BIGENDIAN
> sections with the VsrD macro.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  target/ppc/cpu.h   | 10 ++
>  target/ppc/translate.c | 24 ++--
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d0580c6b6d..326593e0e7 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2603,6 +2603,16 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, 
> int i)
>  return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
>  }
>  
> +static inline int avrh_offset(int i)
> +{
> +return offsetof(CPUPPCState, vsr[32 + i].VsrD(0));
> +}
> +
> +static inline int avrl_offset(int i)
> +{
> +return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
> +}

I really don't see the point of these...

>  static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
>  {
> -#ifdef HOST_WORDS_BIGENDIAN
> -tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
> -  vsr[32 + regno].u64[(high ? 0 : 
> 1)]));
> -#else
> -tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
> -  vsr[32 + regno].u64[(high ? 1 : 
> 0)]));
> -#endif
> +if (high) {
> +tcg_gen_ld_i64(dst, cpu_env, avrh_offset(regno));
> +} else {
> +tcg_gen_ld_i64(dst, cpu_env, avrl_offset(regno));
> +}

When you could just as well write this as

  tcg_gen_ld_i64(dst, cpu_env,
offsetof(CPUPPCState, vsr[32+regno].VsrD(high)));


r~

Re: [Qemu-devel] [PATCH 6/8] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> When VSX support was initially added, the fpr registers were added at
> offset 0 of the VSR register and the vsrl registers were added at offset
> 1. This is in contrast to the VMX registers (the last 32 VSX registers) which
> are stored in host-endian order.
> 
> Switch the fpr/vsrl registers so that the lower 32 VSX registers are now also
> stored in host endian order to match the VMX registers. This ensures that TCG
> vector operations involving mixed VMX and VSX registers will function
> correctly.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  target/ppc/cpu.h  | 4 ++--
>  target/ppc/internal.h | 8 
>  target/ppc/machine.c  | 8 
>  3 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 7/8] target/ppc: introduce vsrh_offset() function

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> -static inline int fpr_offset(int i)
> +static inline int vsrh_offset(int i)

I don't agree with this.  The original is clearer for its uses.


r~

Re: [Qemu-devel] [PATCH 8/8] target/ppc: simplify get_cpu_vsrh() and get_cpu_vsrl() functions

[Richard Henderson]

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
>  static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
>  {
> -if (n < 32) {
> -get_fpr(dst, n);
> -} else {
> -get_avr64(dst, n - 32, true);
> -}
> +tcg_gen_ld_i64(dst, cpu_env, vsrh_offset(n));
>  }
>  
>  static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
>  {
> -if (n < 32) {
> -get_vsrl(dst, n);
> -} else {
> -get_avr64(dst, n - 32, false);
> -}
> +tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
>  }
>  
>  static inline void set_cpu_vsrh(int n, TCGv_i64 src)
>  {
> -if (n < 32) {
> -set_fpr(n, src);
> -} else {
> -set_avr64(n - 32, src, true);
> -}
> +tcg_gen_st_i64(src, cpu_env, vsrh_offset(n));
>  }

I think these ought to have a "high" parameter, like set/get_avr64.


r~

Re: [Qemu-devel] [PATCH] hw/arm/acpi: enable SHPC native hot plug

[Michael S. Tsirkin]

On Fri, Mar 01, 2019 at 10:28:30AM +0800, Heyi Guo wrote:
> After the introduction of generic PCIe root port and PCIe-PCI bridge,
> we will also have SHPC controller on ARM, so just enalbe SHPC native
> hot plug.
> 
> Cc: Shannon Zhao 
> Cc: Peter Maydell 
> Cc: "Michael S. Tsirkin" 
> Cc: Igor Mammedov 
> Signed-off-by: Heyi Guo 
> Signed-off-by: Heyi Guo 

I don't think you need two Signed-off-by lines.
Besides that:

Reviewed-by: Michael S. Tsirkin 

Pls feel free to merge through the ARM tree.

> ---
>  hw/arm/virt-acpi-build.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 04b62c7..7849ec5 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -265,7 +265,12 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
> MemMapEntry *memmap,
>  aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
>  aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
>  aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
> -aml_append(ifctx, aml_store(aml_and(aml_name("CTRL"), aml_int(0x1D), 
> NULL),
> +
> +/*
> + * Allow OS control for all 5 features:
> + * PCIeHotplug SHPCHotplug PME AER PCIeCapability.
> + */
> +aml_append(ifctx, aml_store(aml_and(aml_name("CTRL"), aml_int(0x1F), 
> NULL),
>  aml_name("CTRL")));
>  
>  ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1;
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH] Re-applying Freescale PPC E500 i2c/RTC patch

[David Gibson]

On Sun, Mar 03, 2019 at 12:21:21AM +0300, Andrew Randrianasulu wrote:
> From ad2b4baf8b369c8ef354e56f75ae780413acd989 Mon Sep 17 00:00:00 2001
> From: Amit Singh Tomar 
> Date: Sun, 3 Mar 2019 00:05:04 +0300
> Subject: [PATCH] Re-applying Freescale PPC E500 i2c/RTC patch
> 
> Patch was originally writen by Amit Singh Tomar 
> see http://patchwork.ozlabs.org/patch/431475/
> I only fixed it enough for application on top of current qemu master
> 20b084c4b1401b7f8fbc385649d48c67b6f43d44, and hopefully fixed
> checkpatch errors

So.. that tells me some of the patch's history, but doesn't mention
what the patch actually does and why it's a good thing to have, which
are the most important things to go in the commit message.

> 
> Tested by booting Linux kernel 4.20.12.
> 
> Signed-off-by: Andrew Randrianasulu 
> ---
>  default-configs/ppc-softmmu.mak |   2 +
>  hw/i2c/Makefile.objs|   1 +
>  hw/i2c/mpc_i2c.c| 357 
> 
>  hw/ppc/e500.c   |  54 ++
>  4 files changed, 414 insertions(+)
>  create mode 100644 hw/i2c/mpc_i2c.c
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 52acb7cf39..a560971f0c 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -8,6 +8,8 @@ include usb.mak
>  CONFIG_PPC4XX=y
>  CONFIG_M48T59=y
>  CONFIG_SERIAL=y
> +CONFIG_MPC_I2C=y
> +CONFIG_DS1338=y
>  CONFIG_I8257=y
>  CONFIG_OPENPIC=y
>  CONFIG_PPCE500_PCI=y
> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> index 9205cbee16..3eb584254f 100644
> --- a/hw/i2c/Makefile.objs
> +++ b/hw/i2c/Makefile.objs
> @@ -9,5 +9,6 @@ common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
>  common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
>  common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o
>  common-obj-$(CONFIG_NRF51_SOC) += microbit_i2c.o
> +common-obj-$(CONFIG_MPC_I2C) += mpc_i2c.o
>  obj-$(CONFIG_OMAP) += omap_i2c.o
>  obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> new file mode 100644
> index 00..693ca7ef6b
> --- /dev/null
> +++ b/hw/i2c/mpc_i2c.c
> @@ -0,0 +1,357 @@
> +/*
> + * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Amit Tomar, 
> + *
> + * Description:
> + * This file is derived from IMX I2C controller,
> + * by Jean-Christophe DUBOIS .
> + *
> + * Thanks to Scott Wood and Alexander Graf for their kind help on this.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2 or later,
> + * as published by the Free Software Foundation.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/i2c/i2c.h"
> +#include "qemu/log.h"
> +#include "hw/sysbus.h"
> +
> +/* #define DEBUG_I2C */
> +
> +#ifdef DEBUG_I2C
> +#define DPRINTF(fmt, ...)  \
> +do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__); \
> +} while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define TYPE_MPC_I2C "mpc-i2c"
> +#define MPC_I2C(obj) \
> +OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)
> +
> +#define MPC_I2C_ADR   0x00
> +#define MPC_I2C_FDR   0x04
> +#define MPC_I2C_CR0x08
> +#define MPC_I2C_SR0x0c
> +#define MPC_I2C_DR0x10
> +#define MPC_I2C_DFSRR 0x14
> +
> +#define CCR_MEN  (1 << 7)
> +#define CCR_MIEN (1 << 6)
> +#define CCR_MSTA (1 << 5)
> +#define CCR_MTX  (1 << 4)
> +#define CCR_TXAK (1 << 3)
> +#define CCR_RSTA (1 << 2)
> +#define CCR_BCST (1 << 0)
> +
> +#define CSR_MCF  (1 << 7)
> +#define CSR_MAAS (1 << 6)
> +#define CSR_MBB  (1 << 5)
> +#define CSR_MAL  (1 << 4)
> +#define CSR_SRW  (1 << 2)
> +#define CSR_MIF  (1 << 1)
> +#define CSR_RXAK (1 << 0)
> +
> +#define CADR_MASK 0xFE
> +#define CFDR_MASK 0x3F
> +#define CCR_MASK  0xFC
> +#define CSR_MASK  0xED
> +#define CDR_MASK  0xFF
> +
> +#define CYCLE_RESET 0xFF
> +
> +typedef struct MPCI2CState {
> +SysBusDevice parent_obj;
> +
> +I2CBus *bus;
> +qemu_irq irq;
> +MemoryRegion iomem;
> +
> +uint8_t address;
> +uint8_t adr;
> +uint8_t fdr;
> +uint8_t cr;
> +uint8_t sr;
> +uint8_t dr;
> +uint8_t dfssr;
> +} MPCI2CState;
> +
> +static bool mpc_i2c_is_enabled(MPCI2CState *s)
> +{
> +return s->cr & CCR_MEN;
> +}
> +
> +static bool mpc_i2c_is_master(MPCI2CState *s)
> +{
> +return s->cr & CCR_MSTA;
> +}
> +
> +static bool mpc_i2c_direction_is_tx(MPCI2CState *s)
> +{
> +return s->cr & CCR_MTX;
> +}
> +
> +static bool mpc_i2c_irq_pending(MPCI2CState *s)
> +{
> +return s->sr & CSR_MIF;
> +}
> +
> +static bool mpc_i2c_irq_is_enabled(MPCI2CState *s)
> +{
> +return s->cr & CCR_MIEN;
> +}
> +
> +static void mpc_i2c_reset(DeviceState *dev)
> +{
> +MPCI2CState *i2c = MPC_I2C(dev);
> +

Re: [Qemu-devel] [PATCH] hw/arm/acpi: enable SHPC native hot plug

[Heyi Guo]

On 2019/3/4 7:38, Michael S. Tsirkin wrote:

On Fri, Mar 01, 2019 at 10:28:30AM +0800, Heyi Guo wrote:

After the introduction of generic PCIe root port and PCIe-PCI bridge,
we will also have SHPC controller on ARM, so just enalbe SHPC native
hot plug.

Cc: Shannon Zhao 
Cc: Peter Maydell 
Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 
Signed-off-by: Heyi Guo 
Signed-off-by: Heyi Guo 

I don't think you need two Signed-off-by lines.
Besides that:

Reviewed-by: Michael S. Tsirkin 

Pls feel free to merge through the ARM tree.

Thanks, I'll change that and add your R-b, and send a v2 for some dear 
maintainer to commit it :)

Heyi




---
  hw/arm/virt-acpi-build.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 04b62c7..7849ec5 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -265,7 +265,12 @@ static void acpi_dsdt_add_pci(Aml *scope, const 
MemMapEntry *memmap,
  aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3"));
  aml_append(ifctx, aml_store(aml_name("CDW2"), aml_name("SUPP")));
  aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
-aml_append(ifctx, aml_store(aml_and(aml_name("CTRL"), aml_int(0x1D), NULL),
+
+/*
+ * Allow OS control for all 5 features:
+ * PCIeHotplug SHPCHotplug PME AER PCIeCapability.
+ */
+aml_append(ifctx, aml_store(aml_and(aml_name("CTRL"), aml_int(0x1F), NULL),
  aml_name("CTRL")));
  
  ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1;

--
1.8.3.1

.

Re: [Qemu-devel] [PATCH] Re-applying Freescale PPC E500 i2c/RTC patch

[Andrew Randrianasulu]

В сообщении от Monday 04 March 2019 02:57:28 David Gibson написал(а):
> On Sun, Mar 03, 2019 at 12:21:21AM +0300, Andrew Randrianasulu wrote:
> > From ad2b4baf8b369c8ef354e56f75ae780413acd989 Mon Sep 17 00:00:00 2001
> > From: Amit Singh Tomar 
> > Date: Sun, 3 Mar 2019 00:05:04 +0300
> > Subject: [PATCH] Re-applying Freescale PPC E500 i2c/RTC patch
> > 
> > Patch was originally writen by Amit Singh Tomar 
> > see http://patchwork.ozlabs.org/patch/431475/
> > I only fixed it enough for application on top of current qemu master
> > 20b084c4b1401b7f8fbc385649d48c67b6f43d44, and hopefully fixed
> > checkpatch errors
> 
> So.. that tells me some of the patch's history, but doesn't mention
> what the patch actually does and why it's a good thing to have, which
> are the most important things to go in the commit message.

Then I should add original commit message, i think?

---
This patch adds an emulation model for i2c controller found on most of the FSL 
SoCs.
It also integrates the RTC (ds1338) that sits on the i2c Bus with e500 machine 
model.


I hopefully set up git-send-email this time, so will try to send v2 with fixed 
commit message.


> 
> > 
> > Tested by booting Linux kernel 4.20.12.
> > 
> > Signed-off-by: Andrew Randrianasulu 
> > ---
> >  default-configs/ppc-softmmu.mak |   2 +
> >  hw/i2c/Makefile.objs|   1 +
> >  hw/i2c/mpc_i2c.c| 357 
> > 
> >  hw/ppc/e500.c   |  54 ++
> >  4 files changed, 414 insertions(+)
> >  create mode 100644 hw/i2c/mpc_i2c.c
> > 
> > diff --git a/default-configs/ppc-softmmu.mak 
> > b/default-configs/ppc-softmmu.mak
> > index 52acb7cf39..a560971f0c 100644
> > --- a/default-configs/ppc-softmmu.mak
> > +++ b/default-configs/ppc-softmmu.mak
> > @@ -8,6 +8,8 @@ include usb.mak
> >  CONFIG_PPC4XX=y
> >  CONFIG_M48T59=y
> >  CONFIG_SERIAL=y
> > +CONFIG_MPC_I2C=y
> > +CONFIG_DS1338=y
> >  CONFIG_I8257=y
> >  CONFIG_OPENPIC=y
> >  CONFIG_PPCE500_PCI=y
> > diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> > index 9205cbee16..3eb584254f 100644
> > --- a/hw/i2c/Makefile.objs
> > +++ b/hw/i2c/Makefile.objs
> > @@ -9,5 +9,6 @@ common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
> >  common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
> >  common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o
> >  common-obj-$(CONFIG_NRF51_SOC) += microbit_i2c.o
> > +common-obj-$(CONFIG_MPC_I2C) += mpc_i2c.o
> >  obj-$(CONFIG_OMAP) += omap_i2c.o
> >  obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o
> > diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> > new file mode 100644
> > index 00..693ca7ef6b
> > --- /dev/null
> > +++ b/hw/i2c/mpc_i2c.c
> > @@ -0,0 +1,357 @@
> > +/*
> > + * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.
> > + *
> > + * Author: Amit Tomar, 
> > + *
> > + * Description:
> > + * This file is derived from IMX I2C controller,
> > + * by Jean-Christophe DUBOIS .
> > + *
> > + * Thanks to Scott Wood and Alexander Graf for their kind help on this.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2 or 
> > later,
> > + * as published by the Free Software Foundation.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; if not, see 
> > .
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "qemu/log.h"
> > +#include "hw/sysbus.h"
> > +
> > +/* #define DEBUG_I2C */
> > +
> > +#ifdef DEBUG_I2C
> > +#define DPRINTF(fmt, ...)  \
> > +do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__); \
> > +} while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do {} while (0)
> > +#endif
> > +
> > +#define TYPE_MPC_I2C "mpc-i2c"
> > +#define MPC_I2C(obj) \
> > +OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)
> > +
> > +#define MPC_I2C_ADR   0x00
> > +#define MPC_I2C_FDR   0x04
> > +#define MPC_I2C_CR0x08
> > +#define MPC_I2C_SR0x0c
> > +#define MPC_I2C_DR0x10
> > +#define MPC_I2C_DFSRR 0x14
> > +
> > +#define CCR_MEN  (1 << 7)
> > +#define CCR_MIEN (1 << 6)
> > +#define CCR_MSTA (1 << 5)
> > +#define CCR_MTX  (1 << 4)
> > +#define CCR_TXAK (1 << 3)
> > +#define CCR_RSTA (1 << 2)
> > +#define CCR_BCST (1 << 0)
> > +
> > +#define CSR_MCF  (1 << 7)
> > +#define CSR_MAAS (1 << 6)
> > +#define CSR_MBB  (1 << 5)
> > +#define CSR_MAL  (1 << 4)
> > +#define CSR_SRW  (1 << 2)
> > +#define CSR_MIF  (1 << 1)
> > +#define CSR_RXAK (1 << 0)
> > +
> > +#define CADR_MASK 0xFE
> > +#define CFDR_MASK 0x3F
> > +#define CCR_MASK  0xFC
> > +#define CSR_MASK  0xED
> > +#define CDR_MASK  0xFF
> > +
> > +#define CYCLE_RESET 0xFF
> > +
> > +typedef struct MPCI2CState {
> > +SysBusDevice parent_obj;
> > +
> > +I2CBus *bus;
> > +qemu_irq irq;
> > +MemoryRegion iomem;
> > +
> > +uint8_t address;
> > +uint8_t 

Re: [Qemu-devel] [QEMU-PPC] [PATCH 2/2] target/ppc/spapr: Add SPAPR_CAP_CCF_ASSIST

[David Gibson]

On Fri, Mar 01, 2019 at 03:26:45PM +1100, Suraj Jitindar Singh wrote:
> On Fri, 2019-03-01 at 14:19 +1100, Suraj Jitindar Singh wrote:
> > Introduce a new spapr_cap SPAPR_CAP_CCF_ASSIST to be used to indicate
> > the requirement for a hw-assisted version of the count cache flush
> > workaround.
> > 
> > The count cache flush workaround is a software workaround which can
> > be
> > used to flush the count cache on context switch. Some revisions of
> > hardware may have a hardware accelerated flush, in which case the
> > software flush can be shortened. This cap is used to set the
> > availability of such hardware acceleration for the count cache flush
> > routine.
> > 
> > The availability of such hardware acceleration is indicated by the
> > H_CPU_CHAR_BCCTR_FLUSH_ASSIST flag being set in the characteristics
> > returned from the KVM_PPC_GET_CPU_CHAR ioctl.
> > 
> > Signed-off-by: Suraj Jitindar Singh 
> > ---
> >  hw/ppc/spapr.c |  2 ++
> >  hw/ppc/spapr_caps.c| 25 +
> >  hw/ppc/spapr_hcall.c   |  3 +++
> >  include/hw/ppc/spapr.h |  5 -
> >  target/ppc/kvm.c   | 14 ++
> >  target/ppc/kvm_ppc.h   |  6 ++
> >  6 files changed, 54 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 1df324379f..708e18dcdf 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2086,6 +2086,7 @@ static const VMStateDescription vmstate_spapr =
> > {
> >  &vmstate_spapr_cap_nested_kvm_hv,
> >  &vmstate_spapr_dtb,
> >  &vmstate_spapr_cap_large_decr,
> > +&vmstate_spapr_cap_ccf_assist,
> >  NULL
> >  }
> >  };
> > @@ -4319,6 +4320,7 @@ static void
> > spapr_machine_class_init(ObjectClass *oc, void *data)
> >  smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB
> > */
> >  smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> >  smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] =
> > SPAPR_CAP_ON;
> > +smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> >  spapr_caps_add_properties(smc, &error_abort);
> >  smc->irq = &spapr_irq_xics;
> >  smc->dr_phb_enabled = true;
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 74a48a423a..f03f2f64e7 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -436,6 +436,21 @@ static void
> > cap_large_decr_cpu_apply(sPAPRMachineState *spapr,
> >  ppc_store_lpcr(cpu, lpcr);
> >  }
> >  
> > +static void cap_ccf_assist_apply(sPAPRMachineState *spapr, uint8_t
> > val,
> > + Error **errp)
> > +{
> > +uint8_t kvm_val = kvmppc_get_cap_count_cache_flush_assist();
> > +
> > +if (tcg_enabled() && val) {
> > +/* TODO - for now only allow broken for TCG */
> > +error_setg(errp,
> > +"Requested count cache flush assist capability level not supported
> > by tcg, try cap-ccf-assist=off");
> > +} else if (kvm_enabled() && (val > kvm_val)) {
> > +error_setg(errp,
> > +"Requested count cache flush assist capability level not supported
> > by kvm, try cap-ccf-assist=off");
> > +}
> 
> Actually, this should probably be non-fatal if the count cache flush
> routine isn't enabled

Since the new cap values aren't enabled by default, I've applied
anyway.  You can make this error non-fatal in a followup.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/2] spapr: Simulate CAS for qtest

[David Gibson]

On Fri, Mar 01, 2019 at 04:33:38PM -0600, Michael Roth wrote:
> Quoting Greg Kurz (2019-03-01 13:32:37)
> > The RTAS event hotplug code for machine types 2.8 and newer depends on
> > the CAS negotiated ov5 in order to work properly. However, there's no
> > CAS when running under qtest. There has been a tentative to trick the
> > code by faking the OV5_HP_EVT bit, but it turned out to break other
> > assumptions in the code and the change got reverted.
> > 
> > Go for a more general approach and simulate a CAS when running under
> > qtest. For simplicity, this pseudo CAS simple simulates the case where
> > the guest supports the same features as the machine. It is done at
> > reset time, just before we reset the DRCs, which could potentially
> > exercise the unplug code.
> > 
> > This allows to test unplug on spapr with both older and newer machine
> > types.
> > 
> > Suggested-by: Michael Roth 
> > Signed-off-by: Greg Kurz 
> 
> Tested-by: Michael Roth 
> Reviewed-by: Michael Roth 
> 
> Thanks for sending this!
> 
> Just now realizing we should probably apply the revert after this patch
> however, since the commit we're reverting fixes a `make check` test that
> is run by default, whereas this patch fixes one that only gets run if we
> run the tests with -m=slow specified.
> 
> Maybe David can do that on his end?

Applied in reverse order as suggested to ppc-for-4.0.  Thanks.

> 
> > ---
> >  hw/ppc/spapr.c |   11 +++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b6a571b6f184..6da64ef7ee2b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -29,6 +29,7 @@
> >  #include "qapi/visitor.h"
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/numa.h"
> > +#include "sysemu/qtest.h"
> >  #include "hw/hw.h"
> >  #include "qemu/log.h"
> >  #include "hw/fw-path-provider.h"
> > @@ -1711,6 +1712,16 @@ static void spapr_machine_reset(void)
> >   */
> >  spapr_irq_reset(spapr, &error_fatal);
> > 
> > +/*
> > + * There is no CAS under qtest. Simulate one to please the code that
> > + * depends on spapr->ov5_cas. This is especially needed to test device
> > + * unplug, so we do that before resetting the DRCs.
> > + */
> > +if (qtest_enabled()) {
> > +spapr_ovec_cleanup(spapr->ov5_cas);
> > +spapr->ov5_cas = spapr_ovec_clone(spapr->ov5);
> > +}
> > +
> >  /* DRC reset may cause a device to be unplugged. This will cause 
> > troubles
> >   * if this device is used by another device (eg, a running vhost 
> > backend
> >   * will crash QEMU if the DIMM holding the vring goes away). To avoid 
> > such
> > 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [QEMU-PPC] [PATCH v3 1/4] target/ppc/spapr: Add SPAPR_CAP_LARGE_DECREMENTER

[David Gibson]

On Fri, Mar 01, 2019 at 01:43:14PM +1100, Suraj Jitindar Singh wrote:
> Add spapr_cap SPAPR_CAP_LARGE_DECREMENTER to be used to control the
> availability of the large decrementer for a guest.
> 
> Signed-off-by: Suraj Jitindar Singh 

Series applied to ppc-for-4.0, thanks.

> ---
>  hw/ppc/spapr.c |  2 ++
>  hw/ppc/spapr_caps.c| 17 +
>  include/hw/ppc/spapr.h |  5 -
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6f9208476a..d068982a5e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2077,6 +2077,7 @@ static const VMStateDescription vmstate_spapr = {
>  &vmstate_spapr_irq_map,
>  &vmstate_spapr_cap_nested_kvm_hv,
>  &vmstate_spapr_dtb,
> +&vmstate_spapr_cap_large_decr,
>  NULL
>  }
>  };
> @@ -4309,6 +4310,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>  smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>  smc->default_caps.caps[SPAPR_CAP_HPT_MAXPAGESIZE] = 16; /* 64kiB */
>  smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> +smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_OFF;
>  spapr_caps_add_properties(smc, &error_abort);
>  smc->irq = &spapr_irq_xics;
>  smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 64f98ae68d..3f90f5823e 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -390,6 +390,13 @@ static void cap_nested_kvm_hv_apply(sPAPRMachineState 
> *spapr,
>  }
>  }
>  
> +static void cap_large_decr_apply(sPAPRMachineState *spapr,
> + uint8_t val, Error **errp)
> +{
> +if (val)
> +error_setg(errp, "No large decrementer support, try 
> cap-large-decr=off");
> +}
> +
>  sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>  [SPAPR_CAP_HTM] = {
>  .name = "htm",
> @@ -468,6 +475,15 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>  .type = "bool",
>  .apply = cap_nested_kvm_hv_apply,
>  },
> +[SPAPR_CAP_LARGE_DECREMENTER] = {
> +.name = "large-decr",
> +.description = "Allow Large Decrementer",
> +.index = SPAPR_CAP_LARGE_DECREMENTER,
> +.get = spapr_cap_get_bool,
> +.set = spapr_cap_set_bool,
> +.type = "bool",
> +.apply = cap_large_decr_apply,
> +},
>  };
>  
>  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> @@ -596,6 +612,7 @@ SPAPR_CAP_MIG_STATE(cfpc, SPAPR_CAP_CFPC);
>  SPAPR_CAP_MIG_STATE(sbbc, SPAPR_CAP_SBBC);
>  SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
>  SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
> +SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  
>  void spapr_caps_init(sPAPRMachineState *spapr)
>  {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 86b0488d29..3cd47fb6e8 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -74,8 +74,10 @@ typedef enum {
>  #define SPAPR_CAP_HPT_MAXPAGESIZE   0x06
>  /* Nested KVM-HV */
>  #define SPAPR_CAP_NESTED_KVM_HV 0x07
> +/* Large Decrementer */
> +#define SPAPR_CAP_LARGE_DECREMENTER 0x08
>  /* Num Caps */
> -#define SPAPR_CAP_NUM   (SPAPR_CAP_NESTED_KVM_HV + 1)
> +#define SPAPR_CAP_NUM   (SPAPR_CAP_LARGE_DECREMENTER + 1)
>  
>  /*
>   * Capability Values
> @@ -843,6 +845,7 @@ extern const VMStateDescription vmstate_spapr_cap_cfpc;
>  extern const VMStateDescription vmstate_spapr_cap_sbbc;
>  extern const VMStateDescription vmstate_spapr_cap_ibs;
>  extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
> +extern const VMStateDescription vmstate_spapr_cap_large_decr;
>  
>  static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int cap)
>  {

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v2] Re-applying Freescale PPC E500 i2c/RTC patch

[Andrew Randrianasulu]

From: Amit Singh Tomar 

Original commit message:
This patch adds an emulation model for i2c controller found on most of the FSL 
SoCs.
It also integrates the RTC (ds1338) that sits on the i2c Bus with e500 machine 
model.

Patch was originally written by Amit Singh Tomar 
see http://patchwork.ozlabs.org/patch/431475/
I only fixed it enough for application on top of current qemu master
20b084c4b1401b7f8fbc385649d48c67b6f43d44, and hopefully fixed checkpatch errors

Tested by booting Linux kernel 4.20.12. Now e500 machine doesn't need 
network time protocol daemon because it will have working RTC 
(before all timestamps on files were from 2016)

---

v1->v2: Expanded and fixed commit message


Signed-off-by: Andrew Randrianasulu 
---
 default-configs/ppc-softmmu.mak |   2 +
 hw/i2c/Makefile.objs|   1 +
 hw/i2c/mpc_i2c.c| 357 
 hw/ppc/e500.c   |  54 ++
 4 files changed, 414 insertions(+)
 create mode 100644 hw/i2c/mpc_i2c.c

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 52acb7cf39..a560971f0c 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -8,6 +8,8 @@ include usb.mak
 CONFIG_PPC4XX=y
 CONFIG_M48T59=y
 CONFIG_SERIAL=y
+CONFIG_MPC_I2C=y
+CONFIG_DS1338=y
 CONFIG_I8257=y
 CONFIG_OPENPIC=y
 CONFIG_PPCE500_PCI=y
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 9205cbee16..3eb584254f 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -9,5 +9,6 @@ common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
 common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
 common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o
 common-obj-$(CONFIG_NRF51_SOC) += microbit_i2c.o
+common-obj-$(CONFIG_MPC_I2C) += mpc_i2c.o
 obj-$(CONFIG_OMAP) += omap_i2c.o
 obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o
diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
new file mode 100644
index 00..693ca7ef6b
--- /dev/null
+++ b/hw/i2c/mpc_i2c.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * Author: Amit Tomar, 
+ *
+ * Description:
+ * This file is derived from IMX I2C controller,
+ * by Jean-Christophe DUBOIS .
+ *
+ * Thanks to Scott Wood and Alexander Graf for their kind help on this.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2 or later,
+ * as published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "qemu/log.h"
+#include "hw/sysbus.h"
+
+/* #define DEBUG_I2C */
+
+#ifdef DEBUG_I2C
+#define DPRINTF(fmt, ...)  \
+do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__); \
+} while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define TYPE_MPC_I2C "mpc-i2c"
+#define MPC_I2C(obj) \
+OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)
+
+#define MPC_I2C_ADR   0x00
+#define MPC_I2C_FDR   0x04
+#define MPC_I2C_CR0x08
+#define MPC_I2C_SR0x0c
+#define MPC_I2C_DR0x10
+#define MPC_I2C_DFSRR 0x14
+
+#define CCR_MEN  (1 << 7)
+#define CCR_MIEN (1 << 6)
+#define CCR_MSTA (1 << 5)
+#define CCR_MTX  (1 << 4)
+#define CCR_TXAK (1 << 3)
+#define CCR_RSTA (1 << 2)
+#define CCR_BCST (1 << 0)
+
+#define CSR_MCF  (1 << 7)
+#define CSR_MAAS (1 << 6)
+#define CSR_MBB  (1 << 5)
+#define CSR_MAL  (1 << 4)
+#define CSR_SRW  (1 << 2)
+#define CSR_MIF  (1 << 1)
+#define CSR_RXAK (1 << 0)
+
+#define CADR_MASK 0xFE
+#define CFDR_MASK 0x3F
+#define CCR_MASK  0xFC
+#define CSR_MASK  0xED
+#define CDR_MASK  0xFF
+
+#define CYCLE_RESET 0xFF
+
+typedef struct MPCI2CState {
+SysBusDevice parent_obj;
+
+I2CBus *bus;
+qemu_irq irq;
+MemoryRegion iomem;
+
+uint8_t address;
+uint8_t adr;
+uint8_t fdr;
+uint8_t cr;
+uint8_t sr;
+uint8_t dr;
+uint8_t dfssr;
+} MPCI2CState;
+
+static bool mpc_i2c_is_enabled(MPCI2CState *s)
+{
+return s->cr & CCR_MEN;
+}
+
+static bool mpc_i2c_is_master(MPCI2CState *s)
+{
+return s->cr & CCR_MSTA;
+}
+
+static bool mpc_i2c_direction_is_tx(MPCI2CState *s)
+{
+return s->cr & CCR_MTX;
+}
+
+static bool mpc_i2c_irq_pending(MPCI2CState *s)
+{
+return s->sr & CSR_MIF;
+}
+
+static bool mpc_i2c_irq_is_enabled(MPCI2CState *s)
+{
+return s->cr & CCR_MIEN;
+}
+
+static void mpc_i2c_reset(DeviceState *dev)
+{
+MPCI2CState *i2c = MPC_I2C(dev);
+
+i2c->address = 0xFF;
+i2c->adr = 0x00;
+i2c->fdr = 0x00;
+i2c->cr =  0x00;
+i2c->sr =  0x81;
+i2c->dr =  0x00;
+}
+
+static void mpc_i2c_irq(MPCI2CState *s)
+{
+bool irq_active = false;
+
+if (mpc_i2c_is_enabled(s) && mpc_i2c_irq_is_enabled(s)
+  && mpc_i2c_irq_pending(s)) {
+irq_active = true;
+}
+
+if (ir

Re: [Qemu-devel] [PATCH v2] Re-applying Freescale PPC E500 i2c/RTC patch

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190304015401.14280-1-randrianas...@gmail.com/



Hi,

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

Type: series
Message-id: 20190304015401.14280-1-randrianas...@gmail.com
Subject: [Qemu-devel] [PATCH v2] Re-applying Freescale PPC E500 i2c/RTC patch

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   
patchew/20190304015401.14280-1-randrianas...@gmail.com -> 
patchew/20190304015401.14280-1-randrianas...@gmail.com
Switched to a new branch 'test'
165b7e5808 Re-applying Freescale PPC E500 i2c/RTC patch

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 462 lines checked

Commit 165b7e5808b2 (Re-applying Freescale PPC E500 i2c/RTC patch) has style 
problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190304015401.14280-1-randrianas...@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu]

On Fri, Mar 01, 2019 at 04:25:17PM +, Stefan Hajnoczi wrote:
> On Thu, Feb 28, 2019 at 01:58:56PM +0800, Peter Xu wrote:
> > On Wed, Feb 27, 2019 at 01:38:38PM +, Stefan Hajnoczi wrote:
> > > On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > > > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > > > Hi
> > > > > 
> > > > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu  wrote:
> > > > > >
> > > > > > We were pushing the context until right before running the 
> > > > > > gmainloop.
> > > > > > Now since we have everything unconditionally, we can move this
> > > > > > earlier.
> > > > > >
> > > > > > One benefit is that now it's done even before init_done_sem, so as
> > > > > > long as the iothread user calls iothread_create() and completes, we
> > > > > > know that the thread stack is ready.
> > > > > >
> > > > > 
> > > > > This will change the default context in the iothread, for code running
> > > > > there. This may not be a good idea. Until now, only sources dispatched
> > > > > from iothread_get_g_main_context() would have default context
> > > > > associated to it.
> > > > > 
> > > > > I don't know if the current behaviour is intentional, but it has some
> > > > > logic. With this change, you may create hidden races, by changing the
> > > > > default context of sources to the iothread.
> > > > 
> > > > Yes I agree that the behavior will be changed in this patch that even
> > > > if the iothread user does not use the gcontext they'll also have the
> > > > context set.  I would think it should be ok because IMHO events hooked
> > > > onto the aio context should not depend on the gcontext, but indeed I'd
> > > > like to get some confirmation from others, especially the block layer.
> > > 
> > > I don't understand why Patch 4 is desirable.  The comment about
> > > init_done_sem isn't clear to me but I also wondered the same thing as
> > > Marc-André.
> > > 
> > > Can you explain why we should apply this patch?
> > 
> > Hi, Stefan,
> > 
> > The patch 4 itself does not help much for current QEMU, but it should
> > be required to replace the patch that Marc-Andre proposed below:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]
> > 
> > And IMHO patch 4 along with this whole series should be a cleaner
> > approach comparing to the one proposed in [1].  Here if my
> > understanding is correct the problem is that
> > g_main_context_push_thread_default() is really designed to be called
> > at the very beginning of a thread creation but not dynamically called
> > during the execution of a thread (prove is that it even does not have
> > any error to return when failed to acquire the context so the caller
> > will never know if it failed! see [2] below), in that sense this patch
> > 4 can be seen as a tiny cleanup too.
> > 
> > g_main_context_push_thread_default (GMainContext *context)
> > {
> >   GQueue *stack;
> >   gboolean acquired_context;
> > 
> >   acquired_context = g_main_context_acquire (context);
> >   g_return_if_fail (acquired_context);  <- [2]
> > 
> >   ...
> > }
> 
> I see.  This explains why you want to call it early.  If you're worried
> about that then there should also be a comment warning people that this
> must happen first before anything implicitly uses the thread's
> GMainContext.

Sure, I can add a comment above g_main_context_push_thread_default()
to emphasize why it's preferred at the entry.

> 
> What about Marc-André's concern about the change in behavior?  Now this
> thread is associated with the GMainContext that isn't processed at in
> aio_poll().  Previously the default main context would be used.

IIUC Paolo has answered this question (Message-ID:
<0faeceb2-68fa-59b0-48c3-b8e907b2a...@redhat.com>) - if the block
layer (or say, the explicit aio_poll in the iothread_run) does not use
the GMainContext at all then it should affect nothing, and with that
there should have no real functional change.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v2 31/38] target/xtensa: fetch code with translator_ld

[Max Filippov]

On Mon, Feb 25, 2019 at 6:55 AM Alex Bennée  wrote:
> Emilio G. Cota  writes:
> > Signed-off-by: Emilio G. Cota 
> > ---
> >  target/xtensa/translate.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> > index 46e1338448..c140742562 100644
> > --- a/target/xtensa/translate.c
> > +++ b/target/xtensa/translate.c
> > @@ -882,7 +882,7 @@ static inline unsigned xtensa_op0_insn_len(DisasContext 
> > *dc, uint8_t op0)
> >  static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
> >  {
> >  xtensa_isa isa = dc->config->isa;
> > -unsigned char b[MAX_INSN_LENGTH] = {cpu_ldub_code(env, dc->pc)};
> > +unsigned char b[MAX_INSN_LENGTH] = {translator_ldub(env, dc->pc)};
> >  unsigned len = xtensa_op0_insn_len(dc, b[0]);
> >  xtensa_format fmt;
> >  int slot, slots;
> > @@ -914,7 +914,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, 
> > DisasContext *dc)
> >dc->pc);
> >  }
> >  for (i = 1; i < len; ++i) {
> > -b[i] = cpu_ldub_code(env, dc->pc + i);
> > +b[i] = translator_ldub(env, dc->pc + i);
> >  }
> >  xtensa_insnbuf_from_chars(isa, dc->insnbuf, b, len);
> >  fmt = xtensa_format_decode(isa, dc->insnbuf);
>
> There is also:
>
>   static inline unsigned xtensa_insn_len(CPUXtensaState *env, DisasContext 
> *dc)
>   {
>   uint8_t b0 = cpu_ldub_code(env, dc->pc);
>   return xtensa_op0_insn_len(dc, b0);
>   }
>
> Or is this usage a re-read of something we've already got?

FWIW xtensa_insn_len is used to look into the instruction that
follows the one that has just been translated, but it may only
touch the same page as the last translated instruction.

-- 
Thanks.
-- Max

[Qemu-devel] [PATCH] x86: define a new MSR based feature word -- FEAT_CORE_CAPABILITY

[Xiaoyao Li]

MSR IA32_CORE_CAPABILITY is a feature-enumerating MSR, which
enumerates the capabilitiy of enabling detection of split locks (bit 5
of MSR_TEST_CTL).

MSR IA32_CORE_CAPABILITY can be enumerated by CPUID.0X7.0:EDX[30].

Related kernel patches can be found here:
https://lkml.org/lkml/2019/3/1/749
Patches 15-17 of kvm are exposing this feature to guest.

If host has split lock detection feature, we can expose it to guest by
using '-cpu host' with this patch and kernel's patches.

Signed-off-by: Xiaoyao Li 
---
 target/i386/cpu.c | 22 +-
 target/i386/cpu.h |  3 +++
 target/i386/kvm.c |  9 +
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d3aa6a815b..32df1d358a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1081,7 +1081,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = 
{
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, "spec-ctrl", "stibp",
-NULL, "arch-capabilities", NULL, "ssbd",
+NULL, "arch-capabilities", "core-capability", "ssbd",
 },
 .cpuid = {
 .eax = 7,
@@ -1200,6 +1200,26 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] 
= {
 }
 },
 },
+[FEAT_CORE_CAPABILITY] = {
+.type = MSR_FEATURE_WORD,
+.feat_names = {
+NULL, NULL, NULL, NULL,
+NULL, "split-lock-detect", NULL, NULL,
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+},
+.msr = {
+.index = MSR_IA32_CORE_CAPABILITY,
+.cpuid_dep = {
+FEAT_7_0_EDX,
+CPUID_7_0_EDX_CORE_CAPABILITY
+},
+},
+},
 };
 
 typedef struct X86RegisterInfo32 {
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 95112b9118..6eb89ac735 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -355,6 +355,7 @@ typedef enum X86Seg {
 #define MSR_IA32_SPEC_CTRL  0x48
 #define MSR_VIRT_SSBD   0xc001011f
 #define MSR_IA32_PRED_CMD   0x49
+#define MSR_IA32_CORE_CAPABILITY0xcf
 #define MSR_IA32_ARCH_CAPABILITIES  0x10a
 #define MSR_IA32_TSCDEADLINE0x6e0
 
@@ -505,6 +506,7 @@ typedef enum FeatureWord {
 FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
 FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
 FEAT_ARCH_CAPABILITIES,
+FEAT_CORE_CAPABILITY,
 FEATURE_WORDS,
 } FeatureWord;
 
@@ -696,6 +698,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation 
Single Precision */
 #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */
 #define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities*/
+#define CPUID_7_0_EDX_CORE_CAPABILITY (1U << 30)  /*Core Capabilities*/
 #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass 
Disable */
 
 #define CPUID_8000_0008_EBX_WBNOINVD  (1U << 9)  /* Write back and
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index beae1b99da..8aafd1db77 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -96,6 +96,7 @@ static bool has_msr_spec_ctrl;
 static bool has_msr_virt_ssbd;
 static bool has_msr_smi_count;
 static bool has_msr_arch_capabs;
+static bool has_msr_core_capabs;
 
 static uint32_t has_architectural_pmu_version;
 static uint32_t num_architectural_pmu_gp_counters;
@@ -1507,6 +1508,9 @@ static int kvm_get_supported_msrs(KVMState *s)
 case MSR_IA32_ARCH_CAPABILITIES:
 has_msr_arch_capabs = true;
 break;
+case MSR_IA32_CORE_CAPABILITY:
+has_msr_core_capabs = true;
+break;
 }
 }
 }
@@ -2033,6 +2037,11 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
   env->features[FEAT_ARCH_CAPABILITIES]);
 }
 
+if (has_msr_core_capabs) {
+kvm_msr_entry_add(cpu, MSR_IA32_CORE_CAPABILITY,
+  env->features[FEAT_CORE_CAPABILITY]);
+}
+
 /*
  * The following MSRs have side effects on the guest or are too heavy
  * for normal writeback. Limit them to reset or full state updates.
-- 
2.19.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] Re-applying Freescale PPC E500 i2c/RTC patch

[BALATON Zoltan]

On Mon, 4 Mar 2019, Andrew Randrianasulu wrote:

From: Amit Singh Tomar 

Original commit message:
This patch adds an emulation model for i2c controller found on most of the FSL 
SoCs.
It also integrates the RTC (ds1338) that sits on the i2c Bus with e500 machine 
model.

Patch was originally written by Amit Singh Tomar 
see http://patchwork.ozlabs.org/patch/431475/
I only fixed it enough for application on top of current qemu master
20b084c4b1401b7f8fbc385649d48c67b6f43d44, and hopefully fixed checkpatch errors

Tested by booting Linux kernel 4.20.12. Now e500 machine doesn't need
network time protocol daemon because it will have working RTC
(before all timestamps on files were from 2016)

---

v1->v2: Expanded and fixed commit message


Signed-off-by: Andrew Randrianasulu 
---


Almost... Patch now applies but subject and commit message are not yet 
right. Look at existing commit messages for examples how it should look 
(e.g. git log hw/ppc/e500.c). The email subject will become commit title, 
this should start with something showing which part you change like e500:. 
Then one line summary of what the patch is doing. You can probably keep 
original title, no need to say re-applying or things like that there. You 
can explain this in patch body. The text up to the first --- will be the 
body of the commit message so you should describe in more detail what the 
patch does here. Also this should include all Signed-off-by and other 
tags at the end before the ---.


Everything after --- are additional comments that won't be included in the 
commit message so you can put version history or any other remarks there 
that should not be kept after applying the patch.


This patch is missing Signed-off-by of the original author and has yours 
below --- that's why checkpatch complains. You should keep the the 
original Signed-off-by even if you add From: of the original author. I 
think you may not include From: since you're not forwarding a patch 
unchanged but this is now your patch based on the original since you've 
changed it so it can have your From: address from email header and 
Signed-off-by of both original author and yours to show where it came from 
originally. You can also mention this in commit message to make it clear.


Or you can keep From of original author and explain in commit message what 
you've changed but it still needs both Signed-off-by lines even then.


Hopefully this makes sense. This should already be explained in the 
SubmitAPatch wiki page but that can be complicated at first.


Regards,
BALATON Zoltan


default-configs/ppc-softmmu.mak |   2 +
hw/i2c/Makefile.objs|   1 +
hw/i2c/mpc_i2c.c| 357 
hw/ppc/e500.c   |  54 ++
4 files changed, 414 insertions(+)
create mode 100644 hw/i2c/mpc_i2c.c

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 52acb7cf39..a560971f0c 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -8,6 +8,8 @@ include usb.mak
CONFIG_PPC4XX=y
CONFIG_M48T59=y
CONFIG_SERIAL=y
+CONFIG_MPC_I2C=y
+CONFIG_DS1338=y
CONFIG_I8257=y
CONFIG_OPENPIC=y
CONFIG_PPCE500_PCI=y
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 9205cbee16..3eb584254f 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -9,5 +9,6 @@ common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o
common-obj-$(CONFIG_NRF51_SOC) += microbit_i2c.o
+common-obj-$(CONFIG_MPC_I2C) += mpc_i2c.o
obj-$(CONFIG_OMAP) += omap_i2c.o
obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o
diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
new file mode 100644
index 00..693ca7ef6b
--- /dev/null
+++ b/hw/i2c/mpc_i2c.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * Author: Amit Tomar, 
+ *
+ * Description:
+ * This file is derived from IMX I2C controller,
+ * by Jean-Christophe DUBOIS .
+ *
+ * Thanks to Scott Wood and Alexander Graf for their kind help on this.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2 or later,
+ * as published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "qemu/log.h"
+#include "hw/sysbus.h"
+
+/* #define DEBUG_I2C */
+
+#ifdef DEBUG_I2C
+#define DPRINTF(fmt, ...)  \
+do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__); \
+} while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define TYPE_MPC_I2C "mpc-i2c"
+#define MPC_I2C(obj) \
+OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)
+
+#define MPC_I2C_ADR   0x00
+#define MPC_I2C_FDR   0x04
+#define MPC_

Re: [Qemu-devel] [PATCH 06/10] r2d: Flash memory creation is confused about size, mark FIXME

[Magnus Damm]

Hi guys,

On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster  wrote:
>
> Philippe Mathieu-Daudé  writes:
>
> > On 2/19/19 4:45 PM, Markus Armbruster wrote:
> >> Peter Maydell  writes:
> >>
> >>> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster  wrote:
> 
>  pflash_cfi02_register() takes a size in bytes, a block size in bytes
>  and a number of blocks.  r2d_init() passes FLASH_SIZE, 16 * KiB,
>  FLASH_SIZE >> 16.  Does not compute: size doesn't match block size *
>  number of blocks.  The latter happens to win.  I tried to find
>  documentation on the physcial hardware, no luck.
> >
> > "physical"
>
> Thanks, will fix.
>
> 
>  For now, adjust the byte size passed to match the actual size created,
>  and add a FIXME comment.
> >>>
> >>> I'm pretty sure that FLASH_SIZE here is supposed to be a
> >>> byte count of the size of the pflash. That matches what
> >>> Linux has in arch/sh/boards/mach-r2d/setup.c where it
> >>> sets up the flash_resource struct.
> >>
> >> Okay, that's some evidence for size 0x0200 (32MiB).
> >>
> >> However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB,
> >> since at least commit 368a354f02b (v1.3.0), possibly since forever.
> >>
> >>> The r2dplus board is also I think known as RTS7751R2D. That
> >>> takes us to https://elinux.org/RTS7751R2D_Handling_Manual
> >>> (sadly the link to the "hardware manual" is broken).
> >>
> >> Quote section Flash ROM Mapping:
> >>
> >> Currently, MTD device mapping on Flash ROM is set as below.
> >> 0x-0x0002"bootloader"
> >> 0x0002-0x0032"mtdblock1" XIP kernel
> >> 0x0032-0x0052"mtdblock2"
> >> 0x0052-0x0100"mtdblock3"
> >>
> >> Suggests a size of 0x0100 (16MiB).  Now we have three candidates.
> >>
> >> Pick one, any one, and I'll adjust my patch.  All I really care about is
> >> getting argument @size consistent with arguments @sector_len and
> >> @nb_blocs, so I can ditch @nb_blocs in PATCH 09.
> >>
> >>> No idea what the block size would be.
> >>
> >> As long as that's the case, inertia wins by default.
> >
> > There is also a paper [*]:
> >
> >   The Renesas Technology R0P751RLC001RL (R2DPLUS) board was used
> >   as our target device.
> >   This board is often used to evaluate software for CE devices.
> >   The specification is shown below.
> > CPU: SH7751R(SH4) 240Mhz
> > RAM: 64Mbyte
> > Compact flash: 512Mbyte
> > Flash ROM: 64Mbyte (32Mbyte available for root file system)
>
> Candidate #4: 64MiB.  Bring 'em on!
>
> >
> > Let's use at least 16MB to be able to run the elinux cited kernel.
> >
> > [*] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-125-134.pdf
>
> That's a vote for changing the status quo (8 MiB).
>
> Perhaps Magnus, who maintains the machine, can pick a new value for us.

According to the old board user document in Japanese (under NDA) what
is referred to as FROM (Area0) is connected via a 32-bit bus and CS0
to CN8. The docs mention s29pl127j60tfi130 but since I don't have the
board handy ATM I don't know how the chips are connected.

Hope this helps,

/ magnus

Re: [Qemu-devel] [QEMU-PPC] [PATCH 0/2] Enable mitigations by default for pseries-4.0 machine type

[David Gibson]

On Fri, Mar 01, 2019 at 03:46:07PM +1100, Suraj Jitindar Singh wrote:
> This series is based on the ppc-for-4.0 branch with my large-decrementer
> and count-cache-flush series applied.
> 
> Suraj Jitindar Singh (2):
>   target/ppc/tcg: make spapr_caps apply cap-[cfpc/sbbc/ibs] non-fatal
> for tcg
>   target/ppc/spapr: Enable mitigations by default for pseries-4.0
> machine type

Applied to ppc-for-4.0, thanks.

> 
>  hw/ppc/spapr.c  |  9 ++---
>  hw/ppc/spapr_caps.c | 33 -
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 0/8] target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order

[David Gibson]

On Sun, Mar 03, 2019 at 05:23:35PM +, Mark Cave-Ayland wrote:
> After some investigation into Andrew's report of corruption in his ppc64le
> tests at https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07234.html, 
> I
> discovered the underlying cause was that the first 32 VSX registers are not
> stored in host endian order.
> 
> This is something that Richard and I had discussed before, but missed that 
> with
> VSX if you have source registers from different register sets then even 
> logical
> operations will give you the wrong result.
> 
> Rather than revert 7b8fe477e1 "target/ppc: convert VSX logical operations to
> vector operations" let's keep the use of the accelerated vector instructions,
> and instead fix the real problem which is to switch the first 32 VSX registers
> to host endian order matching the VMX registers.
> 
> Patches 1-5 aim to consolidate the offset calculations for both CPUPPCState
> and the associated _ptr() functions into one single place.
> 
> With this preliminary work complete, patch 6 switches the first 32 registers
> into host endian order without too much difficulty.
> 
> Finally now that all VSX registers are stored in the same way, the vsr offset
> functions and get_cpu_vsrh()/get_cpu_vsrl() can be simplified accordingly.
> 
> Signed-off-by: Mark Cave-Ayland 

I've applied the first two patches.  The rest I'll wait on a respin
addressing Richard's comments.

> 
> 
> Mark Cave-Ayland (8):
>   target/ppc: introduce single fpr_offset() function
>   target/ppc: introduce single vsrl_offset() function
>   target/ppc: move Vsr* macros from internal.h to cpu.h
>   target/ppc: introduce avrh_offset() and avrl_offset() functions
>   target/ppc: introduce avr_offset() function
>   target/ppc: switch fpr/vsrl registers so all VSX registers are in host
> endian order
>   target/ppc: introduce vsrh_offset() function
>   target/ppc: simplify get_cpu_vsrh() and get_cpu_vsrl() functions
> 
>  target/ppc/cpu.h| 56 
> +++--
>  target/ppc/internal.h   | 27 +++---
>  target/ppc/machine.c|  8 +++---
>  target/ppc/translate.c  | 28 ---
>  target/ppc/translate/vmx-impl.inc.c | 27 --
>  target/ppc/translate/vsx-impl.inc.c | 39 +++---
>  6 files changed, 88 insertions(+), 97 deletions(-)
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/8] target/ppc: introduce single vsrl_offset() function

[David Gibson]

On Sun, Mar 03, 2019 at 05:23:37PM +, Mark Cave-Ayland wrote:
> Instead of having multiple copies of the offset calculation logic, move it to 
> a
> single vsrl_offset() function.
> 
> This commit also renames the existing get_vsr()/set_vsr() functions to
> get_vsrl()/set_vsrl() which better describes their purpose.
> 
> Signed-off-by: Mark Cave-Ayland 

Applied to ppc-for-4.0, thanks.

> ---
>  target/ppc/cpu.h|  7 ++-
>  target/ppc/translate/vsx-impl.inc.c | 12 ++--
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 4bb4e42670..4a7df13c2d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2573,9 +2573,14 @@ static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, 
> int i)
>  return (uint64_t *)((uintptr_t)env + fpr_offset(i));
>  }
>  
> +static inline int vsrl_offset(int i)
> +{
> +return offsetof(CPUPPCState, vsr[i].u64[1]);
> +}
> +
>  static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
>  {
> -return &env->vsr[i].u64[1];
> +return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
>  }
>  
>  static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
> diff --git a/target/ppc/translate/vsx-impl.inc.c 
> b/target/ppc/translate/vsx-impl.inc.c
> index e73197e717..381ae0f2e9 100644
> --- a/target/ppc/translate/vsx-impl.inc.c
> +++ b/target/ppc/translate/vsx-impl.inc.c
> @@ -1,13 +1,13 @@
>  /***   VSX extension   
> ***/
>  
> -static inline void get_vsr(TCGv_i64 dst, int n)
> +static inline void get_vsrl(TCGv_i64 dst, int n)
>  {
> -tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
> +tcg_gen_ld_i64(dst, cpu_env, vsrl_offset(n));
>  }
>  
> -static inline void set_vsr(int n, TCGv_i64 src)
> +static inline void set_vsrl(int n, TCGv_i64 src)
>  {
> -tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[n].u64[1]));
> +tcg_gen_st_i64(src, cpu_env, vsrl_offset(n));
>  }
>  
>  static inline int vsr_full_offset(int n)
> @@ -27,7 +27,7 @@ static inline void get_cpu_vsrh(TCGv_i64 dst, int n)
>  static inline void get_cpu_vsrl(TCGv_i64 dst, int n)
>  {
>  if (n < 32) {
> -get_vsr(dst, n);
> +get_vsrl(dst, n);
>  } else {
>  get_avr64(dst, n - 32, false);
>  }
> @@ -45,7 +45,7 @@ static inline void set_cpu_vsrh(int n, TCGv_i64 src)
>  static inline void set_cpu_vsrl(int n, TCGv_i64 src)
>  {
>  if (n < 32) {
> -set_vsr(n, src);
> +set_vsrl(n, src);
>  } else {
>  set_avr64(n - 32, src, false);
>  }

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/8] target/ppc: introduce single fpr_offset() function

[David Gibson]

On Sun, Mar 03, 2019 at 05:23:36PM +, Mark Cave-Ayland wrote:
> Instead of having multiple copies of the offset calculation logic, move it to 
> a
> single fpr_offset() function.
> 
> Signed-off-by: Mark Cave-Ayland 

Applied to ppc-for-4.0, thanks.

> ---
>  target/ppc/cpu.h   | 7 ++-
>  target/ppc/translate.c | 4 ++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 26604ddf98..4bb4e42670 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2563,9 +2563,14 @@ static inline bool lsw_reg_in_range(int start, int 
> nregs, int rx)
>  }
>  
>  /* Accessors for FP, VMX and VSX registers */
> +static inline int fpr_offset(int i)
> +{
> +return offsetof(CPUPPCState, vsr[i].u64[0]);
> +}
> +
>  static inline uint64_t *cpu_fpr_ptr(CPUPPCState *env, int i)
>  {
> -return &env->vsr[i].u64[0];
> +return (uint64_t *)((uintptr_t)env + fpr_offset(i));
>  }
>  
>  static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 819221f246..3b1992faf1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6677,12 +6677,12 @@ GEN_TM_PRIV_NOOP(trechkpt);
>  
>  static inline void get_fpr(TCGv_i64 dst, int regno)
>  {
> -tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
> +tcg_gen_ld_i64(dst, cpu_env, fpr_offset(regno));
>  }
>  
>  static inline void set_fpr(int regno, TCGv_i64 src)
>  {
> -tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState, vsr[regno].u64[0]));
> +tcg_gen_st_i64(src, cpu_env, fpr_offset(regno));
>  }
>  
>  static inline void get_avr64(TCGv_i64 dst, int regno, bool high)

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC PATCH v4 1/5] target/ppc: Move exception vector offset computation into a function

[David Gibson]

On Thu, Feb 28, 2019 at 07:57:55PM -0300, Fabiano Rosas wrote:
> Signed-off-by: Fabiano Rosas 
> Reviewed-by: Alexey Kardashevskiy 

This is a nice cleanup, regardless of the rest of the series.  Applied
to ppc-for-4.0.

> ---
>  target/ppc/excp_helper.c | 30 +++---
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 39bedbb11d..beafcf1ebd 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -107,6 +107,24 @@ static int powerpc_reset_wakeup(CPUState *cs, 
> CPUPPCState *env, int excp,
>  return POWERPC_EXCP_RESET;
>  }
>  
> +static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
> +{
> +uint64_t offset = 0;
> +
> +switch (ail) {
> +case AIL_0001_8000:
> +offset = 0x18000;
> +break;
> +case AIL_C000___4000:
> +offset = 0xc0004000ull;
> +break;
> +default:
> +cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> +break;
> +}
> +
> +return offset;
> +}
>  
>  /* Note that this function should be greatly optimized
>   * when called with a constant excp, from ppc_hw_interrupt
> @@ -708,17 +726,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>  /* Handle AIL */
>  if (ail) {
>  new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> -switch(ail) {
> -case AIL_0001_8000:
> -vector |= 0x18000;
> -break;
> -case AIL_C000___4000:
> -vector |= 0xc0004000ull;
> -break;
> -default:
> -cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> -break;
> -}
> +vector |= ppc_excp_vector_offset(cs, ail);
>  }
>  
>  #if defined(TARGET_PPC64)

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/2] curses: add option to specify VGA font encoding

[Markus Armbruster]

Samuel Thibault  writes:

> This uses iconv to convert glyphs from the specified VGA font encoding to
> unicode, and makes use of cchar_t instead of chtype when using ncursesw,
> which allows to store all wide char as well as the WACS values.
>
> Signed-off-by: Samuel Thibault 
> Cc: Eddie Kohler 
> ---
>  configure   |   5 +-
>  qapi/ui.json|   4 +-
>  qemu-options.hx |   5 +-
>  ui/curses.c | 315 
>  4 files changed, 279 insertions(+), 50 deletions(-)
>
> diff --git a/configure b/configure
> index 9979ca708d..1270dc8dc0 100755
> --- a/configure
> +++ b/configure
> @@ -3449,14 +3449,17 @@ if test "$curses" != "no" ; then
>  #include 
>  #include 
>  #include 
> +#include 
>  int main(void) {
> +  const char *codeset;
>wchar_t wch = L'w';
>setlocale(LC_ALL, "");
>resize_term(0, 0);
>addwstr(L"wide chars\n");
>addnwstr(&wch, 1);
>add_wch(WACS_DEGREE);
> -  return 0;
> +  codeset = nl_langinfo(CODESET);
> +  return codeset != 0;
>  }
>  EOF
>IFS=:
> diff --git a/qapi/ui.json b/qapi/ui.json
> index c5d1d7f099..12d3a2c751 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1131,6 +1131,7 @@
>  # @full-screen:   Start user interface in fullscreen mode (default: off).
>  # @window-close:  Allow to quit qemu with window close button (default: on).
>  # @gl:Enable OpenGL support (default: off).
> +# @charset:   Font charset used by guest (default: CP437).

Can you give brief rationale for defaulting to CP437?

>  #
>  # Since: 2.12
>  #
> @@ -1139,7 +1140,8 @@
>'base': { 'type'   : 'DisplayType',
>  '*full-screen'   : 'bool',
>  '*window-close'  : 'bool',
> -'*gl': 'DisplayGLMode' },
> +'*gl': 'DisplayGLMode',
> +'*charset'   : 'str' },
>'discriminator' : 'type',
>'data': { 'gtk': 'DisplayGTK',
>  'egl-headless'   : 'DisplayEGLHeadless'} }
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 1cf9aac1fe..4bc4e736bb 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1216,7 +1216,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>  "[,window_close=on|off][,gl=on|core|es|off]\n"
>  "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
>  "-display vnc=[,]\n"
> -"-display curses\n"
> +"-display curses[,charset=]\n"
>  "-display none\n"
>  "-display egl-headless[,rendernode=]"
>  "select display type\n"
> @@ -1248,6 +1248,9 @@ support a text mode, QEMU can display this output using 
> a
>  curses/ncurses interface. Nothing is displayed when the graphics
>  device is in graphical mode or if the graphics device does not support
>  a text mode. Generally only the VGA device models support text mode.
> +The font charset used by the guest can be specified with the
> +@code{charset} option, for example @code{charset=CP850} for IBM CP850
> +encoding. The default is @code{CP437}.
>  @item none
>  Do not display video output. The guest will still see an emulated
>  graphics card, but its output will not be displayed to the QEMU
> diff --git a/ui/curses.c b/ui/curses.c
> index 1724dd57d4..203cc075d0 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
[...]
> @@ -492,6 +709,10 @@ static void curses_display_init(DisplayState *ds, 
> DisplayOptions *opts)
>  }
>  #endif
>  
> +setlocale(LC_CTYPE, "");

General principles: any change to locale deserves prominent mention in
the commit message.

> +if (opts->charset) {
> +font_charset = opts->charset;
> +}
>  curses_setup();
>  curses_keyboard_setup();
>  atexit(curses_atexit);

[Qemu-devel] [PATCH] exec.c: remove an unnecessary condition in flatview_add_to_dispatch()

[Wei Yang]

flatview_add_to_dispatch() registers page based on the condition of
*section*, which may looks like this:

|s|PPP|s|

where s stands for subpage and P for page.

The procedure of this function could be described as:

- register first subpage
- register page
- register last subpage

This means only the first offset_within_address_space could be not
TARGET_PAGE_SIZE aligned. During the wile loop, this will not happen.

This patch just removes the unnecessary condition and adds some comment
to clarify it.

Signed-off-by: Wei Yang 
---
 exec.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/exec.c b/exec.c
index 518064530b..e6221d52ba 100644
--- a/exec.c
+++ b/exec.c
@@ -1599,12 +1599,26 @@ static void register_multipage(FlatView *fv,
 phys_page_set(d, start_addr >> TARGET_PAGE_BITS, num_pages, section_index);
 }
 
+/*
+ * The range in *section* may look like this:
+ *
+ *  |s|PPP|s|
+ *
+ * where s stands for subpage and P for page.
+ *
+ * The procedure in following function could be described as:
+ *
+ * - register first subpage
+ * - register page
+ * - register last subpage
+ */
 void flatview_add_to_dispatch(FlatView *fv, MemoryRegionSection *section)
 {
 MemoryRegionSection now = *section, remain = *section;
 Int128 page_size = int128_make64(TARGET_PAGE_SIZE);
 
 if (now.offset_within_address_space & ~TARGET_PAGE_MASK) {
+/* register first subpage */
 uint64_t left = TARGET_PAGE_ALIGN(now.offset_within_address_space)
- now.offset_within_address_space;
 
@@ -1619,11 +1633,10 @@ void flatview_add_to_dispatch(FlatView *fv, 
MemoryRegionSection *section)
 remain.offset_within_region += int128_get64(now.size);
 now = remain;
 if (int128_lt(remain.size, page_size)) {
-register_subpage(fv, &now);
-} else if (remain.offset_within_address_space & ~TARGET_PAGE_MASK) {
-now.size = page_size;
+/* register last subpage */
 register_subpage(fv, &now);
 } else {
+/* register page */
 now.size = int128_and(now.size, int128_neg(page_size));
 register_multipage(fv, &now);
 }
-- 
2.19.1

Re: [Qemu-devel] [RFC PATCH v4 2/5] kvm-all: Introduce kvm_set_singlestep

[David Gibson]

On Thu, Feb 28, 2019 at 07:57:56PM -0300, Fabiano Rosas wrote:
> For single stepping (via KVM) of a guest vcpu to work, KVM needs not
> only to support the SET_GUEST_DEBUG ioctl but to also recognize the
> KVM_GUESTDBG_SINGLESTEP bit in the control field of the
> kvm_guest_debug struct.
> 
> This patch adds support for querying the single step capability so
> that QEMU can decide what to do for the platforms that do not have
> such support.
> 
> This will allow architecture-specific implementations of a fallback
> mechanism for single stepping in cases where KVM does not support it.
> 
> Signed-off-by: Fabiano Rosas 
> ---
>  accel/kvm/kvm-all.c | 16 
>  accel/stubs/kvm-stub.c  |  4 
>  exec.c  |  2 +-
>  include/sysemu/kvm.h|  3 +++
>  stubs/Makefile.objs |  1 +
>  stubs/kvm-arch-set-singlestep.c |  8 
>  6 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/kvm-arch-set-singlestep.c
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index fd92b6f375..d3ac5a9e5c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2267,6 +2267,13 @@ bool kvm_arm_supports_user_irq(void)
>  return kvm_check_extension(kvm_state, KVM_CAP_ARM_USER_IRQ);
>  }
>  
> +/* Whether the KVM_SET_GUEST_DEBUG ioctl supports single stepping */
> +int kvm_has_guestdbg_singlestep(void)
> +{
> +/* return kvm_check_extension(kvm_state, KVM_CAP_GUEST_DEBUG_SSTEP); */

I don't see a KVM_CAP_GUEST_DEBUG_SSTEP in either the qemu or kernel
trees.  Where does that come from?

> +return 0;
> +}
> +
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>  struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
>   target_ulong pc)
> @@ -2316,6 +2323,15 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned 
> long reinject_trap)
>  return data.err;
>  }
>  
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +if (kvm_has_guestdbg_singlestep()) {
> +kvm_update_guest_debug(cs, 0);
> +} else {
> +kvm_arch_set_singlestep(cs, enabled);
> +}
> +}
> +
>  int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>target_ulong len, int type)
>  {
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 02d5170031..69bd07f50e 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -79,6 +79,10 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long 
> reinject_trap)
>  return -ENOSYS;
>  }
>  
> +void kvm_set_singlestep(CPUState *cs, int enabled)
> +{
> +}
> +
>  int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
>target_ulong len, int type)
>  {
> diff --git a/exec.c b/exec.c
> index 518064530b..8817513e26 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1236,7 +1236,7 @@ void cpu_single_step(CPUState *cpu, int enabled)
>  if (cpu->singlestep_enabled != enabled) {
>  cpu->singlestep_enabled = enabled;
>  if (kvm_enabled()) {
> -kvm_update_guest_debug(cpu, 0);
> +kvm_set_singlestep(cpu, enabled);
>  } else {
>  /* must flush all the translated code to avoid inconsistencies */
>  /* XXX: only flush what is necessary */
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index a6d1cd190f..e1ef2f5b99 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -214,6 +214,7 @@ int kvm_has_pit_state2(void);
>  int kvm_has_many_ioeventfds(void);
>  int kvm_has_gsi_routing(void);
>  int kvm_has_intx_set_mask(void);
> +int kvm_has_guestdbg_singlestep(void);
>  
>  int kvm_init_vcpu(CPUState *cpu);
>  int kvm_cpu_exec(CPUState *cpu);
> @@ -246,6 +247,7 @@ bool kvm_memcrypt_enabled(void);
>   */
>  int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len);
>  
> +void kvm_arch_set_singlestep(CPUState *cpu, int enabled);
>  
>  #ifdef NEED_CPU_H
>  #include "cpu.h"
> @@ -258,6 +260,7 @@ int kvm_remove_breakpoint(CPUState *cpu, target_ulong 
> addr,
>target_ulong len, int type);
>  void kvm_remove_all_breakpoints(CPUState *cpu);
>  int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
> +void kvm_set_singlestep(CPUState *cs, int enabled);
>  
>  int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>  int kvm_on_sigbus(int code, void *addr);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 269dfa5832..884f9b2268 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -12,6 +12,7 @@ stub-obj-y += get-vm-name.o
>  stub-obj-y += iothread.o
>  stub-obj-y += iothread-lock.o
>  stub-obj-y += is-daemonized.o
> +stub-obj-y += kvm-arch-set-singlestep.o
>  stub-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>  stub-obj-y += machine-init-done.o
>  stub-obj-y += migr-blocker.o
> diff --git a/stubs/kvm-arch-set-singlestep.c b/stubs/kvm-arch-set-singlestep.c
> new file mode 100644
> index 000

Re: [Qemu-devel] [RFC PATCH v4 3/5] target/ppc: Move handling of hardware breakpoints to a separate function

[David Gibson]

On Thu, Feb 28, 2019 at 07:57:57PM -0300, Fabiano Rosas wrote:
> This is in preparation for a refactoring of the kvm_handle_debug
> function in the next patch.
> 
> Signed-off-by: Fabiano Rosas 

Nice cleanup regardless of anything else.  Applied to ppc-for-4.0.

> ---
>  target/ppc/kvm.c | 47 ---
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index d01852fe31..941c4e7523 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1593,35 +1593,44 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct 
> kvm_guest_debug *dbg)
>  }
>  }
>  
> +static int kvm_handle_hw_breakpoint(CPUState *cs,
> +struct kvm_debug_exit_arch *arch_info)
> +{
> +int handle = 0;
> +int n;
> +int flag = 0;
> +
> +if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> +if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
> +n = find_hw_breakpoint(arch_info->address, GDB_BREAKPOINT_HW);
> +if (n >= 0) {
> +handle = 1;
> +}
> +} else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
> +KVMPPC_DEBUG_WATCH_WRITE)) {
> +n = find_hw_watchpoint(arch_info->address,  &flag);
> +if (n >= 0) {
> +handle = 1;
> +cs->watchpoint_hit = &hw_watchpoint;
> +hw_watchpoint.vaddr = hw_debug_points[n].addr;
> +hw_watchpoint.flags = flag;
> +}
> +}
> +}
> +return handle;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>  CPUState *cs = CPU(cpu);
>  CPUPPCState *env = &cpu->env;
>  struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
>  int handle = 0;
> -int n;
> -int flag = 0;
>  
>  if (cs->singlestep_enabled) {
>  handle = 1;
>  } else if (arch_info->status) {
> -if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> -if (arch_info->status & KVMPPC_DEBUG_BREAKPOINT) {
> -n = find_hw_breakpoint(arch_info->address, 
> GDB_BREAKPOINT_HW);
> -if (n >= 0) {
> -handle = 1;
> -}
> -} else if (arch_info->status & (KVMPPC_DEBUG_WATCH_READ |
> -KVMPPC_DEBUG_WATCH_WRITE)) {
> -n = find_hw_watchpoint(arch_info->address,  &flag);
> -if (n >= 0) {
> -handle = 1;
> -cs->watchpoint_hit = &hw_watchpoint;
> -hw_watchpoint.vaddr = hw_debug_points[n].addr;
> -hw_watchpoint.flags = flag;
> -}
> -}
> -}
> +handle = kvm_handle_hw_breakpoint(cs, arch_info);
>  } else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>  handle = 1;
>  } else {

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC PATCH v4 4/5] target/ppc: Refactor kvm_handle_debug

[David Gibson]

On Thu, Feb 28, 2019 at 07:57:58PM -0300, Fabiano Rosas wrote:
> There are four scenarios being handled in this function:
> 
> - single stepping
> - hardware breakpoints
> - software breakpoints
> - fallback (no debug supported)
> 
> A future patch will add code to handle specific single step and
> software breakpoints cases so let's split each scenario into its own
> function now to avoid hurting readability.
> 
> Signed-off-by: Fabiano Rosas 
> Reviewed-by: Alexey Kardashevskiy 

Again, a nice cleanup regardless of anything else.  Applied.

> ---
>  target/ppc/kvm.c | 86 
>  1 file changed, 50 insertions(+), 36 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 941c4e7523..9392fba192 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1620,52 +1620,66 @@ static int kvm_handle_hw_breakpoint(CPUState *cs,
>  return handle;
>  }
>  
> +static int kvm_handle_singlestep(void)
> +{
> +return 1;
> +}
> +
> +static int kvm_handle_sw_breakpoint(void)
> +{
> +return 1;
> +}
> +
>  static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run)
>  {
>  CPUState *cs = CPU(cpu);
>  CPUPPCState *env = &cpu->env;
>  struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> -int handle = 0;
>  
>  if (cs->singlestep_enabled) {
> -handle = 1;
> -} else if (arch_info->status) {
> -handle = kvm_handle_hw_breakpoint(cs, arch_info);
> -} else if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> -handle = 1;
> -} else {
> -/* QEMU is not able to handle debug exception, so inject
> - * program exception to guest;
> - * Yes program exception NOT debug exception !!
> - * When QEMU is using debug resources then debug exception must
> - * be always set. To achieve this we set MSR_DE and also set
> - * MSRP_DEP so guest cannot change MSR_DE.
> - * When emulating debug resource for guest we want guest
> - * to control MSR_DE (enable/disable debug interrupt on need).
> - * Supporting both configurations are NOT possible.
> - * So the result is that we cannot share debug resources
> - * between QEMU and Guest on BOOKE architecture.
> - * In the current design QEMU gets the priority over guest,
> - * this means that if QEMU is using debug resources then guest
> - * cannot use them;
> - * For software breakpoint QEMU uses a privileged instruction;
> - * So there cannot be any reason that we are here for guest
> - * set debug exception, only possibility is guest executed a
> - * privileged / illegal instruction and that's why we are
> - * injecting a program interrupt.
> - */
> +return kvm_handle_singlestep();
> +}
> +
> +if (arch_info->status) {
> +return kvm_handle_hw_breakpoint(cs, arch_info);
> +}
>  
> -cpu_synchronize_state(cs);
> -/* env->nip is PC, so increment this by 4 to use
> - * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> - */
> -env->nip += 4;
> -cs->exception_index = POWERPC_EXCP_PROGRAM;
> -env->error_code = POWERPC_EXCP_INVAL;
> -ppc_cpu_do_interrupt(cs);
> +if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> +return kvm_handle_sw_breakpoint();
>  }
>  
> -return handle;
> +/*
> + * QEMU is not able to handle debug exception, so inject
> + * program exception to guest;
> + * Yes program exception NOT debug exception !!
> + * When QEMU is using debug resources then debug exception must
> + * be always set. To achieve this we set MSR_DE and also set
> + * MSRP_DEP so guest cannot change MSR_DE.
> + * When emulating debug resource for guest we want guest
> + * to control MSR_DE (enable/disable debug interrupt on need).
> + * Supporting both configurations are NOT possible.
> + * So the result is that we cannot share debug resources
> + * between QEMU and Guest on BOOKE architecture.
> + * In the current design QEMU gets the priority over guest,
> + * this means that if QEMU is using debug resources then guest
> + * cannot use them;
> + * For software breakpoint QEMU uses a privileged instruction;
> + * So there cannot be any reason that we are here for guest
> + * set debug exception, only possibility is guest executed a
> + * privileged / illegal instruction and that's why we are
> + * injecting a program interrupt.
> + */
> +cpu_synchronize_state(cs);
> +/*
> + * env->nip is PC, so increment this by 4 to use
> + * ppc_cpu_do_interrupt(), which set srr0 = env->nip - 4.
> + */
> +env->nip += 4;
> +cs->exception_index = POWERPC_EXCP_PROGRAM;
> +env->error_code = POWERPC_EXCP_INVAL;
> +ppc_cpu_do_interrupt(cs);
> +
> +return 0;
>  }
>  
>  int kvm_arch_handle_exit(C

Re: [Qemu-devel] [PATCH 2/2] curses: add option to specify VGA font encoding

[Samuel Thibault]

Markus Armbruster, le lun. 04 mars 2019 07:44:34 +0100, a ecrit:
> Samuel Thibault  writes:
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1131,6 +1131,7 @@
> >  # @full-screen:   Start user interface in fullscreen mode (default: off).
> >  # @window-close:  Allow to quit qemu with window close button (default: 
> > on).
> >  # @gl:Enable OpenGL support (default: off).
> > +# @charset:   Font charset used by guest (default: CP437).
> 
> Can you give brief rationale for defaulting to CP437?

I have added to the commit message:

“
The default charset is made CP437 since that is the charset of the
hardware default VGA font.
”

> > @@ -492,6 +709,10 @@ static void curses_display_init(DisplayState *ds, 
> > DisplayOptions *opts)
> >  }
> >  #endif
> >  
> > +setlocale(LC_CTYPE, "");
> 
> General principles: any change to locale deserves prominent mention in
> the commit message.

I have added to the commit message:

“
This also makes the curses backend set the LC_CTYPE locale to "" to
allow curses to emit wide characters.
”

Samuel

Re: [Qemu-devel] [libvirt] [PATCH 1/2] numa: deprecate 'mem' parameter of '-numa node' option

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Fri, Mar 01, 2019 at 06:33:28PM +0100, Igor Mammedov wrote:
>> On Fri, 1 Mar 2019 15:49:47 +
>> Daniel P. Berrangé  wrote:
>> 
>> > On Fri, Mar 01, 2019 at 04:42:15PM +0100, Igor Mammedov wrote:
>> > > The parameter allows to configure fake NUMA topology where guest
>> > > VM simulates NUMA topology but not actually getting a performance
>> > > benefits from it. The same or better results could be achieved
>> > > using 'memdev' parameter. In light of that any VM that uses NUMA
>> > > to get its benefits should use 'memdev' and to allow transition
>> > > initial RAM to device based model, deprecate 'mem' parameter as
>> > > its ad-hoc partitioning of initial RAM MemoryRegion can't be
>> > > translated to memdev based backend transparently to users and in
>> > > compatible manner (migration wise).
>> > > 
>> > > That will also allow to clean up a bit our numa code, leaving only
>> > > 'memdev' impl. in place and several boards that use node_mem
>> > > to generate FDT/ACPI description from it.  
>> > 
>> > Can you confirm that the  'mem' and 'memdev' parameters to -numa
>> > are 100% live migration compatible in both directions ?  Libvirt
>> > would need this to be the case in order to use the 'memdev' syntax
>> > instead.
>> Unfortunately they are not migration compatible in any direction,
>> if it where possible to translate them to each other I'd alias 'mem'
>> to 'memdev' without deprecation. The former sends over only one
>> MemoryRegion to target, while the later sends over several (one per
>> memdev).
>
> If we can't migration from one to the other, then we can not deprecate
> the existing 'mem' syntax. Even if libvirt were to provide a config
> option to let apps opt-in to the new syntax, we need to be able to
> support live migration of existing running VMs indefinitely. Effectively
> this means we need the to keep 'mem' support forever, or at least such
> a long time that it effectively means forever.
>
> So I think this patch has to be dropped & replaced with one that
> simply documents that memdev syntax is preferred.

We have this habit of postulating absolutes like "can not deprecate"
instead of engaging with the tradeoffs.  We need to kick it.

So let's have an actual look at the tradeoffs.

We don't actually "support live migration of existing running VMs
indefinitely".

We support live migration to any newer version of QEMU that still
supports the machine type.

We support live migration to any older version of QEMU that already
supports the machine type and all the devices the machine uses.

Aside: "support" is really an honest best effort here.  If you rely on
it, use a downstream that puts in the (substantial!) QA work real
support takes.

Feature deprecation is not a contract to drop the feature after two
releases, or even five.  It's a formal notice that users of the feature
should transition to its replacement in an orderly manner.

If I understand Igor correctly, all users should transition away from
outdated NUMA configurations at least for new VMs in an orderly manner.

So, how could this formal notice be served constructively?

If we reject outdated NUMA configurations starting with machine type T,
we can remove the means to create those configurations along with
machine type T-1.  Won't happen anytime soon, will happen eventually,
because in the long run, all machine types are dead (apologies to
Keynes).

If we deprecate outdated NUMA configurations now, we can start rejecting
them with new machine types after a suitable grace period.

[Qemu-devel] [PATCH v6 0/2] CODING_STYLE: trivial update

[Wei Yang]

The first one is suggested by Igor Mammedov to provide rule for multiline
code.

The second is a trivial fix to make example code all indented with 4 spaces.

v6:
  * add ) for last example of function
v5:
  * mostly address function variants
v4:
  * one exception case for function
v3:
  * fix typo in both changelog and example
v2:
  * adjust Patch 1 as suggested by Eric

Wei Yang (2):
  CODING_STYLE: specify the indent rule for multiline code
  CODING_STYLE: indent example code as all others

 CODING_STYLE | 47 +++
 1 file changed, 43 insertions(+), 4 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH v6 2/2] CODING_STYLE: indent example code as all others

[Wei Yang]

All the example code are indented with four spaces except this one.

Fix this by adding four spaces here.

Signed-off-by: Wei Yang 
Reviewed-by: Eric Blake 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Igor Mammedov 
---
 CODING_STYLE | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index 90321e9c28..cb8edcbb36 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -147,10 +147,10 @@ block to a separate function altogether.
 When comparing a variable for (in)equality with a constant, list the
 constant on the right, as in:
 
-if (a == 1) {
-/* Reads like: "If a equals 1" */
-do_something();
-}
+if (a == 1) {
+/* Reads like: "If a equals 1" */
+do_something();
+}
 
 Rationale: Yoda conditions (as in 'if (1 == a)') are awkward to read.
 Besides, good compilers already warn users when '==' is mis-typed as '=',
-- 
2.19.1

[Qemu-devel] [PATCH v6 1/2] CODING_STYLE: specify the indent rule for multiline code

[Wei Yang]

We didn't specify the indent rule for multiline code here, which may
mislead users. And in current code, the code use various styles.

Add this rule in CODING_STYLE to make sure this is clear to every one.

Signed-off-by: Wei Yang 
Suggested-by: Igor Mammedov 

---
v6:
   * add ) for last example of function
v5:
   * different rules -> various styles
   * describe function variants separately
   * take struct out
v4:
   * widths -> width
   * add an exception example for function
v3:
   * misleading -> mislead
   * add comma after arg2 in example
v2:
   * rephrase changelog suggested by Eric Blake
 - remove one redundant line
 - fix some awkward grammar
 - add { ; at the end of example
---
 CODING_STYLE | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/CODING_STYLE b/CODING_STYLE
index ec075dedc4..90321e9c28 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -29,6 +29,45 @@ Spaces of course are superior to tabs because:
 
 Do not leave whitespace dangling off the ends of lines.
 
+1.1 Multiline Indent
+
+There are several places where indent is necessary:
+
+ - if/else
+ - while/for
+ - function definition & call
+
+When breaking up a long line to fit within line width, we need a proper indent
+for the following lines.
+
+In case of if/else, while/for, align the secondary lines just after the
+opening parenthesis of the first.
+
+For example:
+
+if (a == 1 &&
+b == 2) {
+
+while (a == 1 &&
+   b == 2) {
+
+In case of function, there are several variants:
+
+* 4 spaces indent from the beginning
+* align the secondary lines just after the opening parenthesis of the
+  first
+
+For example:
+
+do_something(x, y,
+z);
+
+do_something(x, y,
+ z);
+
+do_something(x, do_another(y,
+   z));
+
 2. Line width
 
 Lines should be 80 characters; try not to make them longer.
-- 
2.19.1

Re: [Qemu-devel] [PATCH] tests: Do not use "\n" in g_test_message() strings

[Markus Armbruster]

Thomas Huth  writes:

> g_test_message() takes care of the newline on its own, so we
> should not use \n in the strings here.
>
> Signed-off-by: Thomas Huth 

Without "[PATCH] tests: Remove (mostly) useless architecture checks",
the patch misses four instances of '\n', so:

Based-on: <1551456970-463-1-git-send-email-th...@redhat.com>

Re: [Qemu-devel] [PATCH 06/10] r2d: Flash memory creation is confused about size, mark FIXME

[Markus Armbruster]

Magnus Damm  writes:

> Hi guys,
>
> On Wed, Feb 20, 2019 at 2:31 AM Markus Armbruster  wrote:
>>
>> Philippe Mathieu-Daudé  writes:
>>
>> > On 2/19/19 4:45 PM, Markus Armbruster wrote:
>> >> Peter Maydell  writes:
>> >>
>> >>> On Mon, 18 Feb 2019 at 13:07, Markus Armbruster  
>> >>> wrote:
>> 
>>  pflash_cfi02_register() takes a size in bytes, a block size in bytes
>>  and a number of blocks.  r2d_init() passes FLASH_SIZE, 16 * KiB,
>>  FLASH_SIZE >> 16.  Does not compute: size doesn't match block size *
>>  number of blocks.  The latter happens to win.  I tried to find
>>  documentation on the physcial hardware, no luck.
>> >
>> > "physical"
>>
>> Thanks, will fix.
>>
>> 
>>  For now, adjust the byte size passed to match the actual size created,
>>  and add a FIXME comment.
>> >>>
>> >>> I'm pretty sure that FLASH_SIZE here is supposed to be a
>> >>> byte count of the size of the pflash. That matches what
>> >>> Linux has in arch/sh/boards/mach-r2d/setup.c where it
>> >>> sets up the flash_resource struct.
>> >>
>> >> Okay, that's some evidence for size 0x0200 (32MiB).
>> >>
>> >> However, we've created size (16 * KiB) * (FLASH_SIZE >> 16), i.e. 8MiB,
>> >> since at least commit 368a354f02b (v1.3.0), possibly since forever.
>> >>
>> >>> The r2dplus board is also I think known as RTS7751R2D. That
>> >>> takes us to https://elinux.org/RTS7751R2D_Handling_Manual
>> >>> (sadly the link to the "hardware manual" is broken).
>> >>
>> >> Quote section Flash ROM Mapping:
>> >>
>> >> Currently, MTD device mapping on Flash ROM is set as below.
>> >> 0x-0x0002"bootloader"
>> >> 0x0002-0x0032"mtdblock1" XIP kernel
>> >> 0x0032-0x0052"mtdblock2"
>> >> 0x0052-0x0100"mtdblock3"
>> >>
>> >> Suggests a size of 0x0100 (16MiB).  Now we have three candidates.
>> >>
>> >> Pick one, any one, and I'll adjust my patch.  All I really care about is
>> >> getting argument @size consistent with arguments @sector_len and
>> >> @nb_blocs, so I can ditch @nb_blocs in PATCH 09.
>> >>
>> >>> No idea what the block size would be.
>> >>
>> >> As long as that's the case, inertia wins by default.
>> >
>> > There is also a paper [*]:
>> >
>> >   The Renesas Technology R0P751RLC001RL (R2DPLUS) board was used
>> >   as our target device.
>> >   This board is often used to evaluate software for CE devices.
>> >   The specification is shown below.
>> > CPU: SH7751R(SH4) 240Mhz
>> > RAM: 64Mbyte
>> > Compact flash: 512Mbyte
>> > Flash ROM: 64Mbyte (32Mbyte available for root file system)
>>
>> Candidate #4: 64MiB.  Bring 'em on!
>>
>> >
>> > Let's use at least 16MB to be able to run the elinux cited kernel.
>> >
>> > [*] https://www.kernel.org/doc/ols/2008/ols2008v2-pages-125-134.pdf
>>
>> That's a vote for changing the status quo (8 MiB).
>>
>> Perhaps Magnus, who maintains the machine, can pick a new value for us.
>
> According to the old board user document in Japanese (under NDA) what
> is referred to as FROM (Area0) is connected via a 32-bit bus and CS0
> to CN8. The docs mention s29pl127j60tfi130 but since I don't have the
> board handy ATM I don't know how the chips are connected.
>
> Hope this helps,

If you want me to change our emulated flash memory's size, please give
me a number.

Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet

[Lucien Murray-Pitts via Qemu-devel]

This fixes a regression in rsp packet vCont, this was due to the
recently added multiprocess support. (Short commit hash: e40e520).

The result is that vCont now does not recognise the case where
no process/thread is provided after the action.

This may not show up with GDB, but using Lauterbach Trace32,
and Hexrays IDA Pro this issue is immediately seen.

The response is a "$#00" empty packet, showing it is unsupported packet.

This is defined in the RSP document as "An action with no thread-id
matches all threads."
(https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet)

This is where the document lacks details. It says that the vCont packet
is used to "Resume the inferior, specifying different actions for each
thread."

So it seems to target only one inferior (i.e. one process).

When multiprocess extension is in use, this packet must be supported,
and used in place of 'Hc' (which modifies s->c_cpu) and other action
packets (s, S, ...) which are deprecated.

So the question is: when the vCont packet does not specifies a
thread-id, what _process_ we should choose?

Going for all process is probably ok because:
  - we don't have to bother choosing a PID,
  - it will cover the mono-process case correctly (main use-case of this
form of vCont packet I think).

The alternative would be can go for GDB_ALL_THREAD and take the PID of the
first attached CPU.  But may lead to problems, so I have chosen the
GDB_ALL_PROCESSES.

A request for improved documentation has been made at;
https://sourceware.org/bugzilla/show_bug.cgi?id=24177

Thus, for now, the valid vCont packets now are as below.
However, parsing is still not very strict and many other invalid arguments
formats are possible but they are not expected from standard debuggers.

 * vCont;c/s
 ** Step/Continue all threads

 * vCont;c/s:[pX.]Y
 ** Step/Continue optional process X, thread Y

 * vCont;C##/S##:[pX.]Y
 ** Step/Continue with signal ## on optional process X, thread Y

 * If X or Y are -1 then it applies the action to all processes/threads.

Signed-off-by: Lucien Murray-Pitts 
---
Third attempt, taking greatly valued input on formatting.
Also includes a change to the way kind is handled for no-action vcont
---
 gdbstub.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index a4be63f6eb..f8680f7ee8 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1145,6 +1145,7 @@ static int is_query_packet(const char *p, const char 
*query, char separator)
  */
 static int gdb_handle_vcont(GDBState *s, const char *p)
 {
+GDBThreadIdKind kind;
 int res, signal = 0;
 char cur_action;
 char *newstates;
@@ -1194,12 +1195,24 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
 goto out;
 }
 
-if (*p++ != ':') {
+/*
+ * When we have vCont;c or vCont;s - action is on all threads
+ * Alternatively action maybe followed by a ';' and more specifiers
+ * such as vCont;c;s:p1.1 is a possible, but meaningless format.
+ * Otherwise if the action is followed by a ':' then
+ * the pPID.TID format is expected (for example "vCont;c:p1.1;...).
+ */
+if (*p == '\0' || *p == ';') {
+/* unclear behavior in RSP spec, assume GDB_ALL_PROCESSES is ok */
+kind = GDB_ALL_PROCESSES;
+} else if (*p++ == ':') {
+kind = read_thread_id(p, &p, &pid, &tid);
+} else {
 res = -ENOTSUP;
 goto out;
 }
 
-switch (read_thread_id(p, &p, &pid, &tid)) {
+switch (kind) {
 case GDB_READ_THREAD_ERR:
 res = -EINVAL;
 goto out;
-- 
2.20.1

Re: [Qemu-devel] [PATCH v13 00/25] Fixing record/replay and adding reverse debugging

[Pavel Dovgalyuk]

Ping.
Can anyone review block-related patches?

Pavel Dovgalyuk

> -Original Message-
> From: Pavel Dovgalyuk [mailto:pavel.dovga...@ispras.ru]
> Sent: Thursday, February 21, 2019 2:04 PM
> To: qemu-devel@nongnu.org
> Cc: kw...@redhat.com; peter.mayd...@linaro.org; war2jor...@live.com;
> crosthwaite.pe...@gmail.com; boost.li...@gmail.com; 
> artem.k.pisare...@gmail.com;
> quint...@redhat.com; ciro.santi...@gmail.com; jasow...@redhat.com; 
> m...@redhat.com;
> arm...@redhat.com; mre...@redhat.com; maria.klimushenk...@ispras.ru; 
> dovga...@ispras.ru;
> kra...@redhat.com; pavel.dovga...@ispras.ru; thomas.dull...@googlemail.com;
> pbonz...@redhat.com; alex.ben...@linaro.org; dgilb...@redhat.com; 
> r...@twiddle.net
> Subject: [PATCH v13 00/25] Fixing record/replay and adding reverse debugging
> 
> GDB remote protocol supports reverse debugging of the targets.
> It includes 'reverse step' and 'reverse continue' operations.
> The first one finds the previous step of the execution,
> and the second one is intended to stop at the last breakpoint that
> would happen when the program is executed normally.
> 
> Reverse debugging is possible in the replay mode, when at least
> one snapshot was created at the record or replay phase.
> QEMU can use these snapshots for travelling back in time with GDB.
> 
> Running the execution in replay mode allows using GDB reverse debugging
> commands:
>  - reverse-stepi (or rsi): Steps one instruction to the past.
>QEMU loads on of the prior snapshots and proceeds to the desired
>instruction forward. When that step is reaches, execution stops.
>  - reverse-continue (or rc): Runs execution "backwards".
>QEMU tries to find breakpoint or watchpoint by loaded prior snapshot
>and replaying the execution. Then QEMU loads snapshots again and
>replays to the latest breakpoint. When there are no breakpoints in
>the examined section of the execution, QEMU finds one more snapshot
>and tries again. After the first snapshot is processed, execution
>stops at this snapshot.
> 
> The set of patches include the following modifications:
>  - gdbstub update for reverse debugging support
>  - functions that automatically perform reverse step and reverse
>continue operations
>  - hmp/qmp commands for manipulating the replay process
>  - improvement of the snapshotting for saving the execution step
>in the snapshot parameters
>  - other record/replay fixes
> 
> The patches are available in the repository:
> https://github.com/ispras/qemu/tree/rr-190221
> 
> v13 changes:
>  - rebased to make QAPI stuff applicable
>  - minor reverse step/reverse continue fix
> 
> v12 changes:
>  - style fixes
> 
> v11 changes:
>  - added can_do_io resetting before jumping to the next block in the chain
>  - rebase to the latest master
> 
> v10 changes:
>  - added patch for correct deadline calculation with external timers
>  - updated icount-related documentation in json files
>(suggested by Markus Armbruster)
>  - fixed replay shutdown
>  - renamed some functions and variables to make them consistent with
>the documentation and displayed messages
>  - minor changes
> 
> v9 changes:
>  - moved rr qapi stuff to the separate file (suggested by Markus Armbruster)
>  - minor coding style fixes
> 
> v8 changes:
>  - rebased to the new master
>  - added missing fix for prior rr patch
>  - updated 'since' version number in json-related patches
> 
> v7 changes:
>  - rebased to the new master with upstreamed patches from the series
>  - several improvements in hmp/qmp commands handling (suggested by Markus 
> Armbruster)
>  - fixed record/replay with '-rtc base' option enabled
>  - added document with virtual hardware requirements
> 
> v6 changes:
>  - rebased to the new version of master
>  - fixed build of linux-user configurations
>  - added new clock for slirp and vnc timers
> 
> v5 changes:
>  - multiple fixes of record/replay bugs appeared after QEMU core update
>  - changed reverse debugging to 'since 3.1'
> 
> v4 changes:
>  - changed 'since 2.13' to 'since 3.0' in json (as suggested by Eric Blake)
> 
> v3 changes:
>  - Fixed PS/2 bug with save/load vm, which caused failures of the replay.
>  - Rebased to the new code base.
>  - Minor fixes.
> 
> v2 changes:
>  - documented reverse debugging
>  - fixed start vmstate loading in record mode
>  - documented qcow2 changes (as suggested by Eric Blake)
>  - made icount SnapshotInfo field optional (as suggested by Eric Blake)
>  - renamed qmp commands (as suggested by Eric Blake)
>  - minor changes
> 
> ---
> 
> Pavel Dovgalyuk (24):
>   block: implement bdrv_snapshot_goto for blkreplay
>   replay: disable default snapshot for record/replay
>   replay: update docs for record/replay with block devices
>   replay: don't drain/flush bdrv queue while RR is working
>   replay: finish record/replay before closing the disks
>   qcow2: introduce icount field for snapshots
>   migration: introduce icount f

Re: [Qemu-devel] [PATCH] migration: Cleanup during exit

[Peter Xu]

On Fri, Mar 01, 2019 at 10:11:42AM +, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Thu, Feb 28, 2019 at 12:28:22PM +, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berra...@redhat.com) wrote:
> > > > On Thu, Feb 28, 2019 at 11:40:19AM +, Dr. David Alan Gilbert wrote:
> > > > > * Peter Xu (pet...@redhat.com) wrote:
> > > > > > On Wed, Feb 27, 2019 at 04:49:00PM +, Dr. David Alan Gilbert 
> > > > > > (git) wrote:
> > > > > > > From: "Dr. David Alan Gilbert" 
> > > > > > > 
> > > > > > > Currently we cleanup the migration object as we exit main after 
> > > > > > > the
> > > > > > > main_loop finishes; however if there's a migration running things
> > > > > > > get messy and we can end up with the migration thread still trying
> > > > > > > to access freed structures.
> > > > > > > 
> > > > > > > We now take a ref to the object around the migration thread 
> > > > > > > itself,
> > > > > > > so the act of dropping the ref during exit doesn't cause us to 
> > > > > > > lose
> > > > > > > the state until the thread quits.
> > > > > > > 
> > > > > > > Cancelling the migration during migration also tries to get the 
> > > > > > > thread
> > > > > > > to quit.
> > > > > > > 
> > > > > > > We do this a bit earlier; so hopefully migration gets out of the 
> > > > > > > way
> > > > > > > before all the devices etc are freed.
> > > > > > 
> > > > > > So does it mean that even with the patch it's still possible the
> > > > > > migration thread will be accessing device structs that have already
> > > > > > been freed which can still crash QEMU?
> > > > > 
> > > > > Possibly yes; I'm not sure how to go to the next stage and stop that
> > > > > case; the consensus seems to be we don't want to explicitly block
> > > > > during the exit process, so doing a join on the migration thread 
> > > > > doesn't
> > > > > seem to be wanted.
> > > > 
> > > > Essentially the many  *_cleanup() calls at the end of main() in vl.c
> > > > are only ever safe if all background threads have stopped using the
> > > > resources that are being freed. This isn't the case with migration
> > > > currently. I also worry about other threads that might be running
> > > > in QEMU, SPICE in particular as it touchs many device backends.
> > > > 
> > > > Aborting the migration & joining the migration threads is the natural
> > > > way to address this. Cancelling appears to require the main loop to
> > > > still be running, so would require main_loop_should_exit() to issue
> > > > the cancel & return false unless it has completed. This would delay
> > > > shutdown for as long as it takes migration to abort.
> > > 
> > > ish - cancelling performs a shutdown(2) on the fd, that should be enough
> > > in most cases to kick the migration thread into falling out without
> > > main loop interaction; I think...
> > 
> > Dave, could you hint me on when shutdown() will not suffice (say,
> > we'll hang death if we do join() upon the migration thread)?
> 
> I think I mostly worry about when we hang in a device's migration code
> or where that interacts with an external program (e.g. vhost-user).

I see.

> 
> > > 
> > > > FWIW, even the bdrv_close_all() call during shutdown can delay things
> > > > for a considerable time if draining the backends isn't a quick op,
> > > > which could be the case if there are storage problems (blocked local
> > > > I/O, or interrupted network - rbd/ceph/nfs clients). So I'm not sure
> > > > that stopping migration is inherantly worse than what's already
> > > > possible with block cleanup, in terms of delaying things.
> > > > 
> > > > A slightly more hacky approach would be to pthread_cancel() all the
> > > > migration threads. Normally we'd never use pthread_cancel() as it
> > > > is incredibly hard to ensure all memory used by the threads is
> > > > freed. Since we're about to exit the process though, this cleanup
> > > > does not matter. The risk, however, is that one of the threads is
> > > > holding a mutex which then blocks the rest of the *cleanup functions,
> > > > so this still feels dangerous.
> > > > 
> > > > Overall to me a clean cancel & join of the migration threads feels
> > > > like the only safe option, unless we just remove all notion of
> > > > cleanup from QEMU & delete all the _cleanup functions in main()
> > > > and let the OS free all memory.
> > > 
> > > That's actually my preference; I think trying to do clean tidy ups
> > > here is very racy and doesn't gain much.  However, for things like
> > > storage there may be locks that have to be properly dropped and
> > > bitmaps and things to be stored/sync'd - so just exiting probably
> > > isn't safe either.
> > 
> > I'm unsure about whether I caught the whole idea but I feel like we
> > can't drop all the cleanup hooks since what if we want to do something
> > else than "freeing memories"?  Or anything that the OS can't do for us
> > but we want to try to do before the process quits.  If that operation
>