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

[Thomas Huth]

** Changed in: qemu
   Importance: Undecided => Wishlist

-- 
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:
  New

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 RFC v3 12/14] intel_iommu: do replay when context invalidate

[Jason Wang]

On 2017年01月16日 15:43, Peter Xu wrote:

On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote:


On 2017年01月13日 11:06, Peter Xu wrote:

Before this one we only invalidate context cache when we receive context
entry invalidations. However it's possible that the invalidation also
contains a domain switch (only if cache-mode is enabled for vIOMMU).

So let's check for CM before replaying?

When CM is not set, there should have no device needs
IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here
(so the notifier_list will only contain UNMAP notifiers at most, and
sending UNMAP to those devices should not affect them at all).

If we check CM before replay, it'll be faster when guest change iommu
domain for a specific device. But after all this kind of operation is
extremely rare, while if we check CM bit, we have a "assumption" in
the code that MAP is depending on CM. In that case, to make the codes
cleaner, I'd slightly prefer not check it here. How do you think?


Ok, I think maybe it's better to add a comment here.

Thanks



Thanks,

-- peterx

Re: [Qemu-devel] [PATCH RFC v3 13/14] intel_iommu: allow dynamic switch of IOMMU region

[Peter Xu]

On Mon, Jan 16, 2017 at 02:20:31PM +0800, Jason Wang wrote:

[...]

> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index fd75112..2596f11 100644
> >--- a/hw/i386/intel_iommu.c
> >+++ b/hw/i386/intel_iommu.c
> >@@ -1343,9 +1343,49 @@ static void vtd_handle_gcmd_sirtp(IntelIOMMUState *s)
> >  vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS);
> >  }
> >+static void vtd_switch_address_space(VTDAddressSpace *as, bool 
> >iommu_enabled)
> 
> Looks like you can check s->dmar_enabled here?

Yes, we need to check old state in case we don't need a switch at all.
Actually I checked it...

> 
> >+{
> >+assert(as);
> >+
> >+trace_vtd_switch_address_space(pci_bus_num(as->bus),
> >+   VTD_PCI_SLOT(as->devfn),
> >+   VTD_PCI_FUNC(as->devfn),
> >+   iommu_enabled);
> >+
> >+/* Turn off first then on the other */
> >+if (iommu_enabled) {
> >+memory_region_set_enabled(>sys_alias, false);
> >+memory_region_set_enabled(>iommu, true);
> >+} else {
> >+memory_region_set_enabled(>iommu, false);
> >+memory_region_set_enabled(>sys_alias, true);
> >+}
> >+}

[...]

> >  /* Handle Translation Enable/Disable */
> >  static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
> >  {
> >+if (s->dmar_enabled == en) {
> >+return;
> >+}
> >+

... here :) ... and ...

[...]

> >+memory_region_init_alias(_dev_as->sys_alias, OBJECT(s),
> >+ "vtd_sys_alias", get_system_memory(),
> >+ 0, 
> >memory_region_size(get_system_memory()));
> >  memory_region_init_io(_dev_as->iommu_ir, OBJECT(s),
> >_mem_ir_ops, s, "intel_iommu_ir",
> >VTD_INTERRUPT_ADDR_SIZE);
> >-memory_region_add_subregion(_dev_as->iommu, 
> >VTD_INTERRUPT_ADDR_FIRST,
> >-_dev_as->iommu_ir);
> >-address_space_init(_dev_as->as,
> >-   _dev_as->iommu, name);
> >+memory_region_init(_dev_as->root, OBJECT(s),
> >+   "vtd_root", UINT64_MAX);
> >+memory_region_add_subregion_overlap(_dev_as->root,
> >+VTD_INTERRUPT_ADDR_FIRST,
> >+_dev_as->iommu_ir, 64);
> >+address_space_init(_dev_as->as, _dev_as->root, name);
> >+memory_region_add_subregion_overlap(_dev_as->root, 0,
> >+_dev_as->sys_alias, 1);
> >+memory_region_add_subregion_overlap(_dev_as->root, 0,
> >+_dev_as->iommu, 1);
> >+vtd_switch_address_space(vtd_dev_as, s->dmar_enabled);

... here I also used vtd_switch_address_space() to setup the init
state of the regions (in order to share the codes). So how about I
rename vtd_switch_address_space() into something like
vtd_setup_address_space(), to avoid misunderstanding?

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH] ahci: advertise HOST_CAP_64

[Ladi Prosek]

On Fri, Jan 13, 2017 at 8:15 PM, John Snow  wrote:
>
>
> On 01/13/2017 02:01 PM, Ladi Prosek wrote:
>> On Fri, Jan 13, 2017 at 7:31 PM, John Snow  wrote:
>>>
>>>
>>> On 01/13/2017 01:12 PM, Ladi Prosek wrote:
 On Fri, Jan 13, 2017 at 6:23 PM, John Snow  wrote:
> On 01/13/2017 06:02 AM, Ladi Prosek wrote:
>> The AHCI emulation code supports 64-bit addressing and should advertise 
>> this
>> fact in the Host Capabilities register. Both Linux and Windows drivers 
>> test
>> this bit to decide if the upper 32 bits of various registers may be 
>> written
>> to, and at least some versions of Windows have a bug where DMA is 
>> attempted
>> with an address above 4GB but, in the absence of HOST_CAP_64, the upper 
>> 32
>> bits are left unititialized which leads to a memory corruption.
>>
>> Signed-off-by: Ladi Prosek 
>> ---
>>  hw/ide/ahci.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 3c19bda..6a17acf 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -488,7 +488,7 @@ static void ahci_reg_init(AHCIState *s)
>>  s->control_regs.cap = (s->ports - 1) |
>>(AHCI_NUM_COMMAND_SLOTS << 8) |
>>(AHCI_SUPPORTED_SPEED_GEN1 << 
>> AHCI_SUPPORTED_SPEED) |
>> -  HOST_CAP_NCQ | HOST_CAP_AHCI;
>> +  HOST_CAP_NCQ | HOST_CAP_AHCI | HOST_CAP_64;
>>
>>  s->control_regs.impl = (1 << s->ports) - 1;
>>
>>
>
> Was this tested? What was the use case that prompted this patch, and do
> you have a public bugzilla to point to?

 Sorry, I thought you were aware. Here's the BZ with details:
 https://bugzilla.redhat.com/show_bug.cgi?id=1411105

>>>
>>> It's for the public record :)
>>>
>>> I'm going to amend your commit message, OK?
>>>
>>> https://github.com/jnsnow/qemu/commit/f037be92fc0c4580b846134e0355a69d0eccd0d9
>>
>> Minor nit: the OS we know for sure is affected is Windows Server 2008,
>> without the "R2". Thanks!
>>
>
> I misunderstood. It looked like the initial report was for "SP2," though
> I see you saying that the R2 version actually worked okay, but by
> coincidence.
>
> I didn't think there *was* an "SP2," for Windows Server, so I
> interpreted that to mean R2.
>
> So this only manifests with vanilla 2008?

We know it manifests on 2008 SP2, which is very different from 2008
R2. 2008 is the server variant of Vista, 2008 R2 is the server variant
of Win7 (yay for marketing names!)

I believe that 2008 SP2 32-bit is the only OS where it's been confirmed so far.

 In short, Windows guests run into issues in certain virtual HW
 configurations if the bit is not set. I have tested the patch and
 verified that it fixes the failing cases.

>>>
>>> Great. I'm CCing qemu-stable as this should probably be included in
>>> 2.7.2 / 2.8.1 / etc.
>>>
> It looks correct via the spec, thank you for finding this oversight. It
> looks like everywhere this bit would make a difference we already do the
> correct thing for having this bit set.
>
> From what I can gather from the spec, it looks as if even 32bit guests
> must ensure that the upper 32bit registers are cleared, so I think we're
> all set here.
>
> Reviewed-by: John Snow 
>>>
>>> Thanks, applied to my IDE tree:
>>>
>>> https://github.com/jnsnow/qemu/commits/ide
>>> https://github.com/jnsnow/qemu.git
>>>
>>> --js

Re: [Qemu-devel] [PATCH RFC v3 11/14] intel_iommu: provide its own replay() callback

[Jason Wang]

On 2017年01月16日 15:31, Peter Xu wrote:

On Fri, Jan 13, 2017 at 05:26:06PM +0800, Jason Wang wrote:


On 2017年01月13日 11:06, Peter Xu wrote:

The default replay() don't work for VT-d since vt-d will have a huge
default memory region which covers address range 0-(2^64-1). This will
normally bring a dead loop when guest starts.

I think it just takes too much time instead of dead loop?

Hmm, I can touch the commit message above to make it more precise.


The solution is simple - we don't walk over all the regions. Instead, we
jump over the regions when we found that the page directories are empty.
It'll greatly reduce the time to walk the whole region.

Yes, the problem is memory_region_is_iommu_reply() not smart because:

- It doesn't understand large page
- try go over all possible iova

So I'm thinking to introduce something like iommu_ops->iova_iterate() which

1) accept an start iova and return the next exist map
2) understand large page
3) skip unmapped iova

Though I haven't tested with huge pages yet, but this patch should
both solve above issue? I don't know whether you went over the page
walk logic - it should both support huge page, and it will skip
unmapped iova range (at least that's my goal to have this patch). In
that case, looks like this patch is solving the same problem? :)
(though without introducing iova_iterate() interface)

Please correct me if I misunderstood it.


Kind of :) I'm fine with this patch, but just want:

- reuse most of the codes in the patch
- current memory_region_iommu_replay() logic

So what I'm suggesting is a just slight change of API which can let 
caller decide it need to do with each range of iova. So it could be 
reused for other things except for replaying.


But if you like to keep this patch as is, I don't object it.




To achieve this, we provided a page walk helper to do that, invoking
corresponding hook function when we found an page we are interested in.
vtd_page_walk_level() is the core logic for the page walking. It's
interface is designed to suite further use case, e.g., to invalidate a
range of addresses.

Signed-off-by: Peter Xu

For intel iommu, since we intercept all map and unmap, a more tricky ieda is
to we can record the mappings internally in something like a rbtree which
could be iterated during replay. This saves possible guest io page table
traversal, but drawback is it may not survive from OOM attacker.

I think the problem is that we need this rbtree per guest-iommu-domain
(because mapping can be different per domain). In that case, I failed
to understand how the tree can help here. :(


Right, I see.

Thanks



Thanks,

-- peterx

Re: [Qemu-devel] [PATCH RFC v3 12/14] intel_iommu: do replay when context invalidate

[Peter Xu]

On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月13日 11:06, Peter Xu wrote:
> >Before this one we only invalidate context cache when we receive context
> >entry invalidations. However it's possible that the invalidation also
> >contains a domain switch (only if cache-mode is enabled for vIOMMU).
> 
> So let's check for CM before replaying?

When CM is not set, there should have no device needs
IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here
(so the notifier_list will only contain UNMAP notifiers at most, and
sending UNMAP to those devices should not affect them at all).

If we check CM before replay, it'll be faster when guest change iommu
domain for a specific device. But after all this kind of operation is
extremely rare, while if we check CM bit, we have a "assumption" in
the code that MAP is depending on CM. In that case, to make the codes
cleaner, I'd slightly prefer not check it here. How do you think?

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH RFC v3 09/14] memory: introduce memory_region_notify_one()

[Jason Wang]

On 2017年01月16日 15:08, Peter Xu wrote:

On Fri, Jan 13, 2017 at 03:58:59PM +0800, Jason Wang wrote:


On 2017年01月13日 11:06, Peter Xu wrote:

Generalizing the notify logic in memory_region_notify_iommu() into a
single function. This can be further used in customized replay()
functions for IOMMUs.

