Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v4 0/2] virtio: Provide version-specific variants of virtio PCI devices

[Peter Krempa]

On Tue, Mar 05, 2019 at 16:56:43 +0100, Andrea Bolognani wrote:
> On Tue, 2019-03-05 at 15:38 +0100, Gerd Hoffmann wrote:

[...]

> So I agree neither scenario is exactly perfect, but I still think
> adding non-transitional alias devices would overall be more
> user-friendly.

I don't think it makes sense to add it at the qemu level. From libvirt's
point of view users should be shielded from any qemu impl detail or
inconsistency as libvirt is the 'user friendly'[1] layer. In qemu the
devices would be the same and thus does not make sense to do that
because it would be more confusing.

You can argue that we should add the alias at the libvirt level though.

[1] Yes. I'm aware that statement is quite ironical.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 06/11] sam460ex: Don't size flash memory to match backing image

[Philippe Mathieu-Daudé]

On 3/6/19 7:51 AM, Markus Armbruster wrote:
> BALATON Zoltan  writes:
>> On Tue, 5 Mar 2019, Philippe Mathieu-Daudé wrote:
>>> On 2/26/19 8:34 PM, Markus Armbruster wrote:
 Machine "sam460ex" maps its flash memory at address 0xFFF0.  When
 no image is supplied, its size is 1MiB (0x10), and 512KiB of ROM
 get mapped on top of its second half.  Else, it's the size of the
 image rounded up to the next multiple of 64KiB.

 The rounding is actually useless: pflash_cfi01_realize() fails with
 "failed to read the initial flash content" unless it's a no-op.

 I have no idea what happens when the pflash's size exceeds 1MiB.
 Useful outcomes seem unlikely.
>>>
>>> With PFlashCFI02, it depends of the @nb_mappings parameter, which tries
>>> to emulates how the bus connects the pflash (which address lines are
>>> connected).
>>>
>>> PFlashCFI01 doesn't support the feature to remap its content in aliases
>>> (which might look unfortunate, because boards end doing it, in different
>>> ways).
>>
>> I think this is all theoretical at the moment since we don't actually
>> model the flash functions of this board (at least I haven't tested
>> that at all) and unless it somehow uses it in ways I'm unaware of I
>> think currently only the ROM is used.
>>
>>> For this device we have:
>>>
>>>  (qemu) info mtree
>>>  - (prio 0, i/o): system
>>>  0004fff0-0004 (prio 0, romd): sam460ex.flash
>>>
>>> I'm not familiar with this arch/machine, let's assume the system bus is
>>> 32bit, and the flash has a 8bit word (we have 8 data lines connected to
>>> the pflash).
>>
>> Maybe this can help:
>> https://datasheet.octopart.com/PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf
>> http://www.acube-systems.biz/index.php?page=hardware&pid=5
>>
>> Unfortunately I don't have any more detailed docs where it's explained
>> more but according to the above and in my limited understanding the
>> SoC could handle larger flash chips but this board has 512 MB. We have
>> not changed it now because I'm not sure if it would break anything and
>> I don't have time to test it so Marcus just added a comment to remind
>> about this and we're happy with that for now and could come back to it
>> separately.
> 
> And that's good enough for what I'm trying to do in this series, namely
> getting rid of unwarranted magic around pflash devices.
> 
>>> The 'no image' is 1MiB.
>>>
>>> 1 MiB = 8 Mbit
>>> 8 Mbit / 32 = 2 ^ 18
>>> We need 18 address lines to reach the whole flash.
>>>
>>> What happens if we connect a 2MiB flash? We need 19 addr lines.
>>>
>>> If we only have 18 lines to connect our flash, we can hardwire our last
>>> line as 0 or 1.
>>>
>>> - line #17 hardwired as 0:
>>> Only the bottom part of the flash is accessible (range 0x00..0x0f).
>>> CPU reading 0x4fff0 read flash offset 0x0.
>>> Using CFI it is still a announced as 2MiB.
>>>
>>> - line #17 hardwired as 1:
>>> Only the top part of the flash is accessible (range 0x10..0x1f).
>>> Can we trigger any operation from the internal state machine (writing to
>>> address 0x555, named @unlock_addr0 in QEMU) since all access are
>>> hardwardwired on top of 1MiB...?
>>> Yes we can, because the pflash only uses 11 bits for it's I/O, so all
>>> writes are masked and hit the I/O internal unit.
>>> CPU reading 0x4fff0 read flash offset 0x10
>>>
>>> If we do have 19 lines dedicated to our chip and connect a 512KiB flash,
>>> we 'll use 17 lines and let 2 lines unused.
>>> Regardless the values on the lines #17 and #18, the flash will answer to
>>> the value on lines #0..#16. This might be called MMIO aliasing, and is
>>> what setup the @nb_mappings argument.
>>> This example with nb_mappings=4 would mean:
>>> "I have a 2MiB I/O space and a 512KiB flash, map it and create 3 aliases".
> 
> Physical hardware does address lines.  Hardwiring address lines leaves
> part of the hardware unaddressable.  Not decoding address lines gets the
> same stuff mapped multiple times in the address space.  Address lines is
> also what makes sizes powers of two.

I'm amazed about how you sumarized... Thanks!

> QEMU device models are software.  Emulating address lines faithfully
> there takes extra effort.  A hack job that simply maps whatever size
> wherever is easier.  It's why we could do 7919 sectors of 5323 bytes for
> a size of 42152837 bytes, and map it at address 0x12345678.
> 
> Of course, none of our boards is that nuts.  But this one exudes a bit
> of a nutty flavor: with "-drive if=pflash,format=raw,file=1G.img", it
> happily maps 1G at address 0x4fff0.  info mtree:
> 
> address-space: memory
>   - (prio 0, i/o): system
> -1fff (prio 0, i/o): sdram-containers
>   -1fff (prio 0, i/o): alias 
> ppc4xx.sdram0 @ppc4xx.sdram -1fff
> 0004-00

Re: [Qemu-devel] Question about VM virtio device's link down delay when vhost-user reconnect

[Lilijun (Jerry, Cloud Networking)]

Thanks a lot for your advice.

Maybe there are two methods to add this option:
1) Firstly, add a vhost-user protocol feature to tell Qemu if hide the 
disconnects from the guest.  Here we just need the backend such as dpdk 
vhostuser to support this option and the feature.
2) Secondly, add a VM XML vhost-user nic configuration parameters for Qemu.  
This method need more modification and other components such as Libvirt and 
Nova in openstack to configure it.

I'd like to choose the first method,  Do you think so?   

To monitor the status of connection, we can using the command " virsh 
qemu-monitor-command vm1 --hmp info chardev " to lookup that status. Another 
one is to add new type event for Qemu to notify libvirt or other upper level 
components.

Jerry

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Tuesday, March 05, 2019 10:39 AM
> To: Lilijun (Jerry, Cloud Networking) 
> Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; Liujinsong (Paul)
> ; lixiao (H) ; wangyunjian
> ; wangxin (U)
> ; Gonglei (Arei)
> 
> Subject: Re: Question about VM virtio device's link down delay when vhost-
> user reconnect
> 
> On Mon, Mar 04, 2019 at 11:46:32AM +, Lilijun (Jerry, Cloud Networking)
> wrote:
> > Hi all,
> >
> >   I am running my VM using vhost-user NIC with OVS-DPDK.  The steps of
> my question is shown as follows:
> >  1) In the VM, I add one route entry manually on the vNIC eth0 using
> "route add default gw 192.168.1.2".
> >  2) If openvswitch service was restarted, or the process ovs-vswitchd 
> > was
> aborted, the new process may be started successfully after long seconds
> such as 40s for the initialization of DPDK huge page memory.
> >  3) And Qemu's vhost-user closed the connection and reconnected
> successfully after 40s.
> >  4) Here VM's vNIC will receive link down and up events, the interval
> between the two events is about 40s.
> >  5) Then I found that route entry disappeared unexpectedly. This will
> cause some network traffic problems.
> >
> >  I have an idea about this problem. We can add a parameter "
> link_down_delay" for all virtio devices that use vhost-user socket such as
> virtio-net and virtio-blk.
> >
> > If vhost-user socket get a connection closed event when the backend
> process was aborted or restarted, we don't notify VM virtio-net device link
> down right now.
> >When the vhost-user backend recover this socket's connections before
> the time of "link_down_delay" ms passed, we need not do that link down
> notification to VM.
> >Or else, if that's timeout, VM can be notified the link down event as
> before.
> >
> > Is there any other opinions about this solution?  Or some better ideas?
> Thanks.
> >
> > B.R.
> >
> > Jerry
> >
> 
> Rather than hardcode a specific timeout policy, I would go further and start
> with an option to just hide disconnects from guest completely.
> Instead add commands to monitor status of connection and events to report
> changes.  Management tools can then mirror connection status to link if they
> want to.
> 
> --
> MST

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 06/11] sam460ex: Don't size flash memory to match backing image

[Markus Armbruster]

BALATON Zoltan  writes:

> Hello,
>
> On Tue, 5 Mar 2019, Philippe Mathieu-Daudé wrote:
>> On 2/26/19 8:34 PM, Markus Armbruster wrote:
>>> Machine "sam460ex" maps its flash memory at address 0xFFF0.  When
>>> no image is supplied, its size is 1MiB (0x10), and 512KiB of ROM
>>> get mapped on top of its second half.  Else, it's the size of the
>>> image rounded up to the next multiple of 64KiB.
>>>
>>> The rounding is actually useless: pflash_cfi01_realize() fails with
>>> "failed to read the initial flash content" unless it's a no-op.
>>>
>>> I have no idea what happens when the pflash's size exceeds 1MiB.
>>> Useful outcomes seem unlikely.
>>
>> With PFlashCFI02, it depends of the @nb_mappings parameter, which tries
>> to emulates how the bus connects the pflash (which address lines are
>> connected).
>>
>> PFlashCFI01 doesn't support the feature to remap its content in aliases
>> (which might look unfortunate, because boards end doing it, in different
>> ways).
>
> I think this is all theoretical at the moment since we don't actually
> model the flash functions of this board (at least I haven't tested
> that at all) and unless it somehow uses it in ways I'm unaware of I
> think currently only the ROM is used.
>
>> For this device we have:
>>
>>  (qemu) info mtree
>>  - (prio 0, i/o): system
>>  0004fff0-0004 (prio 0, romd): sam460ex.flash
>>
>> I'm not familiar with this arch/machine, let's assume the system bus is
>> 32bit, and the flash has a 8bit word (we have 8 data lines connected to
>> the pflash).
>
> Maybe this can help:
> https://datasheet.octopart.com/PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf
> http://www.acube-systems.biz/index.php?page=hardware&pid=5
>
> Unfortunately I don't have any more detailed docs where it's explained
> more but according to the above and in my limited understanding the
> SoC could handle larger flash chips but this board has 512 MB. We have
> not changed it now because I'm not sure if it would break anything and
> I don't have time to test it so Marcus just added a comment to remind
> about this and we're happy with that for now and could come back to it
> separately.

And that's good enough for what I'm trying to do in this series, namely
getting rid of unwarranted magic around pflash devices.

>> The 'no image' is 1MiB.
>>
>> 1 MiB = 8 Mbit
>> 8 Mbit / 32 = 2 ^ 18
>> We need 18 address lines to reach the whole flash.
>>
>> What happens if we connect a 2MiB flash? We need 19 addr lines.
>>
>> If we only have 18 lines to connect our flash, we can hardwire our last
>> line as 0 or 1.
>>
>> - line #17 hardwired as 0:
>> Only the bottom part of the flash is accessible (range 0x00..0x0f).
>> CPU reading 0x4fff0 read flash offset 0x0.
>> Using CFI it is still a announced as 2MiB.
>>
>> - line #17 hardwired as 1:
>> Only the top part of the flash is accessible (range 0x10..0x1f).
>> Can we trigger any operation from the internal state machine (writing to
>> address 0x555, named @unlock_addr0 in QEMU) since all access are
>> hardwardwired on top of 1MiB...?
>> Yes we can, because the pflash only uses 11 bits for it's I/O, so all
>> writes are masked and hit the I/O internal unit.
>> CPU reading 0x4fff0 read flash offset 0x10
>>
>> If we do have 19 lines dedicated to our chip and connect a 512KiB flash,
>> we 'll use 17 lines and let 2 lines unused.
>> Regardless the values on the lines #17 and #18, the flash will answer to
>> the value on lines #0..#16. This might be called MMIO aliasing, and is
>> what setup the @nb_mappings argument.
>> This example with nb_mappings=4 would mean:
>> "I have a 2MiB I/O space and a 512KiB flash, map it and create 3 aliases".

Physical hardware does address lines.  Hardwiring address lines leaves
part of the hardware unaddressable.  Not decoding address lines gets the
same stuff mapped multiple times in the address space.  Address lines is
also what makes sizes powers of two.

QEMU device models are software.  Emulating address lines faithfully
there takes extra effort.  A hack job that simply maps whatever size
wherever is easier.  It's why we could do 7919 sectors of 5323 bytes for
a size of 42152837 bytes, and map it at address 0x12345678.

Of course, none of our boards is that nuts.  But this one exudes a bit
of a nutty flavor: with "-drive if=pflash,format=raw,file=1G.img", it
happily maps 1G at address 0x4fff0.  info mtree:

address-space: memory
  - (prio 0, i/o): system
-1fff (prio 0, i/o): sdram-containers
  -1fff (prio 0, i/o): alias ppc4xx.sdram0 
@ppc4xx.sdram -1fff
0004-00040003 (prio 0, ram): ppc440.l2cache_ram
0004bffd-0004bffd00ff (prio 0, i/o): ohci
0004bffd0400-0004bffd13ff (prio 0, i/o): ehci
  0004bffd0400-0004bffd

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Aleksandar Markovic]

On Tuesday, March 5, 2019, Philippe Mathieu-Daudé  wrote:

> Hi Markus,
>
> this is a rework of your 'mips_malta: Clean up definition of flash memory
> size somewhat' patch:
> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html
>
> Regards,
>
> Phil.


Philippe,

Could you summarize end-user-visible changes resulting from this series?

Aleksandar


> Based-on: <20190226193408.23862-9-arm...@redhat.com>
>
> Markus Armbruster (1):
>   mips_malta: Clean up definition of flash memory size somewhat
>
> Philippe Mathieu-Daudé (4):
>   hw/mips/malta: Fix the DEBUG_BOARD_INIT code
>   hw/mips/malta: Remove fl_sectors variable (used one single time)
>   hw/mips/malta: Restrict 'bios_size' variable scope
>   hw/mips/malta: Only accept 'monitor' pflash of 4MiB
>
>  hw/mips/mips_malta.c | 27 +--
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> --
> 2.20.1
>
>
>

Re: [Qemu-devel] [PATCH] vhost-user-test: fix leaks

[Thomas Huth]

On 05/03/2019 23.51, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  tests/vhost-user-test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 4cd0a97f13..9364227ba4 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -588,6 +588,7 @@ static void test_server_free(TestServer *server)
>  g_test_message("unable to rmdir: path (%s): %s",
> server->tmpfs, strerror(errno));
>  }
> +g_free(server->tmpfs);
>  
>  qemu_chr_fe_deinit(&server->chr, true);
>  
> @@ -605,6 +606,8 @@ static void test_server_free(TestServer *server)
>  
>  g_main_loop_unref(server->loop);
>  g_main_context_unref(server->context);
> +g_cond_clear(&server->data_cond);
> +g_mutex_clear(&server->data_mutex);
>  g_free(server);
>  }

Reviewed-by: Thomas Huth 

... and queued to my qtest-next branch (unless Michael wants to take it
through hist vhost tree instead).

Re: [Qemu-devel] [RFC PATCH 6/6] pc: Support firmware configuration with -blockdev

[Markus Armbruster]

Laszlo Ersek  writes:

> On 03/04/19 18:50, Markus Armbruster wrote:
>
>> Alright, we can call object_get_class(dev_obj)->unparent(dev_obj).
>> 
>> Final complication: if I call just that, the device's reference counter
>> goes down to zero in the middle of device_unparent(), and we use after
>> free.  So I bracket he call with object_ref() and object_unref().
>
> I don't think that requiring such a bracketing is necessarily a problem.
> I vaguely remember reviewing a kernel patch 6 or so years ago where the
> patch used the same idea, with those "get" and "put" functions (the bug
> the patch was fixing was that the last reference was "temporarily" lost
> mid-operation).

I don't regard it as problem.  My voodoo coding just wasn't prepared for
it.

> So perhaps this can be addressed, for the general case, by extending the
> documentation of device_unparent(). (The function has no documentation
> at all, at the moment.)

I know just enough to be dangerous here, not enough to write
documentation.

We really need a complete life cycle diagram for devices.  The closest
we have is the section on realization in qdev-core.h, which lets me
divine only a part of the life cycle.  The missing part I struggled with
here is how to go from device state "created, not realized" to
"destroyed".

Re: [Qemu-devel] [PULL V2 00/13] Net patches

[Jason Wang]

On 2019/3/5 下午7:21, Peter Maydell wrote:

On Tue, 5 Mar 2019 at 07:12, Jason Wang  wrote:

The following changes since commit b6179aaff961627fcb59d7b234297966b81ac726:

   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-cocoa-20190304' 
into staging (2019-03-04 16:50:41 +)

are available in the git repository at:

   https://github.com/jasowang/qemu.git tags/net-pull-request

for you to fetch changes up to 4b9b7218640a42c3ea908a12665e5840b6cd:

   tests: Add a test for qemu self announcements (2019-03-05 11:27:41 +0800)



Changes from V1:
- build fixes for qmp controlled announcing


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM



Done.

Thanks

Re: [Qemu-devel] Question about hardware cursor in VGA

[Gerd Hoffmann]

On Wed, Mar 06, 2019 at 12:48:59AM +0100, BALATON Zoltan wrote:
> On Tue, 5 Mar 2019, Gerd Hoffmann wrote:
> > On Mon, Mar 04, 2019 at 03:58:00PM +0100, BALATON Zoltan wrote:
> > > Hello,
> > > 
> > > I'm trying to implement hardware cursor for ati-vga before submitting the
> > > last version of it before the freeze.
> > 
> > Use dpy_cursor_define().
> 
> I've done that but it's not working very well. The mouse pointer is quite
> jumpy with this and there are some problems updating the pointer image when
> the guest changes image data in video ram without changing registers. (Like

When using a relative pointing device (i.e. mouse instead of tablet) you
also need dpy_mouse_set(), to update the host mouse position when the
guest moves the cursor sprite around.

Is it possible to figure where the cursor hotspot is?

> least doing hw cursor like this works somewhat but I note that the other
> version with cursor_invalidate and cursor_draw_line callbacks worked much
> smoother, alas I've found that cannot work with shared_surface.

There is a VGAComminState->force_shadow ...

> should be? (Also note that draw_line callback could do complement pixels but
> that's not possible with dpy_cursor_define.)

Yep, dpy_cursor_define is designed for the way modern hardware handles
hardware cursors, to allow it being used as hardware cursor on the host.
cirrus has a few modes which modern hardware can't do, thats why it
doesn't use dpy_cursor_define.

Has ati cursor a complement pixel mode too?

cheers,
  Gerd

[Qemu-devel] [QEMU-PPC] [PATCH] target/ppc/spapr: Enable H_PAGE_INIT in-kernel handling

[Suraj Jitindar Singh]

The H_CALL H_PAGE_INIT can be used to zero or copy a page of guest
memory. Enable the in-kernel H_PAGE_INIT handler.

The in-kernel handler takes half the time to complete compared to
handling the H_CALL in userspace.

Signed-off-by: Suraj Jitindar Singh 
---
 hw/ppc/spapr.c   | 3 +++
 target/ppc/kvm.c | 5 +
 target/ppc/kvm_ppc.h | 5 +
 3 files changed, 13 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cf1ef9ebd4..8ce97ff1e7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2822,6 +2822,9 @@ static void spapr_machine_init(MachineState *machine)
 
 /* H_CLEAR_MOD/_REF are mandatory in PAPR, but off by default */
 kvmppc_enable_clear_ref_mod_hcalls();
+
+/* Enable H_PAGE_INIT */
+kvmppc_enable_h_page_init();
 }
 
 /* allocate RAM */
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 4e3f1e4b78..d0bfb076df 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2043,6 +2043,11 @@ void kvmppc_enable_clear_ref_mod_hcalls(void)
 kvmppc_enable_hcall(kvm_state, H_CLEAR_MOD);
 }
 
+void kvmppc_enable_h_page_init(void)
+{
+kvmppc_enable_hcall(kvm_state, H_PAGE_INIT);
+}
+
 void kvmppc_set_papr(PowerPCCPU *cpu)
 {
 CPUState *cs = CPU(cpu);
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 2937b36cae..2c2ea30e87 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -23,6 +23,7 @@ int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level);
 void kvmppc_enable_logical_ci_hcalls(void);
 void kvmppc_enable_set_mode_hcall(void);
 void kvmppc_enable_clear_ref_mod_hcalls(void);
+void kvmppc_enable_h_page_init(void);
 void kvmppc_set_papr(PowerPCCPU *cpu);
 int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
 void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
@@ -138,6 +139,10 @@ static inline void kvmppc_enable_clear_ref_mod_hcalls(void)
 {
 }
 
+static inline void kvmppc_enable_h_page_init(void)
+{
+}
+
 static inline void kvmppc_set_papr(PowerPCCPU *cpu)
 {
 }
-- 
2.13.6

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

[Markus Armbruster]

Philippe Mathieu-Daudé  writes:

> On 3/5/19 6:25 PM, Peter Maydell wrote:
>> On Tue, 5 Mar 2019 at 17:21, Philippe Mathieu-Daudé  
>> wrote:
>>> But I'd recommend changing/fixing the sector size during the next dev
>>> cycle, so we have more time for testing.
>> 
>> Nobody in the upstream dev community is using or testing this board.
>
> Well I submitted a Avocado test last year:
> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02749.html
>
> And I rebase/run it from time to time.
>
>> The only way we'll find out if there's a problem with changing the
>> sector size is to put the change in and get it into a release.
>> I would vote for just making the change now.
>
> I'm happy with this vote, and am sure Markus will be too :)
>
> Markus: the last field I wasn't sure about without double checking the
> code is the @width one. I find it misleading, is that the size of the
> data bus or the size of the flash words? Answer: this is the size of the
> words in byte. NOR flash devices can not write less data than their word
> boundary.
>
> The S29PL127J60TFI130 only support 16bit words, so using @width=2 is
> correct.
>
> And the winner is ta-da!
>
> pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
>   dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> - 16 * KiB, FLASH_SIZE >> 16,
> - 1, 4, 0x, 0x, 0x, 0x,
> + 64 * KiB, FLASH_SIZE >> 16 /* will get removed
> later */,
> + 1, 2, 0x0001, 0x227e, 0x2220, 0x2200
>   0x555, 0x2aa, 0);

Sold!

[...]

Re: [Qemu-devel] [PULL 10/21] migration: Create socket-address parameter

[Markus Armbruster]

Eric Blake  writes:

> On 3/5/19 12:15 PM, Dr. David Alan Gilbert (git) wrote:
>> From: Juan Quintela 
>>
>> It will be used to store the uri parameters. We want this only for
>> tcp, so we don't set it for other uris.  We need it to know what port
>> is migration running.
>>
>> Reviewed-by: Dr. David Alan Gilbert 
>> Signed-off-by: Juan Quintela 
>>
>> --
>>
>
> This was not the usual '---' divider, and hence:
>
>> This used to be uri parameter, but it has so many troubles to
>> reproduce that it don't just make sense.
>>
>> This used to be a port parameter.  I was asked to move to
>> SocketAddress, done.
>> I also merged the setting of the migration tcp port in this one
>> because now I need to free the address, and this makes it easier.
>> This used to be x-socket-address with a single direction, now it is a
>> list of addresses.
>> Move SocketAddress_to_str here.  I used to try to generalize the one
>> in chardev/char-socket.c, but it is not worth it.
>>
>> Free string (eric)
>> Handle VSOCK address nicely (not that migration can use them yet).
>> Remove useless breaks (dave)
>> rename socket_address to socket_address_list to avoid confusion
>> Update to 4.0 (eric)
>> Put a comment indicating that there is a problem on the qapi
>> generator (markus).
>
> ...all this got included, even if it was perhaps not intended.
>
>> Message-Id: <20190227105128.1655-3-quint...@redhat.com>
>>
>> Signed-off-by: Dr. David Alan Gilbert 
>
> At any rate, it certainly looks odd to split *-by: tags by so much text.
>
>> +++ b/qapi/sockets.json
>> @@ -152,3 +152,21 @@
>>   'unix': 'UnixSocketAddress',
>>   'vsock': 'VsockSocketAddress',
>>   'fd': 'String' } }
>> +
>> +##
>> +# @DummyStruct:
>> +#
>> +# Both block-core and migration needs SocketAddressList
>> +# I am open to comments about how to share it
>> +#
>> +# @dummy-list: A dummy list
>> +#
>> +# FIXME: This shouldn't be needed, but this struct has two users, and
>> +# current qapi generator generates it on the 1st place that uses it,
>> +# so the second user don't see it.  Putting it here it is seen in both
>> +# sides.
>
> If Markus' pull request lands first, we don't need this.
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg01185.html

It did: merge commit a3e3b0a7bd5.  Please drop this hunk, either in a
respin or in a follow-up patch.

A respin would give you a chance to clean up the commit message.

Re: [Qemu-devel] [PATCH v3 0/10] qemu-binfmt-conf.sh

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190306031221.GA53@03612eec87fc/



Hi,

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

Type: series
Message-id: 20190306031221.GA53@03612eec87fc
Subject: [Qemu-devel] [PATCH v3 0/10] qemu-binfmt-conf.sh

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   patchew/20190306031221.GA53@03612eec87fc -> 
patchew/20190306031221.GA53@03612eec87fc
Switched to a new branch 'test'
ba748151ce qemu-binfmt-conf.sh: support QEMU_TARGETS
0e0dad2d82 qemu-binfmt-conf.sh: update usage()
20a92d1c39 qemu-binfmt-conf.sh: refactor usage()
e272055870 qemu-binfmt-conf.sh: add option --reset 
87a83e5953 qemu-binfmt-conf.sh: generalize  to positional 
283e2ed563 qemu-binfmt-conf.sh: honour QEMU_PATH and/or QEMU_SUFFIX
a9a99660ac qemu-binfmt-conf.sh: remove 'qemu' prefix from cli options
eb8911ad6d qemu-binfmt-conf.sh: add QEMU_CREDENTIAL and QEMU_PERSISTENT
4cfbfebe88 qemu-binfmt-conf.sh: make opts -p and -c boolean
7d848c25ae qemu-binfmt-conf.sh: enforce safe style consistency

=== OUTPUT BEGIN ===
1/10 Checking commit 7d848c25aed6 (qemu-binfmt-conf.sh: enforce safe style 
consistency)
WARNING: line over 80 characters
#52: FILE: scripts/qemu-binfmt-conf.sh:299:
+if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family" = "x" ] ; 
then

total: 0 errors, 1 warnings, 47 lines checked

Patch 1/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/10 Checking commit 4cfbfebe88f7 (qemu-binfmt-conf.sh: make opts -p and -c 
boolean)
ERROR: line over 90 characters
#50: FILE: scripts/qemu-binfmt-conf.sh:327:
+options=$(getopt -o ds:Q:S:e:hcp -l 
debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential,persistent 
-- "$@")

total: 1 errors, 0 warnings, 43 lines checked

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

3/10 Checking commit eb8911ad6dcb (qemu-binfmt-conf.sh: add QEMU_CREDENTIAL and 
QEMU_PERSISTENT)
4/10 Checking commit a9a99660ac52 (qemu-binfmt-conf.sh: remove 'qemu' prefix 
from cli options)
ERROR: line over 90 characters
#45: FILE: scripts/qemu-binfmt-conf.sh:331:
+options=$(getopt -o ds:Q:S:e:hcp -l 
debian,systemd:,path:,suffix:,exportdir:,help,credential,persistent -- "$@")

total: 1 errors, 0 warnings, 40 lines checked

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

5/10 Checking commit 283e2ed56352 (qemu-binfmt-conf.sh: honour QEMU_PATH and/or 
QEMU_SUFFIX)
6/10 Checking commit 87a83e59538a (qemu-binfmt-conf.sh: generalize  to 
positional )
WARNING: line over 80 characters
#80: FILE: scripts/qemu-binfmt-conf.sh:205:
+   Supported formats for CPUS are: single arch or comma/space separated 
list.

WARNING: line over 80 characters
#81: FILE: scripts/qemu-binfmt-conf.sh:206:
+   See QEMU target list below. If CPUS is 'ALL' or empty, configure all 
known

ERROR: line over 90 characters
#146: FILE: scripts/qemu-binfmt-conf.sh:367:
+options=$(getopt -o :dsQ:S:e:hcp -l 
debian,systemd,path:,suffix:,exportdir:,help,credential,persistent -- "$@")

