[Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode

[linzhecheng]

1. If we create vcpu thread with QEMU_THREAD_JOINABLE mode,
we will get memory leak when vcpu thread exits, which will happen
when hot-unplug vcpus.
2. We should use QLIST_FOREACH_SAFE instead of QLIST_FOREACH
if we need to remove the entry in QLIST.

Signed-off-by: linzhecheng 

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 071f4f5..fd95b18 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -282,9 +282,9 @@ err:
 
 static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
 {
-struct KVMParkedVcpu *cpu;
+struct KVMParkedVcpu *cpu, *next_cpu;
 
-QLIST_FOREACH(cpu, >kvm_parked_vcpus, node) {
+QLIST_FOREACH_SAFE(cpu, >kvm_parked_vcpus, node, next_cpu) {
 if (cpu->vcpu_id == vcpu_id) {
 int kvm_fd;
 
diff --git a/cpus.c b/cpus.c
index 2cb0af9..de3a96b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1113,6 +1113,9 @@ static void qemu_kvm_destroy_vcpu(CPUState *cpu)
 error_report("kvm_destroy_vcpu failed");
 exit(EXIT_FAILURE);
 }
+g_free(cpu->thread);
+g_free(cpu->halt_cond);
+g_free(cpu->cpu_ases);
 }
 
 static void qemu_tcg_destroy_vcpu(CPUState *cpu)
@@ -1205,6 +1208,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
 cpu->created = false;
 qemu_cond_signal(_cpu_cond);
 qemu_mutex_unlock_iothread();
+rcu_unregister_thread();
 return NULL;
 }
 
@@ -1850,7 +1854,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
 snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
  cpu->cpu_index);
 qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
-   cpu, QEMU_THREAD_JOINABLE);
+   cpu, QEMU_THREAD_DETACHED);
 while (!cpu->created) {
 qemu_cond_wait(_cpu_cond, _global_mutex);
 }
-- 
1.8.3.1

Re: [Qemu-devel] Double-free due to e5dc1a6c6c

[Stefan Berger]

On 01/19/2018 09:05 PM, Emilio G. Cota wrote:

On Fri, Jan 19, 2018 at 17:55:27 -0500, Stefan Berger wrote:

I get double-free memory errors when QEMU terminates due to commit
e5dc1a6c6c.

The way to reproduce the error is to 1st do a 'system_reset' in the monitor
and then get into the grub console and do a 'halt' there.

Can you please check whether this is fixed by the patch below?
Was posted yesterday on the list, should be on master soon.

   https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg04298.html


Yes, fixes it for me.

   Stefan



Thanks,

Emilio

Re: [Qemu-devel] Double-free due to e5dc1a6c6c

[Emilio G. Cota]

On Fri, Jan 19, 2018 at 17:55:27 -0500, Stefan Berger wrote:
> I get double-free memory errors when QEMU terminates due to commit
> e5dc1a6c6c.
> 
> The way to reproduce the error is to 1st do a 'system_reset' in the monitor
> and then get into the grub console and do a 'halt' there.

Can you please check whether this is fixed by the patch below?
Was posted yesterday on the list, should be on master soon.

  https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg04298.html

Thanks,

Emilio

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Richard Henderson]

On 01/19/2018 10:20 AM, Philippe Mathieu-Daudé wrote:
> Maybe we should remove the __nocheck() functions and inline them in the
> 'checked' functions?

No, I use them in TCG, properly protected with CONFIG_ATOMIC*.


r~

Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test

[John Snow]

On 01/19/2018 01:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:
>> 17.01.2018 21:30, John Snow wrote:
>>>
>>> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
 Thank you for reviewing my code!

>>> Time for the re-spin? There's pretty good pressure to get this into 2.12
>>> and say the non-nbd workflow model is feature complete.
>>
>> Yes, you are right. Hope to do it today or tomorrow.
>>
> 
> I've rebased, applied comments from review, and even one some new fixes,
> but now I understand that it is too early for re-spin.
> 
> Now this series depends on
> 1. [PATCH v2 0/3] fix bitmaps migration through shared storage
>   - we need to decide, how to fix it. May be, we just do not need
> bitmaps in inactive state, and should not load them in do_qcow2_open in
> this case
>   [I can ignore it, just dropping one test case from new iotest, and fix
> it later, but..

I'll look at this next.

> 2. [PATCH v2 0/6] qmp dirty bitmap API
>  - here autoload is dropped, and migration should be rebased on it.
> 

For (2) I really want to see a test case showing the utility for enable,
disable and merge to be added to the API, *or* add an x- prefix to them
for now.

I want to see some use-case tests that demonstrate how they are to be
safely used, basically. You might want to expand test 124 for this.

I'm not sure we need this entire series for migration, but I don't want
to make you re-order absolutely everything. Can we merge patch one by
itself for the purposes of the migration patchset?

> so, I'll re-spin migration after these two questions are resolved.
> 

Thanks for your patience, as always. Please send me pings every day on
whatever you have to in order to get migration in to 2.12 -- I'm going
to try to stay focused too, but I've got the attention span of a
goldfish. If I miss something, please speak up.

--js

Re: [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[John Snow]

On 01/17/2018 10:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> 16.01.2018 15:54, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   qapi/transaction.json |  4 +++
>>   blockdev.c    | 79
>> +++
>>   2 files changed, 83 insertions(+)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index bd312792da..b643d848f8 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -46,6 +46,8 @@
>>   # - @abort: since 1.6
>>   # - @block-dirty-bitmap-add: since 2.5
>>   # - @block-dirty-bitmap-clear: since 2.5
>> +# - @block-dirty-bitmap-enable: since 2.12
>> +# - @block-dirty-bitmap-disable: since 2.12
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -59,6 +61,8 @@
>>  'abort': 'Abort',
>>  'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>>  'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>> +   'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>> +   'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>>  'blockdev-backup': 'BlockdevBackup',
>>  'blockdev-snapshot': 'BlockdevSnapshot',
>>  'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 997a6f514c..d338363d78 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
>>   AioContext *aio_context;
>>   HBitmap *backup;
>>   bool prepared;
>> +    bool was_enabled;
>>   } BlockDirtyBitmapState;
>>     static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>> @@ -2069,6 +2070,74 @@ static void
>> block_dirty_bitmap_clear_clean(BlkActionState *common)
>>   }
>>   }
>>   +static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>> +  Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +    return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_enable.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +  action->name,
>> +  >bs,
>> +  errp);
>> +    if (!state->bitmap) {
>> +    return;
>> +    }
>> +
>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>> +    bdrv_enable_dirty_bitmap(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> +
>> +    if (!state->was_enabled) {
>> +    bdrv_disable_dirty_bitmap(state->bitmap);
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>> +   Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +    return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_disable.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +  action->name,
>> +  >bs,
>> +  errp);
>> +    if (!state->bitmap) {

>bs should be NULL if we're not going to use it. If we're going
to use it, we should check the error condition here because it might fail.

>> +    return;
>> +    }
>> +
>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>> +    bdrv_disable_dirty_bitmap(state->bitmap);
> 
> hm. actually, I just make it like _clear is made. But now I doubt,
> why do we need disable here? we can disable in commit, and do not need
> state->was_enabled..
> 

You need to make sure that there is no way for bdrv_disable_dirty_bitmap
to fail, so you need to add that frozen check in. Then you're alright,
and you can move the actual disable portion to the commit, and get rid
of the undo call.

Or, we can even do this kind of trick to remove the redundant error
checking:

void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
   Error **errp)
{
BlockDirtyBitmap data = {
.node = node,
.name = name
};
TransactionAction action = {
.type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE,

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Jack Schwartz]

Hi Anatol, Daniel and Kevin.

On 01/19/18 10:36, Anatol Pomozov wrote:

Hello Jack

On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz
 wrote:

Hi Kevin and Anatol.

Kevin, thanks for your review.

More inline below...

On 01/15/18 07:54, Kevin Wolf wrote:

Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:

Properly account for the possibility of multiboot kernels with a zero
bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr
multiboot
header field.

Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report

There are some conflicts with Anatol's (CCed) multiboot series:
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html

None if these should be hard to resolve, but it would be good if you
could agree with each other whose patch series should come first, and
then the other one should be rebased on top of that.

Anatol,

from my side, there are pros and cons to either patch set going in first,
but advantages to either are pretty negligible.  Pro for you going first: I
can use the constants you will define in header files.  Pro for me going
first: your merge should be about the same as if you went first (since my
changes are small, more localized and affect only multiboot.c) and my merge
will be easier.

What are your thoughts?

Please move ahead with your patches. I'll rebase my changes on top of yours.
OK.  I'm consulting with my company's legal department and waiting for 
their approvals for delivery of a test "kernel".  I'll get in touch will 
everyone once I have an answer about that.  I anticipate about a week 
before taking next steps to deliver.


Kevin and Daniel, thanks for your inputs on this issue (different 
subthread), which I have forwarded to our legal department for review.


    Thanks,
    Jack



Testing:
1) Ran the "make check" test suite.
2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
   grub multiboot.elf test "kernel" by modifying source.)  Verified
   with gdb that new code that reads addresses/offsets from multiboot
   header was accessed.
3) Booted multiboot kernel with non-zero bss_end_addr.
4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages
worked.
5) Code has soaked in an internal repo for two months.
Can you integrate your test kernel from 2) in tests/multiboot/ so we can
keep this as a regression test?

Kevin and alias,

Before I proceed with adding my multiboot test file, I'll clarify here that
I started with a version from the grub2 tree.  In that file I expanded a
header file, also from the same tree.  Neither file had any license header,
though the tree I got them from (Dated October 2017) contains the GNU GPLv3
license file.

I'll need to check with my company before I can say I can deliver this file.
If I deliver it, I'll add a header stating the GPL license, that it came
from grub2 and to check its repository for contributors.

Jack Schwartz (4):
multiboot: bss_end_addr can be zero
multiboot: Remove unused variables from multiboot.c
multiboot: Use header names when displaying fields
multiboot: fprintf(stderr...) -> error_report()

Apart from the conflicts, the patches look good to me.

 Thanks,
 Jack


Kevin

Re: [Qemu-devel] [PATCH qemu v2] RFC: vfio-pci: Allow mmap of MSIX BAR

[Alexey Kardashevskiy]

On 20/01/18 02:57, Alex Williamson wrote:
> On Fri, 19 Jan 2018 19:55:49 +1100
> Alexey Kardashevskiy  wrote:
> 
>> On 19/01/18 08:59, Alex Williamson wrote:
>>> On Tue, 16 Jan 2018 16:17:58 +1100
>>> Alexey Kardashevskiy  wrote:
>>>   
 On 06/01/18 02:29, Alex Williamson wrote:  
> On Fri, 5 Jan 2018 10:48:07 +0100
> Auger Eric  wrote:
> 
>> Hi Alexey,
>>
>> On 15/12/17 07:29, Alexey Kardashevskiy wrote:
>>> This makes use of a new VFIO_REGION_INFO_CAP_MSIX_MAPPABLE capability
>>> which tells that a region with MSIX data can be mapped entirely, i.e.
>>> the VFIO PCI driver won't prevent MSIX vectors area from being mapped.
>>>
>>> With this change, all BARs are mapped in a single chunk and MSIX vectors
>>> are emulated on top unless the machine requests not to by defining and
>>> enabling a new "vfio-no-msix-emulation" property. At the moment only
>>> sPAPR machine does so - it prohibits MSIX emulation and does not allow
>>> enabling it as it does not define the "set" callback for the new 
>>> property;
>>> the new property also does not appear in "-machine pseries,help".
>>>
>>> If the new capability is present, this puts MSIX IO memory region under
>>> mapped memory region. If the capability is not there, it falls back to
>>> the old behaviour with the sparse capability.
>>>
>>> In MSIX vectors section is not aligned to the page size, the KVM memory
>>> listener does not register it with the KVM as a memory slot and MSIX is
>>> emulated by QEMU as before.
>>>
>>> This requires the kernel change - "vfio-pci: Allow mapping MSIX BAR" -
>>> for the new capability: https://www.spinics.net/lists/kvm/msg160282.html
>>>
>>> Signed-off-by: Alexey Kardashevskiy 
>>> ---
>>>
>>> This is mtree and flatview BEFORE this patch:
>>>
>>> "info mtree":
>>> memory-region: p...@8002000.mmio
>>>   - (prio 0, i/o): 
>>> p...@8002000.mmio
>>> 2100-2100 (prio 1, i/o): 0001:03:00.0 BAR 1
>>>   2100e000-2100e5ff (prio 0, i/o): msix-table
>>>   2100f000-2100f00f (prio 0, i/o): msix-pba 
>>> [disabled]
>>> 2104-2107 (prio 1, i/o): 0001:03:00.0 BAR 3
>>>   2104-2107 (prio 0, ramd): 0001:03:00.0 
>>> BAR 3 mmaps[0]
>>>
>>> "info mtree -f":
>>> FlatView #0
>>>  AS "memory", root: system
>>>  AS "cpu-memory", root: system
>>>  Root memory region: system
>>>   -7fff (prio 0, ram): ppc_spapr.ram
>>>   2100-2100dfff (prio 1, i/o): 0001:03:00.0 BAR 1
>>>   2100e000-2100e5ff (prio 0, i/o): msix-table
>>>   2100e600-2100 (prio 1, i/o): 0001:03:00.0 BAR 1 
>>> @e600
>>>   2104-2107 (prio 0, ramd): 0001:03:00.0 BAR 3 
>>> mmaps[0]
>>>
>>>
>>>
>>> This is AFTER this patch applied:
>>>
>>> "info mtree":
>>> memory-region: p...@8002000.mmio
>>>   - (prio 0, i/o): 
>>> p...@8002000.mmio
>>> 2100-2100 (prio 1, i/o): 0001:03:00.0 BAR 1
>>>   2100-2100 (prio 0, ramd): 0001:03:00.0 
>>> BAR 1 mmaps[0]
>>> 2100e000-2100e5ff (prio 0, i/o): msix-table 
>>> [disabled]
>>> 2100f000-2100f00f (prio 0, i/o): msix-pba 
>>> [disabled]
>>> 2104-2107 (prio 1, i/o): 0001:03:00.0 BAR 3
>>>   2104-2107 (prio 0, ramd): 0001:03:00.0 
>>> BAR 3 mmaps[0]
>>>
>>>
>>> "info mtree -f":
>>> FlatView #2
>>>  AS "memory", root: system
>>>  AS "cpu-memory", root: system
>>>  Root memory region: system
>>>   -7fff (prio 0, ram): ppc_spapr.ram
>>>   2100-2100 (prio 0, ramd): 0001:03:00.0 BAR 1 
>>> mmaps[0]
>>>   2104-2107 (prio 0, ramd): 0001:03:00.0 BAR 3 
>>> mmaps[0]
>>>
>>>
>>>
>>> This is AFTER this patch applied AND spapr_get_msix_emulation() patched
>>> to enable emulation:
>>>
>>> "info mtree":
>>> memory-region: p...@8002000.mmio
>>>   - (prio 0, i/o): 
>>> p...@8002000.mmio
>>> 2100-2100 (prio 1, i/o): 0001:03:00.0 BAR 1
>>>   2100-2100 (prio 0, ramd): 0001:03:00.0 
>>> BAR 1 mmaps[0]
>>> 2100e000-2100e5ff (prio 0, i/o): msix-table
>>> 2100f000-2100f00f (prio 0, 

Re: [Qemu-devel] [RFC] qid path collision issues in 9pfs

[Emilio G. Cota]

On Fri, Jan 19, 2018 at 11:27:33 +0100, Greg Kurz wrote:
> On Mon, 15 Jan 2018 11:49:31 +0800
> Antonios Motakis  wrote:
> > On 13-Jan-18 00:14, Greg Kurz wrote:
> > > On Fri, 12 Jan 2018 19:32:10 +0800
> > > Antonios Motakis  wrote:
(snip)
> > >> To avoid this situation, the device id of a file needs to be also taken 
> > >> as
> > >> input for generating a qid path. Unfortunately, the size of both inode
> > >> nr + device id together would be 96 bits, while we have only 64 bits
> > >> for the qid path, so we can't just append them and call it a day :(
(snip)
> > >>
> > >> We have thought of a few approaches, but we would definitely like to hear
> > >> what the upstream maintainers and community think:
(snip)
> > >> * Fallback and/or interim solutions
(snip)
> > >> 3. other ideas, such as allocating new qid paths and keep a look up 
> > >> table of some sorts (possibly too expensive)
> > >>  
> > > 
> > > This would be acceptable for fallback.  
> > 
> > Maybe we can use the QEMU hash table 
> > (https://github.com/qemu/qemu/blob/master/util/qht.c) but I wonder
> > if it scales to millions of entries. Do you think it is appropriate in this 
> > case?
> 
> I don't know QHT, hence Cc'ing Emilio for insights.

QHT stores a 32-bit hash per entry, so with perfect hashing one wouldn't
see collisions (and the subsequent performance drop) below 2**32 entries.
So yes, millions of entries are supported.

Wrt memory overhead, each entry takes 12 bytes on 64-bit hosts
and 8 bytes on 32-bit hosts (pointer + hash). However, we have to account
for empty or partially filled buckets, as well as bucket overhead (header,
tail and lock), so on 64-bit we can approximate the overhead to 32 bytes
per entry. Given that inode sizes are >128 bytes, memory overhead
[size(ht) / size(index)] would be at most 25%. (See below for some
suggestions to lower this.)

> > I was thinking on how to implement something like this, without having to 
> > maintain
> > millions of entries... One option we could consider is to split the bits 
> > into a
> > directly-mapped part, and a lookup part. For example:
> > 
> > Inode =
> > [ 10 first bits ] + [ 54 lowest bits ]
> > 
> > A hash table maps [ inode 10 first bits ] + [ device id ] => [ 10 bit 
> > prefix ]
> > The prefix is uniquely allocated for each input.
> > 
> > Qid path = 
> > [ 10 bit prefix ] + [ inode 54 lowest bits ]
> > 
> > Since inodes are not completely random, and we usually have a handful of 
> > device IDs,
> > we get a much smaller number of entries to track in the hash table.
> > 
> > So what this would give:
> > (1) Would be faster and take less memory than mapping the full 
> > inode_nr,devi_id
> > tuple to unique QID paths
> > (2) Guaranteed not to run out of bits when inode numbers stay below the 
> > lowest
> > 54 bits and we have less than 1024 devices.
> > (3) When we get beyond this this limit, there is a chance we run out of 
> > bits to
> > allocate new QID paths, but we can detect this and refuse to serve the 
> > offending
> > files instead of allowing a collision.
> > 
> > We could tweak the prefix size to match the scenarios that we consider more 
> > likely,
> > but I think close to 10-16 bits sounds reasonable enough. What do you think?

Assuming assumption (2) is very likely to be true, I'd suggest
dropping the intermediate hash table altogether, and simply refuse
to work with any files that do not meet (2).

That said, the naive solution of having a large hash table with all entries
in it might be worth a shot. With QHT and a good hash function
like xxhash, you'd support parallel lookups and at worst it'd add
an extra last-level cache miss per file operation (~300 cycles),
which on average would be dwarfed by the corresponding I/O latency.

This should be easy to implement and benchmark, so I think it is
worth trying -- you might not need to fiddle with the protocol
after all.

BTW since hashing here would be very simple, we could have a variant
of QHT where only pointers are kept in the buckets, thereby lowering
memory overhead. We could also play with the resizing algorithm to
make it less aggresive so that we end up with less (and fuller) buckets.

Thanks,

E.

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/block-core.json | 42 ++
>  blockdev.c   | 42 ++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 827254db22..b3851ee760 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1672,6 +1672,48 @@
>'data': 'BlockDirtyBitmap' }
>  
>  ##
> +# @block-dirty-bitmap-enable:
> +#
> +# Enable dirty bitmap, so that it will continue tracking disk changes.
> +#

suggest:
"Enables a dirty bitmap so that it will begin tracking disk changes."

Key item here is "begin" instead of "continue".

> +# Returns: nothing on success
> +#  If @node is not a valid block device, DeviceNotFound
> +#  If @name is not found, GenericError with an explanation
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-enable",
> +#  "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +  { 'command': 'block-dirty-bitmap-enable',
> +'data': 'BlockDirtyBitmap' }
> +
> +##
> +# @block-dirty-bitmap-disable:
> +#
> +# Disable dirty bitmap, so that it will stop tracking disk changes.
> +#

suggest:
"Disables a dirty bitmap so that it will stop tracking disk changes."

> +# Returns: nothing on success
> +#  If @node is not a valid block device, DeviceNotFound
> +#  If @name is not found, GenericError with an explanation
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-disable",
> +#  "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'block-dirty-bitmap-disable',
> +  'data': 'BlockDirtyBitmap' }
> +
> +##
>  # @BlockDirtyBitmapSha256:
>  #
>  # SHA256 hash of dirty bitmap data
> diff --git a/blockdev.c b/blockdev.c
> index 8068cbd606..997a6f514c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, 
> const char *name,
>  bdrv_clear_dirty_bitmap(bitmap, NULL);
>  }
>  
> +void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
> +   Error **errp)
> +{
> +BlockDriverState *bs;
> +BdrvDirtyBitmap *bitmap;
> +
> +bitmap = block_dirty_bitmap_lookup(node, name, , errp);
> +if (!bitmap || !bs) {
> +return;
> +}
> +
> +if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +error_setg(errp,
> +   "Bitmap '%s' is currently frozen and cannot be enabled",
> +   name);
> +return;
> +}
> +
> +bdrv_enable_dirty_bitmap(bitmap);
> +}
> +
> +void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
> +Error **errp)
> +{
> +BlockDriverState *bs;
> +BdrvDirtyBitmap *bitmap;
> +
> +bitmap = block_dirty_bitmap_lookup(node, name, , errp);
> +if (!bitmap || !bs) {
> +return;
> +}
> +
> +if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +error_setg(errp,
> +   "Bitmap '%s' is currently frozen and cannot be disabled",
> +   name);
> +return;
> +}
> +
> +bdrv_disable_dirty_bitmap(bitmap);
> +}
> +
>  BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char 
> *node,
>const char 
> *name,
>Error **errp)
> 

I have to admit exposing this interface still makes me nervous, but :)

Mechanically correct, and with suggesting phrasing changes:

Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v2 2/6] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add locks and remove comments about BQL accordingly to
> dirty_bitmap_mutex definition in block_int.h.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/dirty-bitmap.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 3777be1985..d0a10c4f5d 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -389,18 +389,20 @@ void 
> bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>  }
>  }
>  
> -/* Called with BQL taken.  */
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
> +bdrv_dirty_bitmap_lock(bitmap);
>  assert(!bdrv_dirty_bitmap_frozen(bitmap));
>  bitmap->disabled = true;
> +bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
> -/* Called with BQL taken.  */
>  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
> +bdrv_dirty_bitmap_lock(bitmap);
>  assert(!bdrv_dirty_bitmap_frozen(bitmap));
>  bitmap->disabled = false;
> +bdrv_dirty_bitmap_unlock(bitmap);
>  }
>  
>  BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> 

Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> To maintain load/store disabled bitmap there is new approach:
> 
>  - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
>  - store enabled bitmaps as "auto" to qcow2
>  - store disabled bitmaps without "auto" flag to qcow2
>  - on qcow2 open load "auto" bitmaps as enabled and others
>as disabled (except in_use bitmaps)
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/block-core.json |  6 +++---
>  block/qcow2.h|  2 +-
>  include/block/dirty-bitmap.h |  1 -
>  block/dirty-bitmap.c | 18 --
>  block/qcow2-bitmap.c | 12 +++-
>  block/qcow2.c|  2 +-
>  blockdev.c   | 10 ++
>  7 files changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab96e348e6..827254db22 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1593,9 +1593,9 @@
>  #  Qcow2 disks support persistent bitmaps. Default is false for
>  #  block-dirty-bitmap-add. (Since: 2.10)
>  #
> -# @autoload: the bitmap will be automatically loaded when the image it is 
> stored
> -#in is opened. This flag may only be specified for persistent
> -#bitmaps. Default is false for block-dirty-bitmap-add. (Since: 
> 2.10)
> +# @autoload: ignored and deprecated since 2.12.
> +#Currently, all dirty tracking bitmaps are loaded from Qcow2 on
> +#open.

Hmm, so we're going to say that *all* persistent bitmaps are loaded into
memory, but they may-or-may-not-be enabled, is that the approach we're
taking now?