Signed-off-by: Peter Xu 
---
  include/exec/memory.h | 15 +++
  memory.c  | 29 ++---
  2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2233f99..f367e54 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -669,6 +669,21 @@ void memory_region_notify_iommu(MemoryRegion *mr,
  IOMMUTLBEntry entry);
  /**
+ * memory_region_notify_one: notify a change in an IOMMU translation
+ *   entry to a single notifier
+ *
+ * This works just like memory_region_notify_iommu(), but it only
+ * notifies a specific notifier, not all of them.
+ *
+ * @notifier: the notifier to be notified
+ * @entry: the new entry in the IOMMU translation table.  The entry
+ * replaces all old entries for the same virtual I/O address range.
+ * Deleted entries have .@perm == 0.
+ */
+void memory_region_notify_one(IOMMUNotifier *notifier,
+  IOMMUTLBEntry *entry);
+
+/**
   * memory_region_register_iommu_notifier: register a notifier for changes to
   * IOMMU translation entries.
   *
diff --git a/memory.c b/memory.c
index df62bd1..6e4c872 100644
--- a/memory.c
+++ b/memory.c
@@ -1665,26 +1665,33 @@ void 
memory_region_unregister_iommu_notifier(MemoryRegion *mr,
  memory_region_update_iommu_notify_flags(mr);
  }
-void memory_region_notify_iommu(MemoryRegion *mr,
-IOMMUTLBEntry entry)
+void memory_region_notify_one(IOMMUNotifier *notifier,
+  IOMMUTLBEntry *entry)
  {
-IOMMUNotifier *iommu_notifier;
  IOMMUNotifierFlag request_flags;
-assert(memory_region_is_iommu(mr));
-
-if (entry.perm & IOMMU_RW) {
+if (entry->perm & IOMMU_RW) {
  request_flags = IOMMU_NOTIFIER_MAP;
  } else {
  request_flags = IOMMU_NOTIFIER_UNMAP;
  }

Nit: you can keep this outside the loop.

Yes, but this function is used in vtd_replay_hook() as well in latter
patch. If I keep the above outside loop (IIUC you mean the loop in
memory_region_notify_iommu()), I'll need to set it up as well in
future vtd_replay_hook(), then that'll be slightly awkward.
Considering that the notification will only happen at mapping changes,
I'll prefer to keep the interface cleaner like what this patch has
done.

Thanks,

-- peterx


Ok, I see.

Re: [Qemu-devel] [PATCH RFC v3 11/14] intel_iommu: provide its own replay() callback

[Peter Xu]

On Fri, Jan 13, 2017 at 05:26:06PM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月13日 11:06, Peter Xu wrote:
> >The default replay() don't work for VT-d since vt-d will have a huge
> >default memory region which covers address range 0-(2^64-1). This will
> >normally bring a dead loop when guest starts.
> 
> I think it just takes too much time instead of dead loop?

Hmm, I can touch the commit message above to make it more precise.

> 
> >
> >The solution is simple - we don't walk over all the regions. Instead, we
> >jump over the regions when we found that the page directories are empty.
> >It'll greatly reduce the time to walk the whole region.
> 
> Yes, the problem is memory_region_is_iommu_reply() not smart because:
> 
> - It doesn't understand large page
> - try go over all possible iova
> 
> So I'm thinking to introduce something like iommu_ops->iova_iterate() which
> 
> 1) accept an start iova and return the next exist map
> 2) understand large page
> 3) skip unmapped iova

Though I haven't tested with huge pages yet, but this patch should
both solve above issue? I don't know whether you went over the page
walk logic - it should both support huge page, and it will skip
unmapped iova range (at least that's my goal to have this patch). In
that case, looks like this patch is solving the same problem? :)
(though without introducing iova_iterate() interface)

Please correct me if I misunderstood it.

> 
> >
> >To achieve this, we provided a page walk helper to do that, invoking
> >corresponding hook function when we found an page we are interested in.
> >vtd_page_walk_level() is the core logic for the page walking. It's
> >interface is designed to suite further use case, e.g., to invalidate a
> >range of addresses.
> >
> >Signed-off-by: Peter Xu
> 
> For intel iommu, since we intercept all map and unmap, a more tricky ieda is
> to we can record the mappings internally in something like a rbtree which
> could be iterated during replay. This saves possible guest io page table
> traversal, but drawback is it may not survive from OOM attacker.

I think the problem is that we need this rbtree per guest-iommu-domain
(because mapping can be different per domain). In that case, I failed
to understand how the tree can help here. :(

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH RFC v3 09/14] memory: introduce memory_region_notify_one()

[Peter Xu]

On Fri, Jan 13, 2017 at 03:58:59PM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月13日 11:06, Peter Xu wrote:
> >Generalizing the notify logic in memory_region_notify_iommu() into a
> >single function. This can be further used in customized replay()
> >functions for IOMMUs.
> >
> >Signed-off-by: Peter Xu 
> >---
> >  include/exec/memory.h | 15 +++
> >  memory.c  | 29 ++---
> >  2 files changed, 33 insertions(+), 11 deletions(-)
> >
> >diff --git a/include/exec/memory.h b/include/exec/memory.h
> >index 2233f99..f367e54 100644
> >--- a/include/exec/memory.h
> >+++ b/include/exec/memory.h
> >@@ -669,6 +669,21 @@ void memory_region_notify_iommu(MemoryRegion *mr,
> >  IOMMUTLBEntry entry);
> >  /**
> >+ * memory_region_notify_one: notify a change in an IOMMU translation
> >+ *   entry to a single notifier
> >+ *
> >+ * This works just like memory_region_notify_iommu(), but it only
> >+ * notifies a specific notifier, not all of them.
> >+ *
> >+ * @notifier: the notifier to be notified
> >+ * @entry: the new entry in the IOMMU translation table.  The entry
> >+ * replaces all old entries for the same virtual I/O address range.
> >+ * Deleted entries have .@perm == 0.
> >+ */
> >+void memory_region_notify_one(IOMMUNotifier *notifier,
> >+  IOMMUTLBEntry *entry);
> >+
> >+/**
> >   * memory_region_register_iommu_notifier: register a notifier for changes 
> > to
> >   * IOMMU translation entries.
> >   *
> >diff --git a/memory.c b/memory.c
> >index df62bd1..6e4c872 100644
> >--- a/memory.c
> >+++ b/memory.c
> >@@ -1665,26 +1665,33 @@ void 
> >memory_region_unregister_iommu_notifier(MemoryRegion *mr,
> >  memory_region_update_iommu_notify_flags(mr);
> >  }
> >-void memory_region_notify_iommu(MemoryRegion *mr,
> >-IOMMUTLBEntry entry)
> >+void memory_region_notify_one(IOMMUNotifier *notifier,
> >+  IOMMUTLBEntry *entry)
> >  {
> >-IOMMUNotifier *iommu_notifier;
> >  IOMMUNotifierFlag request_flags;
> >-assert(memory_region_is_iommu(mr));
> >-
> >-if (entry.perm & IOMMU_RW) {
> >+if (entry->perm & IOMMU_RW) {
> >  request_flags = IOMMU_NOTIFIER_MAP;
> >  } else {
> >  request_flags = IOMMU_NOTIFIER_UNMAP;
> >  }
> 
> Nit: you can keep this outside the loop.

Yes, but this function is used in vtd_replay_hook() as well in latter
patch. If I keep the above outside loop (IIUC you mean the loop in
memory_region_notify_iommu()), I'll need to set it up as well in
future vtd_replay_hook(), then that'll be slightly awkward.
Considering that the notification will only happen at mapping changes,
I'll prefer to keep the interface cleaner like what this patch has
done.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v3 2/2] memory: hmp: add "-f" for "info mtree"

[Peter Xu]

On Fri, Jan 13, 2017 at 06:19:10PM +, Dr. David Alan Gilbert wrote:

[...]

> > -void mtree_info(fprintf_function mon_printf, void *f)
> > +static void mtree_print_flatview(fprintf_function p, void *f,
> > + AddressSpace *as)
> > +{
> > +FlatView *view = address_space_get_flatview(as);
> > +FlatRange *range = >ranges[0];
> > +MemoryRegion *mr;
> > +int n = view->nr;
> > +
> > +if (n <= 0) {
> > +p(f, MTREE_INDENT "No rendered FlatView for "
> > +  "address space '%s'\n", as->name);
> 
> Do you need an unref there?

Definitely. Thanks!

-- peterx

Re: [Qemu-devel] [PATCH] cryptodev: setiv only when really need

[Gonglei (Arei)]

>
> From: longpeng
> Sent: Saturday, January 14, 2017 2:28 PM
> To: Gonglei (Arei)
> Cc: Wubin (H); Zhoujian (jay, Euler); qemu-devel@nongnu.org; longpeng
> Subject: [PATCH] cryptodev: setiv only when really need
> 
> ECB mode cipher doesn't need IV, if we setiv for it then qemu
> crypto API would report "Expected IV size 0 not **", so we should
> setiv only when the cipher algos really need.
> 
> Signed-off-by: Longpeng(Mike) 
> ---
>  backends/cryptodev-builtin.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
Queued.

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH RFC v3 14/14] intel_iommu: enable vfio devices

[Jason Wang]

On 2017年01月13日 11:06, Peter Xu wrote:

This patch is based on Aviv Ben-David ()'s patch
upstream:

   "IOMMU: enable intel_iommu map and unmap notifiers"
   https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg01453.html

However I removed/fixed some content, and added my own codes.

Instead of translate() every page for iotlb invalidations (which is
slower), we walk the pages when needed and notify in a hook function.

This patch enables vfio devices for VT-d emulation.

Signed-off-by: Peter Xu 
---
  hw/i386/intel_iommu.c | 68 +--
  include/hw/i386/intel_iommu.h |  8 +
  2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2596f11..104200b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -839,7 +839,8 @@ next:
   * @private: private data for the hook function
   */
  static int vtd_page_walk(VTDContextEntry *ce, uint64_t start, uint64_t end,
- vtd_page_walk_hook hook_fn, void *private)
+ vtd_page_walk_hook hook_fn, void *private,
+ bool notify_unmap)
  {
  dma_addr_t addr = vtd_get_slpt_base_from_context(ce);
  uint32_t level = vtd_get_level_from_context_entry(ce);
@@ -858,7 +859,7 @@ static int vtd_page_walk(VTDContextEntry *ce, uint64_t 
start, uint64_t end,
  trace_vtd_page_walk(ce->hi, ce->lo, start, end);
  
  return vtd_page_walk_level(addr, start, end, hook_fn, private,

-   level, true, true, NULL, false);
+   level, true, true, NULL, notify_unmap);
  }
  
  /* Map a device to its corresponding domain (context-entry) */

@@ -1212,6 +1213,34 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState 
*s, uint16_t domain_id)
  _id);
  }
  
+static int vtd_page_invalidate_notify_hook(IOMMUTLBEntry *entry,

+   void *private)
+{
+memory_region_notify_iommu((MemoryRegion *)private, *entry);
+return 0;
+}
+
+static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
+   uint16_t domain_id, hwaddr addr,
+   uint8_t am)
+{
+IntelIOMMUNotifierNode *node;
+VTDContextEntry ce;
+int ret;
+
+QLIST_FOREACH(node, &(s->notifiers_list), next) {
+VTDAddressSpace *vtd_as = node->vtd_as;
+ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
+   vtd_as->devfn, );
+if (!ret && domain_id == VTD_CONTEXT_ENTRY_DID(ce.hi)) {
+vtd_page_walk(, addr, addr + (1 << am) * VTD_PAGE_SIZE,
+  vtd_page_invalidate_notify_hook,
+  (void *)_as->iommu, true);
+}
+}
+}
+
+
  static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
hwaddr addr, uint8_t am)
  {
@@ -1222,6 +1251,7 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, 
uint16_t domain_id,
  info.addr = addr;
  info.mask = ~((1 << am) - 1);
  g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, );
+vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);


Is the case of GLOBAL or DSI flush missed, or we don't care it at all?

Thanks


  }
  
  /* Flush IOTLB

@@ -2244,15 +2274,34 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion 
*iommu,
IOMMUNotifierFlag new)
  {
  VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
+IntelIOMMUState *s = vtd_as->iommu_state;
+IntelIOMMUNotifierNode *node = NULL;
+IntelIOMMUNotifierNode *next_node = NULL;
  
-if (new & IOMMU_NOTIFIER_MAP) {

-error_report("Device at bus %s addr %02x.%d requires iommu "
- "notifier which is currently not supported by "
- "intel-iommu emulation",
- vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
- PCI_FUNC(vtd_as->devfn));
+if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
+error_report("We need to set cache_mode=1 for intel-iommu to enable "
+ "device assignment with IOMMU protection.");
  exit(1);
  }
+
+/* Add new ndoe if no mapping was exising before this call */


"node"?


+if (old == IOMMU_NOTIFIER_NONE) {
+node = g_malloc0(sizeof(*node));
+node->vtd_as = vtd_as;
+QLIST_INSERT_HEAD(>notifiers_list, node, next);
+return;
+}
+
+/* update notifier node with new flags */
+QLIST_FOREACH_SAFE(node, >notifiers_list, next, next_node) {
+if (node->vtd_as == vtd_as) {
+if (new == IOMMU_NOTIFIER_NONE) {
+QLIST_REMOVE(node, next);
+g_free(node);
+}

Re: [Qemu-devel] [PATCH RFC v3 13/14] intel_iommu: allow dynamic switch of IOMMU region

[Jason Wang]

On 2017年01月13日 11:06, Peter Xu wrote:

This is preparation work to finally enabled dynamic switching ON/OFF for
VT-d protection. The old VT-d codes is using static IOMMU address space,
and that won't satisfy vfio-pci device listeners.

Let me explain.

vfio-pci devices depend on the memory region listener and IOMMU replay
mechanism to make sure the device mapping is coherent with the guest
even if there are domain switches. And there are two kinds of domain
switches:

   (1) switch from domain A -> B
   (2) switch from domain A -> no domain (e.g., turn DMAR off)

Case (1) is handled by the context entry invalidation handling by the
VT-d replay logic. What the replay function should do here is to replay
the existing page mappings in domain B.

However for case (2), we don't want to replay any domain mappings - we
just need the default GPA->HPA mappings (the address_space_memory
mapping). And this patch helps on case (2) to build up the mapping
automatically by leveraging the vfio-pci memory listeners.

Another important thing that this patch does is to seperate
IR (Interrupt Remapping) from DMAR (DMA Remapping). IR region should not
depend on the DMAR region (like before this patch). It should be a
standalone region, and it should be able to be activated without
DMAR (which is a common behavior of Linux kernel - by default it enables
IR while disabled DMAR).

Signed-off-by: Peter Xu 
---
v3:
- fix another trivial style issue patchew reported but I missed in v2

v2:
- fix issues reported by patchew
- switch domain by enable/disable memory regions [David]
- provide vtd_switch_address_space{_all}()
- provide a better comment on the memory regions

test done: with intel_iommu device, boot vm with/without
"intel_iommu=on" parameter.

Signed-off-by: Peter Xu 
---
  hw/i386/intel_iommu.c | 78 ---
  hw/i386/trace-events  |  2 +-
  include/hw/i386/intel_iommu.h |  2 ++
  3 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index fd75112..2596f11 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1343,9 +1343,49 @@ static void vtd_handle_gcmd_sirtp(IntelIOMMUState *s)
  vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS);
  }
  
+static void vtd_switch_address_space(VTDAddressSpace *as, bool iommu_enabled)


Looks like you can check s->dmar_enabled here?