total: 1 errors, 2 warnings, 144 lines checked

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

7/10 Checking commit e272055870f8 (qemu-binfmt-conf.sh: add option --reset 
)
WARNING: line over 80 characters
#71: FILE: scripts/qemu-binfmt-conf.sh:375:
+find /proc/sys/fs/binfmt_misc/ -type f -name $names -exec sh -c 'printf %s 
-1 > {}' \;

ERROR: line over 90 characters
#85: FILE: scripts/qemu-binfmt-conf.sh:390:
+options=$(getopt -o r:dsQ:S:e:hcp -l 
reset:,debian,systemd,path:,suffix:,exportdir:,help,credential,persistent -- 
"$@")

total: 1 errors, 1 warnings, 74 lines checked

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

8/10 Checking commit 20a92d1c3962 (qemu-binfmt-conf.sh: refactor usage())
9/10 Checking commit 0e0dad2d8241 (qemu-binfmt-conf.sh: update usage())
10/10 Checking commit ba748151cede (qemu-binfmt-conf.sh: support QEMU_TARGETS)
WARNING: line over 80 characters
#44: FILE: scripts/qemu-binfmt-conf.sh:205:
+Supported formats for TARGETS are: single arch or comma/space separated 
list.

WARNING: line over 80 characters
#45: FILE: scripts/qemu-binfmt-conf.sh:206:
+See QEMU target list below. If TARGETS is 'ALL' or 

[Qemu-devel] [PATCH v3 10/10] qemu-binfmt-conf.sh: support QEMU_TARGETS

[Unai Martinez-Corral]

Rename CPUS to TARGETS, and support QEMU_TARGETS environment variable.
This does not break backward compatibility, because it is just a placeholder.

Consistently with 'path', 'suffix', 'persistent' and 'credential',
provide an environment variable to set the list of target architectures.
The supported formats are the same as for positional arguments, which have
priority. If both the variable and the list of positional arguments are empty,
defaults to 'ALL'.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 8ca2647ed0..0676f86512 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -6,7 +6,7 @@ mips mipsel mipsn32 mipsn32el mips64 mips64el \
 sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb \
 microblaze microblazeel or1k x86_64"

-# check if given target CPUS is/are in the supported target list
+# check if given TARGETS is/are in the supported target list
 qemu_check_target_list() {
 all="$qemu_target_list"
 if [ "x$1" = "xALL" ] ; then
@@ -199,12 +199,12 @@ usage() {
 cat <

[Qemu-devel] [PATCH v3 9/10] qemu-binfmt-conf.sh: update usage()

[Unai Martinez-Corral]

Reduce indentation to better use available space.

Add list of supported environment variables and their default values.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 61 -
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 4a45636b53..8ca2647ed0 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -201,34 +201,31 @@ Usage: qemu-binfmt-conf.sh [--help][--path PATH][--suffix 
SUFFIX]
[--persistent][--credential][--exportdir PATH]
[--reset ARCHS][--systemd][--debian][CPUS]

-   Configure binfmt_misc to use qemu interpreter for the given CPUS.
-   Supported formats for CPUS are: single arch or comma/space separated 
list.
-   See QEMU target list below. If CPUS is 'ALL' or empty, configure all 
known
-   cpus. If CPUS is 'NONE', no interpreter is configured.
+Configure binfmt_misc to use qemu interpreter for the given CPUS.
+Supported formats for CPUS are: single arch or comma/space separated list.
+See QEMU target list below. If CPUS is 'ALL' or empty, configure all known
+cpus. If CPUS is 'NONE', no interpreter is configured.

-   --help:display this usage
-   --path:set path to qemu interpreter ($QEMU_PATH)
-   --suffix:  add a suffix to the default interpreter name
-  ($QEMU_SUFFIX)
-   --persistent:  if present, the interpreter is loaded when binfmt is
-  configured and remains in memory. All future uses
-  are cloned from the open file.
-  ($QEMU_PERSISTENT=yes)
-   --credential:  if present, credential and security tokens are
-  calculated according to the binary to interpret
-  ($QEMU_CREDENTIAL=yes)
-   --exportdir:   define where to write configuration files
-  (default: $SYSTEMDDIR or $DEBIANDIR)
-   --reset:   remove registered interpreter for target ARCHS (comma
-  separated list). If ARCHS is 'ALL', remove all registered
-  'qemu-*' interpreters.
-   --systemd: don't write into /proc,
-  instead generate file for systemd-binfmt.service;
-  environment variable HOST_ARCH allows to override 'uname'
-  to generate configuration files for a different
-  architecture than the current one.
-   --debian:  don't write into /proc,
-  instead generate update-binfmts templates
+--help:display this usage.
+--path:set path to qemu interpreter.
+--suffix:  add a suffix to the default interpreter name.
+--persistent:  if present, the interpreter is loaded when binfmt is
+   configured and remains in memory. All future uses
+   are cloned from the open file.
+--credential:  if present, credential and security tokens are
+   calculated according to the binary to interpret.
+--exportdir:   define where to write configuration files.
+   (default: $SYSTEMDDIR or $DEBIANDIR)
+--reset:   remove registered interpreter for target ARCHS (comma
+   separated list). If ARCHS is 'ALL', remove all registered
+   'qemu-*' interpreters.
+--systemd: don't write into /proc,
+   instead generate file(s) for systemd-binfmt.service;
+   environment variable HOST_ARCH allows to override 'uname'
+   to generate configuration files for a different
+   architecture than the current one.
+--debian:  don't write into /proc,
+   instead generate update-binfmts templates.

 To import templates with update-binfmts, use :

@@ -240,6 +237,14 @@ Usage: qemu-binfmt-conf.sh [--help][--path PATH][--suffix 
SUFFIX]

 QEMU target list: $qemu_target_list

+Options 'path, 'suffix', 'persistent' and 'credential' are also supported
+through environment variables. Defaults are:
+
+  QEMU_PATH=/usr/local/bin
+  QEMU_SUFFIX=
+  QEMU_PERSISTENT=no
+  QEMU_CREDENTIAL=no
+
 EOF
 }

@@ -384,8 +389,8 @@ DEBIANDIR="/usr/share/binfmts"

 QEMU_PATH="${QEMU_PATH:-/usr/local/bin}"
 QEMU_SUFFIX="${QEMU_SUFFIX:-}"
-QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}"
 QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}"
+QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}"

 options=$(getopt -o r:dsQ:S:e:hcp -l 
reset:,debian,systemd,path:,suffix:,exportdir:,help,credential,persistent -- 
"$@")
 eval set -- "$options"
--
2.20.1

[Qemu-devel] [PATCH v3 8/10] qemu-binfmt-conf.sh: refactor usage()

[Unai Martinez-Corral]

Reorder how the options are presented to the user. Move 'systemd'
and 'debian' to the end, so that the latter is close to the additional
comments and example commands about it. Make the order in the prototype
consistent with the list where each option is explained.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 824e3c4c34..4a45636b53 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -197,9 +197,9 @@ qemu_get_family() {

 usage() {
 cat <

[Qemu-devel] [PATCH v3 7/10] qemu-binfmt-conf.sh: add option --reset

[Unai Martinez-Corral]

This is a partial implementation.

Allows to remove a single or a list of already registered binfmt
interpreters. If  is a list, it must be comma-separated.
Valid values are those in qemu_target_list. If  is 'ALL', all
the existing 'qemu-*' interpreters are removed.

This is partial because 'debian' and 'systemd' configurations are not
removed. If option 'reset' is provided before any of those, reset is
executed first and the configuration proceeds. However, if 'reset' is
provided after any of them, the script will exit with error 'option
reset not implemented for this mode yet'.

Removal is done by printing '-1' as explained at:
https://www.kernel.org/doc/Documentation/admin-guide/binfmt-misc.rst

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 36 +---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 2751363089..824e3c4c34 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -197,8 +197,8 @@ qemu_get_family() {

 usage() {
 cat <&2
+usage
+exit 1
+}
+
+qemu_remove_interpreter() {
+names='qemu-*'
+if [ "x$1" != "xALL" ] ; then
+qemu_check_target_list $1
+unset names pre
+for t in $checked_target_list ; do
+names="${names}${pre}qemu-$t"
+pre=' -o -name '
+done
+fi
+find /proc/sys/fs/binfmt_misc/ -type f -name $names -exec sh -c 'printf %s 
-1 > {}' \;
+}
+
 CHECK=qemu_check_bintfmt_misc
 BINFMT_SET=qemu_register_interpreter
+BINFMT_REMOVE=qemu_remove_interpreter

 SYSTEMDDIR="/etc/binfmt.d"
 DEBIANDIR="/usr/share/binfmts"
@@ -364,19 +387,26 @@ QEMU_SUFFIX="${QEMU_SUFFIX:-}"
 QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}"
 QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}"

-options=$(getopt -o :dsQ:S:e:hcp -l 
debian,systemd,path:,suffix:,exportdir:,help,credential,persistent -- "$@")
+options=$(getopt -o r:dsQ:S:e:hcp -l 
reset:,debian,systemd,path:,suffix:,exportdir:,help,credential,persistent -- 
"$@")
 eval set -- "$options"

 while true ; do
 case "$1" in
+-r|--reset)
+shift
+$CHECK
+qemu_remove_interpreter $1
+;;
 -d|--debian)
 CHECK=qemu_check_debian
 BINFMT_SET=qemu_generate_debian
+BINFMT_REMOVE=qemu_remove_notimplemented
 EXPORTDIR=${EXPORTDIR:-$DEBIANDIR}
 ;;
 -s|--systemd)
 CHECK=qemu_check_systemd
 BINFMT_SET=qemu_generate_systemd
+BINFMT_REMOVE=qemu_remove_notimplemented
 EXPORTDIR=${EXPORTDIR:-$SYSTEMDDIR}
 ;;
 -Q|--path)
--
2.20.1

[Qemu-devel] [PATCH v3 5/10] qemu-binfmt-conf.sh: honour QEMU_PATH and/or QEMU_SUFFIX

[Unai Martinez-Corral]

Allow to set 'path' or 'suffix' through environment variables,
consistently with 'persistent' and 'credential'.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 68aa4c3f78..c113ff131e 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -176,6 +176,7 @@ Usage: qemu-binfmt-conf.sh [--path 
PATH][--debian][--systemd CPU]
--help:display this usage
--path:set path to qemu interpreter ($QEMU_PATH)
--suffix:  add a suffix to the default interpreter name
+  ($QEMU_SUFFIX)
--debian:  don't write into /proc,
   instead generate update-binfmts templates
--systemd: don't write into /proc,
@@ -321,13 +322,11 @@ BINFMT_SET=qemu_register_interpreter
 SYSTEMDDIR="/etc/binfmt.d"
 DEBIANDIR="/usr/share/binfmts"

-QEMU_PATH=/usr/local/bin
-
+QEMU_PATH="${QEMU_PATH:-/usr/local/bin}"
+QEMU_SUFFIX="${QEMU_SUFFIX:-}"
 QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}"
 QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}"

-QEMU_SUFFIX=""
-
 options=$(getopt -o ds:Q:S:e:hcp -l 
debian,systemd:,path:,suffix:,exportdir:,help,credential,persistent -- "$@")
 eval set -- "$options"

--
2.20.1

[Qemu-devel] [PATCH v3 6/10] qemu-binfmt-conf.sh: generalize to positional

[Unai Martinez-Corral]

This breaks brackward compatibility.

Option '--systemd CPU' allows to register binfmt interpreters for a
single target architecture or for 'ALL' (of them). This patch
generalizes the approach to support it in any mode (default, '--debian'
or '--systemd'). To do so, option 'systemd' is changed to be boolean
(no args). Then, all the positional arguments are considered to be a
list of target architectures.

The list can be separated by spaces, tabs, newlines or commas. If no
positional argument is provided, or when it is 'ALL', all of the
architectures in qemu_target_list are registered.

Conversely, argument value 'NONE' allows to make a 'dry run' of the
script. I.e., checks are executed according to the mode, but no
interpreter is registered.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 92 +++--
 1 file changed, 57 insertions(+), 35 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index c113ff131e..2751363089 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -6,6 +6,36 @@ mips mipsel mipsn32 mipsn32el mips64 mips64el \
 sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb \
 microblaze microblazeel or1k x86_64"

+# check if given target CPUS is/are in the supported target list
+qemu_check_target_list() {
+all="$qemu_target_list"
+if [ "x$1" = "xALL" ] ; then
+  checked_target_list="$all"
+  return
+fi
+list=""
+bIFS="$IFS"
+IFS=$"$IFS",
+for target ; do
+unknown_target="true"
+for cpu in $all ; do
+if [ "x$cpu" = "x$target" ] ; then
+list="$list $target"
+unknown_target="false"
+break
+fi
+done
+if [ "$unknown_target" = "true" ] ; then
+IFS="$bIFS"
+echo "ERROR: unknown CPU \"$target\"" 1>&2
+usage
+exit 1
+fi
+done
+IFS="$bIFS"
+checked_target_list="$list"
+}
+
 
i386_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00'
 
i386_mask='\xff\xff\xff\xff\xff\xfe\xfe\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
 i386_family=i386
@@ -167,11 +197,14 @@ qemu_get_family() {

 usage() {
 cat <&2
-usage
-exit 1
-fi
-fi
 ;;
 -Q|--path)
 shift
@@ -388,5 +408,7 @@ while true ; do
 shift
 done

+shift
+
 $CHECK
-qemu_set_binfmts
+qemu_set_binfmts "$@"
--
2.20.1

[Qemu-devel] [PATCH v3 4/10] qemu-binfmt-conf.sh: remove 'qemu' prefix from cli options

[Unai Martinez-Corral]

This breaks backward compatibility.

Options 'qemu-path' and 'qemu-suffix' have the 'qemu-' prefix, which is
not present in other option names ('debian', 'systemd', 'persistent',
'credential'...). In order to keep consistency, the prefix is removed.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index e7a714e22c..68aa4c3f78 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -167,15 +167,15 @@ qemu_get_family() {

 usage() {
 cat <

[Qemu-devel] [PATCH v3 3/10] qemu-binfmt-conf.sh: add QEMU_CREDENTIAL and QEMU_PERSISTENT

[Unai Martinez-Corral]

Allow to set options '--persistent' and/or '--credential' through
environment variables. If not defined, defaults are used ('no').
Anyway, command-line arguments have priority over environment variables.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index ca15ff8092..e7a714e22c 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -186,9 +186,11 @@ Usage: qemu-binfmt-conf.sh [--qemu-path 
PATH][--debian][--systemd CPU]
   (default: $SYSTEMDDIR or $DEBIANDIR)
--credential:  if present, credential and security tokens are
   calculated according to the binary to interpret
+  ($QEMU_CREDENTIAL=yes)
--persistent:  if present, the interpreter is loaded when binfmt is
   configured and remains in memory. All future uses
   are cloned from the open file.
+  ($QEMU_PERSISTENT=yes)

 To import templates with update-binfmts, use :

@@ -255,10 +257,10 @@ qemu_check_systemd() {

 qemu_generate_register() {
 flags=""
-if [ "x$CREDENTIAL" = "xyes" ] ; then
+if [ "x$QEMU_CREDENTIAL" = "xyes" ] ; then
 flags="OC"
 fi
-if [ "x$PERSISTENT" = "xyes" ] ; then
+if [ "x$QEMU_PERSISTENT" = "xyes" ] ; then
 flags="${flags}F"
 fi

@@ -281,7 +283,7 @@ package qemu-$cpu
 interpreter $qemu
 magic $magic
 mask $mask
-credential $CREDENTIAL
+credential $QEMU_CREDENTIAL
 EOF
 }

@@ -320,8 +322,10 @@ SYSTEMDDIR="/etc/binfmt.d"
 DEBIANDIR="/usr/share/binfmts"

 QEMU_PATH=/usr/local/bin
-CREDENTIAL=no
-PERSISTENT=no
+
+QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}"
+QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}"
+
 QEMU_SUFFIX=""

 options=$(getopt -o ds:Q:S:e:hcp -l 
debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential,persistent 
-- "$@")
@@ -373,10 +377,10 @@ while true ; do
 exit 1
 ;;
 -c|--credential)
-CREDENTIAL="yes"
+QEMU_CREDENTIAL="yes"
 ;;
 -p|--persistent)
-PERSISTENT="yes"
+QEMU_PERSISTENT="yes"
 ;;
 *)
 break
--
2.20.1

[Qemu-devel] [PATCH v3 2/10] qemu-binfmt-conf.sh: make opts -p and -c boolean

[Unai Martinez-Corral]

This patch breaks backward compatibility.

Both '--persistent' and '--credential' default to 'no'. Hence, '-p no'
or '-c no' are reduntant. Overall, accepting an argument might be
misleading because options are, indeed, boolean. This patch makes both
options boolean in getopt, so if any of them is provided the corresponding
variable is set to true.

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 0009385be2..ca15ff8092 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -168,8 +168,8 @@ qemu_get_family() {
 usage() {
 cat <

[Qemu-devel] [PATCH v3 1/10] qemu-binfmt-conf.sh: enforce safe style consistency

[Unai Martinez-Corral]

Spaces are added before '; then', for consistency.

All the tests are prefixed with 'x', in order to avoid risky comparisons
(i.e. a user deliberately trying to provoke a syntax error).

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index b5a16742a1..0009385be2 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -219,12 +219,12 @@ qemu_check_access() {

 qemu_check_bintfmt_misc() {
 # load the binfmt_misc module
-if [ ! -d /proc/sys/fs/binfmt_misc ]; then
+if [ ! -d /proc/sys/fs/binfmt_misc ] ; then
   if ! /sbin/modprobe binfmt_misc ; then
   exit 1
   fi
 fi
-if [ ! -f /proc/sys/fs/binfmt_misc/register ]; then
+if [ ! -f /proc/sys/fs/binfmt_misc/register ] ; then
   if ! mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc ; then
   exit 1
   fi
@@ -255,10 +255,10 @@ qemu_check_systemd() {

 qemu_generate_register() {
 flags=""
-if [ "$CREDENTIAL" = "yes" ] ; then
+if [ "x$CREDENTIAL" = "xyes" ] ; then
 flags="OC"
 fi
-if [ "$PERSISTENT" = "yes" ] ; then
+if [ "x$PERSISTENT" = "xyes" ] ; then
 flags="${flags}F"
 fi

@@ -296,18 +296,18 @@ qemu_set_binfmts() {
 mask=$(eval echo \$${cpu}_mask)
 family=$(eval echo \$${cpu}_family)

-if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then
+if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family" = "x" ] ; 
then
 echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2
 continue
 fi

 qemu="$QEMU_PATH/qemu-$cpu"
-if [ "$cpu" = "i486" ] ; then
+if [ "x$cpu" = "xi486" ] ; then
 qemu="$QEMU_PATH/qemu-i386"
 fi

 qemu="$qemu$QEMU_SUFFIX"
-if [ "$host_family" != "$family" ] ; then
+if [ "x$host_family" != "x$family" ] ; then
 $BINFMT_SET
 fi
 done
--
2.20.1

Re: [Qemu-devel] [PULL 30/50] spapr: Generate FDT fragment for LMBs at configure connector time

[David Gibson]

On Tue, Mar 05, 2019 at 04:10:20PM +, Peter Maydell wrote:
> On Tue, 26 Feb 2019 at 04:53, David Gibson  
> wrote:
> >
> > From: Greg Kurz 
> 
> 
> Hi -- Coverity points out a possible overflow here (CID 1399145):
> 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 00eb3b643c..b92deee771 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -,14 +,26 @@ static void spapr_nmi(NMIState *n, int cpu_index, 
> > Error **errp)
> >  }
> >  }
> >
> > +int spapr_lmb_dt_populate(sPAPRDRConnector *drc, sPAPRMachineState *spapr,
> > +  void *fdt, int *fdt_start_offset, Error **errp)
> > +{
> > +uint64_t addr;
> > +uint32_t node;
> > +
> > +addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;
> 
> This multiplication is done as a 32x32, which might overflow and
> be truncated before the result is put into the 64-bit result.
> Casting one side or the other to uint64_t would fix this.

I've applied the following fix to my tree and will include it in the
next pull request:

From 07d93b239203f4fb655e42f6a8a194a4f9eb40a2 Mon Sep 17 00:00:00 2001
From: David Gibson 
Date: Wed, 6 Mar 2019 14:15:26 +1100
Subject: [PATCH] spapr: Force SPAPR_MEMORY_BLOCK_SIZE to be a hwaddr (64-bit)

SPAPR_MEMORY_BLOCK_SIZE is logically a difference in memory addresses, and
hence of type hwaddr which is 64-bit.  Previously it wasn't marked as such
which means that it could be treated as 32-bit.  That will work in some
circumstances but if multiplied by another 32-bit value it could lead to
a 32-bit overflow and an incorrect result.

One specific instance of this in spapr_lmb_dt_populate() was spotted by
Coverity (CID 1399145).

Reported-by: Peter Maydell 
Signed-off-by: David Gibson 
---
 include/hw/ppc/spapr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ff1bd60615..1311ebe28e 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -792,7 +792,7 @@ int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t 
legacy_offset);
 
 #define TYPE_SPAPR_RNG "spapr-rng"
 
-#define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
+#define SPAPR_MEMORY_BLOCK_SIZE ((hwaddr)1 << 28) /* 256MB */
 
 /*
  * This defines the maximum number of DIMM slots we can have for sPAPR
-- 
2.20.1

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v4] spapr-rtas: add ibm, get-vpd RTAS interface

[David Gibson]

On Thu, Feb 28, 2019 at 05:04:37PM -0300, Maxiwell S. Garcia wrote:
> This adds a handler for ibm,get-vpd RTAS calls, allowing pseries guest
> to collect host information. It is disabled by default to avoid unwanted
> information leakage. To enable it, use:
> 
> ‘-M pseries,host-serial={passthrough|string},host-model={passthrough|string}’
> 
> Only the SE and TM keywords are returned at the moment.
> SE for Machine or Cabinet Serial Number and
> TM for Machine Type and Model
> 
> The SE and TM keywords are controlled by 'host-serial' and 'host-model'
> options, respectively. In the case of 'passthrough', the SE shows the
> host 'system-id' information and the TM shows the host 'model' information.
> 
> Powerpc-utils tools can dispatch RTAS calls to retrieve host
> information using this ibm,get-vpd interface. The 'host-serial'
> and 'host-model' nodes of device-tree hold the same information but
> in a static manner, which is useless after a migration operation.
> 
> Signed-off-by: Maxiwell S. Garcia 
> ---
>  hw/ppc/spapr_rtas.c| 121 +
>  include/hw/ppc/spapr.h |  17 +-
>  2 files changed, 137 insertions(+), 1 deletion(-)
> 
> Update v4:
> * Allows enable/disable host-serial and host-model options individually
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 7a2cb786a3..785c453eb6 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -287,6 +287,123 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU 
> *cpu,
>  rtas_st(rets, 0, ret);
>  }
>  
> +static inline int vpd_st(target_ulong addr, target_ulong len,
> + const void *val, uint16_t val_len)
> +{
> +hwaddr phys = ppc64_phys_to_real(addr);
> +if (len < val_len) {
> +return RTAS_OUT_PARAM_ERROR;
> +}
> +cpu_physical_memory_write(phys, val, val_len);
> +return RTAS_OUT_SUCCESS;
> +}
> +
> +static inline void vpd_ret(target_ulong rets, const int status,
> +   const int next_seq_number, const int 
> bytes_returned)
> +{
> +rtas_st(rets, 0, status);
> +rtas_st(rets, 1, next_seq_number);
> +rtas_st(rets, 2, bytes_returned);
> +}
> +
> +static void rtas_ibm_get_vpd_register_keywords(sPAPRMachineState *sm)
> +{
> +sm->rtas_vpd_keywords = g_malloc0(sizeof(uint8_t) *
> +RTAS_IBM_VPD_KEYWORDS_MAX);
> +
> +int i = 0;
> +if (sm->host_serial && !g_str_equal(sm->host_serial, "none")) {
> +sm->rtas_vpd_keywords[i++] = RTAS_IBM_VPD_KEYWORD_SE;
> +}
> +if (sm->host_model && !g_str_equal(sm->host_model, "none")) {
> +sm->rtas_vpd_keywords[i++] = RTAS_IBM_VPD_KEYWORD_TM;
> +}

Creating this "index" of keywords, then constructing each keyword's
value as we step through it seems quite complex.  It might be simpler
to just generate the whole VPD as an array of structs, indexed by
sequence number at reset time, then have get-vpd just copy that out to
the guest.

That said, the current approach does work, so I'm ok to leave it if
it's too much trouble to rework.

> +}
> +
> +static void rtas_ibm_get_vpd(PowerPCCPU *cpu,
> + sPAPRMachineState *spapr,
> + uint32_t token, uint32_t nargs,
> + target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> +target_ulong loc_code_addr;
> +target_ulong work_area_addr;
> +target_ulong work_area_size;
> +target_ulong seq_number;
> +unsigned char loc_code = 0;
> +unsigned int next_seq_number = 1;
> +int status = RTAS_IBM_GET_VPD_PARAMETER_ERROR;
> +int ret = RTAS_OUT_PARAM_ERROR;
> +char *buf = NULL;
> +char *vpd_field = NULL;
> +unsigned int vpd_field_len = 0;
> +
> +/* RTAS not authorized if no keywords have been registered */
> +if (!spapr->rtas_vpd_keywords[0]) {
> +vpd_ret(rets, RTAS_OUT_NOT_AUTHORIZED, 1, 0);
> +return;
> +}
> +
> +loc_code_addr = rtas_ld(args, 0);
> +work_area_addr = rtas_ld(args, 1);
> +work_area_size = rtas_ld(args, 2);
> +seq_number = rtas_ld(args, 3);
> +
> +/* Specific Location Code is not supported and seq_number */
> +/* must be checked to avoid out of bound index error */
> +cpu_physical_memory_read(loc_code_addr, &loc_code, 1);
> +if ((loc_code != 0) || (seq_number <= 0) ||
> +(seq_number > RTAS_IBM_VPD_KEYWORDS_MAX)) {
> +vpd_ret(rets, RTAS_IBM_GET_VPD_PARAMETER_ERROR, 1, 0);
> +return;
> +}
> +
> +switch (spapr->rtas_vpd_keywords[seq_number - 1]) {
> +case RTAS_IBM_VPD_KEYWORD_SE:
> +if (g_str_equal(spapr->host_serial, "passthrough")) {
> +/* -M host-serial=passthrough */
> +if (kvmppc_get_host_serial(&buf)) {
> +/* LoPAPR: SE for Machine or Cabinet Serial Number */
> +vpd_field = g_strdup_printf("SE %s", buf);
> +}
> +} else {

[Qemu-devel] [PATCH v3 01/10] qemu-binfmt-conf.sh: enforce safe style consistency

[Unai Martinez-Corral]

Spaces are added before '; then', for consistency.

All the tests are prefixed with 'x', in order to avoid risky comparisons
(i.e. a user deliberately trying to provoke a syntax error).

Signed-off-by: Unai Martinez-Corral 
---
 scripts/qemu-binfmt-conf.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index b5a16742a1..0009385be2 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -219,12 +219,12 @@ qemu_check_access() {

 qemu_check_bintfmt_misc() {
 # load the binfmt_misc module
-if [ ! -d /proc/sys/fs/binfmt_misc ]; then
+if [ ! -d /proc/sys/fs/binfmt_misc ] ; then
   if ! /sbin/modprobe binfmt_misc ; then
   exit 1
   fi
 fi
-if [ ! -f /proc/sys/fs/binfmt_misc/register ]; then
+if [ ! -f /proc/sys/fs/binfmt_misc/register ] ; then
   if ! mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc ; then
   exit 1
   fi
@@ -255,10 +255,10 @@ qemu_check_systemd() {

 qemu_generate_register() {
 flags=""
-if [ "$CREDENTIAL" = "yes" ] ; then
+if [ "x$CREDENTIAL" = "xyes" ] ; then
 flags="OC"
 fi
-if [ "$PERSISTENT" = "yes" ] ; then
+if [ "x$PERSISTENT" = "xyes" ] ; then
 flags="${flags}F"
 fi

@@ -296,18 +296,18 @@ qemu_set_binfmts() {
 mask=$(eval echo \$${cpu}_mask)
 family=$(eval echo \$${cpu}_family)

-if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then
+if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family" = "x" ] ; 
then
 echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2
 continue
 fi

 qemu="$QEMU_PATH/qemu-$cpu"
-if [ "$cpu" = "i486" ] ; then
+if [ "x$cpu" = "xi486" ] ; then
 qemu="$QEMU_PATH/qemu-i386"
 fi

 qemu="$qemu$QEMU_SUFFIX"
-if [ "$host_family" != "$family" ] ; then
+if [ "x$host_family" != "x$family" ] ; then
 $BINFMT_SET
 fi
 done
--
2.20.1

[Qemu-devel] [PATCH v3 0/10] qemu-binfmt-conf.sh

[Unai Martinez-Corral]

Hi,

This series reworks qemu-binfmt-conf.sh:

* Argument  from option '--systemd' is generalized to , and it is
  accepted for any mode (default, debian or systemd). It can be a single target
  arch or a list of them. Valid separators are space, tab, newline and comma.
  Keywords 'ALL' and 'NONE' are supported.
* Option '--reset ARCHS' is added, which allows to remove an already registered
  target interpreter or a list of them. Valid separator is comma. Keyword 'ALL'
  is supported. The implementation is functional but partial. Please, see
  the corresponding commit.
* Support to set options through environment variables is added: QEMU_PATH,
  QEMU_SUFFIX, QEMU_PERSISTENT, QEMU_CREDENTIAL and QEMU_TARGETS.

The following changes are not backward compatible:

* Option '--persistent' no longer requires/accepts an argument.
* Option '--credential' no longer requires/accepts an argument.
* Option '--systemd' no longer requires/accepts an argument.
* Option '--qemu-path' is renamed to '--path'.
* Option '--qemu-suffix' is renamed to '--suffix'.

The functionality of all of them is untouched. Changes are related to syntax 
only.

The main differences compared to version 2 are:

* Suggestions by Eric Blake regarding syntax and style consistency have been 
addressed.
* Changes are split in multiple patches, instead of a single one.
* Option names and support of environament variables is more consistent.

Based on:

* [PATCH v2] qemu-binfmt-conf.sh: add CPUS, add --reset, make -p and -c boolean 
(no arg)
* [PATCH] qemu-binfmt-conf.sh: add CPUS, add --reset, make -p and -c boolean 
(no arg)

Regards

Unai Martinez-Corral (10):
  qemu-binfmt-conf.sh: enforce safe style consistency
  qemu-binfmt-conf.sh: make opts -p and -c boolean
  qemu-binfmt-conf.sh: add QEMU_CREDENTIAL and QEMU_PERSISTENT
  qemu-binfmt-conf.sh: remove 'qemu' prefix from cli options
  qemu-binfmt-conf.sh: honour QEMU_PATH and/or QEMU_SUFFIX
  qemu-binfmt-conf.sh: generalize  to positional 
  qemu-binfmt-conf.sh: add option --reset 
  qemu-binfmt-conf.sh: refactor usage()
  qemu-binfmt-conf.sh: update usage()
  qemu-binfmt-conf.sh: support QEMU_TARGETS

 scripts/qemu-binfmt-conf.sh | 190
 1 file changed, 125 insertions(+), 65 deletions(-)

[Qemu-devel] [PATCH 1/3] virtio-balloon: Don't mismatch g_malloc()/free (CID 1399146)

[David Gibson]

ed48c59875b6 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host
page size" introduced a new temporary data structure which tracks 4kiB
chunks which have been inserted into the balloon by the guest but
don't yet form a full host page which we can discard.

Unfortunately, I had a thinko and allocated that structure with
g_malloc0() but freed it with a plain free() rather than g_free().
This corrects the problem.

Fixes: ed48c59875b6
Reported-by: Peter Maydell 
Signed-off-by: David Gibson 
---
 hw/virtio/virtio-balloon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index d3f2913a85..127289ae0e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -81,7 +81,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
 /* We've partially ballooned part of a host page, but now
  * we're trying to balloon part of a different one.  Too hard,
  * give up on the old partial page */
-free(balloon->pbp);
+g_free(balloon->pbp);
 balloon->pbp = NULL;
 }
 
@@ -106,7 +106,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
  * has already reported them, and failing to discard a balloon
  * page is not fatal */
 
-free(balloon->pbp);
+g_free(balloon->pbp);
 balloon->pbp = NULL;
 }
 }
-- 
2.20.1

[Qemu-devel] [PATCH 0/3] virtio-balloon: Several fixes to recent rework

[David Gibson]

A rework to the virtio-balloon driver was recently merged as
f6deb6d..ee1cd00.  This series addresses several minor issues in the
new code.

Changes since RFC:
 * Add a fix for the g_malloc/free mismatch reported by Coverity
 * Remove the not-really-useful hint-on-deflate flag

David Gibson (3):
  virtio-balloon: Don't mismatch g_malloc()/free (CID 1399146)
  virtio-balloon: Fix possible guest memory corruption with inflates &
deflates
  virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

 hw/virtio/virtio-balloon.c | 65 +++---
 1 file changed, 61 insertions(+), 4 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 2/3] virtio-balloon: Fix possible guest memory corruption with inflates & deflates

[David Gibson]

This fixes a balloon bug with a nasty consequence - potentially
corrupting guest memory - but which is extremely unlikely to be
triggered in practice.

The balloon always works in 4kiB units, but the host could have a
larger page size on certain platforms.  Since ed48c59 "virtio-balloon:
Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this
by accumulating requests to balloon 4kiB subpages until they formed a
full host page.  Since f6deb6d "virtio-balloon: Remove unnecessary
MADV_WILLNEED on deflate" we essentially ignore deflate requests.

Suppose we have a host with 8kiB pages, and one host page has subpages
A & B.  If we get this sequence of events -
inflate A
deflate A
inflate B
- the current logic will discard the whole host page.  That's
incorrect because the guest has deflated subpage A, and could have
written important data to it.

This patch fixes the problem by adjusting our state information about
partially ballooned host pages when deflate requests are received.

Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page 
size"

Signed-off-by: David Gibson 
---
 hw/virtio/virtio-balloon.c | 48 --
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 127289ae0e..7412bf8c85 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
 }
 }
 
+static void balloon_deflate_page(VirtIOBalloon *balloon,
+ MemoryRegion *mr, hwaddr offset)
+{
+void *addr = memory_region_get_ram_ptr(mr) + offset;
+RAMBlock *rb;
+size_t rb_page_size;
+ram_addr_t ram_offset, host_page_base;
+
+/* XXX is there a better way to get to the RAMBlock than via a
+ * host address? */
+rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+rb_page_size = qemu_ram_pagesize(rb);
+host_page_base = ram_offset & ~(rb_page_size - 1);
+
+if (balloon->pbp
+&& rb == balloon->pbp->rb
+&& host_page_base == balloon->pbp->base) {
+int subpages = rb_page_size / BALLOON_PAGE_SIZE;
+
+/*
+ * This means the guest has asked to discard some of the 4kiB
+ * subpages of a host page, but then changed its mind and
+ * asked to keep them after all.  It's exceedingly unlikely
+ * for a guest to do this in practice, but handle it anyway,
+ * since getting it wrong could mean discarding memory the
+ * guest is still using. */
+bitmap_clear(balloon->pbp->bitmap,
+ (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
+ subpages);
+
+if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
+g_free(balloon->pbp);
+balloon->pbp = NULL;
+}
+}
+}
+
 static const char *balloon_stat_names[] = {
[VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
[VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
@@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice 
*vdev, VirtQueue *vq)
 
 trace_virtio_balloon_handle_output(memory_region_name(section.mr),
pa);
-if (!qemu_balloon_is_inhibited() && vq != s->dvq) {
-balloon_inflate_page(s, section.mr, 
section.offset_within_region);
+if (!qemu_balloon_is_inhibited()) {
+if (vq == s->ivq) {
+balloon_inflate_page(s, section.mr,
+ section.offset_within_region);
+} else if (vq == s->dvq) {
+balloon_deflate_page(s, section.mr, 
section.offset_within_region);
+} else {
+g_assert_not_reached();
+}
 }
 memory_region_unref(section.mr);
 }
-- 
2.20.1

[Qemu-devel] [PATCH 3/3] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[David Gibson]

Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
deflate", the balloon device issued an madvise() MADV_WILLNEED on
pages removed from the balloon.  That would hint to the host kernel
that the pages were likely to be needed by the guest in the near
future.

It's unclear if this is actually valuable or not, and so f6deb6d9
removed this, essentially ignoring balloon deflate requests.  However,
concerns have been raised that this might cause a performance
regression by causing extra latency for the guest in certain
configurations.

So, until we can get actual benchmark data to see if that's the case,
this restores the old behaviour, issuing a MADV_WILLNEED when a page is
removed from the balloon.

Signed-off-by: David Gibson 
---
 hw/virtio/virtio-balloon.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 7412bf8c85..ac36988605 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -118,6 +118,8 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
 RAMBlock *rb;
 size_t rb_page_size;
 ram_addr_t ram_offset, host_page_base;
+void *host_addr;
+int ret;
 
 /* XXX is there a better way to get to the RAMBlock than via a
  * host address? */
@@ -146,6 +148,17 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
 balloon->pbp = NULL;
 }
 }
+
+host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1));
+
+/* When a page is deflated, we hint the whole host page it lives
+ * on, since we can't do anything smaller */
+ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED);
+if (ret != 0) {
+warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s",
+strerror(errno));
+/* Otherwise ignore, failing to page hint shouldn't be fatal */
+}
 }
 
 static const char *balloon_stat_names[] = {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

On Tue, Mar 05, 2019 at 07:14:09PM -0500, Michael S. Tsirkin wrote:
> On Wed, Mar 06, 2019 at 10:35:12AM +1100, David Gibson wrote:
> > On Tue, Mar 05, 2019 at 09:41:34AM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> > > > On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > > > > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > > > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > > > > When the balloon is inflated, we discard memory place in it 
> > > > > > > > using madvise()
> > > > > > > > with MADV_DONTNEED.  And when we deflate it we use 
> > > > > > > > MADV_WILLNEED, which
> > > > > > > > sounds like it makes sense but is actually unnecessary.
> > > > > > > > 
> > > > > > > > The misleadingly named MADV_DONTNEED just discards the memory 
> > > > > > > > in question,
> > > > > > > > it doesn't set any persistent state on it in-kernel; all that's 
> > > > > > > > necessary
> > > > > > > > to bring the memory back is to touch it.  MADV_WILLNEED in 
> > > > > > > > contrast
> > > > > > > > specifically says that the memory will be used soon and faults 
> > > > > > > > it in.
> > > > > > > > 
> > > > > > > > This patch simplify's the balloon operation by dropping the 
> > > > > > > > madvise()
> > > > > > > > on deflate.  This might have an impact on performance - it will 
> > > > > > > > move a
> > > > > > > > delay at deflate time until that memory is actually touched, 
> > > > > > > > which
> > > > > > > > might be more latency sensitive.  However:
> > > > > > > > 
> > > > > > > >   * Memory that's being given back to the guest by deflating the
> > > > > > > > balloon *might* be used soon, but it equally could just sit 
> > > > > > > > around
> > > > > > > > in the guest's pools until needed (or even be faulted out 
> > > > > > > > again if
> > > > > > > > the host is under memory pressure).
> > > > > > > > 
> > > > > > > >   * Usually, the timescale over which you'll be adjusting the 
> > > > > > > > balloon
> > > > > > > > is long enough that a few extra faults after deflation 
> > > > > > > > aren't
> > > > > > > > going to make a difference.
> > > > > > > > 
> > > > > > > > Signed-off-by: David Gibson 
> > > > > > > > Reviewed-by: David Hildenbrand 
> > > > > > > > Reviewed-by: Michael S. Tsirkin 
> > > > > > > 
> > > > > > > I'm having second thoughts about this. It might affect 
> > > > > > > performance but
> > > > > > > probably won't but we have no idea.  Might cause latency jitter 
> > > > > > > after
> > > > > > > deflate where it previously didn't happen.  This kind of patch 
> > > > > > > should
> > > > > > > really be accompanied by benchmarking results, not philosophy.
> > > > > > 
> > > > > > I guess I see your point, much as it's annoying to spend time
> > > > > > benchmarking a device that's basically broken by design.
> > > > > 
> > > > > Because of 4K page thing?
> > > > 
> > > > For one thing.  I believe David H has bunch of other reasons.
> > > > 
> > > > > It's an annoying bug for sure.  There were
> > > > > patches to add a feature bit to just switch to plan s/g format, but 
> > > > > they
> > > > > were abandoned. You are welcome to revive them though.
> > > > > Additionally or alternatively, we can easily add a field specifying
> > > > > page size.
> > > > 
> > > > We could, but I'm pretty disinclined to work on this when virtio-mem
> > > > is a better solution in nearly every way.
> > > 
> > > Then one way would be to just let balloon be. Make it behave same as
> > > always and don't make changes to it :)
> > 
> > I'd love to, but it is in real world use, so I think we do need to fix
> > serious bugs in it - at least if they can be fixed on one side,
> > without needing to roll out both qemu and guest changes (which adding
> > page size negotiation would require).
> 
> 
> Absolutely I'm just saying don't add optimizations in that case :)

