Re: [Qemu-devel] [PATCH] spice: wakeup QXL worker to pick up mouse changes

[Gerd Hoffmann]

On Mo, 2017-01-30 at 14:45 +0400, Marc-André Lureau wrote:
> Without it, server-mode mouse is "slow" to update position: QXL will
> wait until new display commands come. This is very visible with
> virtio-gpu.

Added to ui queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH] ui/gtk.c: add ctrl-alt-= support for zoom in acceleration

[Gerd Hoffmann]

On Di, 2017-01-31 at 09:32 +0800, Ziyue Yang wrote:
> From: Ziyue Yang 
> 
> Solving wishlist item at
> https://bugs.launchpad.net/qemu/+bug/1656710
> by accepting Ctrl-Alt-= as an additional zoom-in acceleration.
> 
> Using gtk_accel_group_connect to support multiple accelerations
> triggering a single menu item since that gtk_accel_map_add_entry
> seems to support only one acceleration. A wrapper function
> gd_accel_zoom_in is added to support gtk_accel_group_connect's
> callback activities.
> 
> Signed-off-by: Ziyue Yang 

Added to UI queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH v2 00/25] qmp: add async command type

[Gerd Hoffmann]

  Hi,

> > It would be nice for this series to evolve to a generic QMP jobs series
> > so that all background operations are visible to the client and a useful
> > subset of management primitives can be implemented on a per-command
> > basis.  Both live migration and existing block jobs could use this code
> > so that we don't have multiple copies of the same infrastructure.
> 
> Indeed, but I would need to know what proposal or requirements the
> block layer have here, and I would appreciate if they took time to
> review mine. 

Disclaimer:  Havn't looked into the qmp (especially block jobs) details
too much.

But I suspect we can have both, sharing most of the infrastructure,
without too much effort.

We have block jobs today.  Other areas in qemu can use that too.  So
moving the jobs infrastructure from block to qmp level makes perfect
sense to me.  Live migration and possibly others can start using it.

And once we have that building async commands on top of that is probably
easy.  It'll be just a primitive job which typically takes at most a few
seconds to finish, has no ->cancel() callback and sends a different kind
of completion notification ...

cheers,
  Gerd

Re: [Qemu-devel] [libvirt] char: Logging serial pty output when disconnected

[Ed Swierk]

On Fri, Jan 27, 2017 at 1:40 AM, Daniel P. Berrange  wrote:
> On Thu, Jan 26, 2017 at 05:07:16PM -0800, Ed Swierk wrote:
>> Currently qemu_chr_fe_write() calls qemu_chr_fe_write_log() only for
>> data consumed by the backend chr_write function. With the pty backend,
>> pty_chr_write() returns 0 indicating that the data was not consumed
>> when the pty is disconnected. Simply changing it to return len instead
>> of 0 tricks the caller into logging the data even when the pty is
>> disconnected. I don't know what problems this might cause, but one
>> data point is that tcp_chr_write() already happens to work this way.
>>
>> Alternatively, qemu_chr_fe_write() could be modified to log everything
>> passed to it, regardless of how much data chr_write claims to have
>> consumed. The trouble is that the serial device retries writing
>> unconsumed data, so when the pty is disconnected you'd see every
>> character duplicated 4 times in the log file.
>>
>> Any opinions on either approach, or other suggestions? If there are no
>> objections to the first one, I'll prepare a patch.
>
> If the pty backend intends to just drop data into a blackhole when
> no client is connected, then its chr_write() impl should return
> the length of the data discarded, not zero.

That's exactly the question: when no client is connected, should the
pty backend just drop the data into a black hole, returning the length
of the data discarded? Or should it return 0, letting the frontend
device decide what to do with it?

I can't discern a consistent pattern across all the char backends. The
closest analog is the tcp backend, which does discard the data and
return len. In contrast, several backends call
io_channel_send{,_full}(), which returns -1 if the write would block
or fails for any other reason.

It's not clear there's much the frontend can do to recover from an
error, but there's no consistent pattern across serial devices either.
Most just ignore the return value. But the 16550A serial device
retries 4 times after an error. Changing the pty backend to discard
the data on the first attempt would bypass this retry mechanism. Is
that a problem?

--Ed

[Qemu-devel] [Bug 1656710] Re: Please support Ctrl-Alt-= to zoom in

[Ziyue Yang]

** Changed in: qemu
   Status: New => In Progress

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

Title:
  Please support Ctrl-Alt-= to zoom in

Status in QEMU:
  In Progress

Bug description:
  With the GTK3 interface, qemu-system supports pressing Ctrl-Alt-plus
  to zoom in and Ctrl-Alt-minus to zoom out.  However, unlike many
  programs that support similar zoom hotkeys, qemu-system actually
  requires using '+', making the hotkey Ctrl-Alt-Shift-= .  Most programs
  with similar zoom hotkeys allow Ctrl-Alt-= as a synonym.

  Please consider accepting Ctrl-Alt-= as an additional zoom-in hotkey.

  (Observed in QEMU 2.8)

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

Re: [Qemu-devel] [PATCH] target/ppc/cpu-models: Fix/remove bad CPU aliases

[David Gibson]

On Tue, Jan 24, 2017 at 12:48:05PM +0100, Thomas Huth wrote:
> There is no CPU model called "7447_v1.2" in our list, so the
> "7447" alias should point to "7447_v1.1" instead. Let's also
> remove the "codename" aliases that point to non-implemented
> CPU models - they are really of no use here.
> 
> Signed-off-by: Thomas Huth 

Merged to ppc-for-2.9, thanks.

> ---
>  target/ppc/cpu-models.c | 22 ++
>  1 file changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> index 506dee1..4d3e635 100644
> --- a/target/ppc/cpu-models.c
> +++ b/target/ppc/cpu-models.c
> @@ -1375,19 +1375,15 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>  { "7445", "7445_v3.2" },
>  { "7455", "7455_v3.2" },
>  { "Apollo6", "7455" },
> -{ "7447", "7447_v1.2" },
> +{ "7447", "7447_v1.1" },
>  { "7457", "7457_v1.2" },
>  { "Apollo7", "7457" },
>  { "7447A", "7447A_v1.2" },
>  { "7457A", "7457A_v1.2" },
>  { "Apollo7PM", "7457A_v1.0" },
>  #if defined(TARGET_PPC64)
> -{ "Trident", "620" },
>  { "POWER3", "630" },
> -{ "Boxer", "POWER3" },
> -{ "Dino",  "POWER3" },
>  { "POWER3+", "631" },
> -{ "POWER5gr", "POWER5" },
>  { "POWER5+", "POWER5+_v2.1" },
>  { "POWER5gs", "POWER5+_v2.1" },
>  { "POWER7", "POWER7_v2.3" },
> @@ -1399,21 +1395,7 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>  { "970", "970_v2.2" },
>  { "970fx", "970fx_v3.1" },
>  { "970mp", "970mp_v1.1" },
> -{ "Apache", "RS64" },
> -{ "A35","RS64" },
> -{ "NorthStar", "RS64-II" },
> -{ "A50",   "RS64-II" },
> -{ "Pulsar", "RS64-III" },
> -{ "IceStar", "RS64-IV" },
> -{ "IStar",   "RS64-IV" },
> -{ "SStar",   "RS64-IV" },
> -#endif
> -{ "RIOS","POWER" },
> -{ "RSC", "POWER" },
> -{ "RSC3308", "POWER" },
> -{ "RSC4608", "POWER" },
> -{ "RSC2", "POWER2" },
> -{ "P2SC", "POWER2" },
> +#endif
>  
>  /* Generic PowerPCs */
>  #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 3/3] ppc: switch to constants within BUILD_BUG_ON

[David Gibson]

On Fri, Jan 27, 2017 at 06:27:16PM +0200, Michael S. Tsirkin wrote:
> We are switching BUILD_BUG_ON to verify that it's parameter is a
> compile-time constant, and it turns out that some gcc versions
> (specifically gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609) are
> not smart enough to figure it out for expressions involving local
> variables. This is harmless but means that the check is ineffective for
> these platforms.  To fix, replace the variable with macros.
> 
> Reported-by: Peter Maydell 
> Signed-off-by: Michael S. Tsirkin 

Applied to ppc-for-2.9, thanks.

> ---
>  hw/ppc/spapr.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a642e66..b81f2ac 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2630,8 +2630,8 @@ static void spapr_phb_placement(sPAPRMachineState 
> *spapr, uint32_t index,
>   * 1TiB 64-bit MMIO windows for each PHB.
>   */
>  const uint64_t base_buid = 0x8002000ULL;
> -const int max_phbs =
> -(SPAPR_PCI_LIMIT - SPAPR_PCI_BASE) / SPAPR_PCI_MEM64_WIN_SIZE - 1;
> +#define SPAPR_MAX_PHBS ((SPAPR_PCI_LIMIT - SPAPR_PCI_BASE) / \
> +SPAPR_PCI_MEM64_WIN_SIZE - 1)
>  int i;
>  
>  /* Sanity check natural alignments */
> @@ -2640,12 +2640,14 @@ static void spapr_phb_placement(sPAPRMachineState 
> *spapr, uint32_t index,
>  QEMU_BUILD_BUG_ON((SPAPR_PCI_MEM64_WIN_SIZE % SPAPR_PCI_MEM32_WIN_SIZE) 
> != 0);
>  QEMU_BUILD_BUG_ON((SPAPR_PCI_MEM32_WIN_SIZE % SPAPR_PCI_IO_WIN_SIZE) != 
> 0);
>  /* Sanity check bounds */
> -QEMU_BUILD_BUG_ON((max_phbs * SPAPR_PCI_IO_WIN_SIZE) > 
> SPAPR_PCI_MEM32_WIN_SIZE);
> -QEMU_BUILD_BUG_ON((max_phbs * SPAPR_PCI_MEM32_WIN_SIZE) > 
> SPAPR_PCI_MEM64_WIN_SIZE);
> +QEMU_BUILD_BUG_ON((SPAPR_MAX_PHBS * SPAPR_PCI_IO_WIN_SIZE) >
> +  SPAPR_PCI_MEM32_WIN_SIZE);
> +QEMU_BUILD_BUG_ON((SPAPR_MAX_PHBS * SPAPR_PCI_MEM32_WIN_SIZE) >
> +  SPAPR_PCI_MEM64_WIN_SIZE);
>  
> -if (index >= max_phbs) {
> +if (index >= SPAPR_MAX_PHBS) {
>  error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
> -   max_phbs - 1);
> +   SPAPR_MAX_PHBS - 1);
>  return;
>  }
>  

-- 
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 v5 03/18] vfio: allow to notify unmap for very large region

[David Gibson]

On Tue, Jan 24, 2017 at 09:32:07AM -0700, Alex Williamson wrote:
> On Tue, 24 Jan 2017 18:25:56 +0800
> Peter Xu  wrote:
> 
> > Linux vfio driver supports to do VFIO_IOMMU_UNMAP_DMA for a very big
> > region. This can be leveraged by QEMU IOMMU implementation to cleanup
> > existing page mappings for an entire iova address space (by notifying
> > with an IOTLB with extremely huge addr_mask). However current
> > vfio_iommu_map_notify() does not allow that. It make sure that all the
> > translated address in IOTLB is falling into RAM range.
> > 
> > The check makes sense, but it should only be a sensible checker for
> > mapping operations, and mean little for unmap operations.
> > 
> > This patch moves this check into map logic only, so that we'll get
> > faster unmap handling (no need to translate again), and also we can then
> > better support unmapping a very big region when it covers non-ram ranges
> > or even not-existing ranges.
> > 
> > Signed-off-by: Peter Xu 
> > ---
> >  hw/vfio/common.c | 7 +++
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ce55dff..4d90844 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -354,11 +354,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, 
> > IOMMUTLBEntry *iotlb)
> >  return;
> >  }
> >  
> > -if (!vfio_get_vaddr(iotlb, &vaddr, &read_only)) {
> > -return;
> > -}
> > -
> >  if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > +if (!vfio_get_vaddr(iotlb, &vaddr, &read_only)) {
> > +return;
> > +}
> 
> 
> David, is SPAPR going to freak out if it sees unmaps to ranges that
> extend beyond individual mappings, or perhaps include no mappings?
> This effectively allows unmapping the entire address space of the iommu
> in one pass, without validating or translating the backing.

Extending beyond an individual mapping will be fine.  However, if the
unmap extends beyond the extent of IOVAs mapped by a single TCE table,
then the unmap will fail (with ENXIO or EINVAL depending on whether
there's a problem with origin or only size).

With holidays I've lost the context of this thread, so I can't easily
find the whole patch series this relates to.  From your brief
description above it sounds likes it should be ok - a range covering
just the IOVA space (as long as that's actually correct for spapr tce)
should be ok.

> 
> >  ret = vfio_dma_map(container, iova,
> > iotlb->addr_mask + 1, vaddr,
> > read_only);
> 

-- 
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] target/ppc: Remove unused POWERPC_FAMILY(POWER)

[David Gibson]

On Tue, Jan 24, 2017 at 11:55:07AM +0100, Thomas Huth wrote:
> We do not support POWER1 CPUs in QEMU, so it does not make sense
> to keep this stub around.
> 
> Signed-off-by: Thomas Huth 

Merged to ppc-for-2.9, thanks.

> ---
>  target/ppc/translate_init.c | 22 --
>  1 file changed, 22 deletions(-)
> 
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 19ef250..70333c5 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -5217,28 +5217,6 @@ POWERPC_FAMILY(e5500)(ObjectClass *oc, void *data)
>  
>  /* Non-embedded PowerPC  
> */
>  
> -/* POWER : same as 601, without mfmsr, mfsr  
> */
> -POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data)
> -{
> -DeviceClass *dc = DEVICE_CLASS(oc);
> -PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -
> -dc->desc = "POWER";
> -/* pcc->insns_flags = XXX_TODO; */
> -/* POWER RSC (from RAD6000) */
> -pcc->msr_mask = (1ull << MSR_EE) |
> -(1ull << MSR_PR) |
> -(1ull << MSR_FP) |
> -(1ull << MSR_ME) |
> -(1ull << MSR_FE0) |
> -(1ull << MSR_SE) |
> -(1ull << MSR_DE) |
> -(1ull << MSR_AL) |
> -(1ull << MSR_EP) |
> -(1ull << MSR_IR) |
> -(1ull << MSR_DR);
> -}
> -
>  #define POWERPC_MSRR_601 (0x1040ULL)
>  
>  static void init_proc_601 (CPUPPCState *env)

-- 
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] Towards an ivshmem 2.0?

[Wang, Wei W]

On Sunday, January 29, 2017 10:14 PM, Jan Kiszka wrote:
> On 2017-01-29 15:00, Marc-André Lureau wrote:
> > Hi
> >
> > On Sun, Jan 29, 2017 at 12:44 PM Jan Kiszka  > > wrote:
> >
> > >> Of course, I'm careful with investing much time into expanding the
> > >> existing, for Jailhouse possibly sufficient design if there no real
> > >> interest in continuing the ivshmem support in QEMU - because of
> > >> vhost-pci or other reasons. But if that interest exists, it would be
> > >> beneficial for us to have QEMU supporting a compatible version
> > and using
> > >> the same guest drivers. Then I would start looking into concrete
> > patches
> > >> for it as well.
> > >
> > > Interest is difficult for me to gauge, not least because alternatives
> > > are still being worked on.
> >
> > I'm considering to suggest this as GSoC project now.
> >
> >
> > It's better for a student and for the community if the work get
> > accepted in the end.
> >
> > So, I think that could be an intersting GSoC (implementing your
> > ivshmem
> > 2 proposal). However, if the qemu community isn't ready to accept a
> > new ivshmem, and would rather have vhost-pci based solution, I would
> > suggest a different project (hopefully Wei Wang can help define it and 
> > mentor):
> > work on a vhost-pci using dedicated shared PCI BARs (and kernel
> > support to avoid extra copy - if I understand the extra copy situation 
> > correctly).

Thanks for the suggestion. I’m glad to help it. 

For that sort of usage (static configuration extension [1]), I’m thinking that 
it’s possible to build symmetric vhost-pci-net communication, as appose to 
“vhost-pci-net<-> virtio-net”.

> It's still open if vhost-pci can replace ivshmem (not to speak of being 
> desirable
> for Jailhouse - I'm still studying). In that light, having both 
> implementations
> available to do real comparisons is valuable IMHO.
> 
> That said, we will play with open cards, explain the student the situation 
> and let
> her/him decide knowingly.
 
I think the static configuration of vhost-pci would be quite similar to your 
ivshmem based proposal- could be thought of as moving your proposal to the 
virtio device structure. Do you see any more big difference? Or is there any 
fundamental reason that it is not good to do that based on virtio? Thanks.

Best,
Wei

[1] static configuration extension: set the vhost-pci device via the QEMU 
command line (rather than hotplugging via vhost-user protocol) , and  share a 
piece of memory between two VMs (rather than the whole VM's memory)

Re: [Qemu-devel] [PATCH for-2.9 v3 0/5] Sheepdog cleanups

[Paolo Bonzini]

On 04/01/2017 11:47, Jeff Cody wrote:
> On Wed, Jan 04, 2017 at 12:42:53PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 04/01/2017 05:07, Jeff Cody wrote:
>>> On Wed, Dec 21, 2016 at 03:07:07PM +0100, Paolo Bonzini wrote:


 On 29/11/2016 12:32, Paolo Bonzini wrote:
> Cleaning up the code and removing duplication makes it simpler to
> later adapt it for the multiqueue work.
>
> Tested against sheepdog 1.0.  I also tested taking snapshots and reverting
> to older snapshots, but the latter only worked with "dog vdi rollback".
> Neither loadvm nor qemu-img worked for me.
>
> Paolo
>
> v1->v2: placate patchew
> v2->v3: rebase
>
> Paolo Bonzini (5):
>   sheepdog: remove unused cancellation support
>   sheepdog: reorganize coroutine flow
>   sheepdog: do not use BlockAIOCB
>   sheepdog: simplify inflight_aio_head management
>   sheepdog: reorganize check for overlapping requests
>
>  block/sheepdog.c | 289 
> ---
>  1 file changed, 83 insertions(+), 206 deletions(-)
>

 2.8 is now out, so ping.

>>>
>>> I don't have a functional sheepdog setup at the moment; have you tested
>>> these patches, or should I set up a test rig for them? (I am guessing I
>>> should do the latter; either way, I'll pull the patches in once I or someone
>>> else has tested them).
>>
>> Yes, I have tested them.  Fedora's sheepdog package is fubar right now.
>> I've built an updated one and asked for ownership but haven't had any
>> answer yet.
>>
>> Paolo
> 
> Great, thanks.
> 
> Applied to my block branch:
> 
> git://github.com/codyprime/qemu-kvm-jtc.git block
> 
> -Jeff
> 
> 

Ping?

Paolo

[Qemu-devel] [PATCH] ui/gtk.c: add ctrl-alt-= support for zoom in acceleration

[Ziyue Yang]

From: Ziyue Yang 

Solving wishlist item at
https://bugs.launchpad.net/qemu/+bug/1656710
by accepting Ctrl-Alt-= as an additional zoom-in acceleration.

Using gtk_accel_group_connect to support multiple accelerations
triggering a single menu item since that gtk_accel_map_add_entry
seems to support only one acceleration. A wrapper function
gd_accel_zoom_in is added to support gtk_accel_group_connect's
callback activities.

Signed-off-by: Ziyue Yang 
---
 ui/gtk.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/ui/gtk.c b/ui/gtk.c
index bdd831c..3be9f33 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -105,6 +105,7 @@
 #define GDK_KEY_g GDK_g
 #define GDK_KEY_q GDK_q
 #define GDK_KEY_plus GDK_plus
+#define GDK_KEY_equal GDK_equal
 #define GDK_KEY_minus GDK_minus
 #define GDK_KEY_Pause GDK_Pause
 #define GDK_KEY_Delete GDK_Delete
@@ -1325,6 +1326,12 @@ static void gd_menu_zoom_in(GtkMenuItem *item, void 
*opaque)
 gd_update_windowsize(vc);
 }
 
+static void gd_accel_zoom_in(void *opaque)
+{
+GtkDisplayState *s = opaque;
+gtk_menu_item_activate(GTK_MENU_ITEM(s->zoom_in_item));
+}
+
 static void gd_menu_zoom_out(GtkMenuItem *item, void *opaque)
 {
 GtkDisplayState *s = opaque;
@@ -2092,6 +2099,8 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState *s)
  "/View/Zoom In");
 gtk_accel_map_add_entry("/View/Zoom In", GDK_KEY_plus,
 HOTKEY_MODIFIERS);
+gtk_accel_group_connect(s->accel_group, GDK_KEY_equal, HOTKEY_MODIFIERS, 0,
+g_cclosure_new_swap(G_CALLBACK(gd_accel_zoom_in), s, NULL));
 gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), s->zoom_in_item);
 
 s->zoom_out_item = gtk_menu_item_new_with_mnemonic(_("Zoom _Out"));
-- 
2.7.4

Re: [Qemu-devel] qemu-pcc 2.8.0 linux-user segfaults

[Sam Bobroff]

On Mon, Jan 16, 2017 at 04:03:21PM -0600, Aníbal Limón wrote:
> 
> 
> On 01/16/2017 03:56 PM, Aníbal Limón wrote:
> > Hi folks,
> > 
> > I'm trying to upgrade qemu to 2.8.0 in Openembedded-core and segfaults
> > in qemu-ppc when is executing:

Hi Aníbal,

I've recently encountered a similar problem and I've posted a fix. If
you'd like to try it, it's here:

https://lists.gnu.org/archive/html/qemu-ppc/2017-01/msg00413.html

Cheers,
Sam.

> > /home/alimon/repos/poky/build-ppc/tmp/work/x86_64-linux/qemu-native/2.8.0-r0/build/ppc-linux-user/qemu-ppc
> > -s 16M -r 3.2.0 -cpu 7400 -L
> > /home/alimon/repos/poky/build-ppc/tmp/sysroots/qemuppc -E
> > LD_LIBRARY_PATH=/home/alimon/repos/poky/build-ppc/tmp/work/ppc7400-poky-linux/gobject-introspection/1.50.0-r0/build/.libs:.libs:/home/alimon/repos/poky/build-ppc/tmp/sysroots/qemuppc//usr/lib:/home/alimon/repos/poky/build-ppc/tmp/sysroots/qemuppc//lib
> > /home/alimon/repos/poky/build-ppc/tmp/work/ppc7400-poky-linux/gobject-introspection/1.50.0-r0/build/tmp-introspectu_ewt_1z/Gio-2.0
> > --introspect-dump=/home/alimon/repos/poky/build-ppc/tmp/work/ppc7400-poky-linux/gobject-introspection/1.50.0-r0/build/tmp-introspectu_ewt_1z/functions.txt,/home/alimon/repos/poky/build-ppc/tmp/work/ppc7400-poky-linux/gobject-introspection/1.50.0-r0/build/tmp-introspectu_ewt_1z/dump.xml
> > [Thread debugging using libthread_db enabled]
> > 
> > 
> > And the debug info,
> > 
> > (gdb) info threads
> >   Id   Target Id Frame
> >   1Thread 0x77fd0780 (LWP 25457) "qemu-ppc"
> > pthread_cond_wait@@GLIBC_2.3.2 () at
> > ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
> >   2Thread 0x7647e700 (LWP 25461) "qemu-ppc" syscall () at
> > ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
> > * 3Thread 0x77f4d700 (LWP 25462) "qemu-ppc" 0x0086fba4
> > in static_code_gen_buffer ()
> > 
> > (gdb) bt
> > #0  0x0086fba4 in static_code_gen_buffer ()
> > #1  0x0040e922 in cpu_tb_exec (itb=,
> > itb=, cpu=0x29864d0) at
> > /home/alimon/repos/poky/build-ppc/tmp/work/x86_64-linux/qemu-native/2.8.0-r0/qemu-2.8.0/cpu-exec.c:164
> > #2  cpu_loop_exec_tb (sc=, tb_exit=,
> > last_tb=, tb=, cpu=0x29864d0) at
> > /home/alimon/repos/poky/build-ppc/tmp/work/x86_64-linux/qemu-native/2.8.0-r0/qemu-2.8.0/cpu-exec.c:544
> > #3  cpu_exec (cpu=cpu@entry=0x29864d0) at
> > /home/alimon/repos/poky/build-ppc/tmp/work/x86_64-linux/qemu-native/2.8.0-r0/qemu-2.8.0/cpu-exec.c:638
> > #4  0x00445fba in cpu_loop (env=env@entry=0x298e750) at
> > /home/alimon/repos/poky/build-ppc/tmp/work/x86_64-linux/qemu-native/2.8.0-r0/qemu-2.8.0/linux-user/main.c:1359
> > #5  0x00448a95 in clone_func (arg=0x7fffa910) at
> > /home/alimon/repos/poky/build-ppc/tmp/work/x86_64-linux/qemu-native/2.8.0-r0/qemu-2.8.0/linux-user/syscall.c:6090
> > #6  0x76a750a4 in start_thread (arg=0x77f4d700) at
> > pthread_create.c:309
> > #7  0x767aa62d in clone () at
> > ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> 
> Here is the core dump,
> 
> https://drive.google.com/file/d/0B9uDfO-FJ1kgY3ZhendISTZzOUU/view?usp=sharing
> 
> > 
> > Any help will be appreciated.
> > 
> > Best regards,
> > alimon
> > 
> 

[Qemu-devel] [Bug 1622547] Re: qemu-system-sparc fatal error Trap 0x29 on Solaris 2.6

[VIncent S. Cojot]

BTW, the patch posted in comment #9 works for me as well on qemu 2.8.0.
Thanks

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

Title:
  qemu-system-sparc fatal error Trap 0x29 on Solaris 2.6

Status in QEMU:
  New

Bug description:
  When trying to install Solaris 2.6 from original CDROM, qemu fail with
  the following error :

  qemu: fatal: Trap 0x29 while interrupts disabled, Error state
  pc: f0041280  npc: f0041284
  %g0-7:  f0281800 0800   f0243b88 0001 f0244020
  %o0-7: 40400ce2 40400ce2  404000e2 f0243b88  f023ffd8 
f0057914 
  %l0-7: 4cc2 f009645c f0096460 0002 0209 0004 0007 
f023ff90 
  %i0-7: 0042 404000e3  404000e3 e000 f028192a f0240038 
f0096448 
  %f00:     
  %f08:     
  %f16:     
  %f24:     
  psr: 40400cc2 (icc: -Z-- SPE: SP-) wim: 0002
  fsr:  y: 

  The command line was :

  qemu-system-sparc -nographic -bios ./openbios-sparc32 -M SS-20 -hda
  ./36G.disk -m 512 -cdrom Solaris_2.6_Software_05_98.img -boot d
  -serial telnet:0.0.0.0:3000,server -smp 2,cores=2 -monitor null

  It fails with a similar output when using bios ss20_v2.25_rom.

  ▶ qemu-system-sparc --version
  QEMU emulator version 2.7.0, Copyright (c) 2003-2016 Fabrice Bellard and the 