+{
+assert(as);
+
+trace_vtd_switch_address_space(pci_bus_num(as->bus),
+   VTD_PCI_SLOT(as->devfn),
+   VTD_PCI_FUNC(as->devfn),
+   iommu_enabled);
+
+/* Turn off first then on the other */
+if (iommu_enabled) {
+memory_region_set_enabled(>sys_alias, false);
+memory_region_set_enabled(>iommu, true);
+} else {
+memory_region_set_enabled(>iommu, false);
+memory_region_set_enabled(>sys_alias, true);
+}
+}
+
+static void vtd_switch_address_space_all(IntelIOMMUState *s, bool enabled)
+{
+GHashTableIter iter;
+VTDBus *vtd_bus;
+int i;
+
+g_hash_table_iter_init(, s->vtd_as_by_busptr);
+while (g_hash_table_iter_next(, NULL, (void **)_bus)) {
+for (i = 0; i < X86_IOMMU_PCI_DEVFN_MAX; i++) {
+if (!vtd_bus->dev_as[i]) {
+continue;
+}
+vtd_switch_address_space(vtd_bus->dev_as[i], enabled);
+}
+}
+}
+
  /* Handle Translation Enable/Disable */
  static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
  {
+if (s->dmar_enabled == en) {
+return;
+}
+
  VTD_DPRINTF(CSR, "Translation Enable %s", (en ? "on" : "off"));
  
  if (en) {

@@ -1360,6 +1400,8 @@ static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool 
en)
  /* Ok - report back to driver */
  vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
  }
+
+vtd_switch_address_space_all(s, en);
  }
  
  /* Handle Interrupt Remap Enable/Disable */

@@ -2586,15 +2628,43 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
PCIBus *bus, int devfn)
  vtd_dev_as->devfn = (uint8_t)devfn;
  vtd_dev_as->iommu_state = s;
  vtd_dev_as->context_cache_entry.context_cache_gen = 0;
+
+/*
+ * Memory region relationships looks like (Address range shows
+ * only lower 32 bits to make it short in length...):
+ *
+ * |-+---+--|
+ * | Name| Address range | Priority |
+ * |-+---+--+
+ * | vtd_root| - |0 |
+ * |  intel_iommu| - |1 |
+ * |  vtd_sys_alias  | - |1 |
+ * |  intel_iommu_ir | fee0-feef |   64 |
+ * |-+---+--|
+  

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Xiao Guangrong]

On 01/14/2017 02:02 AM, Eduardo Habkost wrote:

On Fri, Jan 13, 2017 at 01:17:27PM +, Stefan Hajnoczi wrote:

On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:

The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
is disabled. QEMU should refuse to plug any NVDIMM device in this case
and report the misconfiguration.

Reported-by: Stefan Hajnoczi 
Signed-off-by: Haozhong Zhang 
Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
Message-Id: 20170111093630.2088-1-stefa...@redhat.com
---
 hw/i386/pc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 25e8586..3907609 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
 }

 if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+if (!pcms->acpi_nvdimm_state.is_enabled) {
+error_setg(_err,
+   "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+goto out;
+}


A warning is definitely useful to notify users of a possible
configuration error.

I wonder what happens when you plug an NVDIMM into a motherboard where
the firmware lacks support.  Does it:
 * Refuse to boot?
 * Treat the DIMM as regular RAM?
 * Boot but the DIMM will not be used by firmware and kernel?

QEMU should act the same way as real hardware.


If real hardware behavior is not useful in any way (e.g. first
and third options above), is there a good reason for QEMU to not
implement an additional safety mechanism preventing NVDIMM from
being connected to a machine that doesn't support it?



Yes. i agree with Eduardo.

For the real hardware the behavior may be different between vendors, we
are asking our HW people to check what will happen on Intel's hardware
under this case.

Thanks!

Re: [Qemu-devel] [PATCH RFC v3 12/14] intel_iommu: do replay when context invalidate

[Jason Wang]

On 2017年01月13日 11:06, Peter Xu wrote:

Before this one we only invalidate context cache when we receive context
entry invalidations. However it's possible that the invalidation also
contains a domain switch (only if cache-mode is enabled for vIOMMU).


So let's check for CM before replaying?


  In
that case we need to notify all the registered components about the new
mapping.

Signed-off-by: Peter Xu 
---
  hw/i386/intel_iommu.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 59bf683..fd75112 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1162,6 +1162,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState 
*s,
  trace_vtd_inv_desc_cc_device(bus_n, (devfn_it >> 3) & 0x1f,
   devfn_it & 3);
  vtd_as->context_cache_entry.context_cache_gen = 0;
+memory_region_iommu_replay_all(_as->iommu);
  }
  }
  }

Re: [Qemu-devel] [PATCH] Makefile: Fix owner and group for qemu-version.h.tmp

[Fam Zheng]

On Sun, 01/15 18:03, Lin Ma wrote:
> By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
> In 'make' stage a qemu-version.h.tmp will be generated. If the content of
> qemu-version.h.tmp and qemu-version.h aren't consistent, The 
> qemu-version.h.tmp
> will be renamed to qemu-version.h. Because of the target FORCE, The same 
> action
> will be do again in 'make install' stage.
> 
> In 'make install' stage, If there is no qemu-version.h.tmp exists and we run
> 'make install' with sudo, The owner and group of new qemu-version.h.tmp will 
> be
> privileged user/group. When we run 'make' next time, qemu-version.h.tmp can't
> be overwrited because of permission issue.

s/overwrited/overwritten/

Otherwise, the change looks fine to me.

Reviewed-by: Fam Zheng 

> 
> This patch uses 'cp' instead of 'mv' to keep the qemu-version.h.tmp file, So
> during the 'sudo make install' stage, new qemu-version.h.tmp's owner and group
> wont be set to privileged user/group.
> 
> Signed-off-by: Lin Ma 
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 1a8bfb2..1ca45b2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -184,7 +184,7 @@ qemu-version.h: FORCE
>   printf '""\n'; \
>   fi; \
>   fi) > $@.tmp)
> - $(call quiet-command, cmp -s $@ $@.tmp || mv $@.tmp $@)
> + $(call quiet-command, cmp -s $@ $@.tmp || cp $@.tmp $@)
>  
>  config-host.h: config-host.h-timestamp
>  config-host.h-timestamp: config-host.mak
> -- 
> 2.9.2
> 
> 

Re: [Qemu-devel] [PATCH v3 1/1] block/vmdk: Fix the endian problem of buf_len and lba

[Fam Zheng]

On Mon, 01/16 10:38, liujing wrote:
> Dears,
> 
> We would like to know if this patch will be pulled
> 
> into upstream or what else we need to do for it?
> 
> Because for upstream, the qemu-iotests case 055 still failed.

Kevin, would you like to take this or should I do it?

Fam

> 
> Thanks.
> 
> Jing
> 
> 
> On 12/16/2016 01:20 PM, QingFeng Hao wrote:
> > The problem was triggered by qemu-iotests case 055. It failed when it
> > was comparing the compressed vmdk image with original test.img.
> > 
> > The cause is that buf_len in vmdk_write_extent wasn't converted to
> > little-endian before it was stored to disk. But later vmdk_read_extent
> > read it and converted it from little-endian to cpu endian.
> > If the cpu is big-endian like s390, the problem will happen and
> > the data length read by vmdk_read_extent will become invalid!
> > The fix is to add the conversion in vmdk_write_extent, meanwhile,
> > repair the endianness problem of lba field which shall also be converted
> > to little-endian before storing to disk.
> > 
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: QingFeng Hao 
> > Signed-off-by: Jing Liu 
> > Signed-off-by: Kevin Wolf 
> > Reviewed-by: Fam Zheng 
> > ---
> >   block/vmdk.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index a11c27a..26e5f95 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1354,8 +1354,8 @@ static int vmdk_write_extent(VmdkExtent *extent, 
> > int64_t cluster_offset,
> >   goto out;
> >   }
> > 
> > -data->lba = offset >> BDRV_SECTOR_BITS;
> > -data->size = buf_len;
> > +data->lba = cpu_to_le64(offset >> BDRV_SECTOR_BITS);
> > +data->size = cpu_to_le32(buf_len);
> > 
> >   n_bytes = buf_len + sizeof(VmdkGrainMarker);
> >   iov = (struct iovec) {
> 
> 

Re: [Qemu-devel] about post copy recovery

[Li, Liang Z]

> * Li, Liang Z (liang.z...@intel.com) wrote:
> >
> > Hi David,
> >
> > I remembered some guys wanted to solve the issue of post copy recovery
> when network broken down, do you know latest status?
> 
> Hi Liang,
>   Yes, Haris looked at it as part of GSoC, the latest version is what was 
> posted:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg03468.html
> 
> I've not done any work on it since then;  there are a couple of hard problems
> to be solved.  The simpler is making sure that we always correctly detect a
> migration error due to networking (rather than some other non-recoverable
> error); there's lots of migration code that doesn't check for a file error
> straight away and only hits the error code later on when it's too late to
> recover.
> 
> The harder problem is that we often end up with the case where the main
> thread is blocked trying to access postcopied-RAM, e.g. an emulated
> network driver tries to write an incoming packet to guest RAM but finds the
> guest RAM hasn't arrived yet.
> With the main thread blocked it's very difficult to recover - we can't issue 
> any
> commands to trigger the recovery and even if we could we'll have to be very
> careful about what things those commands need the main thread to do.
> 
> Dave

Thanks for your information!
I will take a look first, maybe get back to you later.

Liang

Re: [Qemu-devel] [RFC PATCH 01/17] powerpc/cpu-models: rename ISAv3.00 logical PVR definition

[David Gibson]

On Fri, Jan 13, 2017 at 05:28:07PM +1100, Suraj Jitindar Singh wrote:
> This logical PVR value now corresponds to ISA version 3.00 so rename it
> accordingly.
> 
> Signed-off-by: Suraj Jitindar Singh 

This one stands on its own, so I've applied it to ppc-for-2.9 already.

> ---
>  target/ppc/cpu-models.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu-models.h b/target/ppc/cpu-models.h
> index aafbbd7..d587e69 100644
> --- a/target/ppc/cpu-models.h
> +++ b/target/ppc/cpu-models.h
> @@ -601,7 +601,7 @@ enum {
>  CPU_POWERPC_LOGICAL_2_06   = 0x0F03,
>  CPU_POWERPC_LOGICAL_2_06_PLUS  = 0x0F13,
>  CPU_POWERPC_LOGICAL_2_07   = 0x0F04,
> -CPU_POWERPC_LOGICAL_2_08   = 0x0F05,
> +CPU_POWERPC_LOGICAL_3_00   = 0x0F05,
>  };
>  
>  /* System version register (used on MPC 8xxx)
> */

-- 
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 V4 net-next] vhost_net: device IOTLB support

[Jason Wang]

On 2017年01月14日 00:30, Michael S. Tsirkin wrote:

On Fri, Jan 13, 2017 at 10:45:09AM +0800, Jason Wang wrote:


On 2017年01月12日 22:17, Michael S. Tsirkin wrote:

On Wed, Jan 11, 2017 at 12:32:12PM +0800, Jason Wang wrote:

This patches implements Device IOTLB support for vhost kernel. This is
done through:

1) switch to use dma helpers when map/unmap vrings from vhost codes
2) introduce a set of VhostOps to:
 - setting up device IOTLB request callback
 - processing device IOTLB request
 - processing device IOTLB invalidation
2) kernel support for Device IOTLB API:

- allow vhost-net to query the IOMMU IOTLB entry through eventfd
- enable the ability for qemu to update a specified mapping of vhost
- through ioctl.
- enable the ability to invalidate a specified range of iova for the
device IOTLB of vhost through ioctl. In x86/intel_iommu case this is
triggered through iommu memory region notifier from device IOTLB
invalidation descriptor processing routine.

With all the above, kernel vhost_net can co-operate with userspace
IOMMU. For vhost-user, the support could be easily done on top by
implementing the VhostOps.

Cc: Michael S. Tsirkin
Signed-off-by: Jason Wang

Applied, thanks!


---
Changes from V4:
- set iotlb callback only when IOMMU_PLATFORM is negotiated (fix
vhost-user qtest failure)

In fact this only checks virtio_host_has_feature - which is
the right thing to do, we can't trust the guest.


- whitelist VIRTIO_F_IOMMU_PLATFORM instead of manually add it
- keep cpu_physical_memory_map() in vhost_memory_map()

One further enhancement might be to detect that guest disabled
iommu (e.g. globally, or using iommu=pt) and disable
the iotlb to avoid overhead for guests which use DPDK
for assigned devices but not for vhost.



Yes, it's in my todo list.

Thanks

Something that I just noticed is that when user requests iommu_platform
but vhost can not provide it, this patches will just let vhost continue
without.  I think that's wrong, since iommu_platform is a security
feature, when it's not supported I think we should fail init.



Let me post a fix for this.

Thanks

Re: [Qemu-devel] [PATCH v3 1/1] block/vmdk: Fix the endian problem of buf_len and lba

[liujing]

Dears,

We would like to know if this patch will be pulled

into upstream or what else we need to do for it?

Because for upstream, the qemu-iotests case 055 still failed.

Thanks.

Jing


On 12/16/2016 01:20 PM, QingFeng Hao wrote:

The problem was triggered by qemu-iotests case 055. It failed when it
was comparing the compressed vmdk image with original test.img.

The cause is that buf_len in vmdk_write_extent wasn't converted to
little-endian before it was stored to disk. But later vmdk_read_extent
read it and converted it from little-endian to cpu endian.
If the cpu is big-endian like s390, the problem will happen and
the data length read by vmdk_read_extent will become invalid!
The fix is to add the conversion in vmdk_write_extent, meanwhile,
repair the endianness problem of lba field which shall also be converted
to little-endian before storing to disk.

Cc: qemu-sta...@nongnu.org
Signed-off-by: QingFeng Hao 
Signed-off-by: Jing Liu 
Signed-off-by: Kevin Wolf 
Reviewed-by: Fam Zheng 
---
  block/vmdk.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index a11c27a..26e5f95 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1354,8 +1354,8 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t 
cluster_offset,
  goto out;
  }