I don't plan to.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v11 7/7] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang]

On 03/05/2019 10:50 PM, Dr. David Alan Gilbert wrote:

* Wei Wang (wei.w.w...@intel.com) wrote:

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

A notifier is registered to the migration precopy notifier chain. The
notifier calls free_page_start after the migration thread syncs the dirty
bitmap, so that the free page optimization starts to clear bits of free
pages from the bitmap. It calls the free_page_stop before the migration
thread syncs the bitmap, which is the end of the current round of ram
save. The free_page_stop is also called to stop the optimization in the
case when there is an error occurred in the process of ram saving.

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

Signed-off-by: Wei Wang 
CC: Michael S. Tsirkin 
CC: Dr. David Alan Gilbert 
CC: Juan Quintela 
CC: Peter Xu 
---
  hw/virtio/virtio-balloon.c  | 263 
  include/hw/virtio/virtio-balloon.h  |  28 ++-
  include/standard-headers/linux/virtio_balloon.h |   5 +
  3 files changed, 295 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
+if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
+config.free_page_report_cmd_id =
+   cpu_to_le32(dev->free_page_report_cmd_id);
+} else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
+config.free_page_report_cmd_id =
+   cpu_to_le32(VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID);
+} else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
+config.free_page_report_cmd_id =
+   cpu_to_le32(VIRTIO_BALLOON_FREE_PAGE_REPORT_DONE_ID);
+}

It looks like somewhere in the last 3 months the name in the kernel
changed; so I think I've fixed this correctly but please shout if it's
wrong:

 if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
 config.free_page_report_cmd_id =
cpu_to_le32(dev->free_page_report_cmd_id);
 } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
 config.free_page_report_cmd_id =
cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
 } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
 config.free_page_report_cmd_id =
cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
 }

and I've dropped the kernel header update since it's already there.



Looks good. Thanks for the update.

Best,
Wei

Re: [Qemu-devel] [PATCH v3 00/12] Enable build and install of our rST docs

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190305172139.32662-1-peter.mayd...@linaro.org/



Hi,

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

Type: series
Message-id: 20190305172139.32662-1-peter.mayd...@linaro.org
Subject: [Qemu-devel] [PATCH v3 00/12] Enable build and install of our rST docs

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/20190305172139.32662-1-peter.mayd...@linaro.org -> 
patchew/20190305172139.32662-1-peter.mayd...@linaro.org
Switched to a new branch 'test'
b37e0e545d MAINTAINERS: Add entry for Sphinx documentation infrastructure
6c63356b8d docs/conf.py: Don't hard-code QEMU version
f681007dea Makefile: Abstract out "identify the pkgversion" code
68cf6d5ae1 Makefile, configure: Support building rST documentation
902daac77e docs: Provide separate conf.py for each manual we want
fe2d476636 docs/conf.py: Disable option warnings
6379a4387d docs/conf.py: Don't include rST sources in HTML build
ecd2cccf77 docs/conf.py: Configure the 'alabaster' theme
81c5787a7f docs/conf.py: Disable unused _static directory
fd913234b4 docs: Commit initial files from sphinx-quickstart
6638912038 docs: Convert memory.txt to rst format
dc98fa5ed7 docs/cpu-hotplug.rst: Fix rST markup issues

=== OUTPUT BEGIN ===
1/12 Checking commit dc98fa5ed7bb (docs/cpu-hotplug.rst: Fix rST markup issues)
2/12 Checking commit 663891203882 (docs: Convert memory.txt to rst format)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#25: 
rename from docs/devel/memory.txt

total: 0 errors, 1 warnings, 195 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit fd913234b490 (docs: Commit initial files from 
sphinx-quickstart)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#18: 
new file mode 100644

total: 0 errors, 1 warnings, 188 lines checked

Patch 3/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/12 Checking commit 81c5787a7fa7 (docs/conf.py: Disable unused _static 
directory)
5/12 Checking commit ecd2cccf77de (docs/conf.py: Configure the 'alabaster' 
theme)
6/12 Checking commit 6379a4387dea (docs/conf.py: Don't include rST sources in 
HTML build)
7/12 Checking commit fe2d4766361e (docs/conf.py: Disable option warnings)
8/12 Checking commit 902daac77eb6 (docs: Provide separate conf.py for each 
manual we want)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#102: 
new file mode 100644

ERROR: line over 90 characters
#187: FILE: docs/interop/conf.py:15:
+html_theme_options['description'] = u'System Emulation Management and 
Interoperability Guide'

total: 1 errors, 1 warnings, 133 lines checked

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

9/12 Checking commit 68cf6d5ae1d7 (Makefile, configure: Support building rST 
documentation)
10/12 Checking commit f681007dea19 (Makefile: Abstract out "identify the 
pkgversion" code)
11/12 Checking commit 6c63356b8dde (docs/conf.py: Don't hard-code QEMU version)
12/12 Checking commit b37e0e545de1 (MAINTAINERS: Add entry for Sphinx 
documentation infrastructure)
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [Qemu-devel] [PATCH v3 00/12] Enable build and install of our rST docs

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20190305172139.32662-1-peter.mayd...@linaro.org/



Hi,

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

Type: series
Message-id: 20190305172139.32662-1-peter.mayd...@linaro.org
Subject: [Qemu-devel] [PATCH v3 00/12] Enable build and install of our rST docs

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/20190305172139.32662-1-peter.mayd...@linaro.org -> 
patchew/20190305172139.32662-1-peter.mayd...@linaro.org
Switched to a new branch 'test'
735537a7f4 MAINTAINERS: Add entry for Sphinx documentation infrastructure
4428fbad0a docs/conf.py: Don't hard-code QEMU version
89b5d0e749 Makefile: Abstract out "identify the pkgversion" code
cfe0287a58 Makefile, configure: Support building rST documentation
5c3ec2cdfa docs: Provide separate conf.py for each manual we want
d8042c9b50 docs/conf.py: Disable option warnings
4cdaf3004d docs/conf.py: Don't include rST sources in HTML build
5c61204f5c docs/conf.py: Configure the 'alabaster' theme
41dbc87c87 docs/conf.py: Disable unused _static directory
54efc64218 docs: Commit initial files from sphinx-quickstart
3dfd59aa3c docs: Convert memory.txt to rst format
e5881b5de1 docs/cpu-hotplug.rst: Fix rST markup issues

=== OUTPUT BEGIN ===
1/12 Checking commit e5881b5de1d6 (docs/cpu-hotplug.rst: Fix rST markup issues)
2/12 Checking commit 3dfd59aa3c05 (docs: Convert memory.txt to rst format)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#25: 
rename from docs/devel/memory.txt

total: 0 errors, 1 warnings, 195 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit 54efc64218da (docs: Commit initial files from 
sphinx-quickstart)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#18: 
new file mode 100644

total: 0 errors, 1 warnings, 188 lines checked

Patch 3/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/12 Checking commit 41dbc87c870b (docs/conf.py: Disable unused _static 
directory)
5/12 Checking commit 5c61204f5c44 (docs/conf.py: Configure the 'alabaster' 
theme)
6/12 Checking commit 4cdaf3004d54 (docs/conf.py: Don't include rST sources in 
HTML build)
7/12 Checking commit d8042c9b507f (docs/conf.py: Disable option warnings)
8/12 Checking commit 5c3ec2cdfab3 (docs: Provide separate conf.py for each 
manual we want)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#102: 
new file mode 100644

ERROR: line over 90 characters
#187: FILE: docs/interop/conf.py:15:
+html_theme_options['description'] = u'System Emulation Management and 
Interoperability Guide'

total: 1 errors, 1 warnings, 133 lines checked

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

9/12 Checking commit cfe0287a5826 (Makefile, configure: Support building rST 
documentation)
10/12 Checking commit 89b5d0e74995 (Makefile: Abstract out "identify the 
pkgversion" code)
11/12 Checking commit 4428fbad0abc (docs/conf.py: Don't hard-code QEMU version)
12/12 Checking commit 735537a7f4be (MAINTAINERS: Add entry for Sphinx 
documentation infrastructure)
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Richard Henderson]

On 3/5/19 8:28 AM, Philippe Mathieu-Daudé wrote:
> Markus Armbruster (1):
>   mips_malta: Clean up definition of flash memory size somewhat
> 
> Philippe Mathieu-Daudé (4):
>   hw/mips/malta: Fix the DEBUG_BOARD_INIT code
>   hw/mips/malta: Remove fl_sectors variable (used one single time)
>   hw/mips/malta: Restrict 'bios_size' variable scope
>   hw/mips/malta: Only accept 'monitor' pflash of 4MiB
> 
>  hw/mips/mips_malta.c | 27 +--
>  1 file changed, 17 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH 4/5] hw/mips/malta: Only accept 'monitor' pflash of 4MiB

[Richard Henderson]

On 3/5/19 8:28 AM, Philippe Mathieu-Daudé wrote:
> +
> +if (blk_getlength(pflash_blk) != FLASH_SIZE) {
> +error_report("Malta CoreLV card expects a bios of 4MB");
> +exit(1);
> +}

Indentation is off, somehow.  Tabs or extra spaces?


r~

Re: [Qemu-devel] [PATCH] vnc: fix update stalls

[Ying Fang]

On 2019/3/5 21:09, Gerd Hoffmann wrote:
> vnc aborts display update jobs on video mode switches and page flips.
> That can cause vnc update stalls in case an unfinished vnc job gets
> aborted.  The vnc client will never receive the requested update then.
> Fix that by copying the state from job_update back to update in that
> case.
> 
> Reports complain about stalls with two or more clients being connected
> at the same time, on some but not all connections.  I suspect it can
> also happen with a single connection, multiple connections only make
> this more much likely to happen.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1662260
> Reported-by: Ying Fang 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Ying Fang 

> ---
>  ui/vnc.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index da4a21d4ce94..2f2ab62fcf71 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -700,6 +700,12 @@ static void vnc_abort_display_jobs(VncDisplay *vd)
>  }
>  QTAILQ_FOREACH(vs, &vd->clients, next) {
>  vnc_lock_output(vs);
> +if (vs->update == VNC_STATE_UPDATE_NONE &&
> +vs->job_update != VNC_STATE_UPDATE_NONE) {
> +/* job aborted before completion */
> +vs->update = vs->job_update;
> +vs->job_update = VNC_STATE_UPDATE_NONE;
> +}
>  vs->abort = false;
>  vnc_unlock_output(vs);
>  }
> 

Re: [Qemu-devel] [PATCH v3 11/12] docs/conf.py: Don't hard-code QEMU version

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> Don't hard-code the QEMU version number into conf.py. Instead
> we either pass it to sphinx-build on the command line, or
> (if doing a standalone Sphinx run in a readthedocs.org setup)
> extract it from the VERSION file.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Alex Bennée 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-12-peter.mayd...@linaro.org
> ---
>  Makefile |  2 +-
>  docs/conf.py | 21 -
>  2 files changed, 17 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 09/12] Makefile, configure: Support building rST documentation

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> Add support to our configure and makefile machinery for building
> our rST docs into HTML files.
> 
> Building the documentation now requires that sphinx-build is
> available; this seems better than allowing half the docs to
> be built if it is not present but having half of them missing.
> (In particular it means that assuming that distros configured with
> --enable-docs they'll get a helpful error from configure telling
> them the new build dependency.)
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-10-peter.mayd...@linaro.org
> ---
>  configure  | 15 +--
>  Makefile   | 45 ++---
>  .gitignore |  1 +
>  3 files changed, 56 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 07/12] docs/conf.py: Disable option warnings

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> sphinx-build complains about using :option: to mark up option
> flags that it doesn't know about (because they were not defined
> using the "option::" directive):
> docs/pr-manager.rst:68: WARNING: unknown option: -d
> 
> Suppress these warnings. This way we get the semantic markup
> of the option flag but no cross-referencing hyperlink.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Alex Bennée 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-8-peter.mayd...@linaro.org
> ---
>  docs/conf.py | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 02/12] docs: Convert memory.txt to rst format

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> Convert the memory API documentation from plain text
> to restructured text format.
> 
> This is a very minimal conversion: all I had to change
> was to mark up the ASCII art parts as Sphinx expects
> for 'literal blocks', and fix up the bulleted lists
> (Sphinx expects no leading space before the bullet, and
> wants a blank line before after any list).
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Alex Bennée 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-3-peter.mayd...@linaro.org
> ---
>  docs/devel/{memory.txt => memory.rst} | 128 ++
>  1 file changed, 70 insertions(+), 58 deletions(-)
>  rename docs/devel/{memory.txt => memory.rst} (85%)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 03/12] docs: Commit initial files from sphinx-quickstart

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> Commit the initial Sphinx conf.py and skeleton index.rst as
> generated with sphinx-quickstart. We'll update these to
> add QEMU-specific tweaks in subsequent commits.
> 
> Signed-off-by: Peter Maydell 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-4-peter.mayd...@linaro.org
> ---
>  docs/conf.py   | 168 +
>  docs/index.rst |  20 ++
>  2 files changed, 188 insertions(+)
>  create mode 100644 docs/conf.py
>  create mode 100644 docs/index.rst

Acked-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 06/12] docs/conf.py: Don't include rST sources in HTML build

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> Sphinx defaults to including all the rST source files
> in the HTML build and making each HTML page link to the
> source file. Disable that.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Alex Bennée 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-7-peter.mayd...@linaro.org
> ---
>  docs/conf.py | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 04/12] docs/conf.py: Disable unused _static directory

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> We don't yet have any custom static files, so disable this
> config file setting to avoid a warning from sphinx about
> not being able to find the directory.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Alex Bennée 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-5-peter.mayd...@linaro.org
> ---
>  docs/conf.py | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 10/12] Makefile: Abstract out "identify the pkgversion" code

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> +  if test -d .git; then \

As mentioned somewhere else this week, test -e, since .git may be a file.

Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 12/12] MAINTAINERS: Add entry for Sphinx documentation infrastructure

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> Add a MAINTAINERS entry for Sphinx documentation infrastructure:
> this doesn't cover actual content, only the machinery we use to
> build the docs.
> 
> Signed-off-by: Peter Maydell 
> ---
>  MAINTAINERS | 6 ++
>  1 file changed, 6 insertions(+)

Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v3 01/12] docs/cpu-hotplug.rst: Fix rST markup issues

[Richard Henderson]