QEMU Project developers

  ▶ uname -a
  Linux xxx 4.7.1-1-ARCH #1 SMP PREEMPT Wed Aug 17 08:13:35 CEST 2016 x86_64 
GNU/Linux

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

Re: [Qemu-devel] [PATCH v2 02/16] softloat: disable floatx80_invalid_encoding() for m68k

[Andreas Schwab]

On Jan 30 2017, Peter Maydell  wrote:

> I guess we need to look more carefully at exactly what the
> m68k does for these encodings (maybe have a 'normalize value'
> function which squashes them down to whatever the equivalent
> non-weird encoding is?).

On the m68k, the concept of pseudo-inf/nan doesn't exist, because the
integer bit is don't-care for inf and nan.  The combination of a zero
integer bit with other non-zero biased exponents is called unnormal, and
the 68881/2 will always convert them to normal/denormal/zero on input.
(The 68040/60 don't support denormal/unnormal in hardware and lets the
support library handle them, which presumably works the same way as the
68881/2.)

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Qemu-devel] [Bug 1622547] Re: qemu-system-sparc fatal error Trap 0x29 on Solaris 2.6

[VIncent S. Cojot]

Hi everyone,
Thanks for your hard work on SPARC emulation. Almost feels like I got my old 
SS5 and SS20 back.
What commit (if any) might I find the fix for SS20 in?
Thanks,
Vincent

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

Title:
  qemu-system-sparc fatal error Trap 0x29 on Solaris 2.6

Status in QEMU:
  New

Bug description:
  When trying to install Solaris 2.6 from original CDROM, qemu fail with
  the following error :

  qemu: fatal: Trap 0x29 while interrupts disabled, Error state
  pc: f0041280  npc: f0041284
  %g0-7:  f0281800 0800   f0243b88 0001 f0244020
  %o0-7: 40400ce2 40400ce2  404000e2 f0243b88  f023ffd8 
f0057914 
  %l0-7: 4cc2 f009645c f0096460 0002 0209 0004 0007 
f023ff90 
  %i0-7: 0042 404000e3  404000e3 e000 f028192a f0240038 
f0096448 
  %f00:     
  %f08:     
  %f16:     
  %f24:     
  psr: 40400cc2 (icc: -Z-- SPE: SP-) wim: 0002
  fsr:  y: 

  The command line was :

  qemu-system-sparc -nographic -bios ./openbios-sparc32 -M SS-20 -hda
  ./36G.disk -m 512 -cdrom Solaris_2.6_Software_05_98.img -boot d
  -serial telnet:0.0.0.0:3000,server -smp 2,cores=2 -monitor null

  It fails with a similar output when using bios ss20_v2.25_rom.

  ▶ qemu-system-sparc --version
  QEMU emulator version 2.7.0, Copyright (c) 2003-2016 Fabrice Bellard and the 
QEMU Project developers

  ▶ uname -a
  Linux xxx 4.7.1-1-ARCH #1 SMP PREEMPT Wed Aug 17 08:13:35 CEST 2016 x86_64 
GNU/Linux

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

Re: [Qemu-devel] [PATCH 07/17] nbd: convert to use qio_channel_yield

[Paolo Bonzini]

On 30/01/2017 10:50, Stefan Hajnoczi wrote:
> On Fri, Jan 20, 2017 at 05:43:12PM +0100, Paolo Bonzini wrote:
>> +aio_co_wake(s->recv_coroutine[i]);
>>  
>> -qemu_coroutine_enter(nbd_get_client_session(bs)->send_coroutine);
>> +/* We're woken up by the recv_coroutine itself.  */
>> +qemu_coroutine_yield();
> 
> This relies on recv_coroutine() entering us only after we've yielded -
> otherwise QEMU will crash.  The code and comments don't make it obvious
> why this is guaranteed to be safe.

It doesn't rely on that (that's the magic hidden behind aio_co_wake),
but you're right there is a documentation problem because I needed 10
minutes to remind myself why it's correct.

It works because neither coroutine is moving context:

- if the two coroutines are in the same context, aio_co_wake queues the
coroutine for execution after the yield, which is obviously okay;

- if they are in different context, the recv_coroutine's aio_co_wake
queues the read_reply_co with aio_co_schedule.  It is then woken up
through a bottom half, which executes only after read_reply has yielded.

It is implied by the aio_co_wake and aio_co_schedule documentation; all
requirements are satisfied:

1) aio_co_wake's comment says:

   * aio_co_wake may be executed either in coroutine or non-coroutine
   * context.  The coroutine must not be entered by anyone else while
   * aio_co_wake() is active.

   read_reply_co is only woken by one recv_coroutine at a time

2) And for the case where read_reply_co and recv_coroutine are in
   different contexts, aio_co_schedule's comment says:

   * In addition the coroutine must have yielded unless ctx
   * is the context in which the coroutine is running (i.e. the value of
   * qemu_get_current_aio_context() from the coroutine itself).

   which is okay because aio_co_wake always uses "the context in
   which the coroutine is running" as the argument to aio_co_schedule.

Any suggestions on how to document this properly?  I'm not sure a
comment in the NBD driver is the best place, because similar logic will
appear soon in other networked block drivers.

Paolo

Re: [Qemu-devel] [PATCH] migrate: Migration aborts abruptly for machine "none"

[Paolo Bonzini]

On 29/01/2017 08:19, Ashijeet Acharya wrote:
> Also, Paolo has a similar opinion to yours that we should fix it
> instead of blocking it. He suggests to migrate only the device states
> and skip all the RAM related stuff. Maybe he can explain it better
> when he is available.

Yes, there is even support already in ram_migration_cleanup for not
allocating the bitmap:

struct BitmapRcu *bitmap = migration_bitmap_rcu;
atomic_rcu_set(&migration_bitmap_rcu, NULL);
if (bitmap) {
memory_global_dirty_log_stop();
call_rcu(bitmap, migration_bitmap_free, rcu);
}

I think you should just iterate until you find no more misbehaving code.
 The allocation is one, but ram_find_and_save_block should also return 0
if QLIST_FIRST_RCU(&ram_list.blocks) is NULL, for example.

Paolo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote:
> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:
> > On 01/27/17 15:18, Kevin O'Connor wrote:
> > > If an offset is going to be added, shouldn't both a source offset and
> > > destination offset be used?
> > > 
> > > /*
> > >  * COMMAND_WRITE_POINTER - update a writeable file named
> > >  * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> > >  * plus @pointer.src_offset to the blob originating from
> > >  * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> > >  * on @pointer.size.
> > >  */
> > > struct {
> > > char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > > char src_file[BIOS_LINKER_LOADER_FILESZ];
> > > uint32_t src_offset, dest_offset;
> > > uint8_t size;
> > > } pointer;
> > > 
> > > I doubt the offsets or size is really all that important though.
> > 
> > The offset into the fw_cfg file that receives the allocation address is
> > important, that allows the same file to receive several different
> > addresses (for different downloaded blobs), at different offsets.
> > 
> > OTOH, asking the firmware to add a constant to the address value before
> > writing it to the fw_cfg file is not necessary, in my opinion. The blob
> > that the firmware allocated and downloaded originates from QEMU to begin
> > with, so QEMU knows its internal structure.
> 
> I guess I'm missing why QEMU would want to use the same writable file
> for multiple pointers as well as why it would want support for
> pointers smaller than 8 bytes in size.  If it's because it may be
> easier to support an internal QEMU blob of a particular format, then
> adding a src_offset would facilitate that.
> 
> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> that's fine too.  I'm okay with either format.
> 
> -Kevin

Both reasons :) offset is because it's easier for QEMU not to have to add
more files (e.g. it simplifies cross-version migration if we don't).
size is to mimick ADD_POINTER.

-- 
MST

Re: [Qemu-devel] [PATCH 1/1] io: ignore case in WebSocket HTTP header #PSBM-57554

[Denis V. Lunev]

On 01/30/2017 06:47 PM, Daniel P. Berrange wrote:
> What is #PSBM-57554 referring to ?  Is that some custom bug tracker
> you have ? I'm going to drop that unless its something we need to
> keep
it must be dropped. Sorry, this is my mistake.

Den


> On Mon, Jan 30, 2017 at 04:19:56PM +0300, Denis V. Lunev wrote:
>> From: Anton Nefedov 
>>
>> According to RFC7230 Section 3.2, header field name is case-insensitive.
>>
>> The haystack string length is limited by 4096 bytes by
>> qio_channel_websock_handshake_read().
>>
>> Further, handshake_process() dups and NULL-terminates the string
>> so it is safe to call non length-limited functions like strcasestr().
>>
>> Signed-off-by: Anton Nefedov 
>> Signed-off-by: Denis V. Lunev 
>> CC: Daniel P. Berrange 
>> ---
>>  io/channel-websock.c | 25 ++---
>>  1 file changed, 14 insertions(+), 11 deletions(-)
> Reviewed-by: Daniel P. Berrange 
>
> will add this to my io queue
>
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH 01/17] aio: introduce aio_co_schedule and aio_co_wake

[Paolo Bonzini]

On 30/01/2017 10:18, Stefan Hajnoczi wrote:
> On Fri, Jan 20, 2017 at 05:43:06PM +0100, Paolo Bonzini wrote:
>> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
>> index 14d4f1d..1efa356 100644
>> --- a/include/qemu/coroutine_int.h
>> +++ b/include/qemu/coroutine_int.h
>> @@ -40,12 +40,20 @@ struct Coroutine {
>>  CoroutineEntry *entry;
>>  void *entry_arg;
>>  Coroutine *caller;
>> +
>> +/* Only used when the coroutine has terminated.  */
>>  QSLIST_ENTRY(Coroutine) pool_next;
>>  size_t locks_held;
> 
> locks_held is used when the coroutine is running.  Please add a newline
> to separate it from pool_next.
> 
>>  
>> -/* Coroutines that should be woken up when we yield or terminate */
>> +/* Coroutines that should be woken up when we yield or terminate.
>> + * Only used when the coroutine is running.
>> + */
>>  QSIMPLEQ_HEAD(, Coroutine) co_queue_wakeup;
>> +
>> +/* Only used when the coroutine is sleeping.  */
> 
> "running", "yielded", and "terminated" are terms with specific meanings.
> Is "sleeping" a subset of "yielded" where the coroutine is waiting for a
> mutex or its AioContext to schedule it?

No, I'll just change it to "has yielded".

>> +static void test_multi_co_schedule_entry(void *opaque)
> 
> Missing coroutine_fn.
> 
>> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
>> index d2f529b..cb87997 100644
>> --- a/tests/test-vmstate.c
>> +++ b/tests/test-vmstate.c
>> @@ -33,17 +33,6 @@
>>  static char temp_file[] = "/tmp/vmst.test.XX";
>>  static int temp_fd;
>>  
>> -/* Fake yield_until_fd_readable() implementation so we don't have to pull 
>> the
>> - * coroutine code as dependency.
>> - */
>> -void yield_until_fd_readable(int fd)
>> -{
>> -fd_set fds;
>> -FD_ZERO(&fds);
>> -FD_SET(fd, &fds);
>> -select(fd + 1, &fds, NULL, NULL, NULL);
>> -}
>> -
>>  
>>  /* Duplicate temp_fd and seek to the beginning of the file */
>>  static QEMUFile *open_test_file(bool write)
> 
> This should be a separate patch.

Yes, it probably doesn't have to be in this series either.

Paolo

Re: [Qemu-devel] [PATCH v2 28/41] char: move QIOChannel-related stuff to char-io.h

[Eric Blake]

On 01/30/2017 07:39 AM, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau 
> ---
>  chardev/char-io.h |  46 
>  chardev/char-io.c | 192 
> ++
>  chardev/char.c| 174 +
>  chardev/Makefile.objs |   1 +
>  4 files changed, 240 insertions(+), 173 deletions(-)
>  create mode 100644 chardev/char-io.h
>  create mode 100644 chardev/char-io.c
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 19/41] char: remove class kind field

[Paolo Bonzini]

On 30/01/2017 08:39, Marc-André Lureau wrote:
> The class kind is necessary to lookup the chardev name in
> qmp_chardev_add() after calling qemu_chr_new_from_opts() and to set
> the appropriate ChardevBackend (mainly to free the right
> fields).
> 
> qemu_chr_new_from_opts() can be changed to use a non-qmp function
> using the chardev class typename. Introduce qemu_chardev_add() to be
> called from qemu_chr_new_from_opts() and remove the class chardev kind
> field. Set the backend->type in the parse callback (when non-common
> fields are added).
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/sysemu/char.h |  1 -
>  backends/baum.c   |  1 -
>  backends/msmouse.c|  1 -
>  backends/testdev.c|  1 -
>  qemu-char.c   | 99 
> +--
>  spice-qemu-char.c |  4 +--
>  ui/console.c  |  2 +-
>  ui/gtk.c  |  1 -
>  8 files changed, 51 insertions(+), 59 deletions(-)

I am not sure about this patch.  Why not remove backend->type
altogether, and instead look at ChardevClass with object_dynamic_cast?

Paolo

Re: [Qemu-devel] [PATCH v2 25/41] char: move ringbuf/memory to its own file

[Eric Blake]

On 01/30/2017 07:39 AM, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau 
> ---
>  chardev/char-ringbuf.c | 249 
> +
>  chardev/char.c | 218 ---
>  chardev/Makefile.objs  |   1 +
>  3 files changed, 250 insertions(+), 218 deletions(-)
>  create mode 100644 chardev/char-ringbuf.c
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 24/41] char: move mux to its own file

[Eric Blake]

On 01/30/2017 07:39 AM, Marc-André Lureau wrote:
> A mechanical move, except that qemu_chr_write_all() needs to be declared
> in char.h header to be used from chardev unit files.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  chardev/char-mux.h|  63 +
>  include/sysemu/char.h |   3 +-
>  chardev/char-mux.c| 358 
> ++
>  chardev/char.c| 352 +
>  chardev/Makefile.objs |   1 +
>  5 files changed, 426 insertions(+), 351 deletions(-)
>  create mode 100644 chardev/char-mux.h
>  create mode 100644 chardev/char-mux.c
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 23/41] char: move null chardev to its own file

[Eric Blake]

On 01/30/2017 07:39 AM, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau 
> ---
>  chardev/char-null.c   | 54 
> +++
>  chardev/char.c| 23 --
>  chardev/Makefile.objs |  1 +
>  3 files changed, 55 insertions(+), 23 deletions(-)
>  create mode 100644 chardev/char-null.c
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 19/41] char: remove class kind field

[Eric Blake]

On 01/30/2017 07:39 AM, Marc-André Lureau wrote:
> The class kind is necessary to lookup the chardev name in
> qmp_chardev_add() after calling qemu_chr_new_from_opts() and to set
> the appropriate ChardevBackend (mainly to free the right
> fields).
> 
> qemu_chr_new_from_opts() can be changed to use a non-qmp function
> using the chardev class typename. Introduce qemu_chardev_add() to be
> called from qemu_chr_new_from_opts() and remove the class chardev kind
> field. Set the backend->type in the parse callback (when non-common
> fields are added).
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/sysemu/char.h |  1 -
>  backends/baum.c   |  1 -
>  backends/msmouse.c|  1 -
>  backends/testdev.c|  1 -
>  qemu-char.c   | 99 
> +--
>  spice-qemu-char.c |  4 +--
>  ui/console.c  |  2 +-
>  ui/gtk.c  |  1 -
>  8 files changed, 51 insertions(+), 59 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 17/41] char: get rid of CharDriver

[Eric Blake]

On 01/30/2017 07:39 AM, Marc-André Lureau wrote:
> qemu_chr_new_from_opts() is modified to not need CharDriver backend[]
> array, but uses instead objectified qmp_query_chardev_backends() and
> char_get_class(). The alias field is moved outside in a ChardevAlias[],
> similar to QDevAlias for devices.
> 
> "kind" and "parse" are moved to ChardevClass ("kind" is to be removed
> next)
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/sysemu/char.h |  12 +-
>  backends/baum.c   |   6 +-
>  backends/msmouse.c|   6 +-
>  backends/testdev.c|   6 +-
>  qemu-char.c   | 315 
> +++---
>  spice-qemu-char.c |  15 +--
>  ui/console.c  |  10 +-
>  ui/gtk.c  |  10 +-
>  8 files changed, 159 insertions(+), 221 deletions(-)
> 

> +static void
> +chardev_name_foreach(void (*fn)(const char *name, void *opaque), void 
> *opaque)
> +{
> +ChadevClassFE fe = { .fn = fn, .opaque = opaque };
> +int i;
> +
> +object_class_foreach(chardev_class_foreach, TYPE_CHARDEV, false, &fe);
> +
> +for (i = 0; i < ARRAY_SIZE(chardev_alias_table); i++) {
> +fn(chardev_alias_table[i].alias, opaque);
> +}

This now visits all aliases last,...

> @@ -4182,15 +4237,8 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>  
>  if (is_help_option(name)) {
>  GString *str = g_string_new("");
> -for (i = 0; i < ARRAY_SIZE(backends); i++) {
> -cd = backends[i];
> -if (cd) {
> -g_string_append_printf(str, "\n%s", 
> ChardevBackendKind_lookup[cd->kind]);
> -if (cd->alias) {
> -g_string_append_printf(str, "\n%s", cd->alias);
> -}
> -}
> -}

...while the old code visited them inline.  But that's cosmetic; I don't
think it hurts.

> +
> +chardev_name_foreach(help_string_append, str);

>  
>  ChardevBackendInfoList *qmp_query_chardev_backends(Error **errp)
>  {
>  ChardevBackendInfoList *backend_list = NULL;
> -const CharDriver *c;
> -int i;
>  
> -for (i = 0; i < ARRAY_SIZE(backends); i++) {
> -c = backends[i];
> -if (!c) {
> -continue;
> -}
> -
> -backend_list = qmp_prepend_backend(backend_list,
> -   
> ChardevBackendKind_lookup[c->kind]);
> -if (c->alias) {
> -backend_list = qmp_prepend_backend(backend_list, c->alias);
> -}
> -}
> +chardev_name_foreach(qmp_prepend_backend, &backend_list);

And the refactoring into visitor/callback made for some nice reuse.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 01/41] MAINTAINERS: add myself to qemu-char.c

[Paolo Bonzini]

On 30/01/2017 08:39, Marc-André Lureau wrote:
> I consider to have enough experience with qemu-char to propose myself as
> maintainer. This will allow me to send pull request without waiting for
> Paolo.

With pleasure. :)

Paolo

> Signed-off-by: Marc-André Lureau 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e0be7bc0d4..5efd8cc671 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1194,6 +1194,7 @@ T: git git://github.com/jnsnow/qemu.git bitmaps
>  
>  Character device backends
>  M: Paolo Bonzini 
> +M: Marc-André Lureau 
>  S: Maintained
>  F: qemu-char.c
>  F: backends/msmouse.c
> 

Re: [Qemu-devel] [PULL 23/35] x86: ioapic: dump version for "info ioapic"

[Paolo Bonzini]

On 30/01/2017 09:07, Peter Maydell wrote:
> On 20 January 2017 at 13:31, Paolo Bonzini  wrote:
>> From: Peter Xu 
>>
>> Signed-off-by: Peter Xu 
>> Message-Id: <1483952153-7221-3-git-send-email-pet...@redhat.com>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  hw/intc/ioapic_common.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
>> index 1b7ec5e..97c4f9c 100644
>> --- a/hw/intc/ioapic_common.c
>> +++ b/hw/intc/ioapic_common.c
>> @@ -58,7 +58,8 @@ void ioapic_print_redtbl(Monitor *mon, IOAPICCommonState 
>> *s)
>>  uint32_t remote_irr = 0;
>>  int i;
>>
>> -monitor_printf(mon, "ioapic id=0x%02x sel=0x%02x", s->id, s->ioregsel);
>> +monitor_printf(mon, "ioapic ver=0x%x id=0x%02x sel=0x%02x",
>> +   s->version, s->id, s->ioregsel);
>>  if (s->ioregsel) {
>>  monitor_printf(mon, " (redir[%u])\n",
>> (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1);
> 
> Coverity points out (CID 1369422) that this is a use of a possibly
> uninitialized field. In kvm_ioapic_dump_state() we do:
> 
> IOAPICCommonState s;
> kvm_ioapic_get(&s);
> ioapic_print_redtbl(mon, &s);
> 
> and kvm_ioapic_get() doesn't initialize s->version, so when we
> come to print it in ioapic_print_redtbl() it's uninitialized.
> 
> The easy fix is to initialize version to something. The
> underlying problem here I think is that we're manufacturing
> a fake IOAPICCommonState rather than finding the one that
> corresponds to the actual IOAPIC device in the system...

Right, we can probably use object_resolve_path to get one.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 06/15] postcopy: Record largest page size

[Juan Quintela]

"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)"  wrote:
>> > From: "Dr. David Alan Gilbert" 
>> >
>> > Record the largest page size in use; we'll need it soon for allocating
>> > temporary buffers.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert 
>> 
>> Not that I object, but  could we store this in ram_list, and update it
>> it at RAMBlock creation time?  Why searh for the value later when we can
>> store it from the beggining.  Instead of putting it on migration_state,
>> put it on the ram_list itself?
>> 
>
> We could, but the code does get quite a bit more complicated for little gain,
> given that we currently read it exactly once.
> The update at creation time would be easier, but then you have to also
> update at deletion time and that has to run along the list just like this.
> (or cache based on the ram_list.version)

As I am so lazy, I would only update it at creation time, and never at
deletion time.  But I can see that some people could object.  Anyways it
don't matter a lot, it is used only once, and on the other hand, we
don't care a lot if we have a bit bigger than needed buffer.  Just a
case of taste I guess.

Later, Juan.

Re: [Qemu-devel] [PATCH v2 01/16] softfloat: define 680x0 specific values

[Peter Maydell]

On 30 January 2017 at 18:16, Laurent Vivier  wrote:
> Signed-off-by: Laurent Vivier 
> +#elif defined(TARGET_M68K)
> +static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
> +   flag aIsLargerSignificand)
> +{
> +/* If either operand, but not both operands, of an operation is a
> + * nonsignaling NAN, then that NAN is returned as the result. If both
> + * operands are nonsignaling NANs, then the destination operand
> + * nonsignaling NAN is returned as the result.
> + */
> +
> +if (aIsSNaN) {
> +return 0;
> +} else if (bIsSNaN) {
> +return 1;
> +} else if (bIsQNaN) {
> +return 1;
> +} else {
> +return 0;
> +}
> +}

This function doesn't seem to quite do what the comment says (in
particular the comment doesn't say anything about handling
of signaling NaNs but the code makes decisions based on
which inputs are signaling).

PS: NaN is conventionally capitalized with a lowercase 'a'.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/15] postcopy: enhance ram_discard_range for hugepages

[Juan Quintela]

"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)"  wrote:
>> > From: "Dr. David Alan Gilbert" 
>> >
>> > Unfortunately madvise DONTNEED doesn't work on hugepagetlb
>> > so use fallocate(FALLOC_FL_PUNCH_HOLE)
>> > qemu_fd_getpagesize only sets the page based off a file
>> > if the file is from hugetlbfs.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert 

>> > +#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
>> > +#include 
>> > +#endif

I hate this in generic code :-()





I think that the function will have to be called:

qemu_ram_punch_hole(RAMBblock *, length);

Put it all together at the beggining of the file?
What I don't want is that if someone arrives with a way to do this in
other OS, we need to put yet more ifdefs.  I preffer very much that it
just have to define a function with that semantics.  Agreed that this is
a mess, but I can't think of an easier way of doing it either :-()



>> > @@ -1874,15 +1878,27 @@ int ram_discard_range(MigrationIncomingState *mis,
>> >  
>> >  if ((start + length) <= rb->used_length) {
>> >  uint8_t *host_endaddr = host_startaddr + length;
>> > -if ((uintptr_t)host_endaddr & (qemu_host_page_size - 1)) {
>> > +if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> >  error_report("ram_discard_range: Unaligned end address: %p",
>> >   host_endaddr);
>> >  goto err;
>> >  }
>> > -errno = ENOTSUP;
>> > +errno = ENOTSUP; /* If we are missing MADVISE etc */
>> > +
>> > +if (rb->page_size == qemu_host_page_size) {
>> >  #if defined(CONFIG_MADVISE)
>> > -ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
>> > +ret = qemu_madvise(host_startaddr, length, 
>> > QEMU_MADV_DONTNEED);
>> >  #endif
>> > +} else {
>> > +/* Huge page case  - unfortunately it can't do DONTNEED, but
>> > + * it can do the equivalent by FALLOC_FL_PUNCH_HOLE in the
>> > + * huge page file.
>> > + */
>> > +#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>> > +ret = fallocate(rb->fd, FALLOC_FL_PUNCH_HOLE | 
>> > FALLOC_FL_KEEP_SIZE,
>> > +start, length);
>> > +#endif
>> > +}
>> >  if (ret) {
>> >  error_report("ram_discard_range: Failed to discard  range "
>> >   "%s:%" PRIx64 " +%zx (%d)",
>> 
>> Can we move this to qemu-posix or similar?
>> qemu_punch_hole() or similar and just put all the magic there?
>
> I'm trying but it's tricky.
> The problem is that:
>a) To be able to tell which you need you need the pagesize of the
>  area
>b) Then you need the fd if it is a hugepage
>  (You can get (a) from (b) by a syscall we already do once)
>c) If it's normal RAM you need the memory address but...
>d) If it's a hugepage you need the offset in the file
>
>   which is a mess; so you either have to pass all those parameters,
> or end up passing a RAMBlock* which doesn't feel like it should
> make its way into any of the os-* files.
>
>   I could move it to exec.c that already has some ifdef on OSs;
> what do you think?
>
> Dave
>
>> For the rest, I am ok with it.
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 02/16] softloat: disable floatx80_invalid_encoding() for m68k

[Peter Maydell]

On 30 January 2017 at 18:16, Laurent Vivier  wrote:
> According to the comment, this definition of invalid encoding is given
> by intel developer's manual, and doesn't work with the behavior
> of 680x0 FPU.
>
> Signed-off-by: Laurent Vivier 

Part of the reason these checks are here is that the code
behind them doesn't always cope nicely with the invalid
encodings.
https://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01991.html
has an x86 test case for certain sqrt inputs that cause
the floatx80_sqrt() function to go into an infinite loop,
for instance.

I guess we need to look more carefully at exactly what the
m68k does for these encodings (maybe have a 'normalize value'
function which squashes them down to whatever the equivalent
non-weird encoding is?).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 6/6] qemu-img: copy *key-secret opts when opening newly created files

[Eric Blake]

On 01/26/2017 05:04 AM, Daniel P. Berrange wrote:
> The qemu-img dd/convert commands will create a image file and
> then try to open it. Historically it has been possible to open
> new files without passing any options. With encrypted files
> though, the *key-secret options are mandatory, so we need to
> provide those options when opening the newlky created file.