>  #
>  # Since: 2.4
>  ##
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 782a206ecb..a3e29276fc 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -672,7 +672,7 @@ void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache 
> *c, void *table);
>  int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>void **refcount_table,
>int64_t *refcount_table_size);
> -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error 
> **errp);
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
> **errp);
>  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 3579a7597c..144e77a879 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -65,7 +65,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap 
> *bitmap,
>  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>  
>  void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
>  void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
> bool persistent);
>  
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index bd04e991b1..3777be1985 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -52,8 +52,6 @@ struct BdrvDirtyBitmap {
> Such operations must fail and both the 
> image
> and this bitmap must remain unchanged 
> while
> this flag is set. */
> -bool autoload;  /* For persistent bitmaps: bitmap must be
> -   autoloaded on image opening */
>  bool persistent;/* bitmap must be saved to owner disk image 
> */
>  QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
> @@ -104,7 +102,6 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
>  g_free(bitmap->name);
>  bitmap->name = NULL;
>  bitmap->persistent = false;
> -bitmap->autoload = false;
>  }
>  
>  /* Called with BQL taken.  */
> @@ -261,8 +258,6 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>  bitmap->successor = NULL;
>  successor->persistent = bitmap->persistent;
>  bitmap->persistent = false;
> -successor->autoload = bitmap->autoload;
> -bitmap->autoload = false;
>  bdrv_release_dirty_bitmap(bs, bitmap);
>  
>  return successor;
> @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
>  }
>  
>  /* Called with BQL taken. */
> -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
> -{
> -qemu_mutex_lock(bitmap->mutex);
> -bitmap->autoload = autoload;
> -qemu_mutex_unlock(bitmap->mutex);
> -}
> -
> -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
> -{
> -return bitmap->autoload;
> -}
> -
> -/* Called 

Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API

[John Snow]

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
> 
> There are three qmp commands, needed to implement external backup API.
> 
> Using these three commands, client may do all needed bitmap management by
> hand:
> 
> on backup start we need to do a transaction:
>  {disable old bitmap, create new bitmap}
> 
> on backup success:
>  drop old bitmap
> 
> on backup fail:
>  enable old bitmap
>  merge new bitmap to old bitmap
>  drop new bitmap
> 
> v2: fix merge command deadlock
>   add new patches: 1 and 6
> 
> Vladimir Sementsov-Ogievskiy (6):
>   block: maintain persistent disabled bitmaps
>   block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap
>   qapi: add block-dirty-bitmap-enable/disable
>   qmp: transaction support for block-dirty-bitmap-enable/disable
>   qapi: add block-dirty-bitmap-merge
>   qapi: add disabled parameter to block-dirty-bitmap-add
> 
>  qapi/block-core.json |  92 ++-
>  qapi/transaction.json|   4 +
>  block/qcow2.h|   2 +-
>  include/block/dirty-bitmap.h |   3 +-
>  block/dirty-bitmap.c |  42 ++-
>  block/qcow2-bitmap.c |  12 +--
>  block/qcow2.c|   2 +-
>  blockdev.c   | 169 
> +--
>  8 files changed, 287 insertions(+), 39 deletions(-)
> 

Fails to apply to master (b384cd95) on patch four and five. Only
contextual problems, I've patched it up and I'll review that.

(mirrored here if you want to check my rebase work:
https://github.com/jnsnow/qemu/tree/vlad-review)

Since I was full of such bad and stupid ideas last time, I'd like
someone else to look over this one for design and I'll just review it
for accuracy.

--js

Re: [Qemu-devel] [PATCH v4] file-posix: specify expected filetypes

[Eric Blake]

On 01/19/2018 04:47 PM, John Snow wrote:
> Adjust each caller of raw_open_common to specify if they are expecting
> host and character devices or not. Tighten expectations of file types upon
> open in the common code and refuse types that are not expected.
> 
> This has two effects:
> 
> (1) Character and block devices are now considered deprecated for the
> 'file' driver, which expects only S_IFREG, and
> (2) no file-posix driver (file, host_cdrom, or host_device) can open
> directories now.
> 
> I don't think there's a legitimate reason to open directories as if
> they were files. This prevents QEMU from opening and attempting to probe
> a directory inode, which can break in exciting ways. One of those ways
> is lseek on ext4/xfs, which will return 0x7fff as the file
> size instead of EISDIR. This can coax QEMU into responding with a
> confusing "file too big" instead of "Hey, that's not a file".
> 
> See: https://bugs.launchpad.net/qemu/+bug/1739304/
> Signed-off-by: John Snow 
> ---

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] Double-free due to e5dc1a6c6c

[Stefan Berger]

I get double-free memory errors when QEMU terminates due to commit 
e5dc1a6c6c.


The way to reproduce the error is to 1st do a 'system_reset' in the 
monitor and then get into the grub console and do a 'halt' there.



 Stefan


commit e5dc1a6c6c4359cd783810f63eb68e9e09350708
Author: Marc-André Lureau 
Date:   Thu Jan 4 17:05:15 2018 +0100

readline: add a free function

Fixes leaks such as:

Direct leak of 2 byte(s) in 1 object(s) allocated from:
#0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
#1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
#2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
#3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
#4 0x55db720f1d46 in readline_hist_add 
/home/elmarco/src/qq/util/readline.c:258
#5 0x55db720f2d34 in readline_handle_byte 
/home/elmarco/src/qq/util/readline.c:387
#6 0x55db71539d00 in monitor_read 
/home/elmarco/src/qq/monitor.c:3896
#7 0x55db71f9be35 in qemu_chr_be_write_impl 
/home/elmarco/src/qq/chardev/char.c:167
#8 0x55db71f9bed3 in qemu_chr_be_write 
/home/elmarco/src/qq/chardev/char.c:179
#9 0x55db71fa013c in fd_chr_read 
/home/elmarco/src/qq/chardev/char-fd.c:66
#10 0x55db71fe18a8 in qio_channel_fd_source_dispatch 
/home/elmarco/src/qq/io/channel-watch.c:84

#11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
#12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
#13 0x55db720af3bd in glib_pollfds_poll 
/home/elmarco/src/qq/util/main-loop.c:214
#14 0x55db720af505 in os_host_main_loop_wait 
/home/elmarco/src/qq/util/main-loop.c:261
#15 0x55db720af6d6 in main_loop_wait 
/home/elmarco/src/qq/util/main-loop.c:515

#16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
#17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
#18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)

[Qemu-devel] [PATCH v4] file-posix: specify expected filetypes

[John Snow]

Adjust each caller of raw_open_common to specify if they are expecting
host and character devices or not. Tighten expectations of file types upon
open in the common code and refuse types that are not expected.

This has two effects:

(1) Character and block devices are now considered deprecated for the
'file' driver, which expects only S_IFREG, and
(2) no file-posix driver (file, host_cdrom, or host_device) can open
directories now.

I don't think there's a legitimate reason to open directories as if
they were files. This prevents QEMU from opening and attempting to probe
a directory inode, which can break in exciting ways. One of those ways
is lseek on ext4/xfs, which will return 0x7fff as the file
size instead of EISDIR. This can coax QEMU into responding with a
confusing "file too big" instead of "Hey, that's not a file".

See: https://bugs.launchpad.net/qemu/+bug/1739304/
Signed-off-by: John Snow 
---

v4: fixing doc building bug, sorry :(

 block/file-posix.c | 37 +
 qemu-doc.texi  |  6 ++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e940..61fac1d2a4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -417,7 +417,8 @@ static QemuOptsList raw_runtime_opts = {
 };
 
 static int raw_open_common(BlockDriverState *bs, QDict *options,
-   int bdrv_flags, int open_flags, Error **errp)
+   int bdrv_flags, int open_flags,
+   bool device, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 QemuOpts *opts;
@@ -556,10 +557,30 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 error_setg_errno(errp, errno, "Could not stat file");
 goto fail;
 }
-if (S_ISREG(st.st_mode)) {
-s->discard_zeroes = true;
-s->has_fallocate = true;
+
+if (!device) {
+if (S_ISBLK(st.st_mode)) {
+warn_report("Opening a block device as file using 'file' "
+"driver is deprecated");
+} else if (S_ISCHR(st.st_mode)) {
+warn_report("Opening a character device as file using the 'file' "
+"driver is deprecated");
+} else if (!S_ISREG(st.st_mode)) {
+error_setg(errp, "A regular file was expected by the 'file' 
driver, "
+   "but something else was given");
+goto fail;
+} else {
+s->discard_zeroes = true;
+s->has_fallocate = true;
+}
+} else {
+if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
+error_setg(errp, "host_device/host_cdrom driver expects either "
+   "a character or block device");
+goto fail;
+}
 }
+
 if (S_ISBLK(st.st_mode)) {
 #ifdef BLKDISCARDZEROES
 unsigned int arg;
@@ -611,7 +632,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 BDRVRawState *s = bs->opaque;
 
 s->type = FTYPE_FILE;
-return raw_open_common(bs, options, flags, 0, errp);
+return raw_open_common(bs, options, flags, 0, false, errp);
 }
 
 typedef enum {
@@ -2575,7 +2596,7 @@ hdev_open_Mac_error:
 
 s->type = FTYPE_FILE;
 
-ret = raw_open_common(bs, options, flags, 0, _err);
+ret = raw_open_common(bs, options, flags, 0, true, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
@@ -2802,7 +2823,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->type = FTYPE_CD;
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-return raw_open_common(bs, options, flags, O_NONBLOCK, errp);
+return raw_open_common(bs, options, flags, O_NONBLOCK, true, errp);
 }
 
 static int cdrom_probe_device(const char *filename)
@@ -2915,7 +2936,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 s->type = FTYPE_CD;
 
-ret = raw_open_common(bs, options, flags, 0, _err);
+ret = raw_open_common(bs, options, flags, 0, true, _err);
 if (ret) {
 error_propagate(errp, local_err);
 return ret;
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 3e9eb819a6..8c94dcf8a6 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2708,6 +2708,12 @@ that can be specified with the ``-device'' parameter.
 The drive addr argument is replaced by the the addr argument
 that can be specified with the ``-device'' parameter.
 
+@subsection -drive file=json:@{...@{'driver':'file'@}@} (since 2.12.0)
+
+The 'file' driver for drives is no longer appropriate for character or host
+devices and will only accept regular files (S_IFREG). The correct driver
+for these file types is 'host_cdrom' or 'host_device' as appropriate.
+
 @subsection -net dump (since 2.10.0)
 
 The ``--net dump'' argument is now replaced with the
-- 
2.14.3

Re: [Qemu-devel] [PATCH v4 1/3] xlnx-zynqmp-rtc: Initial commit

[Alistair Francis]

On Fri, Jan 19, 2018 at 11:22 AM, Philippe Mathieu-Daudé
 wrote:
> Hi Alistair,
>
> On 01/19/2018 03:35 PM, Alistair Francis wrote:
>> Initial commit of the ZynqMP RTC device.
>>
>> Signed-off-by: Alistair Francis 
>> ---
>> V2:
>>  - Delete unused realise function
>>  - Remove DB_PRINT()
>>
>>  include/hw/timer/xlnx-zynqmp-rtc.h |  84 ++
>>  hw/timer/xlnx-zynqmp-rtc.c | 218 
>> +
>>  hw/timer/Makefile.objs |   1 +
>>  3 files changed, 303 insertions(+)
>>  create mode 100644 include/hw/timer/xlnx-zynqmp-rtc.h
>>  create mode 100644 hw/timer/xlnx-zynqmp-rtc.c
>>
>> diff --git a/include/hw/timer/xlnx-zynqmp-rtc.h 
>> b/include/hw/timer/xlnx-zynqmp-rtc.h
>> new file mode 100644
>> index 00..87649836cc
>> --- /dev/null
>> +++ b/include/hw/timer/xlnx-zynqmp-rtc.h
>> @@ -0,0 +1,84 @@
>> +/*
>> + * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
>> + *
>> + * Copyright (c) 2017 Xilinx Inc.
>> + *
>> + * Written-by: Alistair Francis 
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/register.h"
>> +
>> +#define TYPE_XLNX_ZYNQMP_RTC "xlnx-zynmp.rtc"
>> +
>> +#define XLNX_ZYNQMP_RTC(obj) \
>> + OBJECT_CHECK(XlnxZynqMPRTC, (obj), TYPE_XLNX_ZYNQMP_RTC)
>> +
>> +REG32(SET_TIME_WRITE, 0x0)
>> +REG32(SET_TIME_READ, 0x4)
>> +REG32(CALIB_WRITE, 0x8)
>> +FIELD(CALIB_WRITE, FRACTION_EN, 20, 1)
>> +FIELD(CALIB_WRITE, FRACTION_DATA, 16, 4)
>> +FIELD(CALIB_WRITE, MAX_TICK, 0, 16)
>> +REG32(CALIB_READ, 0xc)
>> +FIELD(CALIB_READ, FRACTION_EN, 20, 1)
>> +FIELD(CALIB_READ, FRACTION_DATA, 16, 4)
>> +FIELD(CALIB_READ, MAX_TICK, 0, 16)
>> +REG32(CURRENT_TIME, 0x10)
>> +REG32(CURRENT_TICK, 0x14)
>> +FIELD(CURRENT_TICK, VALUE, 0, 16)
>> +REG32(ALARM, 0x18)
>> +REG32(RTC_INT_STATUS, 0x20)
>> +FIELD(RTC_INT_STATUS, ALARM, 1, 1)
>> +FIELD(RTC_INT_STATUS, SECONDS, 0, 1)
>> +REG32(RTC_INT_MASK, 0x24)
>> +FIELD(RTC_INT_MASK, ALARM, 1, 1)
>> +FIELD(RTC_INT_MASK, SECONDS, 0, 1)
>> +REG32(RTC_INT_EN, 0x28)
>> +FIELD(RTC_INT_EN, ALARM, 1, 1)
>> +FIELD(RTC_INT_EN, SECONDS, 0, 1)
>> +REG32(RTC_INT_DIS, 0x2c)
>> +FIELD(RTC_INT_DIS, ALARM, 1, 1)
>> +FIELD(RTC_INT_DIS, SECONDS, 0, 1)
>> +REG32(ADDR_ERROR, 0x30)
>> +FIELD(ADDR_ERROR, STATUS, 0, 1)
>> +REG32(ADDR_ERROR_INT_MASK, 0x34)
>> +FIELD(ADDR_ERROR_INT_MASK, MASK, 0, 1)
>> +REG32(ADDR_ERROR_INT_EN, 0x38)
>> +FIELD(ADDR_ERROR_INT_EN, MASK, 0, 1)
>> +REG32(ADDR_ERROR_INT_DIS, 0x3c)
>> +FIELD(ADDR_ERROR_INT_DIS, MASK, 0, 1)
>> +REG32(CONTROL, 0x40)
>> +FIELD(CONTROL, BATTERY_DISABLE, 31, 1)
>> +FIELD(CONTROL, OSC_CNTRL, 24, 4)
>> +FIELD(CONTROL, SLVERR_ENABLE, 0, 1)
>> +REG32(SAFETY_CHK, 0x50)
>> +
>> +#define XLNX_ZYNQMP_RTC_R_MAX (R_SAFETY_CHK + 1)
>> +
>> +typedef struct XlnxZynqMPRTC {
>> +SysBusDevice parent_obj;
>> +MemoryRegion iomem;
>> +qemu_irq irq_rtc_int;
>> +qemu_irq irq_addr_error_int;
>> +
>> +uint32_t regs[XLNX_ZYNQMP_RTC_R_MAX];
>> +RegisterInfo regs_info[XLNX_ZYNQMP_RTC_R_MAX];
>> +} XlnxZynqMPRTC;
>> diff --git a/hw/timer/xlnx-zynqmp-rtc.c b/hw/timer/xlnx-zynqmp-rtc.c
>> new file mode 100644
>> index 00..ead40fc42d
>> --- /dev/null
>> +++ b/hw/timer/xlnx-zynqmp-rtc.c
>> @@ -0,0 +1,218 @@
>> +/*
>> + * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
>> + *
>> + * Copyright (c) 2017 Xilinx Inc.
>> + *
>> + * Written-by: Alistair Francis 
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, 

[Qemu-devel] [PULL 2/4] ide: move ide_sect_range_ok() up

[John Snow]

From: Anton Nefedov 

to use it without a forward declaration in the commit to follow

Signed-off-by: Anton Nefedov 
Message-id: 1512735034-35327-3-git-send-email-anton.nefe...@virtuozzo.com
Signed-off-by: John Snow 
---
 hw/ide/core.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 875f7b442d..27226bfd51 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -380,6 +380,18 @@ static void ide_set_signature(IDEState *s)
 }
 }
 
+static bool ide_sect_range_ok(IDEState *s,
+  uint64_t sector, uint64_t nb_sectors)
+{
+uint64_t total_sectors;
+
+blk_get_geometry(s->blk, _sectors);
+if (sector > total_sectors || nb_sectors > total_sectors - sector) {
+return false;
+}
+return true;
+}
+
 typedef struct TrimAIOCB {
 BlockAIOCB common;
 IDEState *s;
@@ -603,18 +615,6 @@ static void ide_rw_error(IDEState *s) {
 ide_set_irq(s->bus);
 }
 
-static bool ide_sect_range_ok(IDEState *s,
-  uint64_t sector, uint64_t nb_sectors)
-{
-uint64_t total_sectors;
-
-blk_get_geometry(s->blk, _sectors);
-if (sector > total_sectors || nb_sectors > total_sectors - sector) {
-return false;
-}
-return true;
-}
-
 static void ide_buffered_readv_cb(void *opaque, int ret)
 {
 IDEBufferedRequest *req = opaque;
-- 
2.14.3

[Qemu-devel] [PULL 4/4] hw/ide: Remove duplicated definitions from ahci_internal.h

[John Snow]

The same definitions can also be found in include/hw/ide/ahci.h
so let's remove these #defines from ahci_internal.h.

Signed-off-by: Thomas Huth 
Message-id: 1512457825-3847-1-git-send-email-th...@redhat.com
[Maintainer edit: publicize object names, privatize object macros.]
Signed-off-by: John Snow 
---
 hw/ide/ahci_internal.h | 4 
 include/hw/ide/ahci.h  | 6 --
 2 files changed, 10 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index ce2e818c8c..8c755d4ca1 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -311,8 +311,6 @@ struct AHCIPCIState {
 AHCIState ahci;
 };
 
-#define TYPE_ICH9_AHCI "ich9-ahci"
-
 #define ICH_AHCI(obj) \
 OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH9_AHCI)
 
@@ -375,10 +373,8 @@ void ahci_uninit(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
-#define TYPE_SYSBUS_AHCI "sysbus-ahci"
 #define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), TYPE_SYSBUS_AHCI)
 
-#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
 #define ALLWINNER_AHCI(obj) OBJECT_CHECK(AllwinnerAHCIState, (obj), \
TYPE_ALLWINNER_AHCI)
 
diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 5a06537e6b..b7bb2b02d6 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -54,14 +54,10 @@ typedef struct AHCIPCIState AHCIPCIState;
 
 #define TYPE_ICH9_AHCI "ich9-ahci"
 
-#define ICH_AHCI(obj) \
-OBJECT_CHECK(AHCIPCIState, (obj), TYPE_ICH9_AHCI)
-
 int32_t ahci_get_num_ports(PCIDevice *dev);
 void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd);
 
 #define TYPE_SYSBUS_AHCI "sysbus-ahci"
-#define SYSBUS_AHCI(obj) OBJECT_CHECK(SysbusAHCIState, (obj), TYPE_SYSBUS_AHCI)
 
 typedef struct SysbusAHCIState {
 /*< private >*/
@@ -73,8 +69,6 @@ typedef struct SysbusAHCIState {
 } SysbusAHCIState;
 
 #define TYPE_ALLWINNER_AHCI "allwinner-ahci"
-#define ALLWINNER_AHCI(obj) OBJECT_CHECK(AllwinnerAHCIState, (obj), \
-   TYPE_ALLWINNER_AHCI)
 
 #define ALLWINNER_AHCI_MMIO_OFF  0x80
 #define ALLWINNER_AHCI_MMIO_SIZE 0x80
-- 
2.14.3

[Qemu-devel] [PULL 0/4] Ide patches

[John Snow]

The following changes since commit b384cd95eb9c6f73ad84ed1bb0717a26e29cc78f:

  Merge remote-tracking branch 
'remotes/ehabkost/tags/machine-next-pull-request' into staging (2018-01-19 
16:35:25 +)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 3161906df88a471b09c38fff9a618ff83beea0c3:

  hw/ide: Remove duplicated definitions from ahci_internal.h (2018-01-19 
16:04:57 -0500)





Anton Nefedov (3):
  ide: pass IDEState to trim AIO callback
  ide: move ide_sect_range_ok() up
  ide: abort TRIM operation for invalid range

John Snow (1):
  hw/ide: Remove duplicated definitions from ahci_internal.h

 hw/ide/ahci_internal.h |  4 
 hw/ide/core.c  | 53 +++---
 include/hw/ide/ahci.h  |  6 --
 3 files changed, 33 insertions(+), 30 deletions(-)

-- 
2.14.3

[Qemu-devel] [PULL 3/4] ide: abort TRIM operation for invalid range

[John Snow]

From: Anton Nefedov 

ATA8-ACS3, 7.9 DATA SET MANAGEMENT - 06h, DMA

7.9.5 Error Outputs
If the Trim bit is set to one and:
  a) the device detects an invalid LBA Range Entry; or
  b) count is greater than IDENTIFY DEVICE data word 105
 (see 7.16.7.55),
then the device shall return command aborted.
A device may trim one or more LBA Range Entries before it returns
command aborted. See table 209.

This check is not in the common ide_dma_cb() as the range for TRIM
is harder to reach: it is not in LBA/count registers and the buffer has
to be parsed first.

Signed-off-by: Anton Nefedov 
Message-id: 1512735034-35327-4-git-send-email-anton.nefe...@virtuozzo.com
Signed-off-by: John Snow 
---
 hw/ide/core.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 27226bfd51..5be72d41dc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -400,6 +400,7 @@ typedef struct TrimAIOCB {
 QEMUIOVector *qiov;
 BlockAIOCB *aiocb;
 int i, j;
+bool is_invalid;
 } TrimAIOCB;
 
 static void trim_aio_cancel(BlockAIOCB *acb)
@@ -427,8 +428,11 @@ static void ide_trim_bh_cb(void *opaque)
 {
 TrimAIOCB *iocb = opaque;
 
-iocb->common.cb(iocb->common.opaque, iocb->ret);
-
+if (iocb->is_invalid) {
+ide_dma_error(iocb->s);
+} else {
+iocb->common.cb(iocb->common.opaque, iocb->ret);
+}
 qemu_bh_delete(iocb->bh);
 iocb->bh = NULL;
 qemu_aio_unref(iocb);
@@ -455,6 +459,11 @@ static void ide_issue_trim_cb(void *opaque, int ret)
 continue;
 }
 
+if (!ide_sect_range_ok(s, sector, count)) {
+iocb->is_invalid = true;
+goto done;
+}
+
 /* Got an entry! Submit and exit.  */
 iocb->aiocb = blk_aio_pdiscard(s->blk,
sector << BDRV_SECTOR_BITS,
@@ -470,6 +479,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
 iocb->ret = ret;
 }
 