On 3/5/19 9:21 AM, Peter Maydell wrote:
> sphinx-build complains:
> 
> docs/cpu-hotplug.rst:67: ERROR: Unexpected indentation.
> docs/cpu-hotplug.rst:69: ERROR: Unexpected indentation.
> docs/cpu-hotplug.rst:74: WARNING: Block quote ends without a blank line; 
> unexpected unindent.
> docs/cpu-hotplug.rst:75: WARNING: Block quote ends without a blank line; 
> unexpected unindent.
> docs/cpu-hotplug.rst:76: SEVERE: Unexpected section title.
> 
> }
> {
> docs/cpu-hotplug.rst:78: WARNING: Block quote ends without a blank line; 
> unexpected unindent.
> 
> These are the result of not indicating one of the literal
> blocks by finishing the preceding paragraph with the "::" marker.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Alex Bennée 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Acked-by: Aleksandar Markovic 
> Message-id: 20190228145624.24885-2-peter.mayd...@linaro.org
> ---
>  docs/cpu-hotplug.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

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

[David Gibson]

On Mon, Mar 04, 2019 at 01:26:27PM +0300, Andrew Randrianasulu wrote:
> В сообщении от Monday 04 March 2019 05:51:27 BALATON Zoltan написал(а):
> > On Mon, 4 Mar 2019, Andrew Randrianasulu wrote:
> > > From: Amit Singh Tomar 
> > >
> > > Original commit message:
> > > This patch adds an emulation model for i2c controller found on most of 
> > > the FSL SoCs.
> > > It also integrates the RTC (ds1338) that sits on the i2c Bus with e500 
> > > machine model.
> > >
> > > Patch was originally written by Amit Singh Tomar 
> > > 
> > > see http://patchwork.ozlabs.org/patch/431475/
> > > I only fixed it enough for application on top of current qemu master
> > > 20b084c4b1401b7f8fbc385649d48c67b6f43d44, and hopefully fixed checkpatch 
> > > errors
> > >
> > > Tested by booting Linux kernel 4.20.12. Now e500 machine doesn't need
> > > network time protocol daemon because it will have working RTC
> > > (before all timestamps on files were from 2016)
> > >
> > > ---
> > >
> > > v1->v2: Expanded and fixed commit message
> > >
> > >
> > > Signed-off-by: Andrew Randrianasulu 
> > > ---
> > 
> > Almost... Patch now applies but subject and commit message are not yet 
> > right. Look at existing commit messages for examples how it should look 
> > (e.g. git log hw/ppc/e500.c). The email subject will become commit title, 
> > this should start with something showing which part you change like e500:. 
> > Then one line summary of what the patch is doing. You can probably keep 
> > original title, no need to say re-applying or things like that there. You 
> > can explain this in patch body. The text up to the first --- will be the 
> > body of the commit message so you should describe in more detail what the 
> > patch does here. Also this should include all Signed-off-by and other 
> > tags at the end before the ---.
> > 
> > Everything after --- are additional comments that won't be included in the 
> > commit message so you can put version history or any other remarks there 
> > that should not be kept after applying the patch.
> > 
> > This patch is missing Signed-off-by of the original author and has yours 
> > below --- that's why checkpatch complains. You should keep the the 
> > original Signed-off-by even if you add From: of the original author. I 
> > think you may not include From: since you're not forwarding a patch 
> > unchanged but this is now your patch based on the original since you've 
> > changed it so it can have your From: address from email header and 
> > Signed-off-by of both original author and yours to show where it came from 
> > originally. You can also mention this in commit message to make it clear.
> > 
> > Or you can keep From of original author and explain in commit message what 
> > you've changed but it still needs both Signed-off-by lines even then.
> > 
> > Hopefully this makes sense. This should already be explained in the 
> > SubmitAPatch wiki page but that can be complicated at first.
> 
> Thanks for explaining all this.
> Right now top of my patch looks like this:
> 
> From ad2b4baf8b369c8ef354e56f75ae780413acd989 Mon Sep 17 00:00:00 2001
> From: Andrew Randrianasulu 
> Date: Sun, 3 Mar 2019 00:05:04 +0300
> Subject: [PATCH v3] PPC: E500: Add FSL I2C controller and integrate RTC with 
> it
> 
> Original commit message:
> This patch adds an emulation model for i2c controller found on most of the 
> FSL SoCs.
> It also integrates the RTC (ds1338) that sits on the i2c Bus with e500 
> machine model.
> 
> Patch was originally written by Amit Singh Tomar 
> see http://patchwork.ozlabs.org/patch/431475/
> I only fixed it enough for application on top of current qemu master
> 20b084c4b1401b7f8fbc385649d48c67b6f43d44, and hopefully fixed checkpatch 
> errors
> 
> Tested by booting Linux kernel 4.20.12. Now e500 machine doesn't need.
> network time protocol daemon because it will have working RTC.
> (before all timestamps on files were from 2016)
> 
> 
> Signed-off-by: Amit Singh Tomar 
> Signed-off-by: Andrew Randrianasulu 
> ---
> 
> v1->v2: Expanded and fixed commit message
> 
> v2->v3: Changed Subject line back to original and From: field to.
> my email address, moved my SoB line above first '---' and
> added Tomar's Signed-off line back.
> 
> ---
> 
> is it OK ok to send? (assuming it applies, compiles and boots, I test this now
> with git am, make and launching qemu with -kernel option.)

That looks good, please go ahead and send.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 0/5] QEMU VFIO live migration

[Zhao Yan]

hi Alex
we still have some opens as below. could you kindly help review on that? :)

Thanks
Yan

On Mon, Feb 25, 2019 at 10:22:56AM +0800, Zhao Yan wrote:
> On Thu, Feb 21, 2019 at 01:40:51PM -0700, Alex Williamson wrote:
> > Hi Yan,
> > 
> > Thanks for working on this!
> > 
> > On Tue, 19 Feb 2019 16:50:54 +0800
> > Yan Zhao  wrote:
> > 
> > > This patchset enables VFIO devices to have live migration capability.
> > > Currently it does not support post-copy phase.
> > > 
> > > It follows Alex's comments on last version of VFIO live migration patches,
> > > including device states, VFIO device state region layout, dirty bitmap's
> > > query.
> > > 
> > > Device Data
> > > ---
> > > Device data is divided into three types: device memory, device config,
> > > and system memory dirty pages produced by device.
> > > 
> > > Device config: data like MMIOs, page tables...
> > > Every device is supposed to possess device config data.
> > >   Usually device config's size is small (no big than 10M), and it
> > 
> > I'm not sure how we can really impose a limit here, it is what it is
> > for a device.  A smaller state is obviously desirable to reduce
> > downtime, but some devices could have very large states.
> > 
> > > needs to be loaded in certain strict order.
> > > Therefore, device config only needs to be saved/loaded in
> > > stop-and-copy phase.
> > > The data of device config is held in device config region.
> > > Size of device config data is smaller than or equal to that of
> > > device config region.
> > 
> > So the intention here is that this is the last data read from the
> > device and it's done in one pass, so the region needs to be large
> > enough to expose all config data at once.  On restore it's the last
> > data written before switching the device to the run state.
> > 
> > > 
> > > Device Memory: device's internal memory, standalone and outside system
> > 
> > s/system/VM/
> > 
> > > memory. It is usually very big.
> > 
> > Or it doesn't exist.  Not sure we should be setting expectations since
> > it will vary per device.
> > 
> > > This kind of data needs to be saved / loaded in pre-copy and
> > > stop-and-copy phase.
> > > The data of device memory is held in device memory region.
> > > Size of devie memory is usually larger than that of device
> > > memory region. qemu needs to save/load it in chunks of size of
> > > device memory region.
> > > Not all device has device memory. Like IGD only uses system 
> > > memory.
> > 
> > It seems a little gratuitous to me that this is a separate region or
> > that this data is handled separately.  All of this data is opaque to
> > QEMU, so why do we need to separate it?
> hi Alex,
> as the device state interfaces are provided by kernel, it is expected to
> meet as general needs as possible. So, do you think there are such use
> cases from user space that user space knows well of the device, and
> it wants kernel to return desired data back to it.
> E.g. It just wants to get whole device config data including all mmios,
> page tables, pci config data...
> or, It just wants to get current device memory snapshot, not including any
> dirty data.
> Or, It just needs the dirty pages in device memory or system memory.
> With all this accurate query, quite a lot of useful features can be
> developped in user space.
> 
> If all of this data is opaque to user app, seems the only use case is
> for live migration.
> 
> From another aspect, if the final solution is to let the data opaque to
> user space, like what NV did, kernel side's implementation will be more
> complicated, and actually a little challenge to vendor driver.
> in that case, in pre-copy phase,
> 1. in not LOGGING state, vendor driver first returns full data including
> full device memory snapshot
> 2. user space reads some data (you can't expect it to finish reading all
> data)
> 3. then userspace set the device state to LOGGING to start dirty data
> logging
> 4. vendor driver starts dirty data logging, and appends the dirty data to
> the tail of remaining unread full data and increase the pending data size?
> 5. user space keeps reading data.
> 6. vendor driver keeps appending new dirty data to the tail of remaining
> unread full data/dirty data and increase the pending data size?
> 
> in stop-and-copy phase
> 1. user space sets device state to exit LOGGING state,
> 2. vendor driver stops data logging. it has to append device config
>data at the tail of remaining dirty data unread by userspace.
> 
> during this flow, when vendor driver should get dirty data? just keeps
> logging and appends to tail? how to ensure dirty data is refresh new before
> LOGGING state exit? how does vendor driver know whether certain dirty data
> is copied or not?
> 
> I've no idea how NVidia handle this problem, and they don't open their
> kernel side code. 
> just feel it's a bit 

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[Michael S. Tsirkin]

On Wed, Mar 06, 2019 at 10:35:12AM +1100, David Gibson wrote:
> On Tue, Mar 05, 2019 at 09:41:34AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> > > On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > > > When the balloon is inflated, we discard memory place in it using 
> > > > > > > madvise()
> > > > > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, 
> > > > > > > which
> > > > > > > sounds like it makes sense but is actually unnecessary.
> > > > > > > 
> > > > > > > The misleadingly named MADV_DONTNEED just discards the memory in 
> > > > > > > question,
> > > > > > > it doesn't set any persistent state on it in-kernel; all that's 
> > > > > > > necessary
> > > > > > > to bring the memory back is to touch it.  MADV_WILLNEED in 
> > > > > > > contrast
> > > > > > > specifically says that the memory will be used soon and faults it 
> > > > > > > in.
> > > > > > > 
> > > > > > > This patch simplify's the balloon operation by dropping the 
> > > > > > > madvise()
> > > > > > > on deflate.  This might have an impact on performance - it will 
> > > > > > > move a
> > > > > > > delay at deflate time until that memory is actually touched, which
> > > > > > > might be more latency sensitive.  However:
> > > > > > > 
> > > > > > >   * Memory that's being given back to the guest by deflating the
> > > > > > > balloon *might* be used soon, but it equally could just sit 
> > > > > > > around
> > > > > > > in the guest's pools until needed (or even be faulted out 
> > > > > > > again if
> > > > > > > the host is under memory pressure).
> > > > > > > 
> > > > > > >   * Usually, the timescale over which you'll be adjusting the 
> > > > > > > balloon
> > > > > > > is long enough that a few extra faults after deflation aren't
> > > > > > > going to make a difference.
> > > > > > > 
> > > > > > > Signed-off-by: David Gibson 
> > > > > > > Reviewed-by: David Hildenbrand 
> > > > > > > Reviewed-by: Michael S. Tsirkin 
> > > > > > 
> > > > > > I'm having second thoughts about this. It might affect performance 
> > > > > > but
> > > > > > probably won't but we have no idea.  Might cause latency jitter 
> > > > > > after
> > > > > > deflate where it previously didn't happen.  This kind of patch 
> > > > > > should
> > > > > > really be accompanied by benchmarking results, not philosophy.
> > > > > 
> > > > > I guess I see your point, much as it's annoying to spend time
> > > > > benchmarking a device that's basically broken by design.
> > > > 
> > > > Because of 4K page thing?
> > > 
> > > For one thing.  I believe David H has bunch of other reasons.
> > > 
> > > > It's an annoying bug for sure.  There were
> > > > patches to add a feature bit to just switch to plan s/g format, but they
> > > > were abandoned. You are welcome to revive them though.
> > > > Additionally or alternatively, we can easily add a field specifying
> > > > page size.
> > > 
> > > We could, but I'm pretty disinclined to work on this when virtio-mem
> > > is a better solution in nearly every way.
> > 
> > Then one way would be to just let balloon be. Make it behave same as
> > always and don't make changes to it :)
> 
> I'd love to, but it is in real world use, so I think we do need to fix
> serious bugs in it - at least if they can be fixed on one side,
> without needing to roll out both qemu and guest changes (which adding
> page size negotiation would require).


Absolutely I'm just saying don't add optimizations in that case :)

> > 
> > > > > That said.. I don't really know how I'd go about benchmarking it.  Any
> > > > > guesses at a suitable workload which would be most likely to show a
> > > > > performance degradation here?
> > > > 
> > > > Here's one idea - I tried to come up with a worst case scenario here.
> > > > Basically based on idea by Alex Duyck. All credits are his, all bugs are
> > > > mine:
> > > 
> > > Ok.  I'll try to find time to implement this and test it.
> > > 
> > > > Setup:
> > > > Memory-15837 MB
> > > > Guest Memory Size-5 GB
> > > > Swap-Disabled
> > > > Test Program-Simple program which allocates 4GB memory via malloc, 
> > > > touches it via memset and exits.
> > > > Use case-Number of guests that can be launched completely including the 
> > > > successful execution of the test program.
> > > > Procedure:
> > > > Setup:
> > > > A first guest is launched and once its console is up,
> > > > test allocation program is executed with 4 GB memory request (Due to
> > > > this the guest occupies almost 4-5 GB of memory in the host)
> > > > Afterwards balloon is inflated by 4Gbyte in the guest.
> > > > We continue launching the guests until a guest gets
> > > > killed due to low memory cond

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[John Snow]

On 3/5/19 6:43 PM, John Snow wrote:

++ This series relies on [Qemu-devel] [PATCH v3 0/7] bitmaps: add
inconsistent bit, you can apply it on top of those patches, or you can
use my github staging branch as a basis:

https://github.com/jnsnow/qemu/tree/bitmaps

> This series aims to enable block resizes when persistent bitmaps are
> in use. The basic approach here is to recognize that we now load all
> persistent bitmaps in memory, and so we can rely on in-memory resizes
> and then flush the changed metadata back to disk.
> 
> One part that is potentially now quite strange is that bitmap resizes
> may happen twice: once during the qcow2 resize event only if persistent
> bitmaps are found, and then again as part of the generic resize callback
> event whether or not we have any persistent bitmaps.
> 
> The second round is required if we are not using qcow2 or we have only
> transient bitmaps. The first round is required as we wish to flush the
> bitmaps back to disk atomically with the qcow2 resize to avoid violating
> our invariants for the bitmap metadata which is checked in many places.
> 
> This is harmless; hbitmap_truncate will recognize the second round as
> a no-op.
> 
> John Snow (5):
>   block/qcow2-bitmap: Skip length check in some cases
>   block/qcow2-bitmap: Allow bitmap flushing
>   block/qcow2-bitmap: don't remove bitmaps on reopen
>   block/qcow2-bitmap: Allow resizes with persistent bitmaps
>   tests/qemu-iotests: add bitmap resize test 246
> 
>  block/qcow2.h  |   2 +
>  block/qcow2-bitmap.c   | 123 +++
>  block/qcow2.c  |  27 +++-
>  tests/qemu-iotests/246 | 114 ++
>  tests/qemu-iotests/246.out | 296 +
>  tests/qemu-iotests/group   |   1 +
>  6 files changed, 528 insertions(+), 35 deletions(-)
>  create mode 100755 tests/qemu-iotests/246
>  create mode 100644 tests/qemu-iotests/246.out
> 

Re: [Qemu-devel] [PATCH v3 0/7] bitmaps: add inconsistent bit

[John Snow]

On 3/1/19 2:15 PM, John Snow wrote:
> Allow QEMU to read in bitmaps that have the in-use bit set, for the
> purposes of allowing users to delete those bitmaps.
> 
> This is chosen in preference to a hard error on load to minimize
> impact for a non-critical error, but to force the user or management
> utility to acknowledge that the bitmap is no longer viable.
> 
> 1. Changed wording of meaning of persistent bit, inconsistent bit
>Declining to optimize to avoid allocations for this revision.
> 
> 2. Add Reviewed-by from Eric.
> 
> 3. Split into several patches that are more single-purpose, which
>highlights the individual fixes more clearly;
> 
>- Prohibit BUSY or INCONSISTENT bitmaps from being merge sources.
> 
> 4. Declining feedback to prohibit disabling or enabling readonly bitmaps,
>on the basis that users may wish to enable/disable them prior to
>remounting their backing storage RW.
> 
>Decided to prohibit attempting to remove readonly bitmaps, so the
>failure happens earlier.
> 
>Prohibit sync=incremental backups using readonly bitmaps, because
>they're not capable of clearing the bitmap on success.
>sync=differential would be acceptable here. (Good spot, Vladimir.)
> 
> John Snow (7):
>   block/dirty-bitmaps: add inconsistent bit
>   block/dirty-bitmap: add inconsistent status
>   block/dirty-bitmaps: add block_dirty_bitmap_check function
>   block/dirty-bitmaps: prohibit readonly bitmaps for backups
>   block/dirty-bitmaps: prohibit removing readonly bitmaps
>   block/dirty-bitmaps: disallow busy bitmaps as merge source
>   block/dirty-bitmaps: implement inconsistent bit
> 
>  qapi/block-core.json   |  20 +--
>  include/block/dirty-bitmap.h   |  15 -
>  block/dirty-bitmap.c   |  63 +---
>  block/qcow2-bitmap.c   | 103 +
>  blockdev.c |  50 
>  migration/block-dirty-bitmap.c |  12 +---
>  nbd/server.c   |   3 +-
>  7 files changed, 151 insertions(+), 115 deletions(-)
> 

Thanks for the reviews Eric, I have staged this on my bitmaps branch. I
realize this is hasty, but it's time to get patches in for release, and
it will make reviewing the next series earlier.

Vladimir, I realize I didn't take a few of your suggestions here so I am
delaying sending the PR for this a little bit to give you a chance to
object.

--js

Re: [Qemu-devel] [RFC 2/2] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[David Gibson]

On Tue, Mar 05, 2019 at 12:02:30PM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 05:10:42PM +0100, David Hildenbrand wrote:
> > On 05.03.19 17:03, Michael S. Tsirkin wrote:
> > > On Tue, Mar 05, 2019 at 03:15:38PM +0100, David Hildenbrand wrote:
> > >> On 05.03.19 06:11, David Gibson wrote:
> > >>> Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
> > >>> deflate", the balloon device issued an madvise() MADV_WILLNEED on
> > >>> pages removed from the balloon.  That would hint to the host kernel
> > >>> that the pages were likely to be needed by the guest in the near
> > >>> future.
> > >>>
> > >>> It's unclear if this is actually valuable or not, and so f6deb6d9
> > >>> removed this, essentially ignoring balloon deflate requests.  However,
> > >>> concerns have been raised that this might cause a performance
> > >>> regression by causing extra latency for the guest in certain
> > >>> configurations.
> > >>
> > >> I mean, it will mainly create page tables as far as I know. Any write to
> > >> a page will have an overhead either way (COW zero page). Reads *might*
> > >> be faster.
> > >>
> > >> As we are working on 4k granularity in the balloon (and doing
> > >> MADV_DONTNEED on 4k granularity!), there will most probably be page
> > >> tables already either way. A page table could only be zapped if all
> > >> pages of that page table are MADV_DONTNEED'ed (or I assume never were
> > >> touched), and I am not sure if "random MADV_DONTNEED'ing of 4k pages"
> > >> will actually get rid of page tables (my assumption would be: only if a
> > >> complete range is zapped at once). I haven't looked into the details,
> > >> though (plenty of other stuff to do).
> > >>
> > >> I am not sure if I share the concerns. Real-time workload should never
> > >> use the virtio-balloon in a way that anything like that would be 
> > >> possible.
> > >>
> > >>>
> > >>> So, until we can get actual benchmark data to see if that's the case,
> > >>> this restores (by default) the old behaviour, issuing a MADV_WILLNEED
> > >>> when a page is removed from the balloon.  A new property on the
> > >>> balloon device "hint-on-deflate" can be set to false to remove this
> > >>> behaviour for testing.
> > >>
> > >> This is certainly a good approach for you to finally be able to leave
> > >> the ugly land of virtio-balloon :)
> > >>
> > >> But at least to me, this looks completely useless. I'll be happy to be
> > >> proven wrong as always :)
> > > 
> > > Point is if we don't intend to extend balloon any further then let's
> > > not make random untested changes to its behaviour.
> > 
> > Agreed, but than I'd much rather want to avoid the original change
> > instead of adding a new property that most probably nobody will ever use.
> 
> OK, I don't mind either way.

Ok, I'll take the property out.  Or at least, move the new property to
a private experimental patch to make it easier to try to benchmark.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

On Tue, Mar 05, 2019 at 09:41:34AM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> > On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > > When the balloon is inflated, we discard memory place in it using 
> > > > > > madvise()
> > > > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, 
> > > > > > which
> > > > > > sounds like it makes sense but is actually unnecessary.
> > > > > > 
> > > > > > The misleadingly named MADV_DONTNEED just discards the memory in 
> > > > > > question,
> > > > > > it doesn't set any persistent state on it in-kernel; all that's 
> > > > > > necessary
> > > > > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > > > > specifically says that the memory will be used soon and faults it 
> > > > > > in.
> > > > > > 
> > > > > > This patch simplify's the balloon operation by dropping the 
> > > > > > madvise()
> > > > > > on deflate.  This might have an impact on performance - it will 
> > > > > > move a
> > > > > > delay at deflate time until that memory is actually touched, which
> > > > > > might be more latency sensitive.  However:
> > > > > > 
> > > > > >   * Memory that's being given back to the guest by deflating the
> > > > > > balloon *might* be used soon, but it equally could just sit 
> > > > > > around
> > > > > > in the guest's pools until needed (or even be faulted out again 
> > > > > > if
> > > > > > the host is under memory pressure).
> > > > > > 
> > > > > >   * Usually, the timescale over which you'll be adjusting the 
> > > > > > balloon
> > > > > > is long enough that a few extra faults after deflation aren't
> > > > > > going to make a difference.
> > > > > > 
> > > > > > Signed-off-by: David Gibson 
> > > > > > Reviewed-by: David Hildenbrand 
> > > > > > Reviewed-by: Michael S. Tsirkin 
> > > > > 
> > > > > I'm having second thoughts about this. It might affect performance but
> > > > > probably won't but we have no idea.  Might cause latency jitter after
> > > > > deflate where it previously didn't happen.  This kind of patch should
> > > > > really be accompanied by benchmarking results, not philosophy.
> > > > 
> > > > I guess I see your point, much as it's annoying to spend time
> > > > benchmarking a device that's basically broken by design.
> > > 
> > > Because of 4K page thing?
> > 
> > For one thing.  I believe David H has bunch of other reasons.
> > 
> > > It's an annoying bug for sure.  There were
> > > patches to add a feature bit to just switch to plan s/g format, but they
> > > were abandoned. You are welcome to revive them though.
> > > Additionally or alternatively, we can easily add a field specifying
> > > page size.
> > 
> > We could, but I'm pretty disinclined to work on this when virtio-mem
> > is a better solution in nearly every way.
> 
> Then one way would be to just let balloon be. Make it behave same as
> always and don't make changes to it :)

I'd love to, but it is in real world use, so I think we do need to fix
serious bugs in it - at least if they can be fixed on one side,
without needing to roll out both qemu and guest changes (which adding
page size negotiation would require).

> 
> > > > That said.. I don't really know how I'd go about benchmarking it.  Any
> > > > guesses at a suitable workload which would be most likely to show a
> > > > performance degradation here?
> > > 
> > > Here's one idea - I tried to come up with a worst case scenario here.
> > > Basically based on idea by Alex Duyck. All credits are his, all bugs are
> > > mine:
> > 
> > Ok.  I'll try to find time to implement this and test it.
> > 
> > > Setup:
> > > Memory-15837 MB
> > > Guest Memory Size-5 GB
> > > Swap-Disabled
> > > Test Program-Simple program which allocates 4GB memory via malloc, 
> > > touches it via memset and exits.
> > > Use case-Number of guests that can be launched completely including the 
> > > successful execution of the test program.
> > > Procedure:
> > > Setup:
> > > A first guest is launched and once its console is up,
> > > test allocation program is executed with 4 GB memory request (Due to
> > > this the guest occupies almost 4-5 GB of memory in the host)
> > > Afterwards balloon is inflated by 4Gbyte in the guest.
> > > We continue launching the guests until a guest gets
> > > killed due to low memory condition in the host.
> > > 
> > > 
> > > Now repeatedly, in each guest in turn, balloon is deflated and
> > > test allocation program is executed with 4 GB memory request (Due to
> > > this the guest occupies almost 4-5 GB of memory in the host)
> > > After program finishes balloon is inflated by 4GB again.
> > > 
> > > Then we switch to another guest.
> > > 
> > > Time how many c

Re: [Qemu-devel] [PULL 23/26] virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

[David Gibson]

On Tue, Mar 05, 2019 at 04:06:54PM +, Peter Maydell wrote:
> On Fri, 22 Feb 2019 at 02:41, Michael S. Tsirkin  wrote:
> >
> > From: David Gibson 
> >
> > The virtio-balloon always works in units of 4kiB (BALLOON_PAGE_SIZE), but
> > we can only actually discard memory in units of the host page size.
> 
> Hi -- Coverity points out an issue in this patch (CID 1399146):
> 
> > +/* Hard case
> > + *
> > + * We've put a piece of a larger host page into the balloon - we
> > + * need to keep track until we have a whole host page to
> > + * discard
> > + */
> > +warn_report_once(
> > +"Balloon used with backing page size > 4kiB, this may not be reliable");
> > +
> > +subpages = rb_page_size / BALLOON_PAGE_SIZE;
> > +
> > +if (balloon->pbp
> > +&& (rb != balloon->pbp->rb
> > +|| host_page_base != balloon->pbp->base)) {
> > +/* We've partially ballooned part of a host page, but now
> > + * we're trying to balloon part of a different one.  Too hard,
> > + * give up on the old partial page */
> > +free(balloon->pbp);
> > +balloon->pbp = NULL;
> >  }
> >
> > -ram_block_discard_range(rb, ram_offset, rb_page_size);
> > -/* We ignore errors from ram_block_discard_range(), because it has
> > - * already reported them, and failing to discard a balloon page is
> > - * not fatal */
> > +if (!balloon->pbp) {
> > +/* Starting on a new host page */
> > +size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
> > +balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen);
> 
> 
> We allocate balloon->pbp with g_malloc0() here...
> 
> > +balloon->pbp->rb = rb;
> > +balloon->pbp->base = host_page_base;
> > +}
> > +
> > +bitmap_set(balloon->pbp->bitmap,
> > +   (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> > +   subpages);
> > +
> > +if (bitmap_full(balloon->pbp->bitmap, subpages)) {
> > +/* We've accumulated a full host page, we can actually discard
> > + * it now */
> > +
> > +ram_block_discard_range(rb, balloon->pbp->base, rb_page_size);
> > +/* We ignore errors from ram_block_discard_range(), because it
> > + * has already reported them, and failing to discard a balloon
> > + * page is not fatal */
> > +
> > +free(balloon->pbp);
> 
> ...but we free it (here and elsewhere) with free(), not g_free().

Ah.  Whoops.

I'll put a fix for that in the series of followup balloon patches I'm
working on right now.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC 2/2] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[David Gibson]

On Tue, Mar 05, 2019 at 03:15:38PM +0100, David Hildenbrand wrote:
> On 05.03.19 06:11, David Gibson wrote:
> > Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
> > deflate", the balloon device issued an madvise() MADV_WILLNEED on
> > pages removed from the balloon.  That would hint to the host kernel
> > that the pages were likely to be needed by the guest in the near
> > future.
> > 
> > It's unclear if this is actually valuable or not, and so f6deb6d9
> > removed this, essentially ignoring balloon deflate requests.  However,
> > concerns have been raised that this might cause a performance
> > regression by causing extra latency for the guest in certain
> > configurations.
> 
> I mean, it will mainly create page tables as far as I know. Any write to
> a page will have an overhead either way (COW zero page). Reads *might*
> be faster.
> 
> As we are working on 4k granularity in the balloon (and doing
> MADV_DONTNEED on 4k granularity!), there will most probably be page
> tables already either way. A page table could only be zapped if all
> pages of that page table are MADV_DONTNEED'ed (or I assume never were
> touched), and I am not sure if "random MADV_DONTNEED'ing of 4k pages"
> will actually get rid of page tables (my assumption would be: only if a
> complete range is zapped at once). I haven't looked into the details,
> though (plenty of other stuff to do).
> 
> I am not sure if I share the concerns. Real-time workload should never
> use the virtio-balloon in a way that anything like that would be possible.
> 
> > 
> > So, until we can get actual benchmark data to see if that's the case,
> > this restores (by default) the old behaviour, issuing a MADV_WILLNEED
> > when a page is removed from the balloon.  A new property on the
> > balloon device "hint-on-deflate" can be set to false to remove this
> > behaviour for testing.
> 
> This is certainly a good approach for you to finally be able to leave
> the ugly land of virtio-balloon :)
> 
> But at least to me, this looks completely useless. I'll be happy to be
> proven wrong as always :)

Frankly, I agree.  But mst was nervous about us making that change to
the balloon's previous behaviour.

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


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 5/5] tests/qemu-iotests: add bitmap resize test 246

[John Snow]

Test that we can actually resize qcow2 images with persistent bitmaps
correctly. Throw some other goofy stuff at the test while we're at it,
like adding bitmaps of different granularities and at different times.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/246 | 114 ++
 tests/qemu-iotests/246.out | 296 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 411 insertions(+)
 create mode 100755 tests/qemu-iotests/246
 create mode 100644 tests/qemu-iotests/246.out