s/newlky/newly/

> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-img.c | 51 +++
>  1 file changed, 47 insertions(+), 4 deletions(-)
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 00/16] target-m68k: implement 680x0 FPU

[no-reply]

Hi,

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

Type: series
Subject: [Qemu-devel] [PATCH v2 00/16] target-m68k: implement 680x0 FPU
Message-id: 20170130181634.13934-1-laur...@vivier.eu

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170130181634.13934-1-laur...@vivier.eu -> 
patchew/20170130181634.13934-1-laur...@vivier.eu
Switched to a new branch 'test'
dbe6dbe target-m68k: add fsincos
ab31de0 target-m68k: add more FPU instructions
034d842 target-m68k: add explicit single and double precision operations
6c8776d target-m68k: add fsglmul and fsgldiv
541e299 target-m68k: add fscale, fgetman, fgetexp and fmod
9dfe1c1 target-m68k: add fmovecr
e028b28 target-m68k: add fscc.
aea92d1 target-m68k: add fmovem
5eb013e target-m68k: define 96bit FP registers for gdb on 680x0
9e4f1b4 target-m68k: manage FPU exceptions
baadbd9 target-m68k: add FPCR and FPSR
20f7380 target-m68k: use floatx80 internally
a47c018 target-m68k: define ext_opsize
0770d33 target-m68k: move FPU helpers to fpu_helper.c
4132de8 softloat: disable floatx80_invalid_encoding() for m68k
d714fec softfloat: define 680x0 specific values

=== OUTPUT BEGIN ===
Checking PATCH 1/16: softfloat: define 680x0 specific values...
Checking PATCH 2/16: softloat: disable floatx80_invalid_encoding() for m68k...
Checking PATCH 3/16: target-m68k: move FPU helpers to fpu_helper.c...
Checking PATCH 4/16: target-m68k: define ext_opsize...
Checking PATCH 5/16: target-m68k: use floatx80 internally...
ERROR: storage class should be at the beginning of the declaration
#457: FILE: target/m68k/translate.c:39:
+#define DEFF96(name, offset) static TCGv_i32 QREG_##name##H; \

total: 1 errors, 0 warnings, 1203 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 6/16: target-m68k: add FPCR and FPSR...
Checking PATCH 7/16: target-m68k: manage FPU exceptions...
Checking PATCH 8/16: target-m68k: define 96bit FP registers for gdb on 680x0...
Checking PATCH 9/16: target-m68k: add fmovem...
Checking PATCH 10/16: target-m68k: add fscc
Checking PATCH 11/16: target-m68k: add fmovecr...
Checking PATCH 12/16: target-m68k: add fscale, fgetman, fgetexp and fmod...
Checking PATCH 13/16: target-m68k: add fsglmul and fsgldiv...
Checking PATCH 14/16: target-m68k: add explicit single and double precision 
operations...
Checking PATCH 15/16: target-m68k: add more FPU instructions...
Checking PATCH 16/16: target-m68k: add fsincos...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCH 05/15] postcopy: enhance ram_discard_range for hugepages

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)"  wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> > Unfortunately madvise DONTNEED doesn't work on hugepagetlb
> > so use fallocate(FALLOC_FL_PUNCH_HOLE)
> > qemu_fd_getpagesize only sets the page based off a file
> > if the file is from hugetlbfs.
> >
> > Signed-off-by: Dr. David Alan Gilbert 
> > ---
> >  migration/ram.c | 24 
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index fe32836..7afabcd 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -45,6 +45,10 @@
> >  #include "qemu/rcu_queue.h"
> >  #include "migration/colo.h"
> >  
> > +#if defined(CONFIG_FALLOCATE_PUNCH_HOLE)
> > +#include 
> > +#endif
> > +
> >  #ifdef DEBUG_MIGRATION_RAM
> >  #define DPRINTF(fmt, ...) \
> >  do { fprintf(stdout, "migration_ram: " fmt, ## __VA_ARGS__); } while 
> > (0)
> > @@ -1866,7 +1870,7 @@ int ram_discard_range(MigrationIncomingState *mis,
> >  
> >  uint8_t *host_startaddr = rb->host + start;
> >  
> > -if ((uintptr_t)host_startaddr & (qemu_host_page_size - 1)) {
> > +if ((uintptr_t)host_startaddr & (rb->page_size - 1)) {
> >  error_report("ram_discard_range: Unaligned start address: %p",
> >   host_startaddr);
> >  goto err;
> > @@ -1874,15 +1878,27 @@ int ram_discard_range(MigrationIncomingState *mis,
> >  
> >  if ((start + length) <= rb->used_length) {
> >  uint8_t *host_endaddr = host_startaddr + length;
> > -if ((uintptr_t)host_endaddr & (qemu_host_page_size - 1)) {
> > +if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
> >  error_report("ram_discard_range: Unaligned end address: %p",
> >   host_endaddr);
> >  goto err;
> >  }
> > -errno = ENOTSUP;
> > +errno = ENOTSUP; /* If we are missing MADVISE etc */
> > +
> > +if (rb->page_size == qemu_host_page_size) {
> >  #if defined(CONFIG_MADVISE)
> > -ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> > +ret = qemu_madvise(host_startaddr, length, QEMU_MADV_DONTNEED);
> >  #endif
> > +} else {
> > +/* Huge page case  - unfortunately it can't do DONTNEED, but
> > + * it can do the equivalent by FALLOC_FL_PUNCH_HOLE in the
> > + * huge page file.
> > + */
> > +#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> > +ret = fallocate(rb->fd, FALLOC_FL_PUNCH_HOLE | 
> > FALLOC_FL_KEEP_SIZE,
> > +start, length);
> > +#endif
> > +}
> >  if (ret) {
> >  error_report("ram_discard_range: Failed to discard  range "
> >   "%s:%" PRIx64 " +%zx (%d)",
> 
> Can we move this to qemu-posix or similar?
> qemu_punch_hole() or similar and just put all the magic there?

I'm trying but it's tricky.
The problem is that:
   a) To be able to tell which you need you need the pagesize of the
 area
   b) Then you need the fd if it is a hugepage
 (You can get (a) from (b) by a syscall we already do once)
   c) If it's normal RAM you need the memory address but...
   d) If it's a hugepage you need the offset in the file

  which is a mess; so you either have to pass all those parameters,
or end up passing a RAMBlock* which doesn't feel like it should
make its way into any of the os-* files.

  I could move it to exec.c that already has some ifdef on OSs;
what do you think?

Dave

> For the rest, I am ok with it.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 00/25] qmp: add async command type

[Marc-André Lureau]

Hi

- Original Message -
> On Tue, Jan 24, 2017 at 01:43:17PM -0500, Marc-André Lureau wrote:
> > Hi
> > 
> > - Original Message -
> > > On Mon, Jan 23, 2017 at 06:27:29AM -0500, Marc-André Lureau wrote:
> > > > - Original Message -
> > > > > On Wed, Jan 18, 2017 at 08:03:07PM +0400, Marc-André Lureau wrote:
> > > > > > Hi,
> > > > > 
> > > > > CCing Jeff Cody and John Snow, who have been working on generalizing
> > > > > Block Job APIs to generic background jobs.  There is some overlap
> > > > > between async commands and background jobs.
> > > > 
> > > > If you say so :) Did I miss a proposal or a discussion for async qmp
> > > > commands?
> > > 
> > > There is no recent mailing list thread, so it's probably best to discuss
> > > here:
> > > 
> > > The goal of jobs is to support long-running operations that can be
> > > managed via QMP.  Jobs can have a more elaborate lifecycle than just
> > > start -> finish/cancel (e.g. they can be paused/resumed and may have
> > > multiple phases of execution that the client controls).  There are QMP
> > > APIs to query their state (Are they running?  How much "progress" has
> > > been made?).
> > 
> > Indeed, I mention that in my cover. Such use cases require something more
> > complete than simple async qmp commands. I don't see why it would be
> > incompatible with the usage of async qmp commands.
> > 
> > > A client reconnecting to QEMU can query running jobs.  This way a client
> > > can resume with a running QEMU process.  For commands like saving a
> > > screenshot is mostly does not matter, but for commands that modify state
> > > it's critical that clients are aware of running commands after reconnect
> > > to prevent corruption/interference.  This behavior is what I asked about
> > > in my previous mail.
> > 
> > That's what I mention in the cover, some commands are global (and
> > broadcasted events are appropriate) and some are local to the client
> > context. Some could be discarded when the client disconnects etc. It's a
> > case by case.
> > 
> > > Jobs are currently only used by the block layer and called "block jobs",
> > > but the idea is to generalize this.  They use synchronous QMP + events.
> > 
> > That pattern will have the flaws I mentioned (empty return, broadcast
> > events, id conflict, qapi semantic & documentation etc). Something new can
> > be invented, but it will likely make the protocol more complicated
> > compared to the solution I proposed (which is optional btw, and gracefully
> > fallbacks to sync processing for clients that do not support the async qmp
> > capability). However, I believe the job interface could be built on top of
> > what I propose.
> > 
> > > Jobs are more heavy-weight than async QMP commands, but pause/resume,
> > > rate-limiting, progress reporting, robust reconnect, etc are important
> > > features.  Users want to be aware of long-running operations and have
> > > the ability to control them.
> > 
> > You can't generalize such job interface to all async commands. Some may not
> > implement the ability to report progress, to cancel, to pause etc, etc. In
> > the end, it will be complicated and unneeded in many cases (what's the use
> > case to pause or to get the progress of a screendump?). What I propose is
> > simpler and compatible with job/task interfaces appropriate for various
> > domains.
> > 
> > > I suspect that if we transition synchronous QMP commands to async we'll
> > > soon have requirements for progress reporting, pause/resume, etc.  So is
> > > there a set of commands that should be async and others that should be
> > > jobs or should everything just be a job?
> > 
> > Hard to say without a concrete proposal of what "job" is. Likely,
> > everything is not going to be a "job".
> > 
> > But hopefully qmp-async and jobs can co-exist and benefit from each other.
> 
> My concern with this series is that background operations must be
> observable and there must be a way to cancel them.  Otherwise management
> tools cannot do their job and it's hard to troubleshoot a misbehaving
> system because you can't answer the question "what's going on?".  Once
> you add that then a large chunk of block jobs is duplicated.

Tracking ongoing operations can also be done at management layer. If needed, we 
could add qmp-commands to list on-going commands (their ids etc), and add 
commands to cancel them. But then again, not all operations will be 
cancellable, and I am not sure having requirements to list or cancel or modify 
all on-going operation is needed (I would say no, just like today you can't do 
anything while a command is running)

> 
> So then you look at block jobs and realize that no QMP wire protocol
> changes are necessary.  Despite the limitations you've listed, block
> jobs have been working successfully for years and don't require QMP
> clients to change.

Right, but we have existing bugs with functions that need to be async, at least 
at the qemu level. My proposal fixes t

Re: [Qemu-devel] [PATCH 1/2] nvme: implement the DSM command

[Keith Busch]

On Mon, Jan 30, 2017 at 07:13:51PM +0100, Christoph Hellwig wrote:
> Support deallocating of LBAs using the DSM command by wiring it up to
> the qemu discard implementation.  The other DSM operations which are
> purely advisory are ignored for now.
> 
> Based on an implementation by Keith Busch in the qemu-nvme.git repository,
> but rewritten to use the qemu AIO infrastructure properly to not block
> the main thread on discard requests, and cleaned up a little bit.
> 
> Signed-off-by: Christoph Hellwig 

Thanks for doing this. I've one comment below, but I don't think it should
block this unless you see a good way to fix it.

Reviewed-by: Keith Busch 

> +for (i = 0; i < nr; i++) {
> +uint64_t slba = le64_to_cpu(range[i].slba);
> +uint32_t nlb = le32_to_cpu(range[i].nlb);
> +
> +if (slba + nlb > le64_to_cpu(ns->id_ns.nsze)) {
> +return NVME_LBA_RANGE | NVME_DNR;
> +}
> +
> +req->aio_inflight++;
> +req->aiocb = blk_aio_pdiscard(n->conf.blk, slba << data_shift,
> +  nlb << data_shift, nvme_discard_cb, 
> req);

Overwriting the request's aiocb here for a multiple ranges is potentially
a problem if we have to cancel the request later. That scenario is
extremely unlikely, though, so I don't think it's worth addressing.

Re: [Qemu-devel] [PATCH v2 00/16] target-m68k: implement 680x0 FPU

[Andreas Schwab]

On Jan 30 2017, Laurent Vivier  wrote:

> The floatx80 datatype used here is not exactly the same as the
> one used by 680x0 for its extended precision data type, because
> normally the signaling bit of 680x0 NAN is the MSB of the mantissa
> minus one and in floatx80 it is the MSB.

An Intel Extended also has a different exponent bias than a Motorola
Extended in the denormal range.  In other words, for the latter the bias
doesn't change between normal and denormal numbers.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Qemu-devel] [PATCH v2 16/16] target-m68k: add fsincos

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/fpu_helper.c | 21 +
 target/m68k/helper.h |  1 +
 target/m68k/translate.c  | 15 +++
 3 files changed, 37 insertions(+)

diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 95d5cc4..9b9d9aa 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -95,6 +95,12 @@ static floatx80 FP1_to_floatx80(CPUM68KState *env)
 return (floatx80){ .low = env->fp1l, .high = env->fp1h };
 }
 
+static void floatx80_to_FP1(CPUM68KState *env, floatx80 res)
+{
+env->fp1l = res.low;
+env->fp1h = res.high;
+}
+
 void HELPER(exts32_FP0)(CPUM68KState *env)
 {
 floatx80 res;
@@ -949,3 +955,18 @@ void HELPER(cos_FP0)(CPUM68KState *env)
 res = ldouble_to_floatx80(val);
 floatx80_to_FP0(env, res);
 }
+
+void HELPER(sincos_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+long double val, valsin, valcos;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+
+sincosl(val, &valsin, &valcos);
+res = ldouble_to_floatx80(valsin);
+floatx80_to_FP0(env, res);
+res = ldouble_to_floatx80(valcos);
+floatx80_to_FP1(env, res);
+
+}
diff --git a/target/m68k/helper.h b/target/m68k/helper.h
index 600a9a6..16f3370 100644
--- a/target/m68k/helper.h
+++ b/target/m68k/helper.h
@@ -69,6 +69,7 @@ DEF_HELPER_1(log10_FP0, void, env)
 DEF_HELPER_1(cosh_FP0, void, env)
 DEF_HELPER_1(acos_FP0, void, env)
 DEF_HELPER_1(cos_FP0, void, env)
+DEF_HELPER_1(sincos_FP0_FP1, void, env)
 
 DEF_HELPER_3(mac_move, void, env, i32, i32)
 DEF_HELPER_3(macmulf, i64, env, i32, i32)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 4af6a98..30f8a6f 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -923,6 +923,14 @@ static void gen_op_load_fpr_FP1(int freg)
offsetof(CPUM68KState, fregs[freg].l.lower));
 }
 