+done:
 iocb->aiocb = NULL;
 if (iocb->bh) {
 qemu_bh_schedule(iocb->bh);
@@ -490,6 +500,7 @@ BlockAIOCB *ide_issue_trim(
 iocb->qiov = qiov;
 iocb->i = -1;
 iocb->j = 0;
+iocb->is_invalid = false;
 ide_issue_trim_cb(iocb, 0);
 return >common;
 }
-- 
2.14.3

[Qemu-devel] [PULL 1/4] ide: pass IDEState to trim AIO callback

[John Snow]

From: Anton Nefedov 

It will be needed to handle invalid requests

Signed-off-by: Anton Nefedov 
Message-id: 1512735034-35327-2-git-send-email-anton.nefe...@virtuozzo.com
Signed-off-by: John Snow 
---
 hw/ide/core.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1ea5812b7e..875f7b442d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -382,7 +382,7 @@ static void ide_set_signature(IDEState *s)
 
 typedef struct TrimAIOCB {
 BlockAIOCB common;
-BlockBackend *blk;
+IDEState *s;
 QEMUBH *bh;
 int ret;
 QEMUIOVector *qiov;
@@ -425,6 +425,8 @@ static void ide_trim_bh_cb(void *opaque)
 static void ide_issue_trim_cb(void *opaque, int ret)
 {
 TrimAIOCB *iocb = opaque;
+IDEState *s = iocb->s;
+
 if (ret >= 0) {
 while (iocb->j < iocb->qiov->niov) {
 int j = iocb->j;
@@ -442,7 +444,7 @@ static void ide_issue_trim_cb(void *opaque, int ret)
 }
 
 /* Got an entry! Submit and exit.  */
-iocb->aiocb = blk_aio_pdiscard(iocb->blk,
+iocb->aiocb = blk_aio_pdiscard(s->blk,
sector << BDRV_SECTOR_BITS,
count << BDRV_SECTOR_BITS,
ide_issue_trim_cb, opaque);
@@ -466,11 +468,11 @@ BlockAIOCB *ide_issue_trim(
 int64_t offset, QEMUIOVector *qiov,
 BlockCompletionFunc *cb, void *cb_opaque, void *opaque)
 {
-BlockBackend *blk = opaque;
+IDEState *s = opaque;
 TrimAIOCB *iocb;
 
-iocb = blk_aio_get(_aiocb_info, blk, cb, cb_opaque);
-iocb->blk = blk;
+iocb = blk_aio_get(_aiocb_info, s->blk, cb, cb_opaque);
+iocb->s = s;
 iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
 iocb->ret = 0;
 iocb->qiov = qiov;
@@ -900,7 +902,7 @@ static void ide_dma_cb(void *opaque, int ret)
 case IDE_DMA_TRIM:
 s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
 >sg, offset, BDRV_SECTOR_SIZE,
-ide_issue_trim, s->blk, ide_dma_cb, s,
+ide_issue_trim, s, ide_dma_cb, s,
 DMA_DIRECTION_TO_DEVICE);
 break;
 default:
-- 
2.14.3

Re: [Qemu-devel] [PATCH 0/3] ide: abort TRIM operation for invalid range

[John Snow]

On 12/08/2017 07:10 AM, Anton Nefedov wrote:
> Started from the separate series discussion (trim statistics) , see
> http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01059.html
> 
> There is no range check for IDE trim requests now.
> Such request will likely be rejected by the block layer and count as
> failed and not an invalid/aborted operation.
> 
> Anton Nefedov (3):
>   ide: pass IDEState to trim AIO callback
>   ide: move ide_sect_range_ok() up
>   ide: abort TRIM operation for invalid range
> 
>  hw/ide/core.c | 53 +
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 

I forgot about this series due to the 2.11 freeze and winter break. It
appears to still apply, so I'll send it along.

Re: [Qemu-devel] [PATCH v2] hw/ide: Remove duplicated definitions from ahci_internal.h

[John Snow]

On 12/07/2017 12:47 AM, Thomas Huth wrote:
> On 06.12.2017 23:16, John Snow wrote:
>> I tweaked this again, sorry:
>>
>> The names need to stay public, but the wrappers to manipulate the
>> objects can stay internal. Minor difference.
>>
>> If that's okay, I'll just merge this in.
>> OK?
> 
> Sure. Feel also free to replace my "Signed-off-by" with "Reported-by" in
> that case if you like.
> 
>  Thomas
> 

I forgot about this. Sending the PR now.

Re: [Qemu-devel] [PATCH] ide-test: test trim requests

[John Snow]

On 01/19/2018 07:40 AM, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov 
> ---
>  tests/ide-test.c | 71 
> 
>  1 file changed, 71 insertions(+)
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index aa9de06..259f39f 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -52,6 +52,7 @@
>  enum {
>  reg_data= 0x0,
>  reg_feature = 0x1,
> +reg_error   = 0x1,
>  reg_nsectors= 0x2,
>  reg_lba_low = 0x3,
>  reg_lba_middle  = 0x4,
> @@ -69,6 +70,11 @@ enum {
>  ERR = 0x01,
>  };
>  
> +/* Error field */
> +enum {
> +ABRT= 0x04,
> +};
> +
>  enum {
>  DEV = 0x10,
>  LBA = 0x40,
> @@ -81,6 +87,7 @@ enum {
>  };
>  
>  enum {
> +CMD_DSM = 0x06,
>  CMD_READ_DMA= 0xc8,
>  CMD_WRITE_DMA   = 0xca,
>  CMD_FLUSH_CACHE = 0xe7,
> @@ -179,6 +186,12 @@ typedef struct PrdtEntry {
>  #define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==, 
> (mask))
>  #define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
>  
> +static uint64_t trim_range_le(uint64_t sector, uint64_t count)
> +{
> +/* 2-byte range, 6-byte LBA */
> +return cpu_to_le64((count << 48) + sector);
> +}
> +

if count can only be two bytes, why not make it uint16_t?

>  static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>  PrdtEntry *prdt, int prdt_entries,
>  void(*post_exec)(QPCIDevice *dev, QPCIBar 
> ide_bar,
> @@ -204,6 +217,7 @@ static int send_dma_request(int cmd, uint64_t sector, int 
> nb_sectors,
>   * the SCSI command being sent in the packet, too. */
>  from_dev = true;
>  break;
> +case CMD_DSM:
>  case CMD_WRITE_DMA:
>  from_dev = false;
>  break;
> @@ -234,6 +248,10 @@ static int send_dma_request(int cmd, uint64_t sector, 
> int nb_sectors,
>  /* Enables ATAPI DMA; otherwise PIO is attempted */
>  qpci_io_writeb(dev, ide_bar, reg_feature, 0x01);
>  } else {
> +if (cmd == CMD_DSM) {
> +/* trim bit */
> +qpci_io_writeb(dev, ide_bar, reg_feature, 0x01);
> +}
>  qpci_io_writeb(dev, ide_bar, reg_nsectors, nb_sectors);
>  qpci_io_writeb(dev, ide_bar, reg_lba_low,sector & 0xff);
>  qpci_io_writeb(dev, ide_bar, reg_lba_middle, (sector >> 8) & 0xff);
> @@ -344,6 +362,58 @@ static void test_bmdma_simple_rw(void)
>  g_free(cmpbuf);
>  }
>  
> +static void test_bmdma_trim(void)
> +{
> +QPCIDevice *dev;
> +QPCIBar bmdma_bar, ide_bar;
> +uint8_t status;
> +const uint64_t random_range[] = { trim_range_le(0, 2),
> +  trim_range_le(6, 8),
> +  trim_range_le(10, 1),
> +};

Maybe just "trim_range" -- it's not /random/.

> +const uint64_t bad_range = trim_range_le(TEST_IMAGE_SIZE / 512 - 1, 2);
> +size_t len = 512;
> +uint8_t *buf;
> +uintptr_t guest_buf = guest_alloc(guest_malloc, len);
> +
> +PrdtEntry prdt[] = {
> +{
> +.addr = cpu_to_le32(guest_buf),
> +.size = cpu_to_le32(len | PRDT_EOT),
> +},
> +};
> +
> +dev = get_pci_device(_bar, _bar);
> +
> +buf = g_malloc(len);
> +
> +/* Normal request */
> +*((uint64_t *)buf) = random_range[0];
> +*((uint64_t *)buf + 1) = random_range[1];
> +
> +memwrite(guest_buf, buf, 2 * sizeof(uint64_t));
> +
> +status = send_dma_request(CMD_DSM, 0, 1, prdt,
> +  ARRAY_SIZE(prdt), NULL);
> +g_assert_cmphex(status, ==, BM_STS_INTR);
> +assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
> +
> +/* Request contains invalid range */
> +*((uint64_t *)buf) = random_range[2];
> +*((uint64_t *)buf + 1) = bad_range;
> +
> +memwrite(guest_buf, buf, 2 * sizeof(uint64_t));
> +
> +status = send_dma_request(CMD_DSM, 0, 1, prdt,
> +  ARRAY_SIZE(prdt), NULL);
> +g_assert_cmphex(status, ==, BM_STS_INTR);
> +assert_bit_set(qpci_io_readb(dev, ide_bar, reg_status), ERR);
> +assert_bit_set(qpci_io_readb(dev, ide_bar, reg_error), ABRT);
> +
> +free_pci_device(dev);
> +g_free(buf);
> +}
> +
>  static void test_bmdma_short_prdt(void)
>  {
>  QPCIDevice *dev;
> @@ -963,6 +1033,7 @@ int main(int argc, char **argv)
>  
>  qtest_add_func("/ide/bmdma/setup", test_bmdma_setup);
>  qtest_add_func("/ide/bmdma/simple_rw", test_bmdma_simple_rw);
> +qtest_add_func("/ide/bmdma/trim", test_bmdma_trim);
>  qtest_add_func("/ide/bmdma/short_prdt", test_bmdma_short_prdt);
>  qtest_add_func("/ide/bmdma/one_sector_short_prdt",
> test_bmdma_one_sector_short_prdt);
> 

looks good except for those two very small nits.

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/19/2018 01:42 PM, Eduardo Habkost wrote:

On Fri, Jan 19, 2018 at 12:10:03PM -0500, Stefan Berger wrote:

On 01/19/2018 09:11 AM, Marc-André Lureau wrote:

tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
Interface as defined in TCG PC Client Platform TPM Profile (PTP)
Specification Family “2.0” Level 00 Revision 01.03 v22.

The PTP allows device implementation to switch between TIS and CRB
model at run time, but given that CRB is a simpler device to
implement, I chose to implement it as a different device.

The device doesn't implement other locality than 0 for now (my laptop
TPM doesn't either, so I assume this isn't so bad)

The command/reply memory region is statically allocated after the CRB
registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
wonder if the BIOS could or should allocate it instead, or what size
to use, again this seems to fit well expectations)

I removed this last sentence now. It's at the right location.


The PTP doesn't specify a particular bus to put the device. So I added
it on the system bus directly, so it could hopefully be used easily on
a different platform than x86. Currently, it fails to init on piix,
because error_on_sysbus_device() check. The check may be changed in a
near future, see discussion on the qemu-devel ML.

I think this has to be solved. So I remove these last 2 sentences. I'll have
to wait until that other patch series from Eduard is merged since it doesn't
start yet.

The series was just merged to master.  It's possible to make a
machine accept the new device using
machine_class_allow_dynamic_sysbus_dev(), now.


I saw that.


However, is it really necessary to make it a sysbus device?
Having bus-less devices was not possible in the past, but it is
possible today.

What I don't like about it is the fact that I would have to use q35 if 
we only extend that machine type to allow this sysbus device. What is 
the reason that dynamic sysbus devices have to explicitly be allowed? If 
we don't need to limit this device to a certain machine type that may be 
more user friendly.

Re: [Qemu-devel] [PATCH 02/11] smbus_eeprom: replace SMBusDeviceClass::init by DeviceClass::reset

[Philippe Mathieu-Daudé]

On 01/19/2018 03:15 PM, Eduardo Habkost wrote:
> On Tue, Jan 16, 2018 at 10:15:46AM -0300, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/i2c/smbus_eeprom.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index b13ec0fe7a..7e81ae4fe5 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -97,12 +97,11 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, 
>> uint8_t cmd, int n)
>>  return eeprom_receive_byte(dev);
>>  }
>>  
>> -static int smbus_eeprom_initfn(SMBusDevice *dev)
>> +static void smbus_eeprom_reset(DeviceState *dev)
>>  {
>>  SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
>>  
>>  eeprom->offset = 0;
>> -return 0;
>>  }
>>  
>>  static Property smbus_eeprom_properties[] = {
>> @@ -115,7 +114,7 @@ static void smbus_eeprom_class_initfn(ObjectClass 
>> *klass, void *data)
>>  DeviceClass *dc = DEVICE_CLASS(klass);
>>  SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>  
>> -sc->init = smbus_eeprom_initfn;
>> +dc->reset = smbus_eeprom_reset;
> 
> Trying to track down when each of these methods is called:
> 
> sc->init (SMBusDeviceClass::init)
>  -> called by i2c_slave_qdev_init() (DeviceClass:init)
>-> called by device_realize() (DeviceClass::realize)
>  -> called by device_set_realized()
>-> QOM setter for "realized" property
>  -> (multiple callers)
> 
> (eww, so many indirections)
> 
> dc->reset (DeviceClass::reset)
>   -> called by device_reset()
> -> (multiple callers)
> 
> (much better!)
> 
> 
> It looks like this changes how the device behaves.  Is this
> fixing a device emulation bug?  If not, why should the device set
> offset=0 on DeviceClass::reset, and not DeviceClass::realize?

You right, if this is a device bugfix (which I'm not sure) this is
unrelated to this series, so I'll just drop it.

> 
> 
>>  sc->quick_cmd = eeprom_quick_cmd;
>>  sc->send_byte = eeprom_send_byte;
>>  sc->receive_byte = eeprom_receive_byte;
>> -- 
>> 2.15.1
>>
>>
> 

Re: [Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Corey Minyard]

On 01/19/2018 08:07 AM, Corey Minyard wrote:

On 01/18/2018 09:17 PM, Michael S. Tsirkin wrote:

On Thu, Jan 18, 2018 at 07:55:41PM -0600, miny...@acm.org wrote:

From: Corey Minyard 

This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.

The commit being reverted says:

 PIIX4 errata says that "immediate polling of the Host Status 
Register BUSY

 bit may indicate that the SMBus is NOT busy."
 Due to this, some code does the following steps:
 (a) set parameters
 (b) start command
 (c) check for smbus busy bit set (to know that command started)
 (d) check for smbus busy bit not set (to know that command 
finished)


 Let (c) happen, by immediately setting the busy bit, and really 
executing

 the command when status register has been read once.

 This fixes a problem with AMIBIOS, which can now properly 
initialize the

 PIIX4.

Emulating bad hardware so badly written software will work doesn't 
sound

like a good idea to me.  I have patches that add interrupt capability
to pm_smbus, but this change breaks that because the Linux driver
starts the transaction then waits for interrupts before reading the
status register.  That obviously won't work with these changes.

The right way to fix this in AMIBIOS is to ignore the host busy bit
and use the other bits in the host status register to tell if the
transaction has completed.  Using host busy is racy, anyway, if you
get interrupted or something while processing, you may miss step (c)
in your algorithm and fail.

Cc: Hervé Poussineau 
Cc: Philippe Mathieu-Daudé 
Signed-off-by: Corey Minyard 

Would it be possible to limit the change to when guest uses
interrupts?


I did think about that, but it seems rather frail.  What if another 
piece of software
does this but has the interrupt enable bit set?  And AMIBIOS is still 
broken doing
that algorithm on real hardware.  If you get a bus collision, for 
instance, that will

be almost instantaneous and the firmware is likely to miss it.

The 82801 documentation is pretty clear that you should use the INTR 
and error

bits in the status register to know if a transaction is complete.

If you really want to emulate real hardware, I guess the right way to 
do this
would be to add a delay between the start bit being set and the 
transaction
being done.  I'm not sure how timers work with vmstate, I'd have to 
look at

that.


I realized that the timer is not going to be able to correctly work 
around the

AMIBIOS.  It would probably work most of the time, but if qemu got switched
out, then switched back and the timer went off before the guest was allowed
to run, then you would have the same issue.

Also, looking at a more complete implementation of the pm_smbus device,
using the host busy bit to know when to start the transaction won't work,
that bit also does other things when doing byte at a time block transfers.
So a separate bool is needed to know when to do this.

-corey



IMHO it's best to revert this change and fix AMIBIOS.  If that is
impossible, then adding the delay or doing the interrupt enable
thing you suggest (assuming AMIBIOS doesn't have interrupts
enabled), and fixing that assert would be best.  I can submit
a patch either way, depending on what you want.

-corey


---
  hw/i2c/pm_smbus.c | 16 +---
  1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 0d26e0f..a044dd1 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -62,9 +62,6 @@ static void smb_transaction(PMSMBus *s)
  I2CBus *bus = s->smbus;
  int ret;
  -    assert(s->smb_stat & STS_HOST_BUSY);
-    s->smb_stat &= ~STS_HOST_BUSY;
-
  SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, 
prot);

  /* Transaction isn't exec if STS_DEV_ERR bit set */
  if ((s->smb_stat & STS_DEV_ERR) != 0)  {
@@ -137,13 +134,6 @@ error:
    }
  -static void smb_transaction_start(PMSMBus *s)
-{
-    /* Do not execute immediately the command ; it will be
- * executed when guest will read SMB_STAT register */
-    s->smb_stat |= STS_HOST_BUSY;
-}
-
  static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t 
val,

    unsigned width)
  {
@@ -159,7 +149,7 @@ static void smb_ioport_writeb(void *opaque, 
hwaddr addr, uint64_t val,

  case SMBHSTCNT:
  s->smb_ctl = val;
  if (val & 0x40)
-    smb_transaction_start(s);
+    smb_transaction(s);
  break;
  case SMBHSTCMD:
  s->smb_cmd = val;
@@ -191,10 +181,6 @@ static uint64_t smb_ioport_readb(void *opaque, 
hwaddr addr, unsigned width)

  switch(addr) {
  case SMBHSTSTS:
  val = s->smb_stat;
-    if (s->smb_stat & STS_HOST_BUSY) {
-    /* execute command now */
-    smb_transaction(s);
-    }
  break;
  case SMBHSTCNT:
  s->smb_index = 0;

Re: [Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

[no-reply]

Hi,

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

Type: series
Message-id: 20180119205847.7141-1-js...@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

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

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

git config --local diff.renamelimit 0
git config --local diff.renames True

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

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/20180119162554.27270-1-marcandre.lur...@redhat.com -> 
patchew/20180119162554.27270-1-marcandre.lur...@redhat.com
 * [new tag]   patchew/20180119205847.7141-1-js...@redhat.com -> 
patchew/20180119205847.7141-1-js...@redhat.com
Switched to a new branch 'test'
969dddc4fd blockjob: remove block_job_pause_point from interface
792539a07e blockjob: privatize block_job_sleep_ns
ea6f8f2359 block/mirror: remove block_job_sleep_ns calls
7f2ee60c8b block/mirror: condense cancellation and relax calls
9a16d6778e block/backup: remove yield_and_check
bbfcfe9c89 allow block_job_relax to return -ECANCELED
b1324b111c block/backup: use block_job_relax
d208db6fca block/stream: use block_job_relax
788ece436a block/commit: use block_job_relax
4e209e7aed blockjob: allow block_job_throttle to take delay_ns
76ce42710b blockjob: create block_job_relax
9771e84ab8 blockjob: consolidate SLICE_TIME definition
2a1eec68ac blockjob: record time of last entrance

=== OUTPUT BEGIN ===
Checking PATCH 1/13: blockjob: record time of last entrance...
WARNING: line over 80 characters
#52: FILE: block/mirror.c:803:
+delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - 
s->common.last_enter_ns;

total: 0 errors, 1 warnings, 63 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/13: blockjob: consolidate SLICE_TIME definition...
Checking PATCH 3/13: blockjob: create block_job_relax...
Checking PATCH 4/13: blockjob: allow block_job_throttle to take delay_ns...
Checking PATCH 5/13: block/commit: use block_job_relax...
Checking PATCH 6/13: block/stream: use block_job_relax...
Checking PATCH 7/13: block/backup: use block_job_relax...
Checking PATCH 8/13: allow block_job_relax to return -ECANCELED...
Checking PATCH 9/13: block/backup: remove yield_and_check...
Checking PATCH 10/13: block/mirror: condense cancellation and relax calls...
Checking PATCH 11/13: block/mirror: remove block_job_sleep_ns calls...
ERROR: do not initialise statics to 0 or NULL
#30: FILE: block/mirror.c:764:
+static uint64_t delay_ns = 0;

total: 1 errors, 0 warnings, 50 lines checked

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

Checking PATCH 12/13: blockjob: privatize block_job_sleep_ns...
Checking PATCH 13/13: blockjob: remove block_job_pause_point from interface...
=== OUTPUT END ===

Test command exited with code: 1


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

[Qemu-devel] [PATCH v2 11/13] block/mirror: remove block_job_sleep_ns calls

[John Snow]

We're attempting to slacken the mirror loop in three different places,
but we can combine these three attempts. Combine the early loop call to
block_job_pause_point with the two late-loop calls to block_job_sleep_ns.

When delay_ns is 0 and it has not been SLICE_TIME since the last yield,
block_job_relax is merely a call to block_job_pause_point, so this should
be equivalent with the exception that if we have managed to not yield at
all in the last SLICE_TIME ns, we will now do so.

I am not sure that condition was possible,
so this loop should be equivalent.

Signed-off-by: John Snow 
---
 block/mirror.c | 22 +++---
 block/trace-events |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index a0e0044de2..192e03694f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -761,7 +761,7 @@ static void coroutine_fn mirror_run(void *opaque)
 assert(!s->dbi);
 s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
 for (;;) {
-uint64_t delay_ns = 0;
+static uint64_t delay_ns = 0;
 int64_t cnt, delta;
 bool should_complete;
 
@@ -770,9 +770,16 @@ static void coroutine_fn mirror_run(void *opaque)
 goto immediate_exit;
 }
 
-block_job_pause_point(>common);
-
 cnt = bdrv_get_dirty_count(s->dirty_bitmap);
+
+trace_mirror_before_relax(s, cnt, s->synced, delay_ns);
+if (block_job_relax(>common, delay_ns)) {
+if (!s->synced) {
+goto immediate_exit;
+}
+}
+delay_ns = 0;
+
 /* s->common.offset contains the number of bytes already processed so
  * far, cnt is the number of dirty bytes remaining and
  * s->bytes_in_flight is the number of bytes currently being
@@ -849,15 +856,8 @@ static void coroutine_fn mirror_run(void *opaque)
 }
 
 ret = 0;
-trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
-if (!s->synced) {
-block_job_sleep_ns(>common, delay_ns);
-if (block_job_is_cancelled(>common)) {
-break;
-}
-} else if (!should_complete) {
+if (s->synced && !should_complete) {
 delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
-block_job_sleep_ns(>common, delay_ns);
 }
 }
 
diff --git a/block/trace-events b/block/trace-events
index 11c8d5f590..60f36f929a 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -27,7 +27,7 @@ mirror_start(void *bs, void *s, void *opaque) "bs %p s %p 
opaque %p"
 mirror_restart_iter(void *s, int64_t cnt) "s %p dirty count %"PRId64
 mirror_before_flush(void *s) "s %p"
 mirror_before_drain(void *s, int64_t cnt) "s %p dirty count %"PRId64
-mirror_before_sleep(void *s, int64_t cnt, int synced, uint64_t delay_ns) "s %p 
dirty count %"PRId64" synced %d delay %"PRIu64"ns"
+mirror_before_relax(void *s, int64_t cnt, int synced, uint64_t delay_ns) "s %p 
dirty count %"PRId64" synced %d delay %"PRIu64"ns"
 mirror_one_iteration(void *s, int64_t offset, uint64_t bytes) "s %p offset %" 
PRId64 " bytes %" PRIu64
 mirror_iteration_done(void *s, int64_t offset, uint64_t bytes, int ret) "s %p 
offset %" PRId64 " bytes %" PRIu64 " ret %d"
 mirror_yield(void *s, int64_t cnt, int buf_free_count, int in_flight) "s %p 
dirty count %"PRId64" free buffers %d in_flight %d"
-- 
2.14.3

[Qemu-devel] [PATCH v2 10/13] block/mirror: condense cancellation and relax calls

[John Snow]

We can count on the relax call to check cancellation for us, so
condense these concurrent calls.

Signed-off-by: John Snow 
---
 block/mirror.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 3c73caed5e..a0e0044de2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -610,9 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int bytes = MIN(s->bdev_length - offset,
 QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-block_job_relax(>common, 0);
-
-if (block_job_is_cancelled(>common)) {
+if (block_job_relax(>common, 0)) {
 s->initial_zeroing_ongoing = false;
 return 0;
 }
@@ -638,9 +636,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int bytes = MIN(s->bdev_length - offset,
 QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-block_job_relax(>common, 0);
-
-if (block_job_is_cancelled(>common)) {
+if (block_job_relax(>common, 0)) {
 return 0;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 13/13] blockjob: remove block_job_pause_point from interface

[John Snow]

Remove the last call in block/mirror, using relax instead.
relax may do nothing if we are canceled, so allow iteration to return
prematurely and allow mirror_run to handle the cancellation logic.

This is a functional change to mirror that should have the effect of
cancelled mirror jobs being able to respond to that request a little
sooner instead of launching new requests.

Signed-off-by: John Snow 
---
 block/mirror.c   |  4 +++-
 blockjob.c   | 10 +-
 include/block/blockjob_int.h |  9 -
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 192e03694f..8e6b5b25a9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -345,7 +345,9 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 mirror_wait_for_io(s);
 }
 
-block_job_pause_point(>common);
+if (block_job_relax(>common, 0)) {
+return 0;
+}
 
 /* Find the number of consective dirty chunks following the first dirty
  * one, and wait for in flight requests in them. */
diff --git a/blockjob.c b/blockjob.c
index 40167d6896..27c13fdd08 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -60,6 +60,7 @@ static void __attribute__((__constructor__)) 
block_job_init(void)
 static void block_job_event_cancelled(BlockJob *job);
 static void block_job_event_completed(BlockJob *job, const char *msg);
 static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
+static int coroutine_fn block_job_pause_point(BlockJob *job);
 
 /* Transactional group of block jobs */
 struct BlockJobTxn {
@@ -793,7 +794,14 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
 assert(job->busy);
 }
 
-int coroutine_fn block_job_pause_point(BlockJob *job)
+/**
+ * block_job_pause_point:
+ * @job: The job that is ready to pause.
+ *
+ * Pause now if block_job_pause() has been called.  Block jobs that perform
+ * lots of I/O must call this between requests so that the job can be paused.
+ */
+static int coroutine_fn block_job_pause_point(BlockJob *job)
 {
 assert(job && block_job_started(job));
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c4891a5a9b..57327cbc5a 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -201,15 +201,6 @@ void block_job_completed(BlockJob *job, int ret);
  */
 bool block_job_is_cancelled(BlockJob *job);
 
-/**
- * block_job_pause_point:
- * @job: The job that is ready to pause.
- *
- * Pause now if block_job_pause() has been called.  Block jobs that perform
- * lots of I/O must call this between requests so that the job can be paused.
- */
-int coroutine_fn block_job_pause_point(BlockJob *job);
-
 /**
  * block_job_enter:
  * @job: The job to enter.
-- 
2.14.3

[Qemu-devel] [PATCH v2 12/13] blockjob: privatize block_job_sleep_ns

[John Snow]

There's not currently any external caller of it.

Except in tests, but we'll fix that here too.

Replace usages in test cases with block_job_relax, which functions
similarly enough to be used as a drop-in replacement.

Very technically block_job_sleep_ns(job, 0) behaves differently
from block_job_relax(job, 0) in that relax may resolve to a no-op,
but this makes no difference in the test in which it is used.

Signed-off-by: John Snow 
---
 blockjob.c   | 11 ++-
 include/block/blockjob_int.h | 11 ---
 tests/test-bdrv-drain.c  |  2 +-
 tests/test-blockjob-txn.c|  2 +-
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index b5a0cda412..40167d6896 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -876,7 +876,16 @@ bool block_job_is_cancelled(BlockJob *job)
 return job->cancelled;
 }
 
-int block_job_sleep_ns(BlockJob *job, int64_t ns)
+/**
+ * block_job_sleep_ns:
+ * @job: The job that calls the function.
+ * @ns: How many nanoseconds to stop for.
+ *
+ * Put the job to sleep (assuming that it wasn't canceled) for @ns
+ * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
+ * interrupt the wait.
+ */
+static int block_job_sleep_ns(BlockJob *job, int64_t ns)
 {
 assert(job->busy);
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 1ceb47e1e6..c4891a5a9b 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -138,17 +138,6 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
uint64_t shared_perm, int64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp);
 
-/**
- * block_job_sleep_ns:
- * @job: The job that calls the function.
- * @ns: How many nanoseconds to stop for.
- *
- * Put the job to sleep (assuming that it wasn't canceled) for @ns
- * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
- * interrupt the wait.
- */
-int block_job_sleep_ns(BlockJob *job, int64_t ns);
-
 /**
  * block_job_yield:
  * @job: The job that calls the function.
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index d760e2b243..5d47541b4c 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -506,7 +506,7 @@ static void coroutine_fn test_job_start(void *opaque)
 TestBlockJob *s = opaque;
 
 while (!s->should_complete) {
-block_job_sleep_ns(>common, 10);
+block_job_relax(>common, 10);
 }
 
 block_job_defer_to_main_loop(>common, test_job_completed, NULL);
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 3591c9617f..edcf0de6bd 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -44,7 +44,7 @@ static void coroutine_fn test_block_job_run(void *opaque)
 
 while (s->iterations--) {
 if (s->use_timer) {
-block_job_sleep_ns(job, 0);
+block_job_relax(job, 0);
 } else {
 block_job_yield(job);
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 08/13] allow block_job_relax to return -ECANCELED

[John Snow]

This is just an optimization for callers who are likely going to
want to check quite close to this call if the job was canceled or
not anyway.

Along the same lines, add the return to block_job_pause_point and
block_job_sleep_ns, so we don't have to re-check it quite so
excessively.

Signed-off-by: John Snow 
---
 blockjob.c   | 28 +---
 include/block/blockjob_int.h |  8 +---
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 51c0eb5d9e..b5a0cda412 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -793,15 +793,15 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
 assert(job->busy);
 }
 
-void coroutine_fn block_job_pause_point(BlockJob *job)
+int coroutine_fn block_job_pause_point(BlockJob *job)
 {
 assert(job && block_job_started(job));
 
-if (!block_job_should_pause(job)) {
-return;
-}
 if (block_job_is_cancelled(job)) {
-return;
+return -ECANCELED;
+}
+if (!block_job_should_pause(job)) {
+return 0;
 }
 
 if (job->driver->pause) {
@@ -817,6 +817,8 @@ void coroutine_fn block_job_pause_point(BlockJob *job)
 if (job->driver->resume) {
 job->driver->resume(job);
 }
+
+return 0;
 }
 
 void block_job_resume_all(void)
@@ -874,20 +876,20 @@ bool block_job_is_cancelled(BlockJob *job)
 return job->cancelled;
 }
 
-void block_job_sleep_ns(BlockJob *job, int64_t ns)
+int block_job_sleep_ns(BlockJob *job, int64_t ns)
 {
 assert(job->busy);
 
 /* Check cancellation *before* setting busy = false, too!  */
 if (block_job_is_cancelled(job)) {
-return;
+return -ECANCELED;
 }
 
 if (!block_job_should_pause(job)) {
 block_job_do_yield(job, qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + ns);
 }
 
-block_job_pause_point(job);
+return block_job_pause_point(job);
 }
 
 void block_job_yield(BlockJob *job)
@@ -906,13 +908,17 @@ void block_job_yield(BlockJob *job)
 block_job_pause_point(job);
 }
 
-void block_job_relax(BlockJob *job, int64_t delay_ns)
+int block_job_relax(BlockJob *job, int64_t delay_ns)
 {
+if (block_job_is_cancelled(job)) {
+return -ECANCELED;
+}
+
 if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
  job->last_enter_ns > SLICE_TIME)) {
-block_job_sleep_ns(job, delay_ns);
+return block_job_sleep_ns(job, delay_ns);
 } else {
-block_job_pause_point(job);
+return block_job_pause_point(job);
 }
 }
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 5f1520fab7..1ceb47e1e6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -147,7 +147,7 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
  * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
  * interrupt the wait.
  */
-void block_job_sleep_ns(BlockJob *job, int64_t ns);
+int block_job_sleep_ns(BlockJob *job, int64_t ns);
 
 /**
  * block_job_yield:
@@ -167,8 +167,10 @@ void block_job_yield(BlockJob *job);
  * If delay_ns is 0, yield if it has been SLICE_TIME
  * nanoseconds since the last yield. Otherwise, check
  * if we need to yield for a pause event.
+ *
+ * returns ECANCELED if the job has been canceled.
  */
-void block_job_relax(BlockJob *job, int64_t delay_ns);
+int block_job_relax(BlockJob *job, int64_t delay_ns);
 
 /**
  * block_job_pause_all:
@@ -217,7 +219,7 @@ bool block_job_is_cancelled(BlockJob *job);
  * Pause now if block_job_pause() has been called.  Block jobs that perform
  * lots of I/O must call this between requests so that the job can be paused.
  */
-void coroutine_fn block_job_pause_point(BlockJob *job);
+int coroutine_fn block_job_pause_point(BlockJob *job);
 
 /**
  * block_job_enter:
-- 
2.14.3

[Qemu-devel] [PATCH v2 07/13] block/backup: use block_job_relax

[John Snow]

See two commits back for justification.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
---
 block/backup.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 7b1cdd038a..b4204c0ee4 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -336,6 +336,8 @@ static void backup_complete(BlockJob *job, void *opaque)
 
 static bool coroutine_fn yield_and_check(BackupBlockJob *job)
 {
+uint64_t delay_ns = 0;
+
 if (block_job_is_cancelled(>common)) {
 return true;
 }
@@ -344,14 +346,12 @@ static bool coroutine_fn yield_and_check(BackupBlockJob 
*job)
  * (without, VM does not reboot)
  */
 if (job->common.speed) {
-uint64_t delay_ns = ratelimit_calculate_delay(>limit,
-  job->bytes_read);
+delay_ns = ratelimit_calculate_delay(>limit,
+ job->bytes_read);
 job->bytes_read = 0;
-block_job_sleep_ns(>common, delay_ns);
-} else {
-block_job_sleep_ns(>common, 0);
 }
 
+block_job_relax(>common, delay_ns);
 if (block_job_is_cancelled(>common)) {
 return true;
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 05/13] block/commit: use block_job_relax

[John Snow]

Depending on the value of `speed` and how fast our backends are,
delay_ns might be 0 very, very often. This creates some warning
messages that spook users, but it's also pretty inefficient.

Use block_job_relax instead to yield a little more intelligently.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 898545b318..9cfe3ed76b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -172,7 +172,7 @@ static void coroutine_fn commit_run(void *opaque)
 /* Note that even when no rate limit is applied we need to yield
  * with no pending I/O here so that bdrv_drain_all() returns.
  */
-block_job_sleep_ns(>common, delay_ns);
+block_job_relax(>common, delay_ns);
 if (block_job_is_cancelled(>common)) {
 break;
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 09/13] block/backup: remove yield_and_check

[John Snow]

This is a respin of the same functionality as mirror_throttle,
so trash this and replace it with the generic version.

yield_and_check returned true if canceled, false otherwise.
block_job_relax returns -ECANCELED if canceled, 0 otherwise.

Signed-off-by: John Snow 
---
 block/backup.c | 20 
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index b4204c0ee4..0624c3b322 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -334,29 +334,17 @@ static void backup_complete(BlockJob *job, void *opaque)
 g_free(data);
 }
 
-static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+static uint64_t get_delay_ns(BackupBlockJob *job)
 {
 uint64_t delay_ns = 0;
 
-if (block_job_is_cancelled(>common)) {
-return true;
-}
-
-/* we need to yield so that bdrv_drain_all() returns.
- * (without, VM does not reboot)
- */
 if (job->common.speed) {
 delay_ns = ratelimit_calculate_delay(>limit,
  job->bytes_read);
 job->bytes_read = 0;
 }
 
-block_job_relax(>common, delay_ns);
-if (block_job_is_cancelled(>common)) {
-return true;
-}
-
-return false;
+return delay_ns;
 }
 
 static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
@@ -369,7 +357,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 hbitmap_iter_init(, job->copy_bitmap, 0);
 while ((cluster = hbitmap_iter_next()) != -1) {
 do {
-if (yield_and_check(job)) {
+if (block_job_relax(>common, get_delay_ns(job))) {
 return 0;
 }
 ret = backup_do_cow(job, cluster * job->cluster_size,
@@ -465,7 +453,7 @@ static void coroutine_fn backup_run(void *opaque)
 bool error_is_read;
 int alloced = 0;
 
-if (yield_and_check(job)) {
+if (block_job_relax(>common, get_delay_ns(job))) {
 break;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 01/13] blockjob: record time of last entrance

[John Snow]

The mirror job makes a semi-inaccurate record of the last time we yielded
by recording the last time we left a "pause", but this doesn't always
correlate to the time we actually last successfully ceded control.

Record the time we last *exited* a yield centrally. In other words, record
the time we began execution of this job to know how long we have been
selfish for.

Signed-off-by: John Snow 
---
 block/mirror.c   | 8 ++--
 blockjob.c   | 2 ++
 include/block/blockjob.h | 5 +
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9badc1203..88f4e8964d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -63,7 +63,6 @@ typedef struct MirrorBlockJob {
 QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
 int buf_free_count;
 
-uint64_t last_pause_ns;
 unsigned long *in_flight_bitmap;
 int in_flight;
 int64_t bytes_in_flight;
@@ -596,8 +595,7 @@ static void mirror_throttle(MirrorBlockJob *s)
 {
 int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-if (now - s->last_pause_ns > SLICE_TIME) {
-s->last_pause_ns = now;
+if (now - s->common.last_enter_ns > SLICE_TIME) {
 block_job_sleep_ns(>common, 0);
 } else {
 block_job_pause_point(>common);
@@ -769,7 +767,6 @@ static void coroutine_fn mirror_run(void *opaque)
 
 mirror_free_init(s);
 
-s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 if (!s->is_none_mode) {
 ret = mirror_dirty_init(s);
 if (ret < 0 || block_job_is_cancelled(>common)) {
@@ -803,7 +800,7 @@ static void coroutine_fn mirror_run(void *opaque)
  * We do so every SLICE_TIME nanoseconds, or when there is an error,
  * or when the source is clean, whichever comes first.
  */
-delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->last_pause_ns;
+delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - 
s->common.last_enter_ns;
 if (delta < SLICE_TIME &&
 s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
 if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
@@ -878,7 +875,6 @@ static void coroutine_fn mirror_run(void *opaque)
 delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
 block_job_sleep_ns(>common, delay_ns);
 }
-s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 }
 
 immediate_exit:
diff --git a/blockjob.c b/blockjob.c
index f5cea84e73..2a9ff66b95 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -321,6 +321,7 @@ void block_job_start(BlockJob *job)
 job->pause_count--;
 job->busy = true;
 job->paused = false;
+job->last_enter_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 bdrv_coroutine_enter(blk_bs(job->blk), job->co);
 }
 
@@ -786,6 +787,7 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
 job->busy = false;
 block_job_unlock();
 qemu_coroutine_yield();
+job->last_enter_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
 /* Set by block_job_enter before re-entering the coroutine.  */
 assert(job->busy);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 00403d9482..e965845c94 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -141,6 +141,11 @@ typedef struct BlockJob {
  */
 QEMUTimer sleep_timer;
 
+/**
+ * Timestamp of the last yield
+ */
+uint64_t last_enter_ns;
+
 /** Non-NULL if this job is part of a transaction */
 BlockJobTxn *txn;
 QLIST_ENTRY(BlockJob) txn_list;
-- 
2.14.3

[Qemu-devel] [PATCH v2 06/13] block/stream: use block_job_relax

[John Snow]

See prior commit for justification.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
---
 block/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/stream.c b/block/stream.c
index e85af18c54..49dc102565 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -139,7 +139,7 @@ static void coroutine_fn stream_run(void *opaque)
 /* Note that even when no rate limit is applied we need to yield
  * with no pending I/O here so that bdrv_drain_all() returns.
  */
-block_job_sleep_ns(>common, delay_ns);
+block_job_relax(>common, delay_ns);
 if (block_job_is_cancelled(>common)) {
 break;
 }
-- 
2.14.3

[Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

[John Snow]

mirror_throttle attempts to make sure we yield every so often when we're
doing operations that may not otherwise be as cooperative as we'd like.

This pattern is useful to other jobs like commit as well; if commit
continuously decides not to copy data (and calculates delay_ns to be 0),
we'll frequently and aggressively yield, only to be rescheduled immediately.

This causes those "WARNING: I/O thread spun for 1000 iterations"
warnings that everyone loves so much.

Split out the mirror_throttle function to become a new internal
BlockJob API function, block_job_throttle; then use it for all
the other jobs in their runtime loops.

I decided to use it in stream and backup as well, so that regardless of if the
loop structure has the chance to be greedy or not (I did not audit this), the
BlockJob has some kind of failsafe that will occasionally perform a 0ns sleep
every SLICE_TIME.

v2:
 - Fixed spacing consistency (Paolo)
 - Renamed last_yield_ns to last_enter_ns (Stefan)
 - Renamed block_job_throttle to block_job_relax (Stefan)
 - Removed external usages of block_job_pause_point and
   block_job_sleep_ns (Paolo)
 - Further cut down block/backup code to use block_job_relax



For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch block-job-yield
https://github.com/jnsnow/qemu/tree/block-job-yield

This version is tagged block-job-yield-v2:
https://github.com/jnsnow/qemu/releases/tag/block-job-yield-v2

John Snow (13):
  blockjob: record time of last entrance
  blockjob: consolidate SLICE_TIME definition
  blockjob: create block_job_relax
  blockjob: allow block_job_throttle to take delay_ns
  block/commit: use block_job_relax
  block/stream: use block_job_relax
  block/backup: use block_job_relax
  allow block_job_relax to return -ECANCELED
  block/backup: remove yield_and_check
  block/mirror: condense cancellation and relax calls
  block/mirror: remove block_job_sleep_ns calls
  blockjob: privatize block_job_sleep_ns
  blockjob: remove block_job_pause_point from interface

 block/backup.c   | 27 ++-
 block/commit.c   |  4 +---
 block/mirror.c   | 52 +++-
 block/stream.c   |  4 +---
 block/trace-events   |  2 +-
 blockjob.c   | 51 ---
 include/block/blockjob.h |  5 +
 include/block/blockjob_int.h | 37 +++
 tests/test-bdrv-drain.c  |  2 +-
 tests/test-blockjob-txn.c|  2 +-
 10 files changed, 94 insertions(+), 92 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 02/13] blockjob: consolidate SLICE_TIME definition

[John Snow]

They're all the same. If it actually becomes important to configure it,
it can become a job or driver property.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Jeff Cody 
---
 block/backup.c   | 1 -
 block/commit.c   | 2 --
 block/mirror.c   | 1 -
 block/stream.c   | 2 --
 include/block/blockjob_int.h | 2 ++
 5 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 4a16a37229..7b1cdd038a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -27,7 +27,6 @@
 #include "qemu/error-report.h"
 
 #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
-#define SLICE_TIME 1ULL /* ns */
 
 typedef struct BackupBlockJob {
 BlockJob common;
diff --git a/block/commit.c b/block/commit.c
index bb6c904704..898545b318 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -31,8 +31,6 @@ enum {
 COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
 };
 
-#define SLICE_TIME 1ULL /* ns */
-
 typedef struct CommitBlockJob {
 BlockJob common;
 RateLimit limit;
diff --git a/block/mirror.c b/block/mirror.c
index 88f4e8964d..1fb5fc0cb8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -22,7 +22,6 @@
 #include "qemu/ratelimit.h"
 #include "qemu/bitmap.h"
 
-#define SLICE_TIME1ULL /* ns */
 #define MAX_IN_FLIGHT 16
 #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
 #define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
diff --git a/block/stream.c b/block/stream.c
index 499cdacdb0..e85af18c54 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -29,8 +29,6 @@ enum {
 STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
 };
 
-#define SLICE_TIME 1ULL /* ns */
-
 typedef struct StreamBlockJob {
 BlockJob common;
 RateLimit limit;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c9b23b0cc9..209fa1bb3e 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -29,6 +29,8 @@
 #include "block/blockjob.h"
 #include "block/block.h"
 
+#define SLICE_TIME 1ULL /* ns */
+
 /**
  * BlockJobDriver:
  *
-- 
2.14.3

[Qemu-devel] [PATCH v2 03/13] blockjob: create block_job_relax

[John Snow]

This will replace mirror_throttle, for reuse in other jobs.

Signed-off-by: John Snow 
---
 block/mirror.c   | 15 ++-
 blockjob.c   | 11 +++
 include/block/blockjob_int.h |  9 +
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 1fb5fc0cb8..36c681c7a1 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -590,17 +590,6 @@ static void mirror_exit(BlockJob *job, void *opaque)
 bdrv_unref(src);
 }
 
-static void mirror_throttle(MirrorBlockJob *s)
-{
-int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-if (now - s->common.last_enter_ns > SLICE_TIME) {
-block_job_sleep_ns(>common, 0);
-} else {
-block_job_pause_point(>common);
-}
-}
-
 static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 {
 int64_t offset;
@@ -621,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int bytes = MIN(s->bdev_length - offset,
 QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-mirror_throttle(s);
+block_job_relax(>common);
 
 if (block_job_is_cancelled(>common)) {
 s->initial_zeroing_ongoing = false;
@@ -649,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int bytes = MIN(s->bdev_length - offset,
 QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-mirror_throttle(s);
+block_job_relax(>common);
 
 if (block_job_is_cancelled(>common)) {
 return 0;
diff --git a/blockjob.c b/blockjob.c
index 2a9ff66b95..6f2e709b51 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -906,6 +906,17 @@ void block_job_yield(BlockJob *job)
 block_job_pause_point(job);
 }
 
+void block_job_relax(BlockJob *job)
+{
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+
+if (now - job->last_enter_ns > SLICE_TIME) {
+block_job_sleep_ns(job, 0);
+} else {
+block_job_pause_point(job);
+}
+}
+
 void block_job_iostatus_reset(BlockJob *job)
 {
 if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 209fa1bb3e..553784d86f 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -157,6 +157,15 @@ void block_job_sleep_ns(BlockJob *job, int64_t ns);
  */
 void block_job_yield(BlockJob *job);
 
+/**
+ * block_job_relax:
+ * @job: The job that calls the function.
+ *
+ * Yield if it has been SLICE_TIME nanoseconds since the last yield.
+ * Otherwise, check if we need to pause, and yield if so.
+ */
+void block_job_relax(BlockJob *job);
+
 /**
  * block_job_pause_all:
  *
-- 
2.14.3

[Qemu-devel] [PATCH v2 04/13] blockjob: allow block_job_throttle to take delay_ns

[John Snow]

Instead of only sleeping for 0ms when we've hit a timeout, optionally
take a longer more explicit delay_ns that always forces the sleep.

Signed-off-by: John Snow 
---
 block/mirror.c   |  4 ++--
 blockjob.c   |  9 -
 include/block/blockjob_int.h | 10 +++---
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 36c681c7a1..3c73caed5e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -610,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int bytes = MIN(s->bdev_length - offset,
 QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-block_job_relax(>common);
+block_job_relax(>common, 0);
 
 if (block_job_is_cancelled(>common)) {
 s->initial_zeroing_ongoing = false;
@@ -638,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int bytes = MIN(s->bdev_length - offset,
 QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-block_job_relax(>common);
+block_job_relax(>common, 0);
 
 if (block_job_is_cancelled(>common)) {
 return 0;
diff --git a/blockjob.c b/blockjob.c
index 6f2e709b51..51c0eb5d9e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -906,12 +906,11 @@ void block_job_yield(BlockJob *job)
 block_job_pause_point(job);
 }
 
-void block_job_relax(BlockJob *job)
+void block_job_relax(BlockJob *job, int64_t delay_ns)
 {
-int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-if (now - job->last_enter_ns > SLICE_TIME) {
-block_job_sleep_ns(job, 0);
+if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
+ job->last_enter_ns > SLICE_TIME)) {
+block_job_sleep_ns(job, delay_ns);
 } else {
 block_job_pause_point(job);
 }
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 553784d86f..5f1520fab7 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -160,11 +160,15 @@ void block_job_yield(BlockJob *job);
 /**
  * block_job_relax:
  * @job: The job that calls the function.
+ * @delay_ns: The amount of time to sleep for
  *
- * Yield if it has been SLICE_TIME nanoseconds since the last yield.
- * Otherwise, check if we need to pause, and yield if so.
+ * Sleep for delay_ns nanoseconds.
+ *
+ * If delay_ns is 0, yield if it has been SLICE_TIME
+ * nanoseconds since the last yield. Otherwise, check
+ * if we need to yield for a pause event.
  */
-void block_job_relax(BlockJob *job);
+void block_job_relax(BlockJob *job, int64_t delay_ns);
 
 /**
  * block_job_pause_all:
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2] dump-guest-memory.py: fix python 2 support

[Eric Blake]

On 01/19/2018 10:25 AM, Marc-André Lureau wrote:
> Python GDB support may use Python 2 or 3.
> 
> Inferior.read_memory() may return a 'buffer' with Python 2 or a
> 'memoryview' with Python 3 (see also
> https://sourceware.org/gdb/onlinedocs/gdb/Inferiors-In-Python.html)
> 
> The elf.add_vmcoreinfo_note() method expects a "bytes" object. Wrap
> the returned memory with bytes(), which works with both 'memoryview'
> and 'buffer'.
> 
> Fixes a regression introduced with commit
> d23bfa91b7789534d16ede6cb7d925bfac3f3c4c ("add vmcoreinfo").
> 
> Suggested-by: Peter Maydell 
> Signed-off-by: Marc-André Lureau 
> ---
>  scripts/dump-guest-memory.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2] dump-guest-memory.py: fix python 2 support

[Laszlo Ersek]

On 01/19/18 20:48, Eric Blake wrote:
> On 01/19/2018 01:18 PM, Laszlo Ersek wrote:
>> On 01/19/18 17:25, Marc-André Lureau wrote:
>>> Python GDB support may use Python 2 or 3.
>>>
>>> Inferior.read_memory() may return a 'buffer' with Python 2 or a
>>> 'memoryview' with Python 3 (see also
>>> https://sourceware.org/gdb/onlinedocs/gdb/Inferiors-In-Python.html)
>>>
>>> The elf.add_vmcoreinfo_note() method expects a "bytes" object. Wrap
>>> the returned memory with bytes(), which works with both 'memoryview'
>>> and 'buffer'.
>>>
> 
> If I understand it, the issue stems from the fact that vmcoreinfo is a
> bytes buffer under python2, but a memoryview under python3.  Let's
> compare the patches:
> 
> The V1 patch:
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg04010.html
> 
>>
>>  if vmcoreinfo:
>> -self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
>> +# Python 2.7 returns a buffer
>> +vmciview = memoryview(vmcoreinfo)
>> +self.elf.add_vmcoreinfo_note(vmciview.tobytes())
> 
> did an unconditional memoryview(vmcoreinfo) followed by a .tobytes()
> call. Under python 2.7, vmcoreinfo is converted from a buffer to a
> memoryview, then back to bytes; under python 3, memoryview(vmcoreinfo)
> is a no-op (since it is already a memory view), then that is converted
> to bytes; the double conversion was necessary because bytes.tobytes()
> doesn't exist.  But python 2.6 doesn't have the memoryview conversion,
> so we've excluded older python users.
> 
> Now looking at the V2 patch:
> 
> 
>>> @@ -564,7 +564,7 @@ shape and this command should mostly work."""
>>>  
>>>  vmcoreinfo = self.phys_memory_read(addr, size)
>>>  if vmcoreinfo:
>>> -self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
>>> +self.elf.add_vmcoreinfo_note(bytes(vmcoreinfo))
> 
> Under python2, and bytes(buffer) is still bytes; under python3,
> vmcoreinfo is a memoryview but bytes(buffer) is equivalent to
> buffer.to_bytes().  We've avoided the intermediate conversion through
> memoryview, and thus work regardless of python version.

Heh, "semantics" :)

> 
>> Ugh, can you please point me to the v1->v2 difference?
> 
> Hope that helped (and hope I got it right, I'm learning as well from
> this thread).
> 

It sure helped a lot, thank you very much! In fact, you've done all the
review work, so technically, "R-b: Laszlo" would be misappropriation.
Can you give your R-b?

I'll manage an "I agree" statement:

Acked-by: Laszlo Ersek 

Thanks!
Laszlo

[Qemu-devel] virtio_net occasionally stops sending packets

[Brian Rak]

We've been running into a fairly persistent issue where virtio_net 
adapters will suddenly stop sending packets when running under KVM.  
This has persisted through several qemu versions, and a large number of 
guest kernel upgrades.


What we end up seeing is the guest continuing to receive packets, but 
refusing to transmit anything.


If we leave ping running for a minute or two, it will eventually start 
printing "ping: sendmsg: No buffer space available" messages.  tcpdump 
will show nothing being sent from the guest. `ip -s link` will show RX 
bytes/packets incrementing, but not TX. `tc -s qdisc show dev eth1` 
shows the 'dropped' counter incrementing with every new ping attempt.


I attempted to run 'dropwatch -l kas', which showed me a bunch of lines 
that looked like '4 drops at pfifo_fast_enqueue+85'.


So far, we haven't been able to consistently reproduce this.  We have a 
few guests that will hit this issue roughly once every two weeks, but we 
haven't been able to reproduce it on demand.  This seems to happen with 
guests that have more then one network adapter attached.  I do not think 
we've seen it on guests that only have one NIC.


We've tried guest kernels as new as 4.14.13, and qemu versions as new as 
2.11.0.  This doesn't appear to be related to the physical network at 
all, we've seen this happen with a variety of network backends:

* qemu 'multicast' networks
* macvtap attached to a vxlan interface
* bridged interface

We've tried disabling a variety of offloads (gso, tso4, tso6, ecn) from 
both the host and guest sides.  This didn't really have any effect.


The only way to fix this once it breaks is to restart the guest OS.  
`ifdown eth1; ifup eth1` doesn't seem to help.


How can I determine if this is a qemu issue, or an issue with the 
virtio_net driver?  We have not tried this with other virtual nic types 
yet.  I'm not sure if that would provide any useful information or not.  
We're still working on figuring out how to reproduce this, but I'm not 
terribly hopeful about coming up with a simple set of reproduction steps.


This was my post about it a few years ago: 
https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg03907.html

[Qemu-devel] [PULL 00/10] target/xtensa updates

[Max Filippov]

Hi Peter,

please pull the following batch of updates for the target/xtensa.

The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' 
into staging (2018-01-11 14:34:41 +)

are available in the git repository at:

  git://github.com/OSLL/qemu-xtensa.git tags/20180119-xtensa

for you to fetch changes up to 8030ed759be52b2517356c3d0291c45cedc65fd9:

  target/xtensa: disas/xtensa: fix coverity warnings (2018-01-18 10:27:04 -0800)


target/xtensa updates:

- make mini-bootloader independent of the initial CPU state;
- add noMMU XTFPGA variants;
- add two noMMU cores: de212 and sample_controller;
- fix issues reported by coverity against xtensa translator and disassembler.


Max Filippov (10):
  hw/xtensa/xtfpga: rewrite mini bootloader
  hw/xtensa/xtfpga: clean up function/structure names
  target/xtensa: fix default sysrom/sysram addresses
  hw/xtensa: extract xtensa_create_memory_regions
  hw/xtensa/xtfpga: extract flash configuration
  hw/xtensa/xtfpga: support noMMU cores
  target/xtensa: add de212 core
  target/xtensa: use different default CPU for MMU/noMMU
  target/xtensa: add sample_controller core
  target/xtensa: disas/xtensa: fix coverity warnings

 disas/xtensa.c | 4 +-
 hw/xtensa/Makefile.objs| 1 +
 hw/xtensa/sim.c|38 +-
 hw/xtensa/xtensa_memory.c  |55 +
 hw/xtensa/xtensa_memory.h  |40 +
 hw/xtensa/xtfpga.c |   414 +-
 target/xtensa/Makefile.objs| 2 +
 target/xtensa/core-de212.c |53 +
 target/xtensa/core-de212/core-isa.h|   622 +
 target/xtensa/core-de212/gdb-config.c  |   198 +
 target/xtensa/core-de212/xtensa-modules.c  | 14566 +++
 target/xtensa/core-sample_controller.c |53 +
 target/xtensa/core-sample_controller/core-isa.h|   644 +
 target/xtensa/core-sample_controller/gdb-config.c  |   141 +
 .../xtensa/core-sample_controller/xtensa-modules.c | 11377 +++
 target/xtensa/cpu.h| 7 +-
 target/xtensa/overlay_tool.h   | 8 +-
 target/xtensa/translate.c  | 4 +-
 18 files changed, 28081 insertions(+), 146 deletions(-)
 create mode 100644 hw/xtensa/xtensa_memory.c
 create mode 100644 hw/xtensa/xtensa_memory.h
 create mode 100644 target/xtensa/core-de212.c
 create mode 100644 target/xtensa/core-de212/core-isa.h
 create mode 100644 target/xtensa/core-de212/gdb-config.c
 create mode 100644 target/xtensa/core-de212/xtensa-modules.c
 create mode 100644 target/xtensa/core-sample_controller.c
 create mode 100644 target/xtensa/core-sample_controller/core-isa.h
 create mode 100644 target/xtensa/core-sample_controller/gdb-config.c
 create mode 100644 target/xtensa/core-sample_controller/xtensa-modules.c

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH v6 0/3] vhost: two fixes and used_memslots refactoring

[Michael S. Tsirkin]

On Tue, Jan 16, 2018 at 08:17:54AM +, Zhoujian (jay) wrote:
> > -Original Message-
> > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > Sent: Tuesday, January 16, 2018 12:42 PM
> > To: Zhoujian (jay) 
> > Cc: qemu-devel@nongnu.org; imamm...@redhat.com; Huangweidong (C)
> > ; wangxin (U) ; 
> > Gonglei
> > (Arei) ; Liuzhe (Ahriy, Euler) 
> > 
> > Subject: Re: [PATCH v6 0/3] vhost: two fixes and used_memslots refactoring
> > 
> > On Fri, Jan 12, 2018 at 10:47:56AM +0800, Jay Zhou wrote:
> > > Jay Zhou (3):
> > >   vhost: remove assertion to prevent crash
> > >   vhost: fix memslot limit check
> > >   vhost: used_memslots refactoring
> > 
> > This looks good to me, but needs to be rebased on top of vhost mem slot
> > management refactoring by dgilbert is merged. Pls post the rebase, and I'll
> > merge.
> 
> Hi Michael,
> I have tested but there're no conflicts, it is strange.
> 
> The patches I applied locally in sequence:
> 
> [PULL 21/33] vhost: Build temporary section list and deref after commit
> [PULL 22/33] vhost: Move log_dirty check
> [PULL 23/33] vhost: Simplify ring verification checks
> [PULL 24/33] vhost: Merge sections added to temporary list
> 
> And then,
> 
> [PATCH v6 1/3] vhost: remove assertion to prevent crash
> [PATCH v6 2/3] vhost: fix memslot limit check
> [PATCH v6 3/3] vhost: used_memslots refactoring
> 
> Could you point out which patches from Dave(or others) are conflict with mine?
> 
> Regards,
> Jay

So please simply rebase on top of master now.

Thanks!

> > 
> > Thanks!
> > 
> > 
> > 
> > >  hw/virtio/vhost-backend.c | 15 +++-
> > >  hw/virtio/vhost-user.c| 74 
> > > +++
> > 
> > >  hw/virtio/vhost.c | 30 +---
> > >  include/hw/virtio/vhost-backend.h |  6 ++--
> > >  4 files changed, 86 insertions(+), 39 deletions(-)
> > >
> > > --
> > > 1.8.3.1
> > >

Re: [Qemu-devel] [PATCH v2] dump-guest-memory.py: fix python 2 support

[Eric Blake]

On 01/19/2018 01:18 PM, Laszlo Ersek wrote:
> On 01/19/18 17:25, Marc-André Lureau wrote:
>> Python GDB support may use Python 2 or 3.
>>
>> Inferior.read_memory() may return a 'buffer' with Python 2 or a
>> 'memoryview' with Python 3 (see also
>> https://sourceware.org/gdb/onlinedocs/gdb/Inferiors-In-Python.html)
>>
>> The elf.add_vmcoreinfo_note() method expects a "bytes" object. Wrap
>> the returned memory with bytes(), which works with both 'memoryview'
>> and 'buffer'.
>>

If I understand it, the issue stems from the fact that vmcoreinfo is a
bytes buffer under python2, but a memoryview under python3.  Let's
compare the patches:

The V1 patch:
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg04010.html

>
>  if vmcoreinfo:
> -self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
> +# Python 2.7 returns a buffer
> +vmciview = memoryview(vmcoreinfo)
> +self.elf.add_vmcoreinfo_note(vmciview.tobytes())

did an unconditional memoryview(vmcoreinfo) followed by a .tobytes()
call. Under python 2.7, vmcoreinfo is converted from a buffer to a
memoryview, then back to bytes; under python 3, memoryview(vmcoreinfo)
is a no-op (since it is already a memory view), then that is converted
to bytes; the double conversion was necessary because bytes.tobytes()
doesn't exist.  But python 2.6 doesn't have the memoryview conversion,
so we've excluded older python users.

Now looking at the V2 patch:


>> @@ -564,7 +564,7 @@ shape and this command should mostly work."""
>>  
>>  vmcoreinfo = self.phys_memory_read(addr, size)
>>  if vmcoreinfo:
>> -self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
>> +self.elf.add_vmcoreinfo_note(bytes(vmcoreinfo))

Under python2, and bytes(buffer) is still bytes; under python3,
vmcoreinfo is a memoryview but bytes(buffer) is equivalent to
buffer.to_bytes().  We've avoided the intermediate conversion through
memoryview, and thus work regardless of python version.

> Ugh, can you please point me to the v1->v2 difference?

Hope that helped (and hope I got it right, I'm learning as well from
this thread).

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] target/arm: Fix 32-bit address truncation

[Ard Biesheuvel]

Commit ("3b39d734141a target/arm: Handle page table walk load failures
correctly") modified both versions of the page table walking code (i.e.,
arm_ldl_ptw and arm_ldq_ptw) to record the result of the translation in
a temporary 'data' variable so that it can be inspected before being
returned. However, arm_ldq_ptw() returns an uint64_t, and using a
temporary uint32_t variable truncates the upper bits, corrupting the
result. This causes problems when using more than 4 GB of memory in
a TCG guest. So use a uint64_t instead.

Signed-off-by: Ard Biesheuvel 
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 67059033019c..a41b6c3a1b82 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8368,7 +8368,7 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, 
bool is_secure,
 MemTxAttrs attrs = {};
 MemTxResult result = MEMTX_OK;
 AddressSpace *as;
-uint32_t data;
+uint64_t data;
 
 attrs.secure = is_secure;
 as = arm_addressspace(cs, attrs);
-- 
2.11.0

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 8/9] xlnx-zynqmp-pmu: Connect the IPI device to the PMU

[Philippe Mathieu-Daudé]

On 01/19/2018 03:15 PM, Alistair Francis wrote:
> On Thu, Jan 18, 2018 at 1:42 PM, Philippe Mathieu-Daudé  
> wrote:
>> On 01/18/2018 03:38 PM, Alistair Francis wrote:
>>> Signed-off-by: Alistair Francis 
>>> Reviewed-by: Edgar E. Iglesias 
>>> ---
>>> V4:
>>>  - Move the IPI to the machine instead of the SoC
>>>
>>>  hw/microblaze/xlnx-zynqmp-pmu.c | 31 +++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c 
>>> b/hw/microblaze/xlnx-zynqmp-pmu.c
>>> index 7312bfe23e..999a5657cf 100644
>>> --- a/hw/microblaze/xlnx-zynqmp-pmu.c
>>> +++ b/hw/microblaze/xlnx-zynqmp-pmu.c
>>> @@ -24,6 +24,7 @@
>>>  #include "cpu.h"
>>>  #include "boot.h"
>>>
>>> +#include "hw/intc/xlnx-zynqmp-ipi.h"
>>>  #include "hw/intc/xlnx-pmu-iomod-intc.h"
>>>
>>>  /* Define the PMU device */
>>> @@ -38,6 +39,15 @@
>>>
>>>  #define XLNX_ZYNQMP_PMU_INTC_ADDR   0xFFD4
>>>
>>> +#define XLNX_ZYNQMP_PMU_NUM_IPIS4
>>> +
>>> +static const uint64_t ipi_addr[XLNX_ZYNQMP_PMU_NUM_IPIS] = {
>>> +0xFF34, 0xFF35, 0xFF36, 0xFF37,
>>> +};
>>> +static const uint64_t ipi_irq[XLNX_ZYNQMP_PMU_NUM_IPIS] = {
>>> +19, 20, 21, 22,
>>> +};
>>> +
>>>  typedef struct XlnxZynqMPPMUSoCState {
>>>  /*< private >*/
>>>  DeviceState parent_obj;
>>> @@ -136,6 +146,9 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine)
>>>  MemoryRegion *address_space_mem = get_system_memory();
>>>  MemoryRegion *pmu_rom = g_new(MemoryRegion, 1);
>>>  MemoryRegion *pmu_ram = g_new(MemoryRegion, 1);
>>> +XlnxZynqMPIPI *ipi[XLNX_ZYNQMP_PMU_NUM_IPIS];
>>
>> Why use pointers and g_new() here?
>>
>> Isn't a plain array simpler?
> 
> I remember that I tried that and ran into issues. It was so long ago
> though that I can't remember what the problem was. I think it was
> related to adding the object as a child.

It makes sens for the SoC container where it is indeed necessary, but
your v4 moved this into the machine.
Not a blocking issue although.

> 
> Alistair
> 
>>
>>XlnxZynqMPIPI ipi[XLNX_ZYNQMP_PMU_NUM_IPIS];
>>
>> (same apply to pmu_rom/ram)
>>
>> Or maybe do you plan to start implementing MachineClass::exit()?
>>
>> (or go in this direction, which might make sens thinking about having a
>> multiarch single binary).
>>
>>> +qemu_irq irq[32];
>>> +int i;
>>>
>>>  /* Create the ROM */
>>>  memory_region_init_rom(pmu_rom, NULL, "xlnx-zynqmp-pmu.rom",
>>> @@ -155,6 +168,24 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine)
>>>_abort);
>>>  object_property_set_bool(OBJECT(pmu), true, "realized", _fatal);
>>>
>>> +for (i = 0; i < 32; i++) {
>>> +irq[i] = qdev_get_gpio_in(DEVICE(>intc), i);
>>> +}
>>> +
>>> +/* Create and connect the IPI device */
>>> +for (i = 0; i < XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
>>> +ipi[i] = g_new0(XlnxZynqMPIPI, 1);
>>> +object_initialize(ipi[i], sizeof(XlnxZynqMPIPI), 
>>> TYPE_XLNX_ZYNQMP_IPI);
>>> +qdev_set_parent_bus(DEVICE(ipi[i]), sysbus_get_default());
>>> +}
>>> +
>>> +for (i = 0; i < XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
>>> +object_property_set_bool(OBJECT(ipi[i]), true, "realized",
>>> + _abort);
>>> +sysbus_mmio_map(SYS_BUS_DEVICE(ipi[i]), 0, ipi_addr[i]);
>>> +sysbus_connect_irq(SYS_BUS_DEVICE(ipi[i]), 0, irq[ipi_irq[i]]);
>>> +}
>>> +
>>>  /* Load the kernel */
>>>  microblaze_load_kernel(>cpu, XLNX_ZYNQMP_PMU_RAM_ADDR,
>>> machine->ram_size,
>>>
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 5/9] xlnx-pmu-iomod-intc: Add the PMU Interrupt controller

[Philippe Mathieu-Daudé]

On 01/19/2018 03:11 PM, Alistair Francis wrote:
> On Thu, Jan 18, 2018 at 1:29 PM, Philippe Mathieu-Daudé  
> wrote:
>> On 01/18/2018 03:38 PM, Alistair Francis wrote:
>>> Add the PMU IO Module Interrupt controller device.
>>>
>>> Signed-off-by: Alistair Francis 
>>> Reviewed-by: Edgar E. Iglesias 
>>> ---
>>>
>>>  default-configs/microblaze-softmmu.mak |   1 +
>>>  include/hw/intc/xlnx-pmu-iomod-intc.h  |  58 
>>>  hw/intc/xlnx-pmu-iomod-intc.c  | 554 
>>> +
>>
>> Thanks for installing the git.orderfile :)
>>
>>>  hw/intc/Makefile.objs  |   1 +
>>>  4 files changed, 614 insertions(+)
>>>  create mode 100644 include/hw/intc/xlnx-pmu-iomod-intc.h
>>>  create mode 100644 hw/intc/xlnx-pmu-iomod-intc.c
>>>
>>> diff --git a/default-configs/microblaze-softmmu.mak 
>>> b/default-configs/microblaze-softmmu.mak
>>> index ce2630818a..7fca8e4c99 100644
>>> --- a/default-configs/microblaze-softmmu.mak
>>> +++ b/default-configs/microblaze-softmmu.mak
>>> @@ -9,3 +9,4 @@ CONFIG_XILINX_SPI=y
>>>  CONFIG_XILINX_ETHLITE=y
>>>  CONFIG_SSI=y
>>>  CONFIG_SSI_M25P80=y
>>> +CONFIG_XLNX_ZYNQMP=y
>>> diff --git a/include/hw/intc/xlnx-pmu-iomod-intc.h 
>>> b/include/hw/intc/xlnx-pmu-iomod-intc.h
>>> new file mode 100644
>>> index 00..3478d8cf6c
>>> --- /dev/null
>>> +++ b/include/hw/intc/xlnx-pmu-iomod-intc.h
>>> @@ -0,0 +1,58 @@
>>> +/*
>>> + * QEMU model of Xilinx I/O Module Interrupt Controller
>>> + *
>>> + * Copyright (c) 2014 Xilinx Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>>> copy
>>> + * of this software and associated documentation files (the "Software"), 
>>> to deal
>>> + * in the Software without restriction, including without limitation the 
>>> rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
>>> sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included 
>>> in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>>> OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>>> OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>>> FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
>>> IN
>>> + * THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef XLNX_PMU_IO_INTC_H
>>> +#define XLNX_PMU_IO_INTC_H
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/sysbus.h"
>>> +#include "hw/register.h"
>>> +
>>> +#define TYPE_XLNX_PMU_IO_INTC "xlnx.pmu_io_intc"
>>> +
>>> +#define XLNX_PMU_IO_INTC(obj) \
>>> + OBJECT_CHECK(XlnxPMUIOIntc, (obj), TYPE_XLNX_PMU_IO_INTC)
>>> +
>>> +/* This is R_PIT3_CONTROL + 1 */
>>> +#define XlnxPMUIOIntc_R_MAX (0x78 + 1)
>>
>> Good, I prefer this way, rather than having all the registers exposed in
>> the header.
>> Can you rename this with CAPS to stay consistent with the CODING_STYLE?
>>
>>> +
>>> +typedef struct XlnxPMUIOIntc {
>>> +SysBusDevice parent_obj;
>>> +MemoryRegion iomem;
>>> +
>>> +qemu_irq parent_irq;
>>> +
>>> +struct {
>>> +uint32_t intr_size;
>>> +uint32_t level_edge;
>>> +uint32_t positive;
>>> +} cfg;
>>> +
>>> +uint32_t irq_raw;
>>> +
>>> +uint32_t regs[XlnxPMUIOIntc_R_MAX];
>>> +RegisterInfo regs_info[XlnxPMUIOIntc_R_MAX];
>>> +} XlnxPMUIOIntc;
>>> +
>>> +#endif /* XLNX_PMU_IO_INTC_H */
>>> diff --git a/hw/intc/xlnx-pmu-iomod-intc.c b/hw/intc/xlnx-pmu-iomod-intc.c
>>> new file mode 100644
>>> index 00..1ce2f4fa6b
>>> --- /dev/null
>>> +++ b/hw/intc/xlnx-pmu-iomod-intc.c
>>> @@ -0,0 +1,554 @@
>>> +/*
>>> + * QEMU model of Xilinx I/O Module Interrupt Controller
>>> + *
>>> + * Copyright (c) 2013 Xilinx Inc
>>> + * Written by Edgar E. Iglesias 
>>> + * Written by Alistair Francis 
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>>> copy
>>> + * of this software and associated documentation files (the "Software"), 
>>> to deal
>>> + * in the Software without restriction, including without limitation the 
>>> rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
>>> sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included 
>>> in
>>> + * all copies or substantial portions of the 

Re: [Qemu-devel] [PATCH v4 1/3] xlnx-zynqmp-rtc: Initial commit

[Philippe Mathieu-Daudé]

Hi Alistair,

On 01/19/2018 03:35 PM, Alistair Francis wrote:
> Initial commit of the ZynqMP RTC device.
> 
> Signed-off-by: Alistair Francis 
> ---
> V2:
>  - Delete unused realise function
>  - Remove DB_PRINT()
> 
>  include/hw/timer/xlnx-zynqmp-rtc.h |  84 ++
>  hw/timer/xlnx-zynqmp-rtc.c | 218 
> +
>  hw/timer/Makefile.objs |   1 +
>  3 files changed, 303 insertions(+)
>  create mode 100644 include/hw/timer/xlnx-zynqmp-rtc.h
>  create mode 100644 hw/timer/xlnx-zynqmp-rtc.c
> 
> diff --git a/include/hw/timer/xlnx-zynqmp-rtc.h 
> b/include/hw/timer/xlnx-zynqmp-rtc.h
> new file mode 100644
> index 00..87649836cc
> --- /dev/null
> +++ b/include/hw/timer/xlnx-zynqmp-rtc.h
> @@ -0,0 +1,84 @@
> +/*
> + * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
> + *
> + * Copyright (c) 2017 Xilinx Inc.
> + *
> + * Written-by: Alistair Francis 
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/register.h"
> +
> +#define TYPE_XLNX_ZYNQMP_RTC "xlnx-zynmp.rtc"
> +
> +#define XLNX_ZYNQMP_RTC(obj) \
> + OBJECT_CHECK(XlnxZynqMPRTC, (obj), TYPE_XLNX_ZYNQMP_RTC)
> +
> +REG32(SET_TIME_WRITE, 0x0)
> +REG32(SET_TIME_READ, 0x4)
> +REG32(CALIB_WRITE, 0x8)
> +FIELD(CALIB_WRITE, FRACTION_EN, 20, 1)
> +FIELD(CALIB_WRITE, FRACTION_DATA, 16, 4)
> +FIELD(CALIB_WRITE, MAX_TICK, 0, 16)
> +REG32(CALIB_READ, 0xc)
> +FIELD(CALIB_READ, FRACTION_EN, 20, 1)
> +FIELD(CALIB_READ, FRACTION_DATA, 16, 4)
> +FIELD(CALIB_READ, MAX_TICK, 0, 16)
> +REG32(CURRENT_TIME, 0x10)
> +REG32(CURRENT_TICK, 0x14)
> +FIELD(CURRENT_TICK, VALUE, 0, 16)
> +REG32(ALARM, 0x18)
> +REG32(RTC_INT_STATUS, 0x20)
> +FIELD(RTC_INT_STATUS, ALARM, 1, 1)
> +FIELD(RTC_INT_STATUS, SECONDS, 0, 1)
> +REG32(RTC_INT_MASK, 0x24)
> +FIELD(RTC_INT_MASK, ALARM, 1, 1)
> +FIELD(RTC_INT_MASK, SECONDS, 0, 1)
> +REG32(RTC_INT_EN, 0x28)
> +FIELD(RTC_INT_EN, ALARM, 1, 1)
> +FIELD(RTC_INT_EN, SECONDS, 0, 1)
> +REG32(RTC_INT_DIS, 0x2c)
> +FIELD(RTC_INT_DIS, ALARM, 1, 1)
> +FIELD(RTC_INT_DIS, SECONDS, 0, 1)
> +REG32(ADDR_ERROR, 0x30)
> +FIELD(ADDR_ERROR, STATUS, 0, 1)
> +REG32(ADDR_ERROR_INT_MASK, 0x34)
> +FIELD(ADDR_ERROR_INT_MASK, MASK, 0, 1)
> +REG32(ADDR_ERROR_INT_EN, 0x38)
> +FIELD(ADDR_ERROR_INT_EN, MASK, 0, 1)
> +REG32(ADDR_ERROR_INT_DIS, 0x3c)
> +FIELD(ADDR_ERROR_INT_DIS, MASK, 0, 1)
> +REG32(CONTROL, 0x40)
> +FIELD(CONTROL, BATTERY_DISABLE, 31, 1)
> +FIELD(CONTROL, OSC_CNTRL, 24, 4)
> +FIELD(CONTROL, SLVERR_ENABLE, 0, 1)
> +REG32(SAFETY_CHK, 0x50)
> +
> +#define XLNX_ZYNQMP_RTC_R_MAX (R_SAFETY_CHK + 1)
> +
> +typedef struct XlnxZynqMPRTC {
> +SysBusDevice parent_obj;
> +MemoryRegion iomem;
> +qemu_irq irq_rtc_int;
> +qemu_irq irq_addr_error_int;
> +
> +uint32_t regs[XLNX_ZYNQMP_RTC_R_MAX];
> +RegisterInfo regs_info[XLNX_ZYNQMP_RTC_R_MAX];
> +} XlnxZynqMPRTC;
> diff --git a/hw/timer/xlnx-zynqmp-rtc.c b/hw/timer/xlnx-zynqmp-rtc.c
> new file mode 100644
> index 00..ead40fc42d
> --- /dev/null
> +++ b/hw/timer/xlnx-zynqmp-rtc.c
> @@ -0,0 +1,218 @@
> +/*
> + * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
> + *
> + * Copyright (c) 2017 Xilinx Inc.
> + *
> + * Written-by: Alistair Francis 
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice 

Re: [Qemu-devel] [PATCH v2] dump-guest-memory.py: fix python 2 support

[Laszlo Ersek]

On 01/19/18 17:25, Marc-André Lureau wrote:
> Python GDB support may use Python 2 or 3.
> 
> Inferior.read_memory() may return a 'buffer' with Python 2 or a
> 'memoryview' with Python 3 (see also
> https://sourceware.org/gdb/onlinedocs/gdb/Inferiors-In-Python.html)
> 
> The elf.add_vmcoreinfo_note() method expects a "bytes" object. Wrap
> the returned memory with bytes(), which works with both 'memoryview'
> and 'buffer'.
> 
> Fixes a regression introduced with commit
> d23bfa91b7789534d16ede6cb7d925bfac3f3c4c ("add vmcoreinfo").
> 
> Suggested-by: Peter Maydell 
> Signed-off-by: Marc-André Lureau 
> ---
>  scripts/dump-guest-memory.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index 09bec92b50..03fbf69f8a 100644
> --- a/scripts/dump-guest-memory.py
> +++ b/scripts/dump-guest-memory.py
> @@ -564,7 +564,7 @@ shape and this command should mostly work."""
>  
>  vmcoreinfo = self.phys_memory_read(addr, size)
>  if vmcoreinfo:
> -self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
> +self.elf.add_vmcoreinfo_note(bytes(vmcoreinfo))
>  
>  def invoke(self, args, from_tty):
>  """Handles command invocation from gdb."""
> 

Ugh, can you please point me to the v1->v2 difference?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 06/11] sysbus: add realize() and unrealize()

[Philippe Mathieu-Daudé]

On 01/19/2018 03:03 PM, Eduardo Habkost wrote:
> On Tue, Jan 16, 2018 at 10:15:50AM -0300, Philippe Mathieu-Daudé wrote:
> [...]
>> +static void sysbus_realize(DeviceState *dev, Error **errp)
>> +{
>> +SysBusDevice *sd = SYS_BUS_DEVICE(dev);
>> +SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
>> +
>> +if (sbc->realize) {
>> +sbc->realize(sd, errp);
>> +}
>> +}
>> +
>> +static void sysbus_unrealize(DeviceState *dev, Error **errp)
>> +{
>> +SysBusDevice *sd = SYS_BUS_DEVICE(dev);
>> +SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
>> +
>> +if (sbc->unrealize) {
>> +sbc->unrealize(sd, errp);
>> +}
>> +}
> 
> Why not just let the subclasses set DeviceClass::realize and
> DeviceClass::unrealize directly?

yes, clever :)

> 
>> +
>>  DeviceState *sysbus_create_varargs(const char *name,
>> hwaddr addr, ...)
>>  {
>> @@ -325,6 +346,8 @@ static void sysbus_device_class_init(ObjectClass *klass, 
>> void *data)
>>  {
>>  DeviceClass *k = DEVICE_CLASS(klass);
>>  k->init = sysbus_device_init;
>> +k->realize = sysbus_realize;
>> +k->unrealize = sysbus_unrealize;
>>  k->bus_type = TYPE_SYSTEM_BUS;
>>  /*
>>   * device_add plugs devices into a suitable bus.  For "real" buses,
>> -- 
>> 2.15.1
>>
>>
> 

Re: [Qemu-devel] [PATCH 03/11] hw/i2c: convert I2CSlaveClass::init -> realize

[Philippe Mathieu-Daudé]

On 01/19/2018 03:18 PM, Eduardo Habkost wrote:
> On Tue, Jan 16, 2018 at 10:15:47AM -0300, Philippe Mathieu-Daudé wrote:
> [...]
>> -static int i2c_slave_qdev_init(DeviceState *dev)
>> +static void i2c_slave_realize(DeviceState *dev, Error **errp)
>>  {
>>  I2CSlave *s = I2C_SLAVE(dev);
>>  I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
>>  
>> -if (sc->init) {
>> -return sc->init(s);
>> +if (sc->realize) {
>> +return sc->realize(s, errp);
>>  }
> 
> Do you think i2c_slave_realize() will perform additional actions
> in the future?  If not, why not let subclasses set
> DeviceClass::realize directly?

Good idea, also with SMBusDeviceClass::realize.

>> -
>> -return 0;
>>  }
>>  
>>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
>> @@ -301,7 +299,7 @@ DeviceState *i2c_create_slave(I2CBus *bus, const char 
>> *name, uint8_t addr)
>>  static void i2c_slave_class_init(ObjectClass *klass, void *data)
>>  {
>>  DeviceClass *k = DEVICE_CLASS(klass);
>> -k->init = i2c_slave_qdev_init;
>> +k->realize = i2c_slave_realize;
>>  set_bit(DEVICE_CATEGORY_MISC, k->categories);
>>  k->bus_type = TYPE_I2C_BUS;
>>  k->props = i2c_props;
> [...]
> 

Re: [Qemu-devel] [RFC] qid path collision issues in 9pfs

[Eduard Shishkin]

On 1/19/2018 7:05 PM, Greg Kurz wrote:

On Fri, 19 Jan 2018 17:37:58 +0100
Veaceslav Falico  wrote:


On 1/19/2018 4:52 PM, Eduard Shishkin wrote:



On 1/19/2018 11:27 AM, Greg Kurz wrote:

On Mon, 15 Jan 2018 11:49:31 +0800
Antonios Motakis  wrote:


On 13-Jan-18 00:14, Greg Kurz wrote:

On Fri, 12 Jan 2018 19:32:10 +0800
Antonios Motakis  wrote:


Hello all,



Hi Antonios,

I see you have attached a patch to this email... this really isn't the preferred
way to do things since it prevents to comment the patch (at least with my mail
client). The appropriate way would have been to send the patch with a cover
letter, using git-send-email for example.


I apologize for attaching the patch, I should have known better!



np :)




We have found an issue in the 9p implementation of QEMU, with how qid paths are 
generated, which can cause qid path collisions and several issues caused by 
them. In our use case (running containers under VMs) these have proven to be 
critical.



Ouch...


In particular, stat_to_qid in hw/9pfs/9p.c generates a qid path using the inode 
number of the file as input. According to the 9p spec the path should be able 
to uniquely identify a file, distinct files should not share a path value.

The current implementation that defines qid.path = inode nr works fine as long 
as there are not files from multiple partitions visible under the 9p share. In 
that case, distinct files from different devices are allowed to have the same 
inode number. So with multiple partitions, we have a very high probability of 
qid path collisions.

How to demonstrate the issue:
1) Prepare a problematic share:
 - mount one partition under share/p1/ with some files inside
 - mount another one *with identical contents* under share/p2/
 - confirm that both partitions have files with same inode nr, size, etc
2) Demonstrate breakage:
 - start a VM with a virtio-9p pointing to the share
 - mount 9p share with FSCACHE on
 - keep open share/p1/file
 - open and write to share/p2/file

What should happen is, the guest will consider share/p1/file and share/p2/file 
to be the same file, and since we are using the cache it will not reopen it. We 
intended to write to partition 2, but we just wrote to partition 1. This is 
just one example on how the guest may rely on qid paths being unique.

In the use case of containers where we commonly have a few containers per VM, 
all based on similar images, these kind of qid path collisions are very common 
and they seem to cause all kinds of funny behavior (sometimes very subtle).

To avoid this situation, the device id of a file needs to be also taken as 
input for generating a qid path. Unfortunately, the size of both inode nr + 
device id together would be 96 bits, while we have only 64 bits for the qid 
path, so we can't just append them and call it a day :(

We have thought of a few approaches, but we would definitely like to hear what 
the upstream maintainers and community think:

* Full fix: Change the 9p protocol

We would need to support a longer qid path, based on a virtio feature flag. 
This would take reworking of host and guest parts of virtio-9p, so both QEMU 
and Linux for most users.



I agree for a longer qid path, but we shouldn't tie it to a virtio flag since
9p is transport agnostic. And it happens to be used with a variety of 
transports.
QEMU has both virtio-9p and a Xen backend for example.


* Fallback and/or interim solutions

A virtio feature flag may be refused by the guest, so we think we still need to 
make collisions less likely even with 64 bit paths. E.g.


In all cases, we would need a fallback solution to support current
guest setups. Also there are several 9p server implementations out
there (ganesha, diod, kvmtool) that are currently used with linux
clients... it will take some time to get everyone in sync :-\


1. XOR the device id with inode nr to produce the qid path (we attach a proof 
of concept patch)


Hmm... this would still allow collisions. Not good for fallback.


2. 64 bit hash of device id and inode nr


Same here.


3. other ideas, such as allocating new qid paths and keep a look up table of 
some sorts (possibly too expensive)



This would be acceptable for fallback.


Maybe we can use the QEMU hash table 
(https://github.com/qemu/qemu/blob/master/util/qht.c) but I wonder if it scales 
to millions of entries. Do you think it is appropriate in this case?



I don't know QHT, hence Cc'ing Emilio for insights.


I was thinking on how to implement something like this, without having to 
maintain millions of entries... One option we could consider is to split the 
bits into a directly-mapped part, and a lookup part. For example:

Inode =
[ 10 first bits ] + [ 54 lowest bits ]

A hash table maps [ inode 10 first bits ] + [ device id ] => [ 10 bit prefix ]
The prefix is uniquely allocated for each input.

Qid path =
[ 10 bit prefix ] + [ inode 

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Eduardo Habkost]

On Fri, Jan 19, 2018 at 12:10:03PM -0500, Stefan Berger wrote:
> On 01/19/2018 09:11 AM, Marc-André Lureau wrote:
> > tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
> > Interface as defined in TCG PC Client Platform TPM Profile (PTP)
> > Specification Family “2.0” Level 00 Revision 01.03 v22.
> > 
> > The PTP allows device implementation to switch between TIS and CRB
> > model at run time, but given that CRB is a simpler device to
> > implement, I chose to implement it as a different device.
> > 
> > The device doesn't implement other locality than 0 for now (my laptop
> > TPM doesn't either, so I assume this isn't so bad)
> > 
> > The command/reply memory region is statically allocated after the CRB
> > registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
> > wonder if the BIOS could or should allocate it instead, or what size
> > to use, again this seems to fit well expectations)
> 
> I removed this last sentence now. It's at the right location.
> 
> > 
> > The PTP doesn't specify a particular bus to put the device. So I added
> > it on the system bus directly, so it could hopefully be used easily on
> > a different platform than x86. Currently, it fails to init on piix,
> > because error_on_sysbus_device() check. The check may be changed in a
> > near future, see discussion on the qemu-devel ML.
> 
> I think this has to be solved. So I remove these last 2 sentences. I'll have
> to wait until that other patch series from Eduard is merged since it doesn't
> start yet.

The series was just merged to master.  It's possible to make a
machine accept the new device using
machine_class_allow_dynamic_sysbus_dev(), now.

However, is it really necessary to make it a sysbus device?
Having bus-less devices was not possible in the past, but it is
possible today.

-- 
Eduardo

[Qemu-devel] [PATCH v4 3/3] xlnx-zynqmp: Connect the RTC device

[Alistair Francis]

Signed-off-by: Alistair Francis 
Reviewed-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
---

 include/hw/arm/xlnx-zynqmp.h |  2 ++
 hw/arm/xlnx-zynqmp.c | 14 ++
 2 files changed, 16 insertions(+)

diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 3e6fb9b7bd..9e8c9e18dd 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -28,6 +28,7 @@
 #include "hw/ssi/xilinx_spips.h"
 #include "hw/dma/xlnx_dpdma.h"
 #include "hw/display/xlnx_dp.h"
+#include "hw/timer/xlnx-zynqmp-rtc.h"
 
 #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
 #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
@@ -90,6 +91,7 @@ typedef struct XlnxZynqMPState {
 XlnxZynqMPQSPIPS qspi;
 XlnxDPState dp;
 XlnxDPDMAState dpdma;
+XlnxZynqMPRTC rtc;
 
 char *boot_cpu;
 ARMCPU *boot_cpu_ptr;
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 325642058b..deef583c2a 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -50,6 +50,9 @@
 #define DPDMA_ADDR  0xfd4c
 #define DPDMA_IRQ   116
 
+#define RTC_ADDR0xffa6
+#define RTC_IRQ 26
+
 static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
 0xFF0B, 0xFF0C, 0xFF0D, 0xFF0E,
 };
@@ -183,6 +186,9 @@ static void xlnx_zynqmp_init(Object *obj)
 
 object_initialize(>dpdma, sizeof(s->dpdma), TYPE_XLNX_DPDMA);
 qdev_set_parent_bus(DEVICE(>dpdma), sysbus_get_default());
+
+object_initialize(>rtc, sizeof(s->rtc), TYPE_XLNX_ZYNQMP_RTC);
+qdev_set_parent_bus(DEVICE(>rtc), sysbus_get_default());
 }
 
 static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
@@ -454,6 +460,14 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
**errp)
  _abort);
 sysbus_mmio_map(SYS_BUS_DEVICE(>dpdma), 0, DPDMA_ADDR);
 sysbus_connect_irq(SYS_BUS_DEVICE(>dpdma), 0, gic_spi[DPDMA_IRQ]);
+
+object_property_set_bool(OBJECT(>rtc), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>rtc), 0, RTC_ADDR);
+sysbus_connect_irq(SYS_BUS_DEVICE(>rtc), 0, gic_spi[RTC_IRQ]);
 }
 
 static Property xlnx_zynqmp_props[] = {
-- 
2.14.1

[Qemu-devel] [PATCH v4 1/3] xlnx-zynqmp-rtc: Initial commit

[Alistair Francis]

Initial commit of the ZynqMP RTC device.

Signed-off-by: Alistair Francis 
---
V2:
 - Delete unused realise function
 - Remove DB_PRINT()

 include/hw/timer/xlnx-zynqmp-rtc.h |  84 ++
 hw/timer/xlnx-zynqmp-rtc.c | 218 +
 hw/timer/Makefile.objs |   1 +
 3 files changed, 303 insertions(+)
 create mode 100644 include/hw/timer/xlnx-zynqmp-rtc.h
 create mode 100644 hw/timer/xlnx-zynqmp-rtc.c

diff --git a/include/hw/timer/xlnx-zynqmp-rtc.h 
b/include/hw/timer/xlnx-zynqmp-rtc.h
new file mode 100644
index 00..87649836cc
--- /dev/null
+++ b/include/hw/timer/xlnx-zynqmp-rtc.h
@@ -0,0 +1,84 @@
+/*
+ * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
+ *
+ * Copyright (c) 2017 Xilinx Inc.
+ *
+ * Written-by: Alistair Francis 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/register.h"
+
+#define TYPE_XLNX_ZYNQMP_RTC "xlnx-zynmp.rtc"
+
+#define XLNX_ZYNQMP_RTC(obj) \
+ OBJECT_CHECK(XlnxZynqMPRTC, (obj), TYPE_XLNX_ZYNQMP_RTC)
+
+REG32(SET_TIME_WRITE, 0x0)
+REG32(SET_TIME_READ, 0x4)
+REG32(CALIB_WRITE, 0x8)
+FIELD(CALIB_WRITE, FRACTION_EN, 20, 1)
+FIELD(CALIB_WRITE, FRACTION_DATA, 16, 4)
+FIELD(CALIB_WRITE, MAX_TICK, 0, 16)
+REG32(CALIB_READ, 0xc)
+FIELD(CALIB_READ, FRACTION_EN, 20, 1)
+FIELD(CALIB_READ, FRACTION_DATA, 16, 4)
+FIELD(CALIB_READ, MAX_TICK, 0, 16)
+REG32(CURRENT_TIME, 0x10)
+REG32(CURRENT_TICK, 0x14)
+FIELD(CURRENT_TICK, VALUE, 0, 16)
+REG32(ALARM, 0x18)
+REG32(RTC_INT_STATUS, 0x20)
+FIELD(RTC_INT_STATUS, ALARM, 1, 1)
+FIELD(RTC_INT_STATUS, SECONDS, 0, 1)
+REG32(RTC_INT_MASK, 0x24)
+FIELD(RTC_INT_MASK, ALARM, 1, 1)
+FIELD(RTC_INT_MASK, SECONDS, 0, 1)
+REG32(RTC_INT_EN, 0x28)
+FIELD(RTC_INT_EN, ALARM, 1, 1)
+FIELD(RTC_INT_EN, SECONDS, 0, 1)
+REG32(RTC_INT_DIS, 0x2c)
+FIELD(RTC_INT_DIS, ALARM, 1, 1)
+FIELD(RTC_INT_DIS, SECONDS, 0, 1)
+REG32(ADDR_ERROR, 0x30)
+FIELD(ADDR_ERROR, STATUS, 0, 1)
+REG32(ADDR_ERROR_INT_MASK, 0x34)
+FIELD(ADDR_ERROR_INT_MASK, MASK, 0, 1)
+REG32(ADDR_ERROR_INT_EN, 0x38)
+FIELD(ADDR_ERROR_INT_EN, MASK, 0, 1)
+REG32(ADDR_ERROR_INT_DIS, 0x3c)
+FIELD(ADDR_ERROR_INT_DIS, MASK, 0, 1)
+REG32(CONTROL, 0x40)
+FIELD(CONTROL, BATTERY_DISABLE, 31, 1)
+FIELD(CONTROL, OSC_CNTRL, 24, 4)
+FIELD(CONTROL, SLVERR_ENABLE, 0, 1)
+REG32(SAFETY_CHK, 0x50)
+
+#define XLNX_ZYNQMP_RTC_R_MAX (R_SAFETY_CHK + 1)
+
+typedef struct XlnxZynqMPRTC {
+SysBusDevice parent_obj;
+MemoryRegion iomem;
+qemu_irq irq_rtc_int;
+qemu_irq irq_addr_error_int;
+
+uint32_t regs[XLNX_ZYNQMP_RTC_R_MAX];
+RegisterInfo regs_info[XLNX_ZYNQMP_RTC_R_MAX];
+} XlnxZynqMPRTC;
diff --git a/hw/timer/xlnx-zynqmp-rtc.c b/hw/timer/xlnx-zynqmp-rtc.c
new file mode 100644
index 00..ead40fc42d
--- /dev/null
+++ b/hw/timer/xlnx-zynqmp-rtc.c
@@ -0,0 +1,218 @@
+/*
+ * QEMU model of the Xilinx ZynqMP Real Time Clock (RTC).
+ *
+ * Copyright (c) 2017 Xilinx Inc.
+ *
+ * Written-by: Alistair Francis 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT 

[Qemu-devel] [PATCH v4 0/3] Add and connect the ZynqMP RTC

[Alistair Francis]

V4:
 - Use the RegisterAccessInfo .unimp functionality

V3:
 - Store an offset value
 - Use mktimegm()
 - Log unimplemented writes

V2:
 - Delete unused realise function
 - Add cover letter
 - Convert DB_PRINT() macro to trace


Alistair Francis (3):
  xlnx-zynqmp-rtc: Initial commit
  xlnx-zynqmp-rtc: Add basic time support
  xlnx-zynqmp: Connect the RTC device

 include/hw/arm/xlnx-zynqmp.h   |   2 +
 include/hw/timer/xlnx-zynqmp-rtc.h |  88 
 hw/arm/xlnx-zynqmp.c   |  14 ++
 hw/timer/xlnx-zynqmp-rtc.c | 276 +
 hw/timer/Makefile.objs |   1 +
 hw/timer/trace-events  |   3 +
 6 files changed, 384 insertions(+)
 create mode 100644 include/hw/timer/xlnx-zynqmp-rtc.h
 create mode 100644 hw/timer/xlnx-zynqmp-rtc.c

-- 
2.14.1

Re: [Qemu-devel] [PATCH QEMU v1 0/4] multiboot: bss_end_addr can be zero / cleanup

[Anatol Pomozov]

Hello Jack

On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz
 wrote:
> Hi Kevin and Anatol.
>
> Kevin, thanks for your review.
>
> More inline below...
>
> On 01/15/18 07:54, Kevin Wolf wrote:
>>
>> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben:
>>>
>>> Properly account for the possibility of multiboot kernels with a zero
>>> bss_end_addr.  The Multiboot Specification, section 3.1.3 allows for
>>> kernels without a bss section, by allowing a zeroed bss_end_addr
>>> multiboot
>>> header field.
>>>
>>> Do some cleanup to multiboot.c as well:
>>> - Remove some unused variables.
>>> - Use more intuitive header names when displaying fields in messages.
>>> - Change fprintf(stderr...) to error_report
>>
>> There are some conflicts with Anatol's (CCed) multiboot series:
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html
>>
>> None if these should be hard to resolve, but it would be good if you
>> could agree with each other whose patch series should come first, and
>> then the other one should be rebased on top of that.
>
> Anatol,
>
> from my side, there are pros and cons to either patch set going in first,
> but advantages to either are pretty negligible.  Pro for you going first: I
> can use the constants you will define in header files.  Pro for me going
> first: your merge should be about the same as if you went first (since my
> changes are small, more localized and affect only multiboot.c) and my merge
> will be easier.
>
> What are your thoughts?

Please move ahead with your patches. I'll rebase my changes on top of yours.

>>
>> Testing:
>>1) Ran the "make check" test suite.
>>2) Booted multiboot kernel with bss_end_addr=0.  (I rolled my own
>>   grub multiboot.elf test "kernel" by modifying source.)  Verified
>>   with gdb that new code that reads addresses/offsets from multiboot
>>   header was accessed.
>>3) Booted multiboot kernel with non-zero bss_end_addr.
>>4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages
>> worked.
>>5) Code has soaked in an internal repo for two months.
>> Can you integrate your test kernel from 2) in tests/multiboot/ so we can
>> keep this as a regression test?
>
> Kevin and alias,
>
> Before I proceed with adding my multiboot test file, I'll clarify here that
> I started with a version from the grub2 tree.  In that file I expanded a
> header file, also from the same tree.  Neither file had any license header,
> though the tree I got them from (Dated October 2017) contains the GNU GPLv3
> license file.
>
> I'll need to check with my company before I can say I can deliver this file.
> If I deliver it, I'll add a header stating the GPL license, that it came
> from grub2 and to check its repository for contributors.
>>>
>>> Jack Schwartz (4):
>>>multiboot: bss_end_addr can be zero
>>>multiboot: Remove unused variables from multiboot.c
>>>multiboot: Use header names when displaying fields
>>>multiboot: fprintf(stderr...) -> error_report()
>>
>> Apart from the conflicts, the patches look good to me.
>
> Thanks,
> Jack
>>
>>
>> Kevin
>>
>

Re: [Qemu-devel] [PATCH 07/11] qdev: simplify the SysBusDeviceClass::init path

[Eduardo Habkost]

On Tue, Jan 16, 2018 at 10:15:51AM -0300, Philippe Mathieu-Daudé wrote:
> The SysBusDevice is the last DeviceClass::init user.
> 
> Instead of using
>   SysBusDeviceClass::realize
>-> DeviceClass::realize
>-> DeviceClass::init
>-> sysbus_device_init
>   -> SysBusDeviceClass::init
> 
> Simplify the path by directly calling SysBusDeviceClass::init
> in SysBusDeviceClass::realize:
> 
>   SysBusDeviceClass::realize
>-> SysBusDeviceClass::init
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/core/qdev.c   |  9 -
>  hw/core/sysbus.c | 21 +
>  2 files changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 2951a5..a4f76c8f5d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -221,15 +221,6 @@ void device_listener_unregister(DeviceListener *listener)
>  
>  static void device_realize(DeviceState *dev, Error **errp)
>  {
> -DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -
> -if (dc->init) {
> -int rc = dc->init(dev);
> -if (rc < 0) {
> -error_setg(errp, "Device initialization failed.");
> -return;
> -}
> -}
>  }

If you are removing the code that makes DeviceClass::init work,
you could remove the struct field as well.  I suggest either
squashing patches 07/11 and 08/11 together, or moving this hunk
to patch 08/11.

>  
>  static void device_unrealize(DeviceState *dev, Error **errp)
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 04d6061f76..9edf43bc27 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "hw/sysbus.h"
>  #include "monitor/monitor.h"
>  #include "exec/address-spaces.h"
> @@ -200,22 +201,19 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t 
> ioport, uint32_t size)
>  }
>  }
>  
> -/* TODO remove, once users are converted to realize */
> -static int sysbus_device_init(DeviceState *dev)
> +static void sysbus_realize(DeviceState *dev, Error **errp)
>  {
>  SysBusDevice *sd = SYS_BUS_DEVICE(dev);
>  SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
>  
> -if (!sbc->init) {
> -return 0;
> +/* TODO remove, once users are converted to realize */
> +if (sbc->init) {
> +int rc = sbc->init(sd);
> +if (rc < 0) {
> +error_setg(errp, "Device initialization failed.");
> +return;
> +}
>  }
> -return sbc->init(sd);
> -}
> -
> -static void sysbus_realize(DeviceState *dev, Error **errp)
> -{
> -SysBusDevice *sd = SYS_BUS_DEVICE(dev);
> -SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
>  
>  if (sbc->realize) {
>  sbc->realize(sd, errp);
> @@ -345,7 +343,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
>  static void sysbus_device_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *k = DEVICE_CLASS(klass);
> -k->init = sysbus_device_init;
>  k->realize = sysbus_realize;
>  k->unrealize = sysbus_unrealize;
>  k->bus_type = TYPE_SYSTEM_BUS;
> -- 
> 2.15.1
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH v4 2/4] target/arm: implement SHA-3 instructions

[Ard Biesheuvel]

This implements emulation of the new SHA-3 instructions that have
been added as an optional extensions to the ARMv8 Crypto Extensions
in ARM v8.2.

Signed-off-by: Ard Biesheuvel 
---
 target/arm/cpu.h   |   1 +
 target/arm/translate-a64.c | 157 ++--
 2 files changed, 146 insertions(+), 12 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 32a18510e70b..d0b19e0cbc88 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1373,6 +1373,7 @@ enum arm_features {
 ARM_FEATURE_JAZELLE, /* has (trivial) Jazelle implementation */
 ARM_FEATURE_SVE, /* has Scalable Vector Extension */
 ARM_FEATURE_V8_SHA512, /* implements SHA512 part of v8 Crypto Extensions */
+ARM_FEATURE_V8_SHA3, /* implements SHA3 part of v8 Crypto Extensions */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index fe08f3198dac..787b94047286 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -11145,8 +11145,8 @@ static void disas_crypto_three_reg_sha512(DisasContext 
*s, uint32_t insn)
 int rm = extract32(insn, 16, 5);
 int rn = extract32(insn, 5, 5);
 int rd = extract32(insn, 0, 5);
-TCGv_ptr tcg_rd_ptr, tcg_rn_ptr, tcg_rm_ptr;
 CryptoThreeOpFn *genfn;
+int feature;
 
 if (o != 0) {
 unallocated_encoding(s);
@@ -11155,20 +11155,24 @@ static void 
disas_crypto_three_reg_sha512(DisasContext *s, uint32_t insn)
 
 switch (opcode) {
 case 0: /* SHA512H */
+feature = ARM_FEATURE_V8_SHA512;
 genfn = gen_helper_crypto_sha512h;
 break;
 case 1: /* SHA512H2 */
+feature = ARM_FEATURE_V8_SHA512;
 genfn = gen_helper_crypto_sha512h2;
 break;
 case 2: /* SHA512SU1 */
+feature = ARM_FEATURE_V8_SHA512;
 genfn = gen_helper_crypto_sha512su1;
 break;
-default:
-unallocated_encoding(s);
-return;
+case 3: /* RAX1 */
+feature = ARM_FEATURE_V8_SHA3;
+genfn = NULL;
+break;
 }
 
-if (!arm_dc_feature(s, ARM_FEATURE_V8_SHA512)) {
+if (!arm_dc_feature(s, feature)) {
 unallocated_encoding(s);
 return;
 }
@@ -11177,15 +11181,42 @@ static void 
disas_crypto_three_reg_sha512(DisasContext *s, uint32_t insn)
 return;
 }
 
-tcg_rd_ptr = vec_full_reg_ptr(s, rd);
-tcg_rn_ptr = vec_full_reg_ptr(s, rn);
-tcg_rm_ptr = vec_full_reg_ptr(s, rm);
+if (genfn) {
+TCGv_ptr tcg_rd_ptr, tcg_rn_ptr, tcg_rm_ptr;
 
-genfn(tcg_rd_ptr, tcg_rn_ptr, tcg_rm_ptr);
+tcg_rd_ptr = vec_full_reg_ptr(s, rd);
+tcg_rn_ptr = vec_full_reg_ptr(s, rn);
+tcg_rm_ptr = vec_full_reg_ptr(s, rm);
 
-tcg_temp_free_ptr(tcg_rd_ptr);
-tcg_temp_free_ptr(tcg_rn_ptr);
-tcg_temp_free_ptr(tcg_rm_ptr);
+genfn(tcg_rd_ptr, tcg_rn_ptr, tcg_rm_ptr);
+
+tcg_temp_free_ptr(tcg_rd_ptr);
+tcg_temp_free_ptr(tcg_rn_ptr);
+tcg_temp_free_ptr(tcg_rm_ptr);
+} else {
+TCGv_i64 tcg_op1, tcg_op2, tcg_res[2];
+int pass;
+
+tcg_op1 = tcg_temp_new_i64();
+tcg_op2 = tcg_temp_new_i64();
+tcg_res[0] = tcg_temp_new_i64();
+tcg_res[1] = tcg_temp_new_i64();
+
+for (pass = 0; pass < 2; pass++) {
+read_vec_element(s, tcg_op1, rn, pass, MO_64);
+read_vec_element(s, tcg_op2, rm, pass, MO_64);
+
+tcg_gen_rotli_i64(tcg_res[pass], tcg_op2, 1);
+tcg_gen_xor_i64(tcg_res[pass], tcg_res[pass], tcg_op1);
+}
+write_vec_element(s, tcg_res[0], rd, 0, MO_64);
+write_vec_element(s, tcg_res[1], rd, 1, MO_64);
+
+tcg_temp_free(tcg_op1);
+tcg_temp_free(tcg_op2);
+tcg_temp_free(tcg_res[0]);
+tcg_temp_free(tcg_res[1]);
+}
 }
 
 /* Crypto two-reg SHA512
@@ -11229,6 +11260,106 @@ static void disas_crypto_two_reg_sha512(DisasContext 
*s, uint32_t insn)
 tcg_temp_free_ptr(tcg_rn_ptr);
 }
 
+/* Crypto four-register
+ *  31   23 22 21 20  16 15  14  10 95 40
+ * +---+-+--+---+--+--+--+
+ * | 1 1 0 0 1 1 1 0 0 | Op0 |  Rm  | 0 |  Ra  |  Rn  |  Rd  |
+ * +---+-+--+---+--+--+--+
+ */
+static void disas_crypto_four_reg(DisasContext *s, uint32_t insn)
+{
+int op0 = extract32(insn, 21, 2);
+int rm = extract32(insn, 16, 5);
+int ra = extract32(insn, 10, 5);
+int rn = extract32(insn, 5, 5);
+int rd = extract32(insn, 0, 5);
+TCGv_i64 tcg_op1, tcg_op2, tcg_op3, tcg_res[2];
+int pass;
+
+if (op0 > 1 || !arm_dc_feature(s, ARM_FEATURE_V8_SHA3)) {
+unallocated_encoding(s);
+return;
+}
+
+if (!fp_access_check(s)) {
+return;
+}
+
+tcg_op1 = tcg_temp_new_i64();
+tcg_op2 = tcg_temp_new_i64();
+tcg_op3 = tcg_temp_new_i64();
+tcg_res[0] = tcg_temp_new_i64();
+  

[Qemu-devel] [PATCH v4 0/4] target-arm: add SHA-3, SM3 and SHA512 instruction support

[Ard Biesheuvel]

Changes since v3:
- don't bother with helpers for the SHA3 instructions: they are simple enough
  to be emitted as TCG ops directly
- rebase onto Richard's pending SVE work

Changes since v2:
- fix thinko in big-endian aware handling of 64-bit quantities: this is not
  needed given that the NEON registers are represented as arrays of uint64_t
  so they always appear in the correct order.
- add support for SM3 instructions (Chinese SHA derivative)

Changes since v1:
- update SHA512 patch to adhere more closely to the existing style, and to
  the way the instruction encodings are classified in the ARM ARM (#1)
- add patch implementing the new SHA3 instructions EOR3/RAX1/XAR/BCAX (#2)
- enable support for these instructions in user mode emulation (#3)

Ard Biesheuvel (4):
  target/arm: implement SHA-512 instructions
  target/arm: implement SHA-3 instructions
  target/arm: implement SM3 instructions
  target/arm: enable user-mode SHA-3, SM3 and SHA-512 instruction
support

 linux-user/elfload.c   |  18 ++
 target/arm/cpu.h   |   3 +
 target/arm/cpu64.c |   3 +
 target/arm/crypto_helper.c | 192 +++-
 target/arm/helper.h|  10 +
 target/arm/translate-a64.c | 317 
 6 files changed, 542 insertions(+), 1 deletion(-)

-- 
2.11.0

[Qemu-devel] [PATCH v4 4/4] target/arm: enable user-mode SHA-3, SM3 and SHA-512 instruction support

[Ard Biesheuvel]

Add support for the new ARMv8.2 SHA-3, SM3 and SHA-512 instructions to
AArch64 user mode emulation.

Signed-off-by: Ard Biesheuvel 
---
 linux-user/elfload.c | 18 ++
 target/arm/cpu64.c   |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 20f3d8c2c373..5d5aa26d2710 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -512,6 +512,21 @@ enum {
 ARM_HWCAP_A64_SHA1  = 1 << 5,
 ARM_HWCAP_A64_SHA2  = 1 << 6,
 ARM_HWCAP_A64_CRC32 = 1 << 7,
+ARM_HWCAP_A64_ATOMICS   = 1 << 8,
+ARM_HWCAP_A64_FPHP  = 1 << 9,
+ARM_HWCAP_A64_ASIMDHP   = 1 << 10,
+ARM_HWCAP_A64_CPUID = 1 << 11,
+ARM_HWCAP_A64_ASIMDRDM  = 1 << 12,
+ARM_HWCAP_A64_JSCVT = 1 << 13,
+ARM_HWCAP_A64_FCMA  = 1 << 14,
+ARM_HWCAP_A64_LRCPC = 1 << 15,
+ARM_HWCAP_A64_DCPOP = 1 << 16,
+ARM_HWCAP_A64_SHA3  = 1 << 17,
+ARM_HWCAP_A64_SM3   = 1 << 18,
+ARM_HWCAP_A64_SM4   = 1 << 19,
+ARM_HWCAP_A64_ASIMDDP   = 1 << 20,
+ARM_HWCAP_A64_SHA512= 1 << 21,
+ARM_HWCAP_A64_SVE   = 1 << 22,
 };
 
 #define ELF_HWCAP get_elf_hwcap()
@@ -532,6 +547,9 @@ static uint32_t get_elf_hwcap(void)
 GET_FEATURE(ARM_FEATURE_V8_SHA1, ARM_HWCAP_A64_SHA1);
 GET_FEATURE(ARM_FEATURE_V8_SHA256, ARM_HWCAP_A64_SHA2);
 GET_FEATURE(ARM_FEATURE_CRC, ARM_HWCAP_A64_CRC32);
+GET_FEATURE(ARM_FEATURE_V8_SHA3, ARM_HWCAP_A64_SHA3);
+GET_FEATURE(ARM_FEATURE_V8_SM3, ARM_HWCAP_A64_SM3);
+GET_FEATURE(ARM_FEATURE_V8_SHA512, ARM_HWCAP_A64_SHA512);
 #undef GET_FEATURE
 
 return hwcaps;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 670c07ab6ed4..56d50ba57194 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -224,6 +224,9 @@ static void aarch64_any_initfn(Object *obj)
 set_feature(>env, ARM_FEATURE_V8_AES);
 set_feature(>env, ARM_FEATURE_V8_SHA1);
 set_feature(>env, ARM_FEATURE_V8_SHA256);
+set_feature(>env, ARM_FEATURE_V8_SHA512);
+set_feature(>env, ARM_FEATURE_V8_SHA3);
+set_feature(>env, ARM_FEATURE_V8_SM3);
 set_feature(>env, ARM_FEATURE_V8_PMULL);
 set_feature(>env, ARM_FEATURE_CRC);
 cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
-- 
2.11.0

[Qemu-devel] [PATCH v4 3/4] target/arm: implement SM3 instructions

[Ard Biesheuvel]

This implements emulation of the new SM3 instructions that have
been added as an optional extension to the ARMv8 Crypto Extensions
in ARM v8.2.

Signed-off-by: Ard Biesheuvel 
---
 target/arm/cpu.h   |   1 +
 target/arm/crypto_helper.c | 117 +
 target/arm/helper.h|   5 +
 target/arm/translate-a64.c | 183 ++--
 4 files changed, 257 insertions(+), 49 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d0b19e0cbc88..18383666e02d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1374,6 +1374,7 @@ enum arm_features {
 ARM_FEATURE_SVE, /* has Scalable Vector Extension */
 ARM_FEATURE_V8_SHA512, /* implements SHA512 part of v8 Crypto Extensions */
 ARM_FEATURE_V8_SHA3, /* implements SHA3 part of v8 Crypto Extensions */
+ARM_FEATURE_V8_SM3, /* implements SM3 part of v8 Crypto Extensions */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/crypto_helper.c b/target/arm/crypto_helper.c
index fb45948e9f13..c1d9f765cd40 100644
--- a/target/arm/crypto_helper.c
+++ b/target/arm/crypto_helper.c
@@ -492,3 +492,120 @@ void HELPER(crypto_sha512su1)(void *vd, void *vn, void 
*vm)
 rd[0] += s1_512(rn[0]) + rm[0];
 rd[1] += s1_512(rn[1]) + rm[1];
 }
+
+void HELPER(crypto_sm3partw1)(void *vd, void *vn, void *vm)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *rm = vm;
+union CRYPTO_STATE d = { .l = { rd[0], rd[1] } };
+union CRYPTO_STATE n = { .l = { rn[0], rn[1] } };
+union CRYPTO_STATE m = { .l = { rm[0], rm[1] } };
+uint32_t t;
+
+t = CR_ST_WORD(d, 0) ^ CR_ST_WORD(n, 0) ^ ror32(CR_ST_WORD(m, 1), 17);
+CR_ST_WORD(d, 0) = t ^ ror32(t, 17) ^ ror32(t, 9);
+
+t = CR_ST_WORD(d, 1) ^ CR_ST_WORD(n, 1) ^ ror32(CR_ST_WORD(m, 2), 17);
+CR_ST_WORD(d, 1) = t ^ ror32(t, 17) ^ ror32(t, 9);
+
+t = CR_ST_WORD(d, 2) ^ CR_ST_WORD(n, 2) ^ ror32(CR_ST_WORD(m, 3), 17);
+CR_ST_WORD(d, 2) = t ^ ror32(t, 17) ^ ror32(t, 9);
+
+t = CR_ST_WORD(d, 3) ^ CR_ST_WORD(n, 3) ^ ror32(CR_ST_WORD(d, 0), 17);
+CR_ST_WORD(d, 3) = t ^ ror32(t, 17) ^ ror32(t, 9);
+
+rd[0] = d.l[0];
+rd[1] = d.l[1];
+}
+
+void HELPER(crypto_sm3partw2)(void *vd, void *vn, void *vm)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *rm = vm;
+union CRYPTO_STATE d = { .l = { rd[0], rd[1] } };
+union CRYPTO_STATE n = { .l = { rn[0], rn[1] } };
+union CRYPTO_STATE m = { .l = { rm[0], rm[1] } };
+uint32_t t = CR_ST_WORD(n, 0) ^ ror32(CR_ST_WORD(m, 0), 25);
+
+CR_ST_WORD(d, 0) ^= t;
+CR_ST_WORD(d, 1) ^= CR_ST_WORD(n, 1) ^ ror32(CR_ST_WORD(m, 1), 25);
+CR_ST_WORD(d, 2) ^= CR_ST_WORD(n, 2) ^ ror32(CR_ST_WORD(m, 2), 25);
+CR_ST_WORD(d, 3) ^= CR_ST_WORD(n, 3) ^ ror32(CR_ST_WORD(m, 3), 25) ^
+ror32(t, 17) ^ ror32(t, 2) ^ ror32(t, 26);
+
+rd[0] = d.l[0];
+rd[1] = d.l[1];
+}
+
+void HELPER(crypto_sm3ss1)(void *vd, void *vn, void *va, void *vm)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *ra = va;
+uint64_t *rm = vm;
+union CRYPTO_STATE d;
+union CRYPTO_STATE n = { .l = { rn[0], rn[1] } };
+union CRYPTO_STATE a = { .l = { ra[0], ra[1] } };
+union CRYPTO_STATE m = { .l = { rm[0], rm[1] } };
+
+CR_ST_WORD(d, 0) = 0;
+CR_ST_WORD(d, 1) = 0;
+CR_ST_WORD(d, 2) = 0;
+CR_ST_WORD(d, 3) = ror32(ror32(CR_ST_WORD(n, 3), 20) + CR_ST_WORD(m, 3) +
+ CR_ST_WORD(a, 3), 25);
+
+rd[0] = d.l[0];
+rd[1] = d.l[1];
+}
+
+void HELPER(crypto_sm3tt)(void *vd, void *vn, void *vm, uint32_t imm2,
+  uint32_t opcode)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *rm = vm;
+union CRYPTO_STATE d = { .l = { rd[0], rd[1] } };
+union CRYPTO_STATE n = { .l = { rn[0], rn[1] } };
+union CRYPTO_STATE m = { .l = { rm[0], rm[1] } };
+uint32_t t;
+
+assert(imm2 < 4);
+
+if (opcode == 0 || opcode == 2) {
+/* SM3TT1A, SM3TT2A */
+t = par(CR_ST_WORD(d, 3), CR_ST_WORD(d, 2), CR_ST_WORD(d, 1));
+} else if (opcode == 1) {
+/* SM3TT1B */
+t = maj(CR_ST_WORD(d, 3), CR_ST_WORD(d, 2), CR_ST_WORD(d, 1));
+} else if (opcode == 3) {
+/* SM3TT2B */
+t = cho(CR_ST_WORD(d, 3), CR_ST_WORD(d, 2), CR_ST_WORD(d, 1));
+} else {
+g_assert_not_reached();
+}
+
+t += CR_ST_WORD(d, 0) + CR_ST_WORD(m, imm2);
+
+CR_ST_WORD(d, 0) = CR_ST_WORD(d, 1);
+
+if (opcode < 2) {
+/* SM3TT1A, SM3TT1B */
+t += CR_ST_WORD(n, 3) ^ ror32(CR_ST_WORD(d, 3), 20);
+
+CR_ST_WORD(d, 1) = ror32(CR_ST_WORD(d, 2), 23);
+} else {
+/* SM3TT2A, SM3TT2B */
+t += CR_ST_WORD(n, 3);
+t ^= rol32(t, 9) ^ rol32(t, 17);
+
+CR_ST_WORD(d, 1) = ror32(CR_ST_WORD(d, 2), 13);
+}
+
+CR_ST_WORD(d, 2) = CR_ST_WORD(d, 3);
+CR_ST_WORD(d, 3) = t;
+
+rd[0] = d.l[0];
+rd[1] = d.l[1];
+}

[Qemu-devel] [PATCH v4 1/4] target/arm: implement SHA-512 instructions

[Ard Biesheuvel]

This implements emulation of the new SHA-512 instructions that have
been added as an optional extension to the ARMv8 Crypto Extensions
in ARM v8.2.

Signed-off-by: Ard Biesheuvel 
---
 target/arm/cpu.h   |  1 +
 target/arm/crypto_helper.c | 75 ++-
 target/arm/helper.h|  5 +
 target/arm/translate-a64.c | 99 
 4 files changed, 179 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 0a923e42d8bf..32a18510e70b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1372,6 +1372,7 @@ enum arm_features {
 ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
 ARM_FEATURE_JAZELLE, /* has (trivial) Jazelle implementation */
 ARM_FEATURE_SVE, /* has Scalable Vector Extension */
+ARM_FEATURE_V8_SHA512, /* implements SHA512 part of v8 Crypto Extensions */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/crypto_helper.c b/target/arm/crypto_helper.c
index 9ca0bdead7bb..fb45948e9f13 100644
--- a/target/arm/crypto_helper.c
+++ b/target/arm/crypto_helper.c
@@ -1,7 +1,7 @@
 /*
  * crypto_helper.c - emulate v8 Crypto Extensions instructions
  *
- * Copyright (C) 2013 - 2014 Linaro Ltd 
+ * Copyright (C) 2013 - 2018 Linaro Ltd 
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -419,3 +419,76 @@ void HELPER(crypto_sha256su1)(void *vd, void *vn, void *vm)
 rd[0] = d.l[0];
 rd[1] = d.l[1];
 }
+
+/*
+ * The SHA-512 logical functions (same as above but using 64-bit operands)
+ */
+
+static uint64_t cho512(uint64_t x, uint64_t y, uint64_t z)
+{
+return (x & (y ^ z)) ^ z;
+}
+
+static uint64_t maj512(uint64_t x, uint64_t y, uint64_t z)
+{
+return (x & y) | ((x | y) & z);
+}
+
+static uint64_t S0_512(uint64_t x)
+{
+return ror64(x, 28) ^ ror64(x, 34) ^ ror64(x, 39);
+}
+
+static uint64_t S1_512(uint64_t x)
+{
+return ror64(x, 14) ^ ror64(x, 18) ^ ror64(x, 41);
+}
+
+static uint64_t s0_512(uint64_t x)
+{
+return ror64(x, 1) ^ ror64(x, 8) ^ (x >> 7);
+}
+
+static uint64_t s1_512(uint64_t x)
+{
+return ror64(x, 19) ^ ror64(x, 61) ^ (x >> 6);
+}
+
+void HELPER(crypto_sha512h)(void *vd, void *vn, void *vm)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *rm = vm;
+
+rd[1] += S1_512(rm[1]) + cho512(rm[1], rn[0], rn[1]);
+rd[0] += S1_512(rd[1] + rm[0]) + cho512(rd[1] + rm[0], rm[1], rn[0]);
+}
+
+void HELPER(crypto_sha512h2)(void *vd, void *vn, void *vm)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *rm = vm;
+
+rd[1] += S0_512(rm[0]) + maj512(rn[0], rm[1], rm[0]);
+rd[0] += S0_512(rd[1]) + maj512(rd[1], rm[0], rm[1]);
+}
+
+void HELPER(crypto_sha512su0)(void *vd, void *vn)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+
+rd[0] += s0_512(rd[1]);
+rd[1] += s0_512(rn[0]);
+}
+
+void HELPER(crypto_sha512su1)(void *vd, void *vn, void *vm)
+{
+uint64_t *rd = vd;
+uint64_t *rn = vn;
+uint64_t *rm = vm;
+
+rd[0] += s1_512(rn[0]) + rm[0];
+rd[1] += s1_512(rn[1]) + rm[1];
+}
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 5dec2e62626b..81d460702867 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -534,6 +534,11 @@ DEF_HELPER_FLAGS_3(crypto_sha256h2, TCG_CALL_NO_RWG, void, 
ptr, ptr, ptr)
 DEF_HELPER_FLAGS_2(crypto_sha256su0, TCG_CALL_NO_RWG, void, ptr, ptr)
 DEF_HELPER_FLAGS_3(crypto_sha256su1, TCG_CALL_NO_RWG, void, ptr, ptr, ptr)
 
+DEF_HELPER_FLAGS_3(crypto_sha512h, TCG_CALL_NO_RWG, void, ptr, ptr, ptr)
+DEF_HELPER_FLAGS_3(crypto_sha512h2, TCG_CALL_NO_RWG, void, ptr, ptr, ptr)
+DEF_HELPER_FLAGS_2(crypto_sha512su0, TCG_CALL_NO_RWG, void, ptr, ptr)
+DEF_HELPER_FLAGS_3(crypto_sha512su1, TCG_CALL_NO_RWG, void, ptr, ptr, ptr)
+
 DEF_HELPER_FLAGS_3(crc32, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32)
 DEF_HELPER_FLAGS_3(crc32c, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32)
 DEF_HELPER_2(dc_zva, void, env, i64)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 10eef870fee2..fe08f3198dac 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -11132,6 +11132,103 @@ static void disas_crypto_two_reg_sha(DisasContext *s, 
uint32_t insn)
 tcg_temp_free_ptr(tcg_rn_ptr);
 }
 
+/* Crypto three-reg SHA512
+ *  31   21 20  16 15  14  13 12  11  10  95 40
+ * +---+--+---+---+-++--+--+
+ * | 1 1 0 0 1 1 1 0 0 1 1 |  Rm  | 1 | O | 0 0 | opcode |  Rn  |  Rd  |
+ * +---+--+---+---+-++--+--+
+ */
+static void disas_crypto_three_reg_sha512(DisasContext *s, uint32_t insn)
+{
+int opcode = extract32(insn, 10, 2);
+int o =  extract32(insn, 14, 1);
+int rm = extract32(insn, 16, 5);
+int rn = extract32(insn, 5, 5);
+int rd = extract32(insn, 0, 5);
+

Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test

[John Snow]

On 01/19/2018 01:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:
>> 17.01.2018 21:30, John Snow wrote:
>>>
>>> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
 Thank you for reviewing my code!

>>> Time for the re-spin? There's pretty good pressure to get this into 2.12
>>> and say the non-nbd workflow model is feature complete.
>>
>> Yes, you are right. Hope to do it today or tomorrow.
>>
> 
> I've rebased, applied comments from review, and even one some new fixes,
> but now I understand that it is too early for re-spin.
> 
> Now this series depends on
> 1. [PATCH v2 0/3] fix bitmaps migration through shared storage
>   - we need to decide, how to fix it. May be, we just do not need
> bitmaps in inactive state, and should not load them in do_qcow2_open in
> this case
>   [I can ignore it, just dropping one test case from new iotest, and fix
> it later, but..
> 2. [PATCH v2 0/6] qmp dirty bitmap API
>  - here autoload is dropped, and migration should be rebased on it.
> 
> so, I'll re-spin migration after these two questions are resolved.
> 

You got it, thanks.

Re: [Qemu-devel] [PATCH v8 09/17] sdhci: add basic Spec v1 capabilities

[Alistair Francis]

On Thu, Jan 18, 2018 at 10:31 AM, Philippe Mathieu-Daudé
 wrote:
> (Note than Spec v2 is supported by default)
>
> we emit a warning for missing capabilities bits.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/sd/sdhci-internal.h | 24 ++-
>  include/hw/sd/sdhci.h  | 11 +
>  hw/sd/sdhci.c  | 65 
> ++
>  3 files changed, 84 insertions(+), 16 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index b7751c815f..9acafe7b01 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -24,6 +24,8 @@
>  #ifndef SDHCI_INTERNAL_H
>  #define SDHCI_INTERNAL_H
>
> +#include "hw/registerfields.h"
> +
>  /* R/W SDMA System Address register 0x0 */
>  #define SDHC_SYSAD 0x00
>
> @@ -84,6 +86,9 @@
>
>  /* R/W Host control Register 0x0 */
>  #define SDHC_HOSTCTL   0x28
> +FIELD(SDHC_HOSTCTL, LED_CTRL,  0, 1);
> +FIELD(SDHC_HOSTCTL, DATATRANSFERWIDTH, 1, 1); /* SD mode only */
> +FIELD(SDHC_HOSTCTL, HIGH_SPEED,2, 1);
>  #define SDHC_CTRL_DMA_CHECK_MASK   0x18
>  #define SDHC_CTRL_SDMA 0x00
>  #define SDHC_CTRL_ADMA1_32 0x08
> @@ -94,6 +99,7 @@
>  /* R/W Power Control Register 0x0 */
>  #define SDHC_PWRCON0x29
>  #define SDHC_POWER_ON  (1 << 0)
> +FIELD(SDHC_PWRCON, BUS_VOLTAGE,1, 3);
>
>  /* R/W Block Gap Control Register 0x0 */
>  #define SDHC_BLKGAP0x2A
> @@ -116,6 +122,7 @@
>
>  /* R/W Timeout Control Register 0x0 */
>  #define SDHC_TIMEOUTCON0x2E
> +FIELD(SDHC_TIMEOUTCON, COUNTER,0, 4);
>
>  /* R/W Software Reset Register 0x0 */
>  #define SDHC_SWRST 0x2F
> @@ -172,17 +179,32 @@
>
>  /* ROC Auto CMD12 error status register 0x0 */
>  #define SDHC_ACMD12ERRSTS  0x3C
> +FIELD(SDHC_ACMD12ERRSTS, TIMEOUT_ERR,  1, 1);
> +FIELD(SDHC_ACMD12ERRSTS, CRC_ERR,  2, 1);
> +FIELD(SDHC_ACMD12ERRSTS, INDEX_ERR,4, 1);
>
>  /* HWInit Capabilities Register 0x05E80080 */
>  #define SDHC_CAPAB 0x40
> -#define SDHC_CAN_DO_DMA0x0040
>  #define SDHC_CAN_DO_ADMA2  0x0008
>  #define SDHC_CAN_DO_ADMA1  0x0010
>  #define SDHC_64_BIT_BUS_SUPPORT(1 << 28)
>  #define SDHC_CAPAB_BLOCKSIZE(x)(((x) >> 16) & 0x3)
> +FIELD(SDHC_CAPAB, TOCLKFREQ,   0, 6);
> +FIELD(SDHC_CAPAB, TOUNIT,  7, 1);
> +FIELD(SDHC_CAPAB, BASECLKFREQ, 8, 8);
> +FIELD(SDHC_CAPAB, MAXBLOCKLENGTH, 16, 2);
> +FIELD(SDHC_CAPAB, HIGHSPEED,  21, 1);
> +FIELD(SDHC_CAPAB, SDMA,   22, 1);
> +FIELD(SDHC_CAPAB, SUSPRESUME, 23, 1);
> +FIELD(SDHC_CAPAB, V33,24, 1);
> +FIELD(SDHC_CAPAB, V30,25, 1);
> +FIELD(SDHC_CAPAB, V18,26, 1);
>
>  /* HWInit Maximum Current Capabilities Register 0x0 */
>  #define SDHC_MAXCURR   0x48
> +FIELD(SDHC_MAXCURR, V33_VDD1,  0, 8);
> +FIELD(SDHC_MAXCURR, V30_VDD1,  8, 8);
> +FIELD(SDHC_MAXCURR, V18_VDD1, 16, 8);
>
>  /* W Force Event Auto CMD12 Error Interrupt Register 0x */
>  #define SDHC_FEAER 0x50
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index c174a39ecf..bc80f59d3c 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -92,6 +92,17 @@ typedef struct SDHCIState {
>  /* Configurable properties */
>  bool pending_insert_quirk; /* Quirk for Raspberry Pi card insert int */
>  uint8_t spec_version;
> +struct {
> +/***
> + * Spec v1
> + ***/
> +/* Suspend/resume */
> +bool suspend;
> +bool high_speed;
> +bool sdma;
> +/* Voltage */
> +bool v33, v30, v18;
> +} cap;
>  } SDHCIState;
>
>  #define TYPE_PCI_SDHCI "sdhci-pci"
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 76714f1143..e56ee7778c 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -23,6 +23,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "hw/hw.h"
>  #include "sysemu/block-backend.h"
> @@ -45,12 +46,6 @@
>   * 0 - not supported, 1 - supported, other - prohibited.
>   */
>  #define SDHC_CAPAB_64BITBUS   0ul/* 64-bit System Bus Support */
> -#define SDHC_CAPAB_18V1ul/* Voltage support 1.8v */
> -#define SDHC_CAPAB_30V0ul/* Voltage support 3.0v */
> -#define SDHC_CAPAB_33V1ul/* Voltage support 3.3v */
> -#define SDHC_CAPAB_SUSPRESUME 0ul/* Suspend/resume support */
> -#define SDHC_CAPAB_SDMA   1ul/* SDMA support */
> -#define SDHC_CAPAB_HIGHSPEED  1ul/* High speed support */
>  #define 

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Philippe Mathieu-Daudé]

On 01/19/2018 03:01 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4...@amsat.org) wrote:
>> (incorrectly use in 3be98be4e9f)
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
> 
> I'm a bit confused; isnt the only difference between the nocheck
> versions that it'll fail at compile time instead of link?

You are right, this isn't the right approach.

Peter gave a clearer explanation here:
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02103.html

'''
because atomic operations on types larger than the host pointer
size are not portable to all architectures. This should probably
use the atomic_cmpxchg(), not __nocheck, because the latter
bypasses the build time assert for this problem and turns a
"fails on any 32-bit host" into "fails obscurely on some
architectures only".
'''

Maybe we should remove the __nocheck() functions and inline them in the
'checked' functions?
There is no performance cost doing this, right?

> 
> Dave
> 
>> ---
>> currently on ppc32 the linking fails:
>>
>>   CC  migration/postcopy-ram.o
>> ...
>>   LINKmicroblaze-softmmu/qemu-system-microblaze
>> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
>> migration/postcopy-ram.c:717: undefined reference to `__atomic_fetch_add_8'
>> migration/postcopy-ram.c:738: undefined reference to `__atomic_fetch_add_8'
>> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
>> migration/postcopy-ram.c:651: undefined reference to `__atomic_exchange_8'
>> migration/postcopy-ram.c:652: undefined reference to `__atomic_exchange_8'
>> migration/postcopy-ram.c:661: undefined reference to `__atomic_exchange_8'
>> collect2: error: ld returned 1 exit status
>> Makefile:193: recipe for target 'qemu-system-microblaze' failed
>> make[1]: *** [qemu-system-microblaze] Error 1
>>
>> with this patch the compilation fails:
>>
>>   CC  migration/postcopy-ram.o
>> In file included from include/qemu/osdep.h:36:0,
>>  from migration/postcopy-ram.c:19:
>> migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin':
>> include/qemu/compiler.h:86:30: error: static assertion failed: "not 
>> expecting: sizeof(*>last_begin) > ATOMIC_REG_SIZE"
>>  #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
>>   ^
>> include/qemu/atomic.h:183:5: note: in expansion of macro 'QEMU_BUILD_BUG_ON'
>>  QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);  \
>>  ^
>> migration/postcopy-ram.c:651:5: note: in expansion of macro 'atomic_xchg'
>>  atomic_xchg(>last_begin, now_ms);
>>  ^
>>
>>  migration/postcopy-ram.c | 12 ++--
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 7814da5b4b..6ecc1aa820 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -648,17 +648,17 @@ static void mark_postcopy_blocktime_begin(uintptr_t 
>> addr, uint32_t ptid,
>>  atomic_inc(>smp_cpus_down);
>>  }
>>  
>> -atomic_xchg__nocheck(>last_begin, now_ms);
>> -atomic_xchg__nocheck(>page_fault_vcpu_time[cpu], now_ms);
>> -atomic_xchg__nocheck(>vcpu_addr[cpu], addr);
>> +atomic_xchg(>last_begin, now_ms);
>> +atomic_xchg(>page_fault_vcpu_time[cpu], now_ms);
>> +atomic_xchg(>vcpu_addr[cpu], addr);
>>  
>>  /* check it here, not at the begining of the function,
>>   * due to, check could accur early than bitmap_set in
>>   * qemu_ufd_copy_ioctl */
>>  already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
>>  if (already_received) {
>> -atomic_xchg__nocheck(>vcpu_addr[cpu], 0);
>> -atomic_xchg__nocheck(>page_fault_vcpu_time[cpu], 0);
>> +atomic_xchg(>vcpu_addr[cpu], 0);
>> +atomic_xchg(>page_fault_vcpu_time[cpu], 0);
>>  atomic_dec(>smp_cpus_down);
>>  }
>>  trace_mark_postcopy_blocktime_begin(addr, dc, 
>> dc->page_fault_vcpu_time[cpu],
>> @@ -719,7 +719,7 @@ static void mark_postcopy_blocktime_end(uintptr_t addr)
>>  read_vcpu_time == 0) {
>>  continue;
>>  }
>> -atomic_xchg__nocheck(>vcpu_addr[i], 0);
>> +atomic_xchg(>vcpu_addr[i], 0);
>>  vcpu_blocktime = now_ms - read_vcpu_time;
>>  affected_cpu += 1;
>>  /* we need to know is that mark_postcopy_end was due to
>> -- 
>> 2.15.1
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH 03/11] hw/i2c: convert I2CSlaveClass::init -> realize

[Eduardo Habkost]

On Tue, Jan 16, 2018 at 10:15:47AM -0300, Philippe Mathieu-Daudé wrote:
[...]
> -static int i2c_slave_qdev_init(DeviceState *dev)
> +static void i2c_slave_realize(DeviceState *dev, Error **errp)
>  {
>  I2CSlave *s = I2C_SLAVE(dev);
>  I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
>  
> -if (sc->init) {
> -return sc->init(s);
> +if (sc->realize) {
> +return sc->realize(s, errp);
>  }

Do you think i2c_slave_realize() will perform additional actions
in the future?  If not, why not let subclasses set
DeviceClass::realize directly?

> -
> -return 0;
>  }
>  
>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr)
> @@ -301,7 +299,7 @@ DeviceState *i2c_create_slave(I2CBus *bus, const char 
> *name, uint8_t addr)
>  static void i2c_slave_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *k = DEVICE_CLASS(klass);
> -k->init = i2c_slave_qdev_init;
> +k->realize = i2c_slave_realize;
>  set_bit(DEVICE_CATEGORY_MISC, k->categories);
>  k->bus_type = TYPE_I2C_BUS;
>  k->props = i2c_props;
[...]

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 8/9] xlnx-zynqmp-pmu: Connect the IPI device to the PMU

[Alistair Francis]

On Thu, Jan 18, 2018 at 1:42 PM, Philippe Mathieu-Daudé  wrote:
> On 01/18/2018 03:38 PM, Alistair Francis wrote:
>> Signed-off-by: Alistair Francis 
>> Reviewed-by: Edgar E. Iglesias 
>> ---
>> V4:
>>  - Move the IPI to the machine instead of the SoC
>>
>>  hw/microblaze/xlnx-zynqmp-pmu.c | 31 +++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c 
>> b/hw/microblaze/xlnx-zynqmp-pmu.c
>> index 7312bfe23e..999a5657cf 100644
>> --- a/hw/microblaze/xlnx-zynqmp-pmu.c
>> +++ b/hw/microblaze/xlnx-zynqmp-pmu.c
>> @@ -24,6 +24,7 @@
>>  #include "cpu.h"
>>  #include "boot.h"
>>
>> +#include "hw/intc/xlnx-zynqmp-ipi.h"
>>  #include "hw/intc/xlnx-pmu-iomod-intc.h"
>>
>>  /* Define the PMU device */
>> @@ -38,6 +39,15 @@
>>
>>  #define XLNX_ZYNQMP_PMU_INTC_ADDR   0xFFD4
>>
>> +#define XLNX_ZYNQMP_PMU_NUM_IPIS4
>> +
>> +static const uint64_t ipi_addr[XLNX_ZYNQMP_PMU_NUM_IPIS] = {
>> +0xFF34, 0xFF35, 0xFF36, 0xFF37,
>> +};
>> +static const uint64_t ipi_irq[XLNX_ZYNQMP_PMU_NUM_IPIS] = {
>> +19, 20, 21, 22,
>> +};
>> +
>>  typedef struct XlnxZynqMPPMUSoCState {
>>  /*< private >*/
>>  DeviceState parent_obj;
>> @@ -136,6 +146,9 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine)
>>  MemoryRegion *address_space_mem = get_system_memory();
>>  MemoryRegion *pmu_rom = g_new(MemoryRegion, 1);
>>  MemoryRegion *pmu_ram = g_new(MemoryRegion, 1);
>> +XlnxZynqMPIPI *ipi[XLNX_ZYNQMP_PMU_NUM_IPIS];
>
> Why use pointers and g_new() here?
>
> Isn't a plain array simpler?

I remember that I tried that and ran into issues. It was so long ago
though that I can't remember what the problem was. I think it was
related to adding the object as a child.

Alistair

>
>XlnxZynqMPIPI ipi[XLNX_ZYNQMP_PMU_NUM_IPIS];
>
> (same apply to pmu_rom/ram)
>
> Or maybe do you plan to start implementing MachineClass::exit()?
>
> (or go in this direction, which might make sens thinking about having a
> multiarch single binary).
>
>> +qemu_irq irq[32];
>> +int i;
>>
>>  /* Create the ROM */
>>  memory_region_init_rom(pmu_rom, NULL, "xlnx-zynqmp-pmu.rom",
>> @@ -155,6 +168,24 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine)
>>_abort);
>>  object_property_set_bool(OBJECT(pmu), true, "realized", _fatal);
>>
>> +for (i = 0; i < 32; i++) {
>> +irq[i] = qdev_get_gpio_in(DEVICE(>intc), i);
>> +}
>> +
>> +/* Create and connect the IPI device */
>> +for (i = 0; i < XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
>> +ipi[i] = g_new0(XlnxZynqMPIPI, 1);
>> +object_initialize(ipi[i], sizeof(XlnxZynqMPIPI), 
>> TYPE_XLNX_ZYNQMP_IPI);
>> +qdev_set_parent_bus(DEVICE(ipi[i]), sysbus_get_default());
>> +}
>> +
>> +for (i = 0; i < XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
>> +object_property_set_bool(OBJECT(ipi[i]), true, "realized",
>> + _abort);
>> +sysbus_mmio_map(SYS_BUS_DEVICE(ipi[i]), 0, ipi_addr[i]);
>> +sysbus_connect_irq(SYS_BUS_DEVICE(ipi[i]), 0, irq[ipi_irq[i]]);
>> +}
>> +
>>  /* Load the kernel */
>>  microblaze_load_kernel(>cpu, XLNX_ZYNQMP_PMU_RAM_ADDR,
>> machine->ram_size,
>>

Re: [Qemu-devel] [PATCH 02/11] smbus_eeprom: replace SMBusDeviceClass::init by DeviceClass::reset

[Eduardo Habkost]

On Tue, Jan 16, 2018 at 10:15:46AM -0300, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/i2c/smbus_eeprom.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index b13ec0fe7a..7e81ae4fe5 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -97,12 +97,11 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t 
> cmd, int n)
>  return eeprom_receive_byte(dev);
>  }
>  
> -static int smbus_eeprom_initfn(SMBusDevice *dev)
> +static void smbus_eeprom_reset(DeviceState *dev)
>  {
>  SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
>  
>  eeprom->offset = 0;
> -return 0;
>  }
>  
>  static Property smbus_eeprom_properties[] = {
> @@ -115,7 +114,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, 
> void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>  
> -sc->init = smbus_eeprom_initfn;
> +dc->reset = smbus_eeprom_reset;

Trying to track down when each of these methods is called:

sc->init (SMBusDeviceClass::init)
 -> called by i2c_slave_qdev_init() (DeviceClass:init)
   -> called by device_realize() (DeviceClass::realize)
 -> called by device_set_realized()
   -> QOM setter for "realized" property
 -> (multiple callers)

(eww, so many indirections)

dc->reset (DeviceClass::reset)
  -> called by device_reset()
-> (multiple callers)

(much better!)


It looks like this changes how the device behaves.  Is this
fixing a device emulation bug?  If not, why should the device set
offset=0 on DeviceClass::reset, and not DeviceClass::realize?


>  sc->quick_cmd = eeprom_quick_cmd;
>  sc->send_byte = eeprom_send_byte;
>  sc->receive_byte = eeprom_receive_byte;
> -- 
> 2.15.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 5/9] xlnx-pmu-iomod-intc: Add the PMU Interrupt controller

[Alistair Francis]

On Thu, Jan 18, 2018 at 1:29 PM, Philippe Mathieu-Daudé  wrote:
> On 01/18/2018 03:38 PM, Alistair Francis wrote:
>> Add the PMU IO Module Interrupt controller device.
>>
>> Signed-off-by: Alistair Francis 
>> Reviewed-by: Edgar E. Iglesias 
>> ---
>>
>>  default-configs/microblaze-softmmu.mak |   1 +
>>  include/hw/intc/xlnx-pmu-iomod-intc.h  |  58 
>>  hw/intc/xlnx-pmu-iomod-intc.c  | 554 
>> +
>
> Thanks for installing the git.orderfile :)
>
>>  hw/intc/Makefile.objs  |   1 +
>>  4 files changed, 614 insertions(+)
>>  create mode 100644 include/hw/intc/xlnx-pmu-iomod-intc.h
>>  create mode 100644 hw/intc/xlnx-pmu-iomod-intc.c
>>
>> diff --git a/default-configs/microblaze-softmmu.mak 
>> b/default-configs/microblaze-softmmu.mak
>> index ce2630818a..7fca8e4c99 100644
>> --- a/default-configs/microblaze-softmmu.mak
>> +++ b/default-configs/microblaze-softmmu.mak
>> @@ -9,3 +9,4 @@ CONFIG_XILINX_SPI=y
>>  CONFIG_XILINX_ETHLITE=y
>>  CONFIG_SSI=y
>>  CONFIG_SSI_M25P80=y
>> +CONFIG_XLNX_ZYNQMP=y
>> diff --git a/include/hw/intc/xlnx-pmu-iomod-intc.h 
>> b/include/hw/intc/xlnx-pmu-iomod-intc.h
>> new file mode 100644
>> index 00..3478d8cf6c
>> --- /dev/null
>> +++ b/include/hw/intc/xlnx-pmu-iomod-intc.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * QEMU model of Xilinx I/O Module Interrupt Controller
>> + *
>> + * Copyright (c) 2014 Xilinx Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#ifndef XLNX_PMU_IO_INTC_H
>> +#define XLNX_PMU_IO_INTC_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/register.h"
>> +
>> +#define TYPE_XLNX_PMU_IO_INTC "xlnx.pmu_io_intc"
>> +
>> +#define XLNX_PMU_IO_INTC(obj) \
>> + OBJECT_CHECK(XlnxPMUIOIntc, (obj), TYPE_XLNX_PMU_IO_INTC)
>> +
>> +/* This is R_PIT3_CONTROL + 1 */
>> +#define XlnxPMUIOIntc_R_MAX (0x78 + 1)
>
> Good, I prefer this way, rather than having all the registers exposed in
> the header.
> Can you rename this with CAPS to stay consistent with the CODING_STYLE?
>
>> +
>> +typedef struct XlnxPMUIOIntc {
>> +SysBusDevice parent_obj;
>> +MemoryRegion iomem;
>> +
>> +qemu_irq parent_irq;
>> +
>> +struct {
>> +uint32_t intr_size;
>> +uint32_t level_edge;
>> +uint32_t positive;
>> +} cfg;
>> +
>> +uint32_t irq_raw;
>> +
>> +uint32_t regs[XlnxPMUIOIntc_R_MAX];
>> +RegisterInfo regs_info[XlnxPMUIOIntc_R_MAX];
>> +} XlnxPMUIOIntc;
>> +
>> +#endif /* XLNX_PMU_IO_INTC_H */
>> diff --git a/hw/intc/xlnx-pmu-iomod-intc.c b/hw/intc/xlnx-pmu-iomod-intc.c
>> new file mode 100644
>> index 00..1ce2f4fa6b
>> --- /dev/null
>> +++ b/hw/intc/xlnx-pmu-iomod-intc.c
>> @@ -0,0 +1,554 @@
>> +/*
>> + * QEMU model of Xilinx I/O Module Interrupt Controller
>> + *
>> + * Copyright (c) 2013 Xilinx Inc
>> + * Written by Edgar E. Iglesias 
>> + * Written by Alistair Francis 
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to 
>> deal
>> + * in the Software without restriction, including without limitation the 
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS 

Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test

[Vladimir Sementsov-Ogievskiy]

18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:

17.01.2018 21:30, John Snow wrote:


On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:

Thank you for reviewing my code!


Time for the re-spin? There's pretty good pressure to get this into 2.12
and say the non-nbd workflow model is feature complete.


Yes, you are right. Hope to do it today or tomorrow.



I've rebased, applied comments from review, and even one some new fixes, 
but now I understand that it is too early for re-spin.


Now this series depends on
1. [PATCH v2 0/3] fix bitmaps migration through shared storage
  - we need to decide, how to fix it. May be, we just do not need 
bitmaps in inactive state, and should not load them in do_qcow2_open in 
this case
  [I can ignore it, just dropping one test case from new iotest, and 
fix it later, but..

2. [PATCH v2 0/6] qmp dirty bitmap API
 - here autoload is dropped, and migration should be rebased on it.

so, I'll re-spin migration after these two questions are resolved.

--
Best regards,
Vladimir

Re: [Qemu-devel] [PULL v2 00/19] machine queue, 2018-01-19

[Peter Maydell]

On 19 January 2018 at 16:33, Eduardo Habkost  wrote:
> Changes from v1 (2018-01-18):
> * Fix build failure on 32-bit
>
> The following changes since commit 3e5bdc6573edf0585e4085e6a4e349b135abf3b4:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
> (2018-01-19 10:17:20 +)
>
> are available in the Git repository at:
>
>   git://github.com/ehabkost/qemu.git tags/machine-next-pull-request
>
> for you to fetch changes up to d6b6abc51dda79a97f2c7bd6652c1940c068f1ec:
>
>   fw_cfg: fix memory corruption when all fw_cfg slots are used (2018-01-19 
> 11:18:51 -0200)
>
> 
> machine queue, 2018-01-19
>
> 

Applied, thanks.

-- PMM

Re: [Qemu-devel] [RFC] qid path collision issues in 9pfs

[Greg Kurz]

On Fri, 19 Jan 2018 17:37:58 +0100
Veaceslav Falico  wrote:

> On 1/19/2018 4:52 PM, Eduard Shishkin wrote:
> > 
> > 
> > On 1/19/2018 11:27 AM, Greg Kurz wrote:  
> >> On Mon, 15 Jan 2018 11:49:31 +0800
> >> Antonios Motakis  wrote:
> >>  
> >>> On 13-Jan-18 00:14, Greg Kurz wrote:  
>  On Fri, 12 Jan 2018 19:32:10 +0800
>  Antonios Motakis  wrote:
>   
> > Hello all,
> >  
> 
>  Hi Antonios,
> 
>  I see you have attached a patch to this email... this really isn't the 
>  preferred
>  way to do things since it prevents to comment the patch (at least with 
>  my mail
>  client). The appropriate way would have been to send the patch with a 
>  cover
>  letter, using git-send-email for example.  
> >>>
> >>> I apologize for attaching the patch, I should have known better!
> >>>  
> >>
> >> np :)
> >>  
>   
> > We have found an issue in the 9p implementation of QEMU, with how qid 
> > paths are generated, which can cause qid path collisions and several 
> > issues caused by them. In our use case (running containers under VMs) 
> > these have proven to be critical.
> >  
> 
>  Ouch...
>   
> > In particular, stat_to_qid in hw/9pfs/9p.c generates a qid path using 
> > the inode number of the file as input. According to the 9p spec the 
> > path should be able to uniquely identify a file, distinct files should 
> > not share a path value.
> >
> > The current implementation that defines qid.path = inode nr works fine 
> > as long as there are not files from multiple partitions visible under 
> > the 9p share. In that case, distinct files from different devices are 
> > allowed to have the same inode number. So with multiple partitions, we 
> > have a very high probability of qid path collisions.
> >
> > How to demonstrate the issue:
> > 1) Prepare a problematic share:
> >  - mount one partition under share/p1/ with some files inside
> >  - mount another one *with identical contents* under share/p2/
> >  - confirm that both partitions have files with same inode nr, size, etc
> > 2) Demonstrate breakage:
> >  - start a VM with a virtio-9p pointing to the share
> >  - mount 9p share with FSCACHE on
> >  - keep open share/p1/file
> >  - open and write to share/p2/file
> >
> > What should happen is, the guest will consider share/p1/file and 
> > share/p2/file to be the same file, and since we are using the cache it 
> > will not reopen it. We intended to write to partition 2, but we just 
> > wrote to partition 1. This is just one example on how the guest may 
> > rely on qid paths being unique.
> >
> > In the use case of containers where we commonly have a few containers 
> > per VM, all based on similar images, these kind of qid path collisions 
> > are very common and they seem to cause all kinds of funny behavior 
> > (sometimes very subtle).
> >
> > To avoid this situation, the device id of a file needs to be also taken 
> > as input for generating a qid path. Unfortunately, the size of both 
> > inode nr + device id together would be 96 bits, while we have only 64 
> > bits for the qid path, so we can't just append them and call it a day :(
> >
> > We have thought of a few approaches, but we would definitely like to 
> > hear what the upstream maintainers and community think:
> >
> > * Full fix: Change the 9p protocol
> >
> > We would need to support a longer qid path, based on a virtio feature 
> > flag. This would take reworking of host and guest parts of virtio-9p, 
> > so both QEMU and Linux for most users.
> >  
> 
>  I agree for a longer qid path, but we shouldn't tie it to a virtio flag 
>  since
>  9p is transport agnostic. And it happens to be used with a variety of 
>  transports.
>  QEMU has both virtio-9p and a Xen backend for example.
>   
> > * Fallback and/or interim solutions
> >
> > A virtio feature flag may be refused by the guest, so we think we still 
> > need to make collisions less likely even with 64 bit paths. E.g.  
> 
>  In all cases, we would need a fallback solution to support current
>  guest setups. Also there are several 9p server implementations out
>  there (ganesha, diod, kvmtool) that are currently used with linux
>  clients... it will take some time to get everyone in sync :-\
>   
> > 1. XOR the device id with inode nr to produce the qid path (we attach a 
> > proof of concept patch)  
> 
>  Hmm... this would still allow collisions. Not good for fallback.
>   
> > 2. 64 bit hash of device id and inode nr  
> 
>  Same here.
>   
> > 3. other ideas, such as allocating new qid paths and keep a look up 
> 

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix race between threads in page_unprotect()

[Laurent Vivier]

Le 15/01/2018 à 13:48, Peter Maydell a écrit :
> On 28 November 2017 at 14:35, Peter Maydell  wrote:
>> If multiple guest threads in user-mode emulation write to a
>> page which QEMU has marked read-only because of cached TCG
>> translations, the threads can race in page_unprotect:
> 
>> Peter Maydell (2):
>>   linux-user: Propagate siginfo_t through to handle_cpu_signal()
>>   page_unprotect(): handle calls to pages that are PAGE_WRITE
> 
> Ping! Linux-user maintainers, any chance this could get into
> a pull-request sometime soon? (I have another cleanup I'm
> thinking of that will touch the same code so I'd rather this
> went into master before I look at that.)
> 
> (I have a bunch of other pending linux-user patchsets which
> I shan't bother to ping individually unless you want me to.)
> 
> thanks
> -- PMM
> 

I've applied this series to my linux-user branch.

I've tried to find all pending linux-user patches.

If you want to check none of yours is is missing, all patches I have
collected are in:

https://github.com/vivier/qemu/commits/linux-user-for-2.12

I'm going to run some tests before sending the pull request.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 06/11] sysbus: add realize() and unrealize()

[Eduardo Habkost]

On Tue, Jan 16, 2018 at 10:15:50AM -0300, Philippe Mathieu-Daudé wrote:
[...]
> +static void sysbus_realize(DeviceState *dev, Error **errp)
> +{
> +SysBusDevice *sd = SYS_BUS_DEVICE(dev);
> +SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
> +
> +if (sbc->realize) {
> +sbc->realize(sd, errp);
> +}
> +}
> +
> +static void sysbus_unrealize(DeviceState *dev, Error **errp)
> +{
> +SysBusDevice *sd = SYS_BUS_DEVICE(dev);
> +SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
> +
> +if (sbc->unrealize) {
> +sbc->unrealize(sd, errp);
> +}
> +}

Why not just let the subclasses set DeviceClass::realize and
DeviceClass::unrealize directly?

> +
>  DeviceState *sysbus_create_varargs(const char *name,
> hwaddr addr, ...)
>  {
> @@ -325,6 +346,8 @@ static void sysbus_device_class_init(ObjectClass *klass, 
> void *data)
>  {
>  DeviceClass *k = DEVICE_CLASS(klass);
>  k->init = sysbus_device_init;
> +k->realize = sysbus_realize;
> +k->unrealize = sysbus_unrealize;
>  k->bus_type = TYPE_SYSTEM_BUS;
>  /*
>   * device_add plugs devices into a suitable bus.  For "real" buses,
> -- 
> 2.15.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v8 07/17] sdhci: add init_readonly_registers() to initialize the CAPAB register

[Alistair Francis]

On Thu, Jan 18, 2018 at 10:30 AM, Philippe Mathieu-Daudé
 wrote:
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/sd/sdhci.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8c9c0fbc2a..caf5e5a1b4 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1174,12 +1174,19 @@ static inline unsigned int 
> sdhci_get_fifolen(SDHCIState *s)
>  }
>  }
>
> +static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> +{
> +if (s->capareg == UINT64_MAX) {
> +s->capareg = SDHC_CAPAB_REG_DEFAULT;
> +}
> +}
> +
>  /* --- qdev common --- */
>
>  #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> -/* Capabilities registers provide information on supported features
> - * of this specific host controller implementation */ \
> -DEFINE_PROP_UINT64("capareg", _state, capareg, SDHC_CAPAB_REG_DEFAULT), \
> +/* deprecated: Capabilities registers provide information on supported
> + * features of this specific host controller implementation */ \
> +DEFINE_PROP_UINT64("capareg", _state, capareg, UINT64_MAX), \
>  DEFINE_PROP_UINT64("maxcurr", _state, maxcurr, 0)
>
>  static void sdhci_initfn(SDHCIState *s)
> @@ -1204,6 +1211,13 @@ static void sdhci_uninitfn(SDHCIState *s)
>
>  static void sdhci_common_realize(SDHCIState *s, Error **errp)
>  {
> +Error *local_err = NULL;
> +
> +sdhci_init_readonly_registers(s, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
>  s->buf_maxsz = sdhci_get_fifolen(s);
>  s->fifo_buffer = g_malloc0(s->buf_maxsz);
>
> --
> 2.15.1
>
>

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4...@amsat.org) wrote:
> (incorrectly use in 3be98be4e9f)
> 
> Signed-off-by: Philippe Mathieu-Daudé 

I'm a bit confused; isnt the only difference between the nocheck
versions that it'll fail at compile time instead of link?

Dave

> ---
> currently on ppc32 the linking fails:
> 
>   CC  migration/postcopy-ram.o
> ...
>   LINKmicroblaze-softmmu/qemu-system-microblaze
> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
> migration/postcopy-ram.c:717: undefined reference to `__atomic_fetch_add_8'
> migration/postcopy-ram.c:738: undefined reference to `__atomic_fetch_add_8'
> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
> migration/postcopy-ram.c:651: undefined reference to `__atomic_exchange_8'
> migration/postcopy-ram.c:652: undefined reference to `__atomic_exchange_8'
> migration/postcopy-ram.c:661: undefined reference to `__atomic_exchange_8'
> collect2: error: ld returned 1 exit status
> Makefile:193: recipe for target 'qemu-system-microblaze' failed
> make[1]: *** [qemu-system-microblaze] Error 1
> 
> with this patch the compilation fails:
> 
>   CC  migration/postcopy-ram.o
> In file included from include/qemu/osdep.h:36:0,
>  from migration/postcopy-ram.c:19:
> migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin':
> include/qemu/compiler.h:86:30: error: static assertion failed: "not 
> expecting: sizeof(*>last_begin) > ATOMIC_REG_SIZE"
>  #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
>   ^
> include/qemu/atomic.h:183:5: note: in expansion of macro 'QEMU_BUILD_BUG_ON'
>  QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);  \
>  ^
> migration/postcopy-ram.c:651:5: note: in expansion of macro 'atomic_xchg'
>  atomic_xchg(>last_begin, now_ms);
>  ^
> 
>  migration/postcopy-ram.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 7814da5b4b..6ecc1aa820 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -648,17 +648,17 @@ static void mark_postcopy_blocktime_begin(uintptr_t 
> addr, uint32_t ptid,
>  atomic_inc(>smp_cpus_down);
>  }
>  
> -atomic_xchg__nocheck(>last_begin, now_ms);
> -atomic_xchg__nocheck(>page_fault_vcpu_time[cpu], now_ms);
> -atomic_xchg__nocheck(>vcpu_addr[cpu], addr);
> +atomic_xchg(>last_begin, now_ms);
> +atomic_xchg(>page_fault_vcpu_time[cpu], now_ms);
> +atomic_xchg(>vcpu_addr[cpu], addr);
>  
>  /* check it here, not at the begining of the function,
>   * due to, check could accur early than bitmap_set in
>   * qemu_ufd_copy_ioctl */
>  already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
>  if (already_received) {
> -atomic_xchg__nocheck(>vcpu_addr[cpu], 0);
> -atomic_xchg__nocheck(>page_fault_vcpu_time[cpu], 0);
> +atomic_xchg(>vcpu_addr[cpu], 0);
> +atomic_xchg(>page_fault_vcpu_time[cpu], 0);
>  atomic_dec(>smp_cpus_down);
>  }
>  trace_mark_postcopy_blocktime_begin(addr, dc, 
> dc->page_fault_vcpu_time[cpu],
> @@ -719,7 +719,7 @@ static void mark_postcopy_blocktime_end(uintptr_t addr)
>  read_vcpu_time == 0) {
>  continue;
>  }
> -atomic_xchg__nocheck(>vcpu_addr[i], 0);
> +atomic_xchg(>vcpu_addr[i], 0);
>  vcpu_blocktime = now_ms - read_vcpu_time;
>  affected_cpu += 1;
>  /* we need to know is that mark_postcopy_end was due to
> -- 
> 2.15.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v8 01/17] sdhci: use error_propagate(local_err) in realize()

[Alistair Francis]

On Thu, Jan 18, 2018 at 10:30 AM, Philippe Mathieu-Daudé
 wrote:
> avoid the "errp && *errp" pattern (not recommended in "qapi/error.h" 
> comments).
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  hw/sd/sdhci.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index f9264d3be5..8c9c0fbc2a 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1300,10 +1300,12 @@ static Property sdhci_pci_properties[] = {
>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>  {
>  SDHCIState *s = PCI_SDHCI(dev);
> +Error *local_err = NULL;
>
>  sdhci_initfn(s);
>  sdhci_common_realize(s, errp);
> -if (errp && *errp) {
> +if (local_err) {
> +error_propagate(errp, local_err);
>  return;
>  }
>
> @@ -1381,9 +1383,11 @@ static void sdhci_sysbus_realize(DeviceState *dev, 
> Error ** errp)
>  {
>  SDHCIState *s = SYSBUS_SDHCI(dev);
>  SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +Error *local_err = NULL;
>
>  sdhci_common_realize(s, errp);
> -if (errp && *errp) {
> +if (local_err) {
> +error_propagate(errp, local_err);
>  return;
>  }
>
> --
> 2.15.1
>
>

Re: [Qemu-devel] [PATCH 01/11] smbus: add a NULL check for SMBusDeviceClass::init callbacks

[Eduardo Habkost]

On Tue, Jan 16, 2018 at 10:15:45AM -0300, Philippe Mathieu-Daudé wrote:
> ensure SMBusDeviceClass::init is set before calling it
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 8/8] qemu.py: don't launch again before shutdown()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:46AM +0100, Amador Pahim wrote:
> If a VM is launched, files are created and a cleanup is required before
> a new launch. This cleanup is executed by shutdown(), so shutdown() must
> be called even if the VM is manually terminated (i.e. using kill).
> 
> This patch creates a control to make sure launch() will not be executed
> again if shutdown() is not called after the previous launch().
> 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 0b0b61be39..862920099c 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -89,6 +89,7 @@ class QEMUMachine(object):
>  self._qemu_full_args = None
>  self._test_dir = test_dir
>  self._temp_dir = None
> +self._launched = False
>  
>  # just in case logging wasn't configured by the main script:
>  logging.basicConfig()
> @@ -210,10 +211,14 @@ class QEMUMachine(object):
>  if self.is_running():
>  raise QEMUMachineError('VM already running')
>  
> +if self._launched:
> +raise QEMUMachineError('Shutdown pending after previous launch')

I believe "VM already launched" would be clearer than "shutdown
pending". 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 7/8] qemu.py: launch vm only if it's not running

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:45AM +0100, Amador Pahim wrote:
> If we allow launching VM again we will loose track of the first launched
> VM.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 305d7bd098..0b0b61be39 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -207,6 +207,9 @@ class QEMUMachine(object):
>  Launch the VM and make sure we cleanup and expose the
>  command line/output in case of exception
>  """
> +if self.is_running():
> +raise QEMUMachineError('VM already running')

Patch 8/8 seems to make this redundant, as is_running() can't
return True if _launched is True.

I suggest replacing this and patch 8/8 with a single check:

if self._launched:
raise QEMUMachineError('VM already launched')

-- 
Eduardo

Re: [Qemu-devel] [qemu-s390x] [PATCH 12/12] Fix configure for s390 qemu on alpine

[Christian Borntraeger]

And the from email address is also wrong. (spurious x after .com)

On 01/19/2018 05:53 PM, Christian Borntraeger wrote:
> ignore the 12/12, it is just one patch
> 
> On 01/19/2018 05:42 PM, Christian Borntraeger wrote:
>> From: Alice Frosi 
>>
>> In alpine docker image the qemu-system-s390x build is broken and
>> it throws this error:
>> qemu-system-s390x: Initialization of device s390-ipl failed: could not
>> load bootloader 's390-ccw.img'
>>
>> The grep command of busybox uses regex. This fails on binary data
>> (e.g. stops on every \0), so it does not identify the string 
>> BiGeNdIaN in the test case big/little. Therefore, it assumes 
>> that the architecture is little endian.
>>
>> This fix solves the grep problem by printing the content of
>> TMPO with strings
>>
>> Signed-off-by: Alice Frosi 
>> Signed-off-by: Christian Borntraeger 
>> [some changes to patch description, add -a option to strings]
>> ---
>>  configure | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 6d8c996..383b14e 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1906,9 +1906,9 @@ int main(int argc, char *argv[]) {
>>  EOF
>>
>>  if compile_object ; then
>> -if grep -q BiGeNdIaN $TMPO ; then
>> +if strings -a $TMPO | grep -q BiGeNdIaN ; then
>>  bigendian="yes"
>> -elif grep -q LiTtLeEnDiAn $TMPO ; then
>> +elif strings -a $TMPO | grep -q LiTtLeEnDiAn ; then
>>  bigendian="no"
>>  else
>>  echo big/little test failed
>>
> 
> 

Re: [Qemu-devel] [PATCH v11 6/8] qemu.py: cleanup redundant calls in launch()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:44AM +0100, Amador Pahim wrote:
> Now that shutdown() is guaranteed to always execute self._load_io_log()
> and self._post_shutdown(), their calls in 'except' became redundant and
> we can safely replace it by a call to shutdown().
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Amador Pahim 

Reviewed-by: Eduardo Habkost 

> ---
>  scripts/qemu.py | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index d3824a7a0b..305d7bd098 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -212,11 +212,7 @@ class QEMUMachine(object):
>  try:
>  self._launch()
>  except:
> -if self.is_running():
> -self._popen.kill()
> -self._popen.wait()
> -self._load_io_log()
> -self._post_shutdown()
> +self.shutdown()
>  
>  LOG.debug('Error launching VM')
>  if self._qemu_full_args:
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 5/8] qemu.py: use poll() instead of 'returncode'

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:43AM +0100, Amador Pahim wrote:
> The 'returncode' Popen attribute is not guaranteed to be updated. It
> actually depends on a call to either poll(), wait() or communicate().
> 
> On the other hand, poll() will: "Check if child process has terminated.
> Set and return returncode attribute."
> 
> Let's use the poll() to check whether the process is running and to get
> the updated process exit code, when the process is finished.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 7bd10b1a1d..d3824a7a0b 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -149,12 +149,12 @@ class QEMUMachine(object):
>  raise
>  
>  def is_running(self):
> -return self._popen is not None and self._popen.returncode is None
> +return self._popen is not None and self._popen.poll() is None

This should be safe now, after patch 4/8.

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 4/8] qemu.py: always cleanup on shutdown()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:42AM +0100, Amador Pahim wrote:
> Currently we only cleanup on shutdown() if the VM is running.
> 
> To make sure we will always cleanup, this patch makes the
> self._load_io_log() and the self._post_shutdown() to
> always be called on shutdown(), regardless the VM running state.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 28aa3712c7..7bd10b1a1d 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -162,8 +162,9 @@ class QEMUMachine(object):
>  return self._popen.pid
>  
>  def _load_io_log(self):
> -with open(self._qemu_log_path, "r") as iolog:
> -self._iolog = iolog.read()
> +if self._qemu_log_path is not None:
> +with open(self._qemu_log_path, "r") as iolog:
> +self._iolog = iolog.read()
>  
>  def _base_args(self):
>  if isinstance(self._monitor_address, tuple):
> @@ -254,8 +255,8 @@ class QEMUMachine(object):
>  self._popen.kill()
>  self._popen.wait()
>  
> -self._load_io_log()
> -self._post_shutdown()
> +self._load_io_log()

_load_io_log() is now safe to call at any time thanks to the hunk
above.

> +self._post_shutdown()

_post_shutdown() is safe to call at any time thanks to patch 2/8.

Reviewed-by: Eduardo Habkost 

>  
>  exitcode = self.exitcode()
>  if exitcode is not None and exitcode < 0:
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 3/8] qemu.py: refactor launch()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:41AM +0100, Amador Pahim wrote:
> This is just an refactor to separate the exception handler from the
> actual launch procedure, improving the readability and making future
> maintenances in this piece of code easier.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Amador Pahim 

Reviewed-by: Eduardo Habkost 

The only thing preventing me from queueing it right now is the
dependency on patch 2/8.

> ---
>  scripts/qemu.py | 29 ++---
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index d5b1cde044..28aa3712c7 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -202,20 +202,14 @@ class QEMUMachine(object):
>  self._temp_dir = None
>  
>  def launch(self):
> -'''Launch the VM and establish a QMP connection'''
> +"""
> +Launch the VM and make sure we cleanup and expose the
> +command line/output in case of exception
> +"""
>  self._iolog = None
>  self._qemu_full_args = None
> -devnull = open(os.path.devnull, 'rb')
>  try:
> -self._pre_launch()
> -self._qemu_full_args = (self._wrapper + [self._binary] +
> -self._base_args() + self._args)
> -self._popen = subprocess.Popen(self._qemu_full_args,
> -   stdin=devnull,
> -   stdout=self._qemu_log_file,
> -   stderr=subprocess.STDOUT,
> -   shell=False)
> -self._post_launch()
> +self._launch()
>  except:
>  if self.is_running():
>  self._popen.kill()
> @@ -230,6 +224,19 @@ class QEMUMachine(object):
>  LOG.debug('Output: %r', self._iolog)
>  raise
>  
> +def _launch(self):
> +'''Launch the VM and establish a QMP connection'''
> +devnull = open(os.path.devnull, 'rb')
> +self._pre_launch()
> +self._qemu_full_args = (self._wrapper + [self._binary] +
> +self._base_args() + self._args)
> +self._popen = subprocess.Popen(self._qemu_full_args,
> +   stdin=devnull,
> +   stdout=self._qemu_log_file,
> +   stderr=subprocess.STDOUT,
> +   shell=False)
> +self._post_launch()
> +
>  def wait(self):
>  '''Wait for the VM to power off'''
>  self._popen.wait()
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] hw/char: remove legacy interface escc_init()

[Mark Cave-Ayland]

On 19/01/18 13:28, Laurent Vivier wrote:


Move necessary stuff in escc.h and update type names.
Remove slavio_serial_ms_kbd_init().
Fix code style problems reported by checkpatch.pl
Update mac_newworld, mac_oldworld and sun4m to use directly the
QDEV interface.

Signed-off-by: Laurent Vivier 
Reviewed-by: Philippe Mathieu-Daudé 
---
v2: in sun4m, move comments about Slavio TTY close to
 their qdev_prop_set_chr()


Ah okay perhaps I didn't express myself particularly well: what I was 
suggesting was moving the comment above both qdev_create()s rather than 
in the middle e.g.


  /* Slavio TTYA (base+4, Linux ttyS0) is the first QEMU serial device
 Slavio TTYB (base+0, Linux ttyS1) is the second QEMU serial device
   */
   dev = qdev_create(NULL, TYPE_ESCC);
   qdev_prop_set_uint32(dev, "disabled", !machine->enable_graphics);
   ...
   ...
   sysbus_mmio_map(s, 0, hwdef->ms_kb_base);

   dev = qdev_create(NULL, TYPE_ESCC);
   qdev_prop_set_uint32(dev, "disabled", 0);
   ...
   ...
   sysbus_mmio_map(s, 0, hwdef->serial_base);


instead of v1 which does:


   dev = qdev_create(NULL, TYPE_ESCC);
   qdev_prop_set_uint32(dev, "disabled", !machine->enable_graphics);
   ...
   ...
   sysbus_mmio_map(s, 0, hwdef->ms_kb_base);

  /* Slavio TTYA (base+4, Linux ttyS0) is the first QEMU serial device
 Slavio TTYB (base+0, Linux ttyS1) is the second QEMU serial device
   */
   dev = qdev_create(NULL, TYPE_ESCC);
   qdev_prop_set_uint32(dev, "disabled", 0);
   ...   ...
   sysbus_mmio_map(s, 0, hwdef->serial_base);



ATB,

Mark.

Re: [Qemu-devel] [PATCH v11 1/8] qemu.py: remove unused import

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:39AM +0100, Amador Pahim wrote:
> Removing 'import sys' as it's not used anywhere.
> 
> Signed-off-by: Amador Pahim 

Queued on python-next.  Thanks, and sorry for taking 2 months to
do it.

> ---
>  scripts/qemu.py | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 9bfdf6d37d..65d9ad688c 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -15,7 +15,6 @@
>  import errno
>  import logging
>  import os
> -import sys
>  import subprocess
>  import qmp.qmp
>  
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 2/8] qemu.py: better control of created files

[Eduardo Habkost]

Hi Amador,

First of all, sorry for reviewing this so late.

On Tue, Nov 14, 2017 at 11:22:40AM +0100, Amador Pahim wrote:
> To launch a VM, we need to create basically two files: the monitor
> socket (if it's a UNIX socket) and the qemu log file.
> 
> For the qemu log file, we currently just open the path, which will
> create the file if it does not exist or overwrite the file if it does
> exist.
> 
> For the monitor socket, if it already exists, we are currently removing
> it, even if it's not created by us.
> 
> This patch moves to _pre_launch() the responsibility to create a
> temporary directory to host the files so we can remove the whole
> directory on _post_shutdown().
> 
> Signed-off-by: Amador Pahim 
> ---
>  scripts/qemu.py | 32 
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 65d9ad688c..d5b1cde044 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -17,6 +17,8 @@ import logging
>  import os
>  import subprocess
>  import qmp.qmp
> +import shutil
> +import tempfile
>  
>  
>  LOG = logging.getLogger(__name__)
> @@ -72,10 +74,10 @@ class QEMUMachine(object):
>  wrapper = []
>  if name is None:
>  name = "qemu-%d" % os.getpid()
> -if monitor_address is None:
> -monitor_address = os.path.join(test_dir, name + "-monitor.sock")
> +self._name = name
>  self._monitor_address = monitor_address
> -self._qemu_log_path = os.path.join(test_dir, name + ".log")
> +self._qemu_log_path = None
> +self._qemu_log_file = None
>  self._popen = None
>  self._binary = binary
>  self._args = list(args) # Force copy args in case we modify them
> @@ -85,6 +87,8 @@ class QEMUMachine(object):
>  self._socket_scm_helper = socket_scm_helper
>  self._qmp = None
>  self._qemu_full_args = None
> +self._test_dir = test_dir
> +self._temp_dir = None
>  
>  # just in case logging wasn't configured by the main script:
>  logging.basicConfig()
> @@ -173,6 +177,13 @@ class QEMUMachine(object):
>  '-display', 'none', '-vga', 'none']
>  
>  def _pre_launch(self):
> +self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> +if not isinstance(self._monitor_address, tuple):
> +self._monitor_address = os.path.join(self._temp_dir,
> + self._name + 
> "-monitor.sock")

Won't this break QEMUMachine(..., monitor_addres='/some/unix/path')?

What about checking if self._monitor_address is None instead?

The rest looks good to me.

> +self._qemu_log_path = os.path.join(self._temp_dir, self._name + 
> ".log")
> +self._qemu_log_file = open(self._qemu_log_path, 'wb')
> +
>  self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
>  server=True)
>  
> @@ -180,23 +191,28 @@ class QEMUMachine(object):
>  self._qmp.accept()
>  
>  def _post_shutdown(self):
> -if not isinstance(self._monitor_address, tuple):
> -self._remove_if_exists(self._monitor_address)
> -self._remove_if_exists(self._qemu_log_path)
> +if self._qemu_log_file is not None:
> +self._qemu_log_file.close()
> +self._qemu_log_file = None
> +
> +self._qemu_log_path = None
> +
> +if self._temp_dir is not None:
> +shutil.rmtree(self._temp_dir)
> +self._temp_dir = None
>  
>  def launch(self):
>  '''Launch the VM and establish a QMP connection'''
>  self._iolog = None
>  self._qemu_full_args = None
>  devnull = open(os.path.devnull, 'rb')
> -qemulog = open(self._qemu_log_path, 'wb')
>  try:
>  self._pre_launch()
>  self._qemu_full_args = (self._wrapper + [self._binary] +
>  self._base_args() + self._args)
>  self._popen = subprocess.Popen(self._qemu_full_args,
> stdin=devnull,
> -   stdout=qemulog,
> +   stdout=self._qemu_log_file,
> stderr=subprocess.STDOUT,
> shell=False)
>  self._post_launch()
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC] qid path collision issues in 9pfs

[Veaceslav Falico]

On 1/19/2018 4:52 PM, Eduard Shishkin wrote:
> 
> 
> On 1/19/2018 11:27 AM, Greg Kurz wrote:
>> On Mon, 15 Jan 2018 11:49:31 +0800
>> Antonios Motakis  wrote:
>>
>>> On 13-Jan-18 00:14, Greg Kurz wrote:
 On Fri, 12 Jan 2018 19:32:10 +0800
 Antonios Motakis  wrote:

> Hello all,
>

 Hi Antonios,

 I see you have attached a patch to this email... this really isn't the 
 preferred
 way to do things since it prevents to comment the patch (at least with my 
 mail
 client). The appropriate way would have been to send the patch with a cover
 letter, using git-send-email for example.
>>>
>>> I apologize for attaching the patch, I should have known better!
>>>
>>
>> np :)
>>

> We have found an issue in the 9p implementation of QEMU, with how qid 
> paths are generated, which can cause qid path collisions and several 
> issues caused by them. In our use case (running containers under VMs) 
> these have proven to be critical.
>

 Ouch...

> In particular, stat_to_qid in hw/9pfs/9p.c generates a qid path using the 
> inode number of the file as input. According to the 9p spec the path 
> should be able to uniquely identify a file, distinct files should not 
> share a path value.
>
> The current implementation that defines qid.path = inode nr works fine as 
> long as there are not files from multiple partitions visible under the 9p 
> share. In that case, distinct files from different devices are allowed to 
> have the same inode number. So with multiple partitions, we have a very 
> high probability of qid path collisions.
>
> How to demonstrate the issue:
> 1) Prepare a problematic share:
>  - mount one partition under share/p1/ with some files inside
>  - mount another one *with identical contents* under share/p2/
>  - confirm that both partitions have files with same inode nr, size, etc
> 2) Demonstrate breakage:
>  - start a VM with a virtio-9p pointing to the share
>  - mount 9p share with FSCACHE on
>  - keep open share/p1/file
>  - open and write to share/p2/file
>
> What should happen is, the guest will consider share/p1/file and 
> share/p2/file to be the same file, and since we are using the cache it 
> will not reopen it. We intended to write to partition 2, but we just 
> wrote to partition 1. This is just one example on how the guest may rely 
> on qid paths being unique.
>
> In the use case of containers where we commonly have a few containers per 
> VM, all based on similar images, these kind of qid path collisions are 
> very common and they seem to cause all kinds of funny behavior (sometimes 
> very subtle).
>
> To avoid this situation, the device id of a file needs to be also taken 
> as input for generating a qid path. Unfortunately, the size of both inode 
> nr + device id together would be 96 bits, while we have only 64 bits for 
> the qid path, so we can't just append them and call it a day :(
>
> We have thought of a few approaches, but we would definitely like to hear 
> what the upstream maintainers and community think:
>
> * Full fix: Change the 9p protocol
>
> We would need to support a longer qid path, based on a virtio feature 
> flag. This would take reworking of host and guest parts of virtio-9p, so 
> both QEMU and Linux for most users.
>

 I agree for a longer qid path, but we shouldn't tie it to a virtio flag 
 since
 9p is transport agnostic. And it happens to be used with a variety of 
 transports.
 QEMU has both virtio-9p and a Xen backend for example.

> * Fallback and/or interim solutions
>
> A virtio feature flag may be refused by the guest, so we think we still 
> need to make collisions less likely even with 64 bit paths. E.g.

 In all cases, we would need a fallback solution to support current
 guest setups. Also there are several 9p server implementations out
 there (ganesha, diod, kvmtool) that are currently used with linux
 clients... it will take some time to get everyone in sync :-\

> 1. XOR the device id with inode nr to produce the qid path (we attach a 
> proof of concept patch)

 Hmm... this would still allow collisions. Not good for fallback.

> 2. 64 bit hash of device id and inode nr

 Same here.

> 3. other ideas, such as allocating new qid paths and keep a look up table 
> of some sorts (possibly too expensive)
>

 This would be acceptable for fallback.
>>>
>>> Maybe we can use the QEMU hash table 
>>> (https://github.com/qemu/qemu/blob/master/util/qht.c) but I wonder if it 
>>> scales to millions of entries. Do you think it is appropriate in this case?
>>>
>>
>> I don't know QHT, hence Cc'ing Emilio for 

Re: [Qemu-devel] vhost-pci and virtio-vhost-user

[Stefan Hajnoczi]

On Thu, Jan 18, 2018 at 07:51:49PM +0800, Jason Wang wrote:
> 
> 
> On 2018年01月18日 18:51, Stefan Hajnoczi wrote:
> > On Tue, Jan 16, 2018 at 01:41:37PM +0800, Jason Wang wrote:
> > > 
> > > On 2018年01月15日 21:56, Stefan Hajnoczi wrote:
> > > > On Mon, Jan 15, 2018 at 02:56:31PM +0800, Jason Wang wrote:
> > > > > On 2018年01月12日 18:18, Stefan Hajnoczi wrote:
> > > > > > > And what's more important, according to the kvm 2016 slides of 
> > > > > > > vhost-pci,
> > > > > > > the motivation of vhost-pci is not building SDN but a chain of 
> > > > > > > VNFs. So
> > > > > > > bypassing the central vswitch through a private VM2VM path does 
> > > > > > > make sense.
> > > > > > > (Though whether or not vhost-pci is the best choice is still 
> > > > > > > questionable).
> > > > > > This is probably my fault.  Maybe my networking terminology is 
> > > > > > wrong.  I
> > > > > > consider "virtual network functions" to be part of "software-defined
> > > > > > networking" use cases.  I'm not implying there must be a central 
> > > > > > virtual
> > > > > > switch.
> > > > > > 
> > > > > > To rephrase: vhost-pci enables exitless VM2VM communication.
> > > > > The problem is, exitless is not what vhost-pci invents, it could be 
> > > > > achieved
> > > > > now when both sides are doing busypolling.
> > > > The only way I'm aware of is ivshmem.  But ivshmem lacks a family of
> > > > standard device types that allows different implementations to
> > > > interoperate.  We already have the virtio family of device types, so it
> > > > makes sense to work on a virtio-based solution.
> > > > 
> > > > Perhaps I've missed a different approach for exitless VM2VM
> > > > communication.  Please explain how VM1 and VM2 can do exitless network
> > > > communication today?
> > > I'm not sure we're talking the same thing. For VM2VM, do you mean only for
> > > shared memory? I thought we can treat any backends that can transfer data
> > > directly between two VMs for a VM2VM solution. In this case, if virtqueue
> > > notifications were disabled by all sides (e.g busy polling), there will be
> > > no exits at all.
> > > 
> > > And if you want a virtio version of shared memory, it's another kind of
> > > motivation at least from my point of view.
> > I'm confused, we're probably not talking about the same thing.
> > 
> > You said that exitless "could be achieved now when both sides are doing
> > busypolling".  Can you post a QEMU command-line that does this?
> 
> If exitless means no virtqueue kick and interrupt. It does not require any
> special command line, just start a testpmd in both guest and host.
> 
> > 
> > In other words, what exactly are you proposing as an alternative to
> > vhost-pci?
> 
> I don't propose any new idea. I just want to know what's the advantage of
> vhost-pci over zerocopy. Both needs one time of copy, the difference is the
> vhost-pci did it inside a guest but zerocopy did in on host.

Exitless VM2VM communication is desirable if you cannot run software on
the host or if both endpoints are already in VMs.  In that case running
one thing in a VM and another on the host doesn't make sense.

The obvious environment where this applies is in the cloud where
everything is a VM.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 00/14] Support building with py2 or py3

[Eduardo Habkost]

On Tue, Jan 16, 2018 at 01:42:03PM +, Daniel P. Berrange wrote:
> This is an update for my previously posted series:
> 
>  v2: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06528.html
>  v3: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02978.html
>  v4: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg03150.html
> 
> This series enables some level of CI testing for py3 so that our CI jobs will
> get coverage of both py2 and py3 builds to avoid bitrot.
> 

I'm queueing this on python-next.  Thanks!



> I did a test travis build with py 3.0 and py 3.6 and got success:
> 
>   https://travis-ci.org/berrange/qemu/builds/328223261
> 
> The goal was to achieve the following
> 
>   ./configure --python=/usr/bin/python3
>   make
>   make check
> 
> This still requires passing python path to configure explicitly. A
> further improvement would be for configure to automatically detect
> a pythjon 3 binary and use it preferentially to python 2.
> 
> I have not attempted to fix/validate the block I/O tests. I would expect
> them to be broken, but easily fixable with the similar kind of scope
> changes as seen here. I felt it better to tackle that separately to
> avoid this initial series getting too large.
> 
> Although the Python 2 EOL date is 2020, we already have distros which
> are not shipping Python 2 by default (Fedora >= 26 has dropped Py2 from
> the default install). Any new releases of long life and/or enterprise
> distros may well not ship Python 2 given that it would go EOL long
> before the EOL of the distro itself. IOW QEMU does have a fairly pressing
> need to be able to support Python 3 for building.
> 
> A request for py3 is tracked here:
> 
>https://bugs.launchpad.net/qemu/+bug/1708462
> 
> If, rather than supporting py2+py3 in parallel, we wish to entirely drop
> py2 support, this series would not change significantly
> 
>  - The "from __future__ import print_function" line can be removed
>from patch 1.
>  - The code in patches 2, 3, 4 to deal with changed module names
>for a few functions can be simpified to only try the py3 location
>  - The travis + docker jobs would be fully updated to install py3,
>or delete jobs which can't support py3
> 
> Given how little code is removed should we drop py2 support, I don't
> believe it is in our immediate interests to do this. It would create
> extra pain for consumers of QEMU, with little benefit to QEMU code
> maintainance. The key thing is ensuring our travis+docker jobs provide
> satisfactory automated test coverage for the variety of python versions
> in the distros we care about targetting.
> 
> NB, Patch 11 here is not related to python 3 work - it was just a
> temporary pre-requisite of pulling in the keycodemapdb update.
> 
> Changes since v4:
> 
>  - Fix broken rebase which accidentally squashed first two
>patches together
>  - Unset LC_ALL, and set LANG + LC_CTYPE, instead of only LANG (Eric)
> 
> Changes since v3:
> 
>  - Remove space before '(' in print() function calls (Phillippe)
>  - Force use of en_US.UTF-8 for QAPI code generation (Patchew)
> 
> Changes since v2:
> 
>  - Pull in fix for keycodemapdb
>  - Enable testing with Travis
>  - Enable testing with Fedora Docker images
>  - Fix for sort ordering to fix 'make check-qapi-schema'
>  - Fix for signrom data
> 
> Daniel P. Berrange (13):
>   qapi: convert to use python print function instead of statement
>   qapi: use items()/values() intead of iteritems()/itervalues()
>   qapi: Use OrderedDict from standard library if available
>   qapi: adapt to moved location of StringIO module in py3
>   qapi: Adapt to moved location of 'maketrans' function in py3
>   qapi: remove '-q' arg to diff when comparing QAPI output
>   qapi: ensure stable sort ordering when checking QAPI entities
>   qapi: force a UTF-8 locale for running Python
>   scripts: ensure signrom treats data as bytes
>   configure: allow use of python 3
>   ui: update keycodemapdb to get py3 fixes
>   travis: improve python version test coverage
>   docker: change Fedora images to run with python3
> 
> Miika S (1):
>   input: add missing JIS keys to virtio input
> 
>  .travis.yml| 14 +++
>  Makefile   | 22 +
>  configure  |  5 ++--
>  hw/input/virtio-input-hid.c|  7 ++
>  qapi/ui.json   |  5 +++-
>  scripts/qapi.py| 43 
> --
>  scripts/qapi2texi.py   | 11 +
>  scripts/signrom.py |  4 ++--
>  tests/Makefile.include |  6 ++---
>  tests/docker/dockerfiles/fedora.docker |  3 ++-
>  tests/qapi-schema/test-qapi.py | 43 
> +-
>  ui/keycodemapdb|  2 +-
>  12 files changed, 96 insertions(+), 69 deletions(-)
> 
> -- 
> 2.14.3
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Stefan Berger]

On 01/19/2018 09:11 AM, Marc-André Lureau wrote:

tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
Interface as defined in TCG PC Client Platform TPM Profile (PTP)
Specification Family “2.0” Level 00 Revision 01.03 v22.

The PTP allows device implementation to switch between TIS and CRB
model at run time, but given that CRB is a simpler device to
implement, I chose to implement it as a different device.

The device doesn't implement other locality than 0 for now (my laptop
TPM doesn't either, so I assume this isn't so bad)

The command/reply memory region is statically allocated after the CRB
registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
wonder if the BIOS could or should allocate it instead, or what size
to use, again this seems to fit well expectations)


I removed this last sentence now. It's at the right location.



The PTP doesn't specify a particular bus to put the device. So I added
it on the system bus directly, so it could hopefully be used easily on
a different platform than x86. Currently, it fails to init on piix,
because error_on_sysbus_device() check. The check may be changed in a
near future, see discussion on the qemu-devel ML.


I think this has to be solved. So I remove these last 2 sentences. I'll 
have to wait until that other patch series from Eduard is merged since 
it doesn't start yet.


   Stefan

Re: [Qemu-devel] [PATCH V4 1/7] CAN bus simple messages transport implementation for QEMU

[Pavel Pisa]

Hello Philippe,

On Friday 19 of January 2018 13:38:11 Philippe Mathieu-Daudé wrote:
> > diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs
> > new file mode 100644
> > index 00..1028d7c455
> > --- /dev/null
> > +++ b/hw/can/Makefile.objs
> > @@ -0,0 +1,6 @@
> > +# CAN bus interfaces emulation and infrastructure
> > +
> > +ifeq ($(CONFIG_CAN_CORE),y)
> > +common-obj-y += can_core.o
>
> Please follow QEMU style:
>
> common-obj-$(CONFIG_CAN_BUS) += can_core.o

I agree that in the first patch it is not logical
to use if but problem is that final Makefile.objs
needs to resolve operating system logic and other
conditions.

ifeq ($(CONFIG_CAN_BUS),y)
common-obj-y += can_core.o
ifeq ($(CONFIG_LINUX),y)
common-obj-y += can_socketcan.o
else
common-obj-y += can_host_stub.o
endif
common-obj-$(CONFIG_CAN_SJA1000) += can_sja1000.o
common-obj-$(CONFIG_CAN_PCI) += can_kvaser_pci.o
common-obj-$(CONFIG_CAN_PCI) += can_pcm3680_pci.o
common-obj-$(CONFIG_CAN_PCI) += can_mioe3680_pci.o
endif

If there is Kconfig style system
controlling mutual options combination then plain
common-obj-$(CONFIG_*) would work but it is not
the case and I have followed seen in another QEMU 

I have followed style found in another subsystems

qemu-git/hw/smbios/Makefile.objs

ifeq ($(CONFIG_SMBIOS),y)
common-obj-y += smbios.o
common-obj-$(CONFIG_IPMI) += smbios_type_38.o
common-obj-$(call lnot,$(CONFIG_IPMI)) += smbios_type_38-stub.o
else
common-obj-y += smbios-stub.o
endif

common-obj-$(CONFIG_ALL) += smbios-stub.o
common-obj-$(CONFIG_ALL) += smbios_type_38-stub.o

qemu-git/hw/timer/Makefile.objs

...
...
qemu-git/hw/vfio/Makefile.objs

I am not sure how to resolve these conditions better way.

Best wishes,

Pavel

Re: [Qemu-devel] [PULL 00/27] Migration pull

[Peter Maydell]

On 19 January 2018 at 16:43, Alexey Perevalov  wrote:
> As I remember, I tested build in QEMU's docker build system,
> but now I checked it on i386 Ubuntu, and yes linker says about unresolved
> atomic symbols. Next week, I'll have a time to investigate it deeper.

This sounds like exactly the problem I pointed out in a previous
round of this patchset :-(

https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02103.html

Ignoring comments and sending patches anyway makes me grumpy,
especially when the result is exactly "fails obscurely on
some architectures only"...

thanks
-- PMM