diff --git a/tests/qemu-iotests/246 b/tests/qemu-iotests/246
new file mode 100755
index 00..e5bd1ce2d1
--- /dev/null
+++ b/tests/qemu-iotests/246
@@ -0,0 +1,114 @@
+#!/usr/bin/env python
+#
+# Test persistent bitmap resizing.
+#
+# Copyright (c) 2019 John Snow for Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+# owner=js...@redhat.com
+
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+size = 64 * 1024 * 1024 * 1024
+gran_small = 32 * 1024
+gran_large = 128 * 1024
+
+def query_bitmaps(vm):
+res = vm.qmp("query-block")
+return { "bitmaps": { device['device']: device.get('dirty-bitmaps', []) for
+  device in res['return'] } }
+
+with iotests.FilePath('img') as img_path, \
+ iotests.VM() as vm:
+
+log('--- Preparing image & VM ---\n')
+iotests.qemu_img_create('-f', iotests.imgfmt, img_path, str(size))
+vm.add_drive(img_path)
+
+
+log('--- 1st Boot (Establish Baseline Image) ---\n')
+vm.launch()
+
+log('\n--- Adding bitmaps Small, Medium, Large, and Transient ---\n')
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="Small", granularity=gran_small, persistent=True)
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="Medium", persistent=True)
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="Large", granularity=gran_large, persistent=True)
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="Transient", persistent=False)
+
+log('--- Forcing flush of bitmaps to disk ---\n')
+log(query_bitmaps(vm), indent=2)
+vm.shutdown()
+
+
+log('--- 2nd Boot (Grow Image) ---\n')
+vm.launch()
+log(query_bitmaps(vm), indent=2)
+
+log('--- Adding new bitmap, growing image, and adding 2nd new bitmap ---')
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="New", persistent=True)
+vm.qmp_log("human-monitor-command",
+   command_line="block_resize drive0 70G")
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="Newtwo", persistent=True)
+log(query_bitmaps(vm), indent=2)
+
+log('--- Forcing flush of bitmaps to disk ---\n')
+vm.shutdown()
+
+
+log('--- 3rd Boot (Shrink Image) ---\n')
+vm.launch()
+log(query_bitmaps(vm), indent=2)
+
+log('--- Adding "NewB" bitmap, removing "New" bitmap ---')
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="NewB", persistent=True)
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0",
+   name="New")
+
+log('--- Truncating image ---\n')
+vm.qmp_log("human-monitor-command",
+   command_line="block_resize drive0 50G")
+
+log('--- Adding "NewC" bitmap, removing "NewTwo" bitmap ---')
+vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+   name="NewC", persistent=True)
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Newtwo")
+
+log('--- Forcing flush of bitmaps to disk ---\n')
+vm.shutdown()
+
+
+log('--- 4th Boot (Verification and Cleanup) ---\n')
+vm.launch()
+log(query_bitmaps(vm), indent=2)
+
+log('--- Removing all Bitmaps ---\n')
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Small")
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Medium")
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Large")
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="NewB")
+vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="NewC")
+log(query_bitmaps(vm), indent=2)
+
+log('\n--- Done ---\n')
+vm.shutdown()
diff --git a/tests/qemu-iotests/246.out b/tests/qemu-iotests/246.out
new file mode 100644
index 00..ea591b0e46
--- /dev/

Re: [Qemu-devel] Question about hardware cursor in VGA

[BALATON Zoltan]

On Tue, 5 Mar 2019, Gerd Hoffmann wrote:

On Mon, Mar 04, 2019 at 03:58:00PM +0100, BALATON Zoltan wrote:

Hello,

I'm trying to implement hardware cursor for ati-vga before submitting the
last version of it before the freeze.


Use dpy_cursor_define().


I've done that but it's not working very well. The mouse pointer is quite 
jumpy with this and there are some problems updating the pointer image 
when the guest changes image data in video ram without changing registers. 
(Like when it sets cursor data offset first, then writes image data in 
memory after that but QEMUCursor is created from the still empty data on 
setting the register.) I've managed to solve this latter problem (at least 
for the guest I care about) by allowing some unnecessary cursor updates 
but position changes remain erratic. I'll submit this version (after some 
more checking of 2d functions which still don't work well beyond simple 
cases) because at least doing hw cursor like this works somewhat but I 
note that the other version with cursor_invalidate and cursor_draw_line 
callbacks worked much smoother, alas I've found that cannot work with 
shared_surface.


I'm not sure what causes this pointer position update problem. Could it be 
that pointer position updates are now independent of screen refresh with 
dpy_mouse_move so there's a higher chance of doing big jumps where the 
invalidate/draw_line callbacks were called from display update so they've 
sampled mouse position in smaller steps somehow? Or is it because mouse 
pointer is now a real hardware cursor which can be moved independent of 
the guest but any guest update will warp it back to where the guest thinks 
it should be? (Also note that draw_line callback could do complement 
pixels but that's not possible with dpy_cursor_define.)


This also reminds me that jumpy mouse problem was also reported with MacOS 
guests before with the gtk UI I think 
(https://www.emaculation.com/forum/viewtopic.php?f=34&t=9750) but not sure 
if that's related to this or not. Also not sure what would be the best way 
to improve this: reviving at least the mouse invalidate callback and do 
dpy_mouse_move from there instead of calling it on register change? Or 
also fixing the draw_cursor_line callback with shared surface maybe by 
providing a separate surface for the mouse in that case for the device 
model to render the mouse into which then can be composited on the display 
surface somehow by the frontend? This may be problematic for remote 
displays and may also loose the advantage of the shared surface unless 
there's some hardware support for this so I'm not sure about that. But at 
least the jumpy cursor should be fixed somehow because that makes user 
experience with hardware cursor quite bad.


Regards,
BALATON Zoltan

[Qemu-devel] [PATCH 2/5] block/qcow2-bitmap: Allow bitmap flushing

[John Snow]

Usually, we only write out bitmaps when we're about to close out the file,
so we always remove the bitmaps after to make it easier to determine the
source of truth for migration purposes.

However, there may be times we want to flush bitmap data more aggressively,
like after a truncate event where we need to make the metadata consistent
again.

Signed-off-by: John Snow 
---
 block/qcow2.h|  1 +
 block/qcow2-bitmap.c | 45 ++--
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 9dd02df831..4c1fdfcbec 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -690,6 +690,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool 
*header_updated,
  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d02730004a..4e11b6b05a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1392,7 +1392,7 @@ fail:
 bitmap_list_free(bm_list);
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+static void do_store_bitmaps(BlockDriverState *bs, bool remove, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
 BDRVQcow2State *s = bs->opaque;
@@ -1474,6 +1474,9 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
 }
 bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
+if (!remove) {
+bm->flags |= BME_FLAG_IN_USE;
+}
 bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
 bm->dirty_bitmap = bitmap;
 }
@@ -1503,20 +1506,14 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 g_free(tb);
 }
 
-QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-/* For safety, we remove bitmap after storing.
- * We may be here in two cases:
- * 1. bdrv_close. It's ok to drop bitmap.
- * 2. inactivation. It means migration without 'dirty-bitmaps'
- *capability, so bitmaps are not marked with
- *BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
- *and reload on invalidation.
- */
-if (bm->dirty_bitmap == NULL) {
-continue;
-}
+if (remove) {
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+if (bm->dirty_bitmap == NULL) {
+continue;
+}
 
-bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+}
 }
 
 bitmap_list_free(bm_list);
@@ -1538,6 +1535,26 @@ fail:
 bitmap_list_free(bm_list);
 }
 
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+/* For safety, we remove bitmap after storing.
+ * We may be here in two cases:
+ * 1. bdrv_close. It's ok to drop bitmap.
+ * 2. inactivation. It means migration without 'dirty-bitmaps'
+ *capability, so bitmaps are not marked with
+ *BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
+ *and reload on invalidation.
+ */
+do_store_bitmaps(bs, true, errp);
+}
+
+void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+/* We're here because of some event like resize or reopen_ro, and we want
+ * to flush bitmaps to disk without removing them from memory. */
+do_store_bitmaps(bs, false, errp);
+}
+
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
-- 
2.17.2

[Qemu-devel] [PATCH 3/5] block/qcow2-bitmap: don't remove bitmaps on reopen

[John Snow]

We tend to remove bitmaps when we flush them to disk, but it's not appropriate
in all cases. let the reopen mechanism use the lighter weight flush instead of
the heavier store.

Signed-off-by: John Snow 
---
 block/qcow2-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 4e11b6b05a..9373055d3b 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1560,7 +1560,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 BdrvDirtyBitmap *bitmap;
 Error *local_err = NULL;
 
-qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
 if (local_err != NULL) {
 error_propagate(errp, local_err);
 return -EINVAL;
-- 
2.17.2

[Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[John Snow]

This series aims to enable block resizes when persistent bitmaps are
in use. The basic approach here is to recognize that we now load all
persistent bitmaps in memory, and so we can rely on in-memory resizes
and then flush the changed metadata back to disk.

One part that is potentially now quite strange is that bitmap resizes
may happen twice: once during the qcow2 resize event only if persistent
bitmaps are found, and then again as part of the generic resize callback
event whether or not we have any persistent bitmaps.

The second round is required if we are not using qcow2 or we have only
transient bitmaps. The first round is required as we wish to flush the
bitmaps back to disk atomically with the qcow2 resize to avoid violating
our invariants for the bitmap metadata which is checked in many places.

This is harmless; hbitmap_truncate will recognize the second round as
a no-op.

John Snow (5):
  block/qcow2-bitmap: Skip length check in some cases
  block/qcow2-bitmap: Allow bitmap flushing
  block/qcow2-bitmap: don't remove bitmaps on reopen
  block/qcow2-bitmap: Allow resizes with persistent bitmaps
  tests/qemu-iotests: add bitmap resize test 246

 block/qcow2.h  |   2 +
 block/qcow2-bitmap.c   | 123 +++
 block/qcow2.c  |  27 +++-
 tests/qemu-iotests/246 | 114 ++
 tests/qemu-iotests/246.out | 296 +
 tests/qemu-iotests/group   |   1 +
 6 files changed, 528 insertions(+), 35 deletions(-)
 create mode 100755 tests/qemu-iotests/246
 create mode 100644 tests/qemu-iotests/246.out

-- 
2.17.2

[Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[John Snow]

If we were to allow resizes, the length check that happens when we load
bitmap headers from disk when we read or store bitmaps would begin to
fail:

Imagine the circumstance where we've resized bitmaps in memory, but they still
have the old values on-disk. The lengths will no longer match bdrv_getlength,
so we must allow this check to be skipped when flushing bitmaps to disk.

Limit this to when we are about to overwrite the headers: we will verify the
outgoing headers, but we will skip verifying the known stale headers.

Signed-off-by: John Snow 
---
 block/qcow2-bitmap.c | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index c3b210ede1..d02730004a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry 
*next_dir_entry(Qcow2BitmapDirEntry *entry)
 return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
 }
 
-static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
+static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
+   bool allow_resize)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t phys_bitmap_bytes;
@@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, 
Qcow2BitmapDirEntry *entry)
 return len;
 }
 
-fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
-   (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
+if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
+return -EINVAL;
+}
+
+if (!allow_resize &&
+(len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
+return -EINVAL;
+}
 
 return fail ? -EINVAL : 0;
 }
@@ -534,7 +541,8 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
  * checks it and convert to bitmap list.
  */
 static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
- uint64_t size, Error **errp)
+ uint64_t size, bool allow_resize,
+ Error **errp)
 {
 int ret;
 BDRVQcow2State *s = bs->opaque;
@@ -593,7 +601,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState 
*bs, uint64_t offset,
 goto fail;
 }
 
-ret = check_dir_entry(bs, e);
+ret = check_dir_entry(bs, e, allow_resize);
 if (ret < 0) {
 error_setg(errp, "Bitmap '%.*s' doesn't satisfy the constraints",
e->name_size, dir_entry_name_field(e));
@@ -654,7 +662,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 }
 
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-   s->bitmap_directory_size, NULL);
+   s->bitmap_directory_size, false, NULL);
 if (bm_list == NULL) {
 res->corruptions++;
 return -EINVAL;
@@ -755,7 +763,7 @@ static int bitmap_list_store(BlockDriverState *bs, 
Qcow2BitmapList *bm_list,
 e->extra_data_size = 0;
 memcpy(e + 1, bm->name, e->name_size);
 
-if (check_dir_entry(bs, e) < 0) {
+if (check_dir_entry(bs, e, false) < 0) {
 ret = -EINVAL;
 goto fail;
 }
@@ -957,7 +965,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error 
**errp)
 }
 
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-   s->bitmap_directory_size, errp);
+   s->bitmap_directory_size, false, errp);
 if (bm_list == NULL) {
 return false;
 }
@@ -1066,7 +1074,7 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 }
 
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-   s->bitmap_directory_size, errp);
+   s->bitmap_directory_size, false, errp);
 if (bm_list == NULL) {
 return NULL;
 }
@@ -,7 +1119,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 }
 
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-   s->bitmap_directory_size, errp);
+   s->bitmap_directory_size, false, errp);
 if (bm_list == NULL) {
 return -EINVAL;
 }
@@ -1359,7 +1367,7 @@ void 
qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 }
 
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-   s->bitmap_directory_size, errp);
+   s->bitmap_directory_size, false, errp);
 if (bm_list == NULL) {
 return;
 }
@@ -1412,7 +1420,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bm_list = bitmap_list_new();
 } else {
 bm_list = bitmap_list_loa

[Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[John Snow]

Since we now load all bitmaps into memory anyway, we can just truncate them
in-memory and then flush them back to disk. Just in case, we will still check
and enforce that this shortcut is valid -- i.e. that any bitmap described
on-disk is indeed in-memory and can be modified.

If there are any inconsistent bitmaps, we refuse to allow the truncate as
we do not actually load these bitmaps into memory, and it isn't safe or
reasonable to attempt to truncate corrupted data.

Signed-off-by: John Snow 
---
 block/qcow2.h|  1 +
 block/qcow2-bitmap.c | 42 ++
 block/qcow2.c| 27 ---
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 4c1fdfcbec..b9227a72cc 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -689,6 +689,7 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 9373055d3b..24f280bccc 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
 }
 
+/* Checks to see if it's safe to resize bitmaps */
+int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+Qcow2Bitmap *bm;
+int ret = 0;
+
+if (s->nb_bitmaps == 0) {
+return 0;
+}
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, false, errp);
+if (bm_list == NULL) {
+return -EINVAL;
+}
+
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+if (bitmap == NULL) {
+/* We rely on all bitmaps being in-memory to be able to resize 
them,
+ * Otherwise, we'd need to resize them on disk explicitly */
+error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that 
"
+   "were not loaded into memory");
+ret = -ENOTSUP;
+goto out;
+}
+
+/* The checks against readonly and busy are redundant, but certainly
+ * do no harm. checks against inconsistent are crucial: */
+if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+ret = -ENOTSUP;
+goto out;
+}
+}
+
+out:
+bitmap_list_free(bm_list);
+return ret;
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitmap table accordingly.
  */
diff --git a/block/qcow2.c b/block/qcow2.c
index 7fb2730f09..6fd9252494 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3472,7 +3472,7 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t old_length;
-int64_t new_l1_size;
+int64_t new_l1_size, offset_be;
 int ret;
 QDict *options;
 
@@ -3498,10 +3498,8 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 goto fail;
 }
 
-/* cannot proceed if image has bitmaps */
-if (s->nb_bitmaps) {
-/* TODO: resize bitmaps in the image */
-error_setg(errp, "Can't resize an image which has bitmaps");
+/* Only certain persistent bitmaps can be resized: */
+if (qcow2_truncate_bitmaps_check(bs, errp)) {
 ret = -ENOTSUP;
 goto fail;
 }
@@ -3702,9 +3700,9 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 bs->total_sectors = offset / BDRV_SECTOR_SIZE;
 
 /* write updated header.size */
-offset = cpu_to_be64(offset);
+offset_be = cpu_to_be64(offset);
 ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
-   &offset, sizeof(uint64_t));
+   &offset_be, sizeof(uint64_t));
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Failed to update the image size");
 goto fail;
@@ -3719,6 +3717,21 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 if (ret < 0) {
 goto fail;
 }