+static void gen_op_store_fpr_FP1(int freg)
+{
+tcg_gen_st16_i32(QREG_FP1H, cpu_env,
+ offsetof(CPUM68KState, fregs[freg].l.upper));
+tcg_gen_st_i64(QREG_FP1L, cpu_env,
+   offsetof(CPUM68KState, fregs[freg].l.lower));
+}
+
 static void gen_extend_FP0(int opsize)
 {
 switch (opsize) {
@@ -4773,6 +4781,13 @@ DISAS_INSN(fpu)
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_dsub_FP0_FP1(cpu_env);
 break;
+case 0x30: case 0x31: case 0x32:
+case 0x33: case 0x34: case 0x35:
+case 0x36: case 0x37:
+gen_helper_sincos_FP0_FP1(cpu_env);
+gen_op_store_fpr_FP0(REG(ext, 7)); /* sin */
+gen_op_store_fpr_FP1(REG(ext, 0)); /* cos */
+break;
 case 0x38: /* fcmp */
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_cmp_FP0_FP1(cpu_env);
-- 
2.9.3

[Qemu-devel] [PATCH v2 05/16] target-m68k: use floatx80 internally

[Laurent Vivier]

Coldfire uses float64, but 680x0 use floatx80.
This patch introduces the use of floatx80 internally
and enables 680x0 80bits FPU.

Signed-off-by: Laurent Vivier 
---
 target/m68k/cpu.c|  13 +-
 target/m68k/cpu.h|  10 +-
 target/m68k/fpu_helper.c | 202 +---
 target/m68k/helper.c |  12 +-
 target/m68k/helper.h |  32 +--
 target/m68k/qregs.def|   3 +-
 target/m68k/translate.c  | 615 +++
 7 files changed, 615 insertions(+), 272 deletions(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index fa10b6e..cedc272 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -49,6 +49,8 @@ static void m68k_cpu_reset(CPUState *s)
 M68kCPU *cpu = M68K_CPU(s);
 M68kCPUClass *mcc = M68K_CPU_GET_CLASS(cpu);
 CPUM68KState *env = &cpu->env;
+floatx80 nan = floatx80_default_nan(NULL);
+int i;
 
 mcc->parent_reset(s);
 
@@ -57,7 +59,16 @@ static void m68k_cpu_reset(CPUState *s)
 env->sr = 0x2700;
 #endif
 m68k_switch_sp(env);
-/* ??? FP regs should be initialized to NaN.  */
+for (i = 0; i < 8; i++) {
+env->fregs[i].d = nan;
+}
+env->fp0h = nan.high;
+env->fp0l = nan.low;
+env->fp1h = nan.high;
+env->fp1l = nan.low;
+env->fpcr = 0;
+env->fpsr = 0;
+
 cpu_m68k_set_ccr(env, 0);
 /* TODO: We should set PC from the interrupt vector.  */
 env->pc = 0;
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 8095822..192a877 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -64,6 +64,8 @@
 #define NB_MMU_MODES 2
 #define TARGET_INSN_START_EXTRA_WORDS 1
 
+typedef CPU_LDoubleU FPReg;
+
 typedef struct CPUM68KState {
 uint32_t dregs[8];
 uint32_t aregs[8];
@@ -82,12 +84,16 @@ typedef struct CPUM68KState {
 uint32_t cc_c; /* either 0/1, unused, or computed from cc_n and cc_v */
 uint32_t cc_z; /* == 0 or unused */
 
-float64 fregs[8];
-float64 fp_result;
+FPReg fregs[8];
 uint32_t fpcr;
 uint32_t fpsr;
 float_status fp_status;
 
+uint32_t fp0h;
+uint64_t fp0l;
+uint32_t fp1h;
+uint64_t fp1l;
+
 uint64_t mactmp;
 /* EMAC Hardware deals with 48-bit values composed of one 32-bit and
two 8-bit parts.  We store a single 64-bit value and
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 5bf2576..9260e7b 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -3,6 +3,7 @@
  *
  *  Copyright (c) 2006-2007 CodeSourcery
  *  Written by Paul Brook
+ *  Copyright (c) 2011-2016 Laurent Vivier
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -21,92 +22,215 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
+#include "exec/exec-all.h"
 
-uint32_t HELPER(f64_to_i32)(CPUM68KState *env, float64 val)
+static floatx80 FP0_to_floatx80(CPUM68KState *env)
 {
-return float64_to_int32(val, &env->fp_status);
+return (floatx80){ .low = env->fp0l, .high = env->fp0h };
 }
 
-float32 HELPER(f64_to_f32)(CPUM68KState *env, float64 val)
+static void floatx80_to_FP0(CPUM68KState *env, floatx80 res)
 {
-return float64_to_float32(val, &env->fp_status);
+env->fp0l = res.low;
+env->fp0h = res.high;
 }
 
-float64 HELPER(i32_to_f64)(CPUM68KState *env, uint32_t val)
+static int32_t FP0_to_int32(CPUM68KState *env)
 {
-return int32_to_float64(val, &env->fp_status);
+return env->fp0h;
 }
 
-float64 HELPER(f32_to_f64)(CPUM68KState *env, float32 val)
+static void int32_to_FP0(CPUM68KState *env, int32_t val)
 {
-return float32_to_float64(val, &env->fp_status);
+env->fp0h = val;
 }
 
-float64 HELPER(iround_f64)(CPUM68KState *env, float64 val)
+static float32 FP0_to_float32(CPUM68KState *env)
 {
-return float64_round_to_int(val, &env->fp_status);
+return *(float32 *)&env->fp0h;
 }
 
-float64 HELPER(itrunc_f64)(CPUM68KState *env, float64 val)
+static void float32_to_FP0(CPUM68KState *env, float32 val)
 {
-return float64_trunc_to_int(val, &env->fp_status);
+env->fp0h = *(uint32_t *)&val;
 }
 
-float64 HELPER(sqrt_f64)(CPUM68KState *env, float64 val)
+static float64 FP0_to_float64(CPUM68KState *env)
 {
-return float64_sqrt(val, &env->fp_status);
+return *(float64 *)&env->fp0l;
+}
+static void float64_to_FP0(CPUM68KState *env, float64 val)
+{
+env->fp0l = *(uint64_t *)&val;
 }
 
-float64 HELPER(abs_f64)(float64 val)
+static floatx80 FP1_to_floatx80(CPUM68KState *env)
 {
-return float64_abs(val);
+return (floatx80){ .low = env->fp1l, .high = env->fp1h };
 }
 
-float64 HELPER(chs_f64)(float64 val)
+void HELPER(exts32_FP0)(CPUM68KState *env)
 {
-return float64_chs(val);
+floatx80 res;
+
+res = int32_to_floatx80(FP0_to_int32(env), &env->fp_status);
+
+floatx80_to_FP0(env, res);
 }
 
-float64 HELPER(add_f64)(CPUM68KState *env, float64 a, float64 b)
+void HELPER(extf32_FP0)(CPUM68KState *env)
 {
-return 

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Eric Blake]

On 01/26/2017 07:27 AM, Daniel P. Berrange wrote:
> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote:
>> On Thu, 01/26 11:04, Daniel P. Berrange wrote:
>>> The -n arg to the convert command allows use of a pre-existing image,
>>> rather than creating a new image. This adds a -n arg to the dd command
>>> to get feature parity.
>>
>> I remember there was a discussion about changing qemu-img dd's default to a
>> "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part
>> hasn't made it into the tree, and I'm not sure which direction we should 
>> take.
>> (Personally I think default to nocreat is a good idea).
> 
> Use nocreat by default would be semantically different from real "dd"
> binary which feels undesirable if the goal is to make "qemu-img dd"
> be as consistent with "dd" as possible.
> 
> It would be trivial to rewrite this patch to add support for the "conv"
> option, allowing the user to explicitly give 'qemu-img dd conv=nocreat'
> instead of my 'qemu-img dd -n' syntax, without changing default semantics.

Adding 'conv=nocreat' (and not '-n') feels like the right way to me.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 15/16] target-m68k: add more FPU instructions

[Laurent Vivier]

Add fsinh, flognp1, ftanh, fatan, fasin, fatanh,
fsin, ftan, fetox, ftwotox, ftentox, flogn, flog10, facos,
fcos.

As softfloat library does not provide these functions,
we us the libm of the host.

Signed-off-by: Laurent Vivier 
---
 target/m68k/fpu_helper.c | 237 +++
 target/m68k/helper.h |  16 
 target/m68k/translate.c  |  48 ++
 3 files changed, 301 insertions(+)

diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index c69efe1..95d5cc4 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -23,6 +23,7 @@
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
+#include 
 
 static const floatx80 fpu_rom[128] = {
 [0x00] = floatx80_pi,   /* Pi */
@@ -712,3 +713,239 @@ void HELPER(mod_FP0_FP1)(CPUM68KState *env)
 
 floatx80_to_FP0(env, res);
 }
+
+static long double floatx80_to_ldouble(floatx80 val)
+{
+if (floatx80_is_infinity(val)) {
+if (floatx80_is_neg(val)) {
+return -__builtin_infl();
+}
+return __builtin_infl();
+}
+if (floatx80_is_any_nan(val)) {
+char low[20];
+sprintf(low, "0x%016"PRIx64, val.low);
+
+return nanl(low);
+}
+
+return *(long double *)&val;
+}
+
+static floatx80 ldouble_to_floatx80(long double val)
+{
+floatx80 res;
+
+if (isinf(val)) {
+res.high = floatx80_default_nan(NULL).high;
+res.low = 0;
+}
+if (isinf(val) < 0) {
+res.high |= 0x8000;
+}
+if (isnan(val)) {
+res.high = floatx80_default_nan(NULL).high;
+res.low = *(uint64_t *)((char *)&val + 4);
+}
+return *(floatx80 *)&val;
+}
+
+void HELPER(sinh_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = sinhl(floatx80_to_ldouble(FP0_to_floatx80(env)));
+res = ldouble_to_floatx80(val);
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(lognp1_FP0)(CPUM68KState *env)
+{
+floatx80 val;
+long double res;
+
+val = FP0_to_floatx80(env);
+res = logl(floatx80_to_ldouble(val) + 1.0);
+
+floatx80_to_FP0(env, ldouble_to_floatx80(res));
+}
+
+void HELPER(ln_FP0)(CPUM68KState *env)
+{
+floatx80 val;
+long double res;
+
+val = FP0_to_floatx80(env);
+res = logl(floatx80_to_ldouble(val));
+
+floatx80_to_FP0(env, ldouble_to_floatx80(res));
+}
+
+void HELPER(log10_FP0)(CPUM68KState *env)
+{
+floatx80 val;
+long double res;
+
+val = FP0_to_floatx80(env);
+res = log10l(floatx80_to_ldouble(val));
+
+floatx80_to_FP0(env, ldouble_to_floatx80(res));
+}
+
+void HELPER(atan_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+
+val = atanl(val);
+res = ldouble_to_floatx80(val);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(asin_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+if (val < -1.0 || val > 1.0) {
+floatx80_to_FP0(env, floatx80_default_nan(NULL));
+return;
+}
+
+val = asinl(val);
+res = ldouble_to_floatx80(val);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(atanh_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+if (val < -1.0 || val > 1.0) {
+floatx80_to_FP0(env, floatx80_default_nan(NULL));
+return;
+}
+
+val = atanhl(val);
+res = ldouble_to_floatx80(val);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(sin_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+
+val = sinl(val);
+res = ldouble_to_floatx80(val);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(tanh_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+
+val = tanhl(val);
+res = ldouble_to_floatx80(val);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(tan_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+
+val = tanl(val);
+res = ldouble_to_floatx80(val);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(exp_FP0)(CPUM68KState *env)
+{
+floatx80 f;
+long double res;
+
+f = FP0_to_floatx80(env);
+
+res = expl(floatx80_to_ldouble(f));
+
+floatx80_to_FP0(env, ldouble_to_floatx80(res));
+}
+
+void HELPER(exp2_FP0)(CPUM68KState *env)
+{
+floatx80 f;
+long double res;
+
+f = FP0_to_floatx80(env);
+
+res = exp2l(floatx80_to_ldouble(f));
+
+floatx80_to_FP0(env, ldouble_to_floatx80(res));
+}
+
+void HELPER(exp10_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+long double val;
+
+val = floatx80_to_ldouble(FP0_to_floatx80(env));
+
+val = exp10l(val);
+res = ld

[Qemu-devel] [PATCH v2 13/16] target-m68k: add fsglmul and fsgldiv

[Laurent Vivier]

fsglmul and fsgldiv truncate data to single precision before computing
results.

Signed-off-by: Laurent Vivier 
---
 target/m68k/fpu_helper.c | 22 ++
 target/m68k/helper.h |  2 ++
 target/m68k/translate.c  |  8 
 3 files changed, 32 insertions(+)

diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 42f5b5c..8a3eed3 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -351,6 +351,17 @@ void HELPER(mul_FP0_FP1)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+void HELPER(sglmul_FP0_FP1)(CPUM68KState *env)
+{
+float64 a, b, res;
+
+a = floatx80_to_float64(FP0_to_floatx80(env), &env->fp_status);
+b = floatx80_to_float64(FP1_to_floatx80(env), &env->fp_status);
+res = float64_mul(a, b, &env->fp_status);
+
+floatx80_to_FP0(env, float64_to_floatx80(res, &env->fp_status));
+}
+
 void HELPER(div_FP0_FP1)(CPUM68KState *env)
 {
 floatx80 res;
@@ -361,6 +372,17 @@ void HELPER(div_FP0_FP1)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+void HELPER(sgldiv_FP0_FP1)(CPUM68KState *env)
+{
+float64 a, b, res;
+
+a = floatx80_to_float64(FP1_to_floatx80(env), &env->fp_status);
+b = floatx80_to_float64(FP0_to_floatx80(env), &env->fp_status);
+res = float64_div(a, b, &env->fp_status);
+
+floatx80_to_FP0(env, float64_to_floatx80(res, &env->fp_status));
+}
+
 static int float_comp_to_cc(int float_compare)
 {
 switch (float_compare) {
diff --git a/target/m68k/helper.h b/target/m68k/helper.h
index 620e707..c30e5f7 100644
--- a/target/m68k/helper.h
+++ b/target/m68k/helper.h
@@ -26,7 +26,9 @@ DEF_HELPER_1(chs_FP0, void, env)
 DEF_HELPER_1(add_FP0_FP1, void, env)
 DEF_HELPER_1(sub_FP0_FP1, void, env)
 DEF_HELPER_1(mul_FP0_FP1, void, env)
+DEF_HELPER_1(sglmul_FP0_FP1, void, env)
 DEF_HELPER_1(div_FP0_FP1, void, env)
+DEF_HELPER_1(sgldiv_FP0_FP1, void, env)
 DEF_HELPER_1(cmp_FP0_FP1, void, env)
 DEF_HELPER_2(set_fpcr, void, env, i32)
 DEF_HELPER_1(tst_FP0, void, env)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index d954d82..cfdc858 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4651,10 +4651,18 @@ DISAS_INSN(fpu)
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_mul_FP0_FP1(cpu_env);
 break;
+case 0x24: /* fsgldiv */
+gen_op_load_fpr_FP1(REG(ext, 7));
+gen_helper_sgldiv_FP0_FP1(cpu_env);
+break;
 case 0x26: /* fscale */
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_scale_FP0_FP1(cpu_env);
 break;
+case 0x27: /* fsglmul */
+gen_op_load_fpr_FP1(REG(ext, 7));
+gen_helper_sglmul_FP0_FP1(cpu_env);
+break;
 case 0x28: case 0x68: case 0x6c: /* fsub */
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_sub_FP0_FP1(cpu_env);
-- 
2.9.3

[Qemu-devel] [PATCH v2 10/16] target-m68k: add fscc.

[Laurent Vivier]

use DisasCompare with FPU conditions in fscc and fbcc.

Signed-off-by: Laurent Vivier 
---
 target/m68k/translate.c | 228 
 1 file changed, 153 insertions(+), 75 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 699c939..3fc744d 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4654,139 +4654,215 @@ undef:
 disas_undef_fpu(env, s, insn);
 }
 
-DISAS_INSN(fbcc)
+static void gen_fcc_cond(DisasCompare *c, DisasContext *s, int cond)
 {
-uint32_t offset;
-uint32_t addr;
-TCGLabel *l1;
-TCGv tmp;
-
-addr = s->pc;
-offset = cpu_ldsw_code(env, s->pc);
-s->pc += 2;
-if (insn & (1 << 6)) {
-offset = (offset << 16) | read_im16(env, s);
-}
-
-l1 = gen_new_label();
+c->g1 = 0;
+c->v2 = tcg_const_i32(0);
+c->g2 = 1;
 /* TODO: Raise BSUN exception.  */
-/* Jump to l1 if condition is true.  */
-switch (insn & 0x3f)  {
+switch (cond) {
 case 0:  /* False */
 case 16: /* Signaling False */
+c->v1 = c->v2;
+c->tcond = TCG_COND_NEVER;
 break;
 case 1:  /* EQual Z */
 case 17: /* Signaling EQual Z */
-tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_FPSR, FPSR_CC_Z);
-tcg_gen_brcondi_i32(TCG_COND_NE, tmp, 0, l1);
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_andi_i32(c->v1, QREG_FPSR, FPSR_CC_Z);
+c->tcond = TCG_COND_NE;
 break;
 case 2:  /* Ordered Greater Than !(A || Z || N) */
 case 18: /* Greater Than !(A || Z || N) */
-tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_FPSR,
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_andi_i32(c->v1, QREG_FPSR,
  FPSR_CC_A | FPSR_CC_Z | FPSR_CC_N);
-tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, l1);
+c->tcond = TCG_COND_EQ;
 break;
 case 3:  /* Ordered Greater than or Equal Z || !(A || N) */
 case 19: /* Greater than or Equal Z || !(A || N) */
 assert(FPSR_CC_A == (FPSR_CC_N >> 3));
-tmp = tcg_temp_new();
-tcg_gen_shli_i32(tmp, QREG_FPSR, 3);
-tcg_gen_or_i32(tmp, tmp, QREG_FPSR);
-tcg_gen_xori_i32(tmp, tmp, FPSR_CC_N);
-tcg_gen_andi_i32(tmp, tmp, FPSR_CC_N | FPSR_CC_Z);
-tcg_gen_brcondi_i32(TCG_COND_NE, tmp, 0, l1);
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_shli_i32(c->v1, QREG_FPSR, 3);
+tcg_gen_or_i32(c->v1, c->v1, QREG_FPSR);
+tcg_gen_xori_i32(c->v1, c->v1, FPSR_CC_N);
+tcg_gen_andi_i32(c->v1, c->v1, FPSR_CC_N | FPSR_CC_Z);
+c->tcond = TCG_COND_NE;
 break;
 case 4:  /* Ordered Less Than !(!N || A || Z); */
 case 20: /* Less Than !(!N || A || Z); */
-tmp = tcg_temp_new();
-tcg_gen_xori_i32(tmp, QREG_FPSR, FPSR_CC_N);
-tcg_gen_andi_i32(tmp, tmp, FPSR_CC_N | FPSR_CC_A | FPSR_CC_Z);
-tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, l1);
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_xori_i32(c->v1, QREG_FPSR, FPSR_CC_N);
+tcg_gen_andi_i32(c->v1, c->v1, FPSR_CC_N | FPSR_CC_A | FPSR_CC_Z);
+c->tcond = TCG_COND_EQ;
 break;
 case 5:  /* Ordered Less than or Equal Z || (N && !A) */
 case 21: /* Less than or Equal Z || (N && !A) */
 assert(FPSR_CC_A == (FPSR_CC_N >> 3));
-tmp = tcg_temp_new();
-tcg_gen_xori_i32(tmp, QREG_FPSR, FPSR_CC_A);
-tcg_gen_shli_i32(tmp, tmp, 3);
-tcg_gen_ori_i32(tmp, tmp, FPSR_CC_Z);
-tcg_gen_and_i32(tmp, tmp, QREG_FPSR);
-tcg_gen_brcondi_i32(TCG_COND_NE, tmp, 0, l1);
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_xori_i32(c->v1, QREG_FPSR, FPSR_CC_A);
+tcg_gen_shli_i32(c->v1, c->v1, 3);
+tcg_gen_ori_i32(c->v1, c->v1, FPSR_CC_Z);
+tcg_gen_and_i32(c->v1, c->v1, QREG_FPSR);
+c->tcond = TCG_COND_NE;
 break;
 case 6:  /* Ordered Greater or Less than !(A || Z) */
 case 22: /* Greater or Less than !(A || Z) */
-tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_FPSR, FPSR_CC_A | FPSR_CC_Z);
-tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, l1);
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_andi_i32(c->v1, QREG_FPSR, FPSR_CC_A | FPSR_CC_Z);
+c->tcond = TCG_COND_EQ;
 break;
 case 7:  /* Ordered !A */
 case 23: /* Greater, Less or Equal !A */
-tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_FPSR, FPSR_CC_A);
-tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, l1);
+c->v1 = tcg_temp_new();
+c->g1 = 1;
+tcg_gen_andi_i32(c->v1, QREG_FPSR, FPSR_CC_A);
+c->tcond = TCG_COND_EQ;
 break;
 case 8:  /* Unordered A */
 case 24: /* Not Greater, Less or Equal A */
-tmp = tcg_temp_new();
-tcg_gen_andi_i32(tmp, QREG_FPSR, FPSR_CC_A);
-

Re: [Qemu-devel] [PATCH] qcow2: Optimize the refcount-block overlap check

[Eric Blake]

On 01/30/2017 10:34 AM, Alberto Garcia wrote:

> 
> I don't think QEMU produces files where refcount_table[i] == 0 but
> refcount_table[i + 1] != 0. Do they even make sense?

I don't know if qemu can be directly coerced to create such an image,
but a third party tool can (and that probably includes qemu-io), and
such an image makes sense.  In particular, a refcount can be changed
from non-zero to zero during garbage collection when all clusters
covered by that refcount are no longer in use (perhaps because a
snapshot was deleted).

> In any case, my
> patch would cover those cases too, but this simplified version wouldn't.

Since an image can have holes in the refcount_table, it sounds like the
simplified version is not robust enough.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 03/16] target-m68k: move FPU helpers to fpu_helper.c

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/Makefile.objs |   2 +-
 target/m68k/fpu_helper.c  | 112 ++
 target/m68k/helper.c  |  88 
 3 files changed, 113 insertions(+), 89 deletions(-)
 create mode 100644 target/m68k/fpu_helper.c

diff --git a/target/m68k/Makefile.objs b/target/m68k/Makefile.objs
index 02cf616..39141ab 100644
--- a/target/m68k/Makefile.objs
+++ b/target/m68k/Makefile.objs
@@ -1,3 +1,3 @@
 obj-y += m68k-semi.o
-obj-y += translate.o op_helper.o helper.o cpu.o
+obj-y += translate.o op_helper.o helper.o cpu.o fpu_helper.o
 obj-y += gdbstub.o
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
new file mode 100644
index 000..5bf2576
--- /dev/null
+++ b/target/m68k/fpu_helper.c
@@ -0,0 +1,112 @@
+/*
+ *  m68k FPU helpers
+ *
+ *  Copyright (c) 2006-2007 CodeSourcery
+ *  Written by Paul Brook
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "exec/helper-proto.h"
+
+uint32_t HELPER(f64_to_i32)(CPUM68KState *env, float64 val)
+{
+return float64_to_int32(val, &env->fp_status);
+}
+
+float32 HELPER(f64_to_f32)(CPUM68KState *env, float64 val)
+{
+return float64_to_float32(val, &env->fp_status);
+}
+
+float64 HELPER(i32_to_f64)(CPUM68KState *env, uint32_t val)
+{
+return int32_to_float64(val, &env->fp_status);
+}
+
+float64 HELPER(f32_to_f64)(CPUM68KState *env, float32 val)
+{
+return float32_to_float64(val, &env->fp_status);
+}
+
+float64 HELPER(iround_f64)(CPUM68KState *env, float64 val)
+{
+return float64_round_to_int(val, &env->fp_status);
+}
+
+float64 HELPER(itrunc_f64)(CPUM68KState *env, float64 val)
+{
+return float64_trunc_to_int(val, &env->fp_status);
+}
+
+float64 HELPER(sqrt_f64)(CPUM68KState *env, float64 val)
+{
+return float64_sqrt(val, &env->fp_status);
+}
+
+float64 HELPER(abs_f64)(float64 val)
+{
+return float64_abs(val);
+}
+
+float64 HELPER(chs_f64)(float64 val)
+{
+return float64_chs(val);
+}
+
+float64 HELPER(add_f64)(CPUM68KState *env, float64 a, float64 b)
+{
+return float64_add(a, b, &env->fp_status);
+}
+
+float64 HELPER(sub_f64)(CPUM68KState *env, float64 a, float64 b)
+{
+return float64_sub(a, b, &env->fp_status);
+}
+
+float64 HELPER(mul_f64)(CPUM68KState *env, float64 a, float64 b)
+{
+return float64_mul(a, b, &env->fp_status);
+}
+
+float64 HELPER(div_f64)(CPUM68KState *env, float64 a, float64 b)
+{
+return float64_div(a, b, &env->fp_status);
+}
+
+float64 HELPER(sub_cmp_f64)(CPUM68KState *env, float64 a, float64 b)
+{
+/* ??? This may incorrectly raise exceptions.  */
+/* ??? Should flush denormals to zero.  */
+float64 res;
+res = float64_sub(a, b, &env->fp_status);
+if (float64_is_quiet_nan(res, &env->fp_status)) {
+/* +/-inf compares equal against itself, but sub returns nan.  */
+if (!float64_is_quiet_nan(a, &env->fp_status)
+&& !float64_is_quiet_nan(b, &env->fp_status)) {
+res = float64_zero;
+if (float64_lt_quiet(a, res, &env->fp_status)) {
+res = float64_chs(res);
+}
+}
+}
+return res;
+}
+
+uint32_t HELPER(compare_f64)(CPUM68KState *env, float64 val)
+{
+return float64_compare_quiet(val, float64_zero, &env->fp_status);
+}
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index f750d3d..5ca9911 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -284,94 +284,6 @@ void HELPER(set_sr)(CPUM68KState *env, uint32_t val)
 m68k_switch_sp(env);
 }
 
-/* FPU helpers.  */
-uint32_t HELPER(f64_to_i32)(CPUM68KState *env, float64 val)
-{
-return float64_to_int32(val, &env->fp_status);
-}
-
-float32 HELPER(f64_to_f32)(CPUM68KState *env, float64 val)
-{
-return float64_to_float32(val, &env->fp_status);
-}
-
-float64 HELPER(i32_to_f64)(CPUM68KState *env, uint32_t val)
-{
-return int32_to_float64(val, &env->fp_status);
-}
-
-float64 HELPER(f32_to_f64)(CPUM68KState *env, float32 val)
-{
-return float32_to_float64(val, &env->fp_status);
-}
-
-float64 HELPER(iround_f64)(CPUM68KState *env, float64 val)
-{
-return float64_round_to_int(val, &env->fp_status);
-}
-
-float64 HELPER(itrunc_f64)(CPUM68KState *env, float64 val)
-{
-return float64_trunc_to_int(val, &env->fp_status);
-}
-
-float64 HELP

[Qemu-devel] [PATCH v2 04/16] target-m68k: define ext_opsize

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/translate.c | 43 ---
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 9f60fbc..d9ba735 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -669,6 +669,21 @@ static inline int insn_opsize(int insn)
 }
 }
 
+static inline int ext_opsize(int ext, int pos)
+{
+switch ((ext >> pos) & 7) {
+case 0: return OS_LONG;
+case 1: return OS_SINGLE;
+case 2: return OS_EXTENDED;
+case 3: return OS_PACKED;
+case 4: return OS_WORD;
+case 5: return OS_DOUBLE;
+case 6: return OS_BYTE;
+default:
+g_assert_not_reached();
+}
+}
+
 /* Assign value to a register.  If the width is less than the register width
only the low part of the register is set.  */
 static void gen_partset_reg(int opsize, TCGv reg, TCGv val)
@@ -4101,20 +4116,19 @@ DISAS_INSN(fpu)
 tmp32 = tcg_temp_new_i32();
 /* fmove */
 /* ??? TODO: Proper behavior on overflow.  */
-switch ((ext >> 10) & 7) {
-case 0:
-opsize = OS_LONG;
+
+opsize = ext_opsize(ext, 10);
+switch (opsize) {
+case OS_LONG:
 gen_helper_f64_to_i32(tmp32, cpu_env, src);
 break;
-case 1:
-opsize = OS_SINGLE;
+case OS_SINGLE:
 gen_helper_f64_to_f32(tmp32, cpu_env, src);
 break;
-case 4:
-opsize = OS_WORD;
+case OS_WORD:
 gen_helper_f64_to_i32(tmp32, cpu_env, src);
 break;
-case 5: /* OS_DOUBLE */
+case OS_DOUBLE:
 tcg_gen_mov_i32(tmp32, AREG(insn, 0));
 switch ((insn >> 3) & 7) {
 case 2:
@@ -4143,8 +4157,7 @@ DISAS_INSN(fpu)
 }
 tcg_temp_free_i32(tmp32);
 return;
-case 6:
-opsize = OS_BYTE;
+case OS_BYTE:
 gen_helper_f64_to_i32(tmp32, cpu_env, src);
 break;
 default:
@@ -4217,15 +4230,7 @@ DISAS_INSN(fpu)
 }
 if (ext & (1 << 14)) {
 /* Source effective address.  */
-switch ((ext >> 10) & 7) {
-case 0: opsize = OS_LONG; break;
-case 1: opsize = OS_SINGLE; break;
-case 4: opsize = OS_WORD; break;
-case 5: opsize = OS_DOUBLE; break;
-case 6: opsize = OS_BYTE; break;
-default:
-goto undef;
-}
+opsize = ext_opsize(ext, 10);
 if (opsize == OS_DOUBLE) {
 tmp32 = tcg_temp_new_i32();
 tcg_gen_mov_i32(tmp32, AREG(insn, 0));
-- 
2.9.3

[Qemu-devel] [PATCH v2 11/16] target-m68k: add fmovecr

[Laurent Vivier]

fmovecr moves a floating point constant from the
FPU ROM to a floating point register.

Signed-off-by: Laurent Vivier 
---
 target/m68k/fpu_helper.c | 31 +++
 target/m68k/helper.h |  1 +
 target/m68k/translate.c  | 12 +++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index aadfc82..d8145e0 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -24,6 +24,31 @@
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 
+static const floatx80 fpu_rom[128] = {
+[0x00] = floatx80_pi,   /* Pi */
+[0x0b] = make_floatx80(0x3ffd, 0x9a209a84fbcff798ULL),  /* Log10(2) */
+[0x0c] = make_floatx80(0x4000, 0xadf85458a2bb4a9aULL),  /* e*/
+[0x0d] = make_floatx80(0x3fff, 0xb8aa3b295c17f0bcULL),  /* Log2(e)  */
+[0x0e] = make_floatx80(0x3ffd, 0xde5bd8a937287195ULL),  /* Log10(e) */
+[0x0f] = floatx80_zero, /* Zero */
+[0x30] = floatx80_ln2,  /* ln(2)*/
+[0x31] = make_floatx80(0x4000, 0x935d8dddaaa8ac17ULL),  /* ln(10)   */
+[0x32] = floatx80_one,  /* 10^0 */
+[0x33] = make_floatx80(0x4002, 0xa000ULL),  /* 10^1 */
+[0x34] = make_floatx80(0x4005, 0xc800ULL),  /* 10^2 */
+[0x35] = make_floatx80(0x400c, 0x9c40ULL),  /* 10^4 */
+[0x36] = make_floatx80(0x4019, 0xbebc2000ULL),  /* 10^8 */
+[0x37] = make_floatx80(0x4034, 0x8e1bc9bf0400ULL),  /* 10^16*/
+[0x38] = make_floatx80(0x4069, 0x9dc5ada82b70b59eULL),  /* 10^32*/
+[0x39] = make_floatx80(0x40d3, 0xc2781f49ffcfa6d5ULL),  /* 10^64*/
+[0x3a] = make_floatx80(0x41a8, 0x93ba47c980e98ce0ULL),  /* 10^128   */
+[0x3b] = make_floatx80(0x4351, 0xaa7eebfb9df9de8eULL),  /* 10^256   */
+[0x3c] = make_floatx80(0x46a3, 0xe319a0aea60e91c7ULL),  /* 10^512   */
+[0x3d] = make_floatx80(0x4d48, 0xc976758681750c17ULL),  /* 10^1024  */
+[0x3e] = make_floatx80(0x5a92, 0x9e8b3b5dc53d5de5ULL),  /* 10^2048  */
+[0x3f] = make_floatx80(0x7525, 0xc46052028a20979bULL),  /* 10^4096  */
+};
+
 static floatx80 FP0_to_floatx80(CPUM68KState *env)
 {
 return (floatx80){ .low = env->fp0l, .high = env->fp0h };
@@ -427,3 +452,9 @@ void HELPER(fmovem)(CPUM68KState *env, uint32_t opsize,
 {
 fprintf(stderr, "MISSING HELPER fmovem\n");
 }
+
+void HELPER(const_FP0)(CPUM68KState *env, uint32_t offset)
+{
+env->fp0l = fpu_rom[offset].low;
+env->fp0h = fpu_rom[offset].high;
+}
diff --git a/target/m68k/helper.h b/target/m68k/helper.h
index 58bc273..4927324 100644
--- a/target/m68k/helper.h
+++ b/target/m68k/helper.h
@@ -32,6 +32,7 @@ DEF_HELPER_2(set_fpcr, void, env, i32)
 DEF_HELPER_1(tst_FP0, void, env)
 DEF_HELPER_1(update_fpstatus, void, env)
 DEF_HELPER_4(fmovem, void, env, i32, i32, i32)
+DEF_HELPER_2(const_FP0, void, env, i32)
 
 DEF_HELPER_3(mac_move, void, env, i32, i32)
 DEF_HELPER_3(macmulf, i64, env, i32, i32)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 3fc744d..26160a3 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4555,16 +4555,26 @@ static void gen_op_fmovem(CPUM68KState *env, 
DisasContext *s,
 DISAS_INSN(fpu)
 {
 uint16_t ext;
+uint8_t rom_offset;
 int opmode;
 int opsize;
 
 ext = read_im16(env, s);
 opmode = ext & 0x7f;
 switch ((ext >> 13) & 7) {
-case 0: case 2:
+case 0:
 break;
 case 1:
 goto undef;
+case 2:
+if (insn == 0xf200 && (ext & 0xfc00) == 0x5c00) {
+/* fmovecr */
+rom_offset = ext & 0x7f;
+gen_helper_const_FP0(cpu_env, tcg_const_i32(rom_offset));
+gen_op_store_fpr_FP0(REG(ext, 7));
+return;
+}
+break;
 case 3: /* fmove out */
 gen_op_load_fpr_FP0(REG(ext, 7));
 opsize = ext_opsize(ext, 10);
-- 
2.9.3

[Qemu-devel] [PATCH v2 09/16] target-m68k: add fmovem

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/fpu_helper.c |  6 +++
 target/m68k/helper.h |  1 +
 target/m68k/translate.c  | 99 +++-
 3 files changed, 80 insertions(+), 26 deletions(-)

diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 1e68c41..aadfc82 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -421,3 +421,9 @@ void HELPER(update_fpstatus)(CPUM68KState *env)
 
 set_float_exception_flags(flags, &env->fp_status);
 }
+
+void HELPER(fmovem)(CPUM68KState *env, uint32_t opsize,
+uint32_t mode, uint32_t mask)
+{
+fprintf(stderr, "MISSING HELPER fmovem\n");
+}
diff --git a/target/m68k/helper.h b/target/m68k/helper.h
index 072a6d0..58bc273 100644
--- a/target/m68k/helper.h
+++ b/target/m68k/helper.h
@@ -31,6 +31,7 @@ DEF_HELPER_1(cmp_FP0_FP1, void, env)
 DEF_HELPER_2(set_fpcr, void, env, i32)
 DEF_HELPER_1(tst_FP0, void, env)
 DEF_HELPER_1(update_fpstatus, void, env)
+DEF_HELPER_4(fmovem, void, env, i32, i32, i32)
 
 DEF_HELPER_3(mac_move, void, env, i32, i32)
 DEF_HELPER_3(macmulf, i64, env, i32, i32)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 030773b..699c939 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4483,13 +4483,79 @@ static void gen_op_fmove_fcr(CPUM68KState *env, 
DisasContext *s,
 tcg_temp_free_i32(addr);
 }
 
+static void gen_op_fmovem(CPUM68KState *env, DisasContext *s,
+  uint32_t insn, uint32_t ext)
+{
+int opsize;
+uint16_t mask;
+int i;
+uint32_t mode;
+int32_t incr;
+TCGv addr, tmp;
+int is_load;
+
+if (m68k_feature(s->env, M68K_FEATURE_FPU)) {
+opsize = OS_EXTENDED;
+} else {
+opsize = OS_DOUBLE;  /* FIXME */
+}
+
+mode = (ext >> 11) & 0x3;
+if ((mode & 0x1) == 1) {
+gen_helper_fmovem(cpu_env, tcg_const_i32(opsize),
+  tcg_const_i32(mode), DREG(ext, 0));
+return;
+}
+
+tmp = gen_lea(env, s, insn, opsize);
+if (IS_NULL_QREG(tmp)) {
+gen_addr_fault(s);
+return;
+}
+
+addr = tcg_temp_new();
+tcg_gen_mov_i32(addr, tmp);
+is_load = ((ext & 0x2000) == 0);
+incr = opsize_bytes(opsize);
+mask = ext & 0x00FF;
+
+if (!is_load && (mode & 2) == 0) {
+for (i = 7; i >= 0; i--, mask <<= 1) {
+if (mask & 0x80) {
+gen_op_load_fpr_FP0(i);
+gen_store_FP0(s, opsize, addr);
+if ((mask & 0xff) != 0x80) {
+tcg_gen_subi_i32(addr, addr, incr);
+}
+}
+}
+tcg_gen_mov_i32(AREG(insn, 0), addr);
+} else {
+for (i = 0; i < 8; i++, mask <<= 1) {
+if (mask & 0x80) {
+if (is_load) {
+gen_load_FP0(s, opsize, addr);
+gen_op_store_fpr_FP0(i);
+} else {
+gen_op_load_fpr_FP0(i);
+gen_store_FP0(s, opsize, addr);
+}
+tcg_gen_addi_i32(addr, addr, incr);
+}
+}
+if ((insn & 070) == 030) {
+tcg_gen_mov_i32(AREG(insn, 0), addr);
+}
+}
+tcg_temp_free_i32(addr);
+}
+
 /* ??? FP exceptions are not implemented.  Most exceptions are deferred until
immediately before the next FP instruction is executed.  */
 DISAS_INSN(fpu)
 {
 uint16_t ext;
 int opmode;
-TCGv tmp32;
 int opsize;
 
 ext = read_im16(env, s);
@@ -4514,32 +4580,13 @@ DISAS_INSN(fpu)
 return;
 case 6: /* fmovem */
 case 7:
-{
-TCGv addr;
-uint16_t mask;
-int i;
-if ((ext & 0x1f00) != 0x1000 || (ext & 0xff) == 0)
-goto undef;
-tmp32 = gen_lea(env, s, insn, OS_LONG);
-if (IS_NULL_QREG(tmp32)) {
-gen_addr_fault(s);
-return;
-}
-addr = tcg_temp_new_i32();
-tcg_gen_mov_i32(addr, tmp32);
-mask = 0x80;
-for (i = 0; i < 8; i++) {
-if (ext & mask) {
-gen_op_load_fpr_FP0(REG(i, 0));
-gen_ldst_FP0(s, OS_DOUBLE, addr, (ext & (1 << 13)) ?
- EA_STORE : EA_LOADS);
-if (ext & (mask - 1))
-tcg_gen_addi_i32(addr, addr, 8);
-}
-mask >>= 1;
-}
-tcg_temp_free_i32(addr);
+if ((ext & 0xf00) != 0 || (ext & 0xff) == 0) {
+goto undef;
+}
+if ((ext & 0x1000) == 0 && !m68k_feature(s->env, M68K_FEATURE_FPU)) {
+goto undef;
 }
+gen_op_fmovem(env, s, insn, ext);
 return;
 }
 if (ext & (1 << 14)) {
-- 
2.9.3

[Qemu-devel] [PATCH v2 07/16] target-m68k: manage FPU exceptions

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/cpu.h|  28 +
 target/m68k/fpu_helper.c | 107 ++-
 target/m68k/helper.h |   1 +
 target/m68k/translate.c  |  27 
 4 files changed, 162 insertions(+), 1 deletion(-)

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 6b3cb26..7985dc3 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -57,6 +57,15 @@
 #define EXCP_TRAP15 47   /* User trap #15.  */
 #define EXCP_UNSUPPORTED61
 #define EXCP_ICE13
+#define EXCP_FP_BSUN48 /* Branch Set on Unordered */
+#define EXCP_FP_INEX49 /* Inexact result */
+#define EXCP_FP_DZ  50 /* Divide by Zero */
+#define EXCP_FP_UNFL51 /* Underflow */
+#define EXCP_FP_OPERR   52 /* Operand Error */
+#define EXCP_FP_OVFL53 /* Overflow */
+#define EXCP_FP_SNAN54 /* Signaling Not-A-Number */
+#define EXCP_FP_UNIMP   55 /* Unimplemented Data type */
+
 
 #define EXCP_RTE0x100
 #define EXCP_HALT_INSN  0x101
@@ -222,6 +231,25 @@ typedef enum {
 #define FPSR_CC_Z 0x0400 /* Zero */
 #define FPSR_CC_N 0x0800 /* Negative */
 
+/* Exception Status */
+#define FPSR_ES_MASK  0xff00
+#define FPSR_ES_BSUN  0x8000 /* Branch Set on Unordered */
+#define FPSR_ES_SNAN  0x4000 /* Signaling Not-A-Number */
+#define FPSR_ES_OPERR 0x2000 /* Operand Error */
+#define FPSR_ES_OVFL  0x1000 /* Overflow */
+#define FPSR_ES_UNFL  0x0800 /* Underflow */
+#define FPSR_ES_DZ0x0400 /* Divide by Zero */
+#define FPSR_ES_INEX2 0x0200 /* Inexact operation */
+#define FPSR_ES_INEX  0x0100 /* Inexact decimal input */
+
+/* Accrued Exception */
+#define FPSR_AE_MASK  0x00ff
+#define FPSR_AE_IOP   0x0080 /* Invalid Operation */
+#define FPSR_AE_OVFL  0x0040 /* Overflow */
+#define FPSR_AE_UNFL  0x0020 /* Underflow */
+#define FPSR_AE_DZ0x0010 /* Divide by Zero */
+#define FPSR_AE_INEX  0x0008 /* Inexact */
+
 /* Quotient */
 
 #define FPSR_QT_MASK  0x00ff
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 9d39118..1e68c41 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -177,6 +177,70 @@ static void restore_rounding_mode(CPUM68KState *env)
 }
 }
 
+static void set_fpsr_exception(CPUM68KState *env)
+{
+uint32_t fpsr = 0;
+int flags;
+
+flags = get_float_exception_flags(&env->fp_status);
+if (flags == 0) {
+return;
+}
+set_float_exception_flags(0, &env->fp_status);
+
+if (flags & float_flag_invalid) {
+fpsr |= FPSR_AE_IOP;
+}
+if (flags & float_flag_divbyzero) {
+fpsr |= FPSR_AE_DZ;
+}
+if (flags & float_flag_overflow) {
+fpsr |= FPSR_AE_OVFL;
+}
+if (flags & float_flag_underflow) {
+fpsr |= FPSR_AE_UNFL;
+}
+if (flags & float_flag_inexact) {
+fpsr |= FPSR_AE_INEX;
+}
+
+env->fpsr = (env->fpsr & ~FPSR_AE_MASK) | fpsr;
+}
+
+static void fpu_exception(CPUM68KState *env, uint32_t exception)
+{
+CPUState *cs = CPU(m68k_env_get_cpu(env));
+
+env->fpsr = (env->fpsr & ~FPSR_ES_MASK) | exception;
+if (env->fpcr & exception) {
+switch (exception) {
+case FPSR_ES_BSUN:
+cs->exception_index = EXCP_FP_BSUN;
+break;
+case FPSR_ES_SNAN:
+cs->exception_index = EXCP_FP_SNAN;
+break;
+case FPSR_ES_OPERR:
+cs->exception_index = EXCP_FP_OPERR;
+break;
+case FPSR_ES_OVFL:
+cs->exception_index = EXCP_FP_OVFL;
+break;
+case FPSR_ES_UNFL:
+cs->exception_index = EXCP_FP_UNFL;
+break;
+case FPSR_ES_DZ:
+cs->exception_index = EXCP_FP_DZ;
+break;
+case FPSR_ES_INEX:
+case FPSR_ES_INEX2:
+cs->exception_index = EXCP_FP_INEX;
+break;
+}
+cpu_loop_exit_restore(cs, GETPC());
+}
+}
+
 void cpu_m68k_set_fpcr(CPUM68KState *env, uint32_t val)
 {
 env->fpcr = val & 0x;
@@ -292,10 +356,16 @@ void HELPER(cmp_FP0_FP1)(CPUM68KState *env)
 {
 floatx80 fp0 = FP0_to_floatx80(env);
 floatx80 fp1 = FP1_to_floatx80(env);
-int float_compare;
+int flags, float_compare;
 
 float_compare = floatx80_compare(fp1, fp0, &env->fp_status);
 env->fpsr = (env->fpsr & ~FPSR_CC_MASK) | float_comp_to_cc(float_compare);
+
+flags = get_float_exception_flags(&env->fp_status);
+if (flags & float_flag_invalid) {
+fpu_exception(env, FPSR_ES_OPERR);
+   }
+   set_fpsr_exception(env);
 }
 
 void HELPER(tst_FP0)(CPUM68KState *env)
@@ -315,4 +385,39 @@ void HELPER(tst_FP0)(CPUM68KState *env)
 fpsr |= FPSR_CC_Z;
 }
 env->fpsr = (env->fpsr & ~FPSR_CC_MASK) | fpsr;
+
+set_fpsr_exception(env);
+}
+
+void HELPER(update_fpstatus)(CPUM68KState *env)
+{
+int flags = get_float_exception_fla

[Qemu-devel] [PATCH v2 14/16] target-m68k: add explicit single and double precision operations

[Laurent Vivier]

Add fssqrt, fdsqrt, fsabs, fdabs, fsneg, fdneg, fsadd, fdadd,
fssub, fdsub, fsmul, fdmul, fsdiv, fddiv, fsmove and fdmove.

The precision is managed using set_floatx80_rounding_precision(),
except for fsmove, fdmove, fsneg, fdneg, fsabs and fdabs:
the value is converted manually to the given precision and
converted back to floatx80.

Signed-off-by: Laurent Vivier 
---
 target/m68k/fpu_helper.c | 178 ++-
 target/m68k/helper.h |  16 -
 target/m68k/translate.c  |  76 +---
 3 files changed, 259 insertions(+), 11 deletions(-)

diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 8a3eed3..c69efe1 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -294,6 +294,16 @@ void HELPER(itrunc_FP0)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+#define PREC_BEGIN(prec)\
+do {\
+int old;\
+old = get_floatx80_rounding_precision(&env->fp_status); \
+set_floatx80_rounding_precision(prec, &env->fp_status)  \
+
+#define PREC_END()  \
+set_floatx80_rounding_precision(old, &env->fp_status);  \
+} while (0)
+
 void HELPER(sqrt_FP0)(CPUM68KState *env)
 {
 floatx80 res;
@@ -303,6 +313,28 @@ void HELPER(sqrt_FP0)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+void HELPER(ssqrt_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(32);
+res = floatx80_sqrt(FP0_to_floatx80(env), &env->fp_status);
+PREC_END();
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(dsqrt_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(64);
+res = floatx80_sqrt(FP0_to_floatx80(env), &env->fp_status);
+PREC_END();
+
+floatx80_to_FP0(env, res);
+}
+
 void HELPER(abs_FP0)(CPUM68KState *env)
 {
 floatx80 res;
@@ -312,11 +344,59 @@ void HELPER(abs_FP0)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
-void HELPER(chs_FP0)(CPUM68KState *env)
+void HELPER(sabs_FP0)(CPUM68KState *env)
 {
 floatx80 res;
+float32 f32;
+
+res = floatx80_abs(FP0_to_floatx80(env));
+f32 = floatx80_to_float32(res, &env->fp_status);
+res = float32_to_floatx80(f32, &env->fp_status);
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(dabs_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+float64 f64;
+
+res = floatx80_abs(FP0_to_floatx80(env));
+f64 = floatx80_to_float64(res, &env->fp_status);
+res = float64_to_floatx80(f64, &env->fp_status);
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(neg_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+
+res = floatx80_chs(FP0_to_floatx80(env));
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(sneg_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+float32 f32;
+
+res = floatx80_chs(FP0_to_floatx80(env));
+f32 = floatx80_to_float32(res, &env->fp_status);
+res = float32_to_floatx80(f32, &env->fp_status);
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(dneg_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+float64 f64;
 
 res = floatx80_chs(FP0_to_floatx80(env));
+f64 = floatx80_to_float64(res, &env->fp_status);
+res = float64_to_floatx80(f64, &env->fp_status);
 
 floatx80_to_FP0(env, res);
 }
@@ -331,6 +411,30 @@ void HELPER(add_FP0_FP1)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+void HELPER(sadd_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(32);
+res = floatx80_add(FP0_to_floatx80(env), FP1_to_floatx80(env),
+  &env->fp_status);
+PREC_END();
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(dadd_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(64);
+res = floatx80_add(FP0_to_floatx80(env), FP1_to_floatx80(env),
+  &env->fp_status);
+PREC_END();
+
+floatx80_to_FP0(env, res);
+}
+
 void HELPER(sub_FP0_FP1)(CPUM68KState *env)
 {
 floatx80 res;
@@ -341,6 +445,30 @@ void HELPER(sub_FP0_FP1)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+void HELPER(ssub_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(32);
+res = floatx80_sub(FP1_to_floatx80(env), FP0_to_floatx80(env),
+   &env->fp_status);
+PREC_END();
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(dsub_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(64);
+res = floatx80_sub(FP1_to_floatx80(env), FP0_to_floatx80(env),
+   &env->fp_status);
+PREC_END();
+
+floatx80_to_FP0(env, res);
+}
+
 void HELPER(mul_FP0_FP1)(CPUM68KState *env)
 {
 floatx80 res;
@@ -351,6 +479,30 @@ void HELPER(mul_FP0_FP1)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+void HELPER(smul_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+
+PREC_BEGIN(32);
+res = floatx80_mul(FP0_to_floatx80(env), FP1_to_

[Qemu-devel] [PATCH v2 02/16] softloat: disable floatx80_invalid_encoding() for m68k

[Laurent Vivier]

According to the comment, this definition of invalid encoding is given
by intel developer's manual, and doesn't work with the behavior
of 680x0 FPU.

Signed-off-by: Laurent Vivier 
---
 fpu/softfloat.c | 20 
 include/fpu/softfloat.h | 15 ---
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index c295f31..3aa05c1 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -4799,6 +4799,26 @@ int float64_unordered_quiet(float64 a, float64 b, 
float_status *status)
 }
 
 /*
+| Return whether the given value is an invalid floatx80 encoding.
+| Invalid floatx80 encodings arise when the integer bit is not set, but
+| the exponent is not zero. The only times the integer bit is permitted to
+| be zero is in subnormal numbers and the value zero.
+| This includes what the Intel software developer's manual calls pseudo-NaNs,
+| pseudo-infinities and un-normal numbers. It does not include
+| pseudo-denormals, which must still be correctly handled as inputs even
+| if they are never generated as outputs.
+**/
+static inline bool floatx80_invalid_encoding(floatx80 a)
+{
+#if defined(TARGET_M68K)
+return 0;
+#else
+return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
+#endif
+}
+
+
+/*
 | Returns the result of converting the extended double-precision floating-
 | point value `a' to the 32-bit two's complement integer format.  The
 | conversion is performed according to the IEC/IEEE Standard for Binary
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 14f8383..1bde349 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -658,21 +658,6 @@ static inline int floatx80_is_any_nan(floatx80 a)
 return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
 }
 
-/*
-| Return whether the given value is an invalid floatx80 encoding.
-| Invalid floatx80 encodings arise when the integer bit is not set, but
-| the exponent is not zero. The only times the integer bit is permitted to
-| be zero is in subnormal numbers and the value zero.
-| This includes what the Intel software developer's manual calls pseudo-NaNs,
-| pseudo-infinities and un-normal numbers. It does not include
-| pseudo-denormals, which must still be correctly handled as inputs even
-| if they are never generated as outputs.
-**/
-static inline bool floatx80_invalid_encoding(floatx80 a)
-{
-return (a.low & (1ULL << 63)) == 0 && (a.high & 0x7FFF) != 0;
-}
-
 #define floatx80_zero make_floatx80(0x, 0xLL)
 #define floatx80_one make_floatx80(0x3fff, 0x8000LL)
 #define floatx80_ln2 make_floatx80(0x3ffe, 0xb17217f7d1cf79acLL)
-- 
2.9.3

[Qemu-devel] [PATCH v2 06/16] target-m68k: add FPCR and FPSR

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/cpu.c|   2 +-
 target/m68k/cpu.h|  36 +-
 target/m68k/fpu_helper.c | 116 +++---
 target/m68k/helper.c |  20 ++-
 target/m68k/helper.h |   3 +-
 target/m68k/qregs.def|   1 +
 target/m68k/translate.c  | 311 +++
 7 files changed, 381 insertions(+), 108 deletions(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index cedc272..9553df4 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -66,7 +66,7 @@ static void m68k_cpu_reset(CPUState *s)
 env->fp0l = nan.low;
 env->fp1h = nan.high;
 env->fp1l = nan.low;
-env->fpcr = 0;
+cpu_m68k_set_fpcr(env, 0);
 env->fpsr = 0;
 
 cpu_m68k_set_ccr(env, 0);
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 192a877..6b3cb26 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -168,6 +168,7 @@ int cpu_m68k_signal_handler(int host_signum, void *pinfo,
void *puc);
 uint32_t cpu_m68k_get_ccr(CPUM68KState *env);
 void cpu_m68k_set_ccr(CPUM68KState *env, uint32_t);
+void cpu_m68k_set_fpcr(CPUM68KState *env, uint32_t val);
 
 
 /* Instead of computing the condition codes after each m68k instruction,
@@ -212,6 +213,36 @@ typedef enum {
 #define M68K_SSP0
 #define M68K_USP1
 
+/* Floating-Point Status Register */
+
+/* Condition Code */
+#define FPSR_CC_MASK  0x0f00
+#define FPSR_CC_A 0x0100 /* Not-A-Number */
+#define FPSR_CC_I 0x0200 /* Infinity */
+#define FPSR_CC_Z 0x0400 /* Zero */
+#define FPSR_CC_N 0x0800 /* Negative */
+
+/* Quotient */
+
+#define FPSR_QT_MASK  0x00ff
+
+/* Floating-Point Control Register */
+/* Rounding mode */
+#define FPCR_RND_MASK   0x0030
+#define FPCR_RND_N  0x
+#define FPCR_RND_Z  0x0010
+#define FPCR_RND_M  0x0020
+#define FPCR_RND_P  0x0030
+
+/* Rounding precision */
+#define FPCR_PREC_MASK  0x00c0
+#define FPCR_PREC_X 0x
+#define FPCR_PREC_S 0x0040
+#define FPCR_PREC_D 0x0080
+#define FPCR_PREC_U 0x00c0
+
+#define FPCR_EXCP_MASK 0xff00
+
 /* CACR fields are implementation defined, but some bits are common.  */
 #define M68K_CACR_EUSP  0x10
 
@@ -228,8 +259,6 @@ typedef enum {
 void m68k_set_irq_level(M68kCPU *cpu, int level, uint8_t vector);
 void m68k_switch_sp(CPUM68KState *env);
 
-#define M68K_FPCR_PREC (1 << 6)
-
 void do_m68k_semihosting(CPUM68KState *env, int nr);
 
 /* There are 4 ColdFire core ISA revisions: A, A+, B and C.
@@ -306,8 +335,7 @@ static inline void cpu_get_tb_cpu_state(CPUM68KState *env, 
target_ulong *pc,
 {
 *pc = env->pc;
 *cs_base = 0;
-*flags = (env->fpcr & M68K_FPCR_PREC)   /* Bit  6 */
-| (env->sr & SR_S)  /* Bit  13 */
+*flags = (env->sr & SR_S)   /* Bit  13 */
 | ((env->macsr >> 4) & 0xf);/* Bits 0-3 */
 }
 
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index 9260e7b..9d39118 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -132,11 +132,75 @@ void HELPER(iround_FP0)(CPUM68KState *env)
 floatx80_to_FP0(env, res);
 }
 
+static void m68k_restore_precision_mode(CPUM68KState *env)
+{
+switch (env->fpcr & FPCR_PREC_MASK) {
+case FPCR_PREC_X: /* extended */
+set_floatx80_rounding_precision(80, &env->fp_status);
+break;
+case FPCR_PREC_S: /* single */
+set_floatx80_rounding_precision(32, &env->fp_status);
+break;
+case FPCR_PREC_D: /* double */
+set_floatx80_rounding_precision(64, &env->fp_status);
+break;
+case FPCR_PREC_U: /* undefined */
+default:
+break;
+}
+}
+
+static void cf_restore_precision_mode(CPUM68KState *env)
+{
+if (env->fpcr & FPCR_PREC_S) { /* single */
+set_floatx80_rounding_precision(32, &env->fp_status);
+} else { /* double */
+set_floatx80_rounding_precision(64, &env->fp_status);
+}
+}
+
+static void restore_rounding_mode(CPUM68KState *env)
+{
+switch (env->fpcr & FPCR_RND_MASK) {
+case FPCR_RND_N: /* round to nearest */
+set_float_rounding_mode(float_round_nearest_even, &env->fp_status);
+break;
+case FPCR_RND_Z: /* round to zero */
+set_float_rounding_mode(float_round_to_zero, &env->fp_status);
+break;
+case FPCR_RND_M: /* round toward minus infinity */
+set_float_rounding_mode(float_round_down, &env->fp_status);
+break;
+case FPCR_RND_P: /* round toward positive infinity */
+set_float_rounding_mode(float_round_up, &env->fp_status);
+break;
+}
+}
+
+void cpu_m68k_set_fpcr(CPUM68KState *env, uint32_t val)
+{
+env->fpcr = val & 0x;
+
+if (m68k_feature(env, M68K_FEATURE_CF_FPU)) {
+cf_restore_precision_mode(env);
+} else {
+m68k_restore_precision_mode(env);
+}
+restore_rounding_mode(env);
+}
+
+void HELPER(set_fpcr)(CPUM68KState *env, uint32_t va

[Qemu-devel] [PATCH v2 01/16] softfloat: define 680x0 specific values

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 fpu/softfloat-specialize.h | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
index f05c865..01b594f 100644
--- a/fpu/softfloat-specialize.h
+++ b/fpu/softfloat-specialize.h
@@ -111,7 +111,7 @@ float16 float16_default_nan(float_status *status)
 **/
 float32 float32_default_nan(float_status *status)
 {
-#if defined(TARGET_SPARC)
+#if defined(TARGET_SPARC) || defined(TARGET_M68K)
 return const_float32(0x7FFF);
 #elif defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA) || \
   defined(TARGET_XTENSA) || defined(TARGET_S390X) || 
defined(TARGET_TRICORE)
@@ -136,7 +136,7 @@ float32 float32_default_nan(float_status *status)
 **/
 float64 float64_default_nan(float_status *status)
 {
-#if defined(TARGET_SPARC)
+#if defined(TARGET_SPARC) || defined(TARGET_M68K)
 return const_float64(LIT64(0x7FFF));
 #elif defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_ALPHA) || \
   defined(TARGET_S390X)
@@ -162,7 +162,10 @@ float64 float64_default_nan(float_status *status)
 floatx80 floatx80_default_nan(float_status *status)
 {
 floatx80 r;
-
+#if defined(TARGET_M68K)
+r.low = LIT64(0x);
+r.high = 0x7FFF;
+#else
 if (status->snan_bit_is_one) {
 r.low = LIT64(0xBFFF);
 r.high = 0x7FFF;
@@ -170,6 +173,7 @@ floatx80 floatx80_default_nan(float_status *status)
 r.low = LIT64(0xC000);
 r.high = 0x;
 }
+#endif
 return r;
 }
 
@@ -502,6 +506,26 @@ static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag 
bIsQNaN, flag bIsSNaN,
 return 1;
 }
 }
+#elif defined(TARGET_M68K)
+static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
+   flag aIsLargerSignificand)
+{
+/* If either operand, but not both operands, of an operation is a
+ * nonsignaling NAN, then that NAN is returned as the result. If both
+ * operands are nonsignaling NANs, then the destination operand
+ * nonsignaling NAN is returned as the result.
+ */
+
+if (aIsSNaN) {
+return 0;
+} else if (bIsSNaN) {
+return 1;
+} else if (bIsQNaN) {
+return 1;
+} else {
+return 0;
+}
+}
 #else
 static int pickNaN(flag aIsQNaN, flag aIsSNaN, flag bIsQNaN, flag bIsSNaN,
 flag aIsLargerSignificand)
-- 
2.9.3

[Qemu-devel] [PATCH v2 12/16] target-m68k: add fscale, fgetman, fgetexp and fmod

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 target/m68k/cpu.h|  1 +
 target/m68k/fpu_helper.c | 56 
 target/m68k/helper.h |  4 
 target/m68k/translate.c  | 14 
 4 files changed, 75 insertions(+)

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 7985dc3..3042ab7 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -253,6 +253,7 @@ typedef enum {
 /* Quotient */
 
 #define FPSR_QT_MASK  0x00ff
+#define FPSR_QT_SHIFT 16
 
 /* Floating-Point Control Register */
 /* Rounding mode */
diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
index d8145e0..42f5b5c 100644
--- a/target/m68k/fpu_helper.c
+++ b/target/m68k/fpu_helper.c
@@ -458,3 +458,59 @@ void HELPER(const_FP0)(CPUM68KState *env, uint32_t offset)
 env->fp0l = fpu_rom[offset].low;
 env->fp0h = fpu_rom[offset].high;
 }
+
+void HELPER(getexp_FP0)(CPUM68KState *env)
+{
+int32_t exp;
+floatx80 res;
+
+res = FP0_to_floatx80(env);
+if (floatx80_is_zero_or_denormal(res) || floatx80_is_any_nan(res) ||
+floatx80_is_infinity(res)) {
+return;
+}
+exp = (env->fp0h & 0x7fff) - 0x3fff;
+
+res = int32_to_floatx80(exp, &env->fp_status);
+
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(getman_FP0)(CPUM68KState *env)
+{
+floatx80 res;
+res = int64_to_floatx80(env->fp0l, &env->fp_status);
+floatx80_to_FP0(env, res);
+}
+
+void HELPER(scale_FP0_FP1)(CPUM68KState *env)
+{
+int32_t scale;
+int32_t exp;
+
+scale = floatx80_to_int32(FP0_to_floatx80(env), &env->fp_status);
+
+exp = (env->fp1h & 0x7fff) + scale;
+
+env->fp0h = (env->fp1h & 0x8000) | (exp & 0x7fff);
+env->fp0l = env->fp1l;
+}
+
+static void make_quotient(CPUM68KState *env, floatx80 val)
+{
+uint32_t quotient = floatx80_to_int32(val, &env->fp_status);
+uint32_t sign = (quotient >> 24) & 0x80;
+quotient = sign | (quotient & 0x7f);
+env->fpsr = (env->fpsr & ~FPSR_QT_MASK) | (quotient << FPSR_QT_SHIFT);
+}
+
+void HELPER(mod_FP0_FP1)(CPUM68KState *env)
+{
+floatx80 res;
+
+res = floatx80_rem(FP1_to_floatx80(env), FP0_to_floatx80(env),
+   &env->fp_status);
+make_quotient(env, res);
+
+floatx80_to_FP0(env, res);
+}
diff --git a/target/m68k/helper.h b/target/m68k/helper.h
index 4927324..620e707 100644
--- a/target/m68k/helper.h
+++ b/target/m68k/helper.h
@@ -33,6 +33,10 @@ DEF_HELPER_1(tst_FP0, void, env)
 DEF_HELPER_1(update_fpstatus, void, env)
 DEF_HELPER_4(fmovem, void, env, i32, i32, i32)
 DEF_HELPER_2(const_FP0, void, env, i32)
+DEF_HELPER_1(getexp_FP0, void, env)
+DEF_HELPER_1(getman_FP0, void, env)
+DEF_HELPER_1(scale_FP0_FP1, void, env)
+DEF_HELPER_1(mod_FP0_FP1, void, env)
 
 DEF_HELPER_3(mac_move, void, env, i32, i32)
 DEF_HELPER_3(macmulf, i64, env, i32, i32)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 26160a3..d954d82 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -4629,10 +4629,20 @@ DISAS_INSN(fpu)
 case 0x1a: case 0x5a: case 0x5e: /* fneg */
 gen_helper_chs_FP0(cpu_env);
 break;
+case 0x1e: /* fgetexp */
+gen_helper_getexp_FP0(cpu_env);
+break;
+case 0x1f: /* fgetman */
+gen_helper_getman_FP0(cpu_env);
+break;
 case 0x20: case 0x60: case 0x64: /* fdiv */
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_div_FP0_FP1(cpu_env);
 break;
+case 0x21: /* fmod */
+gen_op_load_fpr_FP1(REG(ext, 7));
+gen_helper_mod_FP0_FP1(cpu_env);
+break;
 case 0x22: case 0x62: case 0x66: /* fadd */
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_add_FP0_FP1(cpu_env);
@@ -4641,6 +4651,10 @@ DISAS_INSN(fpu)
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_mul_FP0_FP1(cpu_env);
 break;
+case 0x26: /* fscale */
+gen_op_load_fpr_FP1(REG(ext, 7));
+gen_helper_scale_FP0_FP1(cpu_env);
+break;
 case 0x28: case 0x68: case 0x6c: /* fsub */
 gen_op_load_fpr_FP1(REG(ext, 7));
 gen_helper_sub_FP0_FP1(cpu_env);
-- 
2.9.3

[Qemu-devel] [PATCH v2 00/16] target-m68k: implement 680x0 FPU

[Laurent Vivier]

This series modifies the original ColdFire FPU implementation
to use floatx80 instead of float64 internally as this
is the native datatype for 680x0. I didn't keep the float64
type for ColdFire, but if someone thinks it's required I
can update this series in this way.

The series also adds the FPU status and control registers and
several floating point instructions.

The floatx80 datatype used here is not exactly the same as the
one used by 680x0 for its extended precision data type, because
normally the signaling bit of 680x0 NAN is the MSB of the mantissa
minus one and in floatx80 it is the MSB.

We also add the gdb server parts to read the new FPU registers.
A strange thing happens here: while the gdb client running remotely
from a debian etch-m68k has no issue working with 96bit FPU registers
(the 680x0 extended precision data type), new gdbs (from a debian unstable
and gdb for cross-compiled environment) don't expect this FPU registers
size. But it seems like a bug in gdb, not in this implementation.

After this series is applied, qemu-m68k can run a debian etch-m68k
or a debian unstable chroot.

v2:
complete rework of the series
force single precision in ColdFire mode
add "forced" precision instructions (fsmove, fdmove, fsadd, ...)
fixed Fcc.

Laurent Vivier (16):
  softfloat: define 680x0 specific values
  softloat: disable floatx80_invalid_encoding() for m68k
  target-m68k: move FPU helpers to fpu_helper.c
  target-m68k: define ext_opsize
  target-m68k: use floatx80 internally
  target-m68k: add FPCR and FPSR
  target-m68k: manage FPU exceptions
  target-m68k: define 96bit FP registers for gdb on 680x0
  target-m68k: add fmovem
  target-m68k: add fscc.
  target-m68k: add fmovecr
  target-m68k: add fscale, fgetman, fgetexp and fmod
  target-m68k: add fsglmul and fsgldiv
  target-m68k: add explicit single and double precision operations
  target-m68k: add more FPU instructions
  target-m68k: add fsincos

 configure  |2 +-
 fpu/softfloat-specialize.h |   30 +-
 fpu/softfloat.c|   20 +
 gdb-xml/m68k-fp.xml|   21 +
 include/fpu/softfloat.h|   15 -
 target/m68k/Makefile.objs  |2 +-
 target/m68k/cpu.c  |   13 +-
 target/m68k/cpu.h  |   75 ++-
 target/m68k/fpu_helper.c   |  972 
 target/m68k/helper.c   |  165 +++---
 target/m68k/helper.h   |   73 ++-
 target/m68k/qregs.def  |4 +-
 target/m68k/translate.c| 1316 +---
 13 files changed, 2238 insertions(+), 470 deletions(-)
 create mode 100644 gdb-xml/m68k-fp.xml
 create mode 100644 target/m68k/fpu_helper.c

-- 
2.9.3

[Qemu-devel] [PATCH v2 08/16] target-m68k: define 96bit FP registers for gdb on 680x0

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
---
 configure|  2 +-
 gdb-xml/m68k-fp.xml  | 21 +
 target/m68k/helper.c | 45 +
 3 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100644 gdb-xml/m68k-fp.xml

diff --git a/configure b/configure
index 86fd833..4373ec5 100755
--- a/configure
+++ b/configure
@@ -5912,7 +5912,7 @@ case "$target_name" in
   ;;
   m68k)
 bflt="yes"
-gdb_xml_files="cf-core.xml cf-fp.xml"
+gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
   ;;
   microblaze|microblazeel)
 TARGET_ARCH=microblaze
diff --git a/gdb-xml/m68k-fp.xml b/gdb-xml/m68k-fp.xml
new file mode 100644
index 000..64290d1
--- /dev/null
+++ b/gdb-xml/m68k-fp.xml
@@ -0,0 +1,21 @@
+
+
+
+
+  
+  
+  
+  
+  
+  
+  
+  
+
+  
+  ,
+  
+
diff --git a/target/m68k/helper.c b/target/m68k/helper.c
index cff93dc..d93fad9 100644
--- a/target/m68k/helper.c
+++ b/target/m68k/helper.c
@@ -114,6 +114,48 @@ static int cf_fpu_gdb_set_reg(CPUM68KState *env, uint8_t 
*mem_buf, int n)
 return 0;
 }
 
+static int m68k_fpu_gdb_get_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
+{
+if (n < 8) {
+stw_be_p(mem_buf, env->fregs[n].l.upper);
+memset(mem_buf + 2, 0, 2);
+stq_be_p(mem_buf + 4, env->fregs[n].l.lower);
+return 12;
+}
+switch (n) {
+case 8: /* fpcontrol */
+stl_be_p(mem_buf, env->fpcr);
+return 4;
+case 9: /* fpstatus */
+stl_be_p(mem_buf, env->fpsr);
+return 4;
+case 10: /* fpiar, not implemented */
+memset(mem_buf, 0, 4);
+return 4;
+}
+return 0;
+}
+
+static int m68k_fpu_gdb_set_reg(CPUM68KState *env, uint8_t *mem_buf, int n)
+{
+if (n < 8) {
+env->fregs[n].l.upper = lduw_be_p(mem_buf);
+env->fregs[n].l.lower = ldq_be_p(mem_buf + 4);
+return 12;
+}
+switch (n) {
+case 8: /* fpcontrol */
+env->fpcr = ldl_p(mem_buf);
+return 4;
+case 9: /* fpstatus */
+env->fpsr = ldl_p(mem_buf);
+return 4;
+case 10: /* fpiar, not implemented */
+return 4;
+}
+return 0;
+}
+
 M68kCPU *cpu_m68k_init(const char *cpu_model)
 {
 M68kCPU *cpu;
@@ -142,6 +184,9 @@ void m68k_cpu_init_gdb(M68kCPU *cpu)
 if (m68k_feature(env, M68K_FEATURE_CF_FPU)) {
 gdb_register_coprocessor(cs, cf_fpu_gdb_get_reg, cf_fpu_gdb_set_reg,
  11, "cf-fp.xml", 18);
+} else if (m68k_feature(env, M68K_FEATURE_FPU)) {
+gdb_register_coprocessor(cs, m68k_fpu_gdb_get_reg,
+ m68k_fpu_gdb_set_reg, 11, "m68k-fp.xml", 18);
 }
 /* TODO: Add [E]MAC registers.  */
 }
-- 
2.9.3

[Qemu-devel] [PATCH 1/2] nvme: implement the DSM command

[Christoph Hellwig]

Support deallocating of LBAs using the DSM command by wiring it up to
the qemu discard implementation.  The other DSM operations which are
purely advisory are ignored for now.

Based on an implementation by Keith Busch in the qemu-nvme.git repository,
but rewritten to use the qemu AIO infrastructure properly to not block
the main thread on discard requests, and cleaned up a little bit.

Signed-off-by: Christoph Hellwig 
---
 hw/block/nvme.c | 87 +
 hw/block/nvme.h |  1 +
 2 files changed, 88 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d479fd2..d2f1d9a 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -227,6 +227,90 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeNamespace *ns, 
NvmeCmd *cmd,
 return NVME_NO_COMPLETE;
 }
 
+static void nvme_discard_cb(void *opaque, int ret)
+{
+NvmeRequest *req = opaque;
+NvmeSQueue *sq = req->sq;
+NvmeCtrl *n = sq->ctrl;
+NvmeCQueue *cq = n->cq[sq->cqid];
+
+if (ret) {
+req->status = NVME_INTERNAL_DEV_ERROR;
+}
+
+if (--req->aio_inflight > 0) {
+return;
+}
+
+nvme_enqueue_req_completion(cq, req);
+}
+
+
+static uint16_t nvme_dsm_discard(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
+NvmeRequest *req)
+{
+uint16_t nr = (le32_to_cpu(cmd->cdw10) & 0xff) + 1;
+uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
+uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds - BDRV_SECTOR_BITS;
+NvmeDsmRange *range;
+QEMUSGList qsg;
+int i;
+
+range = g_new(NvmeDsmRange, nr);
+
+if (nvme_map_prp(&qsg, le64_to_cpu(cmd->prp1), le64_to_cpu(cmd->prp2),
+sizeof(range), n)) {
+goto out_free_range;
+}
+
+if (dma_buf_write((uint8_t *)range, sizeof(range), &qsg)) {
+goto out_destroy_qsg;
+}
+
+qemu_sglist_destroy(&qsg);
+
+req->status = NVME_SUCCESS;
+req->has_sg = false;
+req->aio_inflight = 1;
+
+for (i = 0; i < nr; i++) {
+uint64_t slba = le64_to_cpu(range[i].slba);
+uint32_t nlb = le32_to_cpu(range[i].nlb);
+
+if (slba + nlb > le64_to_cpu(ns->id_ns.nsze)) {
+return NVME_LBA_RANGE | NVME_DNR;
+}
+
+req->aio_inflight++;
+req->aiocb = blk_aio_pdiscard(n->conf.blk, slba << data_shift,
+  nlb << data_shift, nvme_discard_cb, req);
+}
+
+g_free(range);
+
+if (--req->aio_inflight > 0) {
+return NVME_NO_COMPLETE;
+}
+
+return NVME_SUCCESS;
+
+out_destroy_qsg:
+qemu_sglist_destroy(&qsg);
+out_free_range:
+g_free(range);
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
+static uint16_t nvme_dsm(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
+NvmeRequest *req)
+{
+if (cmd->cdw11 & cpu_to_le32(NVME_DSMGMT_AD)) {
+return nvme_dsm_discard(n, ns, cmd, req);
+} else {
+return NVME_SUCCESS;
+}
+}
+
 static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
 NvmeRequest *req)
 {
@@ -279,6 +363,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, 
NvmeRequest *req)
 switch (cmd->opcode) {
 case NVME_CMD_FLUSH:
 return nvme_flush(n, ns, cmd, req);
+case NVME_CMD_DSM:
+return nvme_dsm(n, ns, cmd, req);
 case NVME_CMD_WRITE:
 case NVME_CMD_READ:
 return nvme_rw(n, ns, cmd, req);
@@ -889,6 +975,7 @@ static int nvme_init(PCIDevice *pci_dev)
 id->sqes = (0x6 << 4) | 0x6;
 id->cqes = (0x4 << 4) | 0x4;
 id->nn = cpu_to_le32(n->num_namespaces);
+id->oncs = cpu_to_le16(NVME_ONCS_DSM);
 id->psd[0].mp = cpu_to_le16(0x9c4);
 id->psd[0].enlat = cpu_to_le32(0x10);
 id->psd[0].exlat = cpu_to_le32(0x4);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 8fb0c10..e64a758 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -640,6 +640,7 @@ typedef struct NvmeRequest {
 BlockAIOCB  *aiocb;
 uint16_tstatus;
 boolhas_sg;
+uint32_taio_inflight;
 NvmeCqe cqe;
 BlockAcctCookie acct;
 QEMUSGList  qsg;
-- 
2.1.4

[Qemu-devel] [PATCH 0/2] two more NVMe commands

[Christoph Hellwig]

Hi all,

this series implements two more NVMe commands: DSM and Write Zeroes.
Both trace their lineage to Keith's qemu-nvme.git repository, and
while the Write Zeroes one is taken from there almost literally
the DSM one has seen a major rewrite to not block the main thread
as well as various other small improvements.

[Qemu-devel] [PATCH 2/2] nvme: Implement Write Zeroes

[Christoph Hellwig]

From: Keith Busch 

Signed-off-by: Keith Busch 
[hch: ported over from qemu-nvme.git to mainline]
Signed-off-by: Christoph Hellwig 
---
 hw/block/nvme.c | 27 ++-
 hw/block/nvme.h |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d2f1d9a..6dac9ce 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -311,6 +311,29 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeNamespace *ns, 
NvmeCmd *cmd,
 }
 }
 
+static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
+NvmeRequest *req)
+{
+NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
+const uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
+const uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
+uint64_t slba = le64_to_cpu(rw->slba);
+uint32_t nlb  = le16_to_cpu(rw->nlb) + 1;
+uint64_t aio_slba = slba << (data_shift - BDRV_SECTOR_BITS);
+uint32_t aio_nlb = nlb << (data_shift - BDRV_SECTOR_BITS);
+
+if (slba + nlb > ns->id_ns.nsze) {
+return NVME_LBA_RANGE | NVME_DNR;
+}
+
+req->has_sg = false;
+block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
+ BLOCK_ACCT_WRITE);
+req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, aio_slba, aio_nlb, 0,
+nvme_rw_cb, req);
+return NVME_NO_COMPLETE;
+}
+
 static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
 NvmeRequest *req)
 {
@@ -365,6 +388,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, 
NvmeRequest *req)
 return nvme_flush(n, ns, cmd, req);
 case NVME_CMD_DSM:
 return nvme_dsm(n, ns, cmd, req);
+case NVME_CMD_WRITE_ZEROS:
+return nvme_write_zeros(n, ns, cmd, req);
 case NVME_CMD_WRITE:
 case NVME_CMD_READ:
 return nvme_rw(n, ns, cmd, req);
@@ -975,7 +1000,7 @@ static int nvme_init(PCIDevice *pci_dev)
 id->sqes = (0x6 << 4) | 0x6;
 id->cqes = (0x4 << 4) | 0x4;
 id->nn = cpu_to_le32(n->num_namespaces);
-id->oncs = cpu_to_le16(NVME_ONCS_DSM);
+id->oncs = cpu_to_le16(NVME_ONCS_DSM | NVME_ONCS_WRITE_ZEROS);
 id->psd[0].mp = cpu_to_le16(0x9c4);
 id->psd[0].enlat = cpu_to_le32(0x10);
 id->psd[0].exlat = cpu_to_le32(0x4);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index e64a758..f3054b3 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -179,6 +179,7 @@ enum NvmeIoCommands {
 NVME_CMD_READ   = 0x02,
 NVME_CMD_WRITE_UNCOR= 0x04,
 NVME_CMD_COMPARE= 0x05,
+NVME_CMD_WRITE_ZEROS= 0x08,
 NVME_CMD_DSM= 0x09,
 };
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/1] mirror: restore offset after zeroing out the image

[Denis V. Lunev]

On 01/30/2017 08:16 PM, Eric Blake wrote:
> On 01/30/2017 04:22 AM, Denis V. Lunev wrote:
>> If explicit zeroing out before mirroring is required for the target image,
>> it moves the block job offset counter to EOF, then offset and len counters
>> count the image size twice.
>>
>> There is no harm but confusing stats (e.g. for 1G image the completion
>> counter starts from 1G and increases to 2G)
>>
>> The patch fixed that problem by resetting the offset counter.
> Counters are explicitly documented NOT tied to disk length; they are
> merely estimates of proportional completion.  I'm not sure if this makes
> the numbers jump backwards from the observer's viewpoint, but if you can
> ever spot 1g/1g right before rewinding to 0g/1g (where pre-patch could
> see 1g/2g), then that is a reason to not take this patch.  On the other
> hand, your argument that the pre-patch behavior progressing towards 2g
> has a very fast progression from 0-1g/2g, and then a much slower
> 1g-2g/2g, which makes the estimate of percent completion skewed, while a
> newer progression of 0-1g/1g is more realistic, may have some merit.
>
> I'm not sold on this patch yet, but stronger arguments in the commit
> message may sway me.
>
>> +++ b/tests/qemu-iotests/094.out
>> @@ -3,9 +3,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>  Formatting 'TEST_DIR/source.IMGFMT', fmt=IMGFMT size=67108864
>>  {"return": {}}
>>  {"return": {}}
>> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
>> "BLOCK_JOB_READY", "data": {"device": "src", "len": 67108864, "offset": 
>> 67108864, "speed": 0, "type": "mirror"}}
>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
>> "BLOCK_JOB_READY", "data": {"device": "src", "len": 0, "offset": 0, "speed": 
>> 0, "type": "mirror"}}
>>  {"return": {}}
> This part of the change is scary - a ready event showing 0/0 HAS been
> known to confuse libvirt in the past.  Qemu should NEVER advertise a
> ready event with 0/0, it should at least be 1/1 (because of the number
> of clients that have workarounds to deal with older qemu behavior on 0/0
> and which might misbehave if we ever issue that again).
>
>> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
>> "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 67108864, "offset": 
>> 67108864, "speed": 0, "type": "mirror"}}
>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
>> "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 0, "offset": 0, 
>> "speed": 0, "type": "mirror"}}
> So NACK to the patch as currently written, but not necessarily to the
> idea if you can give better progress numbers and never reach the state
> of BLOCK_JOB_READY and BLOCK_JOB_COMPLETED with a 0/0 length/offset.
>
ok. fair enough. Thank you for the review.

Will it be better to (somehow) skip progressing below using
some condition during mirror_dirty_init() stage?

static void mirror_iteration_done(MirrorOp *op, int ret)
{
.

if (ret >= 0) {
if (s->cow_bitmap) {
bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
}
s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
< specifically this progressing
}

Den

Re: [Qemu-devel] [PATCH] io: fix decoding when multiple websockets frames arrive at once

[Eric Blake]

On 01/30/2017 04:51 AM, Daniel P. Berrange wrote:
> The qio_channel_websock_read_wire() method will read upto 4096

s/upto/up to/

> bytes off the socket and then decode the websockets header and
> payload. The code was only decoding a single websockets frame,
> even if the buffered data contained multiple frames. This meant
> that decoding of subsequent frames was delayed until further
> input arrived on the socket. This backlog of delayed frames
> gets worse & worse over time.
> 
> Symptom was that when connecting to the VNC server via the
> built-in websockets server, mouse/keyboard interaction would
> start out fine, but slowly get more & more delayed until it
> was unusable.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  io/channel-websock.c | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/1] mirror: restore offset after zeroing out the image

[Eric Blake]

On 01/30/2017 04:22 AM, Denis V. Lunev wrote:
> If explicit zeroing out before mirroring is required for the target image,
> it moves the block job offset counter to EOF, then offset and len counters
> count the image size twice.
> 
> There is no harm but confusing stats (e.g. for 1G image the completion
> counter starts from 1G and increases to 2G)
> 
> The patch fixed that problem by resetting the offset counter.

Counters are explicitly documented NOT tied to disk length; they are
merely estimates of proportional completion.  I'm not sure if this makes
the numbers jump backwards from the observer's viewpoint, but if you can
ever spot 1g/1g right before rewinding to 0g/1g (where pre-patch could
see 1g/2g), then that is a reason to not take this patch.  On the other
hand, your argument that the pre-patch behavior progressing towards 2g
has a very fast progression from 0-1g/2g, and then a much slower
1g-2g/2g, which makes the estimate of percent completion skewed, while a
newer progression of 0-1g/1g is more realistic, may have some merit.

I'm not sold on this patch yet, but stronger arguments in the commit
message may sway me.

> +++ b/tests/qemu-iotests/094.out
> @@ -3,9 +3,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  Formatting 'TEST_DIR/source.IMGFMT', fmt=IMGFMT size=67108864
>  {"return": {}}
>  {"return": {}}
> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "BLOCK_JOB_READY", "data": {"device": "src", "len": 67108864, "offset": 
> 67108864, "speed": 0, "type": "mirror"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "BLOCK_JOB_READY", "data": {"device": "src", "len": 0, "offset": 0, "speed": 
> 0, "type": "mirror"}}
>  {"return": {}}

This part of the change is scary - a ready event showing 0/0 HAS been
known to confuse libvirt in the past.  Qemu should NEVER advertise a
ready event with 0/0, it should at least be 1/1 (because of the number
of clients that have workarounds to deal with older qemu behavior on 0/0
and which might misbehave if we ever issue that again).

> -{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 67108864, "offset": 
> 67108864, "speed": 0, "type": "mirror"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
> "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 0, "offset": 0, 
> "speed": 0, "type": "mirror"}}

So NACK to the patch as currently written, but not necessarily to the
idea if you can give better progress numbers and never reach the state
of BLOCK_JOB_READY and BLOCK_JOB_COMPLETED with a 0/0 length/offset.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 6/7] blkdebug: Add ability to override unmap geometries

[Eric Blake]

On 01/28/2017 02:57 PM, Max Reitz wrote:
> On 20.12.2016 20:15, Eric Blake wrote:
>> Make it easier to simulate various unusual hardware setups (for
>> example, recent commits 3482b9b and b8d0a98 affect the Dell
>> Equallogic iSCSI with its 15M preferred and maximum unmap and
>> write zero sizing, or b2f95fe deals with the Linux loopback
>> block device having a max_transfer of 64k), by allowing blkdebug
>> to wrap any other device with further restrictions on various
>> alignments.
>>
>> Signed-off-by: Eric Blake 
>>

>> +++ b/qapi/block-core.json
>> @@ -2072,6 +2072,26 @@
>>  # @align:   #optional required alignment for requests in bytes,
>>  #   must be power of 2, or 0 for default
>>  #
>> +# @max-transfer:#optional maximum size for I/O transfers in bytes,
>> +#   must be multiple of @align
> 
> ...and the file's request alignment. Should that be noted here?

As in - if we set @align to 1, but the underlying file still requires an
alignment of 512, then @max-transfer has to be at least 512.  Yeah, that
makes sense to document.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v8 08/25] tcg: drop global lock during TCG code execution

[Richard Henderson]

On 01/30/2017 01:57 AM, Alex Bennée wrote:
>>> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
>>> index a9ee7fddf9..2624d8d909 100644
>>> --- a/hw/intc/arm_gicv3_cpuif.c
>>> +++ b/hw/intc/arm_gicv3_cpuif.c
>>> @@ -14,6 +14,7 @@
>>>
>>>  #include "qemu/osdep.h"
>>>  #include "qemu/bitops.h"
>>> +#include "qemu/main-loop.h"
>>>  #include "trace.h"
>>>  #include "gicv3_internal.h"
>>>  #include "cpu.h"
>>> @@ -733,6 +734,8 @@ void gicv3_cpuif_update(GICv3CPUState *cs)
>>>  ARMCPU *cpu = ARM_CPU(cs->cpu);
>>>  CPUARMState *env = &cpu->env;
>>>
>>> +g_assert(qemu_mutex_iothread_locked());
>>> +
>> tcg_debug_assert()?
> Depends if KVM can use this for emulation as well (which I think it can).
> 

It probably could, but it shouldn't.  Let's not leak tcg_* into hw/.


r~

Re: [Qemu-devel] [PATCH v4 2/7] qcow2: Discard/zero clusters by byte count

[Eric Blake]

On 01/28/2017 02:21 PM, Max Reitz wrote:
> On 20.12.2016 20:15, Eric Blake wrote:
>> Passing a byte offset, but sector count, when we ultimately
>> want to operate on cluster granularity, is madness.  Clean up
>> the interfaces to take both offset and count as bytes, while
>> still keeping the assertion added previously that the caller
>> must align the values to a cluster.  Then rename things to
>> make sure backports don't get confused by changed units:
>> instead of qcow2_discard_clusters() and qcow2_zero_clusters(),
>> we now have qcow2_cluster_discard and qcow2_cluster_zeroize().
>>
>> Signed-off-by: Eric Blake 
>>

>> +int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
>> +  uint64_t count, int flags);
> 
> I know byte count parameters are often called "count", but I find it a
> bit problematic if the function name contains a word that can be a unit.
> In these cases, it's not entirely clear whether "count" refers to bytes
> or clusters. Maybe "length" or "size" would be better?

Now that you mention it, 'cluster' and 'count' is indeed confusing.  I'm
leaning towards 'size'.  Do I need to respin, or is that something that
the maintainer is comfortable tweaking?

> 
> Purely subjective and thus optional, of course.
> 
> Another thing: I'm not quite positive whether qcow2_cluster_zeroize()
> can actually handle a byte count greater than INT_MAX. If you could pass
> such a number to it, the multiplication "ret * s->cluster_size" might
> overflow. It's only caller qcow2_co_pwrite_zeroes() will never exceed
> that limit, but maybe this should be made clear somewhere -- either by
> making the parameter an int instead of a uint64_t or by asserting it in
> the function.

I'd lean towards an assertion, especially since it would be nice to
someday unify all the internal block interfaces to get to a 64-bit
interface wherever possible (limiting ourselves to 32-bit is necessary
for some drivers, like NBD, but that limitation should be enforced via
proper BlockLimits, rather than by inherent limits in our API).  An
assertion with 64-bit types is better than yet one more place to audit
for missing assertions if we use a 32-bit type now and then widen to
64-bit later.


>> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>> -int nb_sectors, enum qcow2_discard_type type, bool full_discard)
>> +int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
>> +  uint64_t count, enum qcow2_discard_type type,
>> +  bool full_discard)
>>  {
>>  BDRVQcow2State *s = bs->opaque;
>> -uint64_t end_offset;
>> +uint64_t end_offset = offset + count;
>>  uint64_t nb_clusters;
>>  int ret;
>>
>> -end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
>> -
>>  /* Caller must pass aligned values */
>>  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>>  assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size));
> 
> Directly below this we have "offset - end_offset" which could be
> shortened to "count" (or "length" or "size" or...).

Okay, there's now enough comments that it's worth me respinning a v5.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v1 1/6] qemu-img: add support for --object with 'dd' command

[Eric Blake]

On 01/26/2017 05:04 AM, Daniel P. Berrange wrote:
> The qemu-img dd command added --image-opts support, but missed
> the corresponding --object support. This prevented passing
> secrets (eg auth passwords) needed by certain disk images.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-img.c | 16 
>  1 file changed, 16 insertions(+)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 3/3] qemu-io: Fix tests expecting the wrong output

[Eric Blake]

On 01/27/2017 09:59 PM, Nir Soffer wrote:
> From: Nir Soffer 
> 
> Many tests expected the wrong behavior when qemu-io call into the
> command with after failing to open the file, writing this error:
> 
> no file open, try 'help open'
> 
> Now that we fail immediately when opening a file fails, this error does
> not exist in the output; remove it from tests output.

Ideally, this should be squashed into the patch that changes the output
(so that a git bisect landing on the change in behavior does not hit
unnecessarily-broken tests).

> 
> Tested using:
> 
> ./check 059 -vmdk (unrelated failure)
> ./check 070 -vhdx
> ./check 075 -cloop
> ./check 076 -parallels
> ./check 078 -bochs
> ./check 080 -qcow2
> ./check 083 -nbd
> ./check 088 -vpc
> ./check 092 -qcow
> ./check 116 -qed
> ./check 131 -parallels
> ./check 140 -raw
> ./check 140 -qcow2
> ./check -raw
> ./check -qcow2
> 
> Signed-off-by: Nir Soffer 
> ---
>  tests/qemu-iotests/059.out |  3 ---
>  tests/qemu-iotests/070.out |  1 -
>  tests/qemu-iotests/075.out |  7 ---
>  tests/qemu-iotests/076.out |  3 ---
>  tests/qemu-iotests/078.out |  6 --
>  tests/qemu-iotests/080.out | 18 --
>  tests/qemu-iotests/083.out | 17 -
>  tests/qemu-iotests/088.out |  6 --
>  tests/qemu-iotests/092.out | 12 
>  tests/qemu-iotests/116.out |  7 ---
>  tests/qemu-iotests/131.out |  1 -
>  tests/qemu-iotests/140.out |  1 -
>  12 files changed, 82 deletions(-)
> 

The changes look reasonable, once squashed in to the right place;
looking forward to v4.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 1/3] qemu-io: Return non-zero exit code on failure

[Eric Blake]

On 01/27/2017 09:59 PM, Nir Soffer wrote:
> From: Nir Soffer 
> 
> The result of openfile was not checked, leading to failure deep in the
> actual command with confusing error message, and exiting with exit code 0.
> 

When posting a series, please ensure that your messages are all marked
In-Reply-To a 0/3 cover letter (it may help if you do 'git config
format.coverletter auto').

> Here is a simple example - trying to read with the wrong format:
> 
> $ touch file
> $ qemu-io -f qcow2 -c 'read -P 1 0 1024' file; echo $?
> can't open device file: Image is not in qcow2 format
> no file open, try 'help open'
> 0
> 
> With this patch, we fail earlier with exit code 1:
> 
> $ ./qemu-io -f qcow2 -c 'read -P 1 0 1024' file; echo $?
> can't open device file: Image is not in qcow2 format
> 1
> 
> Signed-off-by: Nir Soffer 
> Reviewed-by: Eric Blake 
> Reviewed-by: Fam Zheng 
> ---
> 
> Changes since v2:
> - Adding missing signed-off-by
> - Fix tests expecting the wrong output

I don't see any tests changed...

> 
>  qemu-io.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

...in this diffstat.  If something really changed in this particular
patch since v2, then you should drop the Reviewed-by lines in order to
make sure I re-review it.  Or, if the changes you mention here are to
other patches in the series, then the 0/3 cover letter would have been a
better place to put that information.

> 
> diff --git a/qemu-io.c b/qemu-io.c
> index 23a229f..427cbae 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -595,13 +595,17 @@ int main(int argc, char **argv)
>  exit(1);
>  }
>  opts = qemu_opts_to_qdict(qopts, NULL);
> -openfile(NULL, flags, writethrough, opts);
> +if (openfile(NULL, flags, writethrough, opts)) {
> +exit(1);
> +}
>  } else {
>  if (format) {
>  opts = qdict_new();
>  qdict_put(opts, "driver", qstring_from_str(format));
>  }
> -openfile(argv[optind], flags, writethrough, opts);
> +if (openfile(argv[optind], flags, writethrough, opts)) {
> +exit(1);
> +}
>  }
>  }
>  command_loop();
> 

