Re: [Qemu-devel] [PATCH v2 2/4] qjson: do not save/restore contexts

[Laszlo Ersek]

On 11/23/15 19:09, Paolo Bonzini wrote:
> 
> 
> On 23/11/2015 18:59, Laszlo Ersek wrote:
 The only interesting thing is that parse_keyword always eats
 a TOKEN_KEYWORD, even if it is invalid, so it must come last in
 parse_value (otherwise, NULL is returned, parse_literal is invoked
 and it tries to peek beyond end of input).  This is caught by
 /errors/unterminated/literal, which actually checks for an unterminated
 keyword. ಠ_ಠ
>> Is it accepted practice to put UTF-8 in commit messages? (Or, actually,
>> anywhere in patches, except maybe the notes section?)
>>
>> I'd recommend o_O.
> 
> Perhaps not Kannada, but Unicode 0xA1 to 0x1FF is pretty common in
> commit messages.

That seems to be a subset of the union of the following three:

https://en.wikipedia.org/wiki/Latin-1_Supplement_%28Unicode_block%29
https://en.wikipedia.org/wiki/Latin_Extended-A
https://en.wikipedia.org/wiki/Latin_Extended-B

Thanks for the info,
Laszlo

Re: [Qemu-devel] [PATCH] acpi: fix buffer overrun on migration

[John Snow]

On 11/19/2015 08:35 AM, Michael S. Tsirkin wrote:
> ich calls acpi_gpe_init with length ICH9_PMIO_GPE0_LEN so
> ICH9_PMIO_GPE0_LEN/2 bytes are allocated, but then the full
> ICH9_PMIO_GPE0_LEN bytes are migrated.
> 
> As a quick work-around, allocate twice the memory.
> We'll probably want to tweak code to avoid
> migrating the extra ICH9_PMIO_GPE0_LEN/2 bytes,
> but that is a bit trickier to do without breaking
> migration compatibility.
> 
> Tested-by: "Dr. David Alan Gilbert" 
> Reported-by: "Dr. David Alan Gilbert" 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Michael S. Tsirkin 
> ---
>  hw/acpi/core.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index fe6215a..21e113d 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -625,8 +625,12 @@ void acpi_pm1_cnt_reset(ACPIREGS *ar)
>  void acpi_gpe_init(ACPIREGS *ar, uint8_t len)
>  {
>  ar->gpe.len = len;
> -ar->gpe.sts = g_malloc0(len / 2);
> -ar->gpe.en = g_malloc0(len / 2);
> +/* Only first len / 2 bytes are ever used,
> + * but the caller in ich9.c migrates full len bytes.
> + * TODO: fix ich9.c and drop the extra allocation.
> + */
> +ar->gpe.sts = g_malloc0(len);
> +ar->gpe.en = g_malloc0(len);
>  }
>  
>  void acpi_gpe_reset(ACPIREGS *ar)
> 

Reviewed-by: John Snow 

Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm

[Laszlo Ersek]

On 11/23/15 20:31, Gabriel L. Somlo wrote:
> Couple of items:
> 
> 1. Ping ? :)
> 
> 2. Thank you markmb for your R-b !
> 
> 3. If anyone's had a chance to look over the corresponding guest-side
>kernel sysfs driver which utilizes this, you will have noticed I'm
>automatically initializing the driver based on DeviceTree or ACPI
>on ARM and x86, and that there's also the option of passing in
>command line parameters for the other architectures on which fw_cfg
>is available (sun4* and ppc/mac).
> 
>The issue I'm wondering about is that, while architectures where
>fw_cfg registers show up as IO ports (x86 and sun4u) have the same
>register_offset:size values (0:2 for control, 1:1 for data), MMIO
>on everything *other* than ARM is different:
> 
>   - ARM has 8:2 for control, and 0:8 for data
> 
>   - sun4m and ppc/mac both have 0:2 for control, and 2:1 for data
> 
>Right now, neither DeviceTree nor ACPI specify register offsets
>within the MMIO or PortIO area they associate with fw_cfg:
> 
>   - ACPI has (in _CRS) either:
> 
> IO (Decode16,
> 0x0510, // Range Minimum
> 0x0510, // Range Maximum
> 0x01,   // Alignment
> 0x02,   // Length
> )
> 
> or:
> 
> Memory32Fixed (ReadWrite,
>0x0902,  // Address Base
>0x000a,  // Address Length
>   )
> 
>   - DT does something along this lines:
> 
>   {
>   #size-cells = <0x2>;
>   #address-cells = <0x2>;
> 
>   fw-cfg@902 {
>   compatible = "qemu,fw-cfg-mmio";
>   reg = <0x0 0x902 0x0 0xa>;
>   };
>   };
> 
>   So in the guest-side kernel sysfs driver initialization routine,
>   I need to assume x86 (and, respectively, ARM) register offset:size
>   values unless explicitly provided on the command line.
> 
>   Are we likely to EVER try to supply defaults via DT (or ACPI) for
>   any other architectures besides ARM and x86 ? If so, is there a way
>   to additionally provide offsets (and sizes), besides just the
>   overall range ?

I guess this is where the bindings docs would provide the specification...

Laszlo

> 
>   If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
>   then nevermind I said anything :)
> 
> Thanks much,
> --Gabriel 
> 
> On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
>> New since v4:
>>
>>  - rebased on top of Marc's DMA series
>>  - drop machine compat dependency for insertion into x86/ssdt
>>(patch 3/5), following agreement between Igor and Eduardo
>>  - [mm]io register range now covers DMA register as well, if
>>available.
>>  - s/bios/firmware in doc file updates
>>
>> Thanks,
>>   --Gabriel
>>
>>> New since v3:
>>>
>>> - rebased to work on top of 87e896ab (introducing pc-*-25 classes),
>>>   inserting fw_cfg acpi node only for machines >= 2.5.
>>>
>>> - reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
>>>   off to avoid Windows complaining -- thanks Igor for catching that!)
>>>
>>> If there's any other feedback besides questions regarding the
>>> appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
>>>
 New since v2:

- pc/i386 node in ssdt only on machine types *newer* than 2.4
  (as suggested by Eduardo)

 I appreciate any further comments and reviews. Hopefully we can make
 this palatable for upstream, modulo the lingering concerns about whether
 "QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
 sorted out with the kernel crew...

> New since v1:
>
>   - expose control register size (suggested by Marc Marí)
>
>   - leaving out _UID and _STA fields (thanks Shannon & Igor)
>
>   - using "QEMU0002" as the value of _HID (thanks Michael)
>
>   - added documentation blurb to docs/specs/fw_cfg.txt
> (mainly to record usage of the "QEMU0002" string with fw_cfg).
>
>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
>> DSDT (on arm).
>>
>>  - Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
>>define from pc.c to pc.h, so that it could be used from
>>acpi-build.c in patch 2/3.
>>
>>  - Patch 2/3 adds a fw_cfg node to the pc SSDT.
>>
>>  - Patch 3/3 adds a fw_cfg node to the arm DSDT.
>>
>> I made up some names - "FWCF" for the node name, and "FWCF0001"
>> for _HID; no idea whether that's appropriate, or how else I should
>> figure out what to use instead...
>>
>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
>> output of "info qtree". Again, if that's wrong, 

Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm

[Gabriel L. Somlo]

Couple of items:

1. Ping ? :)

2. Thank you markmb for your R-b !

3. If anyone's had a chance to look over the corresponding guest-side
   kernel sysfs driver which utilizes this, you will have noticed I'm
   automatically initializing the driver based on DeviceTree or ACPI
   on ARM and x86, and that there's also the option of passing in
   command line parameters for the other architectures on which fw_cfg
   is available (sun4* and ppc/mac).

   The issue I'm wondering about is that, while architectures where
   fw_cfg registers show up as IO ports (x86 and sun4u) have the same
   register_offset:size values (0:2 for control, 1:1 for data), MMIO
   on everything *other* than ARM is different:

- ARM has 8:2 for control, and 0:8 for data

- sun4m and ppc/mac both have 0:2 for control, and 2:1 for data

   Right now, neither DeviceTree nor ACPI specify register offsets
   within the MMIO or PortIO area they associate with fw_cfg:

- ACPI has (in _CRS) either:

IO (Decode16,
0x0510, // Range Minimum
0x0510, // Range Maximum
0x01,   // Alignment
0x02,   // Length
)

  or:

Memory32Fixed (ReadWrite,
   0x0902,  // Address Base
   0x000a,  // Address Length
  )

- DT does something along this lines:

{
#size-cells = <0x2>;
#address-cells = <0x2>;

fw-cfg@902 {
compatible = "qemu,fw-cfg-mmio";
reg = <0x0 0x902 0x0 0xa>;
};
};

  So in the guest-side kernel sysfs driver initialization routine,
  I need to assume x86 (and, respectively, ARM) register offset:size
  values unless explicitly provided on the command line.

  Are we likely to EVER try to supply defaults via DT (or ACPI) for
  any other architectures besides ARM and x86 ? If so, is there a way
  to additionally provide offsets (and sizes), besides just the
  overall range ?

  If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
  then nevermind I said anything :)

Thanks much,
--Gabriel 

On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
> New since v4:
> 
>   - rebased on top of Marc's DMA series
>   - drop machine compat dependency for insertion into x86/ssdt
> (patch 3/5), following agreement between Igor and Eduardo
>   - [mm]io register range now covers DMA register as well, if
> available.
>   - s/bios/firmware in doc file updates
> 
> Thanks,
>   --Gabriel
> 
> >New since v3:
> >
> > - rebased to work on top of 87e896ab (introducing pc-*-25 classes),
> >   inserting fw_cfg acpi node only for machines >= 2.5.
> >
> > - reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
> >   off to avoid Windows complaining -- thanks Igor for catching that!)
> >
> >If there's any other feedback besides questions regarding the
> >appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
> >
> >>New since v2:
> >>
> >>- pc/i386 node in ssdt only on machine types *newer* than 2.4
> >>  (as suggested by Eduardo)
> >>
> >>I appreciate any further comments and reviews. Hopefully we can make
> >>this palatable for upstream, modulo the lingering concerns about whether
> >>"QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
> >>sorted out with the kernel crew...
> >>
> >>>New since v1:
> >>>
> >>>   - expose control register size (suggested by Marc Marí)
> >>>
> >>>   - leaving out _UID and _STA fields (thanks Shannon & Igor)
> >>>
> >>>   - using "QEMU0002" as the value of _HID (thanks Michael)
> >>>
> >>>   - added documentation blurb to docs/specs/fw_cfg.txt
> >>> (mainly to record usage of the "QEMU0002" string with fw_cfg).
> >>>
>  This series adds a fw_cfg device node to the SSDT (on pc), or to the
>  DSDT (on arm).
> 
>   - Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
> define from pc.c to pc.h, so that it could be used from
> acpi-build.c in patch 2/3.
>  
>   - Patch 2/3 adds a fw_cfg node to the pc SSDT.
>  
>   - Patch 3/3 adds a fw_cfg node to the arm DSDT.
> 
>  I made up some names - "FWCF" for the node name, and "FWCF0001"
>  for _HID; no idea whether that's appropriate, or how else I should
>  figure out what to use instead...
> 
>  Also, using scope "\\_SB", based on where fw_cfg shows up in the
>  output of "info qtree". Again, if that's wrong, please point me in
>  the right direction.
> 
>  Re. 3/3 (also mentioned after the commit blurb in the patch itself),
>  I noticed none of the other DSDT entries contain a _STA field, wondering
>  why it would (not) make sense to 

Re: [Qemu-devel] MCG_CAP ABI breakage (was Re: [PATCH] target-i386: Do not set MCG_SER_P by default)

[Eduardo Habkost]

On Mon, Nov 23, 2015 at 05:43:14PM +0100, Borislav Petkov wrote:
> On Mon, Nov 23, 2015 at 01:11:27PM -0200, Eduardo Habkost wrote:
> > On Mon, Nov 23, 2015 at 11:22:37AM -0200, Eduardo Habkost wrote:
> > [...]
> > > In the case of this code, it looks like it's already broken
> > > because the resulting mcg_cap depends on host kernel capabilities
> > > (the ones reported by kvm_get_mce_cap_supported()), and the data
> > > initialized by target-i386/cpu.c:mce_init() is silently
> > > overwritten by kvm_arch_init_vcpu(). So we would need to fix that
> > > before implementing a proper compatibility mechanism for
> > > mcg_cap.
> > 
> > Fortunately, when running Linux v2.6.37 and later,
> > kvm_arch_init_vcpu() won't actually change mcg_cap (see details
> > below).
> > 
> > But the code is broken if running on Linux between v2.6.32 and
> > v2.6.36: it will clear MCG_SER_P silently (and silently enable
> > MCG_SER_P when migrating to a newer host).
> > 
> > But I don't know what we should do on those cases. If we abort
> > initialization when the host doesn't support MCG_SER_P, all CPU
> > models with MCE and MCA enabled will become unrunnable on Linux
> > between v2.6.32 and v2.6.36. Should we do that, and simply ask
> > people to upgrade their kernels (or explicitly disable MCE) if
> > they want to run latest QEMU?
> > 
> > For reference, these are the capabilities returned by Linux:
> > * KVM_MAX_MCE_BANKS is 32 since
> >   890ca9aefa78f7831f8f633cab9e4803636dffe4 (v2.6.32-rc1~693^2~199)
> > * KVM_MCE_CAP_SUPPORTED is (MCG_CTL_P | MCG_SER_P) since
> >   5854dbca9b235f8cdd414a0961018763d2d5bf77 (v2.6.37-rc1~142^2~3)
> 
> The commit message of that one says that there is MCG_SER_P support in
> the kernel.
> 
> The previous commit talks about MCE injection with KVM_X86_SET_MCE
> ioctl but frankly, I don't see that. From looking at the current code,
> KVM_X86_SET_MCE does kvm_vcpu_ioctl_x86_setup_mce() which simply sets
> MCG_CAP. And it gets those from KVM_X86_GET_MCE_CAP_SUPPORTED which is
> 
> #define KVM_MCE_CAP_SUPPORTED (MCG_CTL_P | MCG_SER_P)
> 
> So it basically sets those two supported bits. But how is
> 
>   supported == actually present
> 
> ?!?!
> 
> That soo doesn't make any sense.

I will let the people working on the actual MCE emulation in KVM
answer that. I am assuming that KVM_MCE_CAP_SUPPORTED is set to
something that makes sense.

> 
> > * KVM_MCE_CAP_SUPPORTED is MCG_CTL_P between
> >   890ca9aefa78f7831f8f633cab9e4803636dffe4 (v2.6.32-rc1~693^2~199)
> >   and 5854dbca9b235f8cdd414a0961018763d2d5bf77 (v2.6.37-rc1~142^2~3)
> > 
> > The current definitions in QEMU are:
> > #define MCE_CAP_DEF (MCG_CTL_P|MCG_SER_P)
> > #define MCE_BANKS_DEF   10
> 
> That's also wrong. The number of banks is, of course,
> generation-specific.

Note that we don't mimick every single detail of real CPUs out
there, and this isn't necessarily a problem (although sometimes
we choose bad defaults). Do you see real world scenarios when
choosing 10 as the default causes problems for guest OSes, or you
just worry that this might cause problems because it doesn't
match any real-world CPU?

The number of banks is whatever QEMU chooses to be the number of
banks. The pc-*-2.4 and older machine-types already expose 10
banks, but we can change it in future machine-types.


> The qemu commit adding that is
> 
> 79c4f6b08009 ("QEMU: MCE: Add MCE simulation to qemu/tcg")
> 
> and I really don't get what the reasoning behind it is? Is this supposed
> to mimick a certain default CPU which is not really resembling a real
> CPU or some generic CPU or what is it?

I don't know the reasoning behind those defaults, but that's the
existing default in pc-*-2.4 and older.

> 
> Because the moment you start qemu with -cpu , all that
> MCA information is totally wrong.

If we really care about matching the number of banks of real
CPUs, we can make it configurable, defined by the CPU model,
and/or have better defaults in future machine-types. That won't
be a problem.

But I still don't know what we should do when somebody runs:
  -machine pc-i440fx-2.4 -cpu Penryn
on a host kernel that doesn't report MCG_SER_P on
KVM_MCE_CAP_SUPPORTED.

> 
> > The target-i386/cpu.c:mce_init() code sets mcg_cap to:
> > env->mcg_cap == MCE_CAP_DEF | MCE_BANKS_DEF;
> >  == (MCG_CTL_P|MCG_SER_P) | 10;
> > 
> > The kvm_arch_init_vcpu() code that changes mcg_cap does the following:
> > kvm_get_mce_cap_supported(cs->kvm_state, _cap, );
> > if (banks > MCE_BANKS_DEF) {
> > banks = MCE_BANKS_DEF;
> > }
> > mcg_cap &= MCE_CAP_DEF;
> > mcg_cap |= banks;
> > env->mcg_cap = mcg_cap;
> >   * Therefore, if running Linux v2.6.37 or newer, this will be
> > the result:
> > banks == 10;
> > mcg_cap == (MCG_CTL_P|MCG_SER_P) | banks
> > == (MCG_CTL_P|MCG_SER_P) | 10;
> > * That's the same value set by mce_init(), so fortunately
> >   kvm_arch_init_vcpu() isn't actually changing 

Re: [Qemu-devel] QAPI vs QMP commands

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova  writes:
>> When adding a new QAPI command, should I also add a corresponding entry in
>> "qmp-commands.hx"?

> Yes.

> Towards the end of the very long QAPI patch queue are patches to get rid
> of qmp-commands.hx.  We'll get there, but until then, you have to update
> qmp-commands.hx, too.

Ok, I just wasn't sure if the wiki page was up to date.

Thanks,
  Lluis

-- 
"And it's much the same thing with knowledge, for whenever you learn
something new, the whole world becomes that much richer."
-- The Princess of Pure Reason, as told by Norton Juster in The Phantom
Tollbooth

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Eric Blake]

On 11/23/2015 07:46 AM, Markus Armbruster wrote:

>>> If it's not broken, please explain to me how the guest should find out
>>> whether its ivshmem device sports a doorbell.
>>
>> If you have received ID, you should be good to use the doorbell.
> 
> That's not a complete answer, so let me try a different tack.
> 
> What value will a read of register IVPosition yield
> 
> * if the device has no doorbell:
> 
>   I think the answer is -1.
> 
> * if the device has a doorbell, but it isn't ready, yet:
> 
>   I think the answer is -1.
> 
> * if the device has a doorbell, and it is ready.
> 
>   This is the part you answered already: >= 0.
> 
> Please correct mistakes.

For what it's worth, I'm agreeing with Markus here - we have a race in
the guest learning whether doorbell is supported, and it's better to do
a clean break in API for 2.5 along with ALL the other fixes into using a
sane union of types (splitting between ivshmem-plain and
ivshmem-doorbell).  If you absolutely want it, you can still maintain
'ivshmem' as a front-end that maps to either ivshmem-plain,
ivshmem-doorbell, or an error (nonsensical combination of requests), but
having a sane interface as your starting point, rather than an
amalgamation of cruft that exists only due to the early implementation,
seems like the way to go.

I'm still waiting for any evidence that we even care about backwards
compatibility - what apps are out there that are actively using ivshmem
with its horrible pre-2.5 interface, and why can they not be fixed to
use the sane 2.5 interface?  Libvirt is NOT exposing ivshmem yet, in
part because the 2.4 interface was not very robust.  We already have a
clean break now due to all your work for 2.5; let's take advantage of
it, and make 2.5 robust, rather than breaking things again in 2.6.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 2/4] qjson: do not save/restore contexts

[Paolo Bonzini]

On 23/11/2015 18:59, Laszlo Ersek wrote:
>> > The only interesting thing is that parse_keyword always eats
>> > a TOKEN_KEYWORD, even if it is invalid, so it must come last in
>> > parse_value (otherwise, NULL is returned, parse_literal is invoked
>> > and it tries to peek beyond end of input).  This is caught by
>> > /errors/unterminated/literal, which actually checks for an unterminated
>> > keyword. ಠ_ಠ
> Is it accepted practice to put UTF-8 in commit messages? (Or, actually,
> anywhere in patches, except maybe the notes section?)
> 
> I'd recommend o_O.

Perhaps not Kannada, but Unicode 0xA1 to 0x1FF is pretty common in
commit messages.

Paolo

Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest

[Daniel P. Berrange]

On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote:
> On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini  wrote:
> > Before: object-initial, chardev, qtest, object-late (not in the patch)
> >
> > After: chardev, qtest, object-initial, object-late (not in the patch)
> >
> > Objects must be initialized before chardev (except rng-egd) since in the
> > future chardev will need to use objects, in particular secret objects.
> > Was the swap intentional?
> 
> Yes, without the swap, qtest was not initialized before memory is allocated.
> 
> The alternative I could think of is to check the QTEST_QEMU_BINARY
> variable: 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html

Why do we not simply delete the warning message about the path not
being on hugetlbfs ? ie, why does QEMU try to force a policy that
a memory-file backend has to be on hugetlbfs, as opposed to on
a plain tmpfs ?  I've previously had user request that we allow
use of plain tmpfs, because they want to use vhost-user without
also using hugepages, and that could be done with plain tmpfs.

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

[Qemu-devel] [RFC][PATCH v2 0/2] utils: Improve and document error reporting

[Lluís Vilanova]

Adds support for reporting non-terminating errors (dubbed warnings), and briefly
documents what routines should developers generally use to keep error reporting
consistent.

Changes in v2
=

* Split in two patches.
* Explicitly add a warning error object.


Signed-off-by: Lluís Vilanova 
---

Lluís Vilanova (2):
  utils: Add warning messages
  doc: Introduce coding style for errors


 HACKING  |   31 +++
 include/qapi/error.h |   20 
 util/error.c |   37 +++--
 3 files changed, 78 insertions(+), 10 deletions(-)


To: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi 
Cc: Dr. David Alan Gilbert 
Cc: Thomas Huth 
Cc: Markus Armbruster 
Cc: Eric Blake 

[Qemu-devel] QAPI vs QMP commands

[Lluís Vilanova]

When adding a new QAPI command, should I also add a corresponding entry in
"qmp-commands.hx"?


Thanks,
  Lluis

-- 
"And it's much the same thing with knowledge, for whenever you learn
something new, the whole world becomes that much richer."
-- The Princess of Pure Reason, as told by Norton Juster in The Phantom
Tollbooth

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Markus Armbruster]

Markus Armbruster  writes:

> Marc-André Lureau  writes:
[...]
>>> * shm appears to be the same as memdev, just less flexible.  Why does it
>>>   exist?
>>
>> It was there before.
>
> Not only is memdev more flexible, it also provides the clean split
> between frontend and backend we generally want.

In my tests, shm=foo can indeed be replaced by -object
memory-backend=file,mem-path=/dev/shm/foo.

However, /dev/shm is Linux-specific.  For portability, we might need a
memory-backend-shm.

[...]

Re: [Qemu-devel] Build problem with qemu Makefile change

[Michael Roth]