+
+/* Flush bitmaps */
+if (s->nb_bitmaps) {
+Error *local_err = NULL;
+
+/* Usually, bitmaps get resized after this call.
+ * Force it earlier so we can make the metadata consistent. */
+bdrv_dirty_bitmap_truncate(bs,

[Qemu-devel] [PATCH v5 12/12] contrib: add vhost-user-input

[Marc-André Lureau]

Add a vhost-user input backend example, based on virtio-input-host
device. It takes an evdev path as argument, and can be associated with
a vhost-user-input device via a UNIX socket:

$ vhost-user-input -p /dev/input/eventX -s /tmp/vui.sock

$ qemu ... -chardev socket,id=vuic,path=/tmp/vui.sock
  -device vhost-user-input-pci,chardev=vuic

Signed-off-by: Marc-André Lureau 
Reviewed-by: Gerd Hoffmann 
---
 contrib/vhost-user-input/main.c| 398 +
 MAINTAINERS|   1 +
 Makefile   |   3 +
 Makefile.objs  |   1 +
 configure  |   3 +
 contrib/vhost-user-input/Makefile.objs |   1 +
 6 files changed, 407 insertions(+)
 create mode 100644 contrib/vhost-user-input/main.c
 create mode 100644 contrib/vhost-user-input/Makefile.objs

diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
new file mode 100644
index 00..bef44f163d
--- /dev/null
+++ b/contrib/vhost-user-input/main.c
@@ -0,0 +1,398 @@
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include 
+#include 
+
+#include "qemu/iov.h"
+#include "qemu/bswap.h"
+#include "contrib/libvhost-user/libvhost-user.h"
+#include "contrib/libvhost-user/libvhost-user-glib.h"
+#include "standard-headers/linux/virtio_input.h"
+#include "qapi/error.h"
+
+typedef struct virtio_input_event virtio_input_event;
+typedef struct virtio_input_config virtio_input_config;
+
+typedef struct VuInput {
+VugDev dev;
+GSource *evsrc;
+int evdevfd;
+GArray *config;
+struct {
+virtio_input_event event;
+VuVirtqElement *elem;
+} *queue;
+uint32_t qindex, qsize;
+} VuInput;
+
+static void vi_input_send(VuInput *vi, struct virtio_input_event *event)
+{
+VuDev *dev = &vi->dev.parent;
+VuVirtq *vq = vu_get_queue(dev, 0);
+VuVirtqElement *elem;
+int i, len;
+
+/* queue up events ... */
+if (vi->qindex == vi->qsize) {
+vi->qsize++;
+vi->queue = g_realloc_n(vi->queue, vi->qsize, sizeof(vi->queue[0]));
+}
+vi->queue[vi->qindex++].event = *event;
+
+/* ... until we see a report sync ... */
+if (event->type != htole16(EV_SYN) ||
+event->code != htole16(SYN_REPORT)) {
+return;
+}
+
+/* ... then check available space ... */
+for (i = 0; i < vi->qindex; i++) {
+elem = vu_queue_pop(dev, vq, sizeof(VuVirtqElement));
+if (!elem) {
+while (--i >= 0) {
+vu_queue_unpop(dev, vq, vi->queue[i].elem, 0);
+}
+vi->qindex = 0;
+g_warning("virtio-input queue full");
+return;
+}
+vi->queue[i].elem = elem;
+}
+
+/* ... and finally pass them to the guest */
+for (i = 0; i < vi->qindex; i++) {
+elem = vi->queue[i].elem;
+len = iov_from_buf(elem->in_sg, elem->in_num,
+   0, &vi->queue[i].event, sizeof(virtio_input_event));
+vu_queue_push(dev, vq, elem, len);
+g_free(elem);
+}
+
+vu_queue_notify(&vi->dev.parent, vq);
+vi->qindex = 0;
+}
+
+static void
+vi_evdev_watch(VuDev *dev, int condition, void *data)
+{
+VuInput *vi = data;
+int fd = vi->evdevfd;
+
+g_debug("Got evdev condition %x", condition);
+
+struct virtio_input_event virtio;
+struct input_event evdev;
+int rc;
+
+for (;;) {
+rc = read(fd, &evdev, sizeof(evdev));
+if (rc != sizeof(evdev)) {
+break;
+}
+
+g_debug("input %d %d %d", evdev.type, evdev.code, evdev.value);
+
+virtio.type  = htole16(evdev.type);
+virtio.code  = htole16(evdev.code);
+virtio.value = htole32(evdev.value);
+vi_input_send(vi, &virtio);
+}
+}
+
+
+static void vi_handle_status(VuInput *vi, virtio_input_event *event)
+{
+struct input_event evdev;
+int rc;
+
+if (gettimeofday(&evdev.time, NULL)) {
+perror("vi_handle_status: gettimeofday");
+return;
+}
+
+evdev.type = le16toh(event->type);
+evdev.code = le16toh(event->code);
+evdev.value = le32toh(event->value);
+
+rc = write(vi->evdevfd, &evdev, sizeof(evdev));
+if (rc == -1) {
+perror("vi_host_handle_status: write");
+}
+}
+
+static void vi_handle_sts(VuDev *dev, int qidx)
+{
+VuInput *vi = container_of(dev, VuInput, dev.parent);
+VuVirtq *vq = vu_get_queue(dev, qidx);
+virtio_input_event event;
+VuVirtqElement *elem;
+int len;
+
+g_debug("%s", G_STRFUNC);
+
+for (;;) {
+elem = vu_queue_pop(dev, vq, sizeof(VuVirtqElement));
+if (!elem) {
+break;
+}
+
+memset(&event, 0, sizeof(event));
+len = iov_to_buf(elem->out_sg, elem->out_num,
+

[Qemu-devel] [PATCH v5 08/12] vhost-user: add vhost_user_input_get_config()

[Marc-André Lureau]

Ask vhost user input backend the list of virtio_input_config.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Gerd Hoffmann 
---
 contrib/libvhost-user/libvhost-user.h |  1 +
 include/hw/virtio/vhost-backend.h |  4 ++
 hw/virtio/vhost-user.c| 60 +++
 docs/interop/vhost-user.txt   |  8 
 4 files changed, 73 insertions(+)

diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index c0133b7f3f..b0c798fa1a 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -91,6 +91,7 @@ typedef enum VhostUserRequest {
 VHOST_USER_POSTCOPY_ADVISE  = 28,
 VHOST_USER_POSTCOPY_LISTEN  = 29,
 VHOST_USER_POSTCOPY_END = 30,
+VHOST_USER_INPUT_GET_CONFIG = 31,
 VHOST_USER_MAX
 } VhostUserRequest;
 
diff --git a/include/hw/virtio/vhost-backend.h 
b/include/hw/virtio/vhost-backend.h
index 81283ec50f..1fca321d8a 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -12,6 +12,7 @@
 #define VHOST_BACKEND_H
 
 #include "exec/memory.h"
+#include "standard-headers/linux/virtio_input.h"
 
 typedef enum VhostBackendType {
 VHOST_BACKEND_TYPE_NONE = 0,
@@ -160,4 +161,7 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev 
*dev,
 int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
   struct vhost_iotlb_msg *imsg);
 
+int vhost_user_input_get_config(struct vhost_dev *dev,
+struct virtio_input_config **config);
+
 #endif /* VHOST_BACKEND_H */
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5df73405bc..cf3fd39035 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -93,6 +93,7 @@ typedef enum VhostUserRequest {
 VHOST_USER_POSTCOPY_ADVISE  = 28,
 VHOST_USER_POSTCOPY_LISTEN  = 29,
 VHOST_USER_POSTCOPY_END = 30,
+VHOST_USER_INPUT_GET_CONFIG = 31,
 VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -342,6 +343,65 @@ static int vhost_user_write(struct vhost_dev *dev, 
VhostUserMsg *msg,
 return 0;
 }
 
+static void *vhost_user_read_size(struct vhost_dev *dev, uint32_t size)
+{
+struct vhost_user *u = dev->opaque;
+CharBackend *chr = u->user->chr;
+int r;
+uint8_t *p = g_malloc(size);
+
+r = qemu_chr_fe_read_all(chr, p, size);
+if (r != size) {
+error_report("Failed to read msg payload."
+ " Read %d instead of %u.", r, size);
+g_free(p);
+return NULL;
+}
+
+return p;
+}
+
+int vhost_user_input_get_config(struct vhost_dev *dev,
+struct virtio_input_config **config)
+{
+void *p = NULL;
+VhostUserMsg msg = {
+.hdr.request = VHOST_USER_INPUT_GET_CONFIG,
+.hdr.flags = VHOST_USER_VERSION,
+};
+
+if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+goto err;
+}
+
+if (vhost_user_read_header(dev, &msg) < 0) {
+goto err;
+}
+
+if (msg.hdr.request != VHOST_USER_INPUT_GET_CONFIG) {
+error_report("Received unexpected msg type. Expected %d received %d",
+ VHOST_USER_INPUT_GET_CONFIG, msg.hdr.request);
+goto err;
+}
+
+if (msg.hdr.size % sizeof(struct virtio_input_config)) {
+error_report("Invalid msg size");
+goto err;
+}
+
+p = vhost_user_read_size(dev, msg.hdr.size);
+if (!p) {
+goto err;
+}
+
+*config = p;
+return msg.hdr.size / sizeof(struct virtio_input_config);
+
+err:
+g_free(p);
+return -1;
+}
+
 static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
struct vhost_log *log)
 {
diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
index 9ee2a60cfb..e145b3ec55 100644
--- a/docs/interop/vhost-user.txt
+++ b/docs/interop/vhost-user.txt
@@ -766,6 +766,14 @@ Master message types
   was previously sent.
   The value returned is an error indication; 0 is success.
 
+ * VHOST_USER_INPUT_GET_CONFIG
+  Id: 31
+  Master payload: N/A
+  Slave payload: (struct virtio_input_config)*
+
+  Ask vhost user input backend the list of virtio_input_config, in
+  host endianness.
+
 Slave message types
 ---
 
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 06/12] Add vhost-user-backend

[Marc-André Lureau]

Create a vhost-user-backend object that holds a connection to a
vhost-user backend and can be referenced from virtio devices that
support it. See later patches for input & gpu usage.

Signed-off-by: Marc-André Lureau 
---
 include/sysemu/vhost-user-backend.h |  57 
 backends/vhost-user.c   | 209 
 MAINTAINERS |   2 +
 backends/Makefile.objs  |   3 +
 4 files changed, 271 insertions(+)
 create mode 100644 include/sysemu/vhost-user-backend.h
 create mode 100644 backends/vhost-user.c

diff --git a/include/sysemu/vhost-user-backend.h 
b/include/sysemu/vhost-user-backend.h
new file mode 100644
index 00..9abf8f06a1
--- /dev/null
+++ b/include/sysemu/vhost-user-backend.h
@@ -0,0 +1,57 @@
+/*
+ * QEMU vhost-user backend
+ *
+ * Copyright (C) 2018 Red Hat Inc
+ *
+ * Authors:
+ *  Marc-André Lureau 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QEMU_VHOST_USER_BACKEND_H
+#define QEMU_VHOST_USER_BACKEND_H
+
+#include "qom/object.h"
+#include "exec/memory.h"
+#include "qemu/option.h"
+#include "qemu/bitmap.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user.h"
+#include "chardev/char-fe.h"
+#include "io/channel.h"
+
+#define TYPE_VHOST_USER_BACKEND "vhost-user-backend"
+#define VHOST_USER_BACKEND(obj) \
+OBJECT_CHECK(VhostUserBackend, (obj), TYPE_VHOST_USER_BACKEND)
+#define VHOST_USER_BACKEND_GET_CLASS(obj) \
+OBJECT_GET_CLASS(VhostUserBackendClass, (obj), TYPE_VHOST_USER_BACKEND)
+#define VHOST_USER_BACKEND_CLASS(klass) \
+OBJECT_CLASS_CHECK(VhostUserBackendClass, (klass), TYPE_VHOST_USER_BACKEND)
+
+typedef struct VhostUserBackend VhostUserBackend;
+typedef struct VhostUserBackendClass VhostUserBackendClass;
+
+struct VhostUserBackendClass {
+ObjectClass parent_class;
+};
+
+struct VhostUserBackend {
+/* private */
+Object parent;
+
+char *chr_name;
+CharBackend chr;
+VhostUserState vhost_user;
+struct vhost_dev dev;
+VirtIODevice *vdev;
+bool started;
+bool completed;
+};
+
+int vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
+unsigned nvqs, Error **errp);
+void vhost_user_backend_start(VhostUserBackend *b);
+void vhost_user_backend_stop(VhostUserBackend *b);
+
+#endif
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
new file mode 100644
index 00..2b055544a7
--- /dev/null
+++ b/backends/vhost-user.c
@@ -0,0 +1,209 @@
+/*
+ * QEMU vhost-user backend
+ *
+ * Copyright (C) 2018 Red Hat Inc
+ *
+ * Authors:
+ *  Marc-André Lureau 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+
+#include "qemu/osdep.h"
+#include "hw/qdev.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/error-report.h"
+#include "qom/object_interfaces.h"
+#include "sysemu/vhost-user-backend.h"
+#include "sysemu/kvm.h"
+#include "io/channel-command.h"
+#include "hw/virtio/virtio-bus.h"
+
+static bool
+ioeventfd_enabled(void)
+{
+return kvm_enabled() && kvm_eventfds_enabled();
+}
+
+int
+vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
+unsigned nvqs, Error **errp)
+{
+int ret;
+
+assert(!b->vdev && vdev);
+
+if (!ioeventfd_enabled()) {
+error_setg(errp, "vhost initialization failed: requires kvm");
+return -1;
+}
+
+if (!vhost_user_init(&b->vhost_user, &b->chr, errp)) {
+return -1;
+}
+
+b->vdev = vdev;
+b->dev.nvqs = nvqs;
+b->dev.vqs = g_new(struct vhost_virtqueue, nvqs);
+
+ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "vhost initialization failed");
+return -1;
+}
+
+return 0;
+}
+
+void
+vhost_user_backend_start(VhostUserBackend *b)
+{
+BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+int ret, i ;
+
+if (b->started) {
+return;
+}
+
+if (!k->set_guest_notifiers) {
+error_report("binding does not support guest notifiers");
+return;
+}
+
+ret = vhost_dev_enable_notifiers(&b->dev, b->vdev);
+if (ret < 0) {
+return;
+}
+
+ret = k->set_guest_notifiers(qbus->parent, b->dev.nvqs, true);
+if (ret < 0) {
+error_report("Error binding guest notifier");
+goto err_host_notifiers;
+}
+
+b->dev.acked_features = b->vdev->guest_features;
+ret = vhost_dev_start(&b->dev, b->vdev);
+if (ret < 0) {
+error_report("Error start vhost dev");
+goto err_guest_notifiers;
+}
+
+/* guest_notifier_mask/pending not used yet, so just unmask
+ * everything here.  virtio-pci will do the right thing by
+ * enabling/disabling

[Qemu-devel] [PATCH v5 09/12] libvhost-user-glib: export vug_source_new()

[Marc-André Lureau]

Simplify the creation of FD sources for other users. This is just
convenience to avoid duplicating similar code elsewhere.

Signed-off-by: Marc-André Lureau 
---
 contrib/libvhost-user/libvhost-user-glib.h |  3 +++
 contrib/libvhost-user/libvhost-user-glib.c | 11 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/contrib/libvhost-user/libvhost-user-glib.h 
b/contrib/libvhost-user/libvhost-user-glib.h
index 6b2110b94c..d3200f3afc 100644
--- a/contrib/libvhost-user/libvhost-user-glib.h
+++ b/contrib/libvhost-user/libvhost-user-glib.h
@@ -29,4 +29,7 @@ void vug_init(VugDev *dev, int socket,
   vu_panic_cb panic, const VuDevIface *iface);
 void vug_deinit(VugDev *dev);
 
+GSource *vug_source_new(VugDev *dev, int fd, GIOCondition cond,
+vu_watch_cb vu_cb, gpointer data);
+
 #endif /* LIBVHOST_USER_GLIB_H */
diff --git a/contrib/libvhost-user/libvhost-user-glib.c 
b/contrib/libvhost-user/libvhost-user-glib.c
index 545f089587..42660a1b36 100644
--- a/contrib/libvhost-user/libvhost-user-glib.c
+++ b/contrib/libvhost-user/libvhost-user-glib.c
@@ -68,15 +68,16 @@ static GSourceFuncs vug_src_funcs = {
 NULL
 };
 
-static GSource *
-vug_source_new(VuDev *dev, int fd, GIOCondition cond,
+GSource *
+vug_source_new(VugDev *gdev, int fd, GIOCondition cond,
vu_watch_cb vu_cb, gpointer data)
 {
+VuDev *dev = &gdev->parent;
 GSource *gsrc;
 VugSrc *src;
 guint id;
 
-g_assert(dev);
+g_assert(gdev);
 g_assert(fd >= 0);
 g_assert(vu_cb);
 
@@ -106,7 +107,7 @@ set_watch(VuDev *vu_dev, int fd, int vu_evt, vu_watch_cb 
cb, void *pvt)
 g_assert(cb);
 
 dev = container_of(vu_dev, VugDev, parent);
-src = vug_source_new(vu_dev, fd, vu_evt, cb, pvt);
+src = vug_source_new(dev, fd, vu_evt, cb, pvt);
 g_hash_table_replace(dev->fdmap, GINT_TO_POINTER(fd), src);
 }
 
@@ -141,7 +142,7 @@ vug_init(VugDev *dev, int socket,
 dev->fdmap = g_hash_table_new_full(NULL, NULL, NULL,
(GDestroyNotify) g_source_destroy);
 
-dev->src = vug_source_new(&dev->parent, socket, G_IO_IN, vug_watch, NULL);
+dev->src = vug_source_new(dev, socket, G_IO_IN, vug_watch, NULL);
 }
 
 void
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 11/12] Add vhost-user-input-pci

[Marc-André Lureau]

Add a new virtio-input device, which connects to a vhost-user
backend.

vhost-user-input is similar to virtio-input-host, it is wrapped by
vhost-user-input-pci. Instead of reading configuration directly from
an input device / evdev, it reads it over vhost-user protocol with
INPUT_GET_CONFIG message. Then vhost-user-backend takes care of
interfacing the virtio device with the backend, for the queue & events
processing.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Gerd Hoffmann 
---
 include/hw/virtio/virtio-input.h | 14 +
 hw/input/vhost-user-input.c  | 99 
 hw/virtio/vhost-user-input-pci.c | 53 +
 MAINTAINERS  |  1 +
 default-configs/virtio.mak   |  1 +
 hw/input/Makefile.objs   |  1 +
 hw/virtio/Makefile.objs  |  1 +
 7 files changed, 170 insertions(+)
 create mode 100644 hw/input/vhost-user-input.c
 create mode 100644 hw/virtio/vhost-user-input-pci.c

diff --git a/include/hw/virtio/virtio-input.h b/include/hw/virtio/virtio-input.h
index 054c38836f..4fca03e796 100644
--- a/include/hw/virtio/virtio-input.h
+++ b/include/hw/virtio/virtio-input.h
@@ -2,6 +2,7 @@
 #define QEMU_VIRTIO_INPUT_H
 
 #include "ui/input.h"
+#include "sysemu/vhost-user-backend.h"
 
 /* - */
 /* virtio input protocol */
@@ -42,11 +43,18 @@ typedef struct virtio_input_event virtio_input_event;
 #define VIRTIO_INPUT_HOST_GET_PARENT_CLASS(obj) \
 OBJECT_GET_PARENT_CLASS(obj, TYPE_VIRTIO_INPUT_HOST)
 
+#define TYPE_VHOST_USER_INPUT   "vhost-user-input"
+#define VHOST_USER_INPUT(obj)  \
+OBJECT_CHECK(VHostUserInput, (obj), TYPE_VHOST_USER_INPUT)
+#define VHOST_USER_INPUT_GET_PARENT_CLASS(obj) \
+OBJECT_GET_PARENT_CLASS(obj, TYPE_VHOST_USER_INPUT)
+
 typedef struct VirtIOInput VirtIOInput;
 typedef struct VirtIOInputClass VirtIOInputClass;
 typedef struct VirtIOInputConfig VirtIOInputConfig;
 typedef struct VirtIOInputHID VirtIOInputHID;
 typedef struct VirtIOInputHost VirtIOInputHost;
+typedef struct VHostUserInput VHostUserInput;
 
 struct VirtIOInputConfig {
 virtio_input_config   config;
@@ -98,6 +106,12 @@ struct VirtIOInputHost {
 int   fd;
 };
 
+struct VHostUserInput {
+VirtIOInput   parent_obj;
+
+VhostUserBackend  *vhost;
+};
+
 void virtio_input_send(VirtIOInput *vinput, virtio_input_event *event);
 void virtio_input_init_config(VirtIOInput *vinput,
   virtio_input_config *config);
diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
new file mode 100644
index 00..41c25d2398
--- /dev/null
+++ b/hw/input/vhost-user-input.c
@@ -0,0 +1,99 @@
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+
+#include "hw/qdev.h"
+#include "hw/virtio/virtio-input.h"
+
+static void vhost_input_realize(DeviceState *dev, Error **errp)
+{
+VHostUserInput *vhi = VHOST_USER_INPUT(dev);
+VirtIOInput *vinput = VIRTIO_INPUT(dev);
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+virtio_input_config *config;
+int i, ret;
+
+if (vhost_user_backend_dev_init(vhi->vhost, vdev, 2, errp) == -1) {
+return;
+}
+
+ret = vhost_user_input_get_config(&vhi->vhost->dev, &config);
+if (ret < 0) {
+error_setg(errp, "failed to get input config");
+return;
+}
+for (i = 0; i < ret; i++) {
+virtio_input_add_config(vinput, &config[i]);
+}
+g_free(config);
+}
+
+static void vhost_input_change_active(VirtIOInput *vinput)
+{
+VHostUserInput *vhi = VHOST_USER_INPUT(vinput);
+
+if (vinput->active) {
+vhost_user_backend_start(vhi->vhost);
+} else {
+vhost_user_backend_stop(vhi->vhost);
+}
+}
+
+static const VMStateDescription vmstate_vhost_input = {
+.name = "vhost-user-input",
+.unmigratable = 1,
+};
+
+static void vhost_input_class_init(ObjectClass *klass, void *data)
+{
+VirtIOInputClass *vic = VIRTIO_INPUT_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
+
+dc->vmsd   = &vmstate_vhost_input;
+vic->realize   = vhost_input_realize;
+vic->change_active = vhost_input_change_active;
+}
+
+static void vhost_input_init(Object *obj)
+{
+VHostUserInput *vhi = VHOST_USER_INPUT(obj);
+VirtIOInput *vinput = VIRTIO_INPUT(obj);
+struct virtio_input_config vhost_input_config[] = { { /* empty list */ } };
+
+virtio_input_init_config(vinput, vhost_input_config);
+
+vhi->vhost = VHOST_USER_BACKEND(object_new(TYPE_VHOST_USER_BACKEND));
+object_property_add_alias(obj, "chardev",
+  OBJECT(vhi->vhost

[Qemu-devel] [PATCH v5 04/12] libvhost-user: exit by default on VHOST_USER_NONE

[Marc-André Lureau]

Since commit 2566378d6d13bf4d28c7770bdbda5f7682594bbe, libvhost-user
no longer panics on disconnect (rc == 0), and instead silently ignores
an invalid VHOST_USER_NONE message.

Without extra work from the API user, this will simply busy-loop on
HUP events. The obvious thing to do is to exit(0) instead, while
additional or different work can be done by overriding
iface->process_msg().

Signed-off-by: Marc-André Lureau 
Reviewed-by: Jens Freimann 
---
 contrib/libvhost-user/libvhost-user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index 3f14b4138b..fcf5014240 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -1285,7 +1285,8 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
 case VHOST_USER_SET_CONFIG:
 return vu_set_config(dev, vmsg);
 case VHOST_USER_NONE:
-break;
+/* if you need processing before exit, override iface->process_msg */
+exit(0);
 case VHOST_USER_POSTCOPY_ADVISE:
 return vu_set_postcopy_advise(dev, vmsg);
 case VHOST_USER_POSTCOPY_LISTEN:
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 05/12] vhost-user: wrap some read/write with retry handling

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/virtio/vhost-user.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 0164060b2b..3b09c40578 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -968,7 +968,10 @@ static void slave_read(void *opaque)
 iov.iov_base = &hdr;
 iov.iov_len = VHOST_USER_HDR_SIZE;
 
-size = recvmsg(u->slave_fd, &msgh, 0);
+do {
+size = recvmsg(u->slave_fd, &msgh, 0);
+} while (size < 0 && (errno == EINTR || errno == EAGAIN));
+
 if (size != VHOST_USER_HDR_SIZE) {
 error_report("Failed to read from slave.");
 goto err;
@@ -997,7 +1000,10 @@ static void slave_read(void *opaque)
 }
 
 /* Read payload */
-size = read(u->slave_fd, &payload, hdr.size);
+do {
+size = read(u->slave_fd, &payload, hdr.size);
+} while (size < 0 && (errno == EINTR || errno == EAGAIN));
+
 if (size != hdr.size) {
 error_report("Failed to read payload from slave.");
 goto err;
@@ -1045,7 +1051,10 @@ static void slave_read(void *opaque)
 iovec[1].iov_base = &payload;
 iovec[1].iov_len = hdr.size;
 
-size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
+do {
+size = writev(u->slave_fd, iovec, ARRAY_SIZE(iovec));
+} while (size < 0 && (errno == EINTR || errno == EAGAIN));
+
 if (size != VHOST_USER_HDR_SIZE + hdr.size) {
 error_report("Failed to send msg reply to slave.");
 goto err;
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 10/12] libvhost-user: add vu_queue_unpop()

[Marc-André Lureau]

vhost-user-input will make use of this function to undo some queue pop
in case the virtio queue does not have enough room.

Signed-off-by: Marc-André Lureau 
---
 contrib/libvhost-user/libvhost-user.h | 14 ++
 contrib/libvhost-user/libvhost-user.c | 16 
 2 files changed, 30 insertions(+)

diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index b0c798fa1a..01738bf44a 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -459,6 +459,20 @@ void vu_queue_notify(VuDev *dev, VuVirtq *vq);
  */
 void *vu_queue_pop(VuDev *dev, VuVirtq *vq, size_t sz);
 
+
+/**
+ * vu_queue_unpop:
+ * @dev: a VuDev context
+ * @vq: a VuVirtq queue
+ * @elem: The #VuVirtqElement
+ * @len: number of bytes written
+ *
+ * Pretend the most recent element wasn't popped from the virtqueue.  The next
+ * call to vu_queue_pop() will refetch the element.
+ */
+void vu_queue_unpop(VuDev *dev, VuVirtq *vq, VuVirtqElement *elem,
+size_t len);
+
 /**
  * vu_queue_rewind:
  * @dev: a VuDev context
diff --git a/contrib/libvhost-user/libvhost-user.c 
b/contrib/libvhost-user/libvhost-user.c
index fcf5014240..df72d3e440 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -1966,6 +1966,22 @@ vu_queue_pop(VuDev *dev, VuVirtq *vq, size_t sz)
 return elem;
 }
 
+static void
+vu_queue_detach_element(VuDev *dev, VuVirtq *vq, VuVirtqElement *elem,
+size_t len)
+{
+vq->inuse--;
+/* unmap, when DMA support is added */
+}
+
+void
+vu_queue_unpop(VuDev *dev, VuVirtq *vq, VuVirtqElement *elem,
+   size_t len)
+{
+vq->last_avail_idx--;
+vu_queue_detach_element(dev, vq, elem, len);
+}
+
 bool
 vu_queue_rewind(VuDev *dev, VuVirtq *vq, unsigned int num)
 {
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 07/12] vhost-user: split vhost_user_read()

[Marc-André Lureau]

Split vhost_user_read(), so only header can be read with
vhost_user_read_header().

Signed-off-by: Marc-André Lureau 
Reviewed-by: Daniel P. Berrangé 
---
 hw/virtio/vhost-user.c | 27 +++
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 3b09c40578..5df73405bc 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -214,7 +214,7 @@ static bool ioeventfd_enabled(void)
 return !kvm_enabled() || kvm_eventfds_enabled();
 }
 
-static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
+static int vhost_user_read_header(struct vhost_dev *dev, VhostUserMsg *msg)
 {
 struct vhost_user *u = dev->opaque;
 CharBackend *chr = u->user->chr;
@@ -225,7 +225,7 @@ static int vhost_user_read(struct vhost_dev *dev, 
VhostUserMsg *msg)
 if (r != size) {
 error_report("Failed to read msg header. Read %d instead of %d."
  " Original request %d.", r, size, msg->hdr.request);
-goto fail;
+return -1;
 }
 
 /* validate received flags */
@@ -233,7 +233,21 @@ static int vhost_user_read(struct vhost_dev *dev, 
VhostUserMsg *msg)
 error_report("Failed to read msg header."
 " Flags 0x%x instead of 0x%x.", msg->hdr.flags,
 VHOST_USER_REPLY_MASK | VHOST_USER_VERSION);
-goto fail;
+return -1;
+}
+
+return 0;
+}
+
+static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
+{
+struct vhost_user *u = dev->opaque;
+CharBackend *chr = u->user->chr;
+uint8_t *p = (uint8_t *) msg;
+int r, size;
+
+if (vhost_user_read_header(dev, msg) < 0) {
+return -1;
 }
 
 /* validate message size is sane */
@@ -241,7 +255,7 @@ static int vhost_user_read(struct vhost_dev *dev, 
VhostUserMsg *msg)
 error_report("Failed to read msg header."
 " Size %d exceeds the maximum %zu.", msg->hdr.size,
 VHOST_USER_PAYLOAD_SIZE);
-goto fail;
+return -1;
 }
 
 if (msg->hdr.size) {
@@ -251,14 +265,11 @@ static int vhost_user_read(struct vhost_dev *dev, 
VhostUserMsg *msg)
 if (r != size) {
 error_report("Failed to read msg payload."
  " Read %d instead of %d.", r, msg->hdr.size);
-goto fail;
+return -1;
 }
 }
 
 return 0;
-
-fail:
-return -1;
 }
 
 static int process_message_reply(struct vhost_dev *dev,
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 03/12] vhost-user: simplify vhost_user_init/vhost_user_cleanup

[Marc-André Lureau]

Take a VhostUserState* that can be pre-allocated, and initialize it
with the associated chardev.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Tiwei Bie 
---
 include/hw/virtio/vhost-user-blk.h  |  2 +-
 include/hw/virtio/vhost-user-scsi.h |  2 +-
 include/hw/virtio/vhost-user.h  |  2 +-
 backends/cryptodev-vhost-user.c | 18 --
 hw/block/vhost-user-blk.c   | 22 --
 hw/scsi/vhost-user-scsi.c   | 20 
 hw/virtio/vhost-stub.c  |  4 ++--
 hw/virtio/vhost-user.c  | 16 
 net/vhost-user.c| 13 -
 9 files changed, 33 insertions(+), 66 deletions(-)

diff --git a/include/hw/virtio/vhost-user-blk.h 
b/include/hw/virtio/vhost-user-blk.h
index d52944aeeb..a8a106eecb 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -36,7 +36,7 @@ typedef struct VHostUserBlk {
 uint32_t queue_size;
 uint32_t config_wce;
 struct vhost_dev dev;
-VhostUserState *vhost_user;
+VhostUserState vhost_user;
 } VHostUserBlk;
 
 #endif
diff --git a/include/hw/virtio/vhost-user-scsi.h 
b/include/hw/virtio/vhost-user-scsi.h
index e429cacd8e..738f9288bd 100644
--- a/include/hw/virtio/vhost-user-scsi.h
+++ b/include/hw/virtio/vhost-user-scsi.h
@@ -30,7 +30,7 @@
 
 typedef struct VHostUserSCSI {
 VHostSCSICommon parent_obj;
-VhostUserState *vhost_user;
+VhostUserState vhost_user;
 } VHostUserSCSI;
 
 #endif /* VHOST_USER_SCSI_H */
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index fd660393a0..811e325f42 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -22,7 +22,7 @@ typedef struct VhostUserState {
 VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX];
 } VhostUserState;
 
-VhostUserState *vhost_user_init(void);
+bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
 void vhost_user_cleanup(VhostUserState *user);
 
 #endif
diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index d539f14d59..1052a5d0e9 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -47,7 +47,7 @@
 typedef struct CryptoDevBackendVhostUser {
 CryptoDevBackend parent_obj;
 
-VhostUserState *vhost_user;
+VhostUserState vhost_user;
 CharBackend chr;
 char *chr_name;
 bool opened;
@@ -104,7 +104,7 @@ cryptodev_vhost_user_start(int queues,
 continue;
 }
 
-options.opaque = s->vhost_user;
+options.opaque = &s->vhost_user;
 options.backend_type = VHOST_BACKEND_TYPE_USER;
 options.cc = b->conf.peers.ccs[i];
 s->vhost_crypto[i] = cryptodev_vhost_init(&options);
@@ -182,7 +182,6 @@ static void cryptodev_vhost_user_init(
 size_t i;
 Error *local_err = NULL;
 Chardev *chr;
-VhostUserState *user;
 CryptoDevBackendClient *cc;
 CryptoDevBackendVhostUser *s =
   CRYPTODEV_BACKEND_VHOST_USER(backend);
@@ -213,15 +212,10 @@ static void cryptodev_vhost_user_init(
 }
 }
 
-user = vhost_user_init();
-if (!user) {
-error_setg(errp, "Failed to init vhost_user");
+if (!vhost_user_init(&s->vhost_user, &s->chr, errp)) {
 return;
 }
 
-user->chr = &s->chr;
-s->vhost_user = user;
-
 qemu_chr_fe_set_handlers(&s->chr, NULL, NULL,
  cryptodev_vhost_user_event, NULL, s, NULL, true);
 
@@ -307,11 +301,7 @@ static void cryptodev_vhost_user_cleanup(
 }
 }
 
-if (s->vhost_user) {
-vhost_user_cleanup(s->vhost_user);
-g_free(s->vhost_user);
-s->vhost_user = NULL;
-}
+vhost_user_cleanup(&s->vhost_user);
 }
 
 static void cryptodev_vhost_user_set_chardev(Object *obj,
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 44ac814016..767c734a81 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -253,7 +253,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 VHostUserBlk *s = VHOST_USER_BLK(vdev);
-VhostUserState *user;
 struct vhost_virtqueue *vqs = NULL;
 int i, ret;
 
@@ -272,15 +271,10 @@ static void vhost_user_blk_device_realize(DeviceState 
*dev, Error **errp)
 return;
 }
 
-user = vhost_user_init();
-if (!user) {
-error_setg(errp, "vhost-user-blk: failed to init vhost_user");
+if (!vhost_user_init(&s->vhost_user, &s->chardev, errp)) {
 return;
 }
 
-user->chr = &s->chardev;
-s->vhost_user = user;
-
 virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
 sizeof(struct virtio_blk_config));
 
@@ -297,7 +291,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, 
Error **errp)
 
 vhost_dev_set_config_notifier(&s->dev, &blk_ops);
 
-ret = vhost_dev_init(&s->dev, s->vhost_user, VHOST_BACKEND_

[Qemu-devel] [PATCH v5 02/12] vhost-user: define conventions for vhost-user backends

[Marc-André Lureau]

As discussed during "[PATCH v4 00/29] vhost-user for input & GPU"
review, let's define a common set of backend conventions to help with
management layer implementation, and interoperability.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Daniel P. Berrangé 
---
 MAINTAINERS  |   1 +
 docs/interop/vhost-user.json | 232 +++
 docs/interop/vhost-user.txt  | 101 ++-
 3 files changed, 332 insertions(+), 2 deletions(-)
 create mode 100644 docs/interop/vhost-user.json

diff --git a/MAINTAINERS b/MAINTAINERS
index 5040d9dfb1..1164f65875 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1451,6 +1451,7 @@ vhost
 M: Michael S. Tsirkin 
 S: Supported
 F: hw/*/*vhost*
+F: docs/interop/vhost-user.json
 F: docs/interop/vhost-user.txt
 F: contrib/vhost-user-*/
 
diff --git a/docs/interop/vhost-user.json b/docs/interop/vhost-user.json
new file mode 100644
index 00..ae88c03117
--- /dev/null
+++ b/docs/interop/vhost-user.json
@@ -0,0 +1,232 @@
+# -*- Mode: Python -*-
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# Authors:
+#  Marc-André Lureau 
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+
+##
+# = vhost user backend discovery & capabilities
+##
+
+##
+# @VHostUserBackendType:
+#
+# List the various vhost user backend types.
+#
+# @9p: 9p virtio console
+# @balloon: virtio balloon
+# @block: virtio block
+# @caif: virtio caif
+# @console: virtio console
+# @crypto: virtio crypto
+# @gpu: virtio gpu
+# @input: virtio input
+# @net: virtio net
+# @rng: virtio rng
+# @rpmsg: virtio remote processor messaging
+# @rproc-serial: virtio remoteproc serial link
+# @scsi: virtio scsi
+# @vsock: virtio vsock transport
+#
+# Since: 4.0
+##
+{
+  'enum': 'VHostUserBackendType',
+  'data': [
+  '9p',
+  'balloon',
+  'block',
+  'caif',
+  'console',
+  'crypto',
+  'gpu',
+  'input',
+  'net',
+  'rng',
+  'rpmsg',
+  'rproc-serial',
+  'scsi',
+  'vsock'
+  ]
+}
+
+##
+# @VHostUserBackendInputFeature:
+#
+# List of vhost user "input" features.
+#
+# @evdev-path: The --evdev-path command line option is supported.
+# @no-grab: The --no-grab command line option is supported.
+#
+# Since: 4.0
+##
+{
+  'enum': 'VHostUserBackendInputFeature',
+  'data': [ 'evdev-path', 'no-grab' ]
+}
+
+##
+# @VHostUserBackendCapabilitiesInput:
+#
+# Capabilities reported by vhost user "input" backends
+#
+# @features: list of supported features.
+#
+# Since: 4.0
+##
+{
+  'struct': 'VHostUserBackendCapabilitiesInput',
+  'data': {
+'features': [ 'VHostUserBackendInputFeature' ]
+  }
+}
+
+##
+# @VHostUserBackendGPUFeature:
+#
+# List of vhost user "gpu" features.
+#
+# @render-node: The --render-node command line option is supported.
+# @virgl: The --virgl command line option is supported.
+#
+# Since: 4.0
+##
+{
+  'enum': 'VHostUserBackendGPUFeature',
+  'data': [ 'render-node', 'virgl' ]
+}
+
+##
+# @VHostUserBackendCapabilitiesGPU:
+#
+# Capabilities reported by vhost user "gpu" backends.
+#
+# @features: list of supported features.
+#
+# Since: 4.0
+##
+{
+  'struct': 'VHostUserBackendCapabilitiesGPU',
+  'data': {
+'features': [ 'VHostUserBackendGPUFeature' ]
+  }
+}
+
+##
+# @VHostUserBackendCapabilities:
+#
+# Capabilities reported by vhost user backends.
+#
+# @type: The vhost user backend type.
+#
+# Since: 4.0
+##
+{
+  'union': 'VHostUserBackendCapabilities',
+  'base': { 'type': 'VHostUserBackendType' },
+  'discriminator': 'type',
+  'data': {
+'input': 'VHostUserBackendCapabilitiesInput',
+'gpu': 'VHostUserBackendCapabilitiesGPU'
+  }
+}
+
+##
+# @VhostUserBackend:
+#
+# Describes a vhost user backend to management software.
+#
+# It is possible for multiple @VhostUserBackend elements to match the
+# search criteria of management software. Applications thus need rules
+# to pick one of the many matches, and users need the ability to
+# override distro defaults.
+#
+# It is recommended to create vhost user backend JSON files (each
+# containing a single @VhostUserBackend root element) with a
+# double-digit prefix, for example "50-qemu-gpu.json",
+# "50-crosvm-gpu.json", etc, so they can be sorted in predictable
+# order. The backend JSON files should be searched for in three
+# directories:
+#
+#   - /usr/share/qemu/vhost-user -- populated by distro-provided
+#   packages (XDG_DATA_DIRS covers
+#   /usr/share by default),
+#
+#   - /etc/qemu/vhost-user -- exclusively for sysadmins' local additions,
+#
+#   - $XDG_CONFIG_HOME/qemu/vhost-user -- exclusively for per-user local
+# additions (XDG_CONFIG_HOME
+# defaults to $HOME/.config).
+#
+# Top-down, the list of directories goes from general to specific.
+#
+# Management software should build a list of files from all three
+# locatio

[Qemu-devel] [PATCH v5 01/12] libvhost-user: fix clang enum-conversion warning

[Marc-André Lureau]

Now that the VhostUserMsg.request field is used for both master &
slave requests, since commit d84599f56c820d8c1ac9928a76500dcdfbbf194d:

contrib/libvhost-user/libvhost-user.c:953:20: error: implicit conversion from 
enumeration type 'enum VhostUserSlaveRequest' to different enumeration type 
'VhostUserRequest' (aka 'enum VhostUserRequest') [-Werror,-Wenum-conversion]
.request = VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG,
   ^~~~

Signed-off-by: Marc-André Lureau 
---
 contrib/libvhost-user/libvhost-user.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 4aa55b4d2d..c0133b7f3f 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -145,7 +145,7 @@ typedef struct VhostUserVringArea {
 #endif
 
 typedef struct VhostUserMsg {
-VhostUserRequest request;
+int request;
 
 #define VHOST_USER_VERSION_MASK (0x3)
 #define VHOST_USER_REPLY_MASK   (0x1 << 2)
-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH v5 00/12] vhost-user-backend & vhost-user-input

[Marc-André Lureau]

Hi,

This series is based on previously discussed "[PATCH v4 00/29]
vhost-user for input & GPU" and "vhost-user: define conventions for
vhost-user backends" work. The GPU part is left off for now.

This series introduces a "vhost-user-backend": an helper object to be
used by vhost-user devices to ease with backend initialization and
handling. As a simple showcase, a "vhost-user-input-pci" device is
introduced, which can be used with the "contrib: add vhost-user-input"
example. vhost-user-input isn't meant to be installed, discovered or
used by libvirt: no installation is done (no vhost-user JSON file is
provided either).

thanks

v5:
- rebased
- removed useless "child" struct field leftover
- add Gerd r-b tags

v4:
- drop "RFC: add explicit can_migrate to vhost_user_backend_dev_init()"
- remove useless "cmd" struct field leftover

v3:
- add previously sent patch "libvhost-user: fix clang enum-conversion
  warning" to fix clang build
- "define conventions for vhost-user backends" updates after Eric review
- Drop user-creatable from vhost-user-backend
- Make vhost-user-input-pci take a chardev= (instead of vhost-user=)

v2:
- rebased (VhostUserInputPCI got most of the changes, due to split)
- added "RFC: add explicit can_migrate to
  vhost_user_backend_dev_init()" to attempt to address Michael
  concerns about migration.

Marc-André Lureau (12):
  libvhost-user: fix clang enum-conversion warning
  vhost-user: define conventions for vhost-user backends
  vhost-user: simplify vhost_user_init/vhost_user_cleanup
  libvhost-user: exit by default on VHOST_USER_NONE
  vhost-user: wrap some read/write with retry handling
  Add vhost-user-backend
  vhost-user: split vhost_user_read()
  vhost-user: add vhost_user_input_get_config()
  libvhost-user-glib: export vug_source_new()
  libvhost-user: add vu_queue_unpop()
  Add vhost-user-input-pci
  contrib: add vhost-user-input

 contrib/libvhost-user/libvhost-user-glib.h |   3 +
 contrib/libvhost-user/libvhost-user.h  |  17 +-
 include/hw/virtio/vhost-backend.h  |   4 +
 include/hw/virtio/vhost-user-blk.h |   2 +-
 include/hw/virtio/vhost-user-scsi.h|   2 +-
 include/hw/virtio/vhost-user.h |   2 +-
 include/hw/virtio/virtio-input.h   |  14 +
 include/sysemu/vhost-user-backend.h|  57 +++
 backends/cryptodev-vhost-user.c|  18 +-
 backends/vhost-user.c  | 209 +++
 contrib/libvhost-user/libvhost-user-glib.c |  11 +-
 contrib/libvhost-user/libvhost-user.c  |  19 +-
 contrib/vhost-user-input/main.c| 398 +
 hw/block/vhost-user-blk.c  |  22 +-
 hw/input/vhost-user-input.c|  99 +
 hw/scsi/vhost-user-scsi.c  |  20 +-
 hw/virtio/vhost-stub.c |   4 +-
 hw/virtio/vhost-user-input-pci.c   |  53 +++
 hw/virtio/vhost-user.c | 118 +-
 net/vhost-user.c   |  13 +-
 MAINTAINERS|   5 +
 Makefile   |   3 +
 Makefile.objs  |   1 +
 backends/Makefile.objs |   3 +
 configure  |   3 +
 contrib/vhost-user-input/Makefile.objs |   1 +
 default-configs/virtio.mak |   1 +
 docs/interop/vhost-user.json   | 232 
 docs/interop/vhost-user.txt| 109 +-
 hw/input/Makefile.objs |   1 +
 hw/virtio/Makefile.objs|   1 +
 31 files changed, 1359 insertions(+), 86 deletions(-)
 create mode 100644 include/sysemu/vhost-user-backend.h
 create mode 100644 backends/vhost-user.c
 create mode 100644 contrib/vhost-user-input/main.c
 create mode 100644 hw/input/vhost-user-input.c
 create mode 100644 hw/virtio/vhost-user-input-pci.c
 create mode 100644 contrib/vhost-user-input/Makefile.objs
 create mode 100644 docs/interop/vhost-user.json

-- 
2.21.0.4.g36eb1cb9cf

[Qemu-devel] [PATCH] vhost-user-test: fix leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau 
---
 tests/vhost-user-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 4cd0a97f13..9364227ba4 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -588,6 +588,7 @@ static void test_server_free(TestServer *server)
 g_test_message("unable to rmdir: path (%s): %s",
server->tmpfs, strerror(errno));
 }
+g_free(server->tmpfs);
 
 qemu_chr_fe_deinit(&server->chr, true);
 
@@ -605,6 +606,8 @@ static void test_server_free(TestServer *server)
 
 g_main_loop_unref(server->loop);
 g_main_context_unref(server->context);
+g_cond_clear(&server->data_cond);
+g_mutex_clear(&server->data_mutex);
 g_free(server);
 }
 