At any rate, I'm happy with this current patch, even if its presentation
in a series is less than ideal, so you can keep my R-b.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Juan Quintela]

Auger Eric  wrote:
> Hi Juan,
>
> On 30/01/2017 10:15, Juan Quintela wrote:
>> Eric Auger  wrote:
>>> We need to handle both registers and ITS tables. While
>>> register handling is standard, ITS table handling is more
>>> challenging since the kernel API is devised so that the
>>> tables are flushed into guest RAM and not in vmstate buffers.
>>>
>>> Flushing the ITS tables on device pre_save() is too late
>>> since the guest RAM had already been saved at this point.
>>>
>>> Table flushing needs to happen when we are sure the vcpus
>>> are stopped and before the last dirty page saving. The
>>> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
>>> VM gets stopped before migration launch so let's simply
>>> flush the tables each time the VM gets stopped.
>>>
>>> For regular ITS registers we just can use vmstate pre_save
>>> and post_load callbacks.
>>>
>>> Signed-off-by: Eric Auger 
>> 
>> Hi
>> 
>> 
>>> + * vm_change_state_handler - VM change state callback aiming at flushing
>>> + * ITS tables into guest RAM
>>> + *
>>> + * The tables get flushed to guest RAM whenever the VM gets stopped.
>>> + */
>>> +static void vm_change_state_handler(void *opaque, int running,
>>> +RunState state)
>>> +{
>>> +GICv3ITSState *s = (GICv3ITSState *)opaque;
>> 
>> Cast is unneeded.
>> 
>>> +
>>> +if (running) {
>>> +return;
>>> +}
>>> +kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
>>> +  0, NULL, false);
>> 
>> As you are adding it to do everytime that we stop the guest, how
>> expensive/slow is that?
>
> This is highly dependent on the number of devices using MSIs and number
> of allocated MSIs on guest. The number of bytes to transfer basically is:
>
> (#nb_vcpus + #nb_devices_using_MSI_on_guest  +  2 *
> nb_allocated_guest_MSIs bytes ) * 8 bytes
>
> So I would say < 10 kB in real life case. In my virtio-pci test case it
> is just 440 Bytes.
>
> For live migration I could hook a callback at RUN_STATE_FINISH_MIGRATE.
> However this does not work with virsh save/restore use case since the
> notifier is not called (the VM being already paused), hence that choice.

