Re: [Qemu-devel] [PATCH 2/8] migration: stop allocating andfreeingmemory frequently

[jiang.biao2]

>>> +err = deflate(stream, Z_FINISH);
>>> +if (err != Z_STREAM_END) {
>>> +return -1;
>>> +}
>>> +
>>> +return stream->next_out - dest;
>>> +}
>>> +
>>>
>>> @@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const 
>>> uint8_t *p, size_t size,
>>> return -1;
>>> }
>>> }
>>> -if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *),
>>> -  (Bytef *)p, size, level) != Z_OK) {
>>> +
>>> +blen = qemu_compress_data(stream, f->buf + f->buf_index + 
>>> sizeof(int32_t),
>>> +  blen, p, size);
>> The "level" parameter is never used after the patch, could we just removed 
>> it?
>> On the other hand, deflate() of zlib supports compression level too(by
>> deflateInit(stream, level)), should we just reuse the level properly?  If 
>> not, the
>> *migrate parameter compress_level* will be useless.
> 
> The 'level' has been pushed to @stream:
> +if (deflateInit(_param[i].stream,
> +   migrate_compress_level()) != Z_OK) {
> +goto exit;
> +}
Indeed, I missed that. 
Reviewed-by: Jiang Biao 

Re: [Qemu-devel] [PATCH] migration: Fix block migration flag case

[Peter Xu]

On Fri, Mar 16, 2018 at 08:21:14PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Fix the case where when a migration with a bad protocol is tried,
> we leave the block migration capability set.
> 
> (This is a cut down version of my 'migration: Fix block failure cases'
> where it's other case was fixed by Peter's dd0ee30caeebbd )
> 
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [Qemu-devel] Partial NUMA config

[Alexey Kardashevskiy]

On 13/3/18 1:26 pm, Alexey Kardashevskiy wrote:
> Hi Igor,
> 
> ec78f8114bc4c1 "numa: use possible_cpus for not mapped CPUs check" added a
> warning about "All CPU(s) up to maxcpus should be described in NUMA config,
> ability to start up with partial NUMA mappings is obsoleted and will be
> removed in future" and this is printed when I add a numa node without
> attached CPU like this:
> 
> -numa node,nodeid=0,cpus=0,mem=4G
> -numa node,nodeid=1,mem=131072M
> 
> And the reason for this command line is that I am trying to pass some dodgy
> host RAM (actually belongs to a GPU but directly accessible via a fast
> NVLink, not PCI fabric) which let's say is equally far from all CPUs, at
> least in the host's NUMA config this memory is also not bound to any CPU:
> 
> [aik@aik ~]$ ssh yc02goos numactl -H
> available: 8 nodes (0,8,250-255)
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23
> 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48
> 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63
> node 0 size: 261735 MB
> node 0 free: 258932 MB
> node 8 cpus: 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84
> 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106
> 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125
> 126 127
> node 8 size: 261739 MB
> node 8 free: 261414 MB
> node 250 cpus:
> node 250 size: 15360 MB
> node 250 free: 15359 MB
> node 251 cpus:
> node 251 size: 0 MB
> node 251 free: 0 MB
> node 252 cpus:
> node 252 size: 15360 MB
> node 252 free: 15359 MB
> node 253 cpus:
> node 253 size: 15360 MB
> node 253 free: 15359 MB
> node 254 cpus:
> node 254 size: 15360 MB
> node 254 free: 15359 MB
> node 255 cpus:
> node 255 size: 15360 MB
> node 255 free: 15359 MB
> node distances:
> node   0   8  250  251  252  253  254  255
>   0:  10  40  80  80  80  80  80  80
>   8:  40  10  80  80  80  80  80  80
>  250:  80  80  10  80  80  80  80  80
>  251:  80  80  80  10  80  80  80  80
>  252:  80  80  80  80  10  80  80  80
>  253:  80  80  80  80  80  10  80  80
>  254:  80  80  80  80  80  80  10  80
>  255:  80  80  80  80  80  80  80  10
> 
> 
> I am not sure I'll progress far enough to get this working with VFIO but if
> I do, I'd like to keep an ability to have such a partial config in the
> future. What was the reason for this warning in the first place?

Ping?



-- 
Alexey

Re: [Qemu-devel] [PATCH v5 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Michael S. Tsirkin]

On Sun, Mar 18, 2018 at 06:36:20PM +0800, Wei Wang wrote:
> On 03/16/2018 11:16 PM, Michael S. Tsirkin wrote:
> > On Fri, Mar 16, 2018 at 06:48:28PM +0800, Wei Wang wrote:
> > > The new feature enables the virtio-balloon device to receive hints of
> > > guest free pages from the free page vq.
> > > 
> > > balloon_free_page_start - start guest free page hint reporting.
> > > balloon_free_page_stop - stop guest free page hint reporting.
> > > 
> > > Note: balloon will report pages which were free at the time
> > > of this call. As the reporting happens asynchronously, dirty bit logging
> > > must be enabled before this call is made. Guest reporting must be
> > > disabled before the migration dirty bitmap is synchronized.
> > > 
> > > Signed-off-by: Wei Wang 
> > > Signed-off-by: Liang Li 
> > > CC: Michael S. Tsirkin 
> > > CC: Dr. David Alan Gilbert 
> > > CC: Juan Quintela 
> > > ---
> > >   balloon.c   |  58 +--
> > >   hw/virtio/virtio-balloon.c  | 217 
> > > ++--
> > >   include/hw/virtio/virtio-balloon.h  |  20 ++-
> > >   include/standard-headers/linux/virtio_balloon.h |   7 +
> > >   include/sysemu/balloon.h|  15 +-
> > >   5 files changed, 288 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/balloon.c b/balloon.c
> > > index 6bf0a96..87a0410 100644
> > > --- a/balloon.c
> > > +++ b/balloon.c
> > > @@ -36,6 +36,9 @@
> > > 
> > > +static void *virtio_balloon_poll_free_page_hints(void *opaque)
> > > +{
> > > +VirtQueueElement *elem;
> > > +VirtIOBalloon *dev = opaque;
> > > +VirtQueue *vq = dev->free_page_vq;
> > > +uint32_t id;
> > > +size_t size;
> > > +
> > > +/* The optimization thread runs only when the guest is running. */
> > > +while (runstate_is_running()) {
> > Note that this check is not guaranteed to be correct
> > when checked like this outside BQL.
> > 
> > I think you are better off relying on status
> > callback to synchronise with the backend thread.
> > 
> 
> It's actually OK here, I think we don't need the guarantee. The device is
> just the consumer of the vq, essentially it doesn't have a dependency (i.e.
> won't block or cause errors) on the guest state.
> For example:
> 1) when the device executes "while (runstate_is_running())" and finds that
> the guest is running, it proceeds;
> 2) the guest is stopped immediately right after the "while
> (runstate_is_running())" check;
> 3) the device side execution reaches virtqueue_pop(), and finds
> "elem==NULL", since the driver (provider) stops adding hints. Then it
> continues by going back to "while (runstate_is_running())", and now it finds
> the guest is stopped, and then exits.


OTOH it seems that if thread stops nothing will wake it up
whem vm is restarted. Such bahaviour change across vmstop/vmstart
is unexpected.

> Essentially, this runstate check is just an optimization to the case that
> the driver is stopped to provide hints while the device side optimization
> thread is still polling the empty vq (i.e. effort in vain). Probably, it
> would be better to check the runstate under "elem==NULL":
> 
> while (1) {
> ...
> elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> if (!elem) {
> qemu_spin_unlock(>free_page_lock);
> if (runstate_is_running())
> continue;
> else
> break;
> }
> ...
> }
> 
> 
> > > +dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> > > +} else if (dev->free_page_report_status ==
> > > +   FREE_PAGE_REPORT_S_START) {
> > > +/*
> > > + * Stop the optimization only when it has started. This 
> > > avoids
> > > + * a stale stop sign for the previous command.
> > > + */
> > > +dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > > +qemu_spin_unlock(>free_page_lock);
> > > +break;
> > > +}
> > And else? What if it does not match and status is not start?
> > Don't you need to skip in elem decoding?
> 
> No, actually we don't need "else". Please see the code inside if
> (elem->in_num) below. If the status isn't set to START,
> qemu_guest_free_page_hint will not be called to decode the elem.
> 
> 
> > 
> > > +}
> > > +
> > > +if (elem->in_num) {
> > > +if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START 
> > > &&
> > > +!dev->poison_val) {
> > poison generally disables everything? Add a TODO to handle
> > it in he future pls.
> 
> 
> OK, will add TODO in the commit.
> 
> 
> 
> @@ -368,6 +499,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>  ((ram_addr_t) dev->actual << 
> VIRTIO_BALLOON_PFN_SHIFT),
>

Re: [Qemu-devel] [PATCH 2/8] migration: stop allocating and freeingmemory frequently

[Xiao Guangrong]

On 03/19/2018 09:49 AM, jiang.bi...@zte.com.cn wrote:

Hi, guangrong


+/* return the size after compression, or negative value on error */
+static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len,
+  const uint8_t *source, size_t source_len)
+{
+int err;
+
+err = deflateReset(stream);
+if (err != Z_OK) {
+return -1;
+}
+
+stream->avail_in = source_len;
+stream->next_in = (uint8_t *)source;
+stream->avail_out = dest_len;
+stream->next_out = dest;
+

duplicated code with qemu_uncompress(), would initializing stream outside
of qemu_compress_data() be better? In that case, we could pass much less
parameters down, and avoid the duplicated code. Or could we encapsulate
some struct to ease the case?


There are multiple places to do compression/uncompression in QEMU,
i am going to introduce common functions to cleanup these places,
that can be another patchset later...


+err = deflate(stream, Z_FINISH);
+if (err != Z_STREAM_END) {
+return -1;
+}
+
+return stream->next_out - dest;
+}
+

@@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const 
uint8_t *p, size_t size,
return -1;
}
}
-if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *),
-  (Bytef *)p, size, level) != Z_OK) {
+
+blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
+  blen, p, size);

The "level" parameter is never used after the patch, could we just removed it?
On the other hand, deflate() of zlib supports compression level too(by
deflateInit(stream, level)), should we just reuse the level properly?  If not, 
the
*migrate parameter compress_level* will be useless.


The 'level' has been pushed to @stream:
+if (deflateInit(_param[i].stream,
+   migrate_compress_level()) != Z_OK) {
+goto exit;
+}


+if (blen < 0) {
error_report("Compress Failed!");
return 0;
}

+/* return the size after decompression, or negative value on error */
+static int qemu_uncompress(z_stream *stream, uint8_t *dest, size_t dest_len,
+   uint8_t *source, size_t source_len)

The name of *qemu_uncompress* does not quite match *qemu_compress_data*,
would *qemu_uncompress_data* be better?


It's good to me. will rename it.


Besides, the prototype is not consistent with  *qemu_compress_data* either,
should the -*source- be -const- also here?


Okay.

Thanks!

[Qemu-devel] [PATCH v2] tcg: Really fix cpu_io_recompile

[Richard Henderson]

We have confused the number of instructions that have been
executed in the TB with the number of instructions needed
to repeat the I/O instruction.

We have used cpu_restore_state_from_tb, which means that
the guest pc is pointing to the I/O instruction.  The only
time the answer to the later question is not 1 is when
MIPS or SH4 need to re-execute the branch for the delay
slot as well.

We must rely on cpu->cflags_next_tb to generate the next TB,
as otherwise we have a race condition with other guest cpus
within the TB cache.

Fixes: 0790f86861079b1932679d0f011e431aaf4ee9e2
Signed-off-by: Richard Henderson 
---