-- 
2.21.0.4.g36eb1cb9cf

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 06/11] sam460ex: Don't size flash memory to match backing image

[BALATON Zoltan]

Hello,

On Tue, 5 Mar 2019, Philippe Mathieu-Daudé wrote:

On 2/26/19 8:34 PM, Markus Armbruster wrote:

Machine "sam460ex" maps its flash memory at address 0xFFF0.  When
no image is supplied, its size is 1MiB (0x10), and 512KiB of ROM
get mapped on top of its second half.  Else, it's the size of the
image rounded up to the next multiple of 64KiB.

The rounding is actually useless: pflash_cfi01_realize() fails with
"failed to read the initial flash content" unless it's a no-op.

I have no idea what happens when the pflash's size exceeds 1MiB.
Useful outcomes seem unlikely.


With PFlashCFI02, it depends of the @nb_mappings parameter, which tries
to emulates how the bus connects the pflash (which address lines are
connected).

PFlashCFI01 doesn't support the feature to remap its content in aliases
(which might look unfortunate, because boards end doing it, in different
ways).


I think this is all theoretical at the moment since we don't actually 
model the flash functions of this board (at least I haven't tested that at 
all) and unless it somehow uses it in ways I'm unaware of I think 
currently only the ROM is used.



For this device we have:

 (qemu) info mtree
 - (prio 0, i/o): system
 0004fff0-0004 (prio 0, romd): sam460ex.flash

I'm not familiar with this arch/machine, let's assume the system bus is
32bit, and the flash has a 8bit word (we have 8 data lines connected to
the pflash).


Maybe this can help:
https://datasheet.octopart.com/PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf
http://www.acube-systems.biz/index.php?page=hardware&pid=5

Unfortunately I don't have any more detailed docs where it's explained 
more but according to the above and in my limited understanding the SoC 
could handle larger flash chips but this board has 512 MB. We have not 
changed it now because I'm not sure if it would break anything and I don't 
have time to test it so Marcus just added a comment to remind about this 
and we're happy with that for now and could come back to it separately.



The 'no image' is 1MiB.

1 MiB = 8 Mbit
8 Mbit / 32 = 2 ^ 18
We need 18 address lines to reach the whole flash.

What happens if we connect a 2MiB flash? We need 19 addr lines.

If we only have 18 lines to connect our flash, we can hardwire our last
line as 0 or 1.

- line #17 hardwired as 0:
Only the bottom part of the flash is accessible (range 0x00..0x0f).
CPU reading 0x4fff0 read flash offset 0x0.
Using CFI it is still a announced as 2MiB.

- line #17 hardwired as 1:
Only the top part of the flash is accessible (range 0x10..0x1f).
Can we trigger any operation from the internal state machine (writing to
address 0x555, named @unlock_addr0 in QEMU) since all access are
hardwardwired on top of 1MiB...?
Yes we can, because the pflash only uses 11 bits for it's I/O, so all
writes are masked and hit the I/O internal unit.
CPU reading 0x4fff0 read flash offset 0x10

If we do have 19 lines dedicated to our chip and connect a 512KiB flash,
we 'll use 17 lines and let 2 lines unused.
Regardless the values on the lines #17 and #18, the flash will answer to
the value on lines #0..#16. This might be called MMIO aliasing, and is
what setup the @nb_mappings argument.
This example with nb_mappings=4 would mean:
"I have a 2MiB I/O space and a 512KiB flash, map it and create 3 aliases".

Back to the architecture, what matters here is that the CPU reset vector
is always user-controlled (mapped on a flash device).
This arch has reset_vector @0x4fffc.
You could also map a 256KiB pflash at 0x4fffc, as long as the reset
vector is covered. If you map it at 0x4fff0 or 0x4fff8 it won't!

This explanation is not arch-specific (adapting the reset vector to each
arch).



I guess memory at the end of the address space remains unmapped when
it's smaller than 1MiB.  Again, useful outcomes seem unlikely.


If you map a 512KiB flash at 0x4fff0, then the reset vector is not
covered. At 0x4fff8 it is.


Yes, I said the same before but this would be a separate patch and would 
need more testing so it wasn't included in this series. Since it works as 
it is now this can wait until later when it can be cleaned up. If we want 
to model the actual board we don't have to consider different flash sizes 
than the 512 MB that the board has so everything else is probably 
"overthinking" it.


Regards,
BALATON Zoltan




The physical hardware appears to have 512KiB of flash memory:
https://eu.mouser.com/datasheet/2/268/atmel_AT49BV040B-1180330.pdf

For now, just set the flash memory size to 1MiB regardless of image
size, and document the mess.

Cc: BALATON Zoltan 
Signed-off-by: Markus Armbruster 
---
 hw/ppc/sam460ex.c | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 75250d49e4..0c919529f8 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -91,32 +91

Re: [Qemu-devel] [PULL v1 0/2] Merge tpm 2019/02/25 v1

[Stefan Berger]

On 3/4/19 11:43 AM, Peter Maydell wrote:

On Mon, 4 Mar 2019 at 15:47, Stefan Berger  wrote:

The 1st patch in this series fixes a bug in the TPM TIS emulator that
skipped a locality when resetting the seize flag when a higher locality
seized access from a lower locality. The 2nd patch converts debugging
code to tracing.

Stefan


The following changes since commit 8eb29f1bf5a974dc4c11d2d1f5e7c7f7a62be116:

   Merge remote-tracking branch 'remotes/awilliam/tags/vfio-updates-20190221.0' 
into staging (2019-02-22 15:48:04 +)

are available in the Git repository at:

   git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2019-02-25-1

for you to fetch changes up to cd38cc519b60bbe5dd8d85f6aaa72b2d7902930b:

   tpm_tis: convert tpm_tis_show_buffer() to use trace event (2019-02-24 
14:46:14 -0500)


Liam Merwick (2):
   tpm_tis: fix loop that cancels any seizure by a lower locality
   tpm_tis: convert tpm_tis_show_buffer() to use trace event

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.



Done.




-- PMM

[Qemu-devel] [PATCH for-3.0.x/PATCH for-3.0.1] tpm_tis: fix loop that cancels any seizure by a lower locality

[Stefan Berger]

From: Liam Merwick 

This patch is rev 37b55d67c0 from master.

In tpm_tis_mmio_write() if the requesting locality is seizing
access, any seizure by a lower locality is cancelled.  However the
loop doing the seizure had an off-by-one error and the locality
immediately preceding the requesting locality was not being cleared.
This is fixed by adjusting the test in the for loop to check the
localities up to the requesting locality.

Signed-off-by: Liam Merwick 
Reviewed-by: Stefan Berger 
Signed-off-by: Stefan Berger 
---
 hw/tpm/tpm_tis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d9322692ee..f4c1b6e813 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -616,7 +616,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
 }
 
 /* cancel any seize by a lower locality */
-for (l = 0; l < locty - 1; l++) {
+for (l = 0; l < locty; l++) {
 s->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
 }
 
-- 
2.17.2

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4] spapr-rtas: add ibm, get-vpd RTAS interface

[Maxiwell S. Garcia]

Hi Murilo,

On Tue, Mar 05, 2019 at 04:39:38PM -0300, Murilo Opsfelder Araujo wrote:
> Hi, Maxiwell.
> 
> On Thu, Feb 28, 2019 at 05:04:37PM -0300, Maxiwell S. Garcia wrote:
> > This adds a handler for ibm,get-vpd RTAS calls, allowing pseries guest
> > to collect host information. It is disabled by default to avoid unwanted
> > information leakage. To enable it, use:
> >
> > ‘-M 
> > pseries,host-serial={passthrough|string},host-model={passthrough|string}’
> >
> > Only the SE and TM keywords are returned at the moment.
> > SE for Machine or Cabinet Serial Number and
> > TM for Machine Type and Model
> >
> > The SE and TM keywords are controlled by 'host-serial' and 'host-model'
> > options, respectively. In the case of 'passthrough', the SE shows the
> > host 'system-id' information and the TM shows the host 'model' information.
> >
> > Powerpc-utils tools can dispatch RTAS calls to retrieve host
> > information using this ibm,get-vpd interface. The 'host-serial'
> > and 'host-model' nodes of device-tree hold the same information but
> > in a static manner, which is useless after a migration operation.
> >
> > Signed-off-by: Maxiwell S. Garcia 
> > ---
> >  hw/ppc/spapr_rtas.c| 121 +
> >  include/hw/ppc/spapr.h |  17 +-
> >  2 files changed, 137 insertions(+), 1 deletion(-)
> >
> > Update v4:
> > * Allows enable/disable host-serial and host-model options individually
> >
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 7a2cb786a3..785c453eb6 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -287,6 +287,123 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU 
> > *cpu,
> >  rtas_st(rets, 0, ret);
> >  }
> >
> > +static inline int vpd_st(target_ulong addr, target_ulong len,
> > + const void *val, uint16_t val_len)
> > +{
> > +hwaddr phys = ppc64_phys_to_real(addr);
> > +if (len < val_len) {
> > +return RTAS_OUT_PARAM_ERROR;
> > +}
> > +cpu_physical_memory_write(phys, val, val_len);
> > +return RTAS_OUT_SUCCESS;
> > +}
> > +
> > +static inline void vpd_ret(target_ulong rets, const int status,
> > +   const int next_seq_number, const int 
> > bytes_returned)
> > +{
> > +rtas_st(rets, 0, status);
> > +rtas_st(rets, 1, next_seq_number);
> > +rtas_st(rets, 2, bytes_returned);
> > +}
> > +
> > +static void rtas_ibm_get_vpd_register_keywords(sPAPRMachineState *sm)
> > +{
> > +sm->rtas_vpd_keywords = g_malloc0(sizeof(uint8_t) *
> > +RTAS_IBM_VPD_KEYWORDS_MAX);
> > +
> > +int i = 0;
> > +if (sm->host_serial && !g_str_equal(sm->host_serial, "none")) {
> > +sm->rtas_vpd_keywords[i++] = RTAS_IBM_VPD_KEYWORD_SE;
> > +}
> > +if (sm->host_model && !g_str_equal(sm->host_model, "none")) {
> > +sm->rtas_vpd_keywords[i++] = RTAS_IBM_VPD_KEYWORD_TM;
> > +}
> > +}
> > +
> > +static void rtas_ibm_get_vpd(PowerPCCPU *cpu,
> > + sPAPRMachineState *spapr,
> > + uint32_t token, uint32_t nargs,
> > + target_ulong args,
> > + uint32_t nret, target_ulong rets)
> > +{
> > +target_ulong loc_code_addr;
> > +target_ulong work_area_addr;
> > +target_ulong work_area_size;
> > +target_ulong seq_number;
> > +unsigned char loc_code = 0;
> > +unsigned int next_seq_number = 1;
> > +int status = RTAS_IBM_GET_VPD_PARAMETER_ERROR;
> > +int ret = RTAS_OUT_PARAM_ERROR;
> > +char *buf = NULL;
> > +char *vpd_field = NULL;
> > +unsigned int vpd_field_len = 0;
> > +
> > +/* RTAS not authorized if no keywords have been registered */
> > +if (!spapr->rtas_vpd_keywords[0]) {
> > +vpd_ret(rets, RTAS_OUT_NOT_AUTHORIZED, 1, 0);
> > +return;
> > +}
> > +
> > +loc_code_addr = rtas_ld(args, 0);
> > +work_area_addr = rtas_ld(args, 1);
> > +work_area_size = rtas_ld(args, 2);
> > +seq_number = rtas_ld(args, 3);
> > +
> > +/* Specific Location Code is not supported and seq_number */
> > +/* must be checked to avoid out of bound index error */
> > +cpu_physical_memory_read(loc_code_addr, &loc_code, 1);
> > +if ((loc_code != 0) || (seq_number <= 0) ||
> > +(seq_number > RTAS_IBM_VPD_KEYWORDS_MAX)) {
> > +vpd_ret(rets, RTAS_IBM_GET_VPD_PARAMETER_ERROR, 1, 0);
> > +return;
> > +}
> > +
> > +switch (spapr->rtas_vpd_keywords[seq_number - 1]) {
> 
> Since seq_number comes from guest userspace, we shouldn't rely its
> value will always be within the range of the array.
> 
> We could perhaps return RTAS_OUT_NOT_AUTHORIZED if `seq_number - 1`
> extrapolates the array range that holds the registered keywords.
> 

The seq_number is tested in the 'if' statement before access
the rtas_vpd_keywords array and it's returns RTAS_IBM_GET_VPD_PARAMETER_ERROR
in case of out of bound index.

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

[Philippe Mathieu-Daudé]

On 3/5/19 6:25 PM, Peter Maydell wrote:
> On Tue, 5 Mar 2019 at 17:21, Philippe Mathieu-Daudé  wrote:
>> But I'd recommend changing/fixing the sector size during the next dev
>> cycle, so we have more time for testing.
> 
> Nobody in the upstream dev community is using or testing this board.

Well I submitted a Avocado test last year:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02749.html

And I rebase/run it from time to time.

> The only way we'll find out if there's a problem with changing the
> sector size is to put the change in and get it into a release.
> I would vote for just making the change now.

I'm happy with this vote, and am sure Markus will be too :)

Markus: the last field I wasn't sure about without double checking the
code is the @width one. I find it misleading, is that the size of the
data bus or the size of the flash words? Answer: this is the size of the
words in byte. NOR flash devices can not write less data than their word
boundary.

The S29PL127J60TFI130 only support 16bit words, so using @width=2 is
correct.

And the winner is ta-da!

pflash_cfi02_register(0x0, NULL, "r2d.flash", FLASH_SIZE,
  dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
- 16 * KiB, FLASH_SIZE >> 16,
- 1, 4, 0x, 0x, 0x, 0x,
+ 64 * KiB, FLASH_SIZE >> 16 /* will get removed
later */,
+ 1, 2, 0x0001, 0x227e, 0x2220, 0x2200
  0x555, 0x2aa, 0);

> 
> thanks
> -- PMM

Thanks for your support!

Phil.

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

[Richard Henderson]

On 3/5/19 9:38 AM, Mark Cave-Ayland wrote:
> It's really that this is a stepping stone to patch 7 where you end up with 
> this:
> 
> static inline int vsrh_offset(int i)
> {
> return offsetof(CPUPPCState, vsr[i].VsrD(0));
> }
> 
> static inline int vsrl_offset(int i)
> {
> return offsetof(CPUPPCState, vsr[i].VsrD(1));
> }
> 
> ...
> 
> static inline int avrh_offset(int i)
> {
> return vsrh_offset(i + 32);
> }
> 
> static inline int avrl_offset(int i)
> {
> return vsrl_offset(i + 32);
> }

Yes, but the only users look like...

> tcg_gen_ld_i64(dst, cpu_env, high ? avrh_offset(regno) : 
> avrl_offset(regno));

... this.  And to me that suggests that one helper instead of two would be
better, so that you can write

  tcg_gen_ld_i64(dst, cpu_env, avr64_offset(regno, high));

and propagate the "high" into the VsrD argument.

> which looks a bit easier on the eye? I'm less keen on pushing the "high" bool 
> all the
> way down into offset functions as I find the separate vsrh/vsrl functions 
> much easier
> to read in the helpers than the get_avr64() version.

Ah.  Hmm.

Will we not in the end require a function A64's

  vec_reg_offset(regno, element, size)

I know DavidH did when writing the s390x vector support last week.  Perhaps
that's more palatable than avr64_offset that only supports 64-bit elements?


r~

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

[Richard Henderson]

On 3/5/19 9:16 AM, Mark Cave-Ayland wrote:
> Really though I don't feel too strongly about this, so would you like me to
> rename it avr_full_offset() to match the existing vsr_full_offset()?
I do think matching the names makes things clearer.


r~

Re: [Qemu-devel] [PULL 0/7] QAPI patches for 2019-03-05

[Peter Maydell]

On Tue, 5 Mar 2019 at 17:07, Markus Armbruster  wrote:
>
> The following changes since commit 0984a157c1c053394adbf64ed7de97f1aebe6a2d:
>
>   Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into 
> staging (2019-03-05 09:33:20 +)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2019-03-05
>
> for you to fetch changes up to 56a4689582433125d7042ba506a081e08dc264d4:
>
>   qapi: Fix array first used in a different module (2019-03-05 14:43:11 +0100)
>
> 
> QAPI patches for 2019-03-05
>
> 
> Markus Armbruster (7):
>   tests/qapi-schema: Make test-qapi.py print arrays
>   tests/qapi-schema: Cover conditional arrays
>   qapi: Pass file name to QAPIGen constructor instead of methods
>   qapi: Fix code generation for sub-modules in other directories
>   tests: Rename UserDefNativeListUnion to UserDefListUnion
>   tests/qapi-schema: Cover forward reference to sub-module
>   qapi: Fix array first used in a different module
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM

Re: [Qemu-devel] [PATCH v5] hw/block: better reporting on pflash backing file mismatch

[Laszlo Ersek]

On 03/05/19 16:33, Markus Armbruster wrote:
> You neglected to cc: the maintainers of hw/block, I fixed that for you.
> 
> Alex Bennée  writes:
> 
>> It looks like there was going to be code to check we had some sort of
>> alignment so lets replace it with an actual check. This is a bit more
>> useful than the enigmatic "failed to read the initial flash content"
>> when we attempt to read the number of bytes the device should have.
>>
>> This is a potential confusing stumbling block when you move from using
>> -bios to using -drive if=pflash,file=blob,format=raw,readonly for
>> loading your firmware code. To mitigate that we automatically pad in
>> the read-only case and warn the user when we have performed magic to
>> enable things to Just Work (tm).
>>
>> Signed-off-by: Alex Bennée 
>> Reviewed-by: Laszlo Ersek 
> 
> Philippe and I talked about various pflash issues last night.  He
> explained to me how physical flash memory works and is used.  This
> brought back my doubts on the wisdom of automatic padding.
> 
> Errors in my recounting of his explanations are almost certainly
> entirely mine.  Please correct them.
> 
> We're talking about NOR flash.  NAND flash works differently.
> 
> You can:
> 
> * Read a cell.
> 
> * Write a cell: change it from 1 to 0.
> 
> * Erase a whole sector (block): change all cells to 1.  This is slow,
>   burns power, and you can do it only so often before the flash wears
>   out
> 
> Say your physical machine has 1 MiB of NOR flash in 16 sectors of 64 KiB
> each (unrealistic, as Philippe has pointed out elsewhere, but it'll do
> here).  You compile your firmware, and the build process spits out a
> flat image of 20 bytes.  Here are a few distinct ways to deploy it
> to your freshly erased flash memory:
> 
> (1) You write your image to the flash.  Everything after byte 20
> remains writable.  This is nice for development.  With a bit of
> ingenuity, you can come up with a patching scheme that lets you avoid
> rewriting the whole flash for every little fix, saving flash wear.
> 
> (2) You zero-pad your image to the full flash size, and write that to
> the flash.  Everything after byte 20 becomes unwritable.  You can't
> erase the first 4 blocks (they hold your firmware), but you can still
> erase the remaining 12.
> 
> (3) You zero-pad your image to the next sector boundary, and write that
> to the flash.  The remainder of block 4 becomes unwritable (and you
> can't erase the block without destroying your firmware).  The remaining
> 12 blocks remain writable.  This is commonly done for production,
> because it reduces the ways a sector holding code can be corrupted,
> making its checksum invalid.
> 
> My point is: in the physical world, there is no single true way to pad.
> 
> Back to your patch.  I think it conflates three changes:
> 
> * We reject an undersized image with a sub-optimal error message.
>   Improve that message.
> 
> * We silently ignore an oversized image's tail.  Warn instead.
> 
> * As a convenience feature, don't reject undersized read-only image, but
>   pad it with 0xff instead, to simulate (1) above.
> 
> Squashing the first two under a "better reporting on pflash backing file
> mismatch" heading seems fine to me.  The last one is not about "better
> reporting", and should therefore be a separate patch.
> 
> I'm willing to do the split in the respin of my pflash fixes series.
> 
> For the record, I'd summarily reject oversized images,

Rejection is not a bad idea IMO; I don't remember any use case where the
user benefits from the acceptance of an oversized image (with or without
warning).

> and I'd drop the
> convenience feature, but I'm not the maintainer here.  It's up to Kevin
> and Max.

Auto-padding can save some space wherever a raw image is provided, even
when QEMU is used through libvirt. It's not hugely important IMO but
nice to have. (Especially if we decide *not* to describe pflash block
count and size traits in the firmware descriptor files.)

Thanks
Laszlo

> 
>> ---
>> v3
>>   - tweak commit title/commentary
>>   - use total_len instead of device_len for checks
>>   - if the device is read-only do the padding for them
>>   - accept baking_len > total_len (how to warn_report with NULL *errp?)
>> v4
>>   - error check blk_getlength
>>   - optimise memset and use NOR erase pattern
>>   - restore singular device (overly confusing)
>>   - add warn_report for when we do magic
>> v5
>>   - remove mention of null padding
>>   - use %zu for size_t fmt string
>>   - add Laszlo r-b
>> ---
>>  hw/block/pflash_cfi01.c | 40 +---
>>  1 file changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9d1c356eb6..d8cfa4789a 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -45,6 +45,7 @@
>>  #include "qemu/bitops.h"
>>  #include "qemu/host-utils.h"
>>  #include "qemu/log.h"
>> +#include "qemu/error-report.h"
>>  #includ

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios()

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 12/10/18 5:02 PM, Laszlo Ersek wrote:
> On 12/07/18 18:03, Philippe Mathieu-Daudé wrote:
>> Since af1f60a4022, the fw_cfg field is always created in machvirt_init().
>> There is no need to null check it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/arm/virt.c | 4 
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f69e7eb399..36303ed59c 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms)
>>  size_t smbios_tables_len, smbios_anchor_len;
>>  const char *product = "QEMU Virtual Machine";
>>  
>> -if (!vms->fw_cfg) {
>> -return;
>> -}
>> -
>>  if (kvm_enabled()) {
>>  product = "KVM Virtual Machine";
>>  }
>>
> 
> This patch looks correct to me; however, the fact that fw_cfg is always
> present on the virt board does not seem related to commit af1f60a4022
> af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said
> commit only flattened the contents of VirtGuestInfo into
> VirtMachineState; the conditions under which fw_cfg would be created
> were not changed.

This is certainly not related to commit af1f60a4022... This is not the
commit I remember I wanted to refer to, I suppose I did a copy/paste
mistake, thanks for carefully reviewing me and catching this!

> As far as I can see (from a number of git-blame / git-show commands),
> fw_cfg was added unconditionally to the virt board in commit
> 578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the
> check that you are now (correctly) removing has always been superfluous
> -- it was superfluous already when it was first added in commit
> c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07).
> 
> As far as I can tell, anyway.
> 
> If you agree, I suggest mentioning commit c30e15658b1b in the commit
> message, rather than commit af1f60a4022.

Yes, this one makes more sense ;)

> 
> With such an update:
> 
> Reviewed-by: Laszlo Ersek 

Thanks!

Phil.

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH v4 00/12] vhost-user-backend & vhost-user-input

[Michael S. Tsirkin]

On Wed, Feb 27, 2019 at 11:30:05AM +0100, Marc-André Lureau wrote:
> Hi,
> 
> This series is based on previously discussed "[PATCH v4 00/29]
> vhost-user for input & GPU" and "vhost-user: define conventions for
> vhost-user backends" work. The GPU part is left off for now.
> 
> This series introduces a "vhost-user-backend": an helper object to be
> used by vhost-user devices to ease with backend initialization and
> handling. As a simple showcase, a "vhost-user-input-pci" device is
> introduced, which can be used with the "contrib: add vhost-user-input"
> example. vhost-user-input isn't meant to be installed, discovered or
> used by libvirt: no installation is done (no vhost-user JSON file is
> provided either).
> 
> thanks
can you rebase on top of master pls?