Agreed as a workaround.

We really need two notifiers:
- one that is run before the "completion stage" on source
- another that is run when we start the guest after a migration

But that is independent of this patch.

Later, Juan.

Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Juan Quintela]

Peter Maydell  wrote:
> On 30 January 2017 at 14:41, Ashijeet Acharya  
> wrote:
>> Commit a3a3d8c7 introduced a segfault bug while checking for
>> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
>> devices which do no set their 'dc->vmsd' yet while initialization.
>> Place a 'dc->vmsd' check prior to it so that we do not segfault for
>> such devices.
>>
>> NOTE: This doesn't compromise the functioning of --only-migratable
>> option as all the unmigratable devices do set their 'dc->vmsd'.
>>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>>  qdev-monitor.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 81d01df..a1106fd 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>  return NULL;
>>  }
>>
>> -if (only_migratable) {
>> +if (only_migratable && dc->vmsd) {
>>  if (dc->vmsd->unmigratable) {
>>  error_setg(errp, "Device %s is not migratable, but "
>> "--only-migratable was specified", driver);
>
> This seems like a good fix for the crash as a short term fix,
> but longer term I think it would be better to make setting
> dc->vmsd mandatory. I think devices which don't set it fall into
> one of these categories:
>  * deliberately has no VMState struct as it has no state that
>needs migrating -- we should have some kind of flag for
>such devices to positively assert that they don't need to
>deal with migration
>  * accidentally failed to provide a VMState struct -- this is
>a bug and the device should be fixed to at minimum mark
>itself as unmigratable (and ideally fixed to implement
>migration!)
>  * didn't provide a VMState struct because they handle
>migration manually using vmstate_register() -- we should
>update these to use VMState, or failing that use the
>flag to say "deliberately not providing VMState"
>
> Then we can make QEMU assert if dc->vmsd is NULL, and
> catch future occurrences of "oops I didn't think about
> migration" bugs in new device models. We also have an
> easy way to grep the codebase for devices that need to
> have migration implemented.

+1

Reviewed-by: Juan Quintela 

Re: [Qemu-devel] [PATCH 06/15] postcopy: Record largest page size

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)"  wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> > Record the largest page size in use; we'll need it soon for allocating
> > temporary buffers.
> >
> > Signed-off-by: Dr. David Alan Gilbert 
> 
> Not that I object, but  could we store this in ram_list, and update it
> it at RAMBlock creation time?  Why searh for the value later when we can
> store it from the beggining.  Instead of putting it on migration_state,
> put it on the ram_list itself?
> 

We could, but the code does get quite a bit more complicated for little gain,
given that we currently read it exactly once.
The update at creation time would be easier, but then you have to also
update at deletion time and that has to run along the list just like this.
(or cache based on the ram_list.version)

Dave

> Later, Juan.
> 
> 
> > ---
> >  exec.c| 13 +
> >  include/exec/cpu-common.h |  1 +
> >  include/migration/migration.h |  1 +
> >  migration/migration.c |  1 +
> >  4 files changed, 16 insertions(+)
> >
> > diff --git a/exec.c b/exec.c
> > index 8d4bb0e..69331d0 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1485,6 +1485,19 @@ size_t qemu_ram_pagesize(RAMBlock *rb)
> >  return rb->page_size;
> >  }
> >  
> > +/* Returns the largest size of page in use */
> > +size_t qemu_ram_pagesize_largest(void)
> > +{
> > +RAMBlock *block;
> > +size_t largest = 0;
> > +
> > +QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> > +largest = MAX(largest, qemu_ram_pagesize(block));
> > +}
> > +
> > +return largest;
> > +}
> > +
> >  static int memory_try_enable_merging(void *addr, size_t len)
> >  {
> >  if (!machine_mem_merge(current_machine)) {
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index bd15853..0e67727 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -64,6 +64,7 @@ void qemu_ram_set_idstr(RAMBlock *block, const char 
> > *name, DeviceState *dev);
> >  void qemu_ram_unset_idstr(RAMBlock *block);
> >  const char *qemu_ram_get_idstr(RAMBlock *rb);
> >  size_t qemu_ram_pagesize(RAMBlock *block);
> > +size_t qemu_ram_pagesize_largest(void);
> >  
> >  void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
> >  int len, int is_write);
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 0188bcf..7b311dd 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -89,6 +89,7 @@ struct MigrationIncomingState {
> >   */
> >  QemuEvent main_thread_load_event;
> >  
> > +size_t largest_page_size;
> >  bool   have_fault_thread;
> >  QemuThread fault_thread;
> >  QemuSemaphore  fault_thread_sem;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f498ab8..d8dafde 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -387,6 +387,7 @@ static void process_incoming_migration_co(void *opaque)
> >  int ret;
> >  
> >  mis = migration_incoming_state_new(f);
> > +mis->largest_page_size = qemu_ram_pagesize_largest();
> >  postcopy_state_set(POSTCOPY_INCOMING_NONE);
> >  migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> >MIGRATION_STATUS_ACTIVE);
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] qcow2: Optimize the refcount-block overlap check