Quoting Steve Ellcey  (2015-11-23 12:06:57)
> My qemu build has been failing since this checkin by Michael Roth:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg03991.html
> 
> which changed the top-level Makefile.  Is anyone else seeing this?
> 
> I am building a qemu for MIPS on an x86 linux box with:
> 
> configure --prefix=/scratch/sellcey/repos/q/install-mips-mti-linux-gnu 
> --disable-tools --disable-system --disable-werror 
> --target-list=mips-linux-user,mipsel-linux-user,mipsn32-linux-user,mipsn32el-linux-user,mips64-linux-user,mips64el-linux-user
> 
> make --jobs=3 all
> make --jobs=3 install
> 
> The error message is:
> 
> install -d -m 0755 "/scratch/sellcey/repos/q/install-mips-mti-linux-gnu/bin"
> libtool --quiet --mode=install install -c -m 0755  
> "/scratch/sellcey/repos/q/install-mips-mti-linux-gnu/bin"
> libtool: install: you must specify a destination
> libtool: install: Try `libtool --help --mode=install' for more information.
> make: *** [install] Error 1
> Error: Make command failed, stopping build.
> 
> 
> If I remove the Makefile patch then my build works and the libtool install
> line looks like:
> 
> install -d -m 0755 "/scratch/sellcey/repos/q/install-mips-mti-linux-gnu/bin"
> libtool --quiet --mode=install install -c -m 0755 qemu-ga  
> "/scratch/sellcey/repos/q2/install-mips-mti-linux-gnu/bin"
> 
> I think the failure may be related to my use of '--disable-tools' or
> '--disable-system' in the configure and how this interacts with the
> Makefile change.

Looks like --disable-tools results in qemu-ga being
the only entry in $TOOLS.

With the filter added in my patch, this results in install-prog
getting called with an empty list, which generates the error.

So now we need an extra check to avoid calling install-prog with an
empty list. I'll post a patch today. Thanks for the catch.

> 
> Steve Ellcey
> sell...@imgtec.com
> 

[Qemu-devel] [PATCH v2 1/2] utils: Add warning messages

[Lluís Vilanova]

Adds a special error object that transforms error messages into
immediately reported warnings.

Signed-off-by: Lluís Vilanova 
---
 include/qapi/error.h |   20 
 util/error.c |   37 +++--
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 4d42cdc..9b7600c 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -57,6 +57,9 @@
  * Call a function treating errors as fatal:
  * foo(arg, _fatal);
  *
+ * Call a function immediately showing all errors as warnings:
+ * foo(arg, _warn);
+ *
  * Receive an error and pass it on to the caller:
  * Error *err = NULL;
  * foo(arg, );
@@ -108,6 +111,7 @@ ErrorClass error_get_class(const Error *err);
  * then.
  * If @errp is _abort, print a suitable message and abort().
  * If @errp is _fatal, print a suitable message and exit(1).
+ * If @errp is _warn, print a suitable message.
  * If @errp is anything else, *@errp must be NULL.
  * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
  * human-readable error message is made from printf-style @fmt, ...
@@ -158,6 +162,7 @@ void error_setg_win32_internal(Error **errp,
  * abort().
  * Else, if @dst_errp is _fatal, print a suitable message and
  * exit(1).
+ * Else, if @dst_errp is _warn, print a suitable message.
  * Else, if @dst_errp already contains an error, ignore this one: free
  * the error object.
  * Else, move the error object from @local_err to *@dst_errp.
@@ -218,12 +223,27 @@ void error_set_internal(Error **errp,
 
 /*
  * Pass to error_setg() & friends to abort() on error.
+ *
+ * WARNING: Do _not_ use for errors that are (or can be) triggered by guest 
code
+ *  (e.g., some unimplimented corner case in guest code translation or
+ *  device code). Otherwise that can be abused by guest code to
+ *  terminate QEMU.
  */
 extern Error *error_abort;
 
 /*
  * Pass to error_setg() & friends to exit(1) on error.
+ *
+ * WARNING: Do _not_ use for errors that are (or can be) triggered by guest 
code
+ *  (e.g., some unimplimented corner case in guest code translation or
+ *  device code). Otherwise that can be abused by guest code to
+ *  terminate QEMU.
  */
 extern Error *error_fatal;
 
+/*
+ * Pass to error_setg() & friends to immediately show an error as a warning.
+ */
+extern Error *error_warn;
+
 #endif
diff --git a/util/error.c b/util/error.c
index 80c89a2..85170e54 100644
--- a/util/error.c
+++ b/util/error.c
@@ -15,6 +15,7 @@
 #include "qemu-common.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/timer.h"
 
 struct Error
 {
@@ -27,8 +28,9 @@ struct Error
 
 Error *error_abort;
 Error *error_fatal;
+Error *error_warn;
 
-static void error_handle_fatal(Error **errp, Error *err)
+static bool error_handle_fatal(Error **errp, Error *err)
 {
 if (errp == _abort) {
 fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
@@ -40,6 +42,20 @@ static void error_handle_fatal(Error **errp, Error *err)
 error_report_err(err);
 exit(1);
 }
+
+if (errp == _warn) {
+/* cannot use error_report_err() because it adds newlines */
+error_report("warning: [%s() at %s:%d] %s",
+ err->func, err->src, err->line, err->msg);
+if (err->hint) {
+error_printf_unless_qmp("warning: [%s() at %s:%d] %s",
+err->func, err->src, err->line,
+err->hint->str);
+}
+return true;
+} else {
+return false;
+}
 }
 
 static void error_setv(Error **errp,
@@ -61,10 +77,10 @@ static void error_setv(Error **errp,
 err->line = line;
 err->func = func;
 
-error_handle_fatal(errp, err);
-*errp = err;
-
-errno = saved_errno;
+if (!error_handle_fatal(errp, err)) {
+*errp = err;
+errno = saved_errno;
+}
 }
 
 void error_set_internal(Error **errp,
@@ -232,10 +248,11 @@ void error_propagate(Error **dst_errp, Error *local_err)
 if (!local_err) {
 return;
 }
-error_handle_fatal(dst_errp, local_err);
-if (dst_errp && !*dst_errp) {
-*dst_errp = local_err;
-} else {
-error_free(local_err);
+if (!error_handle_fatal(dst_errp, local_err)) {
+if (dst_errp && !*dst_errp) {
+*dst_errp = local_err;
+} else {
+error_free(local_err);
+}
 }
 }

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl

[Sergey Fedorov]

On 23.11.2015 19:49, Peter Maydell wrote:
> Ping? I forgot to mark this for-2.5, and given how long the bug's
> been hanging around there's not much urgency to fixing it, but
> we might as well put the fix into 2.5 if it gets reviewed.
>

Hi, Peter. I'm going to review this carefully in a few days :)
For now, I see that the comment for this function should be updated to
match new code.

Best,
Sergey

>
> On 16 November 2015 at 18:28, Peter Maydell  wrote:
>> The checks for the unallocated encodings in the ldst_excl group
>> (exclusives and load-acquire/store-release) were not correct. This
>> error meant that in turn we ended up with code attempting to handle
>> the non-existent case of "non-exclusive load-acquire/store-release
>> pair". Delete that broken and now unreachable code.
>>
>> Reported-by: Laurent Desnogues 
>> Signed-off-by: Peter Maydell 
>> ---
>> The easiest way to validate that we have the unallocated
>> conditions correct now is to look at C4.4.6 "load/store exclusive"
>> in the v8 ARM ARM rev A.h: our three conditions correspond
>> to the three "unallocated" rows in the decode table.
>>
>> PS: Laurent originally reported this way back in 2014:
>> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg01255.html
>>
>>  target-arm/translate-a64.c | 12 ++--
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index fe485a4..890ace4 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -1833,7 +1833,8 @@ static void disas_ldst_excl(DisasContext *s, uint32_t 
>> insn)
>>  int size = extract32(insn, 30, 2);
>>  TCGv_i64 tcg_addr;
>>
>> -if ((!is_excl && !is_lasr) ||
>> +if ((!is_excl && !is_pair && !is_lasr) ||
>> +(!is_excl && is_pair) ||
>>  (is_pair && size < 2)) {
>>  unallocated_encoding(s);
>>  return;
>> @@ -1862,15 +1863,6 @@ static void disas_ldst_excl(DisasContext *s, uint32_t 
>> insn)
>>  } else {
>>  do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
>>  }
>> -if (is_pair) {
>> -TCGv_i64 tcg_rt2 = cpu_reg(s, rt);
>> -tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
>> -if (is_store) {
>> -do_gpr_st(s, tcg_rt2, tcg_addr, size);
>> -} else {
>> -do_gpr_ld(s, tcg_rt2, tcg_addr, size, false, false);
>> -}
>> -}
>>  }
>>  }
>>
>> --
>> 1.9.1
>>

[Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors

[Lluís Vilanova]

Gives some general guidelines for reporting errors in QEMU.

Signed-off-by: Lluís Vilanova 
---
 HACKING |   31 +++
 1 file changed, 31 insertions(+)

diff --git a/HACKING b/HACKING
index 12fbc8a..e59bc34 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,34 @@ painful. These are:
  * you may assume that integers are 2s complement representation
  * you may assume that right shift of a signed integer duplicates
the sign bit (ie it is an arithmetic shift, not a logical shift)
+
+7. Error reporting
+
+QEMU provides two different mechanisms for reporting errors. You should use one
+of these mechanisms instead of manually reporting them (i.e., do not use
+'printf', 'exit' or 'abort').
+
+7.1. Errors in user inputs
+
+QEMU provides the functions in "include/qemu/error-report.h" to report errors
+related to inputs provided by the user (e.g., command line arguments or
+configuration files).
+
+These functions generate error messages with a uniform format that can 
reference
+a location on the offending input.
+
+7.2. Other errors
+
+QEMU provides the functions in "include/qapi/error.h" to report other types of
+errors (i.e., not triggered by command line arguments or configuration files).
+
+Functions in this header are used to accumulate error messages in an 'Error'
+object, which can be propagated up the call chain where it is finally reported.
+
+In its simplest form, you can immediately report an error with:
+
+error_setg(_warn, "Error with %s", "arguments");
+
+See the "include/qapi/error.h" header for additional convenience functions and
+special arguments. Specially, see 'error_fatal' and 'error_abort' to show 
errors
+and immediately terminate QEMU.

Re: [Qemu-devel] [PATCH v2 2/4] qjson: do not save/restore contexts

[Eric Blake]

On 11/23/2015 10:59 AM, Laszlo Ersek wrote:
> On 11/23/15 18:44, Paolo Bonzini wrote:
>> JSON is LL(1) and our parser indeed needs only 1 token lookahead.
>> Saving the parser context is mostly unnecessary; we can replace it
>> with peeking at the next token, or remove it altogether when the
>> restore only happens on errors.  The token list is destroyed anyway
>> on errors.
>>
>> The only interesting thing is that parse_keyword always eats
>> a TOKEN_KEYWORD, even if it is invalid, so it must come last in
>> parse_value (otherwise, NULL is returned, parse_literal is invoked
>> and it tries to peek beyond end of input).  This is caught by
>> /errors/unterminated/literal, which actually checks for an unterminated
>> keyword. ಠ_ಠ
> 
> Is it accepted practice to put UTF-8 in commit messages? (Or, actually,
> anywhere in patches, except maybe the notes section?)
> 

Git handles UTF-8 just fine (and for any other encoding, properly
transmitted in the email, git transcodes to UTF-8 before writing it into
the repository).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors

[Lluís Vilanova]

Daniel P Berrange writes:
[...]
> I don't think this "Errors in user inputs" vs "Other errors" distinction
> really makes sense. Whether an error raised in a piece of code is related
> to user input or not is almost impossible to determine in practice. So as
> a rule to follow it is not practical.

> AFAIK, include/qemu/error-report.h is the historical failed experiment
> in structured error reporting, while  include/qapi/error.h is the new
> preferred error reporting system that everything should be using.

> On this basis, I'd simply say that include/qemu/error-report.h is
> legacy code that should no longer be used, and that new code should
> use include/qapi/error.h exclusively and existing code converted
> where practical.

Mmmm, I've just reviewed both headers and you sound partially right.

AFAIU, "qemu/error-report.h" contains the additional logic to manage "input
locations", not present anywhere else. Also, you state that only the reporting
functions in "qemu/error.h" should be used.

Since "qemu/error.h" internally uses 'error_report()' (from
"qemu/error-report.h"), it includes the input location information (if any). So,
I will simply refer to "qemu/error.h" for the general reporting functions, plus
the location management functions in "qemu/error-report.h".

Does this sound to follow the expected flow of latest QEMU?

Thanks,
  Lluis

-- 
"And it's much the same thing with knowledge, for whenever you learn
something new, the whole world becomes that much richer."
-- The Princess of Pure Reason, as told by Norton Juster in The Phantom
Tollbooth

Re: [Qemu-devel] [PATCH v5 1/4] firmware: introduce sysfs driver for QEMU's fw_cfg device

[kbuild test robot]

Hi Gabriel,

[auto build test WARNING on v4.4-rc2]
[also build test WARNING on next-20151123]
[cannot apply to robh/for-next]

url:
https://github.com/0day-ci/linux/commits/Gabriel-L-Somlo/SysFS-driver-for-QEMU-fw_cfg-device/20151124-000402
config: arm-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_set':
>> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
>> argument of type 'long long int *', but argument 3 has type 'phys_addr_t *' 
>> [-Wformat=]
  _off, _off, );
  ^
>> drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
>> argument of type 'long long int *', but argument 5 has type 'resource_size_t 
>> *' [-Wformat=]
   drivers/firmware/qemu_fw_cfg.c:510:7: warning: format '%lli' expects 
argument of type 'long long int *', but argument 6 has type 'resource_size_t *' 
[-Wformat=]
   drivers/firmware/qemu_fw_cfg.c: In function 'fw_cfg_cmdline_get':
>> drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
>> argument of type 'long long unsigned int', but argument 4 has type 
>> 'resource_size_t' [-Wformat=]
fw_cfg_cmdline_dev->resource[0].start);
^
   drivers/firmware/qemu_fw_cfg.c:563:5: warning: format '%llx' expects 
argument of type 'long long unsigned int', but argument 5 has type 
'resource_size_t' [-Wformat=]
   drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
argument of type 'long long unsigned int', but argument 4 has type 
'resource_size_t' [-Wformat=]
fw_cfg_cmdline_dev->resource[2].start);
^
   drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llx' expects 
argument of type 'long long unsigned int', but argument 5 has type 
'resource_size_t' [-Wformat=]
>> drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
>> argument of type 'long long unsigned int', but argument 6 has type 
>> 'resource_size_t' [-Wformat=]
   drivers/firmware/qemu_fw_cfg.c:569:5: warning: format '%llu' expects 
argument of type 'long long unsigned int', but argument 7 has type 
'resource_size_t' [-Wformat=]

vim +510 drivers/firmware/qemu_fw_cfg.c

   504  /* consume "" portion of command line argument */
   505  size = memparse(arg, );
   506  
   507  /* get "@[::]" chunks */
   508  processed = sscanf(str, "@%lli%n:%lli:%lli%n",
   509 , ,
 > 510 _off, _off, );
   511  
   512  /* sscanf() must process precisely 1 or 3 chunks:
   513   *  is mandatory, optionally followed by 
   514   * and ;
   515   * there must be no extra characters after the last chunk,
   516   * so str[consumed] must be '\0'.
   517   */
   518  if (str[consumed] ||
   519  (processed != 1 && processed != 3))
   520  return -EINVAL;
   521  
   522  res[0].start = base;
   523  res[0].end = base + size - 1;
   524  res[0].flags = !strcmp(kp->name, "mmio") ? IORESOURCE_MEM :
   525 IORESOURCE_IO;
   526  
   527  /* insert register offsets, if provided */
   528  if (processed > 1) {
   529  res[1].name = "ctrl";
   530  res[1].start = ctrl_off;
   531  res[1].flags = IORESOURCE_REG;
   532  res[2].name = "data";
   533  res[2].start = data_off;
   534  res[2].flags = IORESOURCE_REG;
   535  }
   536  
   537  /* "processed" happens to nicely match the number of resources
   538   * we need to pass in to this platform device.
   539   */
   540  fw_cfg_cmdline_dev = platform_device_register_simple("fw_cfg",
   541  PLATFORM_DEVID_NONE, res, 
processed);
   542  if (IS_ERR(fw_cfg_cmdline_dev))
   543  return PTR_ERR(fw_cfg_cmdline_dev);
   544  
   545  return 0;
   546  }
   547  
   548  static int fw_cfg_cmdline_get(char *buf, const struct kernel_param *kp)
   549  {
   550  /* stay silent if device was not configured via the command
   551   * line, or if the parameter name (ioport/mmio) doesn't match
   552   * the device setting
   553   */
   554  if (!fw_cfg_cmdline_dev ||
   555  (!strcmp(kp->name, "mmio") ^
   556   

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Marc-André Lureau]

Hi

- Original Message -
> On 11/23/2015 07:46 AM, Markus Armbruster wrote:
> 
> >>> If it's not broken, please explain to me how the guest should find out
> >>> whether its ivshmem device sports a doorbell.
> >>
> >> If you have received ID, you should be good to use the doorbell.
> > 
> > That's not a complete answer, so let me try a different tack.
> > 
> > What value will a read of register IVPosition yield
> > 
> > * if the device has no doorbell:
> > 
> >   I think the answer is -1.
> > 
> > * if the device has a doorbell, but it isn't ready, yet:
> > 
> >   I think the answer is -1.
> > 
> > * if the device has a doorbell, and it is ready.
> > 
> >   This is the part you answered already: >= 0.
> > 
> > Please correct mistakes.
> 
> For what it's worth, I'm agreeing with Markus here - we have a race in
> the guest learning whether doorbell is supported, and it's better to do

I think there is no race if you communicate this information in the shared 
memory.

> a clean break in API for 2.5 along with ALL the other fixes into using a
> sane union of types (splitting between ivshmem-plain and
> ivshmem-doorbell).  If you absolutely want it, you can still maintain
> 'ivshmem' as a front-end that maps to either ivshmem-plain,
> ivshmem-doorbell, or an error (nonsensical combination of requests), but

It's not about me. Until now I was not aware of anyone complaining about
the ivshmem interface, but by its implementation. I tried to adress all the
issues and comments I have found, and I tried to make sure not to break stuff,
because I usually receive huge complains whenever I do, and I have to throw
up the work. So if there is a consensus to break things now, I think it's
quite late for this cycle, but I am for it.

> having a sane interface as your starting point, rather than an
> amalgamation of cruft that exists only due to the early implementation,
> seems like the way to go.

It is just the way it was, isn't it?

> I'm still waiting for any evidence that we even care about backwards
> compatibility - what apps are out there that are actively using ivshmem
> with its horrible pre-2.5 interface, and why can they not be fixed to

> use the sane 2.5 interface?  Libvirt is NOT exposing ivshmem yet, in
> part because the 2.4 interface was not very robust.  We already have a

Afaik it's there since 1.2.10

> clean break now due to all your work for 2.5; let's take advantage of
> it, and make 2.5 robust, rather than breaking things again in 2.6.

2.5 should not break the ivshmem interface.

Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm

[Gabriel L. Somlo]

On Mon, Nov 23, 2015 at 08:46:33PM +0100, Laszlo Ersek wrote:
> On 11/23/15 20:31, Gabriel L. Somlo wrote:
> > Couple of items:
> > 
> > 1. Ping ? :)
> > 
> > 2. Thank you markmb for your R-b !
> > 
> > 3. If anyone's had a chance to look over the corresponding guest-side
> >kernel sysfs driver which utilizes this, you will have noticed I'm
> >automatically initializing the driver based on DeviceTree or ACPI
> >on ARM and x86, and that there's also the option of passing in
> >command line parameters for the other architectures on which fw_cfg
> >is available (sun4* and ppc/mac).
> > 
> >The issue I'm wondering about is that, while architectures where
> >fw_cfg registers show up as IO ports (x86 and sun4u) have the same
> >register_offset:size values (0:2 for control, 1:1 for data), MMIO
> >on everything *other* than ARM is different:
> > 
> > - ARM has 8:2 for control, and 0:8 for data
> > 
> > - sun4m and ppc/mac both have 0:2 for control, and 2:1 for data
> > 
> >Right now, neither DeviceTree nor ACPI specify register offsets
> >within the MMIO or PortIO area they associate with fw_cfg:
> > 
> > - ACPI has (in _CRS) either:
> > 
> > IO (Decode16,
> > 0x0510, // Range Minimum
> > 0x0510, // Range Maximum
> > 0x01,   // Alignment
> > 0x02,   // Length
> > )
> > 
> >   or:
> > 
> > Memory32Fixed (ReadWrite,
> >0x0902,  // Address Base
> >0x000a,  // Address Length
> >   )
> > 
> > - DT does something along this lines:
> > 
> > {
> > #size-cells = <0x2>;
> > #address-cells = <0x2>;
> > 
> > fw-cfg@902 {
> > compatible = "qemu,fw-cfg-mmio";
> > reg = <0x0 0x902 0x0 0xa>;
> > };
> > };
> > 
> >   So in the guest-side kernel sysfs driver initialization routine,
> >   I need to assume x86 (and, respectively, ARM) register offset:size
> >   values unless explicitly provided on the command line.
> > 
> >   Are we likely to EVER try to supply defaults via DT (or ACPI) for
> >   any other architectures besides ARM and x86 ? If so, is there a way
> >   to additionally provide offsets (and sizes), besides just the
> >   overall range ?
> 
> I guess this is where the bindings docs would provide the specification...

OK, but is there a way to *functionally* specify register offsets, in
a way that can automagically get translated into a set of

( IORESOURCE_IO | IORESOURCE_MEM ) + IORESOURCE_REG + ...

? Right now we get _IO or _MEM only, and have to assume the register
offsets based on the architecture. I tried to figure this out on my
own, but nothing obvious stands out either in the way DT nodes are
documented, nor in the way ACPI nodes are written in several of the
machines I acpidump-ed to look for clues [*]. Maybe not too many devices
have this much variability across the platforms they're deployed on :)

[*] In ACPI there's "Offset", but that's for accessing fields within an
operation range, and there's "Register" which seems promising:

replace IO(...) or Memory32Fixed(...) with a set of Register(...)
statements in _CRS, but not immediately clear how one would
specify which one is Data vs Ctrl (order ?), and whether there's
an equivalent way to specify this via DT...


I guess there's always something like

#if defined ARCH_X86

#define CTRL_OFF 0x00
#define DATA_OFF 0x01

#elif defined ARCH_ARM

#define CTRL_OFF 0x08
#define DATA_OFF 0x00

#else ...

Although I haven't seen a lot of that done anywhere in the kernel, so
it might be frowned upon... :)


Thanks,
--Gabriel


> > 
> >   If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
> >   then nevermind I said anything :)
> > 
> > Thanks much,
> > --Gabriel 
> > 
> > On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
> >> New since v4:
> >>
> >>- rebased on top of Marc's DMA series
> >>- drop machine compat dependency for insertion into x86/ssdt
> >>  (patch 3/5), following agreement between Igor and Eduardo
> >>- [mm]io register range now covers DMA register as well, if
> >>  available.
> >>- s/bios/firmware in doc file updates
> >>
> >> Thanks,
> >>   --Gabriel
> >>
> >>> New since v3:
> >>>
> >>>   - rebased to work on top of 87e896ab (introducing pc-*-25 classes),
> >>> inserting fw_cfg acpi node only for machines >= 2.5.
> >>>
> >>>   - reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
> >>> off to avoid Windows complaining -- thanks Igor for catching that!)
> >>>
> >>> If there's any other feedback besides questions regarding the
> >>> appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
> >>>
> 

Re: [Qemu-devel] [PATCH v2 2/4] qjson: do not save/restore contexts

[Laszlo Ersek]

On 11/23/15 21:05, Eric Blake wrote:
> On 11/23/2015 10:59 AM, Laszlo Ersek wrote:
>> On 11/23/15 18:44, Paolo Bonzini wrote:
>>> JSON is LL(1) and our parser indeed needs only 1 token lookahead.
>>> Saving the parser context is mostly unnecessary; we can replace it
>>> with peeking at the next token, or remove it altogether when the
>>> restore only happens on errors.  The token list is destroyed anyway
>>> on errors.
>>>
>>> The only interesting thing is that parse_keyword always eats
>>> a TOKEN_KEYWORD, even if it is invalid, so it must come last in
>>> parse_value (otherwise, NULL is returned, parse_literal is invoked
>>> and it tries to peek beyond end of input).  This is caught by
>>> /errors/unterminated/literal, which actually checks for an unterminated
>>> keyword. ಠ_ಠ
>>
>> Is it accepted practice to put UTF-8 in commit messages? (Or, actually,
>> anywhere in patches, except maybe the notes section?)
>>
> 
> Git handles UTF-8 just fine (and for any other encoding, properly
> transmitted in the email, git transcodes to UTF-8 before writing it into
> the repository).
> 

Yes, I know. I use latin2:

$ locale

LANG=
LC_CTYPE=hu_HU.ISO8859-2
LC_NUMERIC="POSIX"
LC_TIME="POSIX"
LC_COLLATE="POSIX"
LC_MONETARY="POSIX"
LC_MESSAGES="POSIX"
LC_PAPER="POSIX"
LC_NAME="POSIX"
LC_ADDRESS="POSIX"
LC_TELEPHONE="POSIX"
LC_MEASUREMENT="POSIX"
LC_IDENTIFICATION="POSIX"
LC_ALL=

and from my git config:

[i18n]
logOutputEncoding = latin2
commitencoding = latin2

This works very well -- as long as it doesn't choke on something outside
of latin2 --, both the glibc locale support and git are doing their jobs
perfectly fine; my question concerned any other users who decided to
stay with single-byte encodings (with an ASCII subset).

(I believe that RFCs stick with ASCII to this day, and I also think that
our source code and docs/ should stick with ASCII; but I know I can't
plausibly argue for the same in commit messages, assuming I'm alone with
that anyway.

BTW I should have written "non-ASCII Unicode code points" in my original
question, rather than "UTF-8".)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors

[Daniel P. Berrange]

On Mon, Nov 23, 2015 at 09:05:30PM +0100, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> [...]
> > I don't think this "Errors in user inputs" vs "Other errors" distinction
> > really makes sense. Whether an error raised in a piece of code is related
> > to user input or not is almost impossible to determine in practice. So as
> > a rule to follow it is not practical.
> 
> > AFAIK, include/qemu/error-report.h is the historical failed experiment
> > in structured error reporting, while  include/qapi/error.h is the new
> > preferred error reporting system that everything should be using.
> 
> > On this basis, I'd simply say that include/qemu/error-report.h is
> > legacy code that should no longer be used, and that new code should
> > use include/qapi/error.h exclusively and existing code converted
> > where practical.
> 
> Mmmm, I've just reviewed both headers and you sound partially right.
> 
> AFAIU, "qemu/error-report.h" contains the additional logic to manage "input
> locations", not present anywhere else. Also, you state that only the reporting
> functions in "qemu/error.h" should be used.
> 
> Since "qemu/error.h" internally uses 'error_report()' (from
> "qemu/error-report.h"), it includes the input location information (if any). 
> So,
> I will simply refer to "qemu/error.h" for the general reporting functions, 
> plus
> the location management functions in "qemu/error-report.h".

I don't think the location management functions need to be pointed
out as broadly speaking, no patches ever need to use them. It should
be sufficient to just describe the new error reporting functions
in no new code will ever want to use them in general. Really we
only need document the qapi/error.h functions and tell people not
to use anything else


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

Re: [Qemu-devel] [RFC] virtio: convert to use DMA api

[Michael S. Tsirkin]

On Mon, Nov 23, 2015 at 03:41:11PM +0800, Jason Wang wrote:
> Currently, all virtio devices bypass IOMMU completely. This is because
> address_space_memory is assumed and used during DMA emulation. This
> patch converts the virtio core API to use DMA API. This idea is
> 
> - introducing a new transport specific helper to query the dma address
>   space. (only pci version is implemented).
> - query and use this address space during virtio device guest memory
>   accessing
> 
> With this virtiodevices will not bypass IOMMU anymore. Little tested with
> intel_iommu=on with virtio guest DMA series posted in
> https://lkml.org/lkml/2015/10/28/64.
> 
> TODO:
> - Feature bit for this

This probably implies it should only be implemented
if device supports the modern mode.
Not sure what's the best way to handle transitional
devices.

> - Implement this for all transports

Tested with vhost, and it does not seem to work.
So more TODO:
 - make this work with vhost-user and vhost-net.

Also, it seems prudent to add
 - make the new behaviour conditional on a new property

> Signed-off-by: Jason Wang 
> ---
>  hw/block/virtio-blk.c |  2 +-
>  hw/char/virtio-serial-bus.c   |  2 +-
>  hw/scsi/virtio-scsi.c |  2 +-
>  hw/virtio/virtio-pci.c|  9 +
>  hw/virtio/virtio.c| 36 +++--
>  include/hw/virtio/virtio-access.h | 42 
> +--
>  include/hw/virtio/virtio-bus.h|  1 +
>  include/hw/virtio/virtio.h|  2 +-
>  8 files changed, 67 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e70fccf..5499480 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -846,7 +846,7 @@ static int virtio_blk_load_device(VirtIODevice *vdev, 
> QEMUFile *f,
>  req->next = s->rq;
>  s->rq = req;
>  
> -virtqueue_map(>elem);
> +virtqueue_map(vdev, >elem);
>  }
>  
>  return 0;
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 497b0af..39e9ed2 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -705,7 +705,7 @@ static int fetch_active_ports_list(QEMUFile *f, int 
> version_id,
>  
>  qemu_get_buffer(f, (unsigned char *)>elem,
>  sizeof(port->elem));
> -virtqueue_map(>elem);
> +virtqueue_map(>parent_obj, >elem);
>  
>  /*
>   *  Port was throttled on source machine.  Let's
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 7655401..0734d27 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -206,7 +206,7 @@ static void *virtio_scsi_load_request(QEMUFile *f, 
> SCSIRequest *sreq)
>  req = virtio_scsi_init_req(s, vs->cmd_vqs[n]);
>  qemu_get_buffer(f, (unsigned char *)>elem, sizeof(req->elem));
>  
> -virtqueue_map(>elem);
> +virtqueue_map(>parent_obj, >elem);
>  
>  if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
>sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 
> 0) {
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index dd48562..876505d 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1211,6 +1211,14 @@ static int virtio_pci_query_nvectors(DeviceState *d)
>  return proxy->nvectors;
>  }
>  
> +static AddressSpace *virtio_pci_get_dma_as(DeviceState *d)
> +{
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +PCIDevice *dev = >pci_dev;
> +
> +return pci_get_address_space(dev);
> +}
> +
>  static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
> struct virtio_pci_cap *cap)
>  {
> @@ -2476,6 +2484,7 @@ static void virtio_pci_bus_class_init(ObjectClass 
> *klass, void *data)
>  k->device_plugged = virtio_pci_device_plugged;
>  k->device_unplugged = virtio_pci_device_unplugged;
>  k->query_nvectors = virtio_pci_query_nvectors;
> +k->get_dma_as = virtio_pci_get_dma_as;
>  }
>  
>  static const TypeInfo virtio_pci_bus_info = {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1edef59..e09430d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -21,6 +21,7 @@
>  #include "hw/virtio/virtio-bus.h"
>  #include "migration/migration.h"
>  #include "hw/virtio/virtio-access.h"
> +#include "sysemu/dma.h"
>  
>  /*
>   * The alignment to use between consumer and producer parts of vring.
> @@ -247,6 +248,7 @@ int virtio_queue_empty(VirtQueue *vq)
>  static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
> unsigned int len)
>  {
> +AddressSpace *dma_as = virtio_get_dma_as(vq->vdev);
>  unsigned int offset;
>  int i;
>  
> @@ -254,17 +256,17 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const 
> VirtQueueElement *elem,
>  for 

Re: [Qemu-devel] [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target

[Ming Lin]

On Sat, 2015-11-21 at 14:11 +0100, Paolo Bonzini wrote:
> 
> On 20/11/2015 01:20, Ming Lin wrote:
> > One improvment could be to use google's NVMe vendor extension that
> > I send in another thread, aslo here:
> > https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
> > 
> > Qemu side:
> > http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
> > Kernel side also here:
> > https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
> 
> How much do you get with vhost-nvme plus vendor extension, compared to
> 190 MB/s for QEMU?

There is still some bug. I'll update.

> 
> Note that in all likelihood, QEMU can actually do better than 190 MB/s,
> and gain more parallelism too, by moving the processing of the
> ioeventfds to a separate thread.  This is similar to
> hw/block/dataplane/virtio-blk.c.
> 
> It's actually pretty easy to do.  Even though
> hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
> access in QEMU is now thread-safe.  I have pending patches for 2.6 that
> cut that file down to a mere 200 lines of code, NVMe would probably be
> about the same.

Is there a git tree for your patches?

Did you mean some pseduo code as below?
1. need a iothread for each cq/sq?
2. need a AioContext for each cq/sq?

 hw/block/nvme.c | 32 ++--
 hw/block/nvme.h |  8 
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f27fd35..fed4827 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -28,6 +28,8 @@
 #include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
+#include "qom/object_interfaces.h"
 
 #include "nvme.h"
 
@@ -558,9 +560,22 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
 uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
 
 event_notifier_init(>notifier, 0);
-event_notifier_set_handler(>notifier, nvme_cq_notifier);
 memory_region_add_eventfd(>iomem,
 0x1000 + offset, 4, false, 0, >notifier);
+
+object_initialize(>internal_iothread_obj,
+  sizeof(cq->internal_iothread_obj),
+  TYPE_IOTHREAD);
+user_creatable_complete(OBJECT(>internal_iothread_obj), _abort);
+cq->iothread = >internal_iothread_obj;
+cq->ctx = iothread_get_aio_context(cq->iothread);
+//Question: Need a conf.blk for each cq/sq???
+//blk_set_aio_context(cq->conf->conf.blk, cq->ctx);
+
+aio_context_acquire(cq->ctx);
+aio_set_event_notifier(cq->ctx, >notifier, true,
+   nvme_cq_notifier);
+aio_context_release(cq->ctx);
 }
 
 static void nvme_sq_notifier(EventNotifier *e)
@@ -578,9 +593,22 @@ static void nvme_init_sq_eventfd(NvmeSQueue *sq)
 uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
 
 event_notifier_init(>notifier, 0);
-event_notifier_set_handler(>notifier, nvme_sq_notifier);
 memory_region_add_eventfd(>iomem,
 0x1000 + offset, 4, false, 0, >notifier);
+
+object_initialize(>internal_iothread_obj,
+  sizeof(sq->internal_iothread_obj),
+  TYPE_IOTHREAD);
+user_creatable_complete(OBJECT(>internal_iothread_obj), _abort);
+sq->iothread = >internal_iothread_obj;
+sq->ctx = iothread_get_aio_context(sq->iothread);
+//Question: Need a conf.blk for each cq/sq???
+//blk_set_aio_context(sq->conf->conf.blk, sq->ctx);
+
+aio_context_acquire(sq->ctx);
+aio_set_event_notifier(sq->ctx, >notifier, true,
+   nvme_sq_notifier);
+aio_context_release(sq->ctx);
 }
 
 static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 608f202..171ee0b 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -667,6 +667,10 @@ typedef struct NvmeSQueue {
  * do not go over this value will not result in MMIO writes (but will
  * still write the tail pointer to the "db_addr" location above). */
 uint64_teventidx_addr;
+
+IOThread *iothread;
+IOThread internal_iothread_obj;
+AioContext *ctx;
 EventNotifier notifier;
 } NvmeSQueue;
 
@@ -690,6 +694,10 @@ typedef struct NvmeCQueue {
  * do not go over this value will not result in MMIO writes (but will
  * still write the head pointer to the "db_addr" location above). */
 uint64_teventidx_addr;
+
+IOThread *iothread;
+IOThread internal_iothread_obj;
+AioContext *ctx;
 EventNotifier notifier;
 } NvmeCQueue;
 

> 
> Paolo

Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap"

[Wen Congyang]

On 11/20/2015 05:59 PM, Fam Zheng wrote:
> "s->bitmap" tracks done sectors, we only check bit states without using any
> iterator which HBitmap is good for. Switch to "Bitmap" which is simpler and
> more memory efficient.
> 
> Meanwhile, rename it to done_bitmap, to reflect the intention.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/backup.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 3b39119..d408f98 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -22,6 +22,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
>  #include "sysemu/block-backend.h"
> +#include "qemu/bitmap.h"
>  
>  #define BACKUP_CLUSTER_BITS 16
>  #define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> @@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
>  BlockdevOnError on_target_error;
>  CoRwlock flush_rwlock;
>  uint64_t sectors_read;
> -HBitmap *bitmap;
> +unsigned long *done_bitmap;
>  QLIST_HEAD(, CowRequest) inflight_reqs;
>  } BackupBlockJob;
>  
> @@ -113,7 +114,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState 
> *bs,
>  cow_request_begin(_request, job, start, end);
>  
>  for (; start < end; start++) {
> -if (hbitmap_get(job->bitmap, start)) {
> +if (test_bit(start, job->done_bitmap)) {
>  trace_backup_do_cow_skip(job, start);
>  continue; /* already copied */
>  }
> @@ -164,7 +165,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState 
> *bs,
>  goto out;
>  }
>  
> -hbitmap_set(job->bitmap, start, 1);
> +bitmap_set(job->done_bitmap, start, 1);

You can use set_bit() here.

Thanks
Wen Congyang

>  
>  /* Publish progress, guest I/O counts as progress too.  Note that the
>   * offset field is an opaque progress value, it is not a disk offset.
> @@ -394,7 +395,7 @@ static void coroutine_fn backup_run(void *opaque)
>  start = 0;
>  end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
>  
> -job->bitmap = hbitmap_alloc(end, 0);
> +job->done_bitmap = bitmap_new(end);
>  
>  bdrv_set_enable_write_cache(target, true);
>  if (target->blk) {
> @@ -475,7 +476,7 @@ static void coroutine_fn backup_run(void *opaque)
>  /* wait until pending backup_do_cow() calls have completed */
>  qemu_co_rwlock_wrlock(>flush_rwlock);
>  qemu_co_rwlock_unlock(>flush_rwlock);
> -hbitmap_free(job->bitmap);
> +g_free(job->done_bitmap);
>  
>  if (target->blk) {
>  blk_iostatus_disable(target->blk);
> 

Re: [Qemu-devel] [PATCH] qemu-iotests: Add -nographic when starting QEMU in 119 and 120

[Fam Zheng]

On Mon, 11/23 08:33, Markus Armbruster wrote:
> Fam Zheng  writes:
> 
> > Otherwise, a window flashes on my desktop (built with SDL). Add this as
> > other cases have it.
> >
> > Signed-off-by: Fam Zheng 
> >
> > ---
> > v2: Fix 119 too. [Max]
> > ---
> >  tests/qemu-iotests/119 | 2 +-
> >  tests/qemu-iotests/120 | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/119 b/tests/qemu-iotests/119
> > index 9a11f1b..cc6ec07 100755
> > --- a/tests/qemu-iotests/119
> > +++ b/tests/qemu-iotests/119
> > @@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
> >{'execute': 'human-monitor-command',
> > 'arguments': {'command-line': 'qemu-io drv \"read -P 0 0 64k\"'}}
> >{'execute': 'quit'}" \
> > -| $QEMU -drive id=drv,if=none,file="$TEST_IMG",driver=nbd \
> > +| $QEMU -nographic -drive id=drv,if=none,file="$TEST_IMG",driver=nbd \
> >  -qmp stdio -nodefaults \
> >  | _filter_qmp | _filter_qemu_io
> >  
> > diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
> > index 9f13078..d899a3f 100755
> > --- a/tests/qemu-iotests/120
> > +++ b/tests/qemu-iotests/120
> > @@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
> >{'execute': 'human-monitor-command',
> > 'arguments': {'command-line': 'qemu-io drv \"write -P 42 0 64k\"'}}
> >{'execute': 'quit'}" \
> > -| $QEMU -qmp stdio -nodefaults \
> > +| $QEMU -qmp stdio -nographic -nodefaults \
> >  -drive 
> > id=drv,if=none,file="$TEST_IMG",driver=raw,file.driver=$IMGFMT \
> >  | _filter_qmp | _filter_qemu_io
> >  $QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
> 
> -nographic is legacy.  Using legacy options is fine, but I wonder
> whether you want -display none here.  Unless you really want to redirect
> serial and parallel port, I suspect you do.

I was just following other cases, I'm fine with either way but I suspect it's
worth to convert them. So if there is no other problem let's be consistent and
stick to -nographic for now. Thanks for pointing out, though.

Fam

Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap"

[Wen Congyang]

On 11/23/2015 05:19 PM, Fam Zheng wrote:
> On Mon, 11/23 17:01, Wen Congyang wrote:
>> On 11/20/2015 05:59 PM, Fam Zheng wrote:
>>> "s->bitmap" tracks done sectors, we only check bit states without using any
>>> iterator which HBitmap is good for. Switch to "Bitmap" which is simpler and
>>> more memory efficient.
>>>
>>> Meanwhile, rename it to done_bitmap, to reflect the intention.
>>>
>>> Signed-off-by: Fam Zheng 
>>> ---
>>>  block/backup.c | 11 ++-
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 3b39119..d408f98 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -22,6 +22,7 @@
>>>  #include "qapi/qmp/qerror.h"
>>>  #include "qemu/ratelimit.h"
>>>  #include "sysemu/block-backend.h"
>>> +#include "qemu/bitmap.h"
>>>  
>>>  #define BACKUP_CLUSTER_BITS 16
>>>  #define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
>>> @@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
>>>  BlockdevOnError on_target_error;
>>>  CoRwlock flush_rwlock;
>>>  uint64_t sectors_read;
>>> -HBitmap *bitmap;
>>> +unsigned long *done_bitmap;
>>>  QLIST_HEAD(, CowRequest) inflight_reqs;
>>>  } BackupBlockJob;
>>>  
>>> @@ -113,7 +114,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState 
>>> *bs,
>>>  cow_request_begin(_request, job, start, end);
>>>  
>>>  for (; start < end; start++) {
>>> -if (hbitmap_get(job->bitmap, start)) {
>>> +if (test_bit(start, job->done_bitmap)) {
>>>  trace_backup_do_cow_skip(job, start);
>>>  continue; /* already copied */
>>>  }
>>> @@ -164,7 +165,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState 
>>> *bs,
>>>  goto out;
>>>  }
>>>  
>>> -hbitmap_set(job->bitmap, start, 1);
>>> +bitmap_set(job->done_bitmap, start, 1);
>>
>> You can use set_bit() here.
> 
> Why? I think bitmap_set is a better match with bitmap_new below.

set_bit() is quicker than bitmap_set() if you only set one bit.

Thanks
Wen Congyang

> 
> Fam
> 
>>
>> Thanks
>> Wen Congyang
>>
>>>  
>>>  /* Publish progress, guest I/O counts as progress too.  Note that 
>>> the
>>>   * offset field is an opaque progress value, it is not a disk 
>>> offset.
>>> @@ -394,7 +395,7 @@ static void coroutine_fn backup_run(void *opaque)
>>>  start = 0;
>>>  end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
>>>  
>>> -job->bitmap = hbitmap_alloc(end, 0);
>>> +job->done_bitmap = bitmap_new(end);
>>>  
>>>  bdrv_set_enable_write_cache(target, true);
>>>  if (target->blk) {
>>> @@ -475,7 +476,7 @@ static void coroutine_fn backup_run(void *opaque)
>>>  /* wait until pending backup_do_cow() calls have completed */
>>>  qemu_co_rwlock_wrlock(>flush_rwlock);
>>>  qemu_co_rwlock_unlock(>flush_rwlock);
>>> -hbitmap_free(job->bitmap);
>>> +g_free(job->done_bitmap);
>>>  
>>>  if (target->blk) {
>>>  blk_iostatus_disable(target->blk);
>>>
>>
> .
> 

Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap"

[Fam Zheng]

On Mon, 11/23 17:24, Wen Congyang wrote:
> On 11/23/2015 05:19 PM, Fam Zheng wrote:
> > On Mon, 11/23 17:01, Wen Congyang wrote:
> >> On 11/20/2015 05:59 PM, Fam Zheng wrote:
> >>> "s->bitmap" tracks done sectors, we only check bit states without using 
> >>> any
> >>> iterator which HBitmap is good for. Switch to "Bitmap" which is simpler 
> >>> and
> >>> more memory efficient.
> >>>
> >>> Meanwhile, rename it to done_bitmap, to reflect the intention.
> >>>
> >>> Signed-off-by: Fam Zheng 
> >>> ---
> >>>  block/backup.c | 11 ++-
> >>>  1 file changed, 6 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 3b39119..d408f98 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -22,6 +22,7 @@
> >>>  #include "qapi/qmp/qerror.h"
> >>>  #include "qemu/ratelimit.h"
> >>>  #include "sysemu/block-backend.h"
> >>> +#include "qemu/bitmap.h"
> >>>  
> >>>  #define BACKUP_CLUSTER_BITS 16
> >>>  #define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> >>> @@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
> >>>  BlockdevOnError on_target_error;
> >>>  CoRwlock flush_rwlock;
> >>>  uint64_t sectors_read;
> >>> -HBitmap *bitmap;
> >>> +unsigned long *done_bitmap;
> >>>  QLIST_HEAD(, CowRequest) inflight_reqs;
> >>>  } BackupBlockJob;
> >>>  
> >>> @@ -113,7 +114,7 @@ static int coroutine_fn 
> >>> backup_do_cow(BlockDriverState *bs,
> >>>  cow_request_begin(_request, job, start, end);
> >>>  
> >>>  for (; start < end; start++) {
> >>> -if (hbitmap_get(job->bitmap, start)) {
> >>> +if (test_bit(start, job->done_bitmap)) {
> >>>  trace_backup_do_cow_skip(job, start);
> >>>  continue; /* already copied */
> >>>  }
> >>> @@ -164,7 +165,7 @@ static int coroutine_fn 
> >>> backup_do_cow(BlockDriverState *bs,
> >>>  goto out;
> >>>  }
> >>>  
> >>> -hbitmap_set(job->bitmap, start, 1);
> >>> +bitmap_set(job->done_bitmap, start, 1);
> >>
> >> You can use set_bit() here.
> > 
> > Why? I think bitmap_set is a better match with bitmap_new below.
> 
> set_bit() is quicker than bitmap_set() if you only set one bit.
> 

How much quicker is it? This doesn't sound convincing enough for me to lose the
readability.

Fam

Re: [Qemu-devel] [PATCH for 2.5 1/1] parallels: dirty BAT properly for continuous allocations

[Stefan Hajnoczi]

On Tue, Nov 17, 2015 at 08:02:58PM +0300, Denis V. Lunev wrote:
> From: Vladimir Sementsov-Ogievskiy 
> 
> This patch marks part of the BAT dirty properly. There is a possibility that
> multy-block allocation could have one block allocated on one BAT page and
> next block on the next page. The code without the patch could not save
> updated position to the file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Denis V. Lunev 
> CC: Stefan Hajnoczi 
> ---
>  block/parallels.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH] linux-user,sh4: fix signal retcode address

[Laurent Vivier]

To return from a signal, setup_frame() puts an instruction to
be executed in the stack. This sequence calls the syscall sigreturn().

The address of the instruction must be set in the PR register
to be executed.

This patch fixes this: the current code sets the register to the address
of the instruction in the host address space (which can be 64bit whereas
PR is only 32bit), but the virtual CPU can't access this address space,
so we put in PR the address of the instruction in the guest address space.

This patch also removes an useless variable (ret) in the modified functions.

Signed-off-by: Laurent Vivier 
---
 linux-user/signal.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 55e5405..5e8f6d8 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -3215,7 +3215,6 @@ static void setup_frame(int sig, struct target_sigaction 
*ka,
 struct target_sigframe *frame;
 abi_ulong frame_addr;
 int i;
-int err = 0;
 
 frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
 if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
@@ -3233,15 +3232,14 @@ static void setup_frame(int sig, struct 
target_sigaction *ka,
 regs->pr = (unsigned long) ka->sa_restorer;
 } else {
 /* Generate return code (system call to sigreturn) */
+abi_ulong retcode_addr = frame_addr +
+ offsetof(struct target_sigframe, retcode);
 __put_user(MOVW(2), >retcode[0]);
 __put_user(TRAP_NOARG, >retcode[1]);
 __put_user((TARGET_NR_sigreturn), >retcode[2]);
-regs->pr = (unsigned long) frame->retcode;
+regs->pr = (unsigned long) retcode_addr;
 }
 
-if (err)
-goto give_sigsegv;
-
 /* Set up registers for signal handler */
 regs->gregs[15] = frame_addr;
 regs->gregs[4] = sig; /* Arg for signal handler */
@@ -3264,7 +3262,6 @@ static void setup_rt_frame(int sig, struct 
target_sigaction *ka,
 struct target_rt_sigframe *frame;
 abi_ulong frame_addr;
 int i;
-int err = 0;
 
 frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
 if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
@@ -3293,15 +3290,14 @@ static void setup_rt_frame(int sig, struct 
target_sigaction *ka,
 regs->pr = (unsigned long) ka->sa_restorer;
 } else {
 /* Generate return code (system call to sigreturn) */
+abi_ulong retcode_addr = frame_addr +
+ offsetof(struct target_rt_sigframe, retcode);
 __put_user(MOVW(2), >retcode[0]);
 __put_user(TRAP_NOARG, >retcode[1]);
 __put_user((TARGET_NR_rt_sigreturn), >retcode[2]);
-regs->pr = (unsigned long) frame->retcode;
+regs->pr = (unsigned long) retcode_addr;
 }
 
-if (err)
-goto give_sigsegv;
-
 /* Set up registers for signal handler */
 regs->gregs[15] = frame_addr;
 regs->gregs[4] = sig; /* Arg for signal handler */
-- 
2.4.3

Re: [Qemu-devel] [PATCH] linux-user, sh4: fix signal retcode address

[John Paul Adrian Glaubitz]

On 11/23/2015 11:38 AM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier 

Tested-by: John Paul Adrian Glaubitz 

This patch fixes crashes of qemu-sh4* on amd64 for me [1].
I also haven't seen any regressions ever since.

Cheers,
Adrian

> [1] https://bugs.launchpad.net/ubuntu/+source/qemu-linaro/+bug/1254824

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[Qemu-devel] [PATCH for-2.5] vhost-user: clarify start and enable

[Michael S. Tsirkin]

It seems that we currently have some duplication between
started and enabled states.

The actual reason is that enable is not documented correctly:
what it does is connecting ring to the backend.

This is important for MQ, because a Linux guest expects TX
packets to be completed even if it disables some queues
temporarily.

Cc: Yuanhan Liu 
Cc: Victor Kaplansky 
Signed-off-by: Michael S. Tsirkin 
---
 docs/specs/vhost-user.txt | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 7b9cd6d..0312d40 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -148,13 +148,23 @@ a feature bit was dedicated for this purpose:
 
 Starting and stopping rings
 --
-Client must only process each ring when it is both started and enabled.
+Client must only process each ring when it is started.
+
+Client must only pass data between the ring and the
+backend, when the ring is enabled.
+
+If ring is started but disabled, client must process the
+ring without talking to the backend.
+
+For example, for a networking device, in the disabled state
+client must not supply any new RX packets, but must process
+and discard any TX packets.
 
 If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the ring is 
initialized
 in an enabled state.
 
 If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring is initialized
-in a disabled state. Client must not process it until ring is enabled by
+in a disabled state. Client must not pass data to/from the backend until ring 
is enabled by
 VHOST_USER_SET_VRING_ENABLE with parameter 1, or after it has been disabled by
 VHOST_USER_SET_VRING_ENABLE with parameter 0.
 
@@ -166,7 +176,7 @@ descriptor is readable) on the descriptor specified by
 VHOST_USER_SET_VRING_KICK, and stop ring upon receiving
 VHOST_USER_GET_VRING_BASE.
 
-While processing the rings (when they are started and enabled), client must
+While processing the rings (whether they are enabled or not), client must
 support changing some configuration aspects on the fly.
 
 Multiple queue support
@@ -309,11 +319,11 @@ Message types
   Id: 4
   Master payload: N/A
 
-  This is no longer used. Used to be sent to request stopping
+  This is no longer used. Used to be sent to request disabling
   all rings, but some clients interpreted it to also discard
   connection state (this interpretation would lead to bugs).
   It is recommended that clients either ignore this message,
-  or use it to stop all rings.
+  or use it to disable all rings.
 
  * VHOST_USER_SET_MEM_TABLE
 
-- 
MST

Re: [Qemu-devel] [PATCH] vhost-user: set link down when the char device is closed

[Jason Wang]

On 11/20/2015 01:42 PM, Wen  wrote:
> To Jason Wang:
>
> I think this patch should be for qemu-2.5
>
> Thanks
> Wen Congyang

Hi:

I thought it was for vhost tree.

Michael:

Do you want to take this patch?

>
> On 11/11/2015 02:53 PM, Wen Congyang wrote:
>> Signed-off-by: Wen Congyang 
>> ---
>>  net/vhost-user.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index 0ebd7df..5071602 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -188,7 +188,7 @@ static void net_vhost_user_event(void *opaque, int event)
>>  qmp_set_link(name, true, );
>>  break;
>>  case CHR_EVENT_CLOSED:
>> -qmp_set_link(name, true, );
>> +qmp_set_link(name, false, );
>>  vhost_user_stop(queues, ncs);
>>  break;
>>  }
>>
>

Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Michael S. Tsirkin]

On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote:
> qmp_query_memdev() has two error paths:
> 
> * When object_get_objects_root() returns null.  It never does, so
>   simply drop the useless error handling.
> 
> * When query_memdev() fails.  It leaks err then.  But any failure
>   there is actually a programming error.  Switch it to _abort,
>   and drop the useless error handling.
> 
> Messed up in commit 76b5d85 "qmp: add query-memdev".
> 
> Signed-off-by: Markus Armbruster 


Post 2.5 right?

> ---
>  numa.c | 59 ++-
>  1 file changed, 10 insertions(+), 49 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index fdfe294..1710946 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -517,7 +517,6 @@ static int query_memdev(Object *obj, void *opaque)
>  {
>  MemdevList **list = opaque;
>  MemdevList *m = NULL;
> -Error *err = NULL;
>  
>  if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
>  m = g_malloc0(sizeof(*m));
> @@ -525,72 +524,34 @@ static int query_memdev(Object *obj, void *opaque)
>  m->value = g_malloc0(sizeof(*m->value));
>  
>  m->value->size = object_property_get_int(obj, "size",
> - );
> -if (err) {
> -goto error;
> -}
> -
> + _abort);
>  m->value->merge = object_property_get_bool(obj, "merge",
> -   );
> -if (err) {
> -goto error;
> -}
> -
> +   _abort);
>  m->value->dump = object_property_get_bool(obj, "dump",
> -  );
> -if (err) {
> -goto error;
> -}
> -
> +  _abort);
>  m->value->prealloc = object_property_get_bool(obj,
> -  "prealloc", );
> -if (err) {
> -goto error;
> -}
> -
> +  "prealloc",
> +  _abort);
>  m->value->policy = object_property_get_enum(obj,
>  "policy",
>  "HostMemPolicy",
> -);
> -if (err) {
> -goto error;
> -}
> -
> +_abort);
>  object_property_get_uint16List(obj, "host-nodes",
> -   >value->host_nodes, );
> -if (err) {
> -goto error;
> -}
> +   >value->host_nodes,
> +   _abort);
>  
>  m->next = *list;
>  *list = m;
>  }
>  
>  return 0;
> -error:
> -g_free(m->value);
> -g_free(m);
> -
> -return -1;
>  }
>  
>  MemdevList *qmp_query_memdev(Error **errp)
>  {
> -Object *obj;
> +Object *obj = object_get_objects_root();
>  MemdevList *list = NULL;
>  
> -obj = object_get_objects_root();
> -if (obj == NULL) {
> -return NULL;
> -}
> -
> -if (object_child_foreach(obj, query_memdev, ) != 0) {
> -goto error;
> -}
> -
> +object_child_foreach(obj, query_memdev, );
>  return list;
> -
> -error:
> -qapi_free_MemdevList(list);
> -return NULL;
>  }
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap"

[Fam Zheng]

On Mon, 11/23 17:01, Wen Congyang wrote:
> On 11/20/2015 05:59 PM, Fam Zheng wrote:
> > "s->bitmap" tracks done sectors, we only check bit states without using any
> > iterator which HBitmap is good for. Switch to "Bitmap" which is simpler and
> > more memory efficient.
> > 
> > Meanwhile, rename it to done_bitmap, to reflect the intention.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >  block/backup.c | 11 ++-
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block/backup.c b/block/backup.c
> > index 3b39119..d408f98 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -22,6 +22,7 @@
> >  #include "qapi/qmp/qerror.h"
> >  #include "qemu/ratelimit.h"
> >  #include "sysemu/block-backend.h"
> > +#include "qemu/bitmap.h"
> >  
> >  #define BACKUP_CLUSTER_BITS 16
> >  #define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> > @@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
> >  BlockdevOnError on_target_error;
> >  CoRwlock flush_rwlock;
> >  uint64_t sectors_read;
> > -HBitmap *bitmap;
> > +unsigned long *done_bitmap;
> >  QLIST_HEAD(, CowRequest) inflight_reqs;
> >  } BackupBlockJob;
> >  
> > @@ -113,7 +114,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState 
> > *bs,
> >  cow_request_begin(_request, job, start, end);
> >  
> >  for (; start < end; start++) {
> > -if (hbitmap_get(job->bitmap, start)) {
> > +if (test_bit(start, job->done_bitmap)) {
> >  trace_backup_do_cow_skip(job, start);
> >  continue; /* already copied */
> >  }
> > @@ -164,7 +165,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState 
> > *bs,
> >  goto out;
> >  }
> >  
> > -hbitmap_set(job->bitmap, start, 1);
> > +bitmap_set(job->done_bitmap, start, 1);
> 
> You can use set_bit() here.

Why? I think bitmap_set is a better match with bitmap_new below.

Fam

> 
> Thanks
> Wen Congyang
> 
> >  
> >  /* Publish progress, guest I/O counts as progress too.  Note that 
> > the
> >   * offset field is an opaque progress value, it is not a disk 
> > offset.
> > @@ -394,7 +395,7 @@ static void coroutine_fn backup_run(void *opaque)
> >  start = 0;
> >  end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
> >  
> > -job->bitmap = hbitmap_alloc(end, 0);
> > +job->done_bitmap = bitmap_new(end);
> >  
> >  bdrv_set_enable_write_cache(target, true);
> >  if (target->blk) {
> > @@ -475,7 +476,7 @@ static void coroutine_fn backup_run(void *opaque)
> >  /* wait until pending backup_do_cow() calls have completed */
> >  qemu_co_rwlock_wrlock(>flush_rwlock);
> >  qemu_co_rwlock_unlock(>flush_rwlock);
> > -hbitmap_free(job->bitmap);
> > +g_free(job->done_bitmap);
> >  
> >  if (target->blk) {
> >  blk_iostatus_disable(target->blk);
> > 
> 

Re: [Qemu-devel] [PATCH] block/qapi: Plug memory leak on query-block error path

[Kevin Wolf]

Am 20.11.2015 um 13:53 hat Markus Armbruster geschrieben:
> Spotted by Coverity.
> 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Kevin Wolf 

Re: [Qemu-devel] PING: [PATCH] backends/hostmem: Ignore ENOSYS while setting MPOL_DEFAULT

[Paolo Bonzini]

On 23/11/2015 11:09, Paolo Bonzini wrote:
> On 23/11/2015 07:41, Pavel Fedin wrote:
>>  Hello! No news for a long time, we are at RC stage. Could we get this in?
> 
> Yes, queued for -rc2.

... doh, Eduardo applied it to the NUMA tree already.  I missed that
backends/hostmem* is under NUMA and not memory.

Paolo

> Paolo
> 
>> Kind regards,
>> Pavel Fedin
>> Expert Engineer
>> Samsung Electronics Research center Russia
>>
>>> -Original Message-
>>> From: qemu-devel-bounces+p.fedin=samsung@nongnu.org [mailto:qemu-devel-
>>> bounces+p.fedin=samsung@nongnu.org] On Behalf Of Eduardo Habkost
>>> Sent: Tuesday, October 27, 2015 8:32 PM
>>> To: Pavel Fedin
>>> Cc: 'Paolo Bonzini'; qemu-devel@nongnu.org
>>> Subject: Re: [Qemu-devel] [PATCH] backends/hostmem: Ignore ENOSYS while 
>>> setting MPOL_DEFAULT
>>>
>>> On Tue, Oct 27, 2015 at 03:51:31PM +0300, Pavel Fedin wrote:
 Currently hostmem backend fails if CONFIG_NUMA is enabled for the qemu
 (default), but NUMA is not supported by the kernel. This makes it
 impossible to use ivshmem in such configurations.

 This patch fixes the problem by ignoring ENOSYS error if policy is set to
 MPOL_DEFAULT. This way the code behaves in the same way as if CONFIG_NUMA
 was not defined. qemu will still fail if the user specifies some other
 policy, so that the user knows it.

 Signed-off-by: Pavel Fedin 
>>>
>>> Reviewed-by: Eduardo Habkost 
>>>
>>> Thanks. Applied to numa tree, with the following indentation fix:
>>>
>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>> index 94a4ac0..1b4eb45 100644
>>> --- a/backends/hostmem.c
>>> +++ b/backends/hostmem.c
>>> @@ -315,7 +315,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
>>> Error **errp)
>>>maxnode ? backend->host_nodes : NULL, maxnode + 1, 
>>> flags)) {
>>>  if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
>>>  error_setg_errno(errp, errno,
>>> - "cannot bind memory to host NUMA nodes");
>>> + "cannot bind memory to host NUMA nodes");
>>>  return;
>>>  }
>>>  }
>>>
 ---
  backends/hostmem.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

 diff --git a/backends/hostmem.c b/backends/hostmem.c
 index 41ba2af..94a4ac0 100644
 --- a/backends/hostmem.c
 +++ b/backends/hostmem.c
 @@ -313,9 +313,11 @@ host_memory_backend_memory_complete(UserCreatable 
 *uc, Error **errp)
  assert(maxnode <= MAX_NODES);
  if (mbind(ptr, sz, backend->policy,
maxnode ? backend->host_nodes : NULL, maxnode + 1, 
 flags)) {
 -error_setg_errno(errp, errno,
 +if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
 +error_setg_errno(errp, errno,
   "cannot bind memory to host NUMA nodes");
 -return;
 +return;
 +}
  }
  #endif
  /* Preallocate memory after the NUMA policy has been instantiated.
 --
 1.9.5.msysgit.0


>>>
>>> --
>>> Eduardo
>>
>>
>>
> 
> 

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> - Original Message -
>> >> Hash ivshmem been used in anger?  If yes, how?
>> 
>> Still the question to answer.
>
> I don't expect users to read this ML everyday (anybody
> actually). Personally, I have no clue how widespread ivshmem usage is.
>
>> Besides the usual PCI properties, we have:
>> 
>> ivshmem.chardev=str (ID of a chardev to use as a backend)
>> ivshmem.size=str
>> ivshmem.vectors=uint32
>> ivshmem.ioeventfd=bool (on/off)
>> ivshmem.msi=bool (on/off)
>> ivshmem.shm=str
>> ivshmem.role=str
>> ivshmem.memdev=link
>> ivshmem.use64=uint32
>> 
>> Exactly one of chardev, shm, memdev must be given.  qemu-doc.info claims
>> you can omit all three, but that's a bug.
>
> interesting, I guess the doc must be updated

One-liner.

>> vectors, ioeventfd and msi make sense only with chardev.  The code
>> appears to silently ignore them unless chardev is given.  Except it
>> still rejects ioeventfd=on,msi=off.  I wouldn't bet on nonsensical
>> combinations to actually work.
>
> I haven't tried all combinations, to me it's not a bug if an argument
> is silently ignored, but we are missing a warning.

In general, configuration that doesn't make sense should be flat-out
rejected.

A warning is appropriate when we feel rejecting would break backward
compatibility.

Non-sensical option combinations can signify badly designed interfaces.
I believe this is the case here.  More on that below.

>> qemu-doc documents role only with chardev.  The code doesn't care.
>
> yeah, role is only really useful with a server. Another missing warning.

I think it makes sense only when we can migrate the shared memory
contents out-of-band.  Vaguely similar to block devices: either you
migrate them along (block migration), or you have to ensure they have
identical contents on the target in some other way.

Is the latter possible only with a server?

>> size makes sense only with shm and chardev.  If you specify it with
>> memdev, it's ignored with a warning.
>
> Ah! it's probably fine then?

For a definition of "fine"

>> With shm, we first try to create the shared memory object with the
>> specified size, and if that fails, we try to open it.  The specified
>> size must not be larger than the shared memory object then.
>> 
>> With chardev, we receive the shared memory object from the server.
>> Until we get it, BAR isn't mapped.  If the specified size is larger, the
>> BAR remains unmapped.
>
> yep
>
>> use64 sets PCI_BASE_ADDRESS_SPACE_MEMORY.  Not documented in qemu-doc.
>
> I don't think all properties are documented, are they? Gerd added this
> in c08ba66f with pretty good commit message that we could adapt to add
> in documentation.

Yes.

>> This is a byzantine mess even for QEMU standards.
>> 
>> Questions:
>> 
>> * Is it okay to have an unmapped BAR?
>
> I can't say much, though I remember I did some tests and it was ok.

Did you try chardev=...,size=S, where S is larger than what the server
provides?

A guest that fails there probably works for sufficiently small S only
when it loses the race with the server.

But my question is actually whether it's okay for a PCI device to
advertize a BAR, and then not map anything there.

Should realize wait for the server providing the shared memory?

>> * We actually have two distinct device kinds:
>> 
>>   - The basic device: shm with parameter size, or memdev
>> 
>>   - The doorbell-capable device: chardev with parameters size, vectors,
>> ioeventfd=on, msi
>> 
>>   Common parameters: use64, role, although the latter is only documented
>>   for the doorbell-capable device.
>> 
>>   Why is this a single device model?
>
> No idea, but I agree it would make sense to have two different devices.
>
>>   It's not even obvious to me how the guest is supposed to figure out
>>   which kind it has.
>
> I don't think it can easily: I can imagine sending a doorbell to
> yourself, but that wouldn't be really reliable either.

Ugh!

>> * shm appears to be the same as memdev, just less flexible.  Why does it
>>   exist?
>
> It was there before.

Not only is memdev more flexible, it also provides the clean split
between frontend and backend we generally want.

>> * Are we *sure* this is ready to become ABI?  I doubt it's ABI yet,
>>   because before Marc-André's massive rework, it was even worse (*much*
>>   worse), to a degree that makes me doubt anybody could use it
>>   seriously.
>
> It's supposed to be the same ABI as before, with the memdev addition.

Well, it's *crap*.  We shouldn't extend and known crap so it can get
used more widely, we should deprecate it in favour of something less
crappy.

Here's my attempt:

1. Split ivshmem into ivshmem-plain and ivshmem-doorbell.

   Each one gets its own PCI device ID, so that guests can trivially see
   whether the device got a doorbell.

   Both have property use64.

   ivshmem-plain additionally has property memdev.

   ivshmem-doorbell 

[Qemu-devel] [PULL for-2.5] tcg: Fix highwater check

[Richard Henderson]

From: John Clarke 

A simple typo in the variable to use when comparing vs the highwater mark.
Reports are that qemu can in fact segfault occasionally due to this mistake.

Signed-off-by: John Clarke 
Signed-off-by: Richard Henderson 
---
 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 682af8a..b20ed19 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2443,7 +2443,7 @@ int tcg_gen_code(TCGContext *s, tcg_insn_unit 
*gen_code_buf)
one operation beginning below the high water mark cannot overrun
the buffer completely.  Thus we can test for overflow after
generating code without having to check during generation.  */
-if (unlikely(s->code_gen_ptr > s->code_gen_highwater)) {
+if (unlikely((void *)s->code_ptr > s->code_gen_highwater)) {
 return -1;
 }
 }
-- 
2.4.3

[Qemu-devel] [PULL for-2.5] last minute tcg fix

[Richard Henderson]

Sent to me privately, for some reason, but absolutely correct
that it can occasionally cause problems.


r~


The following changes since commit 541abd10a01da56c5f16582cd32d67114ec22a5c:

  Update version for v2.5.0-rc1 release (2015-11-20 17:43:46 +)

are available in the git repository at:

  git://github.com/rth7680/qemu.git tags/pull-tcg-20151123

for you to fetch changes up to 644da9b39e477caa80bab69d2847dfcb468f0d33:

  tcg: Fix highwater check (2015-11-23 13:16:05 +0100)


Last minute fix.


John Clarke (1):
  tcg: Fix highwater check

 tcg/tcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

[Qemu-devel] [PULL 0/1] NUMA fix for -rc2

[Eduardo Habkost]

(I forgot to submit a pull request for this earlier, sorry.)

The following changes since commit 541abd10a01da56c5f16582cd32d67114ec22a5c:

  Update version for v2.5.0-rc1 release (2015-11-20 17:43:46 +)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/numa-pull-request

for you to fetch changes up to a3567ba1e6171ef7cfad55ae549c0cd8bffb1195:

  hostmem: Ignore ENOSYS while setting MPOL_DEFAULT (2015-11-23 10:43:38 -0200)


NUMA fix for -rc2



Pavel Fedin (1):
  hostmem: Ignore ENOSYS while setting MPOL_DEFAULT

 backends/hostmem.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] Don't require hw_version to be set on every machine

[Eduardo Habkost]

Ping? Any thoughts on this approach?

-rc2 is too late for this kind of change, maybe?


On Thu, Nov 12, 2015 at 03:29:53PM -0200, Eduardo Habkost wrote:
> This changes the qemu_hw_version() default to "2.5+" so we don't
> make every machine class broken by default unless they set
> hw_version.
> 
> For reference, these are the current users of qemu_hw_version():
> 
> hw/arm/nseries.c:pstrcat((void *) w, 12, qemu_hw_version()); /* char 
> version[12] */
> hw/ide/core.c:pstrcpy(s->version, sizeof(s->version), 
> qemu_hw_version());
> hw/scsi/megasas.c:snprintf(info.package_version, 0x60, "%s-QEMU", 
> qemu_hw_version());
> hw/scsi/scsi-bus.c:pstrcpy((char *) >buf[32], 4, 
> qemu_hw_version());
> hw/scsi/scsi-disk.c:s->version = g_strdup(qemu_hw_version());
> target-i386/cpu.c:qemu_hw_version());
> 
> Cc: Andrzej Zaborowski 
> Cc: John Snow 
> Cc: Paolo Bonzini 
> Cc: Hannes Reinecke 
> Cc: Richard Henderson 
> 
> Eduardo Habkost (2):
>   osdep: Change default value of qemu_hw_version() to "2.5+"
>   pc: Don't set hw_version on pc-*-2.5
> 
>  hw/i386/pc_piix.c| 1 -
>  hw/i386/pc_q35.c | 1 -
>  include/hw/boards.h  | 5 +
>  include/qemu/osdep.h | 4 
>  util/osdep.c | 9 -
>  5 files changed, 17 insertions(+), 3 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

-- 
Eduardo

[Qemu-devel] [PULL 1/1] hostmem: Ignore ENOSYS while setting MPOL_DEFAULT

[Eduardo Habkost]

From: Pavel Fedin 

Currently hostmem backend fails if CONFIG_NUMA is enabled in QEMU
(the default) but NUMA is not supported by the kernel. This makes
it impossible to use ivshmem in such configurations.

This patch fixes the problem by ignoring ENOSYS error if policy is set to
MPOL_DEFAULT. This way the code behaves in the same way as if CONFIG_NUMA
was not defined. qemu will still fail if the user specifies some other
policy, so that the user knows it.

Signed-off-by: Pavel Fedin 
Reviewed-by: Eduardo Habkost 
Signed-off-by: Eduardo Habkost 
---
 backends/hostmem.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 41ba2af..1b4eb45 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -313,9 +313,11 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
Error **errp)
 assert(maxnode <= MAX_NODES);
 if (mbind(ptr, sz, backend->policy,
   maxnode ? backend->host_nodes : NULL, maxnode + 1, flags)) {
-error_setg_errno(errp, errno,
- "cannot bind memory to host NUMA nodes");
-return;
+if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
+error_setg_errno(errp, errno,
+ "cannot bind memory to host NUMA nodes");
+return;
+}
 }
 #endif
 /* Preallocate memory after the NUMA policy has been instantiated.
-- 
2.1.0

Re: [Qemu-devel] [PULL for-2.5] tcg: Fix highwater check

[Stefan Weil]

Am 23.11.2015 um 13:45 schrieb Richard Henderson:
> From: John Clarke 
> 
> A simple typo in the variable to use when comparing vs the highwater mark.
> Reports are that qemu can in fact segfault occasionally due to this mistake.
> 
> Signed-off-by: John Clarke 
> Signed-off-by: Richard Henderson 
> ---
>  tcg/tcg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 682af8a..b20ed19 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -2443,7 +2443,7 @@ int tcg_gen_code(TCGContext *s, tcg_insn_unit 
> *gen_code_buf)
> one operation beginning below the high water mark cannot overrun
> the buffer completely.  Thus we can test for overflow after
> generating code without having to check during generation.  */
> -if (unlikely(s->code_gen_ptr > s->code_gen_highwater)) {
> +if (unlikely((void *)s->code_ptr > s->code_gen_highwater)) {
>  return -1;
>  }
>  }
> 

Is a comparison of void pointers portable? Or would it be better
to cast both sides to uintptr_t? Or fix the declaration of
code_gen_highwater to use an uint8_t pointer and cast s->code_ptr
to that type? code_gen_highwater should be fixed anyway because
in translate-all a difference is calculated with it.

Stefan

Re: [Qemu-devel] [PATCH v5 00/10] sPAPR CPU hotplug

[Peter Krempa]

On Fri, Nov 20, 2015 at 18:24:29 +0530, Bharata B Rao wrote:
> This patchset adds CPU hotplug support for sPAPR PowerPC guests using
> device_add and device_del commands
> 
> (qemu) device_add POWER8-powerpc64-cpu,id=cpu0

Is there a reason why this uses 'device_add' rather than the 'cpu_add'
command? Libvirt uses two separate approaches already. Due to legacy
reasons we support the HMP 'cpu_set' command, and lately we added
support for QMP 'cpu-add'. Using device_add here will introduce a
different approach and will require yet another compatibility layer in
libvirt to support this.

Peter



signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH WIP 04/30] qcow2: add a 'keyid' parameter to qcow2 options

[Daniel P. Berrange]

On Fri, Nov 20, 2015 at 03:15:27PM -0700, Eric Blake wrote:
> On 11/20/2015 11:04 AM, Daniel P. Berrange wrote:
> > Add a 'keyid' parameter that refers to the ID of a
> > QCryptoSecret instance that provides the encryption key.
> > 
> > $QEMU \
> > -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
> > -drive file=/home/berrange/encrypted.qcow2,keyid=sec0
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/qcow2.c| 80 
> > +---
> >  block/qcow2.h|  1 +
> >  qapi/block-core.json |  8 --
> >  3 files changed, 64 insertions(+), 25 deletions(-)
> 
> > +++ b/qapi/block-core.json
> > @@ -1698,7 +1698,7 @@
> >  # Driver specific block device options for qcow.
> >  #
> >  # @keyid: #optional ID of the "secret" object providing the
> > -# AES decryption key.
> > +# AES decryption key (since 2.5)
> >  #
> >  # Since: 2.5
> 
> I already pointed this out on the previous post, but this hunk is wrong
> (since the entire BlockdevOptionsQcow struct is new); it instead belongs...
> 
> >  ##
> > @@ -1742,6 +1742,9 @@
> >  # caches. The interval is in seconds. The default 
> > value
> >  # is 0 and it disables this feature (since 2.5)
> >  #
> > +# @keyid: #optional ID of the "secret" object providing the
> > +# AES decryption key.
> 
> ...here as part of BlockdevOptionsQcow2.  Also, I wonder if inheriting
> from BlockdevOptionsQcow is any easier here than just declaring keyid
> directly.

When I fully integrate LUKS support in qcow2, there will be several
more parameters added to this struct, which I won't be adding to
qcow, since I don't fancy doing any work on qcow code to improve
its encryption, since its essentially obsolte. So on this basis,
I don't think inheriting BlockdevOptionsQcow will have tangible
benefit.


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

Re: [Qemu-devel] [PATCH 0/2] Don't require hw_version to be set on every machine

[Michael S. Tsirkin]

On Mon, Nov 23, 2015 at 10:53:39AM -0200, Eduardo Habkost wrote:
> 
> Ping? Any thoughts on this approach?
> 
> -rc2 is too late for this kind of change, maybe?
> 

I queued this up for 2.5, will merge later this week
unless anyone objects.

> On Thu, Nov 12, 2015 at 03:29:53PM -0200, Eduardo Habkost wrote:
> > This changes the qemu_hw_version() default to "2.5+" so we don't
> > make every machine class broken by default unless they set
> > hw_version.
> > 
> > For reference, these are the current users of qemu_hw_version():
> > 
> > hw/arm/nseries.c:pstrcat((void *) w, 12, qemu_hw_version()); /* char 
> > version[12] */
> > hw/ide/core.c:pstrcpy(s->version, sizeof(s->version), 
> > qemu_hw_version());
> > hw/scsi/megasas.c:snprintf(info.package_version, 0x60, "%s-QEMU", 
> > qemu_hw_version());
> > hw/scsi/scsi-bus.c:pstrcpy((char *) >buf[32], 4, 
> > qemu_hw_version());
> > hw/scsi/scsi-disk.c:s->version = g_strdup(qemu_hw_version());
> > target-i386/cpu.c:qemu_hw_version());
> > 
> > Cc: Andrzej Zaborowski 
> > Cc: John Snow 
> > Cc: Paolo Bonzini 
> > Cc: Hannes Reinecke 
> > Cc: Richard Henderson 
> > 
> > Eduardo Habkost (2):
> >   osdep: Change default value of qemu_hw_version() to "2.5+"
> >   pc: Don't set hw_version on pc-*-2.5
> > 
> >  hw/i386/pc_piix.c| 1 -
> >  hw/i386/pc_q35.c | 1 -
> >  include/hw/boards.h  | 5 +
> >  include/qemu/osdep.h | 4 
> >  util/osdep.c | 9 -
> >  5 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > -- 
> > 2.1.0
> > 
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v5 00/10] sPAPR CPU hotplug

[Christian Borntraeger]

On 11/23/2015 12:54 PM, Peter Krempa wrote:
> On Fri, Nov 20, 2015 at 18:24:29 +0530, Bharata B Rao wrote:
>> This patchset adds CPU hotplug support for sPAPR PowerPC guests using
>> device_add and device_del commands
>>
>> (qemu) device_add POWER8-powerpc64-cpu,id=cpu0
> 
> Is there a reason why this uses 'device_add' rather than the 'cpu_add'
> command? Libvirt uses two separate approaches already. Due to legacy
> reasons we support the HMP 'cpu_set' command, and lately we added
> support for QMP 'cpu-add'. Using device_add here will introduce a
> different approach and will require yet another compatibility layer in
> libvirt to support this.

s390 and powerpc both started with cpu_add patches. Andreas Faerber
suggested then to only implement device_add. This was apparently discussed
at the last KVM forum.

Christian

Re: [Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Markus Armbruster]

"Michael S. Tsirkin"  writes:

> On Mon, Nov 23, 2015 at 09:35:31AM +0100, Markus Armbruster wrote:
>> qmp_query_memdev() has two error paths:
>> 
>> * When object_get_objects_root() returns null.  It never does, so
>>   simply drop the useless error handling.
>> 
>> * When query_memdev() fails.  It leaks err then.  But any failure
>>   there is actually a programming error.  Switch it to _abort,
>>   and drop the useless error handling.
>> 
>> Messed up in commit 76b5d85 "qmp: add query-memdev".
>> 
>> Signed-off-by: Markus Armbruster 
>
>
> Post 2.5 right?

I don't mind.  I meant v1 for 2.5, but we've since convinced ourselves
that errors can't happen, and the memory leak is only latent.

Re: [Qemu-devel] [PATCH for-2.5] target-arm: Don't mask out bits [47:40] in LPAE descriptors for v8

[Edgar E. Iglesias]

On Fri, Nov 20, 2015 at 02:32:51PM +, Peter Maydell wrote:
> In an LPAE format descriptor in ARMv8 the address field extends
> up to bit 47, not just bit 39. Correct the masking so we don't
> give incorrect results if the output address size is greater
> than 40 bits, as it can be for AArch64.
> 
> (Note that we don't yet support the new-in-v8 Address Size fault which
> should be generated if any translation table entry or TTBR contains
> an address with non-zero bits above the most significant bit of the
> maximum output address size.)
> 
> Signed-off-by: Peter Maydell 


Reviewed-by: Edgar E. Iglesias 


> ---
> This is worth fixing for 2.5 I think. As the commit message notes,
> we don't support the Addres Size faults we ought to take in some
> cases, but that seems more 2.6-ish.
> ---
>  target-arm/helper.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4ecae61..afc4163 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6642,6 +6642,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>  int ap, ns, xn, pxn;
>  uint32_t el = regime_el(env, mmu_idx);
>  bool ttbr1_valid = true;
> +uint64_t descaddrmask;
>  
>  /* TODO:
>   * This code does not handle the different format TCR for VTCR_EL2.
> @@ -6831,6 +6832,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>  descaddr = extract64(ttbr, 0, 48);
>  descaddr &= ~((1ULL << (inputsize - (stride * (4 - level - 1);
>  
> +/* The address field in the descriptor goes up to bit 39 for ARMv7
> + * but up to bit 47 for ARMv8.
> + */
> +if (arm_feature(env, ARM_FEATURE_V8)) {
> +descaddrmask = 0xf000ULL;
> +} else {
> +descaddrmask = 0xfff000ULL;
> +}
> +
>  /* Secure accesses start with the page table in secure memory and
>   * can be downgraded to non-secure at any step. Non-secure accesses
>   * remain non-secure. We implement this by just ORing in the NSTable/NS
> @@ -6854,7 +6864,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>  /* Invalid, or the Reserved level 3 encoding */
>  goto do_fault;
>  }
> -descaddr = descriptor & 0xfff000ULL;
> +descaddr = descriptor & descaddrmask;
>  
>  if ((descriptor & 2) && (level < 3)) {
>  /* Table entry. The top five bits are attributes which  may
> -- 
> 1.9.1
> 

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Marc-André Lureau]

Hi

- Original Message -
>
> >> qemu-doc documents role only with chardev.  The code doesn't care.
> >
> > yeah, role is only really useful with a server. Another missing warning.
> 
> I think it makes sense only when we can migrate the shared memory
> contents out-of-band.  Vaguely similar to block devices: either you
> migrate them along (block migration), or you have to ensure they have
> identical contents on the target in some other way.
> 
> Is the latter possible only with a server?

The way ivshmem role works is:
- master: will handle migration
- peer: can't migrate

It's not out-of-band, qemu handles the shared memory migration, even if
the memory is owned by the server. In practice, it means you can only
migrate one VM, or you migrate them all but you will migrate the shared
memory N times and will have to synchronize in your guest app. I suppose ivshmem
"role" was designed to only migrate the master. Ability to migrate a pool of
peer would be a significant new feature. I am not aware of such request.

> >> This is a byzantine mess even for QEMU standards.
> >> 
> >> Questions:
> >> 
> >> * Is it okay to have an unmapped BAR?
> >
> > I can't say much, though I remember I did some tests and it was ok.
> 
> Did you try chardev=...,size=S, where S is larger than what the server
> provides?

It will fall in check_shm_size().

> A guest that fails there probably works for sufficiently small S only
> when it loses the race with the server.
> 
> But my question is actually whether it's okay for a PCI device to
> advertize a BAR, and then not map anything there.
> 
> Should realize wait for the server providing the shared memory?
> 
> >> * We actually have two distinct device kinds:
> >> 
> >>   - The basic device: shm with parameter size, or memdev
> >> 
> >>   - The doorbell-capable device: chardev with parameters size, vectors,
> >> ioeventfd=on, msi
> >> 
> >>   Common parameters: use64, role, although the latter is only documented
> >>   for the doorbell-capable device.
> >> 

> 
> >> * Are we *sure* this is ready to become ABI?  I doubt it's ABI yet,
> >>   because before Marc-André's massive rework, it was even worse (*much*
> >>   worse), to a degree that makes me doubt anybody could use it
> >>   seriously.
> >
> > It's supposed to be the same ABI as before, with the memdev addition.
> 
> Well, it's *crap*.  We shouldn't extend and known crap so it can get
> used more widely, we should deprecate it in favour of something less
> crappy.

I think you overstate this, if it would be so bad I don't think anyone could use
it and it wouldn't probably be in qemu. We are mainly talking misconfiguration
and missing features (that people may not actually need). I think
before we split things, we can address your comments with more
options checking and documentation.

> Here's my attempt:
> 
> 1. Split ivshmem into ivshmem-plain and ivshmem-doorbell.
> 
>Each one gets its own PCI device ID, so that guests can trivially see
>whether the device got a doorbell.
> 
>Both have property use64.
> 
>ivshmem-plain additionally has property memdev.
> 
>ivshmem-doorbell additionally has chardev, size, vectors, ioeventfd,
>msi.
> 
>Undecided: property role (see above).
> 
>The only non-sensical property combination is ioeventfd=on,msi=off,
>which we cleanly reject.
> 
>Undecided: behavior before the server provides the shared memory, and
>what to do when it's less than size (see above).  This is the *only*
>part of my proposal that may require code changes beyond
>configuration.  If we can't do this before the release, punt
>ivshmem-doorbell to the next cycle.

> 2. Drop ivshmem property memdev.
> 
>Use ivshmem-plain instead.
> 
> 3. Deprecate ivshmem.

It sounds like a reasonable thing to do, but I don't think the benefit is so 
important.

cheers

Re: [Qemu-devel] [PATCH WIP 01/30] crypto: add QCryptoSecret object class for password/key handling

[Daniel P. Berrange]

On Fri, Nov 20, 2015 at 03:09:25PM -0700, Eric Blake wrote:
> On 11/20/2015 11:04 AM, Daniel P. Berrange wrote:
> > +
> > +static const char *base64_valid_chars =
> > +"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=";
> > +
> > +static int
> > +qcrypto_secret_validate_base64(const uint8_t *input,
> > +   size_t inputlen,
> > +   Error **errp)
> 
> Don't we already have base64 utility methods available?

We normally use glib,  g_base64_encode/decode. Unfortunately the
decode method doesn't provide any usefull error reporting facility.
It just silently skips any characters that are outside the valid
set.  So the only way I could get any kind of sensible error report
was to do this validation myself against the set of permitted base64
characters.


> > +++ b/qapi/crypto.json
> > @@ -19,3 +19,17 @@
> >  { 'enum': 'QCryptoTLSCredsEndpoint',
> >'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
> >'data': ['client', 'server']}
> > +
> > +
> > +##
> > +# QCryptoSecretFormat:
> > +#
> > +# The data format that the secret is provided in
> > +#
> > +# @raw: raw bytes. When encoded in JSON only valid UTF-8 sequences can be 
> > used
> > +# @base64: arbitrary base64 encoded binary data
> > +# Since: 2.5
> 
> You've missed 2.5.  Probably need to tweak the whole series to call out 2.6.

Yep.

> > +##
> > +{ 'enum': 'QCryptoSecretFormat',
> > +  'prefix': 'QCRYPTO_SECRET_FORMAT',
> > +  'data': ['raw', 'base64']}
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 0eea4ee..dd3f7f8 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -3670,6 +3670,7 @@ queue @var{all|rx|tx} is an option that can be 
> > applied to any netfilter.
> >  @option{tx}: the filter is attached to the transmit queue of the netdev,
> >   where it will receive packets sent by the netdev.
> >  
> > +
> >  @item -object 
> > filter-dump,id=@var{id},netdev=@var{dev},file=@var{filename}][,maxlen=@var{len}]
> 
> Why the added blank line here?

Rebase error I presume

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

[Qemu-devel] [PATCH 1/2] tests/Makefile: Add more dependencies for test-timed-average

[Kevin Wolf]

'make check' failed to compile the test case for mingw because of
undefined references. Pull in a few more dependencies so that it builds.

Signed-off-by: Kevin Wolf 
---
 tests/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index b937984..47fe5d5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -416,7 +416,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
$(test-qom-obj-y)
 tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
libqemuutil.a stubs/clock-warp.o stubs/cpu-get-icount.o \
-   stubs/notify-event.o stubs/replay.o
+   stubs/notify-event.o stubs/replay.o stubs/mon-is-qmp.o 
stubs/fd-register.o \
+   stubs/mon-printf.o
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json 
$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.5 0/2] Fix "make check" with mingw and Wine

[Kevin Wolf]

I tried to run "make check" on my mingw build and got two failures. Both of
them are bugs in the test suite rather than qemu proper. They are easy enough
to fix, so here are the fixes.

Kevin Wolf (2):
  tests/Makefile: Add more dependencies for test-timed-average
  test-aio: Fix event notifier cleanup

 tests/Makefile   | 3 ++-
 tests/test-aio.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] test-aio: Fix event notifier cleanup

[Kevin Wolf]

One test case closed an event notifier (event_notifier_cleanup())
without first disabling it (set_event_notifier(..., NULL)). This
resulted in a leftover handle 0 that was added to each subsequent
WaitForMultipleObjects() call, causing the function to fail (invalid
handle).

Signed-off-by: Kevin Wolf 
---
 tests/test-aio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-aio.c b/tests/test-aio.c
index 1623803..e188d8c 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -393,6 +393,7 @@ static void test_aio_external_client(void)
 aio_enable_external(ctx);
 }
 assert(aio_poll(ctx, false));
+set_event_notifier(ctx, , NULL);
 event_notifier_cleanup();
 }
 }
-- 
1.8.3.1

Re: [Qemu-devel] PING: [PATCH] backends/hostmem: Ignore ENOSYS while setting MPOL_DEFAULT

[Eduardo Habkost]

On Mon, Nov 23, 2015 at 11:11:28AM +0100, Paolo Bonzini wrote:
> 
> 
> On 23/11/2015 11:09, Paolo Bonzini wrote:
> > On 23/11/2015 07:41, Pavel Fedin wrote:
> >>  Hello! No news for a long time, we are at RC stage. Could we get this in?
> > 
> > Yes, queued for -rc2.
> 
> ... doh, Eduardo applied it to the NUMA tree already.  I missed that
> backends/hostmem* is under NUMA and not memory.

I had applied it to the numa tree but forgot to send a pull
request, sorry. I will submit a pull request in a moment.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v8 0/5] implement vNVDIMM

[Stefan Hajnoczi]

On Thu, Nov 19, 2015 at 10:39:05AM +0800, Xiao Guangrong wrote:
> On 11/19/2015 04:44 AM, Michael S. Tsirkin wrote:
> >On Wed, Nov 18, 2015 at 05:18:17PM -0200, Eduardo Habkost wrote:
> >>On Wed, Nov 18, 2015 at 09:59:34AM +0800, Xiao Guangrong wrote:
> >sorry, I'm busy with 2.5 now, and this is clearly not 2.5 material.
> 
> I still see some pull requests were send our for 2.5 merge window today and
> yesterday ...
> 
> This patchset is the simplest version we can figure out to implement basic
> functionality for vNVDIMM and only minor change is needed for other code.
> It would be nice and really appreciate if it can go to 2.5.

Here is the release schedule:
http://qemu-project.org/Planning/2.5

QEMU is in hard freeze right now.  That means only critical bug fixes
are being merged.  No new features will be merged until the QEMU 2.6
development cycle begins.

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v4 0/5] Connect the SPI devices to ZynqMP

[Alistair Francis]

Connect the SPI devices to Xilinx's ZynqMP.

I also need to make some changes to the actual SPI device to
imporove the fuctionality, but for the time being this works.

V4:
 - Rebase
 - Rename the SPI busses so that they can all be accessed from the SoC
 - Only create one SPI flash device
V3:
 - Don't reach into the SoC to get the SPI Bus
V2:
 - Connect the SPI flash in the board code
 - Update git patches to properly indicate rename
 - Add sst25wf080 as a SPI flash


Alistair Francis (5):
  m25p80.c: Add sst25wf080 SPI flash device
  ssi: Move ssi.h into a separate directory
  xilinx_spips: Seperate the state struct into a header
  xlnx-zynqmp: Connect the SPI devices
  xlnx-ep108: Connect the SPI Flash

 hw/arm/pxa2xx.c |  2 +-
 hw/arm/spitz.c  |  2 +-
 hw/arm/stellaris.c  |  2 +-
 hw/arm/strongarm.c  |  2 +-
 hw/arm/tosa.c   |  2 +-
 hw/arm/xilinx_zynq.c|  2 +-
 hw/arm/xlnx-ep108.c | 16 +
 hw/arm/xlnx-zynqmp.c| 38 
 hw/arm/z2.c |  2 +-
 hw/block/m25p80.c   |  3 +-
 hw/display/ads7846.c|  2 +-
 hw/display/ssd0323.c|  2 +-
 hw/microblaze/petalogix_ml605_mmu.c |  2 +-
 hw/misc/max111x.c   |  2 +-
 hw/sd/ssi-sd.c  |  2 +-
 hw/ssi/pl022.c  |  2 +-
 hw/ssi/ssi.c|  2 +-
 hw/ssi/xilinx_spi.c |  2 +-
 hw/ssi/xilinx_spips.c   | 48 +++--
 include/hw/arm/xlnx-zynqmp.h|  3 ++
 include/hw/{ => ssi}/ssi.h  | 10 +++---
 include/hw/ssi/xilinx_spips.h   | 72 +
 22 files changed, 157 insertions(+), 63 deletions(-)
 rename include/hw/{ => ssi}/ssi.h (96%)
 create mode 100644 include/hw/ssi/xilinx_spips.h

-- 
2.5.0

[Qemu-devel] [PATCH v4 5/5] xlnx-ep108: Connect the SPI Flash

[Alistair Francis]

Connect the sst25wf080 SPI flash to the EP108 board.

Signed-off-by: Alistair Francis 
---
V4:
 - Only add one SPI flash
V3:
 - Don't reach into the SoC
V2:
 - Use sst25wf080 instead of m25p80

 hw/arm/xlnx-ep108.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 2899698..4ead0df 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -33,6 +33,7 @@ static struct arm_boot_info xlnx_ep108_binfo;
 static void xlnx_ep108_init(MachineState *machine)
 {
 XlnxEP108 *s = g_new0(XlnxEP108, 1);
+int i;
 Error *err = NULL;
 
 object_initialize(>soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
@@ -60,6 +61,21 @@ static void xlnx_ep108_init(MachineState *machine)
  machine->ram_size);
 memory_region_add_subregion(get_system_memory(), 0, >ddr_ram);
 
+for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) {
+SSIBus *spi_bus;
+DeviceState *flash_dev;
+qemu_irq cs_line;
+char bus_name[6];
+
+snprintf(bus_name, 6, "spi%d", i);
+spi_bus = (SSIBus *)qdev_get_child_bus(DEVICE(>soc), bus_name);
+
+flash_dev = ssi_create_slave(spi_bus, "sst25wf080");
+cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>soc.spi[i]), 1, cs_line);
+}
+
 xlnx_ep108_binfo.ram_size = machine->ram_size;
 xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
 xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
-- 
2.5.0

[Qemu-devel] [PATCH v4 1/5] m25p80.c: Add sst25wf080 SPI flash device

[Alistair Francis]

Add the sst25wf080 SPI flash device.

Signed-off-by: Alistair Francis 
Reviewed-by: Peter Crosthwaite 
---

 hw/block/m25p80.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index efc43dd..7b9f97c 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -163,6 +163,7 @@ static const FlashPartInfo known_devices[] = {
 { INFO("sst25wf010",  0xbf2502,  0,  64 << 10,   2, ER_4K) },
 { INFO("sst25wf020",  0xbf2503,  0,  64 << 10,   4, ER_4K) },
 { INFO("sst25wf040",  0xbf2504,  0,  64 << 10,   8, ER_4K) },
+{ INFO("sst25wf080",  0xbf2505,  0,  64 << 10,  16, ER_4K) },
 
 /* ST Microelectronics -- newer production may have feature updates */
 { INFO("m25p05",  0x202010,  0,  32 << 10,   2, 0) },
-- 
2.5.0

[Qemu-devel] [PATCH v4 2/5] ssi: Move ssi.h into a separate directory

[Alistair Francis]

Move the ssi.h include file into the ssi directory.

While touching the code also fix the typdef lines as
checkpatch complains.

Signed-off-by: Alistair Francis 
Reviewed-by: Peter Crosthwaite 
---
V2:
 - Change git patch to indicate rename

 hw/arm/pxa2xx.c |  2 +-
 hw/arm/spitz.c  |  2 +-
 hw/arm/stellaris.c  |  2 +-
 hw/arm/strongarm.c  |  2 +-
 hw/arm/tosa.c   |  2 +-
 hw/arm/xilinx_zynq.c|  2 +-
 hw/arm/z2.c |  2 +-
 hw/block/m25p80.c   |  2 +-
 hw/display/ads7846.c|  2 +-
 hw/display/ssd0323.c|  2 +-
 hw/microblaze/petalogix_ml605_mmu.c |  2 +-
 hw/misc/max111x.c   |  2 +-
 hw/sd/ssi-sd.c  |  2 +-
 hw/ssi/pl022.c  |  2 +-
 hw/ssi/ssi.c|  2 +-
 hw/ssi/xilinx_spi.c |  2 +-
 hw/ssi/xilinx_spips.c   |  2 +-
 include/hw/{ => ssi}/ssi.h  | 10 ++
 18 files changed, 23 insertions(+), 21 deletions(-)
 rename include/hw/{ => ssi}/ssi.h (96%)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 79d22d9..54bf152 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -12,7 +12,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/char/serial.h"
 #include "hw/i2c/i2c.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "sysemu/char.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 8d3cc0b..ee8f889 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -16,7 +16,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/pcmcia.h"
 #include "hw/i2c/i2c.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "hw/block/flash.h"
 #include "qemu/timer.h"
 #include "hw/devices.h"
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 0114e0a..4e5cfd1 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -8,7 +8,7 @@
  */
 
 #include "hw/sysbus.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "hw/arm/arm.h"
 #include "hw/devices.h"
 #include "qemu/timer.h"
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index 9624ecb..4d2ba02 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -34,7 +34,7 @@
 #include "hw/arm/arm.h"
 #include "sysemu/char.h"
 #include "sysemu/sysemu.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 
 //#define DEBUG
 
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 02814d7..68ad01e 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -19,7 +19,7 @@
 #include "hw/pcmcia.h"
 #include "hw/boards.h"
 #include "hw/i2c/i2c.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "sysemu/block-backend.h"
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1c1a445..11a349b 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -25,7 +25,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/loader.h"
 #include "hw/misc/zynq-xadc.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "qemu/error-report.h"
 
 #define NUM_SPI_FLASHES 4
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index b44eb76..c82fe2c 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -16,7 +16,7 @@
 #include "hw/arm/arm.h"
 #include "hw/devices.h"
 #include "hw/i2c/i2c.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "hw/boards.h"
 #include "sysemu/sysemu.h"
 #include "hw/block/flash.h"
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 7b9f97c..addd907 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -24,7 +24,7 @@
 #include "hw/hw.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 
 #ifndef M25P80_ERR_DEBUG
 #define M25P80_ERR_DEBUG 0
diff --git a/hw/display/ads7846.c b/hw/display/ads7846.c
index 3f35369..cb82317 100644
--- a/hw/display/ads7846.c
+++ b/hw/display/ads7846.c
@@ -10,7 +10,7 @@
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "ui/console.h"
 
 typedef struct {
diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index 9727007..7545da8 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -10,7 +10,7 @@
 /* The controller can support a variety of different displays, but we only
implement one.  Most of the commends relating to brightness and geometry
setup are ignored. */
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 #include "ui/console.h"
 
 //#define DEBUG_SSD0323 1
diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
b/hw/microblaze/petalogix_ml605_mmu.c
index 462060f..5366cec 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -35,7 +35,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/char/serial.h"
 #include "exec/address-spaces.h"
-#include "hw/ssi.h"
+#include "hw/ssi/ssi.h"
 
 #include 

Re: [Qemu-devel] [RFC] virtio: convert to use DMA api

[Michael S. Tsirkin]

On Mon, Nov 23, 2015 at 10:36:45AM +0100, Cornelia Huck wrote:
> On Mon, 23 Nov 2015 15:41:11 +0800
> Jason Wang  wrote:
> 
> > Currently, all virtio devices bypass IOMMU completely. This is because
> > address_space_memory is assumed and used during DMA emulation. This
> > patch converts the virtio core API to use DMA API. This idea is
> > 
> > - introducing a new transport specific helper to query the dma address
> >   space. (only pci version is implemented).
> > - query and use this address space during virtio device guest memory
> >   accessing
> > 
> > With this virtiodevices will not bypass IOMMU anymore. Little tested with
> > intel_iommu=on with virtio guest DMA series posted in
> > https://lkml.org/lkml/2015/10/28/64.
> > 
> > TODO:
> > - Feature bit for this
> 
> I'm still not convinced about that feature bit stuff. It just feels
> wrong to use a mechanism that conveys negotiable device features to
> configure what is basically a platform/hypervisor feature. I'd rather
> see this out of the virtio layer and into the pci layer.

I agree it's not something that needs to be negotiated.

But given that we are changing device and driver behaviour in a drastic
way, it seems prudent to have a way for guest and host to discover that,
even if it's just in case.

Whether it's a feature bit or something else pci-specific,
depends on whether this makes sense for any other transport.

> > - Implement this for all transports
> 
> Is it OK to just keep a fallback to today's implementation for
> transports for which the iommu concept doesn't make sense and that will
> always have an identity mapping?

Is there a reason why iommu does not make any sense for ccw or mmio?


> > 
> > Signed-off-by: Jason Wang 
> > ---
> >  hw/block/virtio-blk.c |  2 +-
> >  hw/char/virtio-serial-bus.c   |  2 +-
> >  hw/scsi/virtio-scsi.c |  2 +-
> >  hw/virtio/virtio-pci.c|  9 +
> >  hw/virtio/virtio.c| 36 +++--
> >  include/hw/virtio/virtio-access.h | 42 
> > +--
> >  include/hw/virtio/virtio-bus.h|  1 +
> >  include/hw/virtio/virtio.h|  2 +-
> >  8 files changed, 67 insertions(+), 29 deletions(-)
> 
> FWIW, this doesn't seem to break for the s390-ccw-virtio machine (only
> snifftested).

Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap"

[Wen Congyang]

On 11/23/2015 05:55 PM, Fam Zheng wrote:
> On Mon, 11/23 17:24, Wen Congyang wrote:
>> On 11/23/2015 05:19 PM, Fam Zheng wrote:
>>> On Mon, 11/23 17:01, Wen Congyang wrote:
 On 11/20/2015 05:59 PM, Fam Zheng wrote:
> "s->bitmap" tracks done sectors, we only check bit states without using 
> any
> iterator which HBitmap is good for. Switch to "Bitmap" which is simpler 
> and
> more memory efficient.
>
> Meanwhile, rename it to done_bitmap, to reflect the intention.
>
> Signed-off-by: Fam Zheng 
> ---
>  block/backup.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 3b39119..d408f98 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -22,6 +22,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/ratelimit.h"
>  #include "sysemu/block-backend.h"
> +#include "qemu/bitmap.h"
>  
>  #define BACKUP_CLUSTER_BITS 16
>  #define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> @@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
>  BlockdevOnError on_target_error;
>  CoRwlock flush_rwlock;
>  uint64_t sectors_read;
> -HBitmap *bitmap;
> +unsigned long *done_bitmap;
>  QLIST_HEAD(, CowRequest) inflight_reqs;
>  } BackupBlockJob;
>  
> @@ -113,7 +114,7 @@ static int coroutine_fn 
> backup_do_cow(BlockDriverState *bs,
>  cow_request_begin(_request, job, start, end);
>  
>  for (; start < end; start++) {
> -if (hbitmap_get(job->bitmap, start)) {
> +if (test_bit(start, job->done_bitmap)) {
>  trace_backup_do_cow_skip(job, start);
>  continue; /* already copied */
>  }
> @@ -164,7 +165,7 @@ static int coroutine_fn 
> backup_do_cow(BlockDriverState *bs,
>  goto out;
>  }
>  
> -hbitmap_set(job->bitmap, start, 1);
> +bitmap_set(job->done_bitmap, start, 1);

 You can use set_bit() here.
>>>
>>> Why? I think bitmap_set is a better match with bitmap_new below.
>>
>> set_bit() is quicker than bitmap_set() if you only set one bit.
>>
> 
> How much quicker is it? This doesn't sound convincing enough for me to lose 
> the
> readability.

I don't test it. It is just a suggestion.

Thanks
Wen Congyang

> 
> Fam
> .
> 

[Qemu-devel] [PATCH v2] numa: Clean up query-memdev error handling

[Markus Armbruster]

qmp_query_memdev() has two error paths:

* When object_get_objects_root() returns null.  It never does, so
  simply drop the useless error handling.

* When query_memdev() fails.  It leaks err then.  But any failure
  there is actually a programming error.  Switch it to _abort,
  and drop the useless error handling.

Messed up in commit 76b5d85 "qmp: add query-memdev".

Signed-off-by: Markus Armbruster 
---
 numa.c | 59 ++-
 1 file changed, 10 insertions(+), 49 deletions(-)

diff --git a/numa.c b/numa.c
index fdfe294..1710946 100644
--- a/numa.c
+++ b/numa.c
@@ -517,7 +517,6 @@ static int query_memdev(Object *obj, void *opaque)
 {
 MemdevList **list = opaque;
 MemdevList *m = NULL;
-Error *err = NULL;
 
 if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
 m = g_malloc0(sizeof(*m));
@@ -525,72 +524,34 @@ static int query_memdev(Object *obj, void *opaque)
 m->value = g_malloc0(sizeof(*m->value));
 
 m->value->size = object_property_get_int(obj, "size",
- );
-if (err) {
-goto error;
-}
-
+ _abort);
 m->value->merge = object_property_get_bool(obj, "merge",
-   );
-if (err) {
-goto error;
-}
-
+   _abort);
 m->value->dump = object_property_get_bool(obj, "dump",
-  );
-if (err) {
-goto error;
-}
-
+  _abort);
 m->value->prealloc = object_property_get_bool(obj,
-  "prealloc", );
-if (err) {
-goto error;
-}
-
+  "prealloc",
+  _abort);
 m->value->policy = object_property_get_enum(obj,
 "policy",
 "HostMemPolicy",
-);
-if (err) {
-goto error;
-}
-
+_abort);
 object_property_get_uint16List(obj, "host-nodes",
-   >value->host_nodes, );
-if (err) {
-goto error;
-}
+   >value->host_nodes,
+   _abort);
 
 m->next = *list;
 *list = m;
 }
 
 return 0;
-error:
-g_free(m->value);
-g_free(m);
-
-return -1;
 }
 
 MemdevList *qmp_query_memdev(Error **errp)
 {
-Object *obj;
+Object *obj = object_get_objects_root();
 MemdevList *list = NULL;
 
-obj = object_get_objects_root();
-if (obj == NULL) {
-return NULL;
-}
-
-if (object_child_foreach(obj, query_memdev, ) != 0) {
-goto error;
-}
-
+object_child_foreach(obj, query_memdev, );
 return list;
-
-error:
-qapi_free_MemdevList(list);
-return NULL;
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH for 2.5 1/1] parallels: dirty BAT properly for continuous allocations

[Denis V. Lunev]

On 11/17/2015 08:02 PM, Denis V. Lunev wrote:

From: Vladimir Sementsov-Ogievskiy 

This patch marks part of the BAT dirty properly. There is a possibility that
multy-block allocation could have one block allocated on one BAT page and
next block on the next page. The code without the patch could not save
updated position to the file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Stefan Hajnoczi 
---
  block/parallels.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index 4f79293..f689fde 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -220,7 +220,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
  s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
  s->data_end += s->tracks;
  bitmap_set(s->bat_dirty_bmap,
-   bat_entry_off(idx) / s->bat_dirty_block, 1);
+   bat_entry_off(idx + i) / s->bat_dirty_block, 1);
  }
  
  return bat2sect(s, idx) + sector_num % s->tracks;

Stefan,

how should we proceed with this? Should I send this as
a pull request or you could take this yourself?

Den

Re: [Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Stefan Hajnoczi]

On Thu, Jul 09, 2015 at 10:56:43AM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Default to IOV_MAX instead of INT_MAX [Peter Lieven]
> 
> This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX elements".
> 
> IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
> refuse to take more iovecs per call.  That's actually an implementation detail
> of raw-posix and we shouldn't spread that across the codebase.
> 
> This series adds BlockLimits.max_iov (similar to BlockLimits.max_transfer_len
> and other fields).  Current IOV_MAX users are converted to blk_get_max_iov().
> 
> By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing 
> but
> follows the BlockLimits field naming convention.  Once IOV_MAX is banished no
> one will be bothered by the reverse naming anymore.
> 
> Suggested-by: Kevin Wolf 
> 
> Stefan Hajnoczi (4):
>   block: add BlockLimits.max_iov field
>   block-backend: add blk_get_max_iov()
>   block: replace IOV_MAX with BlockLimits.max_iov
>   block/mirror: replace IOV_MAX with blk_get_max_iov()
> 
>  block/block-backend.c  | 5 +
>  block/io.c | 8 +++-
>  block/mirror.c | 6 --
>  hw/block/virtio-blk.c  | 2 +-
>  include/block/block_int.h  | 3 +++
>  include/sysemu/block-backend.h | 1 +
>  6 files changed, 21 insertions(+), 4 deletions(-)
> 
> -- 
> 2.4.3
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Stefan Hajnoczi]

On Wed, Nov 18, 2015 at 03:51:29PM +0100, Kevin Wolf wrote:
> Am 09.07.2015 um 11:56 hat Stefan Hajnoczi geschrieben:
> > v2:
> >  * Default to IOV_MAX instead of INT_MAX [Peter Lieven]
> > 
> > This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX 
> > elements".
> > 
> > IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
> > refuse to take more iovecs per call.  That's actually an implementation 
> > detail
> > of raw-posix and we shouldn't spread that across the codebase.
> > 
> > This series adds BlockLimits.max_iov (similar to 
> > BlockLimits.max_transfer_len
> > and other fields).  Current IOV_MAX users are converted to 
> > blk_get_max_iov().
> > 
> > By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing 
> > but
> > follows the BlockLimits field naming convention.  Once IOV_MAX is banished 
> > no
> > one will be bothered by the reverse naming anymore.
> > 
> > Suggested-by: Kevin Wolf 
> 
> I was just looking for patches marked as for-2.5 and found this one...
> Is there a reason it wasn't applied or did we just forget about it?

This series was forgotten.  I have applied it for QEMU 2.6 since it is
not a bug fix.

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v4 4/5] xlnx-zynqmp: Connect the SPI devices

[Alistair Francis]

Connect the Xilinx SPI devices to the ZynqMP model.

Signed-off-by: Alistair Francis 
---
V4:
 - Rename the SPI busses so that they can all be accessed from the SoC
 - Don't set the num-busses property
V3:
 - Expose the SPI Bus as part of the SoC device
V2:
 - Don't connect the SPI flash to the SoC

 hw/arm/xlnx-zynqmp.c | 38 ++
 include/hw/arm/xlnx-zynqmp.h |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 87553bb..6c82f83 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -56,6 +56,14 @@ static const int sdhci_intr[XLNX_ZYNQMP_NUM_SDHCI] = {
 48, 49,
 };
 
+static const uint64_t spi_addr[XLNX_ZYNQMP_NUM_SPIS] = {
+0xFF04, 0xFF05,
+};
+
+static const int spi_intr[XLNX_ZYNQMP_NUM_SPIS] = {
+19, 20,
+};
+
 typedef struct XlnxZynqMPGICRegion {
 int region_index;
 uint32_t address;
@@ -112,6 +120,12 @@ static void xlnx_zynqmp_init(Object *obj)
 qdev_set_parent_bus(DEVICE(>sdhci[i]),
 sysbus_get_default());
 }
+
+for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) {
+object_initialize(>spi[i], sizeof(s->spi[i]),
+  TYPE_XILINX_SPIPS);
+qdev_set_parent_bus(DEVICE(>spi[i]), sysbus_get_default());
+}
 }
 
 static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
@@ -286,6 +300,30 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
**errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(>sdhci[i]), 0,
gic_spi[sdhci_intr[i]]);
 }
+
+for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) {
+BusState *spi_bus;
+char bus_name[6];
+
+object_property_set_bool(OBJECT(>spi[i]), true, "realized", );
+
+sysbus_mmio_map(SYS_BUS_DEVICE(>spi[i]), 0, spi_addr[i]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>spi[i]), 0,
+   gic_spi[spi_intr[i]]);
+
+/* Rename each SPI bus after the SPI device to allow the board
+ * to access all of the busses from the SoC.
+ */
+spi_bus = qdev_get_child_bus(DEVICE(>spi[i]), "spi0");
+snprintf(bus_name, 6, "spi%d", i);
+memcpy((char *) spi_bus->name, bus_name, 6 * sizeof(char));
+
+/* Add the SPI buses to the SoC child bus */
+/* FIXME: This causes the later buses to be duplicated in
+ * the SPI devices printout when running qtree.
+ */
+QLIST_INSERT_HEAD(>child_bus, spi_bus, sibling);
+}
 }
 
 static Property xlnx_zynqmp_props[] = {
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index d116092..f598a43 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -25,6 +25,7 @@
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/sd/sdhci.h"
+#include "hw/ssi/xilinx_spips.h"
 
 #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
 #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
@@ -35,6 +36,7 @@
 #define XLNX_ZYNQMP_NUM_GEMS 4
 #define XLNX_ZYNQMP_NUM_UARTS 2
 #define XLNX_ZYNQMP_NUM_SDHCI 2
+#define XLNX_ZYNQMP_NUM_SPIS 2
 
 #define XLNX_ZYNQMP_NUM_OCM_BANKS 4
 #define XLNX_ZYNQMP_OCM_RAM_0_ADDRESS 0xFFFC
@@ -66,6 +68,7 @@ typedef struct XlnxZynqMPState {
 CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
 SysbusAHCIState sata;
 SDHCIState sdhci[XLNX_ZYNQMP_NUM_SDHCI];
+XilinxSPIPS spi[XLNX_ZYNQMP_NUM_SPIS];
 
 char *boot_cpu;
 ARMCPU *boot_cpu_ptr;
-- 
2.5.0

[Qemu-devel] [PATCH v4 3/5] xilinx_spips: Seperate the state struct into a header

[Alistair Francis]

Seperate out the XilinxSPIPS struct into a seperate header
file.

Signed-off-by: Alistair Francis 
---
V4:
 - Don't split off R_MOD_ID and hardcode R_MAX
V2:
 - Only split out required #defines
 - Prefix XLNX_SPIPS_

 hw/ssi/xilinx_spips.c | 46 +++
 include/hw/ssi/xilinx_spips.h | 72 +++
 2 files changed, 76 insertions(+), 42 deletions(-)
 create mode 100644 include/hw/ssi/xilinx_spips.h

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index e9471ff..2111719 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -29,6 +29,7 @@
 #include "qemu/fifo8.h"
 #include "hw/ssi/ssi.h"
 #include "qemu/bitops.h"
+#include "hw/ssi/xilinx_spips.h"
 
 #ifndef XILINX_SPIPS_ERR_DEBUG
 #define XILINX_SPIPS_ERR_DEBUG 0
@@ -103,8 +104,6 @@
 
 #define R_MOD_ID(0xFC / 4)
 
-#define R_MAX (R_MOD_ID+1)
-
 /* size of TXRX FIFOs */
 #define RXFF_A  32
 #define TXFF_A  32
@@ -135,30 +134,6 @@ typedef enum {
 } FlashCMD;
 
 typedef struct {
-SysBusDevice parent_obj;
-
-MemoryRegion iomem;
-MemoryRegion mmlqspi;
-
-qemu_irq irq;
-int irqline;
-
-uint8_t num_cs;
-uint8_t num_busses;
-
-uint8_t snoop_state;
-qemu_irq *cs_lines;
-SSIBus **spi;
-
-Fifo8 rx_fifo;
-Fifo8 tx_fifo;
-
-uint8_t num_txrx_bytes;
-
-uint32_t regs[R_MAX];
-} XilinxSPIPS;
-
-typedef struct {
 XilinxSPIPS parent_obj;
 
 uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
@@ -174,19 +149,6 @@ typedef struct XilinxSPIPSClass {
 uint32_t tx_fifo_size;
 } XilinxSPIPSClass;
 
-#define TYPE_XILINX_SPIPS "xlnx.ps7-spi"
-#define TYPE_XILINX_QSPIPS "xlnx.ps7-qspi"
-
-#define XILINX_SPIPS(obj) \
- OBJECT_CHECK(XilinxSPIPS, (obj), TYPE_XILINX_SPIPS)
-#define XILINX_SPIPS_CLASS(klass) \
- OBJECT_CLASS_CHECK(XilinxSPIPSClass, (klass), TYPE_XILINX_SPIPS)
-#define XILINX_SPIPS_GET_CLASS(obj) \
- OBJECT_GET_CLASS(XilinxSPIPSClass, (obj), TYPE_XILINX_SPIPS)
-
-#define XILINX_QSPIPS(obj) \
- OBJECT_CHECK(XilinxQSPIPS, (obj), TYPE_XILINX_QSPIPS)
-
 static inline int num_effective_busses(XilinxSPIPS *s)
 {
 return (s->regs[R_LQSPI_CFG] & LQSPI_CFG_SEP_BUS &&
@@ -257,7 +219,7 @@ static void xilinx_spips_reset(DeviceState *d)
 XilinxSPIPS *s = XILINX_SPIPS(d);
 
 int i;
-for (i = 0; i < R_MAX; i++) {
+for (i = 0; i < XLNX_SPIPS_R_MAX; i++) {
 s->regs[i] = 0;
 }
 
@@ -664,7 +626,7 @@ static void xilinx_spips_realize(DeviceState *dev, Error 
**errp)
 }
 
 memory_region_init_io(>iomem, OBJECT(s), xsc->reg_ops, s,
-  "spi", R_MAX*4);
+  "spi", XLNX_SPIPS_R_MAX*4);
 sysbus_init_mmio(sbd, >iomem);
 
 s->irqline = -1;
@@ -708,7 +670,7 @@ static const VMStateDescription vmstate_xilinx_spips = {
 .fields = (VMStateField[]) {
 VMSTATE_FIFO8(tx_fifo, XilinxSPIPS),
 VMSTATE_FIFO8(rx_fifo, XilinxSPIPS),
-VMSTATE_UINT32_ARRAY(regs, XilinxSPIPS, R_MAX),
+VMSTATE_UINT32_ARRAY(regs, XilinxSPIPS, XLNX_SPIPS_R_MAX),
 VMSTATE_UINT8(snoop_state, XilinxSPIPS),
 VMSTATE_END_OF_LIST()
 }
diff --git a/include/hw/ssi/xilinx_spips.h b/include/hw/ssi/xilinx_spips.h
new file mode 100644
index 000..dbb9eef
--- /dev/null
+++ b/include/hw/ssi/xilinx_spips.h
@@ -0,0 +1,72 @@
+/*
+ * Header file for the Xilinx Zynq SPI controller
+ *
+ * Copyright (C) 2015 Xilinx Inc
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef XLNX_SPIPS_H
+#define XLNX_SPIPS_H
+
+#include "hw/ssi/ssi.h"
+#include "qemu/fifo8.h"
+
+typedef struct XilinxSPIPS XilinxSPIPS;
+
+#define XLNX_SPIPS_R_MAX(0x100 / 4)
+
+struct XilinxSPIPS {
+SysBusDevice parent_obj;
+
+MemoryRegion iomem;
+MemoryRegion mmlqspi;
+
+qemu_irq irq;
+int irqline;
+
+uint8_t num_cs;
+uint8_t num_busses;
+
+uint8_t snoop_state;
+  

Re: [Qemu-devel] [RFC] virtio: convert to use DMA api

[Cornelia Huck]

On Mon, 23 Nov 2015 15:41:11 +0800
Jason Wang  wrote:

> Currently, all virtio devices bypass IOMMU completely. This is because
> address_space_memory is assumed and used during DMA emulation. This
> patch converts the virtio core API to use DMA API. This idea is
> 
> - introducing a new transport specific helper to query the dma address
>   space. (only pci version is implemented).
> - query and use this address space during virtio device guest memory
>   accessing
> 
> With this virtiodevices will not bypass IOMMU anymore. Little tested with
> intel_iommu=on with virtio guest DMA series posted in
> https://lkml.org/lkml/2015/10/28/64.
> 
> TODO:
> - Feature bit for this

I'm still not convinced about that feature bit stuff. It just feels
wrong to use a mechanism that conveys negotiable device features to
configure what is basically a platform/hypervisor feature. I'd rather
see this out of the virtio layer and into the pci layer.

> - Implement this for all transports

Is it OK to just keep a fallback to today's implementation for
transports for which the iommu concept doesn't make sense and that will
always have an identity mapping?

> 
> Signed-off-by: Jason Wang 
> ---
>  hw/block/virtio-blk.c |  2 +-
>  hw/char/virtio-serial-bus.c   |  2 +-
>  hw/scsi/virtio-scsi.c |  2 +-
>  hw/virtio/virtio-pci.c|  9 +
>  hw/virtio/virtio.c| 36 +++--
>  include/hw/virtio/virtio-access.h | 42 
> +--
>  include/hw/virtio/virtio-bus.h|  1 +
>  include/hw/virtio/virtio.h|  2 +-
>  8 files changed, 67 insertions(+), 29 deletions(-)

FWIW, this doesn't seem to break for the s390-ccw-virtio machine (only
snifftested).

Re: [Qemu-devel] PING: [PATCH] backends/hostmem: Ignore ENOSYS while setting MPOL_DEFAULT

[Paolo Bonzini]

On 23/11/2015 07:41, Pavel Fedin wrote:
>  Hello! No news for a long time, we are at RC stage. Could we get this in?

Yes, queued for -rc2.

Paolo

> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
>> -Original Message-
>> From: qemu-devel-bounces+p.fedin=samsung@nongnu.org [mailto:qemu-devel-
>> bounces+p.fedin=samsung@nongnu.org] On Behalf Of Eduardo Habkost
>> Sent: Tuesday, October 27, 2015 8:32 PM
>> To: Pavel Fedin
>> Cc: 'Paolo Bonzini'; qemu-devel@nongnu.org
>> Subject: Re: [Qemu-devel] [PATCH] backends/hostmem: Ignore ENOSYS while 
>> setting MPOL_DEFAULT
>>
>> On Tue, Oct 27, 2015 at 03:51:31PM +0300, Pavel Fedin wrote:
>>> Currently hostmem backend fails if CONFIG_NUMA is enabled for the qemu
>>> (default), but NUMA is not supported by the kernel. This makes it
>>> impossible to use ivshmem in such configurations.
>>>
>>> This patch fixes the problem by ignoring ENOSYS error if policy is set to
>>> MPOL_DEFAULT. This way the code behaves in the same way as if CONFIG_NUMA
>>> was not defined. qemu will still fail if the user specifies some other
>>> policy, so that the user knows it.
>>>
>>> Signed-off-by: Pavel Fedin 
>>
>> Reviewed-by: Eduardo Habkost 
>>
>> Thanks. Applied to numa tree, with the following indentation fix:
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 94a4ac0..1b4eb45 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -315,7 +315,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
>> Error **errp)
>>maxnode ? backend->host_nodes : NULL, maxnode + 1, 
>> flags)) {
>>  if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
>>  error_setg_errno(errp, errno,
>> - "cannot bind memory to host NUMA nodes");
>> + "cannot bind memory to host NUMA nodes");
>>  return;
>>  }
>>  }
>>
>>> ---
>>>  backends/hostmem.c | 6 --
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>> index 41ba2af..94a4ac0 100644
>>> --- a/backends/hostmem.c
>>> +++ b/backends/hostmem.c
>>> @@ -313,9 +313,11 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
>>> Error **errp)
>>>  assert(maxnode <= MAX_NODES);
>>>  if (mbind(ptr, sz, backend->policy,
>>>maxnode ? backend->host_nodes : NULL, maxnode + 1, 
>>> flags)) {
>>> -error_setg_errno(errp, errno,
>>> +if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
>>> +error_setg_errno(errp, errno,
>>>   "cannot bind memory to host NUMA nodes");
>>> -return;
>>> +return;
>>> +}
>>>  }
>>>  #endif
>>>  /* Preallocate memory after the NUMA policy has been instantiated.
>>> --
>>> 1.9.5.msysgit.0
>>>
>>>
>>
>> --
>> Eduardo
> 
> 
> 

Re: [Qemu-devel] [PATCH V5 8/8] arm: xlnx-zynqmp: Add xlnx-dp and xlnx-dpdma

[KONRAD Frederic]

Le 20/11/2015 13:21, Alistair Francis a écrit :

On Fri, Oct 16, 2015 at 7:11 PM,   wrote:

From: KONRAD Frederic 

This adds the DP and the DPDMA to the Zynq MP platform.

Signed-off-by: KONRAD Frederic 
Reviewed-by: Peter Crosthwaite 
Tested-By: Hyun Kwon 
---
  hw/arm/xlnx-zynqmp.c | 20 
  include/hw/arm/xlnx-zynqmp.h |  5 +
  2 files changed, 25 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index b36ca3d..dfed5cd 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -32,6 +32,12 @@
  #define SATA_ADDR   0xFD0C
  #define SATA_NUM_PORTS  2

+#define DP_ADDR 0xfd4a
+#define DP_IRQ  113
+
+#define DPDMA_ADDR  0xfd4c
+#define DPDMA_IRQ   116
+
  static const uint64_t gem_addr[XLNX_ZYNQMP_NUM_GEMS] = {
  0xFF0B, 0xFF0C, 0xFF0D, 0xFF0E,
  };
@@ -97,6 +103,11 @@ static void xlnx_zynqmp_init(Object *obj)

  object_initialize(>sata, sizeof(s->sata), TYPE_SYSBUS_AHCI);
  qdev_set_parent_bus(DEVICE(>sata), sysbus_get_default());
+
+object_initialize(>dp, sizeof(s->dp), TYPE_XLNX_DP);
+qdev_set_parent_bus(DEVICE(>dp), sysbus_get_default());

New line


+object_initialize(>dpdma, sizeof(s->dpdma), TYPE_XLNX_DPDMA);
+qdev_set_parent_bus(DEVICE(>dpdma), sysbus_get_default());
  }

  static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
@@ -258,6 +269,15 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
**errp)

  sysbus_mmio_map(SYS_BUS_DEVICE(>sata), 0, SATA_ADDR);
  sysbus_connect_irq(SYS_BUS_DEVICE(>sata), 0, gic_spi[SATA_INTR]);
+
+sysbus_mmio_map(SYS_BUS_DEVICE(>dp), 0, DP_ADDR);
+sysbus_connect_irq(SYS_BUS_DEVICE(>dp), 0, gic_spi[DP_IRQ]);

New line


+sysbus_mmio_map(SYS_BUS_DEVICE(>dpdma), 0, DPDMA_ADDR);
+sysbus_connect_irq(SYS_BUS_DEVICE(>dpdma), 0, gic_spi[DPDMA_IRQ]);
+object_property_set_bool(OBJECT(>dp), true, "realized", );
+object_property_set_bool(OBJECT(>dpdma), true, "realized", );

Can you add something to check these errors?


Ok.
BTW I'll move the I2C and AUX device out of xlnx-dp and put that here.
Is that ok with you?

Thanks,
Fred


Thanks,

Alistair


+object_property_set_link(OBJECT(>dp), OBJECT(>dpdma), "dpdma",
+ _abort);
  }

  static Property xlnx_zynqmp_props[] = {
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 4005a99..5a4d6cc 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -24,6 +24,8 @@
  #include "hw/char/cadence_uart.h"
  #include "hw/ide/pci.h"
  #include "hw/ide/ahci.h"
+#include "hw/dma/xlnx_dpdma.h"
+#include "hw/display/xlnx_dp.h"

  #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
  #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
@@ -66,6 +68,9 @@ typedef struct XlnxZynqMPState {

  char *boot_cpu;
  ARMCPU *boot_cpu_ptr;
+
+XlnxDPState dp;
+XlnxDPDMAState dpdma;
  }  XlnxZynqMPState;

  #define XLNX_ZYNQMP_H
--
1.9.0

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> - Original Message -
>>
>> >> qemu-doc documents role only with chardev.  The code doesn't care.
>> >
>> > yeah, role is only really useful with a server. Another missing warning.
>> 
>> I think it makes sense only when we can migrate the shared memory
>> contents out-of-band.  Vaguely similar to block devices: either you
>> migrate them along (block migration), or you have to ensure they have
>> identical contents on the target in some other way.
>> 
>> Is the latter possible only with a server?
>
> The way ivshmem role works is:
> - master: will handle migration
> - peer: can't migrate
>
> It's not out-of-band, qemu handles the shared memory migration, even if
> the memory is owned by the server. In practice, it means you can only
> migrate one VM, or you migrate them all but you will migrate the shared
> memory N times and will have to synchronize in your guest app. I suppose 
> ivshmem
> "role" was designed to only migrate the master. Ability to migrate a pool of
> peer would be a significant new feature. I am not aware of such request.

I see.  But how is this supposed to work?

Before migration: one master and N peers connected to the server on host
A, N>=0.

After migration: one master and N' of the N peers connected to the
server on host B, N>=N'>=0, and the remaining N-N' peers still on host A
with their ivshmem device unplugged.

How would I do this even for N'==0?  I can't see how I'm supposted to
connect the migrated shared memory to a server on host B.

>> >> This is a byzantine mess even for QEMU standards.
>> >> 
>> >> Questions:
>> >> 
>> >> * Is it okay to have an unmapped BAR?
>> >
>> > I can't say much, though I remember I did some tests and it was ok.
>> 
>> Did you try chardev=...,size=S, where S is larger than what the server
>> provides?
>
> It will fall in check_shm_size().

Yes.  Called from ivshmem_read().  ivshmem_read() will then complain to
stderr, close the file descriptor we got from the server and leave the
BAR unmapped.  My question is how guests deal with that state.  Could be
anything from "detect the device is broken and fence it" to "kernel
panic".

Whatever it is, it could easily also happen if the guest wins the race
with the server and tries to use the device before it successfully got
its shared memory from the server.

>> A guest that fails there probably works for sufficiently small S only
>> when it loses the race with the server.
>> 
>> But my question is actually whether it's okay for a PCI device to
>> advertize a BAR, and then not map anything there.
>> 
>> Should realize wait for the server providing the shared memory?
>> 
>> >> * We actually have two distinct device kinds:
>> >> 
>> >>   - The basic device: shm with parameter size, or memdev
>> >> 
>> >>   - The doorbell-capable device: chardev with parameters size, vectors,
>> >> ioeventfd=on, msi
>> >> 
>> >>   Common parameters: use64, role, although the latter is only documented
>> >>   for the doorbell-capable device.
>> >> 
>
>> 
>> >> * Are we *sure* this is ready to become ABI?  I doubt it's ABI yet,
>> >>   because before Marc-André's massive rework, it was even worse (*much*
>> >>   worse), to a degree that makes me doubt anybody could use it
>> >>   seriously.
>> >
>> > It's supposed to be the same ABI as before, with the memdev addition.
>> 
>> Well, it's *crap*.  We shouldn't extend and known crap so it can get
>> used more widely, we should deprecate it in favour of something less
>> crappy.
>
> I think you overstate this, if it would be so bad I don't think anyone could 
> use
> it and it wouldn't probably be in qemu. We are mainly talking misconfiguration
> and missing features (that people may not actually need). I think
> before we split things, we can address your comments with more
> options checking and documentation.

We have at least two problems:

1. Unless the guest can reliably detect the doorbell feature, the
   doorbell feature is *broken*.

   As far as I can tell, a device with a doorbell behaves exactly like
   one without a doorbell until it got its shared memory from the
   server.  If that's correct, then doorbell detection is inherently
   racy.

   The only way to fix this in documentation is "broken, do not use".

   The maximally compatible way to fix this in code is to ensure the
   guest can't read register IVPosition before we got the shared memory
   from the server.  We can make realize wait, or the read.  The latter
   is probably an even worse idea.

   An easier way to fix it in code is splitting up the device, so guests
   can simply check the PCI device ID to figure out whether they have
   one with a doorbell.

   An even easier way is dropping the doorbell feature outright.

2. The UI is crap.

   We can fix this by rejecting nonsensical option combinations.
   However, the result will be more complex than splitting the device in
   two so that nonsensical options combinations are simply 

[Qemu-devel] [PATCH v2 1/1] parallels: add format spec

[Denis V. Lunev]

From: Vladimir Sementsov-Ogievskiy 

This specifies Parallels image format as implemented in Parallels Cloud
Server 6.10

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Denis V. Lunev 
CC: Eric Blake 
CC: John Snow 
CC: Stefan Hajnoczi 
---
v2: add license info
switch to offsets from types in field descriptions

MAINTAINERS  |   1 +
 docs/specs/parallels.txt | 227 +++
 2 files changed, 228 insertions(+)
 create mode 100644 docs/specs/parallels.txt

diff --git a/MAINTAINERS b/MAINTAINERS
index 9e1fa72..23441e1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1471,6 +1471,7 @@ M: Denis V. Lunev 
 L: qemu-bl...@nongnu.org
 S: Supported
 F: block/parallels.c
+F: docs/specs/parallels.txt
 
 qed
 M: Stefan Hajnoczi 
diff --git a/docs/specs/parallels.txt b/docs/specs/parallels.txt
new file mode 100644
index 000..e58fb7c
--- /dev/null
+++ b/docs/specs/parallels.txt
@@ -0,0 +1,227 @@
+= License =
+
+Copyright (c) 2015 Denis Lunev
+Copyright (c) 2015 Vladimir Sementsov-Ogievskiy
+
+This work is licensed under the terms of the GNU GPL, version 2 or later.
+See the COPYING file in the top-level directory.
+
+= Parallels Expandable Image File Format =
+
+A Parallels expandable image file consists of three consecutive parts:
+* header
+* BAT
+* data area
+
+All numbers in a Parallels expandable image are stored in little-endian byte
+order.
+
+
+== Definitions ==
+
+SectorA 512-byte data chunk.
+
+Cluster   A data chunk of the size specified in the image header.
+  Currently, the default size is 1MiB (2048 sectors). In previous
+  versions, cluster sizes of 63 sectors, 256 and 252 kilobytes were
+  used.
+
+BAT   Block Allocation Table, an entity that contains information for
+  guest-to-host I/O data address translation.
+
+
+== Header ==
+
+The header is placed at the start of an image and contains the following
+fields:
+
+Bytes:
+   0 - 15:magic
+  Must contain "WithoutFreeSpace" or "WithouFreSpacExt".
+
+  16 - 19:version
+  Must be 2.
+
+  20 - 23:heads
+  Disk geometry parameter for guest.
+
+  24 - 27:cylinders
+  Disk geometry parameter for guest.
+
+  28 - 31:tracks
+  Cluster size, in sectors.
+
+  32 - 35:nb_bat_entries
+  Disk size, in clusters (BAT size).
+
+  36 - 43:nb_sectors
+  Disk size, in sectors.
+
+  For "WithoutFreeSpace" images:
+  Only the lowest 4 bytes are used. The highest 4 bytes must be
+  cleared in this case.
+
+  For "WithouFreSpacExt" images, there are no such
+  restrictions.
+
+  44 - 47:in_use
+  Set to 0x746F6E59 when the image is opened by software in R/W
+  mode; set to 0x312e3276 when the image is closed.
+
+  A zero in this field means that the image was opened by an old
+  version of the software that doesn't support Format Extension
+  (see below).
+
+  Other values are not allowed.
+
+  48 - 51:data_off
+  An offset, in sectors, from the start of the file to the start of
+  the data area.
+
+  For "WithoutFreeSpace" images:
+  - If data_off is zero, the offset is calculated as the end of BAT
+table plus some padding to ensure sector size alignment.
+  - If data_off is non-zero, the offset should be aligned to sector
+size. However it is recommended to align it to cluster size for
+newly created images.
+
+  For "WithouFreSpacExt" images:
+  data_off must be non-zero and aligned to cluster size.
+
+  52 - 55:flags
+  Miscellaneous flags.
+
+  Bit 0: Empty Image bit. If set, the image should be
+ considered clear.
+
+  Bits 2-31: Unused.
+
+  56 - 63:ext_off
+  Format Extension offset, an offset, in sectors, from the start of
+  the file to the start of the Format Extension Cluster.
+
+  ext_off must meet the same requirements as cluster offsets
+  defined by BAT entries (see below).
+
+
+== BAT ==
+
+BAT is placed immediately after the image header. In the file, BAT is a
+contiguous array of 32-bit unsigned little-endian integers with
+(bat_entries * 4) bytes size.
+
+Each BAT entry contains an offset from the start of the file to the
+corresponding cluster. The offset set in clusters for "WithouFreSpacExt" images
+and in sectors for "WithoutFreeSpace" images.
+
+If a BAT entry is zero, the corresponding cluster is not allocated and should
+be considered as filled with 

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Marc-André Lureau]

Hi

- Original Message -
> > "role" was designed to only migrate the master. Ability to migrate a pool
> > of
> > peer would be a significant new feature. I am not aware of such request.
> 
> I see.  But how is this supposed to work?
> 
> Before migration: one master and N peers connected to the server on host
> A, N>=0.
> 
> After migration: one master and N' of the N peers connected to the
> server on host B, N>=N'>=0, and the remaining N-N' peers still on host A
> with their ivshmem device unplugged.
> 
> How would I do this even for N'==0?  I can't see how I'm supposted to
> connect the migrated shared memory to a server on host B.

I am not sure I understand you.

You can't migrate the peers.

As I said, "ability to migrate a pool of peer would be a significant new 
feature".

> >> Did you try chardev=...,size=S, where S is larger than what the server
> >> provides?
> >
> > It will fall in check_shm_size().
> 
> Yes.  Called from ivshmem_read().  ivshmem_read() will then complain to
> stderr, close the file descriptor we got from the server and leave the
> BAR unmapped.  My question is how guests deal with that state.  Could be
> anything from "detect the device is broken and fence it" to "kernel
> panic".
> Whatever it is, it could easily also happen if the guest wins the race
> with the server and tries to use the device before it successfully got
> its shared memory from the server.

It's nothing bad from what I remember on qemu side. On guest side, it
depends how your driver/user is implemented I suppose. To me, it's not
a normal case, and the error should be enough to diagnose it.

> 1. Unless the guest can reliably detect the doorbell feature, the
>doorbell feature is *broken*.
> 
>As far as I can tell, a device with a doorbell behaves exactly like
>one without a doorbell until it got its shared memory from the
>server.  If that's correct, then doorbell detection is inherently
>racy.

There are many ways you can do synchronization.
In test_ivshmem_server(), I trivially wait for the membar with the
required signature to be mapped. Verify that peers have different ids,
and then start using the doorbell. That seems good enough.

>The only way to fix this in documentation is "broken, do not use".

It works fine in the tests. Feel free to point out races or other issues.

>The maximally compatible way to fix this in code is to ensure the
>guest can't read register IVPosition before we got the shared memory
>from the server.  We can make realize wait, or the read.  The latter
>is probably an even worse idea.
> 
>An easier way to fix it in code is splitting up the device, so guests
>can simply check the PCI device ID to figure out whether they have
>one with a doorbell.
> 
>An even easier way is dropping the doorbell feature outright.
> 
> 2. The UI is crap.
> 
>We can fix this by rejecting nonsensical option combinations.

Yes, I think it's the simplest way for now. I dislike having to break stuff 
when you can overcome it with a few more checks.

>However, the result will be more complex than splitting the device in
>two so that nonsensical options combinations are simply impossible.

I disagree, adding more checks will add a few dozen lines with minimal impact. 
Splitting things will break stuff and require significant effort to share 
correctly what can be shared etc.

>If we need to split it anyway to fix the doorbell, we can clean up
>the UI at next to no cost.

I don't think the doorbell is broken.

[Qemu-devel] [PATCH 1/2] dump-guest-memory: add "detach" flag for QMP/HMP interfaces

[Peter Xu]

This patch only add the interfaces, but not implementing them.

Signed-off-by: Peter Xu 
---
 dump.c   | 3 ++-
 hmp-commands.hx  | 5 +++--
 hmp.c| 3 ++-
 qapi-schema.json | 3 ++-
 qmp-commands.hx  | 4 ++--
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/dump.c b/dump.c
index 78b7d84..3ec3423 100644
--- a/dump.c
+++ b/dump.c
@@ -1598,7 +1598,8 @@ cleanup:
 void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
int64_t begin, bool has_length,
int64_t length, bool has_format,
-   DumpGuestMemoryFormat format, Error **errp)
+   DumpGuestMemoryFormat format, bool detach,
+   Error **errp)
 {
 const char *p;
 int fd = -1;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..8e27671 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1056,10 +1056,11 @@ ETEXI
 
 {
 .name   = "dump-guest-memory",
-.args_type  = 
"paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
-.params = "[-p] [-z|-l|-s] filename [begin length]",
+.args_type  = 
"paging:-p,detach:-d,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
+.params = "[-p] [-d] [-z|-l|-s] filename [begin length]",
 .help   = "dump guest memory into file 'filename'.\n\t\t\t"
   "-p: do paging to get guest's memory mapping.\n\t\t\t"
+  "-d: return immediately (not wait for 
completion).\n\t\t\t"
   "-z: dump in kdump-compressed format, with zlib 
compression.\n\t\t\t"
   "-l: dump in kdump-compressed format, with lzo 
compression.\n\t\t\t"
   "-s: dump in kdump-compressed format, with snappy 
compression.\n\t\t\t"
diff --git a/hmp.c b/hmp.c
index 2140605..4c5480d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1580,6 +1580,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
*qdict)
 {
 Error *err = NULL;
 bool paging = qdict_get_try_bool(qdict, "paging", false);
+bool detach = qdict_get_try_bool(qdict, "detach", false);
 bool zlib = qdict_get_try_bool(qdict, "zlib", false);
 bool lzo = qdict_get_try_bool(qdict, "lzo", false);
 bool snappy = qdict_get_try_bool(qdict, "snappy", false);
@@ -1619,7 +1620,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
*qdict)
 prot = g_strconcat("file:", file, NULL);
 
 qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
-  true, dump_format, );
+  true, dump_format, detach, );
 hmp_handle_error(mon, );
 g_free(prot);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..1211082 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2132,7 +2132,8 @@
 ##
 { 'command': 'dump-guest-memory',
   'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
-'*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
+'*length': 'int', '*format': 'DumpGuestMemoryFormat',
+'detach': 'bool'} }
 
 ##
 # @DumpGuestMemoryCapability:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..86864f6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -840,8 +840,8 @@ EQMP
 
 {
 .name   = "dump-guest-memory",
-.args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
-.params = "-p protocol [begin] [length] [format]",
+.args_type  = "paging:b,detach:d,protocol:s,begin:i?,end:i?,format:s?",
+.params = "-p protocol [-d] [begin] [length] [format]",
 .help   = "dump guest memory to file",
 .mhandler.cmd_new = qmp_marshal_dump_guest_memory,
 },
-- 
2.4.3

[Qemu-devel] [PATCH REPOST 2/2] dump-guest-memory: add basic "detach" support.

[Peter Xu]

This will allow the user specify "-d" (just like command
"migrate") when using "dump-guest-memory" command. When
specified, one background thread is created to do the dump work.
One flag is added to show whether there is a background dump
work in progress.

Signed-off-by: Peter Xu 
---
 dump.c| 59 ++-
 include/sysemu/dump.h |  4 
 qmp.c |  9 
 vl.c  |  3 +++
 4 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/dump.c b/dump.c
index 3ec3423..c2e14cd 100644
--- a/dump.c
+++ b/dump.c
@@ -1442,6 +1442,9 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
 Error *err = NULL;
 int ret;
 
+s->has_format = has_format;
+s->format = format;
+
 /* kdump-compressed is conflict with paging and filter */
 if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
 assert(!paging && !has_filter);
@@ -1595,6 +1598,45 @@ cleanup:
 dump_cleanup(s);
 }
 
+/* whether there is a dump in progress */
+extern bool dump_in_progress_p;
+
+static bool dump_ownership_set(bool value)
+{
+return atomic_xchg(_in_progress_p, value);
+}
+
+/* return true when owner taken, false otherwise */
+static bool dump_ownership_take(void)
+{
+bool ret = dump_ownership_set(1);
+return ret == 0;
+}
+
+/* we should only call this after all dump work finished */
+static void dump_ownership_release(void)
+{
+dump_ownership_set(0);
+}
+
+static void dump_process(DumpState *s, Error **errp)
+{
+if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
+create_kdump_vmcore(s, errp);
+} else {
+create_vmcore(s, errp);
+}
+dump_ownership_release();
+}
+
+static void *dump_thread(void *data)
+{
+DumpState *s = (DumpState *)data;
+dump_process(s, NULL);
+g_free(s);
+return NULL;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
int64_t begin, bool has_length,
int64_t length, bool has_format,
@@ -1662,6 +1704,12 @@ void qmp_dump_guest_memory(bool paging, const char 
*file, bool has_begin,
 return;
 }
 
+/* we could only allow one dump at a time. */
+if (!dump_ownership_take()) {
+error_setg(errp, "another dump is in progress.");
+return;
+}
+
 s = g_malloc0(sizeof(DumpState));
 
 dump_init(s, fd, has_format, format, paging, has_begin,
@@ -1672,13 +1720,14 @@ void qmp_dump_guest_memory(bool paging, const char 
*file, bool has_begin,
 return;
 }
 
-if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-create_kdump_vmcore(s, errp);
+if (!detach) {
+dump_process(s, errp);
+g_free(s);
 } else {
-create_vmcore(s, errp);
+qemu_thread_create(>dump_thread, "dump_thread", dump_thread,
+   s, QEMU_THREAD_DETACHED);
+/* DumpState is freed in dump thread */
 }
-
-g_free(s);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7e4ec5c..2aeffc8 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -183,6 +183,10 @@ typedef struct DumpState {
 off_t offset_page;  /* offset of page part in vmcore */
 size_t num_dumpable;/* number of page that can be dumped */
 uint32_t flag_compress; /* indicate the compression format */
+
+QemuThread dump_thread; /* only used when do async dump */
+bool has_format;/* whether format is provided */
+DumpGuestMemoryFormat format; /* valid only if has_format == true */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/qmp.c b/qmp.c
index 0a1fa19..ea57b57 100644
--- a/qmp.c
+++ b/qmp.c
@@ -168,12 +168,21 @@ SpiceInfo *qmp_query_spice(Error **errp)
 };
 #endif
 
+extern bool dump_in_progress_p;
+
 void qmp_cont(Error **errp)
 {
 Error *local_err = NULL;
 BlockBackend *blk;
 BlockDriverState *bs;
 
+/* if there is a dump in background, we should wait until the dump
+ * finished */
+if (dump_in_progress_p) {
+error_setg(errp, "there is a dump in process, please wait.");
+return;
+}
+
 if (runstate_needs_reset()) {
 error_setg(errp, "Resetting the Virtual Machine is required");
 return;
diff --git a/vl.c b/vl.c
index 525929b..ef7a936 100644
--- a/vl.c
+++ b/vl.c
@@ -204,6 +204,9 @@ bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 
+/* whether dump is in progress */
+bool dump_in_progress_p = false;
+
 static int has_defaults = 1;
 static int default_serial = 1;
 static int default_parallel = 1;
-- 
2.4.3

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> - Original Message -
>> > "role" was designed to only migrate the master. Ability to migrate a pool
>> > of
>> > peer would be a significant new feature. I am not aware of such request.
>> 
>> I see.  But how is this supposed to work?
>> 
>> Before migration: one master and N peers connected to the server on host
>> A, N>=0.
>> 
>> After migration: one master and N' of the N peers connected to the
>> server on host B, N>=N'>=0, and the remaining N-N' peers still on host A
>> with their ivshmem device unplugged.
>> 
>> How would I do this even for N'==0?  I can't see how I'm supposted to
>> connect the migrated shared memory to a server on host B.
>
> I am not sure I understand you.
>
> You can't migrate the peers.

Then explain the case N'=0 to me: how can you migrate the master so that
it's connected to a server afterwards?

> As I said, "ability to migrate a pool of peer would be a significant
> new feature".
>
>> >> Did you try chardev=...,size=S, where S is larger than what the server
>> >> provides?
>> >
>> > It will fall in check_shm_size().
>> 
>> Yes.  Called from ivshmem_read().  ivshmem_read() will then complain to
>> stderr, close the file descriptor we got from the server and leave the
>> BAR unmapped.  My question is how guests deal with that state.  Could be
>> anything from "detect the device is broken and fence it" to "kernel
>> panic".
>> Whatever it is, it could easily also happen if the guest wins the race
>> with the server and tries to use the device before it successfully got
>> its shared memory from the server.
>
> It's nothing bad from what I remember on qemu side. On guest side, it
> depends how your driver/user is implemented I suppose. To me, it's not
> a normal case, and the error should be enough to diagnose it.
>
>> 1. Unless the guest can reliably detect the doorbell feature, the
>>doorbell feature is *broken*.
>> 
>>As far as I can tell, a device with a doorbell behaves exactly like
>>one without a doorbell until it got its shared memory from the
>>server.  If that's correct, then doorbell detection is inherently
>>racy.
>
> There are many ways you can do synchronization.
> In test_ivshmem_server(), I trivially wait for the membar with the
> required signature to be mapped. Verify that peers have different ids,
> and then start using the doorbell. That seems good enough.
>
>>The only way to fix this in documentation is "broken, do not use".
>
> It works fine in the tests. Feel free to point out races or other issues.

I think I did: doorbell detection is inherently racy.

If you think it isn't, please refute my reasoning.

>>The maximally compatible way to fix this in code is to ensure the
>>guest can't read register IVPosition before we got the shared memory
>>from the server.  We can make realize wait, or the read.  The latter
>>is probably an even worse idea.
>> 
>>An easier way to fix it in code is splitting up the device, so guests
>>can simply check the PCI device ID to figure out whether they have
>>one with a doorbell.
>> 
>>An even easier way is dropping the doorbell feature outright.
>> 
>> 2. The UI is crap.
>> 
>>We can fix this by rejecting nonsensical option combinations.
>
> Yes, I think it's the simplest way for now. I dislike having to break
> stuff when you can overcome it with a few more checks.
>
>>However, the result will be more complex than splitting the device in
>>two so that nonsensical options combinations are simply impossible.
>
> I disagree, adding more checks will add a few dozen lines with minimal
> impact. Splitting things will break stuff and require significant
> effort to share correctly what can be shared etc.
>
>>If we need to split it anyway to fix the doorbell, we can clean up
>>the UI at next to no cost.
>
> I don't think the doorbell is broken.

If it's not broken, please explain to me how the guest should find out
whether its ivshmem device sports a doorbell.

Re: [Qemu-devel] [PATCH V5 7/8] introduce xlnx-dp

[KONRAD Frederic]

Le 20/11/2015 11:06, Alistair Francis a écrit :

On Fri, Oct 16, 2015 at 7:11 PM,   wrote:

From: KONRAD Frederic 

This is the implementation of the DisplayPort.
It has an aux-bus to access dpcd and edid.

Graphic plane is connected to the channel 3.
Video plane is connected to the channel 0.
Audio stream are connected to the channels 4 and 5.

This patch doesn't pass checkpatch (I didn't test any others).
Can you run the series through checkpatch?
Yes but I might have introduced some coding style issues during the last 
changes.

Sorry for that will fix it!

Thanks,
Fred


Also a super small nit pick, some of your line splitting doesn't line up,
can you make sure that the function arguments and if statement conditions
line up with the previous line.


Signed-off-by: KONRAD Frederic 
Tested-By: Hyun Kwon 
---
  hw/display/Makefile.objs |1 +
  hw/display/xlnx_dp.c | 1370 ++
  include/hw/display/xlnx_dp.h |  110 
  3 files changed, 1481 insertions(+)
  create mode 100644 hw/display/xlnx_dp.c
  create mode 100644 include/hw/display/xlnx_dp.h

diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 250a43f..3625ab2 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -43,3 +43,4 @@ virtio-gpu.o-libs += $(VIRGL_LIBS)
  virtio-gpu-3d.o-cflags := $(VIRGL_CFLAGS)
  virtio-gpu-3d.o-libs += $(VIRGL_LIBS)
  obj-$(CONFIG_DPCD) += dpcd.o
+obj-$(CONFIG_XLNX_ZYNQMP) += xlnx_dp.o
diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
new file mode 100644
index 000..e91221c
--- /dev/null
+++ b/hw/display/xlnx_dp.c
@@ -0,0 +1,1370 @@
+/*
+ * xlnx_dp.c
+ *
+ *  Copyright (C) 2015 : GreenSocs Ltd
+ *  http://www.greensocs.com/ , email: i...@greensocs.com
+ *
+ *  Developed by :
+ *  Frederic Konrad   
+ *
+ * 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 .
+ *
+ */
+
+#include "hw/display/xlnx_dp.h"
+
+#ifndef DEBUG_DP
+#define DEBUG_DP 0
+#endif
+
+#define DPRINTF(fmt, ...) do { 
\
+if (DEBUG_DP) {
\
+qemu_log("xlnx_dp: " fmt , ## __VA_ARGS__);
\
+}  
\
+} while (0);
+
+/*
+ * Register offset for DP.
+ */
+#define DP_LINK_BW_SET  (0x >> 2)
+#define DP_LANE_COUNT_SET   (0x0004 >> 2)
+#define DP_ENHANCED_FRAME_EN(0x0008 >> 2)
+#define DP_TRAINING_PATTERN_SET (0x000C >> 2)
+#define DP_LINK_QUAL_PATTERN_SET(0x0010 >> 2)
+#define DP_SCRAMBLING_DISABLE   (0x0014 >> 2)
+#define DP_DOWNSPREAD_CTRL  (0x0018 >> 2)
+#define DP_SOFTWARE_RESET   (0x001C >> 2)
+#define DP_TRANSMITTER_ENABLE   (0x0080 >> 2)
+#define DP_MAIN_STREAM_ENABLE   (0x0084 >> 2)
+#define DP_FORCE_SCRAMBLER_RESET(0x00C0 >> 2)
+#define DP_VERSION_REGISTER (0x00F8 >> 2)
+#define DP_CORE_ID  (0x00FC >> 2)
+
+#define DP_AUX_COMMAND_REGISTER (0x0100 >> 2)
+#define AUX_ADDR_ONLY_MASK  (0x1000)
+#define AUX_COMMAND_MASK(0x0F00)
+#define AUX_COMMAND_SHIFT   (8)
+#define AUX_COMMAND_NBYTES  (0x000F)
+
+#define DP_AUX_WRITE_FIFO   (0x0104 >> 2)
+#define DP_AUX_ADDRESS  (0x0108 >> 2)
+#define DP_AUX_CLOCK_DIVIDER(0x010C >> 2)
+#define DP_TX_USER_FIFO_OVERFLOW(0x0110 >> 2)
+#define DP_INTERRUPT_SIGNAL_STATE   (0x0130 >> 2)
+#define DP_AUX_REPLY_DATA   (0x0134 >> 2)
+#define DP_AUX_REPLY_CODE   (0x0138 >> 2)
+#define DP_AUX_REPLY_COUNT  (0x013C >> 2)
+#define DP_REPLY_DATA_COUNT (0x0148 >> 2)
+#define DP_REPLY_STATUS (0x014C >> 2)
+#define DP_HPD_DURATION (0x0150 >> 2)
+#define DP_MAIN_STREAM_HTOTAL   (0x0180 >> 2)
+#define DP_MAIN_STREAM_VTOTAL   (0x0184 >> 2)
+#define DP_MAIN_STREAM_POLARITY (0x0188 >> 2)
+#define DP_MAIN_STREAM_HSWIDTH 

Re: [Qemu-devel] [PATCH] target-i386: Do not set MCG_SER_P by default

[Eduardo Habkost]

On Sat, Nov 21, 2015 at 02:09:25AM +0100, Borislav Petkov wrote:
> On Sat, Nov 21, 2015 at 12:11:35AM +0100, Andreas Färber wrote:
> > Hi,
> > 
> > CC'ing qemu-devel.
> 
> Ah, thanks.
> 
> > Am 21.11.2015 um 00:01 schrieb Borislav Petkov:
> > > From: Borislav Petkov 
> > > 
> > > Software Error Recovery, i.e. SER, is purely an Intel feature and it
> > > shouldn't be set by default. Enable it only on Intel.

What happens when SER is enabled on an AMD CPU? If it really
should't be enabled, why is KVM returning it on
KVM_X86_GET_MCE_CAP_SUPPORTED?

> > 
> > Is this new in 2.5? Otherwise we would probably need compatibility code
> > in pc*.[ch] for incoming live migration from older versions.
> 
> It looks it is really old, AFAIK from 2010:
> 
> c0532a76b407 ("MCE: Relay UCR MCE to guest")
> 
> You'd need to be more verbose about pc*.[ch]. An example perhaps...?

If you change something that's guest-visible and not part of the
migration stream, you need to keep the old behavior on older
machine-types (e.g. pc-i440fx-2.4), or the CPU will change under
the guest's feet when migrating to another host.

For examples, see the recent commits to include/hw/i386/pc.h.
They are all about keeping compatibility when CPUID bits are
changed:

33b5e8c0 target-i386: Disable rdtscp on Opteron_G* CPU models
6aa91e4a target-i386: Remove POPCNT from qemu64 and qemu32 CPU models
71195672 target-i386: Remove ABM from qemu64 CPU model
0909ad24 target-i386: Remove SSE4a from qemu64 CPU model

In the case of this code, it looks like it's already broken
because the resulting mcg_cap depends on host kernel capabilities
(the ones reported by kvm_get_mce_cap_supported()), and the data
initialized by target-i386/cpu.c:mce_init() is silently
overwritten by kvm_arch_init_vcpu(). So we would need to fix that
before implementing a proper compatibility mechanism for
mcg_cap.

-- 
Eduardo

[Qemu-devel] [PATCH] PCI Trivial: remove superfluous code

[Cao jin]

remove superfluous code in do_pci_register_device(). See its caller:
pci_qdev_realize()

Signed-off-by: Cao jin 
---
 hw/pci/pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 168b9cc..4d16da0 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -878,7 +878,6 @@ static PCIDevice *do_pci_register_device(PCIDevice 
*pci_dev, PCIBus *bus,
return NULL;
 }
 
-pci_dev->devfn = devfn;
 dma_as = pci_device_iommu_address_space(pci_dev);
 
 memory_region_init_alias(_dev->bus_master_enable_region,
-- 
2.1.0


-- 
This message has been scanned for viruses and
dangerous content by FCNIC, and is
believed to be clean.

Re: [Qemu-devel] [PATCH WIP 01/30] crypto: add QCryptoSecret object class for password/key handling

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> On Fri, Nov 20, 2015 at 03:09:25PM -0700, Eric Blake wrote:
>> On 11/20/2015 11:04 AM, Daniel P. Berrange wrote:
>> > +
>> > +static const char *base64_valid_chars =
>> > +"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=";
>> > +
>> > +static int
>> > +qcrypto_secret_validate_base64(const uint8_t *input,
>> > +   size_t inputlen,
>> > +   Error **errp)
>> 
>> Don't we already have base64 utility methods available?
>
> We normally use glib,  g_base64_encode/decode. Unfortunately the
> decode method doesn't provide any usefull error reporting facility.
> It just silently skips any characters that are outside the valid
> set.  So the only way I could get any kind of sensible error report
> was to do this validation myself against the set of permitted base64
> characters.

Yes.  Same problem elsewhere, e.g. ringbuf-write.  qapi-schema.json:

#  - base64: data must be base64 encoded text.  Its binary
#decoding gets written.
#Bug: invalid base64 is currently not rejected.
#Whitespace *is* invalid.

This suggests that we shouldn't bury this in crypto/, but instead add it
to util/.

A replacement for g_base64_decode() could be easier to use than a
checker function to use in addition to g_base64_decode(),

[...]

Re: [Qemu-devel] [PULL for-2.5] tcg: Fix highwater check

[Richard Henderson]

On 11/23/2015 02:16 PM, Stefan Weil wrote:

Am 23.11.2015 um 13:45 schrieb Richard Henderson:

From: John Clarke 

A simple typo in the variable to use when comparing vs the highwater mark.
Reports are that qemu can in fact segfault occasionally due to this mistake.

Signed-off-by: John Clarke 
Signed-off-by: Richard Henderson 
---
  tcg/tcg.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 682af8a..b20ed19 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2443,7 +2443,7 @@ int tcg_gen_code(TCGContext *s, tcg_insn_unit 
*gen_code_buf)
 one operation beginning below the high water mark cannot overrun
 the buffer completely.  Thus we can test for overflow after
 generating code without having to check during generation.  */
-if (unlikely(s->code_gen_ptr > s->code_gen_highwater)) {
+if (unlikely((void *)s->code_ptr > s->code_gen_highwater)) {
  return -1;
  }
  }



Is a comparison of void pointers portable?


Of course.  Particularly since these really are pointers into the same 
allocated object.  That's 100% ANSI C.



code_gen_highwater should be fixed anyway because
in translate-all a difference is calculated with it.


Yes, but we freely make use of this gcc extension in many places.


r~

[Qemu-devel] [PATCH 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu]

Currently, dump-guest-memory supports synchronous operation only. This patch
sets are adding "detach" support for it (just like "migrate -d" for
migration). When "-d" is provided, dump-guest-memory command will return
immediately without hanging user. This should be useful when the backend
storage for the dump file is very slow.

Peter Xu (2):
  dump-guest-memory: add "detach" flag for QMP/HMP interfaces
  dump-guest-memory: add basic "detach" support.

 dump.c| 62 ++-
 hmp-commands.hx   |  5 +++--
 hmp.c |  3 ++-
 include/sysemu/dump.h |  4 
 qapi-schema.json  |  3 ++-
 qmp-commands.hx   |  4 ++--
 qmp.c |  9 
 vl.c  |  3 +++
 8 files changed, 81 insertions(+), 12 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 2/2] dump-guest-memory: add basic "detach" support.

[Peter Xu]

This will allow the user specify "-d" (just like command
"migrate") when using "dump-guest-memory" command. When
specified, one background thread is created to do the dump work.
One flag is added to show whether there is a background dump
work in progress.

Signed-off-by: Peter Xu 
---
 dump.c| 59 ++-
 include/sysemu/dump.h |  4 
 qmp.c |  9 
 vl.c  |  3 +++
 4 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/dump.c b/dump.c
index 3ec3423..c2e14cd 100644
--- a/dump.c
+++ b/dump.c
@@ -1442,6 +1442,9 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
 Error *err = NULL;
 int ret;
 
+s->has_format = has_format;
+s->format = format;
+
 /* kdump-compressed is conflict with paging and filter */
 if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
 assert(!paging && !has_filter);
@@ -1595,6 +1598,45 @@ cleanup:
 dump_cleanup(s);
 }
 
+/* whether there is a dump in progress */
+extern bool dump_in_progress_p;
+
+static bool dump_ownership_set(bool value)
+{
+return atomic_xchg(_in_progress_p, value);
+}
+
+/* return true when owner taken, false otherwise */
+static bool dump_ownership_take(void)
+{
+bool ret = dump_ownership_set(1);
+return ret == 0;
+}
+
+/* we should only call this after all dump work finished */
+static void dump_ownership_release(void)
+{
+dump_ownership_set(0);
+}
+
+static void dump_process(DumpState *s, Error **errp)
+{
+if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
+create_kdump_vmcore(s, errp);
+} else {
+create_vmcore(s, errp);
+}
+dump_ownership_release();
+}
+
+static void *dump_thread(void *data)
+{
+DumpState *s = (DumpState *)data;
+dump_process(s, NULL);
+g_free(s);
+return NULL;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
int64_t begin, bool has_length,
int64_t length, bool has_format,
@@ -1662,6 +1704,12 @@ void qmp_dump_guest_memory(bool paging, const char 
*file, bool has_begin,
 return;
 }
 
+/* we could only allow one dump at a time. */
+if (!dump_ownership_take()) {
+error_setg(errp, "another dump is in progress.");
+return;
+}
+
 s = g_malloc0(sizeof(DumpState));
 
 dump_init(s, fd, has_format, format, paging, has_begin,
@@ -1672,13 +1720,14 @@ void qmp_dump_guest_memory(bool paging, const char 
*file, bool has_begin,
 return;
 }
 
-if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-create_kdump_vmcore(s, errp);
+if (!detach) {
+dump_process(s, errp);
+g_free(s);
 } else {
-create_vmcore(s, errp);
+qemu_thread_create(>dump_thread, "dump_thread", dump_thread,
+   s, QEMU_THREAD_DETACHED);
+/* DumpState is freed in dump thread */
 }
-
-g_free(s);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7e4ec5c..2aeffc8 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -183,6 +183,10 @@ typedef struct DumpState {
 off_t offset_page;  /* offset of page part in vmcore */
 size_t num_dumpable;/* number of page that can be dumped */
 uint32_t flag_compress; /* indicate the compression format */
+
+QemuThread dump_thread; /* only used when do async dump */
+bool has_format;/* whether format is provided */
+DumpGuestMemoryFormat format; /* valid only if has_format == true */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/qmp.c b/qmp.c
index 0a1fa19..ea57b57 100644
--- a/qmp.c
+++ b/qmp.c
@@ -168,12 +168,21 @@ SpiceInfo *qmp_query_spice(Error **errp)
 };
 #endif
 
+extern bool dump_in_progress_p;
+
 void qmp_cont(Error **errp)
 {
 Error *local_err = NULL;
 BlockBackend *blk;
 BlockDriverState *bs;
 
+/* if there is a dump in background, we should wait until the dump
+ * finished */
+if (dump_in_progress_p) {
+error_setg(errp, "there is a dump in process, please wait.");
+return;
+}
+
 if (runstate_needs_reset()) {
 error_setg(errp, "Resetting the Virtual Machine is required");
 return;
diff --git a/vl.c b/vl.c
index 525929b..ef7a936 100644
--- a/vl.c
+++ b/vl.c
@@ -204,6 +204,9 @@ bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 
+/* whether dump is in progress */
+bool dump_in_progress_p = false;
+
 static int has_defaults = 1;
 static int default_serial = 1;
 static int default_parallel = 1;
-- 
2.4.3

[Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu]

Currently, dump-guest-memory supports synchronous operation only. This patch
sets are adding "detach" support for it (just like "migrate -d" for
migration). When "-d" is provided, dump-guest-memory command will return
immediately without hanging user. This should be useful when the backend
storage for the dump file is very slow.

Peter Xu (2):
  dump-guest-memory: add "detach" flag for QMP/HMP interfaces
  dump-guest-memory: add basic "detach" support.

 dump.c| 62 ++-
 hmp-commands.hx   |  5 +++--
 hmp.c |  3 ++-
 include/sysemu/dump.h |  4 
 qapi-schema.json  |  3 ++-
 qmp-commands.hx   |  4 ++--
 qmp.c |  9 
 vl.c  |  3 +++
 8 files changed, 81 insertions(+), 12 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH REPOST 1/2] dump-guest-memory: add "detach" flag for QMP/HMP interfaces

[Peter Xu]

This patch only add the interfaces, but not implementing them.

Signed-off-by: Peter Xu 
---
 dump.c   | 3 ++-
 hmp-commands.hx  | 5 +++--
 hmp.c| 3 ++-
 qapi-schema.json | 3 ++-
 qmp-commands.hx  | 4 ++--
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/dump.c b/dump.c
index 78b7d84..3ec3423 100644
--- a/dump.c
+++ b/dump.c
@@ -1598,7 +1598,8 @@ cleanup:
 void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
int64_t begin, bool has_length,
int64_t length, bool has_format,
-   DumpGuestMemoryFormat format, Error **errp)
+   DumpGuestMemoryFormat format, bool detach,
+   Error **errp)
 {
 const char *p;
 int fd = -1;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..8e27671 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1056,10 +1056,11 @@ ETEXI
 
 {
 .name   = "dump-guest-memory",
-.args_type  = 
"paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
-.params = "[-p] [-z|-l|-s] filename [begin length]",
+.args_type  = 
"paging:-p,detach:-d,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
+.params = "[-p] [-d] [-z|-l|-s] filename [begin length]",
 .help   = "dump guest memory into file 'filename'.\n\t\t\t"
   "-p: do paging to get guest's memory mapping.\n\t\t\t"
+  "-d: return immediately (not wait for 
completion).\n\t\t\t"
   "-z: dump in kdump-compressed format, with zlib 
compression.\n\t\t\t"
   "-l: dump in kdump-compressed format, with lzo 
compression.\n\t\t\t"
   "-s: dump in kdump-compressed format, with snappy 
compression.\n\t\t\t"
diff --git a/hmp.c b/hmp.c
index 2140605..4c5480d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1580,6 +1580,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
*qdict)
 {
 Error *err = NULL;
 bool paging = qdict_get_try_bool(qdict, "paging", false);
+bool detach = qdict_get_try_bool(qdict, "detach", false);
 bool zlib = qdict_get_try_bool(qdict, "zlib", false);
 bool lzo = qdict_get_try_bool(qdict, "lzo", false);
 bool snappy = qdict_get_try_bool(qdict, "snappy", false);
@@ -1619,7 +1620,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
*qdict)
 prot = g_strconcat("file:", file, NULL);
 
 qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
-  true, dump_format, );
+  true, dump_format, detach, );
 hmp_handle_error(mon, );
 g_free(prot);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..1211082 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2132,7 +2132,8 @@
 ##
 { 'command': 'dump-guest-memory',
   'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
-'*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
+'*length': 'int', '*format': 'DumpGuestMemoryFormat',
+'detach': 'bool'} }
 
 ##
 # @DumpGuestMemoryCapability:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..86864f6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -840,8 +840,8 @@ EQMP
 
 {
 .name   = "dump-guest-memory",
-.args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
-.params = "-p protocol [begin] [length] [format]",
+.args_type  = "paging:b,detach:d,protocol:s,begin:i?,end:i?,format:s?",
+.params = "-p protocol [-d] [begin] [length] [format]",
 .help   = "dump guest memory to file",
 .mhandler.cmd_new = qmp_marshal_dump_guest_memory,
 },
-- 
2.4.3

[Qemu-devel] [Bug 1518969] [NEW] Instance of QEMU doesn't unplug virtio scsi disk after device_del and drive_del commands

[Marian Horban]

Public bug reported:

device_del and drive_del commands don't cause virtio disk detaching

Steps to reproduce:
1. Run instance

2. Attach virtio scsi disk

3. Reboot instance

4. Immediately after reboot detach disk with QEMU commands:
device_del
drive_del

Expected result:
Disk should be detached anyway

Actual:
Domain info contains attached disk even after waiting long period of time(5 min 
in my case).

Additional info:
QEMU process:
root 29010 42.6  1.9 562036 61272 ?Rl   03:42   0:01 
/usr/bin/qemu-system-x86_64 -name instance-0069 -S -machine 
pc-i440fx-trusty,accel=tcg,usb=off -m 64 -realtime mlock=off -smp 
1,sockets=1,cores=1,threads=1 -uuid d2418536-2547-4740-96b5-0d4f1d74b9f3 
-smbios type=1,manufacturer=OpenStack Foundation,product=OpenStack 
Nova,version=13.0.0,serial=1fd8f69a-909b-4ed1-aae9-4fc9318fc622,uuid=d2418536-2547-4740-96b5-0d4f1d74b9f3,family=Virtual
 Machine -no-user-config -nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/instance-0069.monitor,server,nowait
 -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown 
-boot strict=on -kernel 
/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/kernel 
-initrd 
/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/ramdisk 
-append root=/dev/vda console=tty0 console=ttyS0 no_timer_check -device 
piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -drive 
file=/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/disk,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
 -device 
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
 -drive 
file=/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/disk.config,if=none,id=drive-ide0-1-1,readonly=on,format=raw,cache=none
 -device ide-cd,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 -netdev 
tap,fd=18,id=hostnet0 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=fa:16:3e:1a:10:3d,bus=pci.0,addr=0x3 
-chardev 
file,id=charserial0,path=/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/console.log
 -device isa-serial,chardev=charserial0,id=serial0 -chardev pty,id=charserial1 
-device isa-serial,chardev=charserial1,id=serial1 -vnc 127.0.0.1:1 -k en-us 
-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5

QEMU version:
qemu-system-x86_64 --version
QEMU emulator version 2.0.0 (Debian 2.0.0+dfsg-2ubuntu1.19), Copyright (c) 
2003-2008 Fabrice Bellard

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  Instance of QEMU doesn't unplug virtio scsi disk after device_del and
  drive_del commands

Status in QEMU:
  New

Bug description:
  device_del and drive_del commands don't cause virtio disk detaching

  Steps to reproduce:
  1. Run instance

  2. Attach virtio scsi disk

  3. Reboot instance

  4. Immediately after reboot detach disk with QEMU commands:
  device_del
  drive_del

  Expected result:
  Disk should be detached anyway

  Actual:
  Domain info contains attached disk even after waiting long period of time(5 
min in my case).

  Additional info:
  QEMU process:
  root 29010 42.6  1.9 562036 61272 ?Rl   03:42   0:01 
/usr/bin/qemu-system-x86_64 -name instance-0069 -S -machine 
pc-i440fx-trusty,accel=tcg,usb=off -m 64 -realtime mlock=off -smp 
1,sockets=1,cores=1,threads=1 -uuid d2418536-2547-4740-96b5-0d4f1d74b9f3 
-smbios type=1,manufacturer=OpenStack Foundation,product=OpenStack 
Nova,version=13.0.0,serial=1fd8f69a-909b-4ed1-aae9-4fc9318fc622,uuid=d2418536-2547-4740-96b5-0d4f1d74b9f3,family=Virtual
 Machine -no-user-config -nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/instance-0069.monitor,server,nowait
 -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown 
-boot strict=on -kernel 
/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/kernel 
-initrd 
/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/ramdisk 
-append root=/dev/vda console=tty0 console=ttyS0 no_timer_check -device 
piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -drive 
file=/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/disk,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
 -device 
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
 -drive 
file=/opt/stack/data/nova/instances/d2418536-2547-4740-96b5-0d4f1d74b9f3/disk.config,if=none,id=drive-ide0-1-1,readonly=on,format=raw,cache=none
 -device ide-cd,bus=ide.1,unit=1,drive=drive-ide0-1-1,id=ide0-1-1 -netdev 
tap,fd=18,id=hostnet0 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=fa:16:3e:1a:10:3d,bus=pci.0,addr=0x3 
-chardev 

Re: [Qemu-devel] [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target

[Paolo Bonzini]

On 23/11/2015 09:17, Ming Lin wrote:
> On Sat, 2015-11-21 at 14:11 +0100, Paolo Bonzini wrote:
>>
>> On 20/11/2015 01:20, Ming Lin wrote:
>>> One improvment could be to use google's NVMe vendor extension that
>>> I send in another thread, aslo here:
>>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
>>>
>>> Qemu side:
>>> http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
>>> Kernel side also here:
>>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
>>
>> How much do you get with vhost-nvme plus vendor extension, compared to
>> 190 MB/s for QEMU?
> 
> There is still some bug. I'll update.

Sure.

>> Note that in all likelihood, QEMU can actually do better than 190 MB/s,
>> and gain more parallelism too, by moving the processing of the
>> ioeventfds to a separate thread.  This is similar to
>> hw/block/dataplane/virtio-blk.c.
>>
>> It's actually pretty easy to do.  Even though
>> hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
>> access in QEMU is now thread-safe.  I have pending patches for 2.6 that
>> cut that file down to a mere 200 lines of code, NVMe would probably be
>> about the same.
> 
> Is there a git tree for your patches?

No, not yet.  I'll post them today or tomorrow, will make sure to Cc you.

> Did you mean some pseduo code as below?
> 1. need a iothread for each cq/sq?
> 2. need a AioContext for each cq/sq?
> 
>  hw/block/nvme.c | 32 ++--
>  hw/block/nvme.h |  8 
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f27fd35..fed4827 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -28,6 +28,8 @@
>  #include "sysemu/sysemu.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
> +#include "qom/object_interfaces.h"
>  
>  #include "nvme.h"
>  
> @@ -558,9 +560,22 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
>  uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
>  
>  event_notifier_init(>notifier, 0);
> -event_notifier_set_handler(>notifier, nvme_cq_notifier);
>  memory_region_add_eventfd(>iomem,
>  0x1000 + offset, 4, false, 0, >notifier);
> +
> +object_initialize(>internal_iothread_obj,
> +  sizeof(cq->internal_iothread_obj),
> +  TYPE_IOTHREAD);
> +user_creatable_complete(OBJECT(>internal_iothread_obj), 
> _abort);

For now, you have to use one iothread for all cq/sq of a single NVMe
device; multiqueue block layer is planned for 2.7 or 2.8.  Otherwise
yes, it's very close to just these changes.

If you use "-object iothread,id=NN" and a iothread property, you can
also use an N:M model with multiple disks attached to the same iothread.
 Defining the iothread property is like

object_property_add_link(obj, "iothread", TYPE_IOTHREAD,
(Object **)>conf.iothread,
qdev_prop_allow_set_link_before_realize,
OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);

Thanks,

Paolo

> +cq->iothread = >internal_iothread_obj;
> +cq->ctx = iothread_get_aio_context(cq->iothread);
> +//Question: Need a conf.blk for each cq/sq???
> +//blk_set_aio_context(cq->conf->conf.blk, cq->ctx);
> +aio_context_acquire(cq->ctx);
> +aio_set_event_notifier(cq->ctx, >notifier, true,
> +   nvme_cq_notifier);
> +aio_context_release(cq->ctx);
>  }
>  
>  static void nvme_sq_notifier(EventNotifier *e)
> @@ -578,9 +593,22 @@ static void nvme_init_sq_eventfd(NvmeSQueue *sq)
>  uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
>  
>  event_notifier_init(>notifier, 0);
> -event_notifier_set_handler(>notifier, nvme_sq_notifier);
>  memory_region_add_eventfd(>iomem,
>  0x1000 + offset, 4, false, 0, >notifier);
> +
> +object_initialize(>internal_iothread_obj,
> +  sizeof(sq->internal_iothread_obj),
> +  TYPE_IOTHREAD);
> +user_creatable_complete(OBJECT(>internal_iothread_obj), 
> _abort);
> +sq->iothread = >internal_iothread_obj;
> +sq->ctx = iothread_get_aio_context(sq->iothread);
> +//Question: Need a conf.blk for each cq/sq???
> +//blk_set_aio_context(sq->conf->conf.blk, sq->ctx);
> +
> +aio_context_acquire(sq->ctx);
> +aio_set_event_notifier(sq->ctx, >notifier, true,
> +   nvme_sq_notifier);
> +aio_context_release(sq->ctx);
>  }
>  
>  static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 608f202..171ee0b 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -667,6 +667,10 @@ typedef struct NvmeSQueue {
>   * do not go over this value will not result in MMIO writes (but will
>   * still write the tail pointer to the 

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Marc-André Lureau]

Hi

- Original Message -
> >
> > You can't migrate the peers.
> 
> Then explain the case N'=0 to me: how can you migrate the master so that
> it's connected to a server afterwards?

Dest qemu: -incoming.. -chardev socket,path=dest-server 

That is, start your destination qemu with a destination server. The master
origin qemu will migrate the shared memory, and the dest memory will be sync
when the migration is done.

> > It works fine in the tests. Feel free to point out races or other issues.
> 
> I think I did: doorbell detection is inherently racy.
> 
> If you think it isn't, please refute my reasoning.

I gave you some clues on how I did it in ivshmem-test.c: waiting for a 
signature on the memory to be mapped (and also checking that peers received ids)

> If it's not broken, please explain to me how the guest should find out
> whether its ivshmem device sports a doorbell.

If you have received ID, you should be good to use the doorbell.

Re: [Qemu-devel] [PATCH v5 0/6] fw_cfg: spec update, misc. cleanup, optimize read

[Gabriel L. Somlo]

Ping ?

I can send out v6 and fix the commit blurb typo in patch 6/6 pointed
out by Laszlo (unless the series is already winding its way toward
eventually being applied).

Please advise.

Thanks,
--Gabriel

On Thu, Nov 05, 2015 at 09:32:46AM -0500, Gabriel L. Somlo wrote:
> New since v4:
> 
>   - Patch 5: Protect against (undefined) left-shifting a uint64_t
> by 64 bits
> 
>   - Patches 4 & 5: Reworked comment and commit blurb wording
> 
> Laszlo, thanks again for the thorough review (and the "C master class") :)
> 
> Regards,
>   --Gabriel
> 
> >New since v3:
> >
> >- Patches 1..3 unchanged
> >
> >- Inserted new patch at position #4: avoid calculating an entry
> >  pointer for the current item when s->cur_entry is FW_CFG_INVALID
> >
> >- Patch 5 (formerly #4) now has the generic read method correctly
> >  shift-left (adding 0 padding on the right) in the event that the
> >  current item payload is exhausted in the course of a multi-byte
> >  read operation (Laszlo, thanks again for catching that!)
> >
> >- Patch 6 (formerly #5) is a rebased version of its former self,
> >  but with no actual significant modifications.
> >
> >>New since v2:
> >>
> >>- Patches 1-3: updated to address Laszlo's suggestions for better
> >>  and more accurate change descriptions in commit logs, comments,
> >>  etc.
> >>
> >>- Patch 4: Per Laszlo's recommendation, this has been split into
> >>  two separate components for improved legibility:
> >>
> >>- Patch 4/5: Introduces the new generic read method, and
> >>  replaces the existing MMIO logic
> >>
> >>- Patch 5/5: Replaces the IOPort read logic, and cleans
> >>  up the remaining unused bits of code.
> >>
> >>>This series' main purpose is to update (and simplify) the specified
> >>>read callback behavior. An earlier standalone patch to move qemu function
> >>>call API documentation into fw_cfg.h should logically be part of the 
> >>>series.
> >>>
> >>>Here's the summary of what each patch does:
> >>>
> >>>- Patch 1/4 is an updated version of the standalone v1 patch
> >>>  I sent out earlier; it moves all the qemu-internal host-side
> >>>  function call api documentation out of docs/specs/fw_cfg.txt,
> >>>  and into the fw_cfg.h header file, next to the prototype of
> >>>  each documented api function.
> >>>
> >>>- Patch 2/4 modifies the specified behavior of read callbacks
> >>>  (from being invoked once per byte read, to being invoked once,
> >>>   before ANY data is read, specifically once each time an item
> >>>   is selected).
> >>>
> >>>- Patch 3/4 additionally removes the now-redundant offset argument
> >>>  from the read callback prototype.
> >>>
> >>>- Finally, 4/4 consolidates (non-DMA) reads, minimizing the number
> >>>  of times redundant sanity checks are performed, particularly for
> >>>  wide (> byte) sized reads.
> 
> Gabriel L. Somlo (6):
>   fw_cfg: move internal function call docs to header file
>   fw_cfg: amend callback behavior spec to once per select
>   fw_cfg: remove offset argument from callback prototype
>   fw_cfg: avoid calculating invalid current entry pointer
>   fw_cfg: add generic non-DMA read method
>   fw_cfg: replace ioport data read with generic method
> 
>  docs/specs/fw_cfg.txt |  85 +-
>  hw/arm/virt-acpi-build.c  |   2 +-
>  hw/i386/acpi-build.c  |   2 +-
>  hw/nvram/fw_cfg.c |  75 +--
>  include/hw/nvram/fw_cfg.h | 128 
> +-
>  trace-events  |   2 +-
>  6 files changed, 166 insertions(+), 128 deletions(-)
> 
> -- 
> 2.4.3
> 

[Qemu-devel] [PATCH] [doc] Introduce coding style for errors

[Lluís Vilanova]

Gives some general guidelines for reporting errors in QEMU.


Signed-off-by: Lluís Vilanova 
---
 HACKING  |   52 ++
 include/qapi/error.h |   12 
 util/error.c |   24 +--
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/HACKING b/HACKING
index 12fbc8a..af36009 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,55 @@ painful. These are:
  * you may assume that integers are 2s complement representation
  * you may assume that right shift of a signed integer duplicates
the sign bit (ie it is an arithmetic shift, not a logical shift)
+
+7. Error reporting
+
+QEMU provides two different mechanisms for reporting errors. You should use one
+of these mechanisms instead of manually reporting them (i.e., do not use
+'printf', 'exit' or 'abort').
+
+7.1. Errors in user inputs
+
+QEMU provides the functions in "include/qemu/error-report.h" to report errors
+related to inputs provided by the user (e.g., command line arguments or
+configuration files).
+
+These functions generate error messages with a uniform format that can 
reference
+a location on the offending input.
+
+7.2. Other errors
+
+QEMU provides the functions in "include/qapi/error.h" to report other types of
+errors (i.e., not triggered by command line arguments or configuration files).
+
+Functions in this header are used to accumulate error messages in an 'Error'
+object, which can be propagated up the call chain where it is finally reported.
+
+In its simplest form, you can immediately report an error with:
+
+error_setg(_now, "Error with %s", "arguments");
+
+For convenience, you can also use 'error_setg_errno' and 'error_setg_win32' to
+append a message for OS-specific errors, and 'error_setg_file_open' for errors
+when opening files.
+
+7.3. Errors with an immediate exit/abort
+
+There are two convenience functions to report errors that immediately terminate
+QEMU:
+
+* 'error_setg(_fatal, msg, ...)'
+
+  Reports a fatal error with the given error message and exits QEMU.
+
+* 'error_setg(_abort, msg, ...)'
+
+  Reports a programming error with the given error message and aborts QEMU.
+
+In general, you should report errors but *not* terminate QEMU if the errors are
+(or can be) triggered by guest code (e.g., some unimplemented corner case).
+This also applies to hardware emulation (it is OK to leave the device in a
+non-operational state until next reboot, though). Otherwise that can be abused
+by guest code to terminate QEMU.
+
+In such cases you should use the argument 'error_now'.
diff --git a/include/qapi/error.h b/include/qapi/error.h
index c69dddb..e2bfc19 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -53,6 +53,9 @@
  * Call a function treating errors as fatal:
  * foo(arg, _fatal);
  *
+ * Call a function immediately showing all errors:
+ * foo(arg, _now);
+ *
  * Receive an error and pass it on to the caller:
  * Error *err = NULL;
  * foo(arg, );
@@ -104,6 +107,7 @@ ErrorClass error_get_class(const Error *err);
  * then.
  * If @errp is _abort, print a suitable message and abort().
  * If @errp is _fatal, print a suitable message and exit(1).
+ * If @errp is _now, print a suitable message.
  * If @errp is anything else, *@errp must be NULL.
  * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
  * human-readable error message is made from printf-style @fmt, ...
@@ -154,6 +158,7 @@ void error_setg_win32_internal(Error **errp,
  * abort().
  * Else, if @dst_errp is _fatal, print a suitable message and
  * exit(1).
+ * Else, if @dst_errp is _now, print a suitable message.
  * Else, if @dst_errp already contains an error, ignore this one: free
  * the error object.
  * Else, move the error object from @local_err to *@dst_errp.
@@ -217,4 +222,11 @@ extern Error *error_abort;
  */
 extern Error *error_fatal;
 
+/*
+ * Pass to error_setg() & friends to immediately show an error.
+ *
+ * Has the same formatting of #error_abort, but does not terminate QEMU.
+ */
+extern Error *error_now;
+
 #endif
diff --git a/util/error.c b/util/error.c
index 8b86490..e507039 100644
--- a/util/error.c
+++ b/util/error.c
@@ -27,6 +27,7 @@ struct Error
 
 Error *error_abort;
 Error *error_fatal;
+Error *error_now;
 
 static void error_handle_fatal(Error **errp, Error *err)
 {
@@ -62,9 +63,14 @@ static void error_setv(Error **errp,
 err->func = func;
 
 error_handle_fatal(errp, err);
-*errp = err;
-
-errno = saved_errno;
+if (errp == _now) {
+fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
+err->func, err->src, err->line);
+error_report_err(err);
+} else {
+*errp = err;
+errno = saved_errno;
+}
 }
 
 void error_set_internal(Error **errp,
@@ -226,9 +232,15 @@ void error_propagate(Error **dst_errp, Error *local_err)
 return;
 }
 error_handle_fatal(dst_errp, local_err);
-if (dst_errp 

Re: [Qemu-devel] Is ivshmem's test for unix domain client socket valid?

[Paolo Bonzini]

On 20/11/2015 18:56, Markus Armbruster wrote:
> Looks rather fishy:
> 
> if (strncmp(s->server_chr->filename, "unix:", 5)) {
> error_setg(errp, "chardev is not a unix client socket");
> return;
> }
> 
> Paolo, is this reliable?

Yes, though by total chance (which is already a curious definition of
reliability).  If you create "-chardev file,path=unix:foo",
chr->filename ends up with "file" because only pty and socket chardevs
set chr->filename.  Everything else does not set it, and the field is
thus set to the default---the backend name.

> Is it the proper way to check?

It's the only one, which makes it the most proper too.  Sarcasm is
intentional.

Paolo

Re: [Qemu-devel] [PATCH v2 4/4] json-streamer: Limit number of tokens in addition to total size

[Paolo Bonzini]

On 20/11/2015 18:32, Eric Blake wrote:
> > static void qlist_size_iter(QObject *obj, void *opaque)
> > {
> > size_t *count = opaque;
> > (*count)++;
> > }
> 
> Yuck - we don't track size independently?  Seems like it might make a
> worthwhile addition,

Would you change your mind, if I told you that qlist_size is unused? :)

Paolo

Re: [Qemu-devel] [RFC] virtio: convert to use DMA api

[Cornelia Huck]

On Mon, 23 Nov 2015 11:52:50 +0200
"Michael S. Tsirkin"  wrote:

> On Mon, Nov 23, 2015 at 10:36:45AM +0100, Cornelia Huck wrote:
> > On Mon, 23 Nov 2015 15:41:11 +0800
> > Jason Wang  wrote:
> > 
> > > Currently, all virtio devices bypass IOMMU completely. This is because
> > > address_space_memory is assumed and used during DMA emulation. This
> > > patch converts the virtio core API to use DMA API. This idea is
> > > 
> > > - introducing a new transport specific helper to query the dma address
> > >   space. (only pci version is implemented).
> > > - query and use this address space during virtio device guest memory
> > >   accessing
> > > 
> > > With this virtiodevices will not bypass IOMMU anymore. Little tested with
> > > intel_iommu=on with virtio guest DMA series posted in
> > > https://lkml.org/lkml/2015/10/28/64.
> > > 
> > > TODO:
> > > - Feature bit for this
> > 
> > I'm still not convinced about that feature bit stuff. It just feels
> > wrong to use a mechanism that conveys negotiable device features to
> > configure what is basically a platform/hypervisor feature. I'd rather
> > see this out of the virtio layer and into the pci layer.
> 
> I agree it's not something that needs to be negotiated.
> 
> But given that we are changing device and driver behaviour in a drastic
> way, it seems prudent to have a way for guest and host to discover that,
> even if it's just in case.
> 
> Whether it's a feature bit or something else pci-specific,
> depends on whether this makes sense for any other transport.
> 
> > > - Implement this for all transports
> > 
> > Is it OK to just keep a fallback to today's implementation for
> > transports for which the iommu concept doesn't make sense and that will
> > always have an identity mapping?
> 
> Is there a reason why iommu does not make any sense for ccw or mmio?

Channel I/O uses a different concept when performing data transfers:
The addresses referenced by the channel program can be anywhere in the
guest's main memory (today's qemu only supports adresses under 2G, as
IDAWs are not yet implemented). Basically, the OS will refer to
addresses anywhere in its address space and the channel subsystem is
subsequently free to read from/write to that memory. This also applies
to queues: The OS will tell the channel subsystem/device which memory
areas can be accessed (the proprietary qdio transport is the same as
virtio in that regard). The first time we needed an iommu on s390 was
when pci was introduced.

If I understand correctly what an iommu does, that concept does not
match. For dma, we can fall back to an identity mapping, but I don't
see how we can fit in an iommu.

The mmio transport is a completely different beast; I'm afraid I don't
know enough about it to say how it interacts with iommus.

Re: [Qemu-devel] [RFC] virtio: convert to use DMA api

[Peter Maydell]

On 23 November 2015 at 14:34, Cornelia Huck  wrote:
> The mmio transport is a completely different beast; I'm afraid I don't
> know enough about it to say how it interacts with iommus.

Conceptually, it's just a device that does DMA, I think.
You could in theory put it behind an IOMMU, but nobody does
(and I don't know whether the kernel code could cope with
that).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio-blk: Move resetting of req->mr_next to virtio_blk_handle_rw_error

[Paolo Bonzini]

On 23/11/2015 01:41, Fam Zheng wrote:
> "werror=report" would free the req in virtio_blk_handle_rw_error, we
> mustn't write to it in that case.
> 
> Reported-by: Paolo Bonzini 
> Signed-off-by: Fam Zheng 
> ---
>  hw/block/virtio-blk.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 848f3fe..756ae5c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -72,6 +72,9 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, 
> int error,
>  VirtIOBlock *s = req->dev;
>  
>  if (action == BLOCK_ERROR_ACTION_STOP) {
> +/* Break the link as the next request is going to be parsed from the
> + * ring again. Otherwise we may end up doing a double completion! */
> +req->mr_next = NULL;
>  req->next = s->rq;
>  s->rq = req;
>  } else if (action == BLOCK_ERROR_ACTION_REPORT) {
> @@ -112,10 +115,6 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>   * happen on the other side of the migration).
>   */
>  if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
> -/* Break the link in case the next request is added to the
> - * restart queue and is going to be parsed from the ring 
> again.
> - */
> -req->mr_next = NULL;
>  continue;
>  }
>  }
> 

Reviewed-by: Paolo Bonzini 

Re: [Qemu-devel] [PATCH 1/2] tests/Makefile: Add more dependencies for test-timed-average

[Paolo Bonzini]

On 23/11/2015 13:39, Kevin Wolf wrote:
>  tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
>   libqemuutil.a stubs/clock-warp.o stubs/cpu-get-icount.o \
> - stubs/notify-event.o stubs/replay.o
> + stubs/notify-event.o stubs/replay.o stubs/mon-is-qmp.o 
> stubs/fd-register.o \
> + stubs/mon-printf.o
>  

Why not just add libqemustub.a?  (If it works, do not even bother
reposting).

Paolo

Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> - Original Message -
>> >
>> > You can't migrate the peers.
>> 
>> Then explain the case N'=0 to me: how can you migrate the master so that
>> it's connected to a server afterwards?
>
> Dest qemu: -incoming.. -chardev socket,path=dest-server 
>
> That is, start your destination qemu with a destination server. The master
> origin qemu will migrate the shared memory, and the dest memory will be sync
> when the migration is done.

Got it!

>> > It works fine in the tests. Feel free to point out races or other issues.
>> 
>> I think I did: doorbell detection is inherently racy.
>> 
>> If you think it isn't, please refute my reasoning.
>
> I gave you some clues on how I did it in ivshmem-test.c: waiting for a
> signature on the memory to be mapped (and also checking that peers
> received ids)
>
>> If it's not broken, please explain to me how the guest should find out
>> whether its ivshmem device sports a doorbell.
>
> If you have received ID, you should be good to use the doorbell.

That's not a complete answer, so let me try a different tack.

What value will a read of register IVPosition yield

* if the device has no doorbell:

  I think the answer is -1.

* if the device has a doorbell, but it isn't ready, yet:

  I think the answer is -1.

* if the device has a doorbell, and it is ready.

  This is the part you answered already: >= 0.

Please correct mistakes.