> v4:
> - drop "RFC: add explicit can_migrate to vhost_user_backend_dev_init()"
> - remove useless "cmd" struct field leftover
> 
> v3:
> - add previously sent patch "libvhost-user: fix clang enum-conversion
>   warning" to fix clang build
> - "define conventions for vhost-user backends" updates after Eric review
> - Drop user-creatable from vhost-user-backend
> - Make vhost-user-input-pci take a chardev= (instead of vhost-user=)
> 
> v2:
> - rebased (VhostUserInputPCI got most of the changes, due to split)
> - added "RFC: add explicit can_migrate to
>   vhost_user_backend_dev_init()" to attempt to address Michael
>   concerns about migration.
> 
> Marc-André Lureau (12):
>   libvhost-user: fix clang enum-conversion warning
>   vhost-user: define conventions for vhost-user backends
>   vhost-user: simplify vhost_user_init/vhost_user_cleanup
>   libvhost-user: exit by default on VHOST_USER_NONE
>   vhost-user: wrap some read/write with retry handling
>   Add vhost-user-backend
>   vhost-user: split vhost_user_read()
>   vhost-user: add vhost_user_input_get_config()
>   libvhost-user-glib: export vug_source_new()
>   libvhost-user: add vu_queue_unpop()
>   Add vhost-user-input-pci
>   contrib: add vhost-user-input
> 
>  contrib/libvhost-user/libvhost-user-glib.h |   3 +
>  contrib/libvhost-user/libvhost-user.h  |  17 +-
>  include/hw/virtio/vhost-backend.h  |   4 +
>  include/hw/virtio/vhost-user-blk.h |   2 +-
>  include/hw/virtio/vhost-user-scsi.h|   2 +-
>  include/hw/virtio/vhost-user.h |   2 +-
>  include/hw/virtio/virtio-input.h   |  14 +
>  include/sysemu/vhost-user-backend.h|  58 +++
>  backends/cryptodev-vhost-user.c|  18 +-
>  backends/vhost-user.c  | 213 +++
>  contrib/libvhost-user/libvhost-user-glib.c |  11 +-
>  contrib/libvhost-user/libvhost-user.c  |  19 +-
>  contrib/vhost-user-input/main.c| 398 +
>  hw/block/vhost-user-blk.c  |  22 +-
>  hw/input/vhost-user-input.c|  99 +
>  hw/scsi/vhost-user-scsi.c  |  20 +-
>  hw/virtio/vhost-stub.c |   4 +-
>  hw/virtio/vhost-user-input-pci.c   |  53 +++
>  hw/virtio/vhost-user.c | 118 +-
>  net/vhost-user.c   |  13 +-
>  MAINTAINERS|   5 +
>  Makefile   |   3 +
>  Makefile.objs  |   1 +
>  backends/Makefile.objs |   3 +-
>  configure  |   3 +
>  contrib/vhost-user-input/Makefile.objs |   1 +
>  default-configs/virtio.mak |   1 +
>  docs/interop/vhost-user.json   | 232 
>  docs/interop/vhost-user.txt| 109 +-
>  hw/input/Makefile.objs |   1 +
>  hw/virtio/Makefile.objs|   1 +
>  31 files changed, 1363 insertions(+), 87 deletions(-)
>  create mode 100644 include/sysemu/vhost-user-backend.h
>  create mode 100644 backends/vhost-user.c
>  create mode 100644 contrib/vhost-user-input/main.c
>  create mode 100644 hw/input/vhost-user-input.c
>  create mode 100644 hw/virtio/vhost-user-input-pci.c
>  create mode 100644 contrib/vhost-user-input/Makefile.objs
>  create mode 100644 docs/interop/vhost-user.json
> 
> -- 
> 2.21.0

Re: [Qemu-devel] [PULL 00/54] Kconfig conversion, excluding ARM and MIPS

[Philippe Mathieu-Daudé]

On 3/5/19 9:08 PM, Eric Blake wrote:
> On 3/5/19 1:45 PM, Thomas Huth wrote:
> 
>>> If there are special instructions for what to do with
>>> build trees over the transition to kconfig, the pullreq
>>> cover letter would be a good place to mention them :-)
>>
>> I think you've got to do a "make distclean" inbetween... that's the old
>> problem when a default-configs/*.mak file gets added or erased - we do
>> not properly re-generate the dependencies in that case.
> 
> As in this?
> 
> upgrade path:
> build old commit
> make distclean
> git pull/branch/...

make distclean again?

> build new commit
> 
> downgrade path (when bisecting, backporting, ...)
> build new commit
> make distclean
> git branch/reset/...

make distclean again?

> build old commit
> 
> We obviously can't fix old commits to recognize when we are downgrading
> from a new commit, but is there anything we can do when upgrading to a
> newer commit to more gracefully inform the user if they forgot a 'make
> distclean' (or even better, to not make a 'make distclean' on upgrade
> mandatory)?  In particular, once this patch series lands, developers
> doing a blind 'git pull' will end up in the situation:
> 
> build old commit
> git pull
> build new commit # oops
> 
> but may not realize that they first have to reset back to the old commit
> prior to 'make distclean' to guarantee that it will work. Unless I'm
> mistaken and 'make distclean' on an incremental build will work in spite
> of the missing dependencies on *.mak files even when you forgot to clean
> before upgrading.
> 
> 'make distclean' is a heavy hammer, is there anything smaller in scope
> that will fix the problem without nuking everything, such as a strategic
> touch or rm of one particular file?
> 

Re: [Qemu-devel] [PATCH v2] qemu-binfmt-conf.sh: add CPUS, add --reset, make -p and -c boolean (no arg)

[Unai Martinez Corral]

2019/3/5 17:57, Eric Blake:
> I don't have a strong preference between the two forms - so this is your
> chance to assert your creative liberty. But I don't know enough about
> binfmt_misc to know if the newline matters. I'm just reviewing on the
> basis of shell portability, and hope that other reviewers more familiar
> with binfmt will review on content.

According to 
https://www.kernel.org/doc/Documentation/admin-guide/binfmt-misc.rst
it seems that only the integer matters. However, I'd also like to hear
any other thoughts.

> you may want to split this into a series of
> patches for v3, although you could also wait a day or so to see if
> anyone else reviews and minimize the list churn.

I will do so. It will take some time to reorganize the content and
apply the latest fixes.

> Welcome to the qemu community, and I hope you find your experience with
> your first patch pleasant.

Thanks! I'm really struggling to find a proper mail client, since I'm
on Windows and none of the web clients work properly for me.
But I hope I'll find some workaround.

> Long subject line, I'd trim ' (no arg)'. The commit body itself should
> go into the "why" the patch is useful (what behaviors are you fixing,
> what risk of backwards-compatibility breaks do we have to contend with
> for older clients of the script, etc).

Will do.

>  Also, a v2 patch is best sent as
> a new top-level thread, rather than in-reply to the v1 (as some of our
> automated CI tooling misses it otherwise).

I'm sorry I missed this explanation.

Thanks again for your time and suggestions.

Unai

Re: [Qemu-devel] [PULL 00/54] Kconfig conversion, excluding ARM and MIPS

[Eric Blake]

On 3/5/19 1:45 PM, Thomas Huth wrote:


If there are special instructions for what to do with
build trees over the transition to kconfig, the pullreq
cover letter would be a good place to mention them :-)


I think you've got to do a "make distclean" inbetween... that's the old
problem when a default-configs/*.mak file gets added or erased - we do
not properly re-generate the dependencies in that case.


As in this?

upgrade path:
build old commit
make distclean
git pull/branch/...
build new commit

downgrade path (when bisecting, backporting, ...)
build new commit
make distclean
git branch/reset/...
build old commit

We obviously can't fix old commits to recognize when we are downgrading 
from a new commit, but is there anything we can do when upgrading to a 
newer commit to more gracefully inform the user if they forgot a 'make 
distclean' (or even better, to not make a 'make distclean' on upgrade 
mandatory)?  In particular, once this patch series lands, developers 
doing a blind 'git pull' will end up in the situation:


build old commit
git pull
build new commit # oops

but may not realize that they first have to reset back to the old commit 
prior to 'make distclean' to guarantee that it will work. Unless I'm 
mistaken and 'make distclean' on an incremental build will work in spite 
of the missing dependencies on *.mak files even when you forgot to clean 
before upgrading.


'make distclean' is a heavy hammer, is there anything smaller in scope 
that will fix the problem without nuking everything, such as a strategic 
touch or rm of one particular file?


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

[Qemu-devel] [PATCH v2 5/5] lsi: return dfifo value

[Sven Schnelle]

Code was assigning DFIFO, but didn't return the value to users.

Signed-off-by: Sven Schnelle 
---
 hw/scsi/lsi53c895a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 7d66d1a870..3783779da6 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -1688,7 +1688,7 @@ static uint8_t lsi_reg_readb(LSIState *s, int offset)
 break;
 CASE_GET_REG32(temp, 0x1c)
 case 0x20: /* DFIFO */
-ret = 0;
+ret = s->dfifo;
 break;
 case 0x21: /* CTEST4 */
 ret = s->ctest4;
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/5] lsi: use enum type for s->waiting

[Sven Schnelle]

This makes the code easier to read - no functional change.

Signed-off-by: Sven Schnelle 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/scsi/lsi53c895a.c | 42 +++---
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 5f336606b9..cafe84c85b 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -194,6 +194,13 @@ typedef struct lsi_request {
 QTAILQ_ENTRY(lsi_request) next;
 } lsi_request;
 
+enum {
+LSI_NOWAIT, /* SCRIPTS are running or stopped */
+LSI_WAIT_RESELECT, /* Wait Reselect instruction has been issued */
+LSI_DMA_SCRIPTS, /* processing DMA from lsi_execute_script */
+LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
+};
+
 typedef struct {
 /*< private >*/
 PCIDevice parent_obj;
@@ -212,10 +219,6 @@ typedef struct {
 int msg_action;
 int msg_len;
 uint8_t msg[LSI_MAX_MSGIN_LEN];
-/* 0 if SCRIPTS are running or stopped.
- * 1 if a Wait Reselect instruction has been issued.
- * 2 if processing DMA from lsi_execute_script.
- * 3 if a DMA operation is in progress.  */
 int waiting;
 SCSIBus bus;
 int current_lun;
@@ -322,7 +325,7 @@ static void lsi_soft_reset(LSIState *s)
 
 s->msg_action = 0;
 s->msg_len = 0;
-s->waiting = 0;
+s->waiting = LSI_NOWAIT;
 s->dsa = 0;
 s->dnad = 0;
 s->dbc = 0;
@@ -564,10 +567,10 @@ static void lsi_bad_phase(LSIState *s, int out, int 
new_phase)
 static void lsi_resume_script(LSIState *s)
 {
 if (s->waiting != 2) {
-s->waiting = 0;
+s->waiting = LSI_NOWAIT;
 lsi_execute_script(s);
 } else {
-s->waiting = 0;
+s->waiting = LSI_NOWAIT;
 }
 }
 
@@ -744,7 +747,7 @@ static int lsi_queue_req(LSIState *s, SCSIRequest *req, 
uint32_t len)
Since no interrupt stacking is implemented in the emulation, it
is also required that there are no pending interrupts waiting
for service from the device driver. */
-if (s->waiting == 1 ||
+if (s->waiting == LSI_WAIT_RESELECT ||
 (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON) &&
  !(s->istat0 & (LSI_ISTAT0_SIP | LSI_ISTAT0_DIP {
 /* Reselect device.  */
@@ -789,7 +792,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t 
len)
 int out;
 
 assert(req->hba_private);
-if (s->waiting == 1 || req->hba_private != s->current ||
+if (s->waiting == LSI_WAIT_RESELECT || req->hba_private != s->current ||
 (lsi_irq_on_rsl(s) && !(s->scntl1 & LSI_SCNTL1_CON))) {
 if (lsi_queue_req(s, req, len)) {
 return;
@@ -803,7 +806,7 @@ static void lsi_transfer_data(SCSIRequest *req, uint32_t 
len)
 s->current->dma_len = len;
 s->command_complete = 1;
 if (s->waiting) {
-if (s->waiting == 1 || s->dbc == 0) {
+if (s->waiting == LSI_WAIT_RESELECT || s->dbc == 0) {
 lsi_resume_script(s);
 } else {
 lsi_do_dma(s, out);
@@ -1093,7 +1096,7 @@ static void lsi_wait_reselect(LSIState *s)
 lsi_reselect(s, p);
 }
 if (s->current == NULL) {
-s->waiting = 1;
+s->waiting = LSI_WAIT_RESELECT;
 }
 }
 
@@ -1202,16 +1205,16 @@ again:
 s->dnad64 = addr_high;
 switch (s->sstat1 & 0x7) {
 case PHASE_DO:
-s->waiting = 2;
+s->waiting = LSI_DMA_SCRIPTS;
 lsi_do_dma(s, 1);
 if (s->waiting)
-s->waiting = 3;
+s->waiting = LSI_DMA_IN_PROGRESS;
 break;
 case PHASE_DI:
-s->waiting = 2;
+s->waiting = LSI_DMA_SCRIPTS;
 lsi_do_dma(s, 0);
 if (s->waiting)
-s->waiting = 3;
+s->waiting = LSI_DMA_IN_PROGRESS;
 break;
 case PHASE_CMD:
 lsi_do_command(s);
@@ -1278,6 +1281,7 @@ again:
 }
 s->sbcl |= LSI_SBCL_BSY;
 lsi_set_phase(s, PHASE_MO);
+s->waiting = LSI_NOWAIT;
 break;
 case 1: /* Disconnect */
 trace_lsi_execute_script_io_disconnect();
@@ -1544,7 +1548,7 @@ again:
 }
 }
 }
-if (insn_processed > 1 && !s->waiting) {
+if (insn_processed > 1 && s->waiting == LSI_NOWAIT) {
 /* Some windows drivers make the device spin waiting for a memory
location to change.  If we have been executed a lot of code then
assume this is the case and force an unexpected device disconnect.
@@ -1556,7 +1560,7 @@ again:
 }
 lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
 lsi_disconnect(s);
-} else if (s->istat1 & LSI_ISTAT1_SRUN && !s->waiting) {
+} else if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) {
 if (s->dcntl & LSI_DCNTL_SSM) {
 lsi_script_dma_interrupt(s, LSI_DSTAT_SSI);
 } 

Re: [Qemu-devel] [PATCH] qemu-binfmt-conf.sh: add CPUS, add --reset, make -p and -c boolean (no arg)

[Eric Blake]

On 3/5/19 1:36 PM, Unai Martinez Corral wrote:

2019/3/5 17:57, Eric Blake:

You are correct that 'printf -1' is likely to fail, 'printf -- -1' is
portable but unusual, and 'printf %s\\n -1' is identical to the common
(but non-portable) behavior of 'echo -1'. Is the newline important?


In this case, the newline seems not to be relevant. I find 'printf --
-1' elegant, but since I already submitted v2 of the patch, I think
the difference compared to 'printf %s -1' is not worth a new version.
Please, let me know if you feel otherwise.


I don't have a strong preference between the two forms - so this is your 
chance to assert your creative liberty. But I don't know enough about 
binfmt_misc to know if the newline matters. I'm just reviewing on the 
basis of shell portability, and hope that other reviewers more familiar 
with binfmt will review on content. That said, I already have enough 
other comments on v2 that you may want to split this into a series of 
patches for v3, although you could also wait a day or so to see if 
anyone else reviews and minimize the list churn.


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

[Qemu-devel] [PATCH v2 3/5] lsi: use enum type for s->msg_action

[Sven Schnelle]

This makes the code easier to read - no functional change.

Signed-off-by: Sven Schnelle 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/scsi/lsi53c895a.c | 27 ---
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index cafe84c85b..843fa90b39 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -201,6 +201,13 @@ enum {
 LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
 };
 
+enum {
+LSI_MSG_ACTION_COMMAND = 0,
+LSI_MSG_ACTION_DISCONNECT = 1,
+LSI_MSG_ACTION_DOUT = 2,
+LSI_MSG_ACTION_DIN = 3,
+};
+
 typedef struct {
 /*< private >*/
 PCIDevice parent_obj;
@@ -214,8 +221,6 @@ typedef struct {
 
 int carry; /* ??? Should this be an a visible register somewhere?  */
 int status;
-/* Action to take at the end of a MSG IN phase.
-   0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
 int msg_action;
 int msg_len;
 uint8_t msg[LSI_MAX_MSGIN_LEN];
@@ -323,7 +328,7 @@ static void lsi_soft_reset(LSIState *s)
 trace_lsi_reset();
 s->carry = 0;
 
-s->msg_action = 0;
+s->msg_action = LSI_MSG_ACTION_COMMAND;
 s->msg_len = 0;
 s->waiting = LSI_NOWAIT;
 s->dsa = 0;
@@ -686,7 +691,7 @@ static void lsi_reselect(LSIState *s, lsi_request *p)
 trace_lsi_reselect(id);
 s->scntl1 |= LSI_SCNTL1_CON;
 lsi_set_phase(s, PHASE_MI);
-s->msg_action = p->out ? 2 : 3;
+s->msg_action = p->out ? LSI_MSG_ACTION_DOUT : LSI_MSG_ACTION_DIN;
 s->current->dma_len = p->pending;
 lsi_add_msg_byte(s, 0x80);
 if (s->current->tag & LSI_TAG_VALID) {
@@ -857,7 +862,7 @@ static void lsi_do_command(LSIState *s)
 lsi_add_msg_byte(s, 4); /* DISCONNECT */
 /* wait data */
 lsi_set_phase(s, PHASE_MI);
-s->msg_action = 1;
+s->msg_action = LSI_MSG_ACTION_DISCONNECT;
 lsi_queue_command(s);
 } else {
 /* wait command complete */
@@ -878,7 +883,7 @@ static void lsi_do_status(LSIState *s)
 s->sfbr = status;
 pci_dma_write(PCI_DEVICE(s), s->dnad, &status, 1);
 lsi_set_phase(s, PHASE_MI);
-s->msg_action = 1;
+s->msg_action = LSI_MSG_ACTION_DISCONNECT;
 lsi_add_msg_byte(s, 0); /* COMMAND COMPLETE */
 }
 
@@ -901,16 +906,16 @@ static void lsi_do_msgin(LSIState *s)
 /* ??? Check if ATN (not yet implemented) is asserted and maybe
switch to PHASE_MO.  */
 switch (s->msg_action) {
-case 0:
+case LSI_MSG_ACTION_COMMAND:
 lsi_set_phase(s, PHASE_CMD);
 break;
-case 1:
+case LSI_MSG_ACTION_DISCONNECT:
 lsi_disconnect(s);
 break;
-case 2:
+case LSI_MSG_ACTION_DOUT:
 lsi_set_phase(s, PHASE_DO);
 break;
-case 3:
+case LSI_MSG_ACTION_DIN:
 lsi_set_phase(s, PHASE_DI);
 break;
 default:
@@ -1062,7 +1067,7 @@ bad:
 qemu_log_mask(LOG_UNIMP, "Unimplemented message 0x%02x\n", msg);
 lsi_set_phase(s, PHASE_MI);
 lsi_add_msg_byte(s, 7); /* MESSAGE REJECT */
-s->msg_action = 0;
+s->msg_action = LSI_MSG_ACTION_COMMAND;
 }
 
 #define LSI_BUF_SIZE 4096
-- 
2.20.1

[Qemu-devel] [PATCH v2 1/5] lsi: use ldn_le_p()/stn_le_p()

[Sven Schnelle]

Instead of using the open-coded versions, use the helper already
present as this makes the code easier to read and less error-prone.

Signed-off-by: Sven Schnelle 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/scsi/lsi53c895a.c | 24 
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 5623da8950..5f336606b9 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -289,8 +289,7 @@ typedef struct {
 uint8_t sbr;
 uint32_t adder;
 
-/* Script ram is stored as 32-bit words in host byteorder.  */
-uint32_t script_ram[2048];
+uint8_t script_ram[2048 * sizeof(uint32_t)];
 } LSIState;
 
 #define TYPE_LSI53C810  "lsi53c810"
@@ -2079,29 +2078,14 @@ static void lsi_ram_write(void *opaque, hwaddr addr,
   uint64_t val, unsigned size)
 {
 LSIState *s = opaque;
-uint32_t newval;
-uint32_t mask;
-int shift;
-
-newval = s->script_ram[addr >> 2];
-shift = (addr & 3) * 8;
-mask = ((uint64_t)1 << (size * 8)) - 1;
-newval &= ~(mask << shift);
-newval |= val << shift;
-s->script_ram[addr >> 2] = newval;
+stn_le_p(s->script_ram + addr, size, val);
 }
 
 static uint64_t lsi_ram_read(void *opaque, hwaddr addr,
  unsigned size)
 {
 LSIState *s = opaque;
-uint32_t val;
-uint32_t mask;
-
-val = s->script_ram[addr >> 2];
-mask = ((uint64_t)1 << (size * 8)) - 1;
-val >>= (addr & 3) * 8;
-return val & mask;
+return ldn_le_p(s->script_ram + addr, size);
 }
 
 static const MemoryRegionOps lsi_ram_ops = {
@@ -2244,7 +2228,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
 VMSTATE_BUFFER_UNSAFE(scratch, LSIState, 0, 18 * sizeof(uint32_t)),
 VMSTATE_UINT8(sbr, LSIState),
 
-VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 2048 * 
sizeof(uint32_t)),
+VMSTATE_BUFFER_UNSAFE(script_ram, LSIState, 0, 8192),
 VMSTATE_END_OF_LIST()
 }
 };
-- 
2.20.1

[Qemu-devel] [PATCH v2 0/5] LSI53C895 cleanups

[Sven Schnelle]

Hi,

this series contains a few cosmetic cleanups and one small bugfix for the
LSI53C895 emulation.

Regards
Sven

Sven Schnelle (5):
  lsi: use ldn_le_p()/stn_le_p()
  lsi: use enum type for s->waiting
  lsi: use enum type for s->msg_action
  lsi: use SCSI phase names instead of numbers in trace
  lsi: return dfifo value

 hw/scsi/lsi53c895a.c | 126 +++
 hw/scsi/trace-events |   6 +--
 2 files changed, 70 insertions(+), 62 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 4/5] lsi: use SCSI phase names instead of numbers in trace

[Sven Schnelle]

This makes trace logs much easier to read, especially for
people who are not fluent in SCSI.

Signed-off-by: Sven Schnelle 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/scsi/lsi53c895a.c | 31 +++
 hw/scsi/trace-events |  6 +++---
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 843fa90b39..7d66d1a870 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -306,6 +306,22 @@ typedef struct {
 #define LSI53C895A(obj) \
 OBJECT_CHECK(LSIState, (obj), TYPE_LSI53C895A)
 
+static const char *scsi_phases[] = {
+"DOUT",
+"DIN",
+"CMD",
+"STATUS",
+"RSVOUT",
+"RSVIN",
+"MSGOUT",
+"MSGIN"
+};
+
+static const char *scsi_phase_name(int phase)
+{
+return scsi_phases[phase & PHASE_MASK];
+}
+
 static inline int lsi_irq_on_rsl(LSIState *s)
 {
 return (s->sien0 & LSI_SIST0_RSL) && (s->scid & LSI_SCID_RRE);
@@ -1201,8 +1217,9 @@ again:
 s->ia = s->dsp - 12;
 }
 if ((s->sstat1 & PHASE_MASK) != ((insn >> 24) & 7)) {
-trace_lsi_execute_script_blockmove_badphase(s->sstat1 & PHASE_MASK,
-(insn >> 24) & 7);
+trace_lsi_execute_script_blockmove_badphase(
+scsi_phase_name(s->sstat1),
+scsi_phase_name(insn >> 24));
 lsi_script_scsi_interrupt(s, LSI_SIST0_MA, 0);
 break;
 }
@@ -1234,8 +1251,8 @@ again:
 lsi_do_msgin(s);
 break;
 default:
-qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %d\n",
-  s->sstat1 & PHASE_MASK);
+qemu_log_mask(LOG_UNIMP, "lsi_scsi: Unimplemented phase %s\n",
+  scsi_phase_name(s->sstat1));
 }
 s->dfifo = s->dbc & 0xff;
 s->ctest5 = (s->ctest5 & 0xfc) | ((s->dbc >> 8) & 3);
@@ -1463,10 +1480,8 @@ again:
 cond = s->carry != 0;
 }
 if (cond == jmp && (insn & (1 << 17))) {
-trace_lsi_execute_script_tc_compp(
-(s->sstat1 & PHASE_MASK),
-jmp ? '=' : '!',
-((insn >> 24) & 7));
+trace_lsi_execute_script_tc_compp(scsi_phase_name(s->sstat1),
+jmp ? '=' : '!', scsi_phase_name(insn >> 24));
 cond = (s->sstat1 & PHASE_MASK) == ((insn >> 24) & 7);
 }
 if (cond == jmp && (insn & (1 << 18))) {
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index 29aaa752d1..09f3fc3086 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -268,7 +268,7 @@ lsi_memcpy(uint32_t dest, uint32_t src, int count) "memcpy 
dest 0x%"PRIx32" src
 lsi_wait_reselect(void) "Wait Reselect"
 lsi_execute_script(uint32_t dsp, uint32_t insn, uint32_t addr) "SCRIPTS 
dsp=0x%"PRIx32" opcode 0x%"PRIx32" arg 0x%"PRIx32
 lsi_execute_script_blockmove_delayed(void) "Delayed select timeout"
-lsi_execute_script_blockmove_badphase(uint8_t phase, uint8_t expected) "Wrong 
phase got %d expected %d"
+lsi_execute_script_blockmove_badphase(const char *phase, const char *expected) 
"Wrong phase got %s expected %s"
 lsi_execute_script_io_alreadyreselected(void) "Already reselected, jumping to 
alternative address"
 lsi_execute_script_io_selected(uint8_t id, const char *atn) "Selected target 
%d%s"
 lsi_execute_script_io_disconnect(void) "Wait Disconnect"
@@ -278,8 +278,8 @@ lsi_execute_script_io_opcode(const char *opcode, int reg, 
const char *opname, ui
 lsi_execute_script_tc_nop(void) "NOP"
 lsi_execute_script_tc_delayedselect_timeout(void) "Delayed select timeout"
 lsi_execute_script_tc_compc(int result) "Compare carry %d"
-lsi_execute_script_tc_compp(uint8_t phase, int op, uint8_t insn_phase) 
"Compare phase %d %c= %d"
-lsi_execute_script_tc_compd(uint32_t sfbr, uint8_t mask, int op, int result) 
"Compare data 0x%"PRIx32" & 0x%x %c= 0x%x"
+lsi_execute_script_tc_compp(const char *phase, char op, const char 
*insn_phase) "Compare phase %s %c= %s"
+lsi_execute_script_tc_compd(uint32_t sfbr, uint8_t mask, char op, int result) 
"Compare data 0x%"PRIx32" & 0x%x %c= 0x%x"
 lsi_execute_script_tc_jump(uint32_t addr) "Jump to 0x%"PRIx32
 lsi_execute_script_tc_call(uint32_t addr) "Call 0x%"PRIx32
 lsi_execute_script_tc_return(uint32_t addr) "Return to 0x%"PRIx32
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2] qemu-binfmt-conf.sh: add CPUS, add --reset, make -p and -c boolean (no arg)

[Eric Blake]

On 3/5/19 1:32 PM, Unai Martinez Corral wrote:

Signed-off-by: Unai Martinez-Corral 


Welcome to the qemu community, and I hope you find your experience with 
your first patch pleasant.


Long subject line, I'd trim ' (no arg)'. The commit body itself should 
go into the "why" the patch is useful (what behaviors are you fixing, 
what risk of backwards-compatibility breaks do we have to contend with 
for older clients of the script, etc).  Also, a v2 patch is best sent as 
a new top-level thread, rather than in-reply to the v1 (as some of our 
automated CI tooling misses it otherwise).  More submission hints can be 
found at: https://wiki.qemu.org/Contribute/SubmitAPatch


There may be enough changes here to be worth splitting this into 
multiple patches, for easier review (focus on one thing per patch: 
adding --reset sounds different enough from changing -p/-c, which in 
turn sounds different from fixing pre-existing shell usage bugs 
regarding prepending x when using []).



---
  scripts/qemu-binfmt-conf.sh | 190 +++-
  1 file changed, 121 insertions(+), 69 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index b5a16742a1..b1aa4a312a 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -6,6 +6,35 @@ mips mipsel mipsn32 mipsn32el mips64 mips64el \
  sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb \
  microblaze microblazeel or1k x86_64"

+# check if given target CPUS is/are in the supported target list
+qemu_check_target_list() {
+all="$qemu_target_list"
+if [ "x$1" = "xALL" ]; then
+  checked_target_list="$all"
+  return
+fi
+list=""
+bIFS="$IFS"
+IFS=" ,"


If you are allowing space, why not tab and newline?


+for target in $@; do


The ' in $@' is redundant, but doesn't hurt.


+unknown_target="true"
+for cpu in $all ; do


Why the space before ';' here, but not two lines earlier? The space 
doesn't hurt, but being consistent is nice.



+if [ "x$cpu" = "x$target" ] ; then


Similar questions about why only some of your additions have space 
before ; after ]



+list="$list $target"
+unknown_target="false"
+break
+fi
+done
+if [ "$unknown_target" = "true" ] ; then
+echo "ERROR: unknown CPU \"$target\"" 1>&2
+usage
+exit 1
+fi
+done
+IFS="$bIFS"
+checked_target_list="$list"
+}
+
  
i386_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00'
  
i386_mask='\xff\xff\xff\xff\xff\xfe\xfe\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
  i386_family=i386
@@ -167,45 +196,48 @@ qemu_get_family() {

  usage() {
  cat <


+Usage: qemu-binfmt-conf.sh [--help][--qemu-path PATH][--qemu-suffix SUFFIX]
+   [--persistent][--credential][--exportdir PATH]
+   [--reset ARCHS][--systemd][--debian][CPUS]
+


You are making a backwards-incompatible change with -p/-c - why is that 
okay? (It might be, but the commit message needs to give it more attention).



+Configure binfmt_misc to use qemu interpreter for the given target CPUS.
+Supported formats for CPUS are: single arch or comma/space separated list.
+If CPUS is empty, configure all known cpus. See QEMU target list below.


The wholesale reindentation of the --help text makes it harder to see 
what is actually new content. A separate patch to reflow/reindent the 
text before making additions will better call the reviewer's attention 
to the important changes.




  $CHECK
-qemu_set_binfmts
+qemu_set_binfmts $@


This should be "$@", not $@.

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