[Alberto Garcia]

On Mon 30 Jan 2017 05:14:41 PM CET, Alberto Garcia wrote:

> This patch keeps the index of the last used (i.e. non-zero) entry in
> the refcount table and updates it every time the table changes. The
> refcount-block overlap check then uses that index instead of reading
> the whole table.

Note that while I decided to go for this approach the patch can be made
much simpler by simply stopping at the first empty entry in the refcount
table:

if ((chk & QCOW2_OL_REFCOUNT_BLOCK) && s->refcount_table) {
for (i = 0; i < s->refcount_table_size; i++) {
if (!(s->refcount_table[i] & REFT_OFFSET_MASK)) {
break;
}
if (overlaps_with(s->refcount_table[i] & REFT_OFFSET_MASK,
s->cluster_size)) {
return QCOW2_OL_REFCOUNT_BLOCK;
}
}
}

I don't think QEMU produces files where refcount_table[i] == 0 but
refcount_table[i + 1] != 0. Do they even make sense? In any case, my
patch would cover those cases too, but this simplified version wouldn't.

Berto

Re: [Qemu-devel] QEMU websockets support is laggy?

[Brian Rak]

On 1/30/2017 5:51 AM, Daniel P. Berrange wrote:

On Fri, Jan 27, 2017 at 06:08:20PM +, Daniel P. Berrange wrote:

On Fri, Jan 27, 2017 at 09:35:38AM +, Daniel P. Berrange wrote:

On Tue, Jan 24, 2017 at 05:02:25PM -0500, Brian Rak wrote:

We've been considering switching over to using qemu's built in websockets
support (to avoid the overhead of needing websockify running).  We've been
seeing very poor performance after the switch (it takes the console 4-5
seconds to update after pressing a key).  So far, I haven't been able to
find any indication of why this is happening.  The exact same configuration
works perfectly when running with websockify, but laggy when hitting qemu
directly.

I've tried a few things (disabling encryption, bypassing our usual nginx
proxy, even connecting via a ssh tunnel), and haven't made any sort of
progress here.  Has anyone else seen this?  Any suggestions as to where I
should start looking?

Can you clarify the exact setup you have ?  As mentioned on IRC, I don't
see any degradation in performance betweeen builtin websockets vs a
websockets proxy - if anything the builtin websockets is marginally less
laggy.  I was connecting over TCP localhost, however, so would not see
any effects of network latency. My test was QEMU git master, with noVNC
git master.

It turns out that to see the problem you have a wait a while - the
connection is initially fine, but gets worse over time. Using a TCP
connection seems to make it get worse more quickly.

After a painful debugging session I've discovered the problem is that
QEMU is reading data off the socket, decoding a websockets packet and
then processing it. The problem is if the read off the socket gets
multiple websockets packets at once, it only decodes the first packet.
The remaining packets aren't decoded until more data arrives on the
socket. This gets progressively worse & worse. I'll send a patch for
this next week...

I've just sent a patch & would appreciate your testing to confirm that
it fixes the problem for you too.

Regards,
Daniel
Confirmed, with the patch the built in websockets support performs just 
as well as vnc + websockify.  Thanks for the help!

[Qemu-devel] [PATCH] qcow2: Optimize the refcount-block overlap check

[Alberto Garcia]

The metadata overlap checks introduced in a40f1c2add help detect
corruption in the qcow2 image by verifying that data writes don't
overlap with existing metadata sections.

The 'refcount-block' check in particular iterates over the refcount
table in order to get the addresses of all refcount blocks and check
that none of them overlap with the region where we want to write.

The problem with the refcount table is that since it always occupies
complete clusters its size is usually very big. With the default
values of cluster_size=64KB and refcount_bits=16 this table holds 8192
entries, each one of them enough to map 2GB worth of host clusters.

So unless we're using images with several TB of allocated data this
table is going to be mostly empty, and iterating over it is a waste of
CPU. If the storage backend is fast enough this can have an effect on
I/O performance.

This patch keeps the index of the last used (i.e. non-zero) entry in
the refcount table and updates it every time the table changes. The
refcount-block overlap check then uses that index instead of reading
the whole table.

In my tests with a 4GB qcow2 file stored in RAM this doubles the
amount of write IOPS.

Signed-off-by: Alberto Garcia 
---
 block/qcow2-refcount.c | 21 -
 block/qcow2.c  |  1 +
 block/qcow2.h  |  1 +
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cbfb3fe064..5e4d36587f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -83,6 +83,16 @@ static Qcow2SetRefcountFunc *const set_refcount_funcs[] = {
 /*/
 /* refcount handling */
 
+static void update_max_refcount_table_index(BDRVQcow2State *s)
+{
+unsigned i = s->refcount_table_size - 1;
+while (i > 0 && (s->refcount_table[i] & REFT_OFFSET_MASK) == 0) {
+i--;
+}
+/* Set s->max_refcount_table_index to the index of the last used entry */
+s->max_refcount_table_index = i;
+}
+
 int qcow2_refcount_init(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
@@ -111,6 +121,7 @@ int qcow2_refcount_init(BlockDriverState *bs)
 }
 for(i = 0; i < s->refcount_table_size; i++)
 be64_to_cpus(&s->refcount_table[i]);
+update_max_refcount_table_index(s);
 }
 return 0;
  fail:
@@ -439,6 +450,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
 }
 
 s->refcount_table[refcount_table_index] = new_block;
+s->max_refcount_table_index = refcount_table_index;
 
 /* The new refcount block may be where the caller intended to put its
  * data, so let it restart the search. */
@@ -580,6 +592,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
 s->refcount_table = new_table;
 s->refcount_table_size = table_size;
 s->refcount_table_offset = table_offset;
+update_max_refcount_table_index(s);
 
 /* Free old table. */
 qcow2_free_clusters(bs, old_table_offset, old_table_size * 
sizeof(uint64_t),
@@ -2171,6 +2184,7 @@ write_refblocks:
 s->refcount_table = on_disk_reftable;
 s->refcount_table_offset = reftable_offset;
 s->refcount_table_size = reftable_size;
+update_max_refcount_table_index(s);
 
 return 0;
 
@@ -2383,7 +2397,11 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, 
int ign, int64_t offset,
 }
 
 if ((chk & QCOW2_OL_REFCOUNT_BLOCK) && s->refcount_table) {
-for (i = 0; i < s->refcount_table_size; i++) {
+unsigned last_entry = s->max_refcount_table_index;
+assert(last_entry < s->refcount_table_size);
+assert(last_entry + 1 == s->refcount_table_size ||
+   (s->refcount_table[last_entry + 1] & REFT_OFFSET_MASK) == 0);
+for (i = 0; i <= last_entry; i++) {
 if ((s->refcount_table[i] & REFT_OFFSET_MASK) &&
 overlaps_with(s->refcount_table[i] & REFT_OFFSET_MASK,
 s->cluster_size)) {
@@ -2871,6 +2889,7 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int 
refcount_order,
 /* Now update the rest of the in-memory information */
 old_reftable = s->refcount_table;
 s->refcount_table = new_reftable;
+update_max_refcount_table_index(s);
 
 s->refcount_bits = 1 << refcount_order;
 s->refcount_max = UINT64_C(1) << (s->refcount_bits - 1);
diff --git a/block/qcow2.c b/block/qcow2.c
index 96fb8a8f16..3e274bd1ba 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2743,6 +2743,7 @@ static int make_completely_empty(BlockDriverState *bs)
 
 s->refcount_table_offset = s->cluster_size;
 s->refcount_table_size   = s->cluster_size / sizeof(uint64_t);
+s->max_refcount_table_index = 0;
 
 g_free(s->refcount_table);
 s->refcount_table = new_reftable;
diff --git a/block/qcow2.h b/block/qcow2.h
index 182341483a..f8aeb08794 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -251,6 +251,7 @@ typedef struct BDRVQcow2State {
 u

Re: [Qemu-devel] [PATCH 11/17] block: explicitly acquire aiocontext in timers that need it

[Stefan Hajnoczi]

On Fri, Jan 20, 2017 at 05:43:16PM +0100, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  block/curl.c|  2 ++
>  block/io.c  |  5 +
>  block/iscsi.c   |  8 ++--
>  block/null.c|  4 
>  block/qed.c | 12 
>  block/qed.h |  3 +++
>  block/throttle-groups.c |  2 ++
>  util/aio-posix.c|  2 --
>  util/aio-win32.c|  2 --
>  util/qemu-coroutine-sleep.c |  2 +-
>  10 files changed, 35 insertions(+), 7 deletions(-)
> 
> v1->v2: document restrictions in bdrv_aio_cancel due to qemu_aio_ref
>   [Stefan]

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] Towards an ivshmem 2.0?

[Jan Kiszka]

On 2017-01-30 13:19, Markus Armbruster wrote:
>>> Can you explain why not letting the guest map the shared memory into its
>>> address space on its own just like any other piece of device memory is a
>>> requirement?
>>
>> It requires reconfiguration of the sensitive 2nd level page tables
>> during runtime of the guest. We are avoiding the neccessery checking and
>> synchronization measures so far which reduces code complexity further.
> 
> You mean the hypervisor needs to act when the guest maps BARs, and that
> gives the guest an attack vector?

Possibly, at least correctness issue will arise. We need to add TLB
flushes e.g., something that is not needed right now with the mappings
remaining static while a guest is running.

> 
> Don't you have to deal with that anyway, for other PCI devices?

Physical devices are presented to the guest with their BARs programmed
(as if the firmware did that already), and Jailhouse denies
reprogramming (only for the purpose of size discovery). Linux is fine
with that, and RTOSes ported to Jailhouse only become simpler.

Virtualized regions are trapped-and-emulate anyway, so no need for
reprogramming the mappings

Jan

Re: [Qemu-devel] [PATCH] xen: use qdev_unplug() insteda of g_free() in xen_pv_find_xendev()

[Juergen Gross]

On 30/01/17 16:46, Peter Maydell wrote:
> On 30 January 2017 at 15:14, Juergen Gross  wrote:
>> The error exits of xen_pv_find_xendev() free the new xen-device via
>> g_free() which is wrong.
>>
>> As the xen-device has been initialized as qdev it must be removed
>> via qdev_unplug().
>>
>> This bug has been introduced with commit 3a6c9172ac5951e6dac2b3f6
>> ("xen: create qdev for each backend device").
>>
>> Reported-by: Roger Pau Monné 
>> Tested-by: Roger Pau Monné 
>> Signed-off-by: Juergen Gross 
>> ---
>>  hw/xen/xen_backend.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
>> index d119004..030772b 100644
>> --- a/hw/xen/xen_backend.c
>> +++ b/hw/xen/xen_backend.c
>> @@ -145,7 +145,7 @@ static struct XenDevice *xen_be_get_xendev(const char 
>> *type, int dom, int dev,
>>  xendev->evtchndev = xenevtchn_open(NULL, 0);
>>  if (xendev->evtchndev == NULL) {
>>  xen_pv_printf(NULL, 0, "can't open evtchn device\n");
>> -g_free(xendev);
>> +qdev_unplug(&xendev->qdev, NULL);
>>  return NULL;
>>  }
>>  fcntl(xenevtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
>> @@ -155,7 +155,7 @@ static struct XenDevice *xen_be_get_xendev(const char 
>> *type, int dom, int dev,
>>  if (xendev->gnttabdev == NULL) {
>>  xen_pv_printf(NULL, 0, "can't open gnttab device\n");
>>  xenevtchn_close(xendev->evtchndev);
>> -g_free(xendev);
>> +qdev_unplug(&xendev->qdev, NULL);
>>  return NULL;
>>  }
>>  } else {
> 
> I think this will leak memory (and that we're already leaking
> memory), because the code is creating the xendev with
> xendev = g_malloc0(ops->size);
> object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
> 
> This is saying "I own and am responsible for freeing the memory
> that the device object lives in". If you want the object system
> to handle freeing the memory for you when the reference count
> goes to zero, then you need to create it with
>xendev = object_new()
> which we can't do here because we're allocating ops->size bytes
> rather than just the size of the object type.
> 
> Two options I think:
>  (1) have your code do
> OBJECT(xendev)->free = g_free;
>  after the object_initialize (to tell the object system how
> to free this object)
>  (2) call both qdev_unplug and g_free
> 
> I think (1) is better because it will definitely work even
> if the qdev bus system is holding on to an object reference
> after it returns from qdev_unplug() for some reason, and
> it will also mean we free the memory when we do a
> qdev_unplug in xen_pv_del_xendev(), rather than leaking it.

I agree. I'll send V2 with (1) included.

> Side note: Using DEVICE(xendev) is better QOM style than
> &xendev->qdev.

Okay. Will change.

Thanks for the very precise description.


Juergen

Re: [Qemu-devel] [RFC PATCH 1/5] checkpatch: add a check for utf-8 in commit logs

[Thomas Huth]

On 30.01.2017 15:12, Stefan Hajnoczi wrote:
> On Thu, Jan 26, 2017 at 02:11:01PM +0100, Thomas Huth wrote:
>> This patch is a port of the following commit from the Linux kernel:
>>
>> commit 15662b3e8644905032c2e26808401a487d4e90c1
>> Author: Joe Perches 
>> Date:   Mon Oct 31 17:13:12 2011 -0700
>>
>> checkpatch: add a --strict check for utf-8 in commit logs
>>
>> Some find using utf-8 in commit logs inappropriate.
>>
>> Some patch commit logs contain unintended utf-8 characters when doing
>> things like copy/pasting compilation output.
>>
>> Look for the start of any commit log by skipping initial lines that look
>> like email headers and "From: " lines.
>>
>> Stop looking for utf-8 at the first signature line.
>>
>> Signed-off-by: Joe Perches 
>> Suggested-by: Andrew Morton 
>> Cc: Andy Whitcroft 
>> Signed-off-by: Andrew Morton 
>> Signed-off-by: Linus Torvalds 
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  scripts/checkpatch.pl | 30 ++
>>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> This patch prevents including names with non-ASCII characters in the
> commit description.  Some people care about the proper spelling of their
> names.
> 
> Allowing UTF-8 in Signed-off-by and other headers isn't enough.

Right, and I guess the folks from the kernel checkpatch noticed this,
too. That's likely why the next patch restricts this check to only
happen if the patch is from a mail with non-UTF-8 content type, but
still contains UTF-8 characters, i.e. there is really something fishy
with the character set of the patch description. Maybe I should squash
the two patches together, so that it is more obvious what is going on here?

 Thomas





signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 07/17] nbd: convert to use qio_channel_yield

[Stefan Hajnoczi]

On Fri, Jan 20, 2017 at 05:43:12PM +0100, Paolo Bonzini wrote:
> +aio_co_wake(s->recv_coroutine[i]);
>  
> -qemu_coroutine_enter(nbd_get_client_session(bs)->send_coroutine);
> +/* We're woken up by the recv_coroutine itself.  */
> +qemu_coroutine_yield();

This relies on recv_coroutine() entering us only after we've yielded -
otherwise QEMU will crash.  The code and comments don't make it obvious
why this is guaranteed to be safe.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 06/17] io: make qio_channel_yield aware of AioContexts

[Stefan Hajnoczi]

On Fri, Jan 20, 2017 at 05:43:11PM +0100, Paolo Bonzini wrote:
> Support separate coroutines for reading and writing, and place the
> read/write handlers on the AioContext that the QIOChannel is registered
> with.
> 
> Cc: Daniel P. Berrange 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2:
> improved qio_channel_set_aio_context docs, renamed to
> qio_channel_attach_aio_context [Daniel, Stefan]
> fixed pasto in "io: make qio_channel_yield aware of AioContexts"
> 
>  include/io/channel.h | 47 ++--
>  io/channel.c | 86 
> +++-
>  2 files changed, 109 insertions(+), 24 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/1] io: ignore case in WebSocket HTTP header #PSBM-57554

[Daniel P. Berrange]

What is #PSBM-57554 referring to ?  Is that some custom bug tracker
you have ? I'm going to drop that unless its something we need to
keep

On Mon, Jan 30, 2017 at 04:19:56PM +0300, Denis V. Lunev wrote:
> From: Anton Nefedov 
> 
> According to RFC7230 Section 3.2, header field name is case-insensitive.
> 
> The haystack string length is limited by 4096 bytes by
> qio_channel_websock_handshake_read().
> 
> Further, handshake_process() dups and NULL-terminates the string
> so it is safe to call non length-limited functions like strcasestr().
> 
> Signed-off-by: Anton Nefedov 
> Signed-off-by: Denis V. Lunev 
> CC: Daniel P. Berrange 
> ---
>  io/channel-websock.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrange 

will add this to my io queue

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH] xen: use qdev_unplug() insteda of g_free() in xen_pv_find_xendev()

[Peter Maydell]

On 30 January 2017 at 15:14, Juergen Gross  wrote:
> The error exits of xen_pv_find_xendev() free the new xen-device via
> g_free() which is wrong.
>
> As the xen-device has been initialized as qdev it must be removed
> via qdev_unplug().
>
> This bug has been introduced with commit 3a6c9172ac5951e6dac2b3f6
> ("xen: create qdev for each backend device").
>
> Reported-by: Roger Pau Monné 
> Tested-by: Roger Pau Monné 
> Signed-off-by: Juergen Gross 
> ---
>  hw/xen/xen_backend.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index d119004..030772b 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -145,7 +145,7 @@ static struct XenDevice *xen_be_get_xendev(const char 
> *type, int dom, int dev,
>  xendev->evtchndev = xenevtchn_open(NULL, 0);
>  if (xendev->evtchndev == NULL) {
>  xen_pv_printf(NULL, 0, "can't open evtchn device\n");
> -g_free(xendev);
> +qdev_unplug(&xendev->qdev, NULL);
>  return NULL;
>  }
>  fcntl(xenevtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> @@ -155,7 +155,7 @@ static struct XenDevice *xen_be_get_xendev(const char 
> *type, int dom, int dev,
>  if (xendev->gnttabdev == NULL) {
>  xen_pv_printf(NULL, 0, "can't open gnttab device\n");
>  xenevtchn_close(xendev->evtchndev);
> -g_free(xendev);
> +qdev_unplug(&xendev->qdev, NULL);
>  return NULL;
>  }
>  } else {

I think this will leak memory (and that we're already leaking
memory), because the code is creating the xendev with
xendev = g_malloc0(ops->size);
object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);

This is saying "I own and am responsible for freeing the memory
that the device object lives in". If you want the object system
to handle freeing the memory for you when the reference count
goes to zero, then you need to create it with
   xendev = object_new()
which we can't do here because we're allocating ops->size bytes
rather than just the size of the object type.

Two options I think:
 (1) have your code do
OBJECT(xendev)->free = g_free;
 after the object_initialize (to tell the object system how
to free this object)
 (2) call both qdev_unplug and g_free

I think (1) is better because it will definitely work even
if the qdev bus system is holding on to an object reference
after it returns from qdev_unplug() for some reason, and
it will also mean we free the memory when we do a
qdev_unplug in xen_pv_del_xendev(), rather than leaking it.

Side note: Using DEVICE(xendev) is better QOM style than
&xendev->qdev.

(The weirdness with doing a g_malloc0(ops->size) appears
to be because Xen backends have their own setup for saying
"allocate this many bytes for my data", rather than having
XenBlkDev, XenConsole, etc be QOM subclasses of XenDevice.
Presumably that code all predates QEMU's QOMification.)

We also do OBJECT(thing)->free = something in hw/core/sysbus.c;
maybe we should have a function to abstract this away?
Not sure.

thanks
-- PMM

[Qemu-devel] [PATCH 3/4] xhci: don't kick in xhci_submit and xhci_fire_ctl_transfer

[Gerd Hoffmann]

xhci_submit and xhci_fire_ctl_transfer are is called from
xhci_kick_epctx processing loop only, so there is no need to call
xhci_kick_epctx make sure processing continues.  Also eecursive calls
into xhci_kick_epctx can cause trouble.

Drop the xhci_kick_epctx calls.

Cc: 1653...@bugs.launchpad.net
Fixes: 94b037f2a451b3dc855f9f2c346e5049a361bd55
Reported-by: Fabian Lesniak 
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-xhci.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4e2807e..899a410 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2001,11 +2001,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, 
XHCITransfer *xfer)
 xfer->packet.parameter = trb_setup->parameter;
 
 usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
-
 xhci_try_complete_packet(xfer);
-if (!xfer->running_async && !xfer->running_retry) {
-xhci_kick_epctx(xfer->epctx, 0);
-}
 return 0;
 }
 
@@ -2105,11 +2101,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer 
*xfer, XHCIEPContext *epctx
 return -1;
 }
 usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
-
 xhci_try_complete_packet(xfer);
-if (!xfer->running_async && !xfer->running_retry) {
-xhci_kick_epctx(xfer->epctx, xfer->streamid);
-}
 return 0;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] xhci: guard xhci_kick_epctx against recursive calls

[Gerd Hoffmann]

Track xhci_kick_epctx processing being active in a variable.  Check the
variable before calling xhci_kick_epctx from xhci_kick_ep.  Add an
assert to make sure we don't call recursively into xhci_kick_epctx.

Cc: 1653...@bugs.launchpad.net
Fixes: 94b037f2a451b3dc855f9f2c346e5049a361bd55
Reported-by: Fabian Lesniak 
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-xhci.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 899a410..12cac89 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -390,6 +390,7 @@ struct XHCIEPContext {
 dma_addr_t pctx;
 unsigned int max_psize;
 uint32_t state;
+uint32_t kick_active;
 
 /* streams */
 unsigned int max_pstreams;
@@ -2131,6 +2132,9 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int 
slotid,
 return;
 }
 
+if (!epctx->kick_active) {
+return;
+}
 xhci_kick_epctx(epctx, streamid);
 }
 
@@ -2155,6 +2159,9 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, 
unsigned int streamid)
 return;
 }
 