-data->lba = offset >> BDRV_SECTOR_BITS;
-data->size = buf_len;
+data->lba = cpu_to_le64(offset >> BDRV_SECTOR_BITS);
+data->size = cpu_to_le32(buf_len);

  n_bytes = buf_len + sizeof(VmdkGrainMarker);
  iov = (struct iovec) {

Re: [Qemu-devel] [PATCH v4] monitor: Fix crashes when using HMP commands without CPU

[David Gibson]

On Fri, Jan 13, 2017 at 01:12:35PM +0100, Thomas Huth wrote:
> When running certain HMP commands ("info registers", "info cpustats",
> "info tlb", "nmi", "memsave" or dumping virtual memory) with the "none"
> machine, QEMU crashes with a segmentation fault. This happens because the
> "none" machine does not have any CPUs by default, but these HMP commands
> did not check for a valid CPU pointer yet. Add such checks now, so we get
> an error message about the missing CPU instead.
> 
> Signed-off-by: Thomas Huth 

ppc portion

Acked-by: David Gibson 

> ---
>  v4:
>  - Added some more target-specifc checks (for "info tlb" for example)
>  v3:
>  - Use UNASSIGNED_CPU_INDEX instead of hard-coded -1
>  v2:
>  - Added more checks to cover "nmi" and "memsave", too
> 
>  hmp.c   |  8 +++-
>  monitor.c   | 42 ++
>  target/i386/monitor.c   | 16 +++-
>  target/ppc/monitor.c|  4 
>  target/sh4/monitor.c|  5 +
>  target/sparc/monitor.c  |  4 
>  target/xtensa/monitor.c |  4 
>  7 files changed, 73 insertions(+), 10 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index b869617..b1c503a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1013,8 +1013,14 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
>  const char *filename = qdict_get_str(qdict, "filename");
>  uint64_t addr = qdict_get_int(qdict, "val");
>  Error *err = NULL;
> +int cpu_index = monitor_get_cpu_index();
>  
> -qmp_memsave(addr, size, filename, true, monitor_get_cpu_index(), );
> +if (cpu_index < 0) {
> +monitor_printf(mon, "No CPU available\n");
> +return;
> +}
> +
> +qmp_memsave(addr, size, filename, true, cpu_index, );
>  hmp_handle_error(mon, );
>  }
>  
> diff --git a/monitor.c b/monitor.c
> index 0841d43..bf488e9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1025,6 +1025,9 @@ int monitor_set_cpu(int cpu_index)
>  CPUState *mon_get_cpu(void)
>  {
>  if (!cur_mon->mon_cpu) {
> +if (!first_cpu) {
> +return NULL;
> +}
>  monitor_set_cpu(first_cpu->cpu_index);
>  }
>  cpu_synchronize_state(cur_mon->mon_cpu);
> @@ -1033,17 +1036,27 @@ CPUState *mon_get_cpu(void)
>  
>  CPUArchState *mon_get_cpu_env(void)
>  {
> -return mon_get_cpu()->env_ptr;
> +CPUState *cs = mon_get_cpu();
> +
> +return cs ? cs->env_ptr : NULL;
>  }
>  
>  int monitor_get_cpu_index(void)
>  {
> -return mon_get_cpu()->cpu_index;
> +CPUState *cs = mon_get_cpu();
> +
> +return cs ? cs->cpu_index : UNASSIGNED_CPU_INDEX;
>  }
>  
>  static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>  {
> -cpu_dump_state(mon_get_cpu(), (FILE *)mon, monitor_fprintf, 
> CPU_DUMP_FPU);
> +CPUState *cs = mon_get_cpu();
> +
> +if (!cs) {
> +monitor_printf(mon, "No CPU available\n");
> +return;
> +}
> +cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
>  }
>  
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> @@ -1076,7 +1089,13 @@ static void hmp_info_history(Monitor *mon, const QDict 
> *qdict)
>  
>  static void hmp_info_cpustats(Monitor *mon, const QDict *qdict)
>  {
> -cpu_dump_statistics(mon_get_cpu(), (FILE *)mon, _fprintf, 0);
> +CPUState *cs = mon_get_cpu();
> +
> +if (!cs) {
> +monitor_printf(mon, "No CPU available\n");
> +return;
> +}
> +cpu_dump_statistics(cs, (FILE *)mon, _fprintf, 0);
>  }
>  
>  static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
> @@ -1235,6 +1254,12 @@ static void memory_dump(Monitor *mon, int count, int 
> format, int wsize,
>  int l, line_size, i, max_digits, len;
>  uint8_t buf[16];
>  uint64_t v;
> +CPUState *cs = mon_get_cpu();
> +
> +if (!cs && (format == 'i' || !is_physical)) {
> +monitor_printf(mon, "Can not dump without CPU\n");
> +return;
> +}
>  
>  if (format == 'i') {
>  int flags = 0;
> @@ -1264,7 +1289,7 @@ static void memory_dump(Monitor *mon, int count, int 
> format, int wsize,
>  flags = msr_le << 16;
>  flags |= env->bfd_mach;
>  #endif
> -monitor_disas(mon, mon_get_cpu(), addr, count, is_physical, flags);
> +monitor_disas(mon, cs, addr, count, is_physical, flags);
>  return;
>  }
>  
> @@ -1303,7 +1328,7 @@ static void memory_dump(Monitor *mon, int count, int 
> format, int wsize,
>  if (is_physical) {
>  cpu_physical_memory_read(addr, buf, l);
>  } else {
> -if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < 0) {
> +if (cpu_memory_rw_debug(cs, addr, buf, l, 0) < 0) {
>  monitor_printf(mon, " Cannot access memory\n");
>  break;
>  }
> @@ -2186,11 +2211,12 @@ expr_error(Monitor *mon, const char *fmt, ...)
>  static int get_monitor_def(target_long *pval, const char 

[Qemu-devel] [Bug 1656711] [NEW] GTK3 interface doesn't zoom-to-fit by default

[Josh Triplett]

Public bug reported:

The SDL interface automatically scales the video output to
match the window size.  The GTK3 interface has an off-by-default option
"Zoom To Fit" for that.  As far as I can tell, no command-line option
exists to turn that option on.  That makes it harder to quickly zoom a
freshly launched VM; instead of just hitting a maximize-window hotkey, I
also have to navigate through the menu to select "Zoom To Fit".

Given that VMs typically start out running in a much lower-resolution
video mode than the host (and VMs not running a full graphical
environment often stay that way), this seriously impacts the usability
of qemu-system.

(Observed in QEMU 2.8)

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  GTK3 interface doesn't zoom-to-fit by default

Status in QEMU:
  New

Bug description:
  The SDL interface automatically scales the video output to
  match the window size.  The GTK3 interface has an off-by-default option
  "Zoom To Fit" for that.  As far as I can tell, no command-line option
  exists to turn that option on.  That makes it harder to quickly zoom a
  freshly launched VM; instead of just hitting a maximize-window hotkey, I
  also have to navigate through the menu to select "Zoom To Fit".

  Given that VMs typically start out running in a much lower-resolution
  video mode than the host (and VMs not running a full graphical
  environment often stay that way), this seriously impacts the usability
  of qemu-system.

  (Observed in QEMU 2.8)

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

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

[Josh Triplett]

Public bug reported:

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)

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
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:
  New

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] implementing architectural timers using QEMU timers

[Max Filippov]

On Thu, Jan 12, 2017 at 3:28 AM, Pavel Dovgalyuk  wrote:
>> From: Max Filippov [mailto:jcmvb...@gmail.com]
>> On Tue, Jan 10, 2017 at 12:31 AM, Pavel Dovgalyuk  wrote:
>> >> From: Max Filippov [mailto:jcmvb...@gmail.com]
>> >
>> >> I'm trying to reimplement xtensa CCOUNT (cycle counter) and
>> >> CCOMPARE (CCOUNT-based timer interrupts) using QEMU
>> >> timers. That is CCOUNT value is derived from the
>> >> QEMU_CLOCK_VIRTUAL clock and CCOMPARE interrupts are
>> >> generated from the QEMU_CLOCK_VIRTUAL timer callbacks.
>> >> The code is here:
>> >>   https://github.com/OSLL/qemu-xtensa/commits/xtensa-ccount
>> >>
>> >> I've got the following issues doing that:
>> >>
>> >> - I thought that could be improved in -icount mode, so I tried that.
>> >>   It is better with -icount, but it's still not 100% accurate. That is
>> >>   I was able to observe guest reading QEMU clock value that is
>> >>   past QEMU timer deadline before that timer callback was
>> >>   invoked.
>> >
>> > icount is meant to be 100% accurate.
>> > tcg_get_icount_limit function calculates the deadline before the soonest
>> > virtual timer and executes number of instructions that will fit this
>> > timeout.
>>
>> Ok, looks like what happens in my case is that instruction that
>> sets CCOMPARE and thus changes remaining icount does not
>> cause exit from the cpu_exec. So merely ending TB on
>> QEMU_CLOCK_VIRTUAL timer update is not enough, I need to
>> throw an exception of some kind? Or does the timer code need
>> to take care of that?
>
> Yes, it seems that you should end the block with an exception,
> to allow icount loop recalculate the timeouts.

I've added raising EXCP_YIELD in the next TB:
  http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg02811.html
With that icount timers work perfectly for me.

-- 
Thanks.
-- Max

[Qemu-devel] [PATCH] target/xtensa: tests: clean up interrupt tests

[Max Filippov]

Don't use hardcoded software interrupt masks, use XCHAL macros.
Mask off timer interrupt bits that are not checked for.

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/test_interrupt.S | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/tests/tcg/xtensa/test_interrupt.S 
b/tests/tcg/xtensa/test_interrupt.S
index 334ddab..8766835 100644
--- a/tests/tcg/xtensa/test_interrupt.S
+++ b/tests/tcg/xtensa/test_interrupt.S
@@ -1,5 +1,7 @@
 #include "macros.inc"
 
+#define LSBIT(v) ((v) ^ ((v) & ((v) - 1)))
+
 test_suite interrupt
 
 .macro clear_interrupts
@@ -46,14 +48,17 @@ test soft_disabled
 set_vector kernel, 1f
 clear_interrupts
 
-movia2, 0x80
+movia2, LSBIT(XCHAL_INTTYPE_MASK_SOFTWARE)
 wsr a2, intset
 esync
 rsr a3, interrupt
+movia4, ~XCHAL_INTTYPE_MASK_TIMER
+and a3, a3, a4
 assert  eq, a2, a3
 wsr a2, intclear
 esync
 rsr a3, interrupt
+and a3, a3, a4
 assert  eqi, a3, 0
 j   2f
 1:
@@ -65,10 +70,12 @@ test soft_intenable
 set_vector kernel, 1f
 clear_interrupts
 
-movia2, 0x80
+movia2, LSBIT(XCHAL_INTTYPE_MASK_SOFTWARE)
 wsr a2, intset
 esync
 rsr a3, interrupt
+movia4, ~XCHAL_INTTYPE_MASK_TIMER
+and a3, a3, a4
 assert  eq, a2, a3
 rsila3, 0
 wsr a2, intenable
@@ -82,10 +89,12 @@ test soft_rsil
 set_vector kernel, 1f
 clear_interrupts
 
-movia2, 0x80
+movia2, LSBIT(XCHAL_INTTYPE_MASK_SOFTWARE)
 wsr a2, intset
 esync
 rsr a3, interrupt
+movia4, ~XCHAL_INTTYPE_MASK_TIMER
+and a3, a3, a4
 assert  eq, a2, a3
 wsr a2, intenable
 rsila3, 0
@@ -99,10 +108,12 @@ test soft_waiti
 set_vector kernel, 1f
 clear_interrupts
 
-movia2, 0x80
+movia2, LSBIT(XCHAL_INTTYPE_MASK_SOFTWARE)
 wsr a2, intset
 esync
 rsr a3, interrupt
+movia4, ~XCHAL_INTTYPE_MASK_TIMER
+and a3, a3, a4
 assert  eq, a2, a3
 wsr a2, intenable
 waiti   0
@@ -116,10 +127,12 @@ test soft_user
 set_vector user, 2f
 clear_interrupts
 
-movia2, 0x80
+movia2, LSBIT(XCHAL_INTTYPE_MASK_SOFTWARE)
 wsr a2, intset
 esync
 rsr a3, interrupt
+movia4, ~XCHAL_INTTYPE_MASK_TIMER
+and a3, a3, a4
 assert  eq, a2, a3
 wsr a2, intenable
 
@@ -139,7 +152,7 @@ test soft_priority
 set_vector level3, 2f
 clear_interrupts
 
-movia2, 0x880
+movia2, XCHAL_INTTYPE_MASK_SOFTWARE
 wsr a2, intenable
 rsila3, 0
 esync
@@ -161,7 +174,7 @@ test eps_epc_rfi
 clear_interrupts
 reset_ps
 
-movia2, 0x880
+movia2, XCHAL_INTTYPE_MASK_SOFTWARE
 wsr a2, intenable
 rsila3, 0
 rsr a3, ps
-- 
2.1.4

[Qemu-devel] [PATCH 1/2] target/xtensa: implement MEMCTL SR

[Max Filippov]

MEMCTL SR controls zero overhead loop buffer and number of ways enabled
in L1 caches.

Signed-off-by: Max Filippov 
---
 target/xtensa/cpu.c  |  1 +
 target/xtensa/cpu.h  | 19 +++
 target/xtensa/helper.h   |  1 +
 target/xtensa/op_helper.c| 24 
 target/xtensa/overlay_tool.h | 15 +++
 target/xtensa/translate.c|  8 
 6 files changed, 68 insertions(+)

diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 1c18892..811d878 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -66,6 +66,7 @@ static void xtensa_cpu_reset(CPUState *s)
 XTENSA_OPTION_INTERRUPT) ? 0x1f : 0x10;
 env->sregs[VECBASE] = env->config->vecbase;
 env->sregs[IBREAKENABLE] = 0;
+env->sregs[MEMCTL] = MEMCTL_IL0EN & env->config->memctl_mask;
 env->sregs[CACHEATTR] = 0x;
 env->sregs[ATOMCTL] = xtensa_option_enabled(env->config,
 XTENSA_OPTION_ATOMCTL) ? 0x28 : 0x15;
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index a10f1ef..9a130bd 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -129,6 +129,7 @@ enum {
 ITLBCFG = 91,
 DTLBCFG = 92,
 IBREAKENABLE = 96,
+MEMCTL = 97,
 CACHEATTR = 98,
 ATOMCTL = 99,
 IBREAKA = 128,
@@ -189,6 +190,20 @@ enum {
 #define DBREAKC_SB_LB (DBREAKC_SB | DBREAKC_LB)
 #define DBREAKC_MASK 0x3f
 
+#define MEMCTL_INIT 0x0080
+#define MEMCTL_IUSEWAYS_SHIFT 18
+#define MEMCTL_IUSEWAYS_LEN 5
+#define MEMCTL_IUSEWAYS_MASK 0x007c
+#define MEMCTL_DALLOCWAYS_SHIFT 13
+#define MEMCTL_DALLOCWAYS_LEN 5
+#define MEMCTL_DALLOCWAYS_MASK 0x0003e000
+#define MEMCTL_DUSEWAYS_SHIFT 8
+#define MEMCTL_DUSEWAYS_LEN 5
+#define MEMCTL_DUSEWAYS_MASK 0x1f00
+#define MEMCTL_ISNP 0x4
+#define MEMCTL_DSNP 0x2
+#define MEMCTL_IL0EN 0x1
+
 #define MAX_NAREG 64
 #define MAX_NINTERRUPT 32
 #define MAX_NLEVEL 6
@@ -332,6 +347,10 @@ struct XtensaConfig {
 unsigned nibreak;
 unsigned ndbreak;
 
+unsigned icache_ways;
+unsigned dcache_ways;
+uint32_t memctl_mask;
+
 uint32_t configid[2];
 
 uint32_t clock_freq_khz;
diff --git a/target/xtensa/helper.h b/target/xtensa/helper.h
index 427bdc7..7e14741 100644
--- a/target/xtensa/helper.h
+++ b/target/xtensa/helper.h
@@ -23,6 +23,7 @@ DEF_HELPER_2(wsr_ccount, void, env, i32)
 DEF_HELPER_2(update_ccompare, void, env, i32)
 DEF_HELPER_1(check_interrupts, void, env)
 DEF_HELPER_3(check_atomctl, void, env, i32, i32)
+DEF_HELPER_2(wsr_memctl, void, env, i32)
 
 DEF_HELPER_2(itlb_hit_test, void, env, i32)
 DEF_HELPER_2(wsr_rasid, void, env, i32)
diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c
index 864a8f6..989578a 100644
--- a/target/xtensa/op_helper.c
+++ b/target/xtensa/op_helper.c
@@ -506,6 +506,30 @@ void HELPER(check_atomctl)(CPUXtensaState *env, uint32_t 
pc, uint32_t vaddr)
 }
 }
 
+void HELPER(wsr_memctl)(CPUXtensaState *env, uint32_t v)
+{
+if (xtensa_option_enabled(env->config, XTENSA_OPTION_ICACHE)) {
+if (extract32(v, MEMCTL_IUSEWAYS_SHIFT, MEMCTL_IUSEWAYS_LEN) >
+env->config->icache_ways) {
+deposit32(v, MEMCTL_IUSEWAYS_SHIFT, MEMCTL_IUSEWAYS_LEN,
+  env->config->icache_ways);
+}
+}
+if (xtensa_option_enabled(env->config, XTENSA_OPTION_DCACHE)) {
+if (extract32(v, MEMCTL_DUSEWAYS_SHIFT, MEMCTL_DUSEWAYS_LEN) >
+env->config->dcache_ways) {
+deposit32(v, MEMCTL_DUSEWAYS_SHIFT, MEMCTL_DUSEWAYS_LEN,
+  env->config->dcache_ways);
+}
+if (extract32(v, MEMCTL_DALLOCWAYS_SHIFT, MEMCTL_DALLOCWAYS_LEN) >
+env->config->dcache_ways) {
+deposit32(v, MEMCTL_DALLOCWAYS_SHIFT, MEMCTL_DALLOCWAYS_LEN,
+  env->config->dcache_ways);
+}
+}
+env->sregs[MEMCTL] = v & env->config->memctl_mask;
+}
+
 void HELPER(wsr_rasid)(CPUXtensaState *env, uint32_t v)
 {
 XtensaCPU *cpu = xtensa_env_get_cpu(env);
diff --git a/target/xtensa/overlay_tool.h b/target/xtensa/overlay_tool.h
index b73fd14..bf36b5c 100644
--- a/target/xtensa/overlay_tool.h
+++ b/target/xtensa/overlay_tool.h
@@ -59,6 +59,10 @@
 #define XCHAL_HW_MIN_VERSION 0
 #endif
 
+#ifndef XCHAL_LOOP_BUFFER_SIZE
+#define XCHAL_LOOP_BUFFER_SIZE 0
+#endif
+
 #define XCHAL_OPTION(xchal, qemu) ((xchal) ? XTENSA_OPTION_BIT(qemu) : 0)
 
 #define XTENSA_OPTIONS ( \
@@ -343,6 +347,16 @@
 .nibreak = XCHAL_NUM_IBREAK, \
 .ndbreak = XCHAL_NUM_DBREAK
 
+#define CACHE_SECTION \
+.icache_ways = XCHAL_ICACHE_WAYS, \
+.dcache_ways = XCHAL_DCACHE_WAYS, \
+.memctl_mask = \
+(XCHAL_ICACHE_SIZE ? MEMCTL_IUSEWAYS_MASK : 0) | \
+(XCHAL_DCACHE_SIZE ? \
+ MEMCTL_DALLOCWAYS_MASK | MEMCTL_DUSEWAYS_MASK : 0) | \
+MEMCTL_ISNP | MEMCTL_DSNP | \
+(XCHAL_HAVE_LOOPS && XCHAL_LOOP_BUFFER_SIZE ? MEMCTL_IL0EN : 0)
+
 #define CONFIG_SECTION \
 .configid = { \
 

[Qemu-devel] [PATCH 2/2] target/xtensa: tests: add memctl test

[Max Filippov]

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/test_sr.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/tcg/xtensa/test_sr.S b/tests/tcg/xtensa/test_sr.S
index 4fac46e..42e3e5e 100644
--- a/tests/tcg/xtensa/test_sr.S
+++ b/tests/tcg/xtensa/test_sr.S
@@ -44,6 +44,7 @@ test_end
 
 test_sr acchi, 1
 test_sr acclo, 1
+test_sr /*memctl*/97, 0
 test_sr_mask /*atomctl*/99, 0, 0
 test_sr_mask /*br*/4, 0, 0
 test_sr_mask /*cacheattr*/98, 0, 0
-- 
2.1.4

[Qemu-devel] [PATCH] target/xtensa: fix ICACHE/DCACHE options detection

[Max Filippov]

Configuration overlay does not explicitly say whether there are ICACHE
and DCACHE in the core. Current code uses XCHAL_[ID]CACHE_WAYS to detect
if corresponding cache option is enabled, but that's not correct: on
cores without cache these macros are defined as 1, not as 0.
Check XCHAL_[ID]CACHE_SIZE instead.

Signed-off-by: Max Filippov 
---
 target/xtensa/overlay_tool.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/xtensa/overlay_tool.h b/target/xtensa/overlay_tool.h
index 5357142..b73fd14 100644
--- a/target/xtensa/overlay_tool.h
+++ b/target/xtensa/overlay_tool.h
@@ -92,10 +92,10 @@
 XTENSA_OPTION_HIGH_PRIORITY_INTERRUPT) | \
 XCHAL_OPTION(XCHAL_HAVE_CCOUNT, XTENSA_OPTION_TIMER_INTERRUPT) | \
 /* Local memory, TODO */ \
-XCHAL_OPTION(XCHAL_ICACHE_WAYS, XTENSA_OPTION_ICACHE) | \
+XCHAL_OPTION(XCHAL_ICACHE_SIZE, XTENSA_OPTION_ICACHE) | \
 XCHAL_OPTION(XCHAL_ICACHE_LINE_LOCKABLE, \
 XTENSA_OPTION_ICACHE_INDEX_LOCK) | \
-XCHAL_OPTION(XCHAL_DCACHE_WAYS, XTENSA_OPTION_DCACHE) | \
+XCHAL_OPTION(XCHAL_DCACHE_SIZE, XTENSA_OPTION_DCACHE) | \
 XCHAL_OPTION(XCHAL_DCACHE_LINE_LOCKABLE, \
 XTENSA_OPTION_DCACHE_INDEX_LOCK) | \
 XCHAL_OPTION(XCHAL_UNALIGNED_LOAD_HW, XTENSA_OPTION_HW_ALIGNMENT) | \
-- 
2.1.4

[Qemu-devel] [PATCH 2/7] target/xtensa: support icount

[Max Filippov]

Delimit each instruction that may access timers or IRQ state with
qemu_io_start/qemu_io_end, so that qemu-system-xtensa could be run with
-icount option.

Raise EXCP_YIELD after CCOMPARE reprogramming to let tcg_cpu_exec
recalculate how long this CPU is allowed to run.

RSR now may need to terminate TB, but it can't be done in RSR handler
because the same handler is used for XSR together with WSR handler, which
may also need to terminate TB. Change RSR and WSR handlers return type
to bool indicating whether TB termination is needed (RSR) or has been
done (WSR), and add TB termination after RSR/WSR dispatcher call.

Signed-off-by: Max Filippov 
---
 target/xtensa/cpu.h   |   5 ++
 target/xtensa/op_helper.c |   4 ++
 target/xtensa/translate.c | 179 ++
 3 files changed, 143 insertions(+), 45 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 744af81..a10f1ef 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -382,6 +382,7 @@ typedef struct CPUXtensaState {
 uint32_t ccount_base;
 
 int exception_taken;
+int yield_needed;
 unsigned static_vectors;
 
 /* Watchpoints for DBREAK registers */
@@ -554,6 +555,7 @@ static inline int cpu_mmu_index(CPUXtensaState *env, bool 
ifetch)
 #define XTENSA_TBFLAG_EXCEPTION 0x4000
 #define XTENSA_TBFLAG_WINDOW_MASK 0x18000
 #define XTENSA_TBFLAG_WINDOW_SHIFT 15
+#define XTENSA_TBFLAG_YIELD 0x2
 
 static inline void cpu_get_tb_cpu_state(CPUXtensaState *env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *flags)
@@ -595,6 +597,9 @@ static inline void cpu_get_tb_cpu_state(CPUXtensaState 
*env, target_ulong *pc,
 } else {
 *flags |= 3 << XTENSA_TBFLAG_WINDOW_SHIFT;
 }
+if (env->yield_needed) {
+*flags |= XTENSA_TBFLAG_YIELD;
+}
 }
 
 #include "exec/cpu-all.h"
diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c
index 5e5c7da..864a8f6 100644
--- a/target/xtensa/op_helper.c
+++ b/target/xtensa/op_helper.c
@@ -105,6 +105,9 @@ void HELPER(exception)(CPUXtensaState *env, uint32_t excp)
 CPUState *cs = CPU(xtensa_env_get_cpu(env));
 
 cs->exception_index = excp;
+if (excp == EXCP_YIELD) {
+env->yield_needed = 0;
+}
 if (excp == EXCP_DEBUG) {
 env->exception_taken = 0;
 }
@@ -431,6 +434,7 @@ void HELPER(update_ccompare)(CPUXtensaState *env, uint32_t 
i)
 dcc = (uint64_t)(env->sregs[CCOMPARE + i] - env->sregs[CCOUNT] - 1) + 1;
 timer_mod(env->ccompare[i].timer,
   env->ccount_time + (dcc * 100) / 
env->config->clock_freq_khz);
+env->yield_needed = 1;
 }
 
 void HELPER(check_interrupts)(CPUXtensaState *env)
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index cb42945..96c64d6 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -510,22 +510,31 @@ static bool gen_check_sr(DisasContext *dc, uint32_t sr, 
unsigned access)
 return true;
 }
 
-static void gen_rsr_ccount(DisasContext *dc, TCGv_i32 d, uint32_t sr)
+static bool gen_rsr_ccount(DisasContext *dc, TCGv_i32 d, uint32_t sr)
 {
+if (dc->tb->cflags & CF_USE_ICOUNT) {
+gen_io_start();
+}
 gen_helper_update_ccount(cpu_env);
 tcg_gen_mov_i32(d, cpu_SR[sr]);
+if (dc->tb->cflags & CF_USE_ICOUNT) {
+gen_io_end();
+return true;
+}
+return false;
 }
 
-static void gen_rsr_ptevaddr(DisasContext *dc, TCGv_i32 d, uint32_t sr)
+static bool gen_rsr_ptevaddr(DisasContext *dc, TCGv_i32 d, uint32_t sr)
 {
 tcg_gen_shri_i32(d, cpu_SR[EXCVADDR], 10);
 tcg_gen_or_i32(d, d, cpu_SR[sr]);
 tcg_gen_andi_i32(d, d, 0xfffc);
+return false;
 }
 
-static void gen_rsr(DisasContext *dc, TCGv_i32 d, uint32_t sr)
+static bool gen_rsr(DisasContext *dc, TCGv_i32 d, uint32_t sr)
 {
-static void (* const rsr_handler[256])(DisasContext *dc,
+static bool (* const rsr_handler[256])(DisasContext *dc,
 TCGv_i32 d, uint32_t sr) = {
 [CCOUNT] = gen_rsr_ccount,
 [INTSET] = gen_rsr_ccount,
@@ -533,25 +542,28 @@ static void gen_rsr(DisasContext *dc, TCGv_i32 d, 
uint32_t sr)
 };
 
 if (rsr_handler[sr]) {
-rsr_handler[sr](dc, d, sr);
+return rsr_handler[sr](dc, d, sr);
 } else {
 tcg_gen_mov_i32(d, cpu_SR[sr]);
+return false;
 }
 }
 
-static void gen_wsr_lbeg(DisasContext *dc, uint32_t sr, TCGv_i32 s)
+static bool gen_wsr_lbeg(DisasContext *dc, uint32_t sr, TCGv_i32 s)
 {
 gen_helper_wsr_lbeg(cpu_env, s);
 gen_jumpi_check_loop_end(dc, 0);
+return false;
 }
 
-static void gen_wsr_lend(DisasContext *dc, uint32_t sr, TCGv_i32 s)
+static bool gen_wsr_lend(DisasContext *dc, uint32_t sr, TCGv_i32 s)
 {
 gen_helper_wsr_lend(cpu_env, s);
 gen_jumpi_check_loop_end(dc, 0);
+return false;
 }
 
-static void gen_wsr_sar(DisasContext *dc, uint32_t sr, TCGv_i32 s)
+static bool gen_wsr_sar(DisasContext *dc, uint32_t sr, TCGv_i32 s)

[Qemu-devel] [PATCH 5/7] target/xtensa: tests: fix timer tests

[Max Filippov]

Don't expect that CCOUNT increments are equal to the number of executed
instructions. Verify that timer interrupt does not fire before the
programmed CCOMPARE value and does fire after.

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/test_timer.S | 61 +++
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/tests/tcg/xtensa/test_timer.S b/tests/tcg/xtensa/test_timer.S
index f8c6f74..9e6012d 100644
--- a/tests/tcg/xtensa/test_timer.S
+++ b/tests/tcg/xtensa/test_timer.S
@@ -1,12 +1,22 @@
 #include "macros.inc"
 
+#define CCOUNT_SHIFT 4
+#define WAIT_LOOPS 20
+
+.macro  make_ccount_delta target, delta
+rsr \delta, ccount
+rsr \target, ccount
+sub \delta, \target, \delta
+slli\delta, \delta, CCOUNT_SHIFT
+add \target, \target, \delta
+.endm
+
 test_suite timer
 
 test ccount
 rsr a3, ccount
 rsr a4, ccount
-sub a3, a4, a3
-assert  eqi, a3, 1
+assert  ne, a3, a4
 test_end
 
 test ccompare
@@ -18,18 +28,18 @@ test ccompare
 wsr a2, ccompare1
 wsr a2, ccompare2
 
-movia3, 20
-rsr a2, ccount
-addia2, a2, 20
+make_ccount_delta a2, a15
 wsr a2, ccompare0
-rsr a2, interrupt
-assert  eqi, a2, 0
-loopa3, 1f
-rsr a3, interrupt
-bneza3, 2f
 1:
-test_fail
+rsr a3, interrupt
+rsr a4, ccount
+rsr a5, interrupt
+sub a4, a4, a2
+bgeza4, 2f
+assert  eqi, a3, 0
+j   1b
 2:
+assert  nei, a5, 0
 test_end
 
 test ccompare0_interrupt
@@ -42,9 +52,8 @@ test ccompare0_interrupt
 wsr a2, ccompare1
 wsr a2, ccompare2
 
-movia3, 20
-rsr a2, ccount
-addia2, a2, 20
+movia3, WAIT_LOOPS
+make_ccount_delta a2, a15
 wsr a2, ccompare0
 rsync
 rsr a2, interrupt
@@ -72,9 +81,8 @@ test ccompare1_interrupt
 wsr a2, ccompare0
 wsr a2, ccompare2
 
-movia3, 20
-rsr a2, ccount
-addia2, a2, 20
+movia3, WAIT_LOOPS
+make_ccount_delta a2, a15
 wsr a2, ccompare1
 rsync
 rsr a2, interrupt
@@ -99,9 +107,8 @@ test ccompare2_interrupt
 wsr a2, ccompare0
 wsr a2, ccompare1
 
-movia3, 20
-rsr a2, ccount
-addia2, a2, 20
+movia3, WAIT_LOOPS
+make_ccount_delta a2, a15
 wsr a2, ccompare2
 rsync
 rsr a2, interrupt
@@ -125,11 +132,10 @@ test ccompare_interrupt_masked
 movia2, 0
 wsr a2, ccompare2
 
-movia3, 40
-rsr a2, ccount
-addia2, a2, 20
+movia3, 2 * WAIT_LOOPS
+make_ccount_delta a2, a15
 wsr a2, ccompare1
-addia2, a2, 20
+add a2, a2, a15
 wsr a2, ccompare0
 rsync
 rsr a2, interrupt
@@ -156,11 +162,10 @@ test ccompare_interrupt_masked_waiti
 movia2, 0
 wsr a2, ccompare2
 
-movia3, 40
-rsr a2, ccount
-addia2, a2, 20
+movia3, 2 * WAIT_LOOPS
+make_ccount_delta a2, a15
 wsr a2, ccompare1
-addia2, a2, 20
+add a2, a2, a15
 wsr a2, ccompare0
 rsync
 rsr a2, interrupt
-- 
2.1.4

[Qemu-devel] [PATCH 7/7] target/xtensa: tests: add ccount write tests

[Max Filippov]

Check that CCOUNT SR is writable and that CCOMPARE timers are updated
when CCOUNT is written to.

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/test_timer.S | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/tests/tcg/xtensa/test_timer.S b/tests/tcg/xtensa/test_timer.S
index 844c032..6cda71a 100644
--- a/tests/tcg/xtensa/test_timer.S
+++ b/tests/tcg/xtensa/test_timer.S
@@ -19,6 +19,40 @@ test ccount
 assert  ne, a3, a4
 test_end
 
+test ccount_write
+rsr a3, ccount
+rsr a4, ccount
+sub a4, a4, a3
+movia2, 0x12345678
+wsr a2, ccount
+esync
+rsr a3, ccount
+sub a3, a3, a2
+sllia4, a4, 2
+assert  ltu, a3, a4
+test_end
+
+test ccount_update_deadline
+movia2, 0
+wsr a2, intenable
+rsr a2, interrupt
+wsr a2, intclear
+movia2, 0
+wsr a2, ccompare1
+wsr a2, ccompare2
+movia2, 0x12345678
+wsr a2, ccompare0
+rsr a3, interrupt
+assert  eqi, a3, 0
+movia2, 0x12345677
+wsr a2, ccount
+esync
+nop
+rsr a2, interrupt
+movia3, 1 << XCHAL_TIMER0_INTERRUPT
+assert  eq, a2, a3
+test_end
+
 test ccompare
 movia2, 0
 wsr a2, intenable
-- 
2.1.4

[Qemu-devel] [PATCH 0/2] target/xtensa: implement MEMCTL SR

[Max Filippov]

Hello,

this series implements MEMCTL special register and adds a test for it.

Max Filippov (2):
  target/xtensa: implement MEMCTL SR
  target/xtensa: tests: add memctl test

 target/xtensa/cpu.c  |  1 +
 target/xtensa/cpu.h  | 19 +++
 target/xtensa/helper.h   |  1 +
 target/xtensa/op_helper.c| 24 
 target/xtensa/overlay_tool.h | 15 +++
 target/xtensa/translate.c|  8 
 tests/tcg/xtensa/test_sr.S   |  1 +
 7 files changed, 69 insertions(+)

-- 
2.1.4

[Qemu-devel] [PATCH 4/7] target/xtensa: tests: run tests with icount

[Max Filippov]

Timer tests expect certain determinism in CCOUNT updates and timer
interrupts firing. Run QEMU with -icount to get deterministic results.

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/tcg/xtensa/Makefile b/tests/tcg/xtensa/Makefile
index 7f9f2d9..2882c43 100644
--- a/tests/tcg/xtensa/Makefile
+++ b/tests/tcg/xtensa/Makefile
@@ -5,7 +5,7 @@ CROSS=xtensa-$(CORE)-elf-
 
 ifndef XT
 SIM = ../../../xtensa-softmmu/qemu-system-xtensa
-SIMFLAGS = -M sim -cpu $(CORE) -nographic -semihosting $(EXTFLAGS) -kernel
+SIMFLAGS = -M sim -cpu $(CORE) -nographic -semihosting -icount 7 $(EXTFLAGS) 
-kernel
 SIMDEBUG = -s -S
 else
 SIM = xt-run
-- 
2.1.4

[Qemu-devel] [PATCH 1/7] target/xtensa: refactor CCOUNT/CCOMPARE

[Max Filippov]

Xtensa cores may have a register (CCOUNT) that counts core clock cycles.
It may also have a number of registers (CCOMPAREx); when CCOUNT value
passes the value of CCOMPAREx, timer interrupt x is raised.

Currently xtensa target counts a number of completed instructions and
assumes that for CCOUNT one instruction takes one cycle to complete.
It calls helper function to update CCOUNT register at every TB end and
raise timer interrupts. This scheme works very predictably and doesn't
have noticeable performance impact, but it is hard to use with multiple
synchronized processors, especially with coming MTTCG.

Derive CCOUNT from the virtual simulation time, QEMU_CLOCK_VIRTUAL.
Use native QEMU timers for CCOMPARE timers, one timer for each register.

Signed-off-by: Max Filippov 
---
 hw/xtensa/pic_cpu.c   | 75 +--
 target/xtensa/cpu.h   | 16 ++
 target/xtensa/helper.h|  5 ++--
 target/xtensa/op_helper.c | 33 -
 target/xtensa/translate.c | 43 ---
 5 files changed, 65 insertions(+), 107 deletions(-)

diff --git a/hw/xtensa/pic_cpu.c b/hw/xtensa/pic_cpu.c
index 2bed64f..0e812d7 100644
--- a/hw/xtensa/pic_cpu.c
+++ b/hw/xtensa/pic_cpu.c
@@ -31,22 +31,6 @@
 #include "qemu/log.h"
 #include "qemu/timer.h"
 
-void xtensa_advance_ccount(CPUXtensaState *env, uint32_t d)
-{
-uint32_t old_ccount = env->sregs[CCOUNT] + 1;
-
-env->sregs[CCOUNT] += d;
-
-if (xtensa_option_enabled(env->config, XTENSA_OPTION_TIMER_INTERRUPT)) {
-int i;
-for (i = 0; i < env->config->nccompare; ++i) {
-if (env->sregs[CCOMPARE + i] - old_ccount < d) {
-xtensa_timer_irq(env, i, 1);
-}
-}
-}
-}
-
 void check_interrupts(CPUXtensaState *env)
 {
 CPUState *cs = CPU(xtensa_env_get_cpu(env));
@@ -54,17 +38,6 @@ void check_interrupts(CPUXtensaState *env)
 uint32_t int_set_enabled = env->sregs[INTSET] & env->sregs[INTENABLE];
 int level;
 
-/* If the CPU is halted advance CCOUNT according to the QEMU_CLOCK_VIRTUAL 
time
- * elapsed since the moment when it was advanced last time.
- */
-if (cs->halted) {
-int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-xtensa_advance_ccount(env,
-muldiv64(now - env->halt_clock,
-env->config->clock_freq_khz, 100));
-env->halt_clock = now;
-}
 for (level = env->config->nlevel; level > minlevel; --level) {
 if (env->config->level_mask[level] & int_set_enabled) {
 env->pending_irq_level = level;
@@ -109,49 +82,29 @@ void xtensa_timer_irq(CPUXtensaState *env, uint32_t id, 
uint32_t active)
 qemu_set_irq(env->irq_inputs[env->config->timerint[id]], active);
 }
 
-void xtensa_rearm_ccompare_timer(CPUXtensaState *env)
-{
-int i;
-uint32_t wake_ccount = env->sregs[CCOUNT] - 1;
-
-for (i = 0; i < env->config->nccompare; ++i) {
-if (env->sregs[CCOMPARE + i] - env->sregs[CCOUNT] <
-wake_ccount - env->sregs[CCOUNT]) {
-wake_ccount = env->sregs[CCOMPARE + i];
-}
-}
-env->wake_ccount = wake_ccount;
-timer_mod(env->ccompare_timer, env->halt_clock +
-(uint64_t)(wake_ccount - env->sregs[CCOUNT]) *
-100 / env->config->clock_freq_khz);
-}
-
 static void xtensa_ccompare_cb(void *opaque)
 {
-XtensaCPU *cpu = opaque;
-CPUXtensaState *env = >env;
-CPUState *cs = CPU(cpu);
+XtensaCcompareTimer *ccompare = opaque;
+CPUXtensaState *env = ccompare->env;
+unsigned i = ccompare - env->ccompare;
 
-if (cs->halted) {
-env->halt_clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-xtensa_advance_ccount(env, env->wake_ccount - env->sregs[CCOUNT]);
-if (!cpu_has_work(cs)) {
-env->sregs[CCOUNT] = env->wake_ccount + 1;
-xtensa_rearm_ccompare_timer(env);
-}
-}
+xtensa_timer_irq(env, i, 1);
 }
 
 void xtensa_irq_init(CPUXtensaState *env)
 {
-XtensaCPU *cpu = xtensa_env_get_cpu(env);
-
 env->irq_inputs = (void **)qemu_allocate_irqs(
 xtensa_set_irq, env, env->config->ninterrupt);
-if (xtensa_option_enabled(env->config, XTENSA_OPTION_TIMER_INTERRUPT) &&
-env->config->nccompare > 0) {
-env->ccompare_timer =
-timer_new_ns(QEMU_CLOCK_VIRTUAL, _ccompare_cb, cpu);
+if (xtensa_option_enabled(env->config, XTENSA_OPTION_TIMER_INTERRUPT)) {
+unsigned i;
+
+env->time_base = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+env->ccount_base = env->sregs[CCOUNT];
+for (i = 0; i < env->config->nccompare; ++i) {
+env->ccompare[i].env = env;
+env->ccompare[i].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+xtensa_ccompare_cb, env->ccompare + i);
+}
 }
 }
 
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 77bd9d2..744af81 

[Qemu-devel] [PATCH 0/7] target/xtensa: refactor timers

[Max Filippov]

Hello,

this series reimplements xtensa CCOUNT/CCOMPARE features using QEMU
timers, enables support for running with -icount option and updates
timer tests.

Max Filippov (7):
  target/xtensa: refactor CCOUNT/CCOMPARE
  target/xtensa: support icount
  target/xtensa: don't continue translation after exception
  target/xtensa: tests: run tests with icount
  target/xtensa: tests: fix timer tests
  target/xtensa: tests: replace hardcoded interrupt masks
  target/xtensa: tests: add ccount write tests

 hw/xtensa/pic_cpu.c   |  75 +++---
 target/xtensa/cpu.h   |  21 +++-
 target/xtensa/helper.h|   5 +-
 target/xtensa/op_helper.c |  37 +--
 target/xtensa/translate.c | 225 --
 tests/tcg/xtensa/Makefile |   2 +-
 tests/tcg/xtensa/test_timer.S | 105 +---
 7 files changed, 284 insertions(+), 186 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 3/7] target/xtensa: don't continue translation after exception

[Max Filippov]

There's no point in continuing translating guest instructions once an
unconditional exception is thrown.
There's also no point in updating pc before any instruction is
translated, don't do it.

Signed-off-by: Max Filippov 
---
 target/xtensa/translate.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 96c64d6..7a198fa 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -3152,8 +3152,11 @@ void gen_intermediate_code(CPUXtensaState *env, 
TranslationBlock *tb)
 goto done;
 }
 if (tb->flags & XTENSA_TBFLAG_EXCEPTION) {
-tcg_gen_movi_i32(cpu_pc, dc.pc);
+tcg_gen_insn_start(dc.pc);
+++insn_count;
 gen_exception(, EXCP_DEBUG);
+dc.is_jmp = DISAS_UPDATE;
+goto done;
 }
 
 do {
-- 
2.1.4

[Qemu-devel] [PATCH 6/7] target/xtensa: tests: replace hardcoded interrupt masks

[Max Filippov]

Signed-off-by: Max Filippov 
---
 tests/tcg/xtensa/test_timer.S | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/tcg/xtensa/test_timer.S b/tests/tcg/xtensa/test_timer.S
index 9e6012d..844c032 100644
--- a/tests/tcg/xtensa/test_timer.S
+++ b/tests/tcg/xtensa/test_timer.S
@@ -59,7 +59,7 @@ test ccompare0_interrupt
 rsr a2, interrupt
 assert  eqi, a2, 0
 
-movia2, 0x40
+movia2, 1 << XCHAL_TIMER0_INTERRUPT
 wsr a2, intenable
 rsila2, 0
 loopa3, 1f
@@ -87,7 +87,7 @@ test ccompare1_interrupt
 rsync
 rsr a2, interrupt
 assert  eqi, a2, 0
-movia2, 0x400
+movia2, 1 << XCHAL_TIMER1_INTERRUPT
 wsr a2, intenable
 rsila2, 2
 loopa3, 1f
@@ -113,7 +113,7 @@ test ccompare2_interrupt
 rsync
 rsr a2, interrupt
 assert  eqi, a2, 0
-movia2, 0x2000
+movia2, 1 << XCHAL_TIMER2_INTERRUPT
 wsr a2, intenable
 rsila2, 4
 loopa3, 1f
@@ -141,7 +141,7 @@ test ccompare_interrupt_masked
 rsr a2, interrupt
 assert  eqi, a2, 0
 
-movia2, 0x40
+movia2, 1 << XCHAL_TIMER0_INTERRUPT
 wsr a2, intenable
 rsila2, 0
 loopa3, 1f
@@ -171,7 +171,7 @@ test ccompare_interrupt_masked_waiti
 rsr a2, interrupt
 assert  eqi, a2, 0
 
-movia2, 0x40
+movia2, 1 << XCHAL_TIMER0_INTERRUPT
 wsr a2, intenable
 waiti   0
 test_fail
-- 
2.1.4

[Qemu-devel] [PATCH] target/xtensa: implement RUNSTALL

[Max Filippov]

RUNSTALL signal stalls core execution while it's applied. It is widely
used in multicore configurations to control activity of additional
cores.

Signed-off-by: Max Filippov 
---
 target/xtensa/cpu.c|  3 ++-
 target/xtensa/cpu.h|  3 ++-
 target/xtensa/helper.c | 13 +
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 09b53c7..1c18892 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -47,7 +47,7 @@ static bool xtensa_cpu_has_work(CPUState *cs)
 {
 XtensaCPU *cpu = XTENSA_CPU(cs);
 
-return cpu->env.pending_irq_level;
+return !cpu->env.runstall && cpu->env.pending_irq_level;
 }
 
 /* CPUClass::reset() */
@@ -74,6 +74,7 @@ static void xtensa_cpu_reset(CPUState *s)
 
 env->pending_irq_level = 0;
 reset_mmu(env);
+s->halted = env->runstall;
 }
 
 static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model)
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 6b044d3..77bd9d2 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -366,7 +366,7 @@ typedef struct CPUXtensaState {
 xtensa_tlb_entry itlb[7][MAX_TLB_WAY_SIZE];
 xtensa_tlb_entry dtlb[10][MAX_TLB_WAY_SIZE];
 unsigned autorefill_idx;
-
+bool runstall;
 int pending_irq_level; /* level of last raised IRQ */
 void **irq_inputs;
 QEMUTimer *ccompare_timer;
@@ -469,6 +469,7 @@ static inline void 
xtensa_select_static_vectors(CPUXtensaState *env,
 assert(n < 2);
 env->static_vectors = n;
 }
+void xtensa_runstall(CPUXtensaState *env, bool runstall);
 
 #define XTENSA_OPTION_BIT(opt) (((uint64_t)1) << (opt))
 #define XTENSA_OPTION_ALL (~(uint64_t)0)
diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
index 768b32c..c67d715 100644
--- a/target/xtensa/helper.c
+++ b/target/xtensa/helper.c
@@ -728,3 +728,16 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
CPUXtensaState *env)
 cpu_fprintf(f, "No TLB for this CPU core\n");
 }
 }
+
+void xtensa_runstall(CPUXtensaState *env, bool runstall)
+{
+CPUState *cpu = CPU(xtensa_env_get_cpu(env));
+
+env->runstall = runstall;
+cpu->halted = runstall;
+if (runstall) {
+cpu_interrupt(cpu, CPU_INTERRUPT_HALT);
+} else {
+cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
+}
+}
-- 
2.1.4

[Qemu-devel] [PATCH] target/xtensa: add static vectors selection

[Max Filippov]

Xtensa cores may have two distinct addresses for the static vectors
group. Provide a function to select one of them.

Signed-off-by: Max Filippov 
---
 target/xtensa/cpu.c  |  2 +-
 target/xtensa/cpu.h  | 10 +-
 target/xtensa/overlay_tool.h | 11 ++-
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index e8e9f91..09b53c7 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -60,7 +60,7 @@ static void xtensa_cpu_reset(CPUState *s)
 xcc->parent_reset(s);
 
 env->exception_taken = 0;
-env->pc = env->config->exception_vector[EXC_RESET];
+env->pc = env->config->exception_vector[EXC_RESET0 + env->static_vectors];
 env->sregs[LITBASE] &= ~1;
 env->sregs[PS] = xtensa_option_enabled(env->config,
 XTENSA_OPTION_INTERRUPT) ? 0x1f : 0x10;
diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index 7fe82a3..6b044d3 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -209,7 +209,8 @@ enum {
 
 enum {
 /* Static vectors */
-EXC_RESET,
+EXC_RESET0,
+EXC_RESET1,
 EXC_MEMORY_ERROR,
 
 /* Dynamic vectors */
@@ -373,6 +374,7 @@ typedef struct CPUXtensaState {
 int64_t halt_clock;
 
 int exception_taken;
+unsigned static_vectors;
 
 /* Watchpoints for DBREAK registers */
 struct CPUWatchpoint *cpu_watchpoint[MAX_NDBREAK];
@@ -461,6 +463,12 @@ void reset_mmu(CPUXtensaState *env);
 void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUXtensaState *env);
 void debug_exception_env(CPUXtensaState *new_env, uint32_t cause);
 
+static inline void xtensa_select_static_vectors(CPUXtensaState *env,
+unsigned n)
+{
+assert(n < 2);
+env->static_vectors = n;
+}
 
 #define XTENSA_OPTION_BIT(opt) (((uint64_t)1) << (opt))
 #define XTENSA_OPTION_ALL (~(uint64_t)0)
diff --git a/target/xtensa/overlay_tool.h b/target/xtensa/overlay_tool.h
index e8a7fda..5357142 100644
--- a/target/xtensa/overlay_tool.h
+++ b/target/xtensa/overlay_tool.h
@@ -47,6 +47,14 @@
 #define XCHAL_VECBASE_RESET_VADDR 0
 #endif
 
+#ifndef XCHAL_RESET_VECTOR0_VADDR
+#define XCHAL_RESET_VECTOR0_VADDR XCHAL_RESET_VECTOR_VADDR
+#endif
+
+#ifndef XCHAL_RESET_VECTOR1_VADDR
+#define XCHAL_RESET_VECTOR1_VADDR XCHAL_RESET_VECTOR_VADDR
+#endif
+
 #ifndef XCHAL_HW_MIN_VERSION
 #define XCHAL_HW_MIN_VERSION 0
 #endif
@@ -133,7 +141,8 @@
 #endif
 
 #define EXCEPTION_VECTORS { \
-[EXC_RESET] = XCHAL_RESET_VECTOR_VADDR, \
+[EXC_RESET0] = XCHAL_RESET_VECTOR0_VADDR, \
+[EXC_RESET1] = XCHAL_RESET_VECTOR1_VADDR, \
 WINDOW_VECTORS \
 [EXC_KERNEL] = XCHAL_KERNEL_VECTOR_VADDR, \
 [EXC_USER] = XCHAL_USER_VECTOR_VADDR, \
-- 
2.1.4

[Qemu-devel] [PATCH] Drop duplicate display option documentation

[Samuel Thibault]

The curses and none possibilities are already documented on a separate line,
so documenting it on the sdl line was both unneeded and confusing.

Signed-off-by: Samuel Thibault 

diff --git a/qemu-options.hx b/qemu-options.hx
index c534a2f7f9..9c81d3752d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -927,7 +927,7 @@ ETEXI
 
 DEF("display", HAS_ARG, QEMU_OPTION_display,
 "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
-"[,window_close=on|off][,gl=on|off]|curses|none|\n"
+"[,window_close=on|off][,gl=on|off]\n"
 "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
 "-display vnc=[,]\n"
 "-display curses\n"

Re: [Qemu-devel] [PATCH v5 3/4] migration: disallow migrate_add_blocker during migration

[Greg Kurz]

On Thu, 12 Jan 2017 21:22:33 +0530
Ashijeet Acharya  wrote:

> If a migration is already in progress and somebody attempts
> to add a migration blocker, this should rightly fail.
> 
> Add an errp parameter and a retcode return value to migrate_add_blocker.
> 
> Signed-off-by: John Snow 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/qcow.c  |  7 ++-
>  block/vdi.c   |  7 ++-
>  block/vhdx.c  | 16 ++--
>  block/vmdk.c  |  8 +++-
>  block/vpc.c   | 10 +++---
>  block/vvfat.c | 20 
>  hw/9pfs/9p.c  | 31 ---
>  hw/display/virtio-gpu.c   | 31 ++-
>  hw/intc/arm_gic_kvm.c | 16 ++--
>  hw/intc/arm_gicv3_its_kvm.c   | 19 ---
>  hw/intc/arm_gicv3_kvm.c   | 18 +++---
>  hw/misc/ivshmem.c | 13 +
>  hw/scsi/vhost-scsi.c  | 24 ++--
>  hw/virtio/vhost.c |  7 ++-
>  include/migration/migration.h |  7 ++-
>  migration/migration.c | 38 --
>  stubs/migr-blocker.c  |  3 ++-
>  target-i386/kvm.c | 15 ---
>  18 files changed, 208 insertions(+), 82 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 7540f43..90ebe78 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -104,6 +104,7 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  unsigned int len, i, shift;
>  int ret;
>  QCowHeader header;
> +Error *local_err = NULL;
>  
>  ret = bdrv_pread(bs->file, 0, , sizeof(header));
>  if (ret < 0) {
> @@ -252,7 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  error_setg(>migration_blocker, "The qcow format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> -migrate_add_blocker(s->migration_blocker);
> +ret = migrate_add_blocker(s->migration_blocker, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto fail;
> +}
>  
>  qemu_co_mutex_init(>lock);
>  return 0;
> diff --git a/block/vdi.c b/block/vdi.c
> index 96b78d5..2cb8ef0 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -361,6 +361,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, 
> int flags,
>  VdiHeader header;
>  size_t bmap_size;
>  int ret;
> +Error *local_err = NULL;
>  
>  logout("\n");
>  
> @@ -471,7 +472,11 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  error_setg(>migration_blocker, "The vdi format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> -migrate_add_blocker(s->migration_blocker);
> +ret = migrate_add_blocker(s->migration_blocker, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto fail_free_bmap;
> +}
>  
>  qemu_co_mutex_init(>write_lock);
>  
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 0ba2f0a..77ced7c 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -991,6 +991,16 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  }
>  }
>  
> +/* Disable migration when VHDX images are used */
> +error_setg(>migration_blocker, "The vhdx format used by node '%s' "
> +   "does not support live migration",
> +   bdrv_get_device_or_node_name(bs));
> +ret = migrate_add_blocker(s->migration_blocker, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto fail;
> +}
> +
>  if (flags & BDRV_O_RDWR) {
>  ret = vhdx_update_headers(bs, s, false, NULL);
>  if (ret < 0) {
> @@ -1000,12 +1010,6 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  /* TODO: differencing files */
>  
> -/* Disable migration when VHDX images are used */
> -error_setg(>migration_blocker, "The vhdx format used by node '%s' "
> -   "does not support live migration",
> -   bdrv_get_device_or_node_name(bs));
> -migrate_add_blocker(s->migration_blocker);
> -
>  return 0;
>  fail:
>  vhdx_close(bs);
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a11c27a..f1d75ae 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -941,6 +941,7 @@ static int vmdk_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  int ret;
>  BDRVVmdkState *s = bs->opaque;
>  uint32_t magic;
> +Error *local_err = NULL;
>  
>  buf = vmdk_read_desc(bs->file, 0, errp);
>  if (!buf) {
> @@ -976,7 +977,12 @@ static int vmdk_open(BlockDriverState *bs, QDict 
> 

[Qemu-devel] [Bug 1656676] [NEW] nvram/fw_cfg.c ‘read’ may be used uninitialized

[Medicine Yeh]

Public bug reported:

Commit Number: b6af8ea60282df514f87d32e36afd1c9aeee28c8

The gcc version version 6.3.1 catches a new uninitialized variable in the 
master branch of QEMU on the Github. After looking through the function, it is 
really not properly assigned to a value in a certain path (the else condition 
of assigning read value in the code).
Here is the snippet of the condition assigning value:
if (dma.control & FW_CFG_DMA_CTL_READ) {
read = 1;
} else if (dma.control & FW_CFG_DMA_CTL_SKIP) {
read = 0;
} else {
dma.length = 0;
}

Error (Warning) message is as following:
hw/nvram/fw_cfg.c: In function ‘fw_cfg_dma_transfer’:
hw/nvram/fw_cfg.c:372:16: error: ‘read’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]

Solution:
You can fix this by either assign a proper initial value when defining it, or 
give a proper value in the else condition. 
Sorry that I don't have a patch for this. I'm not sure whether to assign 1 or 0 
in the else condition.

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  nvram/fw_cfg.c ‘read’ may be used uninitialized

Status in QEMU:
  New

Bug description:
  Commit Number: b6af8ea60282df514f87d32e36afd1c9aeee28c8

  The gcc version version 6.3.1 catches a new uninitialized variable in the 
master branch of QEMU on the Github. After looking through the function, it is 
really not properly assigned to a value in a certain path (the else condition 
of assigning read value in the code).
  Here is the snippet of the condition assigning value:
  if (dma.control & FW_CFG_DMA_CTL_READ) {
  read = 1;
  } else if (dma.control & FW_CFG_DMA_CTL_SKIP) {
  read = 0;
  } else {
  dma.length = 0;
  }

  Error (Warning) message is as following:
  hw/nvram/fw_cfg.c: In function ‘fw_cfg_dma_transfer’:
  hw/nvram/fw_cfg.c:372:16: error: ‘read’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]

  Solution:
  You can fix this by either assign a proper initial value when defining it, or 
give a proper value in the else condition. 
  Sorry that I don't have a patch for this. I'm not sure whether to assign 1 or 
0 in the else condition.

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

Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port

[Marcel Apfelbaum]

On 01/13/2017 10:35 AM, Gerd Hoffmann wrote:

  Hi,


Sure, thanks for the pointers!
I suppose I can change later to msi-x in a follow up patch
to avoid introducing new bugs from the beginning :)


I'd use msi-x right from start to avoid any backward compatibility
issues.



Hi Gerd, Michael

I'll post a version with msi-x.

Thanks,
Marcel


cheers,
  Gerd

[Qemu-devel] [PATCH] arm: virt: Fix the segmentation fault when specifying an unsupported CPU

[Shannon Zhao]

From: Shannon Zhao 

For example, using -cpu generic will cause qemu segmentation fault.

Signed-off-by: Shannon Zhao 
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7a03f84..4b301c2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -175,7 +175,7 @@ static bool cpuname_valid(const char *cpu)
 int i;
 
 for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-if (strcmp(cpu, valid_cpus[i]) == 0) {
+if (valid_cpus[i] != NULL && strcmp(cpu, valid_cpus[i]) == 0) {
 return true;
 }
 }
-- 
2.0.4

[Qemu-devel] [PATCH] Makefile: Fix owner and group for qemu-version.h.tmp

[Lin Ma]

By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
In 'make' stage a qemu-version.h.tmp will be generated. If the content of
qemu-version.h.tmp and qemu-version.h aren't consistent, The qemu-version.h.tmp
will be renamed to qemu-version.h. Because of the target FORCE, The same action
will be do again in 'make install' stage.

In 'make install' stage, If there is no qemu-version.h.tmp exists and we run
'make install' with sudo, The owner and group of new qemu-version.h.tmp will be
privileged user/group. When we run 'make' next time, qemu-version.h.tmp can't
be overwrited because of permission issue.

This patch uses 'cp' instead of 'mv' to keep the qemu-version.h.tmp file, So
during the 'sudo make install' stage, new qemu-version.h.tmp's owner and group
wont be set to privileged user/group.

Signed-off-by: Lin Ma 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a8bfb2..1ca45b2 100644
--- a/Makefile
+++ b/Makefile
@@ -184,7 +184,7 @@ qemu-version.h: FORCE
printf '""\n'; \
fi; \
fi) > $@.tmp)
-   $(call quiet-command, cmp -s $@ $@.tmp || mv $@.tmp $@)
+   $(call quiet-command, cmp -s $@ $@.tmp || cp $@.tmp $@)
 
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak
-- 
2.9.2

[Qemu-devel] [PATCH v4 2/2] block/qapi: reduce the execution time of qmp_query_blockstats

[Dou Liyang]

In order to reduce the execution time, this patch optimize
the qmp_query_blockstats():
Remove the next_query_bds function.
Remove the bdrv_query_stats function.
Remove some judgement sentence.

The original qmp_query_blockstats calls next_query_bds to get
the next objects in each loops. In the next_query_bds, it checks
the query_nodes and blk. It also call bdrv_query_stats to get
the stats, In the bdrv_query_stats, it checks blk and bs each
times. This waste more times, which may stall the main loop a
bit. And if the disk is too many and donot use the dataplane
feature, this may affect the performance in main loop thread.

This patch removes that two functions, and makes the structure
clearly.

Signed-off-by: Dou Liyang 
---
 block/qapi.c | 73 
 1 file changed, 29 insertions(+), 44 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index bc622cd..45e9d47 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -456,23 +456,6 @@ static BlockStats *bdrv_query_bds_stats(const 
BlockDriverState *bs,
 return s;
 }
 
-static BlockStats *bdrv_query_stats(BlockBackend *blk,
-const BlockDriverState *bs,
-bool query_backing)
-{
-BlockStats *s;
-
-s = bdrv_query_bds_stats(bs, query_backing);
-
-if (blk) {
-s->has_device = true;
-s->device = g_strdup(blk_name(blk));
-bdrv_query_blk_stats(s->stats, blk);
-}
-
-return s;
-}
-
 BlockInfoList *qmp_query_block(Error **errp)
 {
 BlockInfoList *head = NULL, **p_next = 
@@ -496,42 +479,44 @@ BlockInfoList *qmp_query_block(Error **errp)
 return head;
 }
 
-static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
-   bool query_nodes)
-{
-if (query_nodes) {
-*bs = bdrv_next_node(*bs);
-return !!*bs;
-}
-
-*blk = blk_next(*blk);
-*bs = *blk ? blk_bs(*blk) : NULL;
-
-return !!*blk;
-}
-
 BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
  bool query_nodes,
  Error **errp)
 {
 BlockStatsList *head = NULL, **p_next = 
-BlockBackend *blk = NULL;
-BlockDriverState *bs = NULL;
+BlockBackend *blk;
+BlockDriverState *bs;
 
 /* Just to be safe if query_nodes is not always initialized */
-query_nodes = has_query_nodes && query_nodes;
-
-while (next_query_bds(, , query_nodes)) {
-BlockStatsList *info = g_malloc0(sizeof(*info));
-AioContext *ctx = blk ? blk_get_aio_context(blk)
-  : bdrv_get_aio_context(bs);
+if (has_query_nodes && query_nodes) {
+for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) {
+BlockStatsList *info = g_malloc0(sizeof(*info));
+AioContext *ctx = bdrv_get_aio_context(bs);
 
-aio_context_acquire(ctx);
-info->value = bdrv_query_stats(blk, bs, !query_nodes);
-aio_context_release(ctx);
+aio_context_acquire(ctx);
+info->value = bdrv_query_bds_stats(bs, false);
+aio_context_release(ctx);
 
-*p_next = info;
-p_next = >next;
+*p_next = info;
+p_next = >next;
+}
+} else {
+for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+BlockStatsList *info = g_malloc0(sizeof(*info));
+AioContext *ctx = blk_get_aio_context(blk);
+BlockStats *s;
+
+aio_context_acquire(ctx);
+info->value = s = bdrv_query_bds_stats(blk_bs(blk), true);
+s->has_device = true;
+s->device = g_strdup(blk_name(blk));
+bdrv_query_blk_stats(s->stats, blk);
+aio_context_release(ctx);
+
+info->value = s;
+*p_next = info;
+p_next = >next;
+}
 }
 
 return head;
-- 
2.5.5

[Qemu-devel] [PATCH v4 1/2] block/qapi: reduce the coupling between the bdrv_query_stats and bdrv_query_bds_stats

[Dou Liyang]

The bdrv_query_stats and bdrv_query_bds_stats functions need to call
each other, that increases the coupling. it also makes the program
complicated and makes some unnecessary tests.

Remove the call from bdrv_query_bds_stats to bdrv_query_stats, just
take some recursion to make it clearly.

Avoid testing whether the blk is NULL during querying the bds stats.
It is unnecessary.

Signed-off-by: Dou Liyang 
---
 block/qapi.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a62e862..bc622cd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -357,10 +357,6 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo 
**p_info,
 qapi_free_BlockInfo(info);
 }
 
-static BlockStats *bdrv_query_stats(BlockBackend *blk,
-const BlockDriverState *bs,
-bool query_backing);
-
 static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
 {
 BlockAcctStats *stats = blk_get_stats(blk);
@@ -428,9 +424,18 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, 
BlockBackend *blk)
 }
 }
 