My v1 raced with Paolo's pull request, so v2 now fixes Pavel's fix.


r~

---
 accel/tcg/translate-all.c | 37 ++---
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 5ad1b919bc..d4190602d1 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1728,8 +1728,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
 CPUArchState *env = cpu->env_ptr;
 #endif
 TranslationBlock *tb;
-uint32_t n, flags;
-target_ulong pc, cs_base;
+uint32_t n;
 
 tb_lock();
 tb = tb_find_pc(retaddr);
@@ -1737,44 +1736,33 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
 cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
   (void *)retaddr);
 }
-n = cpu->icount_decr.u16.low + tb->icount;
 cpu_restore_state_from_tb(cpu, tb, retaddr);
-/* Calculate how many instructions had been executed before the fault
-   occurred.  */
-n = n - cpu->icount_decr.u16.low;
-/* Generate a new TB ending on the I/O insn.  */
-n++;
+
 /* On MIPS and SH, delay slot instructions can only be restarted if
they were already the first instruction in the TB.  If this is not
the first instruction in a TB then re-execute the preceding
branch.  */
+n = 1;
 #if defined(TARGET_MIPS)
-if ((env->hflags & MIPS_HFLAG_BMASK) != 0 && n > 1) {
+if ((env->hflags & MIPS_HFLAG_BMASK) != 0
+&& env->active_tc.PC != tb->pc) {
 env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
 cpu->icount_decr.u16.low++;
 env->hflags &= ~MIPS_HFLAG_BMASK;
+n = 2;
 }
 #elif defined(TARGET_SH4)
 if ((env->flags & ((DELAY_SLOT | DELAY_SLOT_CONDITIONAL))) != 0
-&& n > 1) {
+&& env->pc != tb->pc) {
 env->pc -= 2;
 cpu->icount_decr.u16.low++;
 env->flags &= ~(DELAY_SLOT | DELAY_SLOT_CONDITIONAL);
+n = 2;
 }
 #endif
-/* This should never happen.  */
-if (n > CF_COUNT_MASK) {
-cpu_abort(cpu, "TB too big during recompile");
-}
 
-pc = tb->pc;
-cs_base = tb->cs_base;
-flags = tb->flags;
-tb_phys_invalidate(tb, -1);
-
-/* Execute one IO instruction without caching
-   instead of creating large TB. */
-cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | CF_NOCACHE | 1;
+/* Generate a new TB executing the I/O insn.  */
+cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | n;
 
 if (tb->cflags & CF_NOCACHE) {
 if (tb->orig_tb) {
@@ -1785,11 +1773,6 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
 tb_remove(tb);
 }
 
-/* Generate new TB instead of the current one. */
-/* FIXME: In theory this could raise an exception.  In practice
-   we have already translated the block once so it's probably ok.  */
-tb_gen_code(cpu, pc, cs_base, flags, curr_cflags() | CF_LAST_IO | n);
-
 /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
  * the first in the TB) then we end up generating a whole new TB and
  *  repeating the fault, which is horribly inefficient.
-- 
2.14.3

[Qemu-devel] [Bug 1754542] Re: colo: vm crash with segmentation fault

[李穗恒]

Hi Zhang Chen，
I follow the https://wiki.qemu.org/Features/COLO, And Vm no crash.
But SVM rebooting constantly after print RESET, PVM normal startup.

Secondary:
{"timestamp": {"seconds": 1521421788, "microseconds": 541058}, "event": 
"RESUME"}
{"timestamp": {"seconds": 1521421808, "microseconds": 493484}, "event": "STOP"}
{"timestamp": {"seconds": 1521421808, "microseconds": 686466}, "event": 
"RESUME"}
{"timestamp": {"seconds": 1521421808, "microseconds": 696152}, "event": 
"RESET", "data": {"guest": true}}
{"timestamp": {"seconds": 1521421808, "microseconds": 740653}, "event": 
"RESET", "data": {"guest": true}}
{"timestamp": {"seconds": 1521421818, "microseconds": 74}, "event": "STOP"}
{"timestamp": {"seconds": 1521421818, "microseconds": 969883}, "event": 
"RESUME"}
{"timestamp": {"seconds": 1521421818, "microseconds": 979986}, "event": 
"RESET", "data": {"guest": true}}
{"timestamp": {"seconds": 1521421819, "microseconds": 22652}, "event": "RESET", 
"data": {"guest": true}}


The command(I run two VM in sample machine):

Primary:
sudo /home/lee/Documents/qemu/x86_64-softmmu/qemu-system-x86_64 -enable-kvm 
-boot c -m 2048 -smp 2 -qmp stdio  -name primary -cpu qemu64,+kvmclock -device 
piix3-usb-uhci -device usb-tablet \
-netdev 
tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown -device 
rtl8139,id=e0,netdev=hn0 \
-chardev socket,id=mirror0,host=192.168.0.33,port=9003,server,nowait \
-chardev socket,id=compare1,host=192.168.0.33,port=9004,server,wait \
-chardev socket,id=compare0,host=192.168.0.33,port=9001,server,nowait \
-chardev socket,id=compare0-0,host=192.168.0.33,port=9001 \
-chardev socket,id=compare_out,host=192.168.0.33,port=9005,server,nowait \
-chardev socket,id=compare_out0,host=192.168.0.33,port=9005 \
-object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0 \
-object filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out \
-object filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 \
-object iothread,id=iothread1 \
-object 
colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0,iothread=iothread1
 \
-drive 
if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,children.0.file.filename=/var/lib/libvirt/images/1.raw,children.0.driver=raw
 -S

Secondary:
sudo /home/lee/Documents/qemu/x86_64-softmmu/qemu-system-x86_64 -boot c -m 2048 
-smp 2 -qmp stdio  -name secondary -enable-kvm -cpu qemu64,+kvmclock \
-device piix3-usb-uhci -device usb-tablet \
-netdev 
tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown \
-device rtl8139,netdev=hn0 \
-chardev socket,id=red0,host=192.168.0.33,port=9003,reconnect=1 \
-chardev socket,id=red1,host=192.168.0.33,port=9004,reconnect=1 \
-object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 \
-object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1 \
-object filter-rewriter,id=rew0,netdev=hn0,queue=all \
-drive 
if=none,id=colo-disk0,file.filename=/var/lib/libvirt/images/2.raw,driver=raw,node-name=node0
 \
-drive 
if=ide,id=active-disk0,driver=replication,mode=secondary,file.driver=qcow2,top-id=active-disk0,file.file.filename=/mnt/ramfs/active_disk.img,file.backing.driver=qcow2,file.backing.file.filename=/mnt/ramfs/hidden_disk.img,file.backing.backing=colo-disk0
 \
-incoming tcp:0:

Secondary:
  {'execute':'qmp_capabilities'}
  { 'execute': 'nbd-server-start',
'arguments': {'addr': {'type': 'inet', 'data': {'host': '192.168.0.33', 
'port': '8889'} } }
  }
  {'execute': 'nbd-server-add', 'arguments': {'device': 'colo-disk0', 
'writable': true } }
  {'execute': 'trace-event-set-state', 'arguments': {'name': 'colo*', 'enable': 
true} }


Primary:
  {'execute':'qmp_capabilities'}
  { 'execute': 'human-monitor-command',
'arguments': {'command-line': 'drive_add -n buddy 
driver=replication,mode=primary,file.driver=nbd,file.host=192.168.0.33,file.port=8889,file.export=colo-disk0,node-name=node0'}}
  { 'execute':'x-blockdev-change', 'arguments':{'parent': 'colo-disk0', 'node': 
'node0' } }
  { 'execute': 'migrate-set-capabilities',
'arguments': {'capabilities': [ {'capability': 'x-colo', 'state': true 
} ] } }
  { 'execute': 'migrate', 'arguments': {'uri': 'tcp:192.168.0.33:' } }

Thanks
Suiheng

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

Title:
  colo:  vm crash with segmentation fault

Status in QEMU:
  New

Bug description:
  I use Arch Linux x86_64
  Zhang Chen's(https://github.com/zhangckid/qemu/tree/qemu-colo-18mar10)
  Following document 'COLO-FT.txt',
  I test colo feature on my hosts

  I run this command
  Primary:
  sudo /usr/local/bin/qemu-system-x86_64 -enable-kvm -m 2048 -smp 2 -qmp stdio 
-name primary \
  -device piix3-usb-uhci \
  -device usb-tablet -netdev tap,id=hn0,vhost=off \
  -device 

Re: [Qemu-devel] [PATCH 2/8] migration: stop allocating and freeingmemory frequently

[jiang.biao2]

Hi, guangrong
> 
> +/* return the size after compression, or negative value on error */
> +static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t 
> dest_len,
> +  const uint8_t *source, size_t source_len)
> +{
> +int err;
> +
> +err = deflateReset(stream);
> +if (err != Z_OK) {
> +return -1;
> +}
> +
> +stream->avail_in = source_len;
> +stream->next_in = (uint8_t *)source;
> +stream->avail_out = dest_len;
> +stream->next_out = dest;
>+
duplicated code with qemu_uncompress(), would initializing stream outside 
of qemu_compress_data() be better? In that case, we could pass much less
parameters down, and avoid the duplicated code. Or could we encapsulate 
some struct to ease the case?
> +err = deflate(stream, Z_FINISH);
> +if (err != Z_STREAM_END) {
> +return -1;
> +}
> +
> +return stream->next_out - dest;
> +}
> +
> 
> @@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const 
> uint8_t *p, size_t size,
> return -1;
> }
> }
> -if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *),
> -  (Bytef *)p, size, level) != Z_OK) {
> +
> +blen = qemu_compress_data(stream, f->buf + f->buf_index + 
> sizeof(int32_t),
> +  blen, p, size);
The "level" parameter is never used after the patch, could we just removed it? 
On the other hand, deflate() of zlib supports compression level too(by 
deflateInit(stream, level)), should we just reuse the level properly?  If not, 
the 
*migrate parameter compress_level* will be useless. 
> +if (blen < 0) {
> error_report("Compress Failed!");
> return 0;
> }
>
> +/* return the size after decompression, or negative value on error */
> +static int qemu_uncompress(z_stream *stream, uint8_t *dest, size_t dest_len,
> +   uint8_t *source, size_t source_len)
The name of *qemu_uncompress* does not quite match *qemu_compress_data*,
would *qemu_uncompress_data* be better?
Besides, the prototype is not consistent with  *qemu_compress_data* either, 
should the -*source- be -const- also here?
> +{
> +int err;
> +
> +err = inflateReset(stream);
> +if (err != Z_OK) {
> +return -1;
> +}
> +
> +stream->avail_in = source_len;
> +stream->next_in = source;
> +stream->avail_out = dest_len;
> +stream->next_out = dest;
> +
> +err = inflate(stream, Z_NO_FLUSH);
> +if (err != Z_STREAM_END) {
> +return -1;
> +}
> +
> +return stream->total_out;
> +}
> +

Jiang
Regards,

[Qemu-devel] [PULL 2/8] tests/boot-serial: Check the 40p machine, too

[David Gibson]

From: Thomas Huth 

The "40p" machine is using the Open Hack'Ware BIOS, just like the "prep"
machine, so we can test it accordingly with the boot-serial tester, too.
While we're at it, also change the strings that we are using for the
"prep" machine, so that this test now also checks some CLI parameters.

Signed-off-by: Thomas Huth 
Reviewed-by: Hervé Poussineau 
Signed-off-by: David Gibson 
---
 tests/boot-serial-test.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index ece25c694f..5b24cd26c1 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -75,11 +75,13 @@ typedef struct testdef {
 static testdef_t tests[] = {
 { "alpha", "clipper", "", "PCI:" },
 { "ppc", "ppce500", "", "U-Boot" },
-{ "ppc", "prep", "", "Open Hack'Ware BIOS" },
+{ "ppc", "prep", "-m 96", "Memory size: 96 MB" },
+{ "ppc", "40p", "-boot d", "Booting from device d" },
 { "ppc", "g3beige", "", "PowerPC,750" },
 { "ppc", "mac99", "", "PowerPC,G4" },
 { "ppc64", "ppce500", "", "U-Boot" },
-{ "ppc64", "prep", "", "Open Hack'Ware BIOS" },
+{ "ppc64", "prep", "-boot e", "Booting from device e" },
+{ "ppc64", "40p", "-m 192", "Memory size: 192 MB" },
 { "ppc64", "mac99", "", "PowerPC,970FX" },
 { "ppc64", "pseries", "", "Open Firmware" },
 { "ppc64", "powernv", "-cpu POWER8", "OPAL" },
-- 
2.14.3

Re: [Qemu-devel] [Qemu-ppc] [PULL 0/9] ppc-for-2.12 queue 20180315

[David Gibson]

On Sat, Mar 17, 2018 at 12:30:58PM +0100, BALATON Zoltan wrote:
> On Sat, 17 Mar 2018, BALATON Zoltan wrote:
> > On Sat, 17 Mar 2018, Peter Maydell wrote:
> > > On 17 March 2018 at 04:02, David Gibson
> > >  wrote:
> > > > On Fri, Mar 16, 2018 at 05:25:04PM +, Peter Maydell wrote:
> > > > > Hi -- this looks like it provokes new runtime error warnings from the
> > > > > clang sanitizer:
> > > > 
> > > > Hrm.  What options do you need to trip these warnings?  Just using
> > > > --cc=clang doesn't give them to me, and using --enable-sanitizers
> > > > gives my piles of unrelated warnings.
> > > 
> > > https://wiki.qemu.org/Testing#clang_UBSan documents the necessary
> > > cflags.
> > > 
> > > > 
> > > > > 
> > > > > TEST: tests/boot-serial-test... (pid=926)
> > > > >   /ppc/boot-serial/ppce500:   
> > > > >  OK
> > > > >   /ppc/boot-serial/prep:  
> > > > >  OK
> > > > >   /ppc/boot-serial/40p:   
> > > > >  OK
> > > > >   /ppc/boot-serial/g3beige:   
> > > > >  OK
> > > > >   /ppc/boot-serial/mac99: 
> > > > >  OK
> > > > >   /ppc/boot-serial/sam460ex:
> > > > > /home/petmay01/linaro/qemu-for-merges/target/ppc/translate.c:2979:15:
> > > > > runtime error: load of value 142, which is not a valid value for type
> > > > > 'bool'
> > > > > OK
> > > > > 
> > > > > TEST: tests/boot-serial-test... (pid=1016)
> > > > >   /ppc64/boot-serial/ppce500: 
> > > > >  OK
> > > > >   /ppc64/boot-serial/prep:
> > > > >  OK
> > > > >   /ppc64/boot-serial/40p: 
> > > > >  OK
> > > > >   /ppc64/boot-serial/mac99:   
> > > > >  OK
> > > > >   /ppc64/boot-serial/pseries: 
> > > > >  OK
> > > > >   /ppc64/boot-serial/powernv: 
> > > > >  OK
> > > > >   /ppc64/boot-serial/sam460ex:
> > > > > /home/petmay01/linaro/qemu-for-merges/target/ppc/translate.c:2979:15:
> > > > > runtime error: load of value 85, which is not a valid value for type
> > > > > 'bool'
> > > > > OK
> > > > > 
> > > > > Looks like you're not initializing ctx->lazy_tlb_flush for all 
> > > > > configs:
> > > > > if (env->mmu_model == POWERPC_MMU_32B ||
> > > > > env->mmu_model == POWERPC_MMU_601 ||
> > > > > (env->mmu_model & POWERPC_MMU_64B))
> > > > > ctx->lazy_tlb_flush = true;
> > > > > 
> > > > > should perhaps be
> > > > > ctx->lazy_tlb_flush =
> > > > > env->mmu_model == POWERPC_MMU_32B ||
> > > > > env->mmu_model == POWERPC_MMU_601 ||
> > > > > (env->mmu_model & POWERPC_MMU_64B);
> > > > > 
> > > > > ?
> > > > 
> > > > Uh.. maybe.. except I don't see anything in the series that would be
> > > > likely to change that behaviour.
> > > 
> > > I imagine it's "tests/boot-serial: Test the sam460ex board" --
> > > this code was previously not being exercised in 'make check',
> > > and now it is.
> > 
> > I'm not sure what could cause this in case of sam460ex. It has PPC440
> > which has POWERPC_MMU_BOOKE but the ppce500 should also have that and a
> > similar u-boot and that does not produce this error. Is there maybe some
> > initialisation of some structure I've missed somewhere? But these
> > DisasContext structs seem to be internal to TCG so I'm not sure what
> > could be missing outside of TCG to avoid this. Could be that the
> > different u-boot version does something that triggers this while the one
> > for ppce500 does not execute code that causes this warning during the
> > test?
> 
> Oops, replied too soon. I've checked e500 and it seems to have
> POWERPC_MMU_BOOKE206 (I thought e500 was BookE but I don't know these very
> well). Only bamboo, virtex-ml507 and sam460ex seem to be POWERPC_MMU_BOOKE
> so if only the sam460ex test is added now and the others were never tested
> then it could be this is the first time this is catched.

Thanks for the pointer.  I've now confirmed that the sam460ex test was
the problem.

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


signature.asc
Description: PGP signature

[Qemu-devel] [PULL 7/8] ppc440_pcix: Change some error_report to qemu_log_mask(LOG_UNIMP, ...)

[David Gibson]

From: BALATON Zoltan 

Using log unimp is more appropriate for these messages and this also
silences them by default so they won't clobber make check output when
tests are added for this board.

Signed-off-by: BALATON Zoltan 
Reviewed-by: Thomas Huth 
Signed-off-by: David Gibson 
---
 hw/ppc/ppc440_pcix.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index ab2626a9de..b1307e6477 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qemu/log.h"
 #include "hw/hw.h"
 #include "hw/ppc/ppc.h"
 #include "hw/ppc/ppc4xx.h"
@@ -286,8 +287,9 @@ static void ppc440_pcix_reg_write4(void *opaque, hwaddr 
addr,
 break;
 
 default:
-error_report("%s: unhandled PCI internal register 0x%lx", __func__,
- (unsigned long)addr);
+qemu_log_mask(LOG_UNIMP,
+  "%s: unhandled PCI internal register 0x%"HWADDR_PRIx"\n",
+  __func__, addr);
 break;
 }
 }
@@ -377,8 +379,9 @@ static uint64_t ppc440_pcix_reg_read4(void *opaque, hwaddr 
addr,
 break;
 
 default:
-error_report("%s: invalid PCI internal register 0x%lx", __func__,
- (unsigned long)addr);
+qemu_log_mask(LOG_UNIMP,
+  "%s: invalid PCI internal register 0x%" HWADDR_PRIx "\n",
+  __func__, addr);
 val = 0;
 }
 
-- 
2.14.3

[Qemu-devel] [PULL 8/8] target/ppc: fix tlbsync to check privilege level depending on GTSE

[David Gibson]

From: Cédric Le Goater 

tlbsync also needs to check the Guest Translation Shootdown Enable
(GTSE) bit in the Logical Partition Control Register (LPCR) to
determine at which privilege level it is running.

See commit c6fd28fd573d ("target/ppc: Update tlbie to check privilege
level based on GTSE")

Signed-off-by: Cédric Le Goater 
Reviewed-by: Suraj Jitindar Singh 
Signed-off-by: David Gibson 
---
 target/ppc/translate.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0a0c090c99..218665b408 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4526,7 +4526,7 @@ static void gen_tlbie(DisasContext *ctx)
 TCGv_i32 t1;
 
 if (ctx->gtse) {
-CHK_SV; /* If gtse is set then tblie is supervisor privileged */
+CHK_SV; /* If gtse is set then tlbie is supervisor privileged */
 } else {
 CHK_HV; /* Else hypervisor privileged */
 }
@@ -4553,7 +4553,12 @@ static void gen_tlbsync(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
 GEN_PRIV;
 #else
-CHK_HV;
+
+if (ctx->gtse) {
+CHK_SV; /* If gtse is set then tlbsync is supervisor privileged */
+} else {
+CHK_HV; /* Else hypervisor privileged */
+}
 
 /* BookS does both ptesync and tlbsync make tlbsync a nop for server */
 if (ctx->insns_flags & PPC_BOOKE) {
-- 
2.14.3

[Qemu-devel] [PULL 3/8] hw/ppc/prep: Fix implicit creation of "-drive if=scsi" devices

[David Gibson]

From: Thomas Huth 

The global hack for creating SCSI devices has recently been removed,
but this apparently broke SCSI devices on some boards that were not
ready for this change yet. For the 40p machine you now get:

$ ppc64-softmmu/qemu-system-ppc64 -M 40p -cdrom x.iso
qemu-system-ppc64: -cdrom x.iso: machine type does not support 
if=scsi,bus=0,unit=2

Fix it by providing a lsi53c810_create() function that takes care
of calling scsi_bus_legacy_handle_cmdline() after creating the
corresponding SCSI controller.

Fixes: 1454509726719e0933c800fad00d6999752688ea
Signed-off-by: Thomas Huth 
Signed-off-by: David Gibson 
---
 hw/ppc/prep.c| 2 +-
 hw/scsi/lsi53c895a.c | 7 +++
 include/hw/pci/pci.h | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 5c78503069..a1e7219db6 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -787,7 +787,7 @@ static void ibm_40p_init(MachineState *machine)
 qdev_prop_set_uint32(dev, "equipment", 0xc0);
 qdev_init_nofail(dev);
 
-pci_create_simple(pci_bus, PCI_DEVFN(1, 0), "lsi53c810");
+lsi53c810_create(pci_bus, PCI_DEVFN(1, 0));
 
 /* XXX: s3-trio at PCI_DEVFN(2, 0) */
 pci_vga_init(pci_bus);
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index f3d4c4d230..160657f4b9 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2279,3 +2279,10 @@ void lsi53c895a_create(PCIBus *bus)
 
 scsi_bus_legacy_handle_cmdline(>bus);
 }
+
+void lsi53c810_create(PCIBus *bus, int devfn)
+{
+LSIState *s = LSI53C895A(pci_create_simple(bus, devfn, "lsi53c810"));
+
+scsi_bus_legacy_handle_cmdline(>bus);
+}
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d8c18c7fa4..e255941b5a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -708,6 +708,7 @@ PCIDevice *pci_create(PCIBus *bus, int devfn, const char 
*name);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
 void lsi53c895a_create(PCIBus *bus);
+void lsi53c810_create(PCIBus *bus, int devfn);
 
 qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
 void pci_set_irq(PCIDevice *pci_dev, int level);
-- 
2.14.3

[Qemu-devel] [PULL 4/8] hw/misc/macio: Mark the macio devices with user_creatable = false

[David Gibson]

From: Thomas Huth 

The macio devices currently cause a crash when the user tries to
instantiate them on a different machine:

$ ppc64-softmmu/qemu-system-ppc64 -device macio-newworld
Unexpected error in qemu_chr_fe_init() at chardev/char-fe.c:222:
qemu-system-ppc64: -device macio-newworld: Device 'serial0' is in use
Aborted (core dumped)

These devices are clearly not intended to be creatable by the user
since they are using serial_hds[] directly in their instance_init
function. So let's mark them with user_creatable = false.

Signed-off-by: Thomas Huth 
Reviewed-by: Mark Cave-Ayland 
Signed-off-by: David Gibson 
---
 hw/misc/macio/macio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index af1bd46b4b..454244f59e 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -406,6 +406,8 @@ static void macio_class_init(ObjectClass *klass, void *data)
 k->class_id = PCI_CLASS_OTHERS << 8;
 dc->props = macio_properties;
 set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+/* Reason: Uses serial_hds in macio_instance_init */
+dc->user_creatable = false;
 }
 
 static const TypeInfo macio_oldworld_type_info = {
-- 
2.14.3

[Qemu-devel] [PULL 6/8] hw/ppc/spapr: Allow "spapr-vlan" as NIC model name beside "ibmveth"

[David Gibson]

From: Thomas Huth 

With the new "--nic" command line parameter option, the "old" way of
specifying a NIC model via the nd_table[] is becoming more prominent
again. But for the pseries "spapr-vlan" device, there is a confusing
discrepancy between the model name that is used for "--device" (i.e.
"spapr-vlan") and the model name that has to be used for "--net nic"
or the new "--nic" parameter (i.e. "ibmveth"). Since "spapr-vlan" is
the "real" name of the device, let's allow "spapr-vlan" to be used
as model name for the nd_table[] entries, too.

Signed-off-by: Thomas Huth 
Reviewed-by: Greg Kurz 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 032d03423f..fba76abee2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2607,10 +2607,11 @@ static void spapr_machine_init(MachineState *machine)
 NICInfo *nd = _table[i];
 
 if (!nd->model) {
-nd->model = g_strdup("ibmveth");
+nd->model = g_strdup("spapr-vlan");
 }
 
-if (strcmp(nd->model, "ibmveth") == 0) {
+if (g_str_equal(nd->model, "spapr-vlan") ||
+g_str_equal(nd->model, "ibmveth")) {
 spapr_vlan_create(spapr->vio_bus, nd);
 } else {
 pci_nic_init_nofail(_table[i], phb->bus, nd->model, NULL);
-- 
2.14.3

[Qemu-devel] [PULL 1/8] sii3112: Remove unneeded exit function

[David Gibson]

From: BALATON Zoltan 

An exit function was mistakenly left here but it's not needed because
the PCI bars are organised differently in this device. Calling this
exit function during device_del was causing an abort with
memory_region_del_subregion: `Assertion subregion->container == mr' failed.

Reported-by: Thomas Huth 
Signed-off-by: BALATON Zoltan 
Signed-off-by: David Gibson 
---
 hw/ide/sii3112.c | 12 
 1 file changed, 12 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index e3896c65b4..743a50ed51 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -327,17 +327,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 qemu_register_reset(sii3112_reset, s);
 }
 
-static void sii3112_pci_exitfn(PCIDevice *dev)
-{
-PCIIDEState *d = PCI_IDE(dev);
-int i;
-
-for (i = 0; i < 2; ++i) {
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].extra_io);
-memory_region_del_subregion(>bmdma_bar, >bmdma[i].addr_ioport);
-}
-}
-
 static void sii3112_pci_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -348,7 +337,6 @@ static void sii3112_pci_class_init(ObjectClass *klass, void 
*data)
 pd->class_id = PCI_CLASS_STORAGE_RAID;
 pd->revision = 1;
 pd->realize = sii3112_pci_realize;
-pd->exit = sii3112_pci_exitfn;
 dc->desc = "SiI3112A SATA controller";
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
-- 
2.14.3

[Qemu-devel] [PULL 5/8] PPC e500: Fix gap between u-boot and kernel

[David Gibson]

From: David Engraf 

This patch moves the gap between u-boot and kernel at the correct location.

Signed-off-by: David Engraf 
Signed-off-by: David Gibson 
---
 hw/ppc/e500.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 2238f963c4..9a85a41362 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1009,6 +1009,10 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 }
 
 cur_base = loadaddr + payload_size;
+if (cur_base < (32 * 1024 * 1024)) {
+/* u-boot occupies memory up to 32MB, so load blobs above */
+cur_base = (32 * 1024 * 1024);
+}
 
 /* Load bare kernel only if no bios/u-boot has been provided */
 if (machine->kernel_filename && !kernel_as_payload) {
@@ -1025,11 +1029,6 @@ void ppce500_init(MachineState *machine, PPCE500Params 
*params)
 cur_base += kernel_size;
 }
 
-if (cur_base < (32 * 1024 * 1024)) {
-/* u-boot occupies memory up to 32MB, so load blobs above */
-cur_base = (32 * 1024 * 1024);
-}
-
 /* Load initrd. */
 if (machine->initrd_filename) {
 initrd_base = (cur_base + INITRD_LOAD_PAD) & ~INITRD_PAD_MASK;
-- 
2.14.3

Re: [Qemu-devel] [PULL 0/9] ppc-for-2.12 queue 20180315

[David Gibson]

On Sat, Mar 17, 2018 at 10:08:26AM +, Peter Maydell wrote:
> On 17 March 2018 at 04:02, David Gibson  wrote:
> > On Fri, Mar 16, 2018 at 05:25:04PM +, Peter Maydell wrote:
> >> Hi -- this looks like it provokes new runtime error warnings from the
> >> clang sanitizer:
> >
> > Hrm.  What options do you need to trip these warnings?  Just using
> > --cc=clang doesn't give them to me, and using --enable-sanitizers
> > gives my piles of unrelated warnings.
> 
> https://wiki.qemu.org/Testing#clang_UBSan documents the necessary
> cflags.

Thanks.

> >> TEST: tests/boot-serial-test... (pid=926)
> >>   /ppc/boot-serial/ppce500:OK
> >>   /ppc/boot-serial/prep:   OK
> >>   /ppc/boot-serial/40p:OK
> >>   /ppc/boot-serial/g3beige:OK
> >>   /ppc/boot-serial/mac99:  OK
> >>   /ppc/boot-serial/sam460ex:
> >> /home/petmay01/linaro/qemu-for-merges/target/ppc/translate.c:2979:15:
> >> runtime error: load of value 142, which is not a valid value for type
> >> 'bool'
> >> OK
> >>
> >> TEST: tests/boot-serial-test... (pid=1016)
> >>   /ppc64/boot-serial/ppce500:  OK
> >>   /ppc64/boot-serial/prep: OK
> >>   /ppc64/boot-serial/40p:  OK
> >>   /ppc64/boot-serial/mac99:OK
> >>   /ppc64/boot-serial/pseries:  OK
> >>   /ppc64/boot-serial/powernv:  OK
> >>   /ppc64/boot-serial/sam460ex:
> >> /home/petmay01/linaro/qemu-for-merges/target/ppc/translate.c:2979:15:
> >> runtime error: load of value 85, which is not a valid value for type
> >> 'bool'
> >> OK
> >>
> >> Looks like you're not initializing ctx->lazy_tlb_flush for all configs:
> >> if (env->mmu_model == POWERPC_MMU_32B ||
> >> env->mmu_model == POWERPC_MMU_601 ||
> >> (env->mmu_model & POWERPC_MMU_64B))
> >> ctx->lazy_tlb_flush = true;
> >>
> >> should perhaps be
> >> ctx->lazy_tlb_flush =
> >> env->mmu_model == POWERPC_MMU_32B ||
> >> env->mmu_model == POWERPC_MMU_601 ||
> >> (env->mmu_model & POWERPC_MMU_64B);
> >>
> >> ?
> >
> > Uh.. maybe.. except I don't see anything in the series that would be
> > likely to change that behaviour.
> 
> I imagine it's "tests/boot-serial: Test the sam460ex board" --
> this code was previously not being exercised in 'make check',
> and now it is.

Yeah, looks like.

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


signature.asc
Description: PGP signature

[Qemu-devel] [PULL 0/8] ppc-for-2.12 queue 20180319

[David Gibson]

The following changes since commit e1e44a9916b4318e943aecd669e096222cb3eaeb:

  Merge remote-tracking branch 'remotes/xtensa/tags/20180316-xtensa' into 
staging (2018-03-17 14:15:03 +)

are available in the Git repository at:

  git://github.com/dgibson/qemu.git tags/ppc-for-2.12-20180319

for you to fetch changes up to 91c60f124e682a78c9a2ef951e8e58ebab6441d0:

  target/ppc: fix tlbsync to check privilege level depending on GTSE 
(2018-03-18 21:03:20 +1100)


ppc patch queue for 2018-03-19

This pull request supersedes the one for 2018-03-15.  The only
difference is one patch is removed, since it exposed some code which
triggers ubsan warnings.

Here's the set of accumulated patches now that we're into soft freeze.
I've split new functionality into a ppc-for-2.13 branch, so this only
has bugfixes.  Well.. and a couple of simple cleanups to make bugfixes
easier, some test improvements and a trivial change to make command
line options more obvious.  I think those are all acceptable for soft
freeze.


BALATON Zoltan (2):
  sii3112: Remove unneeded exit function
  ppc440_pcix: Change some error_report to qemu_log_mask(LOG_UNIMP, ...)

Cédric Le Goater (1):
  target/ppc: fix tlbsync to check privilege level depending on GTSE

David Engraf (1):
  PPC e500: Fix gap between u-boot and kernel

Thomas Huth (4):
  tests/boot-serial: Check the 40p machine, too
  hw/ppc/prep: Fix implicit creation of "-drive if=scsi" devices
  hw/misc/macio: Mark the macio devices with user_creatable = false
  hw/ppc/spapr: Allow "spapr-vlan" as NIC model name beside "ibmveth"

 hw/ide/sii3112.c | 12 
 hw/misc/macio/macio.c|  2 ++
 hw/ppc/e500.c|  9 -
 hw/ppc/ppc440_pcix.c | 11 +++
 hw/ppc/prep.c|  2 +-
 hw/ppc/spapr.c   |  5 +++--
 hw/scsi/lsi53c895a.c |  7 +++
 include/hw/pci/pci.h |  1 +
 target/ppc/translate.c   |  9 +++--
 tests/boot-serial-test.c |  6 --
 10 files changed, 36 insertions(+), 28 deletions(-)

Re: [Qemu-devel] [virtio-dev] Re: [v23 1/2] virtio-crypto: Add virtio crypto device specification

[Michael S. Tsirkin]

On Fri, Mar 16, 2018 at 07:18:45PM +0100, Halil Pasic wrote:
> 
> 
> On 03/16/2018 05:27 PM, Michael S. Tsirkin wrote:
> > On Tue, Jan 09, 2018 at 06:05:41PM +0100, Halil Pasic wrote:
> >>> +\item[\field{max_cipher_key_len}] is the maximum length of cipher key 
> >>> supported by the device.
> >>
> >> I can't find what happens if this limit isn't honored by the driver. 
> >> Moreover
> >> reading it is only SHOULD. Is it undefined behavior or should the device 
> >> reject/fail
> >> such requests? I think in qemu implementation we fail the request.
> >>
> >> This question is only about niceness. We are already good enough, IMHO:
> >> since the implementer of the driver can't be sure what is going to happen
> >> if the driver disregards max_cipher_key_len it is already an implicit
> >> SHOULD.
> > 
> > I am not sure documenting undefined behaviour is always required.
> 
> I kind of agree. But I'm afraid I did not get through my point. It's
> about clarity. The driver supplying a cipher key larger that
> max_cipher_key_len isn't violating any driver normative statement.
> I find it strange make obtaining a piece of configuration a driver
> normative


> but have neither a driver normative that says the driver must
> (or should) operate according to the same (at least in certain)
> circumstances


I agree with that. I think it would be clearer to say
that driver must not (or should not?) submit longer requests
than to say that it should read this field.

> nor a device normative that implicitly educates the driver
> implementer what happens if the driver is acting stupid (see below).

About not saying what happens if driver does something stupid,
we generally don't.


> 
> > We certainly do not do this for all other devices> 
> 
> """
> 5.2.6.2 Device Requirements: Device Operation
> 
> A device MUST set the status byte to VIRTIO_BLK_S_IOERR for a write request 
> if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT write any data.
> """
> 
> I was under the impression, that we sometimes express what is naively
> a driver-requirement (e.g. thou shall not write to a read only
> device) as a device-requirement. This has benefits in my opinion:
> the driver implementer is educated about a certain behavior being a no-no
> and hopefully leading to sane error handling (with a compliant device
> sitting on the other side) --- instead of  offending drivers landing beyond
> the spec (in undefined behavior land) by violating a driver-normative.
> 
> > Reading a field being SHOULD seems reasonable: e.g.
> > driver might read it once and cache it in memory.
> 
> I don't quite understand. Let me quote the normative section
> 
> +\drivernormative{\subsubsection}{Device configuration layout}{Device Types / 
> Crypto Device / Device configuration layout}
> +
> +\begin{itemize*}
> +\item The driver MUST read the \field{status} from the bottom bit of status 
> to check whether the
> +VIRTIO_CRYPTO_S_HW_READY is set, and the driver MUST reread it after 
> device reset.
> +\item The driver MUST NOT transmit any requests to the device if the 
> VIRTIO_CRYPTO_S_HW_READY is not set.
> +\item The driver MUST read \field{max_dataqueues} field to discover the 
> number of data queues the device supports.
> 
> [..]
> 
> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the 
> maximum length of cipher key
> +the device supports.
> 
> Does it mean it's OK for the driver (e.g. after a configuration change
> notification to use a stale) \field{max_cipher_key_len} ) as it is SHOULD read
> bit it's not OK to use a stale \field{max_dataqueues}?
> 
> AFAIU all configuration space stuff eligible for caching, but
> under certain circumstances the cache invalidates and a re-read
> is necessary.

So you are saying why aren't max_dataqueues and
max_cipher_key_len consistent?
It's a valid point.

> > 
> > Halil, could you try to split your comments between requirements
> > for more conformance clauses/clarifications as opposed to
> > defects where it's wrong and does not match actual or
> > expected behaviour?
> 
> Yes. I'm already trying to tag my comments. 'This question is only
>  about niceness. We are already good enough' was supposed to indicate
> that this one is not requirement.

to me most questions look like you have a rather specific wording in mind.
How about instead of asking questions you just propose a
specific fixed wording as your own patch?

That will make things converge faster.


> Do you mean putting these in separate emails?

That's up to you, wasn't very clear to me but maybe it's clear
enough to Mike.

> > 
> > I think spec is better off with some documentation for this
> > device than none at all like today.
> > 
> 
> 
> If the rest community says it's good enough, I won't fight against
> inclusion neither in the repo nor in the next Committee Specification.
> 
> I would %100 agree with you if this were normal documentation.
> The problem with standards is that both correctness and completeness

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Philippe Mathieu-Daudé]

Hi Wolfram,

On 03/13/2018 09:16 PM, Wolfram Sang wrote:
> Hi Philippe,
> 
>>>  static Property at24c_eeprom_props[] = {
>>> -DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
>>> +DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
>>
>> This patch should goes before your 2/3 in your series.
> 
> I don't mind much, but why? My reasoning was "let's first fix the cause
> and then the symptom"?

The '0' case is worst than incorrect, it segfaults, so you are right :)

> 
>> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
> 
> Can do, of course. But won't that give room for regressions because
> people are already using it with lower values?

Your patch already introduce the regression :)

I prefer self-explanatory #defines than magic value, but I see your
point, so if we can not decide a value, can you add a comment to explain
the magic value? I think the clearer is to add a #define with a comment.

> 
> Ideally, we would have a "model" variable. The model type would define
> the size of the memory. The "rom-size" variable could then be kept as is
> (except for the 0 bugfix) or deprecated?

IMO there are too many AT24C eeproms to model, so the "rom-size"
variable is the easiest way. Your patch #2 is simple enough to avoid the
#DIV/0!

> 
> Thanks for the review,
> 
>Wolfram
> 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] target-mips: Add initrd support for the Boston board

[Philippe Mathieu-Daudé]

Hi Aleksandar,

On 03/14/2018 04:17 PM, Aleksandar Rikalo wrote:
> From: Aleksandar Rikalo 
> 
> Add support for initial ramdisk loading for the Mips Boston board.
> 
> Signed-off-by: Aleksandar Rikalo 
> ---
>  hw/mips/boston.c | 54 +-
>  1 file changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index fb23161..67ca54f 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -30,6 +30,7 @@
>  #include "hw/loader-fit.h"
>  #include "hw/mips/cps.h"
>  #include "hw/mips/cpudevs.h"
> +#include "hw/mips/mips.h"
>  #include "hw/pci-host/xilinx-pcie.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
> @@ -333,10 +334,12 @@ static const void *boston_fdt_filter(void *opaque, 
> const void *fdt_orig,
>  {
>  BostonState *s = BOSTON(opaque);
>  MachineState *machine = s->mach;
> -const char *cmdline;
> +GString *cmdline;
>  int err;
>  void *fdt;
>  size_t fdt_sz, ram_low_sz, ram_high_sz;
> +long initrd_size;

   target_ulong initrd_size;

> +ram_addr_t initrd_offset;
>  
>  fdt_sz = fdt_totalsize(fdt_orig) * 2;
>  fdt = g_malloc0(fdt_sz);
> @@ -347,20 +350,53 @@ static const void *boston_fdt_filter(void *opaque, 
> const void *fdt_orig,
>  return NULL;
>  }
>  
> -cmdline = (machine->kernel_cmdline && machine->kernel_cmdline[0])
> -? machine->kernel_cmdline : " ";
> -err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> -if (err < 0) {
> -fprintf(stderr, "couldn't set /chosen/bootargs\n");
> -return NULL;
> -}
> -
>  ram_low_sz = MIN(256 * M_BYTE, machine->ram_size);
>  ram_high_sz = machine->ram_size - ram_low_sz;
>  qemu_fdt_setprop_sized_cells(fdt, "/memory@0", "reg",
>   1, 0x, 1, ram_low_sz,
>   1, 0x9000, 1, ram_high_sz);
>  
> +cmdline = g_string_new(machine->kernel_cmdline);
> +
> +/* load initrd */
> +initrd_offset = 0;
> +if (machine->initrd_filename) {
> +initrd_size = get_image_size(machine->initrd_filename);
> +if (initrd_size > 0) {
> +/* The kernel allocates the bootmap memory in the low memory 
> after
> +   the initrd.  It takes at most 128kiB for 2GB RAM and 4kiB
> +   pages.  */
> +initrd_offset = (ram_low_sz - initrd_size - 131072
> + - ~INITRD_PAGE_MASK) & INITRD_PAGE_MASK;
> +
> +if ((int64_t)cpu_mips_kseg0_to_phys(NULL, *load_addr + fdt_sz)
> +>= (int64_t)initrd_offset) {
> +error_report("memory too small for initial ram disk '%s'",
> + machine->initrd_filename);
> +exit(1);
> +}
> +
> +initrd_size = load_image_targphys(machine->initrd_filename,
> +  initrd_offset,
> +  initrd_size);
> +}
> +if (initrd_size == (target_ulong) -1) {
> +error_report("could not load initial ram disk '%s'",
> + machine->initrd_filename);
> +exit(1);
> +}
> +g_string_append_printf(cmdline, " rd_start=0x%" PRIx64 " 
> rd_size=%li",
> +   cpu_mips_phys_to_kseg0(NULL, initrd_offset),
> +   initrd_size);
> +}
> +
> +err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline->str);
> +g_string_free(cmdline, true);
> +if (err < 0) {
> +fprintf(stderr, "couldn't set /chosen/bootargs\n");

While here, please remove the fprintf():

   error_report("couldn't set /chosen/bootargs");

> +return NULL;
> +}
> +
>  fdt = g_realloc(fdt, fdt_totalsize(fdt));
>  qemu_fdt_dumpdtb(fdt, fdt_sz);
>  
> 

With comment addressed:
Reviewed-by: Philippe Mathieu-Daudé 

Regards,

Phil.

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[Philippe Mathieu-Daudé]

On 03/16/2018 05:23 PM, KONRAD Frederic wrote:
> Since the commit:
> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
> Author: Stefan Hajnoczi 
> Date:   Wed Mar 7 14:42:05 2018 +
> 
> vl: introduce vm_shutdown()
> 
> GDB crash when qemu exits (at least on sparc-softmmu):
> Remote communication error.  Target disconnected.: Connection reset by peer.
> Quitting: putpkt: write failed: Broken pipe.
> 
> So send a packet to kill GDB before we exit QEMU:
> [Inferior 1 (Thread 0) exited normally]
> 
> Signed-off-by: KONRAD Frederic 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  gdbstub.c  | 7 +++
>  include/exec/gdbstub.h | 2 ++
>  vl.c   | 2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index f1d5148..a76b2fa 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2052,6 +2052,13 @@ int gdbserver_start(const char *device)
>  return 0;
>  }
>  
> +void gdbserver_cleanup(void)
> +{
> +if (gdbserver_state) {
> +put_packet(gdbserver_state, "W00");
> +}
> +}
> +
>  static void register_types(void)
>  {
>  type_register_static(_gdb_type_info);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 9aa7756..2e8a4b8 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -103,6 +103,8 @@ int gdbserver_start(int);
>  int gdbserver_start(const char *port);
>  #endif
>  
> +void gdbserver_cleanup(void);
> +
>  /**
>   * gdb_has_xml:
>   * This is an ugly hack to cope with both new and old gdb.
> diff --git a/vl.c b/vl.c
> index 3ef04ce..0427b15 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4723,6 +4723,8 @@ int main(int argc, char **argv, char **envp)
>  
>  main_loop();
>  
> +gdbserver_cleanup();
> +
>  /* No more vcpu or device emulation activity beyond this point */
>  vm_shutdown();
>  
> 

Re: [Qemu-devel] [PATCH] target/m68k: add a mechanism to automatically free TCGv

[Philippe Mathieu-Daudé]

Hi Laurent,

On 03/18/2018 05:12 PM, Laurent Vivier wrote:
> SRC_EA() and gen_extend() can return either a temporary
> TCGv or a memory allocated one. Mark them when they are
> allocated, and free them automatically at end of the
> instruction translation.
> 
> We want to free locally allocated TCGv to avoid
> overflow in sequence like:
> 
>   0xc00ae406:  movel %fp@(-132),%fp@(-268)
>   0xc00ae40c:  movel %fp@(-128),%fp@(-264)
>   0xc00ae412:  movel %fp@(-20),%fp@(-212)
>   0xc00ae418:  movel %fp@(-16),%fp@(-208)
>   0xc00ae41e:  movel %fp@(-60),%fp@(-220)
>   0xc00ae424:  movel %fp@(-56),%fp@(-216)
>   0xc00ae42a:  movel %fp@(-124),%fp@(-252)
>   0xc00ae430:  movel %fp@(-120),%fp@(-248)
>   0xc00ae436:  movel %fp@(-12),%fp@(-260)
>   0xc00ae43c:  movel %fp@(-8),%fp@(-256)
>   0xc00ae442:  movel %fp@(-52),%fp@(-276)
>   0xc00ae448:  movel %fp@(-48),%fp@(-272)
>   ...
> 
> That can fill a lot of TCGv entries in a sequence,
> especially since 15fa08f845 ("tcg: Dynamically allocate TCGOps")
> we have no limit to fill the TCGOps cache and we can fill
> the entire TCG variables array and overflow it.
> 
> Suggested-by: Richard Henderson 
> Signed-off-by: Laurent Vivier 
> ---
>  target/m68k/translate.c | 100 
> +++-
>  1 file changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index cef6f663ad..a660388054 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -123,8 +123,34 @@ typedef struct DisasContext {
>  int done_mac;
>  int writeback_mask;
>  TCGv writeback[8];
> +#define MAX_TO_RELEASE 8
> +int release_count;
> +TCGv release[MAX_TO_RELEASE];
>  } DisasContext;
>  
> +static void init_release_array(DisasContext *s)
> +{
> +#ifdef CONFIG_DEBUG_TCG
> +memset(s->release, 0, sizeof(s->release));
> +#endif
> +s->release_count = 0;
> +}
> +
> +static void do_release(DisasContext *s)
> +{
> +int i;
> +for (i = 0; i < s->release_count; i++) {
> +tcg_temp_free(s->release[i]);
> +}
> +init_release_array(s);
> +}
> +
> +static TCGv mark_to_release(DisasContext *s, TCGv tmp)
> +{
> +g_assert(s->release_count < MAX_TO_RELEASE);
> +return s->release[s->release_count++] = tmp;
> +}
> +
>  static TCGv get_areg(DisasContext *s, unsigned regno)
>  {
>  if (s->writeback_mask & (1 << regno)) {
> @@ -347,7 +373,8 @@ static TCGv gen_ldst(DisasContext *s, int opsize, TCGv 
> addr, TCGv val,
>  gen_store(s, opsize, addr, val, index);
>  return store_dummy;
>  } else {
> -return gen_load(s, opsize, addr, what == EA_LOADS, index);
> +return mark_to_release(s, gen_load(s, opsize, addr,
> +   what == EA_LOADS, index));
>  }
>  }
>  
> @@ -439,7 +466,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>  } else {
>  bd = 0;
>  }
> -tmp = tcg_temp_new();
> +tmp = mark_to_release(s, tcg_temp_new());
>  if ((ext & 0x44) == 0) {
>  /* pre-index */
>  add = gen_addr_index(s, ext, tmp);
> @@ -449,7 +476,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>  if ((ext & 0x80) == 0) {
>  /* base not suppressed */
>  if (IS_NULL_QREG(base)) {
> -base = tcg_const_i32(offset + bd);
> +base = mark_to_release(s, tcg_const_i32(offset + bd));
>  bd = 0;
>  }
>  if (!IS_NULL_QREG(add)) {
> @@ -465,7 +492,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>  add = tmp;
>  }
>  } else {
> -add = tcg_const_i32(bd);
> +add = mark_to_release(s, tcg_const_i32(bd));
>  }
>  if ((ext & 3) != 0) {
>  /* memory indirect */
> @@ -494,7 +521,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, 
> DisasContext *s, TCGv base)
>  }
>  } else {
>  /* brief extension word format */
> -tmp = tcg_temp_new();
> +tmp = mark_to_release(s, tcg_temp_new());
>  add = gen_addr_index(s, ext, tmp);
>  if (!IS_NULL_QREG(base)) {
>  tcg_gen_add_i32(tmp, add, base);
> @@ -617,14 +644,14 @@ static void gen_flush_flags(DisasContext *s)
>  s->cc_op = CC_OP_FLAGS;
>  }
>  
> -static inline TCGv gen_extend(TCGv val, int opsize, int sign)
> +static inline TCGv gen_extend(DisasContext *s, TCGv val, int opsize, int 
> sign)

I'd rather see this done in 2 patches, one adding the DisasContext
argument, and another to free the TCGv.

Split in 2 or as it:
Reviewed-by: Philippe Mathieu-Daudé 

>  {
>  TCGv tmp;
>  
>  if (opsize == OS_LONG) {
>  tmp = val;
>  } else {
> -tmp = tcg_temp_new();
> +tmp = 

Re: [Qemu-devel] [PATCH] tcg: fix cpu_io_recompile

[Philippe Mathieu-Daudé]

On 03/16/2018 04:53 PM, Richard Henderson wrote:
> We have confused the number of instructions that have been
> executed in the TB with the number of instructions needed
> to repeat the I/O instruction.
> 
> We have used cpu_restore_state_from_tb, which means that
> the guest pc is pointing to the I/O instruction.  The only
> time the answer to the later question is not 1 is when
> MIPS or SH4 need to re-execute the branch for the delay
> slot as well.
> 
> Signed-off-by: Richard Henderson 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  accel/tcg/translate-all.c | 21 -
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 67795cd78c..d4190602d1 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1736,37 +1736,32 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t 
> retaddr)
>  cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
>(void *)retaddr);
>  }
> -n = cpu->icount_decr.u16.low + tb->icount;
>  cpu_restore_state_from_tb(cpu, tb, retaddr);
> -/* Calculate how many instructions had been executed before the fault
> -   occurred.  */
> -n = n - cpu->icount_decr.u16.low;
> -/* Generate a new TB ending on the I/O insn.  */
> -n++;
> +
>  /* On MIPS and SH, delay slot instructions can only be restarted if
> they were already the first instruction in the TB.  If this is not
> the first instruction in a TB then re-execute the preceding
> branch.  */
> +n = 1;
>  #if defined(TARGET_MIPS)
> -if ((env->hflags & MIPS_HFLAG_BMASK) != 0 && n > 1) {
> +if ((env->hflags & MIPS_HFLAG_BMASK) != 0
> +&& env->active_tc.PC != tb->pc) {
>  env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
>  cpu->icount_decr.u16.low++;
>  env->hflags &= ~MIPS_HFLAG_BMASK;
> +n = 2;
>  }
>  #elif defined(TARGET_SH4)
>  if ((env->flags & ((DELAY_SLOT | DELAY_SLOT_CONDITIONAL))) != 0
> -&& n > 1) {
> +&& env->pc != tb->pc) {
>  env->pc -= 2;
>  cpu->icount_decr.u16.low++;
>  env->flags &= ~(DELAY_SLOT | DELAY_SLOT_CONDITIONAL);
> +n = 2;
>  }
>  #endif
> -/* This should never happen.  */
> -if (n > CF_COUNT_MASK) {
> -cpu_abort(cpu, "TB too big during recompile");
> -}
>  
> -/* Adjust the execution state of the next TB.  */
> +/* Generate a new TB executing the I/O insn.  */
>  cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | n;
>  
>  if (tb->cflags & CF_NOCACHE) {
> 

Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Philippe Mathieu-Daudé]

On 03/16/2018 06:26 PM, Michael Clark wrote:
> This version uses a constant size memory buffer sized for
> the maximum possible ISA string length. It also uses g_new
> instead of g_new0, uses more efficient logic to append
> extensions and adds manual zero termination of the string.
> 
> Cc: Palmer Dabbelt 
> Cc: Peter Maydell 
> Cc: Philippe Mathieu-Daudé 
> Signed-off-by: Michael Clark 
> ---
>  target/riscv/cpu.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4851890..298abbd 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -391,16 +391,16 @@ static const TypeInfo riscv_cpu_type_info = {
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>  int i;
> -size_t maxlen = 5 + ctz32(cpu->env.misa);
> -char *isa_string = g_new0(char, maxlen);
> -snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS);
> +const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1;
> +char *isa_str = g_new(char, maxlen);
> +char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
>  for (i = 0; i < sizeof(riscv_exts); i++) {
>  if (cpu->env.misa & RV(riscv_exts[i])) {
> -isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a';
> -
> +*p++ = riscv_exts[i] - 'A' + 'a';
 /* tolower() */

I prefer unobfuscated code (think newcomers and reviewers) :|

Reviewed-by: Philippe Mathieu-Daudé 

>  }
>  }
> -return isa_string;
> +*p = '\0';
> +return isa_str;
>  }
>  
>  void riscv_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> 

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Philippe Mathieu-Daudé]

Hi Aaron,

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> This is a bug fix to ensure 64-bit reads of this register don't read
> adjacent data.
> 
> Signed-off-by: Aaron Lindsay 
> ---
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9c3b5ef..fb2f983 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -367,7 +367,7 @@ typedef struct CPUARMState {
>  uint32_t c9_data;
>  uint64_t c9_pmcr; /* performance monitor control register */
>  uint64_t c9_pmcnten; /* perf monitor counter enables */
> -uint32_t c9_pmovsr; /* perf monitor overflow status */
> +uint64_t c9_pmovsr; /* perf monitor overflow status */

This doesn't look correct, since this reg is 32b.

I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:

{ .name = "PMOVSR", ...
- ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+ ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
  .accessfn = pmreg_access,
  .writefn = pmovsr_write,
  .raw_writefn = raw_write },

>  uint32_t c9_pmuserenr; /* perf monitor user enable */
>  uint64_t c9_pmselr; /* perf monitor counter selection register */
>  uint64_t c9_pminten; /* perf monitor interrupt enables */
> 

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Philippe Mathieu-Daudé]

On 03/18/2018 11:35 PM, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
>> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
>> pmceid[01] are already being initialized to zero for both A15 and A57.
>>
>> Signed-off-by: Aaron Lindsay 
>> ---
>>  target/arm/cpu64.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 991d764..8c4db31 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
>>  cpu->id_isar5 = 0x00011121;
>>  cpu->id_aa64pfr0 = 0x;
>>  cpu->id_aa64dfr0 = 0x10305106;
>> +cpu->pmceid0 = 0x;
>> +cpu->pmceid1 = 0x;
>>  cpu->id_aa64isar0 = 0x00011120;
>>  cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
>>  cpu->dbgdidr = 0x3516d000;
>>
> 
> Maybe we can move this at a single place in arm_cpu_post_init():

Err, arm_cpu_reset() :)

> 
> if (arm_feature(>env, ARM_FEATURE_PMU)) {
> cpu->pmceid0 = 0x;
> cpu->pmceid1 = 0x;
> }
> 
> Regards,
> 
> Phil.
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> The instruction event is only enabled when icount is used, cycles are
> always supported. Always defining get_cycle_count (but altering its
> behavior depending on CONFIG_USER_ONLY) allows us to remove some
> CONFIG_USER_ONLY #defines throughout the rest of the code.
> 
> Signed-off-by: Aaron Lindsay 
> ---
[...]>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
>  static const pm_event pm_events[] = {
> +#ifndef CONFIG_USER_ONLY
> +{ .number = 0x008, /* INST_RETIRED */

"Instruction architecturally executed" seems more explicit to me.

> +  .supported = instructions_supported,
> +  .get_count = instructions_get_count
> +},
> +{ .number = 0x011, /* CPU_CYCLES */
> +  .supported = event_always_supported,
> +  .get_count = cycles_get_count
> +},
> +#endif
[...]

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/cpu.c | 3 +++
>  target/arm/cpu.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b0d032c..e544f1d 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  /* Some features automatically imply others: */
>  if (arm_feature(env, ARM_FEATURE_V8)) {
>  set_feature(env, ARM_FEATURE_V7);
> +set_feature(env, ARM_FEATURE_V7VE);
>  set_feature(env, ARM_FEATURE_ARM_DIV);
>  set_feature(env, ARM_FEATURE_LPAE);
>  }
> @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
>  
>  cpu->dtb_compatible = "arm,cortex-a7";
>  set_feature(>env, ARM_FEATURE_V7);
> +set_feature(>env, ARM_FEATURE_V7VE);
>  set_feature(>env, ARM_FEATURE_VFP4);
>  set_feature(>env, ARM_FEATURE_NEON);
>  set_feature(>env, ARM_FEATURE_THUMB2EE);
> @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
>  
>  cpu->dtb_compatible = "arm,cortex-a15";
>  set_feature(>env, ARM_FEATURE_V7);
> +set_feature(>env, ARM_FEATURE_V7VE);
>  set_feature(>env, ARM_FEATURE_VFP4);
>  set_feature(>env, ARM_FEATURE_NEON);
>  set_feature(>env, ARM_FEATURE_THUMB2EE);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index fb2f983..cc1e2fb 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1439,6 +1439,7 @@ enum arm_features {
>  ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
>  ARM_FEATURE_THUMB2EE,
>  ARM_FEATURE_V7MP,/* v7 Multiprocessing Extensions */
> +ARM_FEATURE_V7VE,/* v7 with Virtualization Extensions */
>  ARM_FEATURE_V4T,
>  ARM_FEATURE_V5,
>  ARM_FEATURE_STRONGARM,
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> The instruction event is only enabled when icount is used, cycles are
> always supported. Always defining get_cycle_count (but altering its
> behavior depending on CONFIG_USER_ONLY) allows us to remove some
> CONFIG_USER_ONLY #defines throughout the rest of the code.
> 
> Signed-off-by: Aaron Lindsay 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/helper.c | 99 
> -
>  1 file changed, 52 insertions(+), 47 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2fa8308..679897a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -15,6 +15,7 @@
>  #include "arm_ldst.h"
>  #include  /* For crc32 */
>  #include "exec/semihost.h"
> +#include "sysemu/cpus.h"
>  #include "sysemu/kvm.h"
>  #include "fpu/softfloat.h"
>  
> @@ -935,8 +936,54 @@ typedef struct pm_event {
>  uint64_t (*get_count)(CPUARMState *, uint64_t cycles);
>  } pm_event;
>  
> +/*
> + * Return the underlying cycle count for the PMU cycle counters. If we're in
> + * usermode, simply return 0.
> + */
> +static uint64_t get_cycle_count(CPUARMState *env)
> +{
> +#ifndef CONFIG_USER_ONLY
> +return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> +#else
> +return 0;
> +#endif
> +}
> +
> +static bool event_always_supported(CPUARMState *env)
> +{
> +return true;
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +static uint64_t cycles_get_count(CPUARMState *env, uint64_t cycles)
> +{
> +return cycles;
> +}
> +
> +static bool instructions_supported(CPUARMState *env)
> +{
> +return use_icount == 1 /* Precise instruction counting */;
> +}
> +
> +static uint64_t instructions_get_count(CPUARMState *env, uint64_t cycles)
> +{
> +return (uint64_t)cpu_get_icount_raw();
> +}
> +#endif
> +
>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
>  static const pm_event pm_events[] = {
> +#ifndef CONFIG_USER_ONLY
> +{ .number = 0x008, /* INST_RETIRED */
> +  .supported = instructions_supported,
> +  .get_count = instructions_get_count
> +},
> +{ .number = 0x011, /* CPU_CYCLES */
> +  .supported = event_always_supported,
> +  .get_count = cycles_get_count
> +},
> +#endif
>  { .number = SUPPORTED_EVENT_SENTINEL }
>  };
>  static uint16_t supported_event_map[0x3f];
> @@ -1016,8 +1063,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState 
> *env,
>  return pmreg_access(env, ri, isread);
>  }
>  
> -#ifndef CONFIG_USER_ONLY
> -
>  static CPAccessResult pmreg_access_selr(CPUARMState *env,
>  const ARMCPRegInfo *ri,
>  bool isread)
> @@ -1126,11 +1171,7 @@ static inline bool pmu_counter_filtered(CPUARMState 
> *env, uint64_t pmxevtyper)
>   */
>  uint64_t pmccntr_op_start(CPUARMState *env)
>  {
> -uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> -#endif
> +uint64_t cycles = get_cycle_count(env);
>  
>  if (arm_ccnt_enabled(env) &&
>!pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
> @@ -1268,26 +1309,6 @@ static void pmccntr_write32(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  pmccntr_write(env, ri, deposit64(cur_val, 0, 32, value));
>  }
>  
> -#else /* CONFIG_USER_ONLY */
> -
> -uint64_t pmccntr_op_start(CPUARMState *env)
> -{
> -}
> -
> -void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
> -{
> -}
> -
> -uint64_t pmu_op_start(CPUARMState *env)
> -{
> -}
> -
> -void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
> -{
> -}
> -
> -#endif
> -
>  static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  uint64_t value)
>  {
> @@ -1346,11 +1367,7 @@ static void pmevtyper_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  if (counter == 0x1f) {
>  pmccfiltr_write(env, ri, value);
>  } else if (counter < PMU_NUM_COUNTERS(env)) {
> -uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> -#endif
> +uint64_t cycles = get_cycle_count(env);
>  pmu_sync_counter(env, counter, cycles);
>  env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
>  pmu_sync_counter(env, counter, cycles);
> @@ -1404,11 +1421,7 @@ static void pmevcntr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>   uint64_t value, uint8_t counter)
>  {
>  if (counter < PMU_NUM_COUNTERS(env)) {
> -uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay 
> ---
>  target/arm/cpu.c   | 15 ++-
>  target/arm/cpu.h   | 23 ---
>  target/arm/internals.h |  7 ---
>  3 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 072cbbf..5f782bf 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
>   | CPU_INTERRUPT_EXITTB);
>  }
>  
> -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>   void *opaque)
>  {
> -/* We currently only support registering a single hook function */
> -assert(!cpu->el_change_hook);
> -cpu->el_change_hook = hook;
> -cpu->el_change_hook_opaque = opaque;
> +ARMELChangeHook *entry;
> +entry = g_malloc0(sizeof (*entry));

imho g_malloc() is enough.

> +
> +entry->hook = hook;
> +entry->opaque = opaque;
> +
> +QLIST_INSERT_HEAD(>el_change_hooks, entry, node);
>  }
>  
>  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  return;
>  }
>  
> +QLIST_INIT(>el_change_hooks);
> +

You missed to fill arm_cpu_unrealizefn() with:

QLIST_FOREACH(...) {
QLIST_REMOVE(...);
g_free(...);
}

>  /* Some features automatically imply others: */
>  if (arm_feature(env, ARM_FEATURE_V8)) {
>  set_feature(env, ARM_FEATURE_V7);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f17592b..3b45d3d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -633,11 +633,17 @@ typedef struct CPUARMState {
>  
>  /**
>   * ARMELChangeHook:
> - * type of a function which can be registered via 
> arm_register_el_change_hook()
> - * to get callbacks when the CPU changes its exception level or mode.
> + * Support registering functions with ARMELChangeHookFn's signature via
> + * arm_register_el_change_hook() to get callbacks when the CPU changes its
> + * exception level or mode.
>   */
> -typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque);
> -
> +typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque);
> +typedef struct ARMELChangeHook ARMELChangeHook;
> +struct ARMELChangeHook {
> +ARMELChangeHookFn *hook;
> +void *opaque;
> +QLIST_ENTRY(ARMELChangeHook) node;
> +};
>  
>  /* These values map onto the return values for
>   * QEMU_PSCI_0_2_FN_AFFINITY_INFO */
> @@ -826,8 +832,7 @@ struct ARMCPU {
>   */
>  bool cfgend;
>  
> -ARMELChangeHook *el_change_hook;
> -void *el_change_hook_opaque;
> +QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
>  
>  int32_t node_id; /* NUMA node this CPU belongs to */
>  
> @@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState 
> *cs, MemTxAttrs attrs)
>   * CPU changes exception level or mode. The hook function will be
>   * passed a pointer to the ARMCPU and the opaque data pointer passed
>   * to this function when the hook was registered.
> - *
> - * Note that we currently only support registering a single hook function,
> - * and will assert if this function is called twice.
> - * This facility is intended for the use of the GICv3 emulation.
>   */
> -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>   void *opaque);
>  
>  /**
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 47cc224..7df3eda 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr 
> physaddr,
> int mmu_idx, MemTxAttrs attrs,
> MemTxResult response, uintptr_t retaddr);
>  
> -/* Call the EL change hook if one has been registered */
> +/* Call any registered EL change hooks */
>  static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  {
> -if (cpu->el_change_hook) {
> -cpu->el_change_hook(cpu, cpu->el_change_hook_opaque);
> +ARMELChangeHook *hook, *next;
> +QLIST_FOREACH_SAFE(hook, >el_change_hooks, node, next) {
> +hook->hook(cpu, hook->opaque);
>  }
>  }
>  
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Philippe Mathieu-Daudé]

Hi Aaron,

On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
> pmceid[01] are already being initialized to zero for both A15 and A57.
> 
> Signed-off-by: Aaron Lindsay 
> ---
>  target/arm/cpu64.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 991d764..8c4db31 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
>  cpu->id_isar5 = 0x00011121;
>  cpu->id_aa64pfr0 = 0x;
>  cpu->id_aa64dfr0 = 0x10305106;
> +cpu->pmceid0 = 0x;
> +cpu->pmceid1 = 0x;
>  cpu->id_aa64isar0 = 0x00011120;
>  cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
>  cpu->dbgdidr = 0x3516d000;
> 

Maybe we can move this at a single place in arm_cpu_post_init():

if (arm_feature(>env, ARM_FEATURE_PMU)) {
cpu->pmceid0 = 0x;
cpu->pmceid1 = 0x;
}

Regards,

Phil.

[Qemu-devel] Google Summer of Code 2018 - add Windows NT 4.0 PowerPC support

[Programmingkid]

Is there still time to add another idea to the Google Summer of Code 2018 list? 
I just came across an interesting idea to add support for Windows NT 4.0 
PowerPC to QEMU. If there is still time, would there be anyone interested in 
mentoring this project?

[Qemu-devel] [Bug 1756728] Re: virtio-scsi virtio-scsi-single and virtio-blk on raw image, games are not starting

[Alexandre ARNOUD]

** Description changed:

- Using virtio-scsi / vitro-scsi-sing and vitro-blk on a ZFS/raw image,
+ Using virtio-scsi / vitro-scsi-single or vitro-blk on a ZFS/raw image,
  most Assassin's Creed (Origin especially) games are not starting
  (uPlay), it seems related to some check or commands applications are
  doing on the disk drive that fails to respond.
  
  Workaround has been found by creating a VHD volume, mounting it and
  installing games on it.
  
  On a side note, application like HDDScan, CrystalDiskInfo returns
  nothing regarding disk drives.
  
  Guest:
- Windows 10 (build 1709) 
+ Windows 10 (build 1709)
  
  Host:
  $ kvm --version
  QEMU emulator version 2.9.1 pve-qemu-kvm_2.9.1-9
  $ uname -a
  Linux  4.13.13-5-pve #1 SMP PVE 4.13.13-36 (Mon, 15 Jan 2018 12:36:49 
+0100) x86_64 GNU/Linux

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

Title:
  virtio-scsi virtio-scsi-single and virtio-blk on raw image, games are
  not starting

Status in QEMU:
  New

Bug description:
  Using virtio-scsi / vitro-scsi-single or vitro-blk on a ZFS/raw image,
  most Assassin's Creed (Origin especially) games are not starting
  (uPlay), it seems related to some check or commands applications are
  doing on the disk drive that fails to respond.

  Workaround has been found by creating a VHD volume, mounting it and
  installing games on it.

  On a side note, application like HDDScan, CrystalDiskInfo returns
  nothing regarding disk drives.

  Guest:
  Windows 10 (build 1709)

  Host:
  $ kvm --version
  QEMU emulator version 2.9.1 pve-qemu-kvm_2.9.1-9
  $ uname -a
  Linux  4.13.13-5-pve #1 SMP PVE 4.13.13-36 (Mon, 15 Jan 2018 12:36:49 
+0100) x86_64 GNU/Linux

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

[Qemu-devel] [Bug 1756728] [NEW] virtio-scsi virtio-scsi-single and virtio-blk on raw image, games are not starting

[Alexandre ARNOUD]

Public bug reported:

Using virtio-scsi / vitro-scsi-sing and vitro-blk on a ZFS/raw image,
most Assassin's Creed (Origin especially) games are not starting
(uPlay), it seems related to some check or commands applications are
doing on the disk drive that fails to respond.

Workaround has been found by creating a VHD volume, mounting it and
installing games on it.

On a side note, application like HDDScan, CrystalDiskInfo returns
nothing regarding disk drives.

Guest:
Windows 10 (build 1709) 

Host:
$ kvm --version
QEMU emulator version 2.9.1 pve-qemu-kvm_2.9.1-9
$ uname -a
Linux  4.13.13-5-pve #1 SMP PVE 4.13.13-36 (Mon, 15 Jan 2018 12:36:49 
+0100) x86_64 GNU/Linux

** 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/1756728

Title:
  virtio-scsi virtio-scsi-single and virtio-blk on raw image, games are
  not starting

Status in QEMU:
  New

Bug description:
  Using virtio-scsi / vitro-scsi-sing and vitro-blk on a ZFS/raw image,
  most Assassin's Creed (Origin especially) games are not starting
  (uPlay), it seems related to some check or commands applications are
  doing on the disk drive that fails to respond.

  Workaround has been found by creating a VHD volume, mounting it and
  installing games on it.

  On a side note, application like HDDScan, CrystalDiskInfo returns
  nothing regarding disk drives.

  Guest:
  Windows 10 (build 1709) 

  Host:
  $ kvm --version
  QEMU emulator version 2.9.1 pve-qemu-kvm_2.9.1-9
  $ uname -a
  Linux  4.13.13-5-pve #1 SMP PVE 4.13.13-36 (Mon, 15 Jan 2018 12:36:49 
+0100) x86_64 GNU/Linux

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

[Qemu-devel] [PATCH] target/m68k: add a mechanism to automatically free TCGv

[Laurent Vivier]

SRC_EA() and gen_extend() can return either a temporary
TCGv or a memory allocated one. Mark them when they are
allocated, and free them automatically at end of the
instruction translation.

We want to free locally allocated TCGv to avoid
overflow in sequence like:

  0xc00ae406:  movel %fp@(-132),%fp@(-268)
  0xc00ae40c:  movel %fp@(-128),%fp@(-264)
  0xc00ae412:  movel %fp@(-20),%fp@(-212)
  0xc00ae418:  movel %fp@(-16),%fp@(-208)
  0xc00ae41e:  movel %fp@(-60),%fp@(-220)
  0xc00ae424:  movel %fp@(-56),%fp@(-216)
  0xc00ae42a:  movel %fp@(-124),%fp@(-252)
  0xc00ae430:  movel %fp@(-120),%fp@(-248)
  0xc00ae436:  movel %fp@(-12),%fp@(-260)
  0xc00ae43c:  movel %fp@(-8),%fp@(-256)
  0xc00ae442:  movel %fp@(-52),%fp@(-276)
  0xc00ae448:  movel %fp@(-48),%fp@(-272)
  ...

That can fill a lot of TCGv entries in a sequence,
especially since 15fa08f845 ("tcg: Dynamically allocate TCGOps")
we have no limit to fill the TCGOps cache and we can fill
the entire TCG variables array and overflow it.

Suggested-by: Richard Henderson 
Signed-off-by: Laurent Vivier 
---
 target/m68k/translate.c | 100 +++-
 1 file changed, 65 insertions(+), 35 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index cef6f663ad..a660388054 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -123,8 +123,34 @@ typedef struct DisasContext {
 int done_mac;
 int writeback_mask;
 TCGv writeback[8];
+#define MAX_TO_RELEASE 8
+int release_count;
+TCGv release[MAX_TO_RELEASE];
 } DisasContext;
 
+static void init_release_array(DisasContext *s)
+{
+#ifdef CONFIG_DEBUG_TCG
+memset(s->release, 0, sizeof(s->release));
+#endif
+s->release_count = 0;
+}
+
+static void do_release(DisasContext *s)
+{
+int i;
+for (i = 0; i < s->release_count; i++) {
+tcg_temp_free(s->release[i]);
+}
+init_release_array(s);
+}
+
+static TCGv mark_to_release(DisasContext *s, TCGv tmp)
+{
+g_assert(s->release_count < MAX_TO_RELEASE);
+return s->release[s->release_count++] = tmp;
+}
+
 static TCGv get_areg(DisasContext *s, unsigned regno)
 {
 if (s->writeback_mask & (1 << regno)) {
@@ -347,7 +373,8 @@ static TCGv gen_ldst(DisasContext *s, int opsize, TCGv 
addr, TCGv val,
 gen_store(s, opsize, addr, val, index);
 return store_dummy;
 } else {
-return gen_load(s, opsize, addr, what == EA_LOADS, index);
+return mark_to_release(s, gen_load(s, opsize, addr,
+   what == EA_LOADS, index));
 }
 }
 
@@ -439,7 +466,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext 
*s, TCGv base)
 } else {
 bd = 0;
 }
-tmp = tcg_temp_new();
+tmp = mark_to_release(s, tcg_temp_new());
 if ((ext & 0x44) == 0) {
 /* pre-index */
 add = gen_addr_index(s, ext, tmp);
@@ -449,7 +476,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext 
*s, TCGv base)
 if ((ext & 0x80) == 0) {
 /* base not suppressed */
 if (IS_NULL_QREG(base)) {
-base = tcg_const_i32(offset + bd);
+base = mark_to_release(s, tcg_const_i32(offset + bd));
 bd = 0;
 }
 if (!IS_NULL_QREG(add)) {
@@ -465,7 +492,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext 
*s, TCGv base)
 add = tmp;
 }
 } else {
-add = tcg_const_i32(bd);
+add = mark_to_release(s, tcg_const_i32(bd));
 }
 if ((ext & 3) != 0) {
 /* memory indirect */
@@ -494,7 +521,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext 
*s, TCGv base)
 }
 } else {
 /* brief extension word format */
-tmp = tcg_temp_new();
+tmp = mark_to_release(s, tcg_temp_new());
 add = gen_addr_index(s, ext, tmp);
 if (!IS_NULL_QREG(base)) {
 tcg_gen_add_i32(tmp, add, base);
@@ -617,14 +644,14 @@ static void gen_flush_flags(DisasContext *s)
 s->cc_op = CC_OP_FLAGS;
 }
 
-static inline TCGv gen_extend(TCGv val, int opsize, int sign)
+static inline TCGv gen_extend(DisasContext *s, TCGv val, int opsize, int sign)
 {
 TCGv tmp;
 
 if (opsize == OS_LONG) {
 tmp = val;
 } else {
-tmp = tcg_temp_new();
+tmp = mark_to_release(s, tcg_temp_new());
 gen_ext(tmp, val, opsize, sign);
 }
 
@@ -746,7 +773,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, DisasContext *s,
 return NULL_QREG;
 }
 reg = get_areg(s, reg0);
-tmp = tcg_temp_new();
+tmp = mark_to_release(s, tcg_temp_new());
 if (reg0 == 7 && opsize == OS_BYTE &&
 m68k_feature(s->env, M68K_FEATURE_M68000)) {
 tcg_gen_subi_i32(tmp, reg, 2);
@@ -756,7 +783,7 @@ static TCGv gen_lea_mode(CPUM68KState *env, 

Re: [Qemu-devel] [PATCH v5 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang]

On 03/16/2018 11:16 PM, Michael S. Tsirkin wrote:

On Fri, Mar 16, 2018 at 06:48:28PM +0800, Wei Wang wrote:

The new feature enables the virtio-balloon device to receive hints of
guest free pages from the free page vq.

balloon_free_page_start - start guest free page hint reporting.
balloon_free_page_stop - stop guest free page hint reporting.

Note: balloon will report pages which were free at the time
of this call. As the reporting happens asynchronously, dirty bit logging
must be enabled before this call is made. Guest reporting must be
disabled before the migration dirty bitmap is synchronized.

Signed-off-by: Wei Wang 
Signed-off-by: Liang Li 
CC: Michael S. Tsirkin 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
---
  balloon.c   |  58 +--
  hw/virtio/virtio-balloon.c  | 217 ++--
  include/hw/virtio/virtio-balloon.h  |  20 ++-
  include/standard-headers/linux/virtio_balloon.h |   7 +
  include/sysemu/balloon.h|  15 +-
  5 files changed, 288 insertions(+), 29 deletions(-)

diff --git a/balloon.c b/balloon.c
index 6bf0a96..87a0410 100644
--- a/balloon.c
+++ b/balloon.c
@@ -36,6 +36,9 @@
  


+static void *virtio_balloon_poll_free_page_hints(void *opaque)
+{
+VirtQueueElement *elem;
+VirtIOBalloon *dev = opaque;
+VirtQueue *vq = dev->free_page_vq;
+uint32_t id;
+size_t size;
+
+/* The optimization thread runs only when the guest is running. */
+while (runstate_is_running()) {

Note that this check is not guaranteed to be correct
when checked like this outside BQL.

I think you are better off relying on status
callback to synchronise with the backend thread.



It's actually OK here, I think we don't need the guarantee. The device 
is just the consumer of the vq, essentially it doesn't have a dependency 
(i.e. won't block or cause errors) on the guest state.

For example:
1) when the device executes "while (runstate_is_running())" and finds 
that the guest is running, it proceeds;
2) the guest is stopped immediately right after the "while 
(runstate_is_running())" check;
3) the device side execution reaches virtqueue_pop(), and finds 
"elem==NULL", since the driver (provider) stops adding hints. Then it 
continues by going back to "while (runstate_is_running())", and now it 
finds the guest is stopped, and then exits.


Essentially, this runstate check is just an optimization to the case 
that the driver is stopped to provide hints while the device side 
optimization thread is still polling the empty vq (i.e. effort in vain). 
Probably, it would be better to check the runstate under "elem==NULL":


while (1) {
...
elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
if (!elem) {
qemu_spin_unlock(>free_page_lock);
if (runstate_is_running())
continue;
else
break;
}
...
}



+dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+} else if (dev->free_page_report_status ==
+   FREE_PAGE_REPORT_S_START) {
+/*
+ * Stop the optimization only when it has started. This avoids
+ * a stale stop sign for the previous command.
+ */
+dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+qemu_spin_unlock(>free_page_lock);
+break;
+}

And else? What if it does not match and status is not start?
Don't you need to skip in elem decoding?


No, actually we don't need "else". Please see the code inside if 
(elem->in_num) below. If the status isn't set to START, 
qemu_guest_free_page_hint will not be called to decode the elem.






+}
+
+if (elem->in_num) {
+if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START &&
+!dev->poison_val) {

poison generally disables everything? Add a TODO to handle
it in he future pls.



OK, will add TODO in the commit.



@@ -368,6 +499,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
 _abort);
 }
+dev->poison_val = le32_to_cpu(config.poison_val);


We probably should not assume this value is correct if guest didn't
ack the appropriate feature.



OK, I'll add a comment to explain that.


@@ -475,11 +632,37 @@ static void virtio_balloon_set_status(VirtIODevice *vdev, 
uint8_t status)
 {
 VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
-if (!s->stats_vq_elem && vdev->vm_running &&

-(status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, 1)) {
-/* poll stats queue for the element we have discarded when the VM
- * was stopped */
-virtio_balloon_receive_stats(vdev, s->svq);