+assert(!epctx->kick_active);
+epctx->kick_active++;
+
 if (epctx->retry) {
 XHCITransfer *xfer = epctx->retry;
 
@@ -2253,6 +2260,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, 
unsigned int streamid)
 break;
 }
 }
+epctx->kick_active--;
 
 ep = xhci_epid_to_usbep(epctx);
 if (ep) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/17] io: add methods to set I/O handlers on AioContext

[Stefan Hajnoczi]

On Fri, Jan 20, 2017 at 05:43:10PM +0100, Paolo Bonzini wrote:
> This is in preparation for making qio_channel_yield work on
> AioContexts other than the main one.
> 
> Cc: Daniel P. Berrange 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: removed QIOChannelRestart [Daniel]
> 
>  include/io/channel.h | 25 +
>  io/channel-command.c | 13 +
>  io/channel-file.c| 11 +++
>  io/channel-socket.c  | 16 +++-
>  io/channel-tls.c | 12 
>  io/channel-watch.c   |  6 ++
>  io/channel.c | 11 +++
>  7 files changed, 89 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 0/4] xhci: fix misc regressions

[Gerd Hoffmann]

  Hi,

Commit 94b037f2a451b3dc855f9f2c346e5049a361bd55 caused some regressions,
partly plain bugs in that commit, partly it seems to have uncovered
other issues lurking in the xhci code.  This series fixes the isses
which poped up so far.

cheers,
  Gerd

Gerd Hoffmann (4):
  xhci: only free completed transfers
  xhci: rename xhci_complete_packet to xhci_try_complete_packet
  xhci: don't kick in xhci_submit and xhci_fire_ctl_transfer
  xhci: guard xhci_kick_epctx against recursive calls

 hw/usb/hcd-xhci.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] xhci: rename xhci_complete_packet to xhci_try_complete_packet

[Gerd Hoffmann]

Make clear that this isn't guaranteed to actually complete the transfer,
the usb packet can still be in flight after calling that function.

Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-xhci.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index dd429dc..4e2807e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1897,7 +1897,7 @@ static int xhci_setup_packet(XHCITransfer *xfer)
 return 0;
 }
 
-static int xhci_complete_packet(XHCITransfer *xfer)
+static int xhci_try_complete_packet(XHCITransfer *xfer)
 {
 if (xfer->packet.status == USB_RET_ASYNC) {
 trace_usb_xhci_xfer_async(xfer);
@@ -2002,7 +2002,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, 
XHCITransfer *xfer)
 
 usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
 
-xhci_complete_packet(xfer);
+xhci_try_complete_packet(xfer);
 if (!xfer->running_async && !xfer->running_retry) {
 xhci_kick_epctx(xfer->epctx, 0);
 }
@@ -2106,7 +2106,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer 
*xfer, XHCIEPContext *epctx
 }
 usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
 
-xhci_complete_packet(xfer);
+xhci_try_complete_packet(xfer);
 if (!xfer->running_async && !xfer->running_retry) {
 xhci_kick_epctx(xfer->epctx, xfer->streamid);
 }
@@ -2185,7 +2185,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, 
unsigned int streamid)
 }
 usb_handle_packet(xfer->packet.ep->dev, &xfer->packet);
 assert(xfer->packet.status != USB_RET_NAK);
-xhci_complete_packet(xfer);
+xhci_try_complete_packet(xfer);
 } else {
 /* retry nak'ed transfer */
 if (xhci_setup_packet(xfer) < 0) {
@@ -2195,7 +2195,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, 
unsigned int streamid)
 if (xfer->packet.status == USB_RET_NAK) {
 return;
 }
-xhci_complete_packet(xfer);
+xhci_try_complete_packet(xfer);
 }
 assert(!xfer->running_retry);
 if (xfer->complete) {
@@ -3492,7 +3492,7 @@ static void xhci_complete(USBPort *port, USBPacket 
*packet)
 xhci_ep_nuke_one_xfer(xfer, 0);
 return;
 }
-xhci_complete_packet(xfer);
+xhci_try_complete_packet(xfer);
 xhci_kick_epctx(xfer->epctx, xfer->streamid);
 if (xfer->complete) {
 xhci_ep_free_xfer(xfer);
-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] xhci: only free completed transfers

[Gerd Hoffmann]

Most callsites check already, one was missed.

Cc: 1653...@bugs.launchpad.net
Fixes: 94b037f2a451b3dc855f9f2c346e5049a361bd55
Reported-by: Fabian Lesniak 
Signed-off-by: Gerd Hoffmann 
---
 hw/usb/hcd-xhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index e0b5169..dd429dc 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2198,7 +2198,9 @@ static void xhci_kick_epctx(XHCIEPContext *epctx, 
unsigned int streamid)
 xhci_complete_packet(xfer);
 }
 assert(!xfer->running_retry);
-xhci_ep_free_xfer(epctx->retry);
+if (xfer->complete) {
+xhci_ep_free_xfer(epctx->retry);
+}
 epctx->retry = NULL;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start

[Halil Pasic]

On 10/20/2016 03:25 PM, Halil Pasic wrote:
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fc29acf..8767e40 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -66,10 +66,10 @@ static void *vmstate_base_addr(void *opaque, VMStateField 
> *field, bool alloc)
>  }
>  }
>  if (size) {
> -*((void **)base_addr + field->start) = g_malloc(size);
> +*(void **)base_addr = g_malloc(size);
>  }
>  }
> -base_addr = *(void **)base_addr + field->start;
> +base_addr = *(void **)base_addr;
>  }
> 
>  return base_addr;
Hi!

It is been a while, and IMHO this is still broken, and the
VMSTATE_VBUFFER* macros are still only used with the start argument
being zero.

What changed is that with commit 94869d5c ("migration: migrate QTAILQ")
from Jan 19 we have code actually using VMStateDecription.start -- but
for something different (IMHO), as allocation is done by get_qtailq and
not by vmstate_base_addr (as in case of VMSTATE_VBUFFER_ALLOC_UINT32).
Thus I would need to update the commit message and keep the start field
at least.

But before I do so, I would like to ask the maintainers if there is
interest in a change like this?

Regards,
Halil

Re: [Qemu-devel] [PATCH 04/17] block: move AioContext and QEMUTimer to libqemuutil

[Stefan Hajnoczi]

On Fri, Jan 20, 2017 at 05:43:09PM +0100, Paolo Bonzini wrote:
> AioContext is fairly self contained, the only dependency is QEMUTimer but
> that in turn doesn't need anything else.  So move them out of block-obj-y
> to avoid introducing a dependency from io/ to block-obj-y.
> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: new patch [Daniel]
> 
>  Makefile.objs   |  5 +---
>  block/io.c  | 29 ---
>  stubs/Makefile.objs |  2 ++
>  stubs/linux-aio.c   | 32 +
>  stubs/main-loop.c   |  8 ++
>  stubs/set-fd-handler.c  | 11 
>  tests/Makefile.include  | 13 -
>  util/Makefile.objs  |  5 +++-
>  aio-posix.c => util/aio-posix.c |  0
>  aio-win32.c => util/aio-win32.c |  0
>  util/aiocb.c| 55 
> +
>  async.c => util/async.c |  3 +-
>  qemu-timer.c => util/qemu-timer.c   |  0
>  thread-pool.c => util/thread-pool.c |  0
>  14 files changed, 110 insertions(+), 53 deletions(-)
>  create mode 100644 stubs/linux-aio.c
>  create mode 100644 stubs/main-loop.c
>  rename aio-posix.c => util/aio-posix.c (100%)
>  rename aio-win32.c => util/aio-win32.c (100%)
>  create mode 100644 util/aiocb.c
>  rename async.c => util/async.c (99%)
>  rename qemu-timer.c => util/qemu-timer.c (100%)
>  rename thread-pool.c => util/thread-pool.c (100%)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] target/s390x: use "qemu" cpu model in user mode

[Stefan Weil]

Am 30.01.2017 um 16:11 schrieb Peter Maydell:
> On 30 January 2017 at 15:09, Stefan Weil  wrote:
>> Am 30.01.2017 um 15:50 schrieb David Hildenbrand:
>>> "any" does not exist, therefore resulting in a misleading error message.
>>>
>>> Reported-by: Stefan Weil 
>>> Signed-off-by: David Hildenbrand 
>>> ---
>>>  linux-user/main.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/linux-user/main.c b/linux-user/main.c
>>> index 3004958..e588f58 100644
>>> --- a/linux-user/main.c
>>> +++ b/linux-user/main.c
>>> @@ -4322,6 +4322,8 @@ int main(int argc, char **argv, char **envp)
>>>  # endif
>>>  #elif defined TARGET_SH4
>>>  cpu_model = TYPE_SH7785_CPU;
>>> +#elif defined TARGET_S390X
>>> +cpu_model = "qemu";
>>>  #else
>>>  cpu_model = "any";
>>>  #endif
>>
>> Thanks.
>>
>> Reviewed-by: Stefan Weil 
>>
>> This fix is also needed for Debian's QEMU user emulation.
> Does it merit a cc: qemu-stable ?
>
> thanks
> -- PMM

Yes, of course. Thanks for the reminder - I added that list now as cc.

Re: [Qemu-devel] [PATCH 01/17] aio: introduce aio_co_schedule and aio_co_wake

[Stefan Hajnoczi]

On Fri, Jan 20, 2017 at 05:43:06PM +0100, Paolo Bonzini wrote:
> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
> index 14d4f1d..1efa356 100644
> --- a/include/qemu/coroutine_int.h
> +++ b/include/qemu/coroutine_int.h
> @@ -40,12 +40,20 @@ struct Coroutine {
>  CoroutineEntry *entry;
>  void *entry_arg;
>  Coroutine *caller;
> +
> +/* Only used when the coroutine has terminated.  */
>  QSLIST_ENTRY(Coroutine) pool_next;
>  size_t locks_held;

locks_held is used when the coroutine is running.  Please add a newline
to separate it from pool_next.

>  
> -/* Coroutines that should be woken up when we yield or terminate */
> +/* Coroutines that should be woken up when we yield or terminate.
> + * Only used when the coroutine is running.
> + */
>  QSIMPLEQ_HEAD(, Coroutine) co_queue_wakeup;
> +
> +/* Only used when the coroutine is sleeping.  */

"running", "yielded", and "terminated" are terms with specific meanings.
Is "sleeping" a subset of "yielded" where the coroutine is waiting for a
mutex or its AioContext to schedule it?

> +static void test_multi_co_schedule_entry(void *opaque)

Missing coroutine_fn.

> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index d2f529b..cb87997 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -33,17 +33,6 @@
>  static char temp_file[] = "/tmp/vmst.test.XX";
>  static int temp_fd;
>  
> -/* Fake yield_until_fd_readable() implementation so we don't have to pull the
> - * coroutine code as dependency.
> - */
> -void yield_until_fd_readable(int fd)
> -{
> -fd_set fds;
> -FD_ZERO(&fds);
> -FD_SET(fd, &fds);
> -select(fd + 1, &fds, NULL, NULL, NULL);
> -}
> -
>  
>  /* Duplicate temp_fd and seek to the beginning of the file */
>  static QEMUFile *open_test_file(bool write)

This should be a separate patch.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH] xen: use qdev_unplug() insteda of g_free() in xen_pv_find_xendev()

[Juergen Gross]

The error exits of xen_pv_find_xendev() free the new xen-device via
g_free() which is wrong.

As the xen-device has been initialized as qdev it must be removed
via qdev_unplug().

This bug has been introduced with commit 3a6c9172ac5951e6dac2b3f6
("xen: create qdev for each backend device").

Reported-by: Roger Pau Monné 
Tested-by: Roger Pau Monné 
Signed-off-by: Juergen Gross 
---
 hw/xen/xen_backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index d119004..030772b 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -145,7 +145,7 @@ static struct XenDevice *xen_be_get_xendev(const char 
*type, int dom, int dev,
 xendev->evtchndev = xenevtchn_open(NULL, 0);
 if (xendev->evtchndev == NULL) {
 xen_pv_printf(NULL, 0, "can't open evtchn device\n");
-g_free(xendev);
+qdev_unplug(&xendev->qdev, NULL);
 return NULL;
 }
 fcntl(xenevtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
@@ -155,7 +155,7 @@ static struct XenDevice *xen_be_get_xendev(const char 
*type, int dom, int dev,
 if (xendev->gnttabdev == NULL) {
 xen_pv_printf(NULL, 0, "can't open gnttab device\n");
 xenevtchn_close(xendev->evtchndev);
-g_free(xendev);
+qdev_unplug(&xendev->qdev, NULL);
 return NULL;
 }
 } else {
-- 
2.10.2

Re: [Qemu-devel] [PATCH] target/s390x: use "qemu" cpu model in user mode

[Peter Maydell]

On 30 January 2017 at 15:09, Stefan Weil  wrote:
> Am 30.01.2017 um 15:50 schrieb David Hildenbrand:
>> "any" does not exist, therefore resulting in a misleading error message.
>>
>> Reported-by: Stefan Weil 
>> Signed-off-by: David Hildenbrand 
>> ---
>>  linux-user/main.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index 3004958..e588f58 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -4322,6 +4322,8 @@ int main(int argc, char **argv, char **envp)
>>  # endif
>>  #elif defined TARGET_SH4
>>  cpu_model = TYPE_SH7785_CPU;
>> +#elif defined TARGET_S390X
>> +cpu_model = "qemu";
>>  #else
>>  cpu_model = "any";
>>  #endif
>
>
> Thanks.
>
> Reviewed-by: Stefan Weil 
>
> This fix is also needed for Debian's QEMU user emulation.

Does it merit a cc: qemu-stable ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/s390x: use "qemu" cpu model in user mode

[Stefan Weil]

Am 30.01.2017 um 15:50 schrieb David Hildenbrand:
> "any" does not exist, therefore resulting in a misleading error message.
>
> Reported-by: Stefan Weil 
> Signed-off-by: David Hildenbrand 
> ---
>  linux-user/main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 3004958..e588f58 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -4322,6 +4322,8 @@ int main(int argc, char **argv, char **envp)
>  # endif
>  #elif defined TARGET_SH4
>  cpu_model = TYPE_SH7785_CPU;
> +#elif defined TARGET_S390X
> +cpu_model = "qemu";
>  #else
>  cpu_model = "any";
>  #endif


Thanks.

Reviewed-by: Stefan Weil 

This fix is also needed for Debian's QEMU user emulation.

Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Peter Maydell]

On 30 January 2017 at 14:41, Ashijeet Acharya  wrote:
> Commit a3a3d8c7 introduced a segfault bug while checking for
> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
> devices which do no set their 'dc->vmsd' yet while initialization.
> Place a 'dc->vmsd' check prior to it so that we do not segfault for
> such devices.
>
> NOTE: This doesn't compromise the functioning of --only-migratable
> option as all the unmigratable devices do set their 'dc->vmsd'.
>
> Signed-off-by: Ashijeet Acharya 
> ---
>  qdev-monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 81d01df..a1106fd 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>  return NULL;
>  }
>
> -if (only_migratable) {
> +if (only_migratable && dc->vmsd) {
>  if (dc->vmsd->unmigratable) {
>  error_setg(errp, "Device %s is not migratable, but "
> "--only-migratable was specified", driver);

This seems like a good fix for the crash as a short term fix,
but longer term I think it would be better to make setting
dc->vmsd mandatory. I think devices which don't set it fall into
one of these categories:
 * deliberately has no VMState struct as it has no state that
   needs migrating -- we should have some kind of flag for
   such devices to positively assert that they don't need to
   deal with migration
 * accidentally failed to provide a VMState struct -- this is
   a bug and the device should be fixed to at minimum mark
   itself as unmigratable (and ideally fixed to implement
   migration!)
 * didn't provide a VMState struct because they handle
   migration manually using vmstate_register() -- we should
   update these to use VMState, or failing that use the
   flag to say "deliberately not providing VMState"

Then we can make QEMU assert if dc->vmsd is NULL, and
catch future occurrences of "oops I didn't think about
migration" bugs in new device models. We also have an
easy way to grep the codebase for devices that need to
have migration implemented.

thanks
-- PMM

[Qemu-devel] [PATCH] q35: Provide improved sample configurations

[Andrea Bolognani]

Instead of having a single sample configuration file,
now we have two: one gives access to the guest through
the serial console and only includes a minimal set of
devices, the other uses a graphical console and includes
extra devices such as an audio card.

Both configuration file are full commented, neatly
organized, and use paravirtualized devices instead of
emulated devices whenever possible for a better user
experience. Moreover, they follow the PCI Express
Guidelines (docs/pcie.txt) for their topology.
---
I plan to provide similar sample configurations for
aarch64/virt guests once the generic PCIe Root Ports
have been merged.

 docs/q35-chipset.cfg   | 152 
 docs/q35-graphical.cfg | 154 +
 docs/q35-serial.cfg| 110 +++
 3 files changed, 264 insertions(+), 152 deletions(-)
 delete mode 100644 docs/q35-chipset.cfg
 create mode 100644 docs/q35-graphical.cfg
 create mode 100644 docs/q35-serial.cfg

diff --git a/docs/q35-chipset.cfg b/docs/q35-chipset.cfg
deleted file mode 100644
index e4ddb7d..000
--- a/docs/q35-chipset.cfg
+++ /dev/null
@@ -1,152 +0,0 @@
-
-#
-# qemu -M q35 creates a bare machine with just the very essential
-# chipset devices being present:
-#
-# 00.0 - Host bridge
-# 1f.0 - ISA bridge / LPC
-# 1f.2 - SATA (AHCI) controller
-# 1f.3 - SMBus controller
-#
-# This config file documents the other devices and how they are
-# created.  You can simply use "-readconfig $thisfile" to create
-# them all.  Here is a overview:
-#
-# 19.0 - Ethernet controller (not created, our e1000 emulation
-# doesn't emulate the ich9 device).
-# 1a.* - USB Controller #2 (ehci + uhci companions)
-# 1b.0 - HD Audio Controller
-# 1c.* - PCI Express Ports
-# 1d.* - USB Controller #1 (ehci + uhci companions,
-#   "qemu -M q35 -usb" creates these too)
-# 1e.0 - PCI Bridge
-#
-
-[device "ich9-ehci-2"]
-  driver = "ich9-usb-ehci2"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1a.7"
-
-[device "ich9-uhci-4"]
-  driver = "ich9-usb-uhci4"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1a.0"
-  masterbus = "ich9-ehci-2.0"
-  firstport = "0"
-
-[device "ich9-uhci-5"]
-  driver = "ich9-usb-uhci5"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1a.1"
-  masterbus = "ich9-ehci-2.0"
-  firstport = "2"
-
-[device "ich9-uhci-6"]
-  driver = "ich9-usb-uhci6"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1a.2"
-  masterbus = "ich9-ehci-2.0"
-  firstport = "4"
-
-
-[device "ich9-hda-audio"]
-  driver = "ich9-intel-hda"
-  bus = "pcie.0"
-  addr = "1b.0"
-
-
-[device "ich9-pcie-port-1"]
-  driver = "ioh3420"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1c.0"
-  port = "1"
-  chassis = "1"
-
-[device "ich9-pcie-port-2"]
-  driver = "ioh3420"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1c.1"
-  port = "2"
-  chassis = "2"
-
-[device "ich9-pcie-port-3"]
-  driver = "ioh3420"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1c.2"
-  port = "3"
-  chassis = "3"
-
-[device "ich9-pcie-port-4"]
-  driver = "ioh3420"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1c.3"
-  port = "4"
-  chassis = "4"
-
-##
-# Example PCIe switch with two downstream ports
-#
-#[device "pcie-switch-upstream-port-1"]
-#  driver = "x3130-upstream"
-#  bus = "ich9-pcie-port-4"
-#  addr = "00.0"
-#
-#[device "pcie-switch-downstream-port-1-1"]
-#  driver = "xio3130-downstream"
-#  multifunction = "on"
-#  bus = "pcie-switch-upstream-port-1"
-#  addr = "00.0"
-#  port = "1"
-#  chassis = "5"
-#
-#[device "pcie-switch-downstream-port-1-2"]
-#  driver = "xio3130-downstream"
-#  multifunction = "on"
-#  bus = "pcie-switch-upstream-port-1"
-#  addr = "00.1"
-#  port = "1"
-#  chassis = "6"
-
-[device "ich9-ehci-1"]
-  driver = "ich9-usb-ehci1"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1d.7"
-
-[device "ich9-uhci-1"]
-  driver = "ich9-usb-uhci1"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1d.0"
-  masterbus = "ich9-ehci-1.0"
-  firstport = "0"
-
-[device "ich9-uhci-2"]
-  driver = "ich9-usb-uhci2"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1d.1"
-  masterbus = "ich9-ehci-1.0"
-  firstport = "2"
-
-[device "ich9-uhci-3"]
-  driver = "ich9-usb-uhci3"
-  multifunction = "on"
-  bus = "pcie.0"
-  addr = "1d.2"
-  masterbus = "ich9-ehci-1.0"
-  firstport = "4"
-
-
-[device "ich9-pci-bridge"]
-  driver = "i82801b11-bridge"
-  bus = "pcie.0"
-  addr = "1e.0"
diff --git a/docs/q35-graphical.cfg b/docs/q35-graphical.cfg
new file mode 100644
index 000..90c25cc
--- /dev/null
+++ b/docs/q35-graphical.cfg
@@ -0,0 +1,154 @@
+# q35 guest - sample configuration (graphical console)
+# =
+#
+# Usage:
+#
+#   $ qemu-system-x86_64 \
+# -nodefaults \
+#