-static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
+static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
  bool query_backing)
 {
+BlockStats *s = NULL;
+
+s = g_malloc0(sizeof(*s));
+s->stats = g_malloc0(sizeof(*s->stats));
+
+if (!bs) {
+return s;
+}
+
 if (bdrv_get_node_name(bs)[0]) {
 s->has_node_name = true;
 s->node_name = g_strdup(bdrv_get_node_name(bs));
@@ -440,14 +445,15 @@ static void bdrv_query_bds_stats(BlockStats *s, const 
BlockDriverState *bs,
 
 if (bs->file) {
 s->has_parent = true;
-s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
+s->parent = bdrv_query_bds_stats(bs->file->bs, query_backing);
 }
 
 if (query_backing && bs->backing) {
 s->has_backing = true;
-s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);
+s->backing = bdrv_query_bds_stats(bs->backing->bs, query_backing);
 }
 
+return s;
 }
 
 static BlockStats *bdrv_query_stats(BlockBackend *blk,
@@ -456,17 +462,13 @@ static BlockStats *bdrv_query_stats(BlockBackend *blk,
 {
 BlockStats *s;
 
-s = g_malloc0(sizeof(*s));
-s->stats = g_malloc0(sizeof(*s->stats));
+s = bdrv_query_bds_stats(bs, query_backing);
 
 if (blk) {
 s->has_device = true;
 s->device = g_strdup(blk_name(blk));
 bdrv_query_blk_stats(s->stats, blk);
 }
-if (bs) {
-bdrv_query_bds_stats(s, bs, query_backing);
-}
 
 return s;
 }
-- 
2.5.5

[Qemu-devel] [PATCH v4 0/2] block/qapi: refactor and optimize the qmp_query_blockstats()

[Dou Liyang]

Change log v3 -> v4:
 1. Develop these into the non-RFC patches.
 2. Fix some comments.
 3. do declarations first.

Change log v2 -> v3:
 1. Remove the unnecessary code for the bdrv_next_node().
 2. Remove the change of the locking rules.
Even if this change can improve the performance, but it may
effect the consistency.

The Qemu uses the qmp_query_blockstats() function to get the
blockstats. As the function has been changed several times, it
becomes more complex and longer.

For the multi-disks guest, if we want to execute I/O operations
and this function at the same time. we can use the dataplane
feature to hold I/O performance does not drop. But, Normally
without this feature, How to reduce the decline in performance?

These patches refactor the qmp_query_blockstats() to make the
code easier to follow, and shorter as follows:

From:

+--+  +-+
 | 1|  | 4.  |
 |next_query_bds|  |bdrv_query_bds_stats +---+
 |  |  | |   |
 +^-+  +-^---+   |
  |  |   |
+-+--+  ++---+   |
| 0. |  | 2. |   |
|qmp_query_blockstats+-->bdrv_query_stats<
||  ||
++  ++---+
 |
   +-v---+
   | 3.  |
   |bdrv_query_blk_stats |
   | |
   +-+

To:

+--+
|  |
   +v---+  |
   +--->  3.|  |
+---+  |   |bdrv_query_bds_stats+--+
| 1.+--+   ||
|   +  ++
|qmp_query_blockstats--+
|   |  |
+---+  |   ++
   |   | 2. |
   +--->|
   |bdrv_query_blk_stats|
   ||
   ++


They also optimize the fuction by reducing its running time.

1. The function running time

the time it takes(ns) in each requests:
the disk numbers | 10| 500
-
before these patches | 19429 | 667722 
after these patches  | 18536 | 627945

2. For the performance
used the dd command likes this to test: 
dd if=date_1.dat of=date_2.dat conv=fsync oflag=direct bs=1k count=100k.

the I/O performance is degraded(%) during the monitor:
the disk numbers | 10| 500
-
before these patches | 1.3   | 14.2
after these patches  | 1.0   | 11.3

Dou Liyang (2):
  block/qapi: reduce the coupling between the bdrv_query_stats and
bdrv_query_bds_stats
  block/qapi: reduce the execution time of qmp_query_blockstats

 block/qapi.c | 95 ++--
 1 file changed, 41 insertions(+), 54 deletions(-)

-- 
2.5.5