Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip

[Greg Kurz]

On Tue, 16 May 2017 14:35:55 +1000
David Gibson  wrote:

> On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote:
> > QEMU should exit if the user explicitely asked for kernel-irqchip support
> > and "xics-kvm" initialization fails.
> > 
> > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> > in xics-kvm creation") reads:
> > 
> > While there, improve the error message when we can't satisfy an
> > explicit user request for "xics-kvm", and exit(1) instead of abort().
> > Simplify the abort when we can't create "xics".
> > 
> > This patch adds the missing call to exit().
> > 
> > Signed-off-by: Greg Kurz 
> > ---
> >  hw/ppc/spapr.c |1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index abfb99b71b7d..f477d7b8a210 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int 
> > nr_irqs, Error **errp)
> >  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> >  error_reportf_err(err,
> >"kernel_irqchip requested but unavailable: 
> > ");
> > +exit(EXIT_FAILURE);
> >  } else {
> >  error_free(err);
> >  }
> >   
> 
> 
> This doesn't look right.  We have an errp pointer in the caller.  So
> on failure we should error_propagate(), rather than deciding for
> ourselves that exiting is the right course of action.
> 

I generally agree with that but can the caller cope with the fact that
the user passed -machine accel=kvm,kernel_irqchip=on and this cannot
be satisfied ?


pgp2BRIkdRmPd.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] fix mingw build failure

[Stefan Weil]

Am 16.05.2017 um 07:24 schrieb Gerd Hoffmann:

Signed-off-by: Gerd Hoffmann 
---
 crypto/random-platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/random-platform.c b/crypto/random-platform.c
index 0eddb915b7..92eed0ee78 100644
--- a/crypto/random-platform.c
+++ b/crypto/random-platform.c
@@ -23,7 +23,7 @@
 #include "crypto/random.h"

 #ifdef _WIN32
-#include 
+#include 
 static HCRYPTPROV hCryptProv;
 #else
 static int fd; /* a file handle to either /dev/urandom or /dev/random */



Thank you.

Reviewed-by: Stefan Weil 

cc`ing qemu-trivial.

[Qemu-devel] [PATCH] fix mingw build failure

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 crypto/random-platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/random-platform.c b/crypto/random-platform.c
index 0eddb915b7..92eed0ee78 100644
--- a/crypto/random-platform.c
+++ b/crypto/random-platform.c
@@ -23,7 +23,7 @@
 #include "crypto/random.h"
 
 #ifdef _WIN32
-#include 
+#include 
 static HCRYPTPROV hCryptProv;
 #else
 static int fd; /* a file handle to either /dev/urandom or /dev/random */
-- 
2.9.3

Re: [Qemu-devel] [PATCH 4/6] spapr: sanitize error handling in spapr_ics_create()

[David Gibson]

On Mon, May 15, 2017 at 01:39:45PM +0200, Greg Kurz wrote:
> The spapr_ics_create() function handles errors in a rather convoluted
> way, with two local Error * variables. Moreover, failing to parent the
> ICS object to the machine should be considered as a bug but it is
> currently ignored.
> 
> This patch addresses both issues.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-2.10

> ---
>  hw/ppc/spapr.c |   19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44f7dc7f40e9..c53989bb10b1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -101,21 +101,26 @@ static ICSState *spapr_ics_create(sPAPRMachineState 
> *spapr,
>const char *type_ics,
>int nr_irqs, Error **errp)
>  {
> -Error *err = NULL, *local_err = NULL;
> +Error *local_err = NULL;
>  Object *obj;
>  
>  obj = object_new(type_ics);
> -object_property_add_child(OBJECT(spapr), "ics", obj, NULL);
> +object_property_add_child(OBJECT(spapr), "ics", obj, _abort);
>  object_property_add_const_link(obj, "xics", OBJECT(spapr), _abort);
> -object_property_set_int(obj, nr_irqs, "nr-irqs", );
> +object_property_set_int(obj, nr_irqs, "nr-irqs", _err);
> +if (local_err) {
> +goto error;
> +}
>  object_property_set_bool(obj, true, "realized", _err);
> -error_propagate(, local_err);
> -if (err) {
> -error_propagate(errp, err);
> -return NULL;
> +if (local_err) {
> +goto error;
>  }
>  
>  return ICS_SIMPLE(obj);
> +
> +error:
> +error_propagate(errp, local_err);
> +return NULL;
>  }
>  
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error 
> **errp)
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 4/6] spapr: sanitize error handling in spapr_ics_create()

[David Gibson]

On Mon, May 15, 2017 at 02:06:18PM +0200, Greg Kurz wrote:
> On Mon, 15 May 2017 13:59:33 +0200
> Cédric Le Goater  wrote:
> 
> > On 05/15/2017 01:39 PM, Greg Kurz wrote:
> > > The spapr_ics_create() function handles errors in a rather convoluted
> > > way, with two local Error * variables. Moreover, failing to parent the
> > > ICS object to the machine should be considered as a bug but it is
> > > currently ignored.  
> > 
> > I am not sure what should be done for object_property_add_child()
> > errors but QEMU generally uses NULL for 'Error **'. It might be 
> > wrong though.
> > 
> > As for the local error handling, it is following what is described in 
> > qapi/error.h. Isn't it ?
> > 
> 
> Yes, it does follow the "Receive and accumulate multiple errors" 
> recommandation,
> but does it make sense to realize the ICS object if we failed to set
> nr-irqs ?

Nor is it necessary to have two different local error variables.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 5/6] spapr-cpu-core: release ICP object when realization fails

[David Gibson]

On Mon, May 15, 2017 at 01:39:55PM +0200, Greg Kurz wrote:
> While here we introduce a single error path to avoid code duplication.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc=for-2.10.

> ---
>  hw/ppc/spapr_cpu_core.c |   16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4389ef4c2aef..63d160f7e010 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -147,25 +147,25 @@ static void spapr_cpu_core_realize_child(Object *child, 
> Error **errp)
>  object_property_add_const_link(obj, "xics", OBJECT(spapr), _abort);
>  object_property_set_bool(obj, true, "realized", _err);
>  if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> +goto error;
>  }
>  
>  object_property_set_bool(child, true, "realized", _err);
>  if (local_err) {
> -object_unparent(obj);
> -error_propagate(errp, local_err);
> -return;
> +goto error;
>  }
>  
>  spapr_cpu_init(spapr, cpu, _err);
>  if (local_err) {
> -object_unparent(obj);
> -error_propagate(errp, local_err);
> -return;
> +goto error;
>  }
>  
>  xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
> +return;
> +
> +error:
> +object_unparent(obj);
> +error_propagate(errp, local_err);
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 3/6] spapr: fix error reporting in xics_system_init()

[David Gibson]

On Mon, May 15, 2017 at 01:39:36PM +0200, Greg Kurz wrote:
> The xics_system_init() function passes its errp argument to xics_kvm_init().
> If the call fails and the user requested in-kernel irqchip, it then ends up
> passing a NULL Error * to error_reportf_err() and the error message is
> silently dropped.
> 
> Passing an errp argument is generally wrong, unless you really just need
> to convey an error to the caller.
> 
> This patch converts xics_system_init() to *only* pass a pointer to its own
> Error * and then to propagate it with error_propagate(), as recommended
> in error.h.
> 
> The local_err name is used for consistency with the rest of the code.
> 
> Signed-off-by: Greg Kurz 

The change is good, but will need updating to work with the problem in
the previous patch.

> ---
>  hw/ppc/spapr.c |   17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f477d7b8a210..44f7dc7f40e9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState 
> *spapr,
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error 
> **errp)
>  {
>  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +Error *local_err = NULL;
>  
>  if (kvm_enabled()) {
> -Error *err = NULL;
> -
>  if (machine_kernel_irqchip_allowed(machine) &&
> -!xics_kvm_init(spapr, errp)) {
> +!xics_kvm_init(spapr, _err)) {
>  spapr->icp_type = TYPE_KVM_ICP;
> -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> );
> +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> +  _err);
>  }
>  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -error_reportf_err(err,
> +error_reportf_err(local_err,
>"kernel_irqchip requested but unavailable: ");
>  exit(EXIT_FAILURE);
>  } else {
> -error_free(err);
> +error_free(local_err);
>  }
>  }
>  
>  if (!spapr->ics) {
>  xics_spapr_init(spapr);
>  spapr->icp_type = TYPE_ICP;
> -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> +  _err);
>  }
> +
> +error_propagate(errp, local_err);
>  }
>  
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip

[David Gibson]

On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote:
> QEMU should exit if the user explicitely asked for kernel-irqchip support
> and "xics-kvm" initialization fails.
> 
> The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> in xics-kvm creation") reads:
> 
> While there, improve the error message when we can't satisfy an
> explicit user request for "xics-kvm", and exit(1) instead of abort().
> Simplify the abort when we can't create "xics".
> 
> This patch adds the missing call to exit().
> 
> Signed-off-by: Greg Kurz 
> ---
>  hw/ppc/spapr.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index abfb99b71b7d..f477d7b8a210 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int 
> nr_irqs, Error **errp)
>  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>  error_reportf_err(err,
>"kernel_irqchip requested but unavailable: ");
> +exit(EXIT_FAILURE);
>  } else {
>  error_free(err);
>  }
> 


This doesn't look right.  We have an errp pointer in the caller.  So
on failure we should error_propagate(), rather than deciding for
ourselves that exiting is the right course of action.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [virtio-dev] Re: [PATCH] virtio-net: keep the packet layout intact

[Wei Wang]

On 05/16/2017 12:27 PM, Wei Wang wrote:

On 05/15/2017 10:46 PM, Michael S. Tsirkin wrote:

On Mon, May 15, 2017 at 05:29:15PM +0800, Wei Wang wrote:

Ping for comments, thanks.

On 05/11/2017 12:57 PM, Wei Wang wrote:

The current implementation may change the packet layout when
vnet_hdr needs an endianness swap. The layout change causes
one more iov to be added to the iov[] passed from the guest, which
is a barrier to making the TX queue size 1024 due to the possible
off-by-one issue.

It blocks making it 512 but I don't think we can make it 1024
as entries might cross page boundaries and get split.



I agree with the performance lose issue you mentioned
below, thanks. To understand more here, could you please
shed some light on "entries can't cross page boundaries"?

Seems the virtio spec doesn't mention that vring_desc entries
shouldn't be in two physically continuous pages. Also didn't
find an issue from the implementation.
On the device side, the writev manual does 

typo - "does" -> "doesn't"



require the iov[]
array to be in one page only, and the limit to iovcnt is 1024.



This patch changes the implementation to remain the packet layout
intact. In this case, the number of iov[] passed to writev will be
equal to the number obtained from the guest.

Signed-off-by: Wei Wang 

As this is at the cost of a full data copy, I don't think
this makes sense. We could limit this when sg list does not fit
in 1024.


Yes, this would cause a performance loss with the layout that data
is adjacent with vnet_hdr. Since you prefer another solution below,
I will skip and not discuss my ideas to avoid that copy.


But I really think we should just add a max s/g field to virtio
and then we'll be free to increase the ring size.


Yes, that's also a good way to solve it. So, add a new device
property, "max_chain_size" and a feature bit to detect it?




Best,
Wei

Re: [Qemu-devel] [PATCH 1/6] ppc/xics: simplify prototype of xics_spapr_init()

[David Gibson]

On Mon, May 15, 2017 at 01:39:16PM +0200, Greg Kurz wrote:
> This function only does hypercall and RTAS-call registration, and thus
> never returns an error. This patch adapt the prototype to reflect that.
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-2.10, thanks.

> ---
>  hw/intc/xics_spapr.c  |3 +--
>  hw/ppc/spapr.c|2 +-
>  include/hw/ppc/xics.h |2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index f05308b897f2..d98ea8b13068 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -229,7 +229,7 @@ static void rtas_int_on(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
> +void xics_spapr_init(sPAPRMachineState *spapr)
>  {
>  /* Registration of global state belongs into realize */
>  spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
> @@ -243,7 +243,6 @@ int xics_spapr_init(sPAPRMachineState *spapr, Error 
> **errp)
>  spapr_register_hypercall(H_XIRR_X, h_xirr_x);
>  spapr_register_hypercall(H_EOI, h_eoi);
>  spapr_register_hypercall(H_IPOLL, h_ipoll);
> -return 0;
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b7cadab0cdf..abfb99b71b7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -139,7 +139,7 @@ static void xics_system_init(MachineState *machine, int 
> nr_irqs, Error **errp)
>  }
>  
>  if (!spapr->ics) {
> -xics_spapr_init(spapr, errp);
> +xics_spapr_init(spapr);
>  spapr->icp_type = TYPE_ICP;
>  spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
>  }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 05e6acbb3533..d6cb51f3ad5d 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -206,6 +206,6 @@ void icp_resend(ICPState *ss);
>  typedef struct sPAPRMachineState sPAPRMachineState;
>  
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp);
> +void xics_spapr_init(sPAPRMachineState *spapr);
>  
>  #endif /* XICS_H */
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [virtio-dev] Re: [PATCH] virtio-net: keep the packet layout intact

[Wei Wang]

On 05/15/2017 10:46 PM, Michael S. Tsirkin wrote:

On Mon, May 15, 2017 at 05:29:15PM +0800, Wei Wang wrote:

Ping for comments, thanks.

On 05/11/2017 12:57 PM, Wei Wang wrote:

The current implementation may change the packet layout when
vnet_hdr needs an endianness swap. The layout change causes
one more iov to be added to the iov[] passed from the guest, which
is a barrier to making the TX queue size 1024 due to the possible
off-by-one issue.

It blocks making it 512 but I don't think we can make it 1024
as entries might cross page boundaries and get split.



I agree with the performance lose issue you mentioned
below, thanks. To understand more here, could you please
shed some light on "entries can't cross page boundaries"?

Seems the virtio spec doesn't mention that vring_desc entries
shouldn't be in two physically continuous pages. Also didn't
find an issue from the implementation.
On the device side, the writev manual does require the iov[]
array to be in one page only, and the limit to iovcnt is 1024.



This patch changes the implementation to remain the packet layout
intact. In this case, the number of iov[] passed to writev will be
equal to the number obtained from the guest.

Signed-off-by: Wei Wang 

As this is at the cost of a full data copy, I don't think
this makes sense. We could limit this when sg list does not fit
in 1024.


Yes, this would cause a performance loss with the layout that data
is adjacent with vnet_hdr. Since you prefer another solution below,
I will skip and not discuss my ideas to avoid that copy.


But I really think we should just add a max s/g field to virtio
and then we'll be free to increase the ring size.


Yes, that's also a good way to solve it. So, add a new device
property, "max_chain_size" and a feature bit to detect it?


Best,
Wei

Re: [Qemu-devel] [PATCH 1/4] exec: simplify phys_page_find() params

[David Gibson]

On Mon, May 15, 2017 at 04:50:57PM +0800, Peter Xu wrote:
> It really only plays with the dispatchers, so the parameter list does
> not need that complexity. This helps for readability at least.
> 
> Signed-off-by: Peter Xu 

Reviewed-by: David Gibson 

> ---
>  exec.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index eac6085..1d76c63 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -371,10 +371,11 @@ static inline bool section_covers_addr(const 
> MemoryRegionSection *section,
>   int128_getlo(section->size), addr);
>  }
>  
> -static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
> -   Node *nodes, MemoryRegionSection 
> *sections)
> +static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr 
> addr)
>  {
> -PhysPageEntry *p;
> +PhysPageEntry lp = d->phys_map, *p;
> +Node *nodes = d->map.nodes;
> +MemoryRegionSection *sections = d->map.sections;
>  hwaddr index = addr >> TARGET_PAGE_BITS;
>  int i;
>  
> @@ -412,8 +413,7 @@ static MemoryRegionSection 
> *address_space_lookup_region(AddressSpaceDispatch *d,
>  section_covers_addr(section, addr)) {
>  update = false;
>  } else {
> -section = phys_page_find(d->phys_map, addr, d->map.nodes,
> - d->map.sections);
> +section = phys_page_find(d, addr);
>  update = true;
>  }
>  if (resolve_subpage && section->mr->subpage) {
> @@ -1246,8 +1246,7 @@ static void register_subpage(AddressSpaceDispatch *d, 
> MemoryRegionSection *secti
>  subpage_t *subpage;
>  hwaddr base = section->offset_within_address_space
>  & TARGET_PAGE_MASK;
> -MemoryRegionSection *existing = phys_page_find(d->phys_map, base,
> -   d->map.nodes, 
> d->map.sections);
> +MemoryRegionSection *existing = phys_page_find(d, base);
>  MemoryRegionSection subsection = {
>  .offset_within_address_space = base,
>  .size = int128_make64(TARGET_PAGE_SIZE),

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] target/ppc: reset reservation in do_rfi()

[David Gibson]

On Mon, May 15, 2017 at 02:05:09PM +0530, Nikunj A Dadhania wrote:
> For transitioning back to userspace after the interrupt.
> 
> Suggested-by: Richard Henderson 
> Signed-off-by: Nikunj A Dadhania 

Applied to ppc-for-2.10. thanks.

> ---
>  target/ppc/excp_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index a6bcb47..9cb2123 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -995,6 +995,9 @@ static inline void do_rfi(CPUPPCState *env, target_ulong 
> nip, target_ulong msr)
>   */
>  cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>  
> +/* Reset the reservation */
> +env->reserve_addr = -1;
> +
>  /* Context synchronizing: check if TCG TLB needs flush */
>  check_tlb_flush(env, false);
>  }

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v10] migration: spapr: migrate pending_events of spapr state

[David Gibson]

On Mon, May 15, 2017 at 10:10:52AM -0300, Daniel Henrique Barboza wrote:
> From: Jianjun Duan 
> 
> In racing situations between hotplug events and migration operation,
> a rtas hotplug event could have not yet be delivered to the source
> guest when migration is started. In this case the pending_events of
> spapr state need be transmitted to the target so that the hotplug
> event can be finished on the target.
> 
> All the different fields of the events are encoded as defined by
> PAPR. We can migrate them as uint8_t binary stream without any
> concerns about data padding or endianess.
> 
> pending_events is put in a subsection in the spapr state VMSD to make
> sure migration across different versions is not broken.
> 
> Signed-off-by: Jianjun Duan 
> Signed-off-by: Daniel Henrique Barboza 

Ok, thanks for splitting this out, but you don't seem to have
addressed the other comments I had on this patch as presented before.

> ---
>  hw/ppc/spapr.c | 33 +
>  hw/ppc/spapr_events.c  | 24 +---
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d0..8cfdc71 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1437,6 +1437,38 @@ static bool version_before_3(void *opaque, int 
> version_id)
>  return version_id < 3;
>  }
>  
> +static bool spapr_pending_events_needed(void *opaque)
> +{
> +sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +return !QTAILQ_EMPTY(>pending_events);
> +}
> +
> +static const VMStateDescription vmstate_spapr_event_entry = {
> +.name = "spapr_event_log_entry",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_INT32(log_type, sPAPREventLogEntry),
> +VMSTATE_BOOL(exception, sPAPREventLogEntry),

I'd like some more information to convince me there isn't redundant
data here.

> +VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> +VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
> +0, vmstate_info_uint8, uint8_t),

And I think this should be VBUFFER rather than VARRAY to avoid the
data type change.

> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> +static const VMStateDescription vmstate_spapr_pending_events = {
> +.name = "spapr_pending_events",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_pending_events_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
> + vmstate_spapr_event_entry, sPAPREventLogEntry, 
> next),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
>  static bool spapr_ov5_cas_needed(void *opaque)
>  {
>  sPAPRMachineState *spapr = opaque;
> @@ -1535,6 +1567,7 @@ static const VMStateDescription vmstate_spapr = {
>  .subsections = (const VMStateDescription*[]) {
>  _spapr_ov5_cas,
>  _spapr_patb_entry,
> +_spapr_pending_events,
>  NULL
>  }
>  };
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index f0b28d8..70c7cfc 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -342,7 +342,8 @@ static int rtas_event_log_to_irq(sPAPRMachineState 
> *spapr, int log_type)
>  return source->irq;
>  }
>  
> -static void rtas_event_log_queue(int log_type, void *data, bool exception)
> +static void rtas_event_log_queue(int log_type, void *data, bool exception,
 + int data_size)

This could derive data_size from the header passed in.

>  {
>  sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
> @@ -351,6 +352,7 @@ static void rtas_event_log_queue(int log_type, void 
> *data, bool exception)
>  entry->log_type = log_type;
>  entry->exception = exception;
>  entry->data = data;
> +entry->data_size = data_size;
>  QTAILQ_INSERT_TAIL(>pending_events, entry, next);
>  }
>  
> @@ -445,6 +447,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>  struct rtas_event_log_v6_mainb *mainb;
>  struct rtas_event_log_v6_epow *epow;
>  struct epow_log_full *new_epow;
> +uint32_t data_size;
>  
>  new_epow = g_malloc0(sizeof(*new_epow));
>  hdr = _epow->hdr;
> @@ -453,14 +456,13 @@ static void spapr_powerdown_req(Notifier *n, void 
> *opaque)
>  mainb = _epow->mainb;
>  epow = _epow->epow;
>  
> +data_size = sizeof(*new_epow);
>  hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
> | RTAS_LOG_SEVERITY_EVENT
> | RTAS_LOG_DISPOSITION_NOT_RECOVERED
> | RTAS_LOG_OPTIONAL_PART_PRESENT
> | RTAS_LOG_TYPE_EPOW);
> 

Re: [Qemu-devel] [PATCH v18 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

Hi Halil,

Sorry for delay because I'm busy on inner production project recently.

> 
> 
> START HERE
> 
> > +The device can set the operation status as follows: VIRTIO_CRYPTO_OK:
> success;
> > +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP:
> not supported;
> > +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto
> operations.
> 
> You describe everything but BADMSG. Could you be a bit more specific
> about the differences between _ERR _BADMSG and _NOTSUPP? Is for instance
> trying
> to do something with a not-advertised service or algo a _NOTSUPP or a
> _BADMSG or just a generic _ERR? Same qestion goes for different sorts of
> out of resources.
> 
Sure. Will do.

> > +
> > +\begin{lstlisting}
> > +enum VIRTIO_CRYPTO_STATUS {
> > +VIRTIO_CRYPTO_OK = 0,
> > +VIRTIO_CRYPTO_ERR = 1,
> > +VIRTIO_CRYPTO_BADMSG = 2,
> > +VIRTIO_CRYPTO_NOTSUPP = 3,
> > +VIRTIO_CRYPTO_INVSESS = 4,
> > +VIRTIO_CRYPTO_MAX
> > +};
> > +\end{lstlisting}
> > +
> 
> There are no more mentions of the values in the spec, so the description
> above should be real good.
> 
Agree.

> > +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device /
> Device Operation / Control Virtqueue}
> > +
> > +The driver uses the control virtqueue to send control commands to the
> > +device, such as session operations (See \ref{sec:Device Types / Crypto
> Device / Device Operation / Control Virtqueue / Session operation}).
> > +
> > +Controlq requests are as follows:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_op_ctrl_req {
> > +struct virtio_crypto_ctrl_header header;
> > +
> > +union {
> > +struct virtio_crypto_sym_create_session_req
> sym_create_session;
> > +struct virtio_crypto_hash_create_session_req
> hash_create_session;
> > +struct virtio_crypto_mac_create_session_req
> mac_create_session;
> > +struct virtio_crypto_aead_create_session_req
> aead_create_session;
> > +struct virtio_crypto_destroy_session_req  destroy_session;
> > +} u;
> > +};
> > +\end{lstlisting}
> > +
> > +struct virtio_crypto_op_ctrl_req is the only allowed control request.
> > +The header is the general header, and the union is of the 
> > algorithm-specific
> type,
> > +which is set by the driver. All the properties in the union are shown as
> follows.
> > +
> > +\paragraph{Session operation}\label{sec:Device Types / Crypto Device /
> Device Operation / Control Virtqueue / Session operation}
> > +
> > +The symmetric algorithms involve the concept of sessions. A session is a
> > +handle which describes the cryptographic parameters to be applied to
> > +a number of buffers.
> 
> 8<
> > The data within a session handle includes:
> > +
> > +\begin{enumerate}
> > +\item The operation (CIPHER, HASH/MAC or both, and if both, the order in
> > +  which the algorithms should be applied).
> > +\item The CIPHER set data, including the CIPHER algorithm and mode,
> > +  the key and its length, and the direction (encryption or decryption).
> > +\item The HASH/MAC set data, including the HASH algorithm or MAC
> algorithm,
> > +  and hash result length (to allow for truncation).
> > +\begin{itemize*}
> > +\item Authenticated mode can refer to MAC, which requires that the key
> and
> > +  its length are also specified.
> > +\item For nested mode, the inner and outer prefix data and length are
> specified,
> > +  as well as the outer HASH algorithm.
> > +\end{itemize*}
> > +\end{enumerate}
> > +
> >8
> 
> This part is slightly confusing for me. I guess you are trying to describe
> what data can live in the session context (considering all the different
> applications). I think we do not have to describe that here, because we have
> to describe the individual session operations, and there we must describe
> the precise impact of these operations (and their parameters).
> 
Make sense to me.

> > +The following structure stores the result of session creation set by the
> device:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_session_input {
> > +/* Device-writable part */
> > +le64 session_id;
> > +le32 status;
> > +le32 padding;
> > +};
> > +\end{lstlisting}
> > +
> > +A request to destroy a session includes the following information:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_destroy_session_req {
> > +/* Device-readable part */
> > +le64  session_id;
> > +/* Device-writable part */
> > +le32  status;
> > +le32  padding;
> > +};
> > +\end{lstlisting}
> > +
> > +\subparagraph{Session operation: HASH session}\label{sec:Device Types /
> Crypto Device / Device
> > +Operation / Control Virtqueue / Session operation / Session operation: HASH
> session}
> > +
> > +HASH session requests are as follows:
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_hash_session_para {
> > +/* See VIRTIO_CRYPTO_HASH_* above */
> > +le32 algo;
> > +/* hash result length */
> > +le32 

Re: [Qemu-devel] [PATCH v2] migration: Move check_migratable() into qdev.c

[Peter Xu]

On Fri, May 12, 2017 at 05:54:36PM +0200, Juan Quintela wrote:
> The function is only used once, and nothing else in migration knows
> about objects.  Create the function vmstate_device_is_migratable() in
> savem.c that really do the bit that is related with migration.

(may need to touch up the commit message as well, since we don't have
 vmstate_device_is_migratable() now)

> 
> Signed-off-by: Juan Quintela 

For the build error for linux-user on previous v1, not sure defining
one only_migratable in stubs/vmstate.c can work. Anyway, I think
current patch is okay as well (considering that our goal is to cleanup
the header "#include"s). So, best with the commit message touched up:

Reviewed-by: Peter Xu 

Thanks,

> ---
> 
> - move only_migrateble use back to savevm.c to fix linux-user compilation.
> 
> 
>  hw/core/qdev.c| 20 
>  include/migration/migration.h |  6 --
>  include/migration/vmstate.h   |  2 ++
>  include/sysemu/sysemu.h   |  2 +-
>  migration/migration.c | 15 ---
>  migration/savevm.c| 10 ++
>  stubs/vmstate.c   |  5 ++---
>  7 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 02b632f..6f1b070 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -37,7 +37,7 @@
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  #include "qapi-event.h"
> -#include "migration/migration.h"
> +#include "migration/vmstate.h"
>  
>  bool qdev_hotplug = false;
>  static bool qdev_hot_added = false;
> @@ -861,6 +861,20 @@ static bool device_get_realized(Object *obj, Error 
> **errp)
>  return dev->realized;
>  }
>  
> +static bool check_only_migratable(Object *obj, Error **err)
> +{
> +DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +
> +if (!vmstate_check_only_migratable(dc->vmsd)) {
> +error_setg(err, "Device %s is not migratable, but "
> +   "--only-migratable was specified",
> +   object_get_typename(obj));
> +return false;
> +}
> +
> +return true;
> +}
> +
>  static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>  DeviceState *dev = DEVICE(obj);
> @@ -870,7 +884,6 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  Error *local_err = NULL;
>  bool unattached_parent = false;
>  static int unattached_count;
> -int ret;
>  
>  if (dev->hotplugged && !dc->hotpluggable) {
>  error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> @@ -878,8 +891,7 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  }
>  
>  if (value && !dev->realized) {
> -ret = check_migratable(obj, _err);
> -if (ret < 0) {
> +if (!check_only_migratable(obj, _err)) {
>  goto fail;
>  }
>  
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 1e23536..7ce3a7e 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -22,7 +22,6 @@
>  #include "qapi-types.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/coroutine_int.h"
> -#include "qom/object.h"
>  
>  #define QEMU_VM_FILE_MAGIC   0x5145564d
>  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> @@ -39,9 +38,6 @@
>  #define QEMU_VM_COMMAND  0x08
>  #define QEMU_VM_SECTION_FOOTER   0x7e
>  
> -/* for vl.c */
> -extern int only_migratable;
> -
>  /* Messages sent on the return path from destination to source */
>  enum mig_rp_message_type {
>  MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
> @@ -268,8 +264,6 @@ int ram_discard_range(const char *block_name, uint64_t 
> start, size_t length);
>  int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>  void ram_postcopy_migrated_memory_release(MigrationState *ms);
>  
> -int check_migratable(Object *obj, Error **err);
> -
>  bool migrate_release_ram(void);
>  bool migrate_postcopy_ram(void);
>  bool migrate_zero_blocks(void);
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 81af5fb..dacb052 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1066,4 +1066,6 @@ int64_t self_announce_delay(int round)
>  
>  void dump_vmstate_json_to_file(FILE *out_fp);
>  
> +bool vmstate_check_only_migratable(const VMStateDescription *vmsd);
> +
>  #endif
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 3340202..83ecd13 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -15,7 +15,7 @@
>  /* vl.c */
>  
>  extern const char *bios_name;
> -
> +extern int only_migratable;
>  extern const char *qemu_name;
>  extern QemuUUID qemu_uuid;
>  extern bool qemu_uuid_set;
> diff --git a/migration/migration.c b/migration/migration.c
> index a6a9922..55c3958 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1162,21 +1162,6 @@ 

Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

> 
> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
> >
> >> From: Halil Pasic [mailto:pa...@linux.vnet.ibm.com]
> >> Sent: Friday, May 12, 2017 7:02 PM
> >>
> >>
> >> On 05/08/2017 01:38 PM, Gonglei wrote:
> >>> According to the new spec, we should use different
> >>> requst structure to store the data request based
> >>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> >>> negotiated or not.
> >>>
> >>> In this patch, we havn't supported stateless mode
> >>> yet. The device reportes an error if both
> >>> VIRTIO_CRYPTO_F_MUX_MODE and
> >> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> >>> are negotiated, meanwhile the header.flag doesn't set
> >>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> >>>
> >>> Let's handle this scenario in the following patches.
> >>>
> >>> Signed-off-by: Gonglei 
> >>> ---
> >>>  hw/virtio/virtio-crypto.c | 83
> >> ---
> >>>  1 file changed, 71 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> >>> index 0353eb6..c4b8a2c 100644
> >>> --- a/hw/virtio/virtio-crypto.c
> >>> +++ b/hw/virtio/virtio-crypto.c
> >>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >> *request)
> >>>  VirtQueueElement *elem = >elem;
> >>>  int queue_index =
> >> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> >>>  struct virtio_crypto_op_data_req req;
> >>> +struct virtio_crypto_op_data_req_mux req_mux;
> >>>  int ret;
> >>>  struct iovec *in_iov;
> >>>  struct iovec *out_iov;
> >>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >> *request)
> >>>  uint64_t session_id;
> >>>  CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> >>>  Error *local_err = NULL;
> >>> +bool mux_mode_is_negotiated;
> >>> +struct virtio_crypto_op_header *header;
> >>> +bool is_stateless_req = false;
> >>>
> >>>  if (elem->out_num < 1 || elem->in_num < 1) {
> >>>  virtio_error(vdev, "virtio-crypto dataq missing headers");
> >>> @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> >> *request)
> >>>  out_iov = elem->out_sg;
> >>>  in_num = elem->in_num;
> >>>  in_iov = elem->in_sg;
> >>> -if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(req))
> >>> -!= sizeof(req))) {
> >>> -virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>> -return -1;
> >>> +
> >>> +mux_mode_is_negotiated =
> >>> +virtio_vdev_has_feature(vdev,
> VIRTIO_CRYPTO_F_MUX_MODE);
> >>> +if (!mux_mode_is_negotiated) {
> >>> +if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(req))
> >>> +!= sizeof(req))) {
> >>> +virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>> +return -1;
> >>> +}
> >>> +iov_discard_front(_iov, _num, sizeof(req));
> >>> +
> >>> +header = 
> >>> +} else {
> >>> +if (unlikely(iov_to_buf(out_iov, out_num, 0, _mux,
> >>> +sizeof(req_mux)) != sizeof(req_mux))) {
> >>> +virtio_error(vdev, "virtio-crypto request outhdr too short");
> >>> +return -1;
> >>> +}
> >>> +iov_discard_front(_iov, _num, sizeof(req_mux));
> >>> +
> >>> +header = _mux.header;
> >>
> >> I wonder if this request length checking logic is conform to the
> >> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
> >> virtio crypto device specification").
> >>
> > Sure. Please see below normative formulation:
> >
> > '''
> > \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types
> / Crypto Device / Device Operation / Symmetric algorithms Operation}
> > ...
> > \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the
> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
> requests.
> > Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> > ...
> > '''
> >
> 
> As far as I can remember, we have already agreed that in terms of the
> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your

Sorry, I don't think so. :(

> code you have a substantially different struct virtio_crypto_op_data_req
> than in your spec! For instance in the spec virtio_crypto_op_data_req is
> the full request and contains the data buffers (src_data and the
> dest_data), while in your code it's effectively just a header and does
> not contain any data buffers.
> 
I said struct virtio_crypto_op_data_req in the spec is just a symbol.
I didn't find a better way to express the src_data and dst_data etc. So
I used u8[len] xxx_data to occupy a sit in the request.

> 
> >> AFAIU here you allow only requests of two sizes: one fixed size
> >> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
> >> means that some requests need quite some padding between what
> >> you call the 'request' and the actual data on which the crypto
> >> 

Re: [Qemu-devel] [PATCH] block: Use QDict helpers for --force-share

[Fam Zheng]

On Mon, 05/15 14:54, Eric Blake wrote:
> Fam's addition of --force-share in commits 459571f7 and 335e9937
> were developed prior to the addition of QDict scalar insertion
> macros, but merged after the general cleanup in commit 46f5ac20.
> Patch created mechanically by rerunning:
> 
>  spatch --sp-file scripts/coccinelle/qobject.cocci \
> --macro-file scripts/cocci-macro-file.h --dir . --in-place
> 
> Signed-off-by: Eric Blake 
> ---
> 
> This could go in through Markus' qapi tree (since he took the
> original cleanup), a block maintainer (since it only touches block
> files), or even the trivial patch queue (since it's so small and
> mechanical) - I don't care which, as long as we don't drop it!
> 
>  qemu-img.c | 7 +++
>  qemu-io.c  | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b506839..60f1784 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -296,7 +296,7 @@ static BlockBackend *img_open_opts(const char *optstr,
>  error_report("--force-share/-U conflicts with image options");
>  return NULL;
>  }
> -qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
> +qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
>  }
>  blk = blk_new_open(NULL, NULL, options, flags, _err);
>  if (!blk) {
> @@ -326,7 +326,7 @@ static BlockBackend *img_open_file(const char *filename,
>  }
> 
>  if (force_share) {
> -qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
> +qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
>  }
>  blk = blk_new_open(filename, NULL, options, flags, _err);
>  if (!blk) {
> @@ -3156,8 +3156,7 @@ static int img_rebase(int argc, char **argv)
>  if (!options) {
>  options = qdict_new();
>  }
> -qdict_put(options, BDRV_OPT_FORCE_SHARE,
> -  qbool_from_bool(true));
> +qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
>  }
>  bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>  blk_old_backing = blk_new_open(backing_name, NULL,
> diff --git a/qemu-io.c b/qemu-io.c
> index 34fa8a1..8e38b28 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -76,7 +76,7 @@ static int openfile(char *name, int flags, bool 
> writethrough, bool force_share,
>  QDECREF(opts);
>  return 1;
>  }
> -qdict_put(opts, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
> +qdict_put_bool(opts, BDRV_OPT_FORCE_SHARE, true);
>  }
>  qemuio_blk = blk_new_open(name, NULL, opts, flags, _err);
>  if (!qemuio_blk) {
> -- 
> 2.9.4
> 
> 

Reviewed-by: Fam Zheng 

[Qemu-devel] [PATCH] SMM: disable smram region if smm is disabled

[Anthony Xu]

when smm is disabled, smram is not used, so disable it

Signed-off-by: Anthony Xu 
---
 hw/pci-host/piix.c | 45 +++--
 hw/pci-host/q35.c  | 83 +-
 kvm-all.c  |  3 +-
 target/i386/kvm.c  |  2 +-
 4 files changed, 70 insertions(+), 63 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index bf4221d..ce43f87 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -142,10 +142,12 @@ static void i440fx_update_memory_mappings(PCII440FXState 
*d)
 pam_update(>pam_regions[i], i,
pd->config[I440FX_PAM + ((i + 1) / 2)]);
 }
-memory_region_set_enabled(>smram_region,
-  !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
-memory_region_set_enabled(>smram,
-  pd->config[I440FX_SMRAM] & SMRAM_G_SMRAME);
+if (pc_machine_is_smm_enabled(PC_MACHINE(current_machine))) {
+memory_region_set_enabled(>smram_region,
+!(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
+memory_region_set_enabled(>smram,
+pd->config[I440FX_SMRAM] & SMRAM_G_SMRAME);
+}
 memory_region_transaction_commit();
 }
 
@@ -355,23 +357,24 @@ PCIBus *i440fx_init(const char *host_type, const char 
*pci_type,
 pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
f->pci_address_space);
 
-/* if *disabled* show SMRAM to all CPUs */
-memory_region_init_alias(>smram_region, OBJECT(d), "smram-region",
- f->pci_address_space, 0xa, 0x2);
-memory_region_add_subregion_overlap(f->system_memory, 0xa,
->smram_region, 1);
-memory_region_set_enabled(>smram_region, true);
-
-/* smram, as seen by SMM CPUs */
-memory_region_init(>smram, OBJECT(d), "smram", 1ull << 32);
-memory_region_set_enabled(>smram, true);
-memory_region_init_alias(>low_smram, OBJECT(d), "smram-low",
- f->ram_memory, 0xa, 0x2);
-memory_region_set_enabled(>low_smram, true);
-memory_region_add_subregion(>smram, 0xa, >low_smram);
-object_property_add_const_link(qdev_get_machine(), "smram",
-   OBJECT(>smram), _abort);
-
+if (pc_machine_is_smm_enabled(PC_MACHINE(current_machine))) {
+/* if *disabled* show SMRAM to all CPUs */
+memory_region_init_alias(>smram_region, OBJECT(d), "smram-region",
+f->pci_address_space, 0xa, 0x2);
+memory_region_add_subregion_overlap(f->system_memory, 0xa,
+>smram_region, 1);
+memory_region_set_enabled(>smram_region, true);
+
+/* smram, as seen by SMM CPUs */
+memory_region_init(>smram, OBJECT(d), "smram", 1ull << 32);
+memory_region_set_enabled(>smram, true);
+memory_region_init_alias(>low_smram, OBJECT(d), "smram-low",
+f->ram_memory, 0xa, 0x2);
+memory_region_set_enabled(>low_smram, true);
+memory_region_add_subregion(>smram, 0xa, >low_smram);
+object_property_add_const_link(qdev_get_machine(), "smram",
+OBJECT(>smram), _abort);
+}
 init_pam(dev, f->ram_memory, f->system_memory, f->pci_address_space,
  >pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
 for (i = 0; i < 12; ++i) {
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 344f77b..a10d79e 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -324,6 +324,9 @@ static void mch_update_pam(MCHPCIState *mch)
 /* SMRAM */
 static void mch_update_smram(MCHPCIState *mch)
 {
+if (!pc_machine_is_smm_enabled(PC_MACHINE(current_machine))) {
+return;
+}
 PCIDevice *pd = PCI_DEVICE(mch);
 bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & 
MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME);
 uint32_t tseg_size;
@@ -469,46 +472,48 @@ static void mch_realize(PCIDevice *d, Error **errp)
 pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
mch->pci_address_space);
 
-/* if *disabled* show SMRAM to all CPUs */
-memory_region_init_alias(>smram_region, OBJECT(mch), "smram-region",
- mch->pci_address_space, 0xa, 0x2);
-memory_region_add_subregion_overlap(mch->system_memory, 0xa,
->smram_region, 1);
-memory_region_set_enabled(>smram_region, true);
-
-memory_region_init_alias(>open_high_smram, OBJECT(mch), 
"smram-open-high",
- mch->ram_memory, 0xa, 0x2);
-memory_region_add_subregion_overlap(mch->system_memory, 0xfeda,
->open_high_smram, 1);
-memory_region_set_enabled(>open_high_smram, false);
-
-/* smram, as seen by SMM CPUs */
-memory_region_init(>smram, OBJECT(mch), "smram", 1ull << 32);
-

Re: [Qemu-devel] Installing Windows with virtio (was: QEMU, increase graphics memory)

[jenia mtl]

Hello Fam.


I managed to installed Windows with the *Virtio* drivers. Thanks for your
tip.

It has made a change in that I got 2X more graphics memory. But not nearly
enough. I need 512 and now I have 16.

How do I specify to Virtio how much memory to allocate?

Thanks
Jenia

On Sun, May 14, 2017 at 9:36 PM, Fam Zheng  wrote:

> On Sun, 05/14 19:39, jenia.ivlev wrote:
> >
> > I wanted to get more graphics memory on my QEMU Windows client.
> >
> > I decided to install **Virtio** drivers for QEMU to achieve that purpose.
> >
> > I create an *imagine_file* like this:
> >
> >
> > qemu-img create -f raw image_file 4G
> >
> >
> > Also, I ran the windows-install like this:
> >
> >
> > qemu-system-x86_64 -enable-kvm -m 4G -cdrom "OS.iso" -boot order=c
> -drive file=image_file,if=virtio
> >
> >
> > But when Windows ran, it didn't find any hard-drive (the *image_file*
> > basically). It asked for some drivers instead. I think it wanted drivers
> > to communicate with the hard-drive (*image_file*).
> >
> > Running the install without Virtio works though:
> >
> >
> > qemu-system-x86_64 -enable-kvm -m 4G -cdrom "OS.iso" -boot order=d
> -drive file=image_file,format=raw
> >
> >
> > The difference betweeen the two commands is:
> >
> >
> > order=c vs order=d
> >
> >
> > and
> >
> >
> > if=virtio vs format=raw
> >
> >
> > How do I install windows with Virtio in QEMU?
> >
> > My original goal is to get 512 MB of graphics memory on Windows (running
> > as a guest in QEMU). Unfortunately, by default  (no Virtio) I get 8MB of
> > video memory which is not enough for my purposes
> >
> >
> > Thanks
> >
> > P.S. My OS is Arch-Linux
>
> Since you use ArchLinux, wiki.archlinux.org is rather helpful, as
> always.  Yes,
> you need virtio drivers:
>
> https://wiki.archlinux.org/index.php/QEMU#New_Install_of_Windows
>
>
> Fam
>

[Qemu-devel] [PATCH] coccinelle: fix typo in comment

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/coccinelle/return_directly.cocci | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/coccinelle/return_directly.cocci 
b/scripts/coccinelle/return_directly.cocci
index 48680f2c2a..4cf50e75ea 100644
--- a/scripts/coccinelle/return_directly.cocci
+++ b/scripts/coccinelle/return_directly.cocci
@@ -1,4 +1,4 @@
-// replace 'R = X; return R;' with 'return R;'
+// replace 'R = X; return R;' with 'return X;'
 @@
 identifier VAR;
 expression E;
-- 
2.11.0

[Qemu-devel] [PATCH] oslib: strip trailing '\n' from error_setg() string argument

[Philippe Mathieu-Daudé]

spotted by Coccinelle script scripts/coccinelle/err-bad-newline.cocci

Signed-off-by: Philippe Mathieu-Daudé 
---
 util/oslib-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 4d9189e9ef..7ca02f0103 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -447,7 +447,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int 
smp_cpus,
 /* touch pages simultaneously */
 if (touch_all_pages(area, hpagesize, numpages, smp_cpus)) {
 error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
-"pages available to allocate guest RAM\n");
+"pages available to allocate guest RAM");
 }
 
 ret = sigaction(SIGBUS, , NULL);
-- 
2.11.0

Re: [Qemu-devel] [Qemu-arm] Changes to Broadcom(BCM) files and Raspberry Pi files. Addition of PanelEmu

[John Bradley via Qemu-devel]

Hi,
The XML files in the base are not in the patch. They where net beans files. I 
can easily get it into 3 files, one large at 91KB but contains only new files 
and so is easy to read. Could be smaller but seems pointless. 

another which is about 19KB of quite simple changes to mostly make files.
Then one 15KB which quite straight forward when you look at it. Applying them 
in that order should allow step wise addition. I've uploaded them to a share if 
anyone wants to comment.

The 3 files are new.patch
  
|  
|   
|   
|   ||

   |

  |
|  
||  
new.patch
 Shared with Dropbox  |   |

  |

  |

 
 mod1.patch

  
|  
|   
|   
|   ||

   |

  |
|  
||  
mod1.patch
 Shared with Dropbox  |   |

  |

  |

 

and 
 a_hw_gpio_bcm2835_gpio.c.patch
  
|  
|   
|   
|   ||

   |

  |
|  
||  
a_hw_gpio_bcm2835_gpio.c.patch
 Shared with Dropbox  |   |

  |

  |

 



John BradleyTel: 07896 839635Skype: flypie125 125B Grove StreetEdge Hill 
Liverpool L7 7AF 

On Tuesday, 16 May 2017, 0:20, Philippe Mathieu-Daudé  
wrote:
 

 Hi John,

> That is going to be very difficult as a lot of the changes are
> interlinked the vast majority of the patch is new files.

I rebased your branch on latest qemu/master here:

https://github.com/philmd/qemu/tree/flypie-GDummyPanel-rebased

It is much easier to follow now, the big XML files you added/removed 
also disappeared (gitk was crashing 'Out Of Memory' trying to look at 
your tree).

I hope it can help you to continue reordering in smaller patches.

Regards,

Phil.

On 05/15/2017 01:46 PM, Alistair Francis wrote:
>
> Hey John,
>
> Thanks for the patch!
>
> Unfortunately this patch is too long to review, you need to split the
> patch up into shorter more readable patches. Otherwise it's too hard
> to people to understand what you are changing and why.
>
> There are some details here:
> http://wiki.qemu.org/Contribute/SubmitAPatch
> about how to split up
> patches. Each patch applied in order shouldn't break any compilation
> or runtime. Generally the flow is to add the logic in earlier patches
> and then connect it and switch it on in the later patches.
>
> Try splitting up adding/editing each individual device and send that
> our first. That is generally the easiest to review/accept.
>
> Thanks,
>
> Alistair
>
>> ---
>> .gitignore                          |  54 ++
>> hw/arm/Makefile.objs                |    2 +-
>> hw/arm/bcm2835.c                    |  114 
>> hw/arm/bcm2835_peripherals.c        |  104 
>> hw/arm/bcm2836.c                    |    3 +-
>> hw/arm/raspi.c                      |  77 ++-
>> hw/gpio/bcm2835_gpio.c              |  333 ++-
>> hw/misc/Makefile.objs                |    2 +
>> hw/misc/bcm2835_mphi.c              |  163 ++
>> hw/misc/bcm2835_power.c              |  106 
>> hw/timer/Makefile.objs              |    2 +
>> hw/timer/bcm2835_st.c                |  202 +++
>> hw/timer/bcm2835_timer.c            |  224 +++
>> hw/usb/Makefile.objs                |    4 +-
>> hw/usb/bcm2835_usb.c                |  604 +++
>> hw/usb/bcm2835_usb_regs.h            | 1061
> ++
>> hw/usb/dev-network.c                |    2 +-
>> include/hw/arm/bcm2835.h            |  37 ++
>> include/hw/arm/bcm2835_peripherals.h |  10 +
>> include/hw/gpio/bcm2835_gpio.h      |    5 +
>> include/hw/intc/bcm2835_control.h    |  53 ++
>> include/hw/intc/bcm2836_control.h    |    2 +
>> include/hw/misc/bcm2835_mphi.h      |  28 +
>> include/hw/misc/bcm2835_power.h      |  22 +
>> include/hw/timer/bcm2835_st.h        |  25 +
>> include/hw/timer/bcm2835_timer.h    |  32 +
>> include/hw/usb/bcm2835_usb.h        |  78 +++
>> include/qemu/PanelEmu.h              |  53 ++
>> util/Makefile.objs                  |    1 +
>> util/PanelEmu.c                      |  293 ++
>> 30 files changed, 3547 insertions(+), 149 deletions(-)
>> create mode 100644 hw/arm/bcm2835.c
>> create mode 100644 hw/misc/bcm2835_mphi.c
>> create mode 100644 hw/misc/bcm2835_power.c
>> create mode 100644 hw/timer/bcm2835_st.c
>> create mode 100644 hw/timer/bcm2835_timer.c
>> create mode 100644 hw/usb/bcm2835_usb.c
>> create mode 100644 hw/usb/bcm2835_usb_regs.h
>> create mode 100644 include/hw/arm/bcm2835.h
>> create mode 100644 include/hw/intc/bcm2835_control.h
>> create mode 100644 include/hw/misc/bcm2835_mphi.h
>> create mode 100644 include/hw/misc/bcm2835_power.h
>> create mode 100644 include/hw/timer/bcm2835_st.h
>> create mode 100644 include/hw/timer/bcm2835_timer.h
>> create mode 100644 include/hw/usb/bcm2835_usb.h
>> create mode 100644 include/qemu/PanelEmu.h
>> create mode 100644 util/PanelEmu.c
>>
>
>


   

Re: [Qemu-devel] [PATCH v4 5/6] target/ppc: optimize various functions using extract op

[Philippe Mathieu-Daudé]

Hi David,

On 05/15/2017 01:12 AM, David Gibson wrote:

On Fri, May 12, 2017 at 08:38:42PM -0300, Philippe Mathieu-Daudé wrote:

Patch created mechanically using Coccinelle script via:

$ spatch --macro-file scripts/cocci-macro-file.h --in-place \
--sp-file scripts/coccinelle/tcg_gen_extract.cocci --dir target

Signed-off-by: Philippe Mathieu-Daudé 


Acked-by: David Gibson 

Do you want me to merge this via my ppc tree, or is the whole set
going in via some other path?


Thank for the review!

As you wish, I think it makes sens this serie goes altogether via 
Richard's tree, once I finish correcting few details.


Regards,

Phil.

Re: [Qemu-devel] [PATCH] configure: Remove -lxencall for Xen detection

[Stefano Stabellini]

On Thu, 11 May 2017, Anthony PERARD wrote:
> QEMU does not depends on libxencall, it was added because it was a
> missing link dependency of libxendevicemodel, but now the later should
> be built properly.
> 
> Signed-off-by: Anthony PERARD 

Reviewed-by: Stefano Stabellini 


> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 7c020c076b..fa3522d18a 100755
> --- a/configure
> +++ b/configure
> @@ -2014,7 +2014,7 @@ if test "$xen" != "no" ; then
>else
>  
>  xen_libs="-lxenstore -lxenctrl -lxenguest"
> -xen_stable_libs="-lxencall -lxenforeignmemory -lxengnttab -lxenevtchn"
> +xen_stable_libs="-lxenforeignmemory -lxengnttab -lxenevtchn"
>  
>  # First we test whether Xen headers and libraries are available.
>  # If no, we are done and there is no Xen support.
> -- 
> Anthony PERARD
> 

Re: [Qemu-devel] [PATCH v2 05/16] linux-user: fix argument type declaration of rt_sigqueinfo() syscall

[Philippe Mathieu-Daudé]

On 05/15/2017 11:59 AM, Miloš Stojanović wrote:

Change the type of the first argument of rt_sigqueinfo() from int to pid_t
in the syscall declaration to match specifications of the system call.

Proper spacing is added to satisfy checkpatch.pl.

Signed-off-by: Miloš Stojanović 


Reviewed-by: Philippe Mathieu-Daudé 


---
 linux-user/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ff03e1a..9ec7ccd 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -274,7 +274,7 @@ _syscall3(int, sys_getdents64, uint, fd, struct 
linux_dirent64 *, dirp, uint, co
 _syscall5(int, _llseek,  uint,  fd, ulong, hi, ulong, lo,
   loff_t *, res, uint, wh);
 #endif
-_syscall3(int,sys_rt_sigqueueinfo,int,pid,int,sig,siginfo_t *,uinfo)
+_syscall3(int, sys_rt_sigqueueinfo, pid_t, pid, int, sig, siginfo_t *, uinfo)
 _syscall3(int,sys_syslog,int,type,char*,bufp,int,len)
 #ifdef __NR_exit_group
 _syscall1(int,exit_group,int,error_code)

Re: [Qemu-devel] [PATCH v2 01/16] linux-user: add strace for getuid(), gettid(), getppid(), geteuid()

[Philippe Mathieu-Daudé]

On 05/15/2017 11:59 AM, Miloš Stojanović wrote:

Improve strace support for syscalls getuid(), gettid(), getppid()
and geteuid(). Since these system calls don't have arguments, "%s()"
is added in the corresponding strace.list entry so that no arguments
are printed.

getuid:
Prior to this commit, typical strace output used to look like this:
4894 getuid(4894,0,0,274886293296,-3689348814741910323,4832615904) = 1000
After this commit, it looks like this:
4894 getuid() = 1000

gettid:
Prior to this commit, typical strace output used to look like this:
8307 gettid(0,0,64,0,4832630528,4832615840) = 8307
After this commit, it looks like this:
8307 gettid() = 8307

getppid:
Prior to this commit, typical strace output used to look like this:
20588 getppid(20588,64,0,4832630528,4832615888,0) = 20625
After this commit, it looks like this:
20588 getppid() = 20625

geteuid:
Prior to this commit, typical strace output used to look like this:
20588 geteuid(64,0,0,4832615888,0,-9151031864016699136) = 1000
After this commit, it looks like this:
20588 geteuid() = 1000

Signed-off-by: Miloš Stojanović 


Reviewed-by: Philippe Mathieu-Daudé 


---
 linux-user/strace.list | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/strace.list b/linux-user/strace.list
index 3b1282e..6e33788 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -290,7 +290,7 @@
 { TARGET_NR_getegid32, "getegid32" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_geteuid
-{ TARGET_NR_geteuid, "geteuid" , NULL, NULL, NULL },
+{ TARGET_NR_geteuid, "geteuid" , "%s()", NULL, NULL },
 #endif
 #ifdef TARGET_NR_geteuid32
 { TARGET_NR_geteuid32, "geteuid32" , NULL, NULL, NULL },
@@ -338,7 +338,7 @@
 { TARGET_NR_getpmsg, "getpmsg" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_getppid
-{ TARGET_NR_getppid, "getppid" , NULL, NULL, NULL },
+{ TARGET_NR_getppid, "getppid" , "%s()", NULL, NULL },
 #endif
 #ifdef TARGET_NR_getpriority
 { TARGET_NR_getpriority, "getpriority", "%s(%#x,%#x)", NULL, NULL },
@@ -381,13 +381,13 @@
   NULL, NULL },
 #endif
 #ifdef TARGET_NR_gettid
-{ TARGET_NR_gettid, "gettid" , NULL, NULL, NULL },
+{ TARGET_NR_gettid, "gettid" , "%s()", NULL, NULL },
 #endif
 #ifdef TARGET_NR_gettimeofday
 { TARGET_NR_gettimeofday, "gettimeofday" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_getuid
-{ TARGET_NR_getuid, "getuid" , NULL, NULL, NULL },
+{ TARGET_NR_getuid, "getuid" , "%s()", NULL, NULL },
 #endif
 #ifdef TARGET_NR_getuid32
 { TARGET_NR_getuid32, "getuid32" , NULL, NULL, NULL },

Re: [Qemu-devel] [Qemu-arm] Changes to Broadcom(BCM) files and Raspberry Pi files. Addition of PanelEmu

[Philippe Mathieu-Daudé]

Hi John,


That is going to be very difficult as a lot of the changes are
interlinked the vast majority of the patch is new files.


I rebased your branch on latest qemu/master here:

https://github.com/philmd/qemu/tree/flypie-GDummyPanel-rebased

It is much easier to follow now, the big XML files you added/removed 
also disappeared (gitk was crashing 'Out Of Memory' trying to look at 
your tree).


I hope it can help you to continue reordering in smaller patches.

Regards,

Phil.

On 05/15/2017 01:46 PM, Alistair Francis wrote:


Hey John,

Thanks for the patch!

Unfortunately this patch is too long to review, you need to split the
patch up into shorter more readable patches. Otherwise it's too hard
to people to understand what you are changing and why.

There are some details here:
http://wiki.qemu.org/Contribute/SubmitAPatch
about how to split up
patches. Each patch applied in order shouldn't break any compilation
or runtime. Generally the flow is to add the logic in earlier patches
and then connect it and switch it on in the later patches.

Try splitting up adding/editing each individual device and send that
our first. That is generally the easiest to review/accept.

Thanks,

Alistair


---
.gitignore  |  54 ++
hw/arm/Makefile.objs|2 +-
hw/arm/bcm2835.c|  114 
hw/arm/bcm2835_peripherals.c|  104 
hw/arm/bcm2836.c|3 +-
hw/arm/raspi.c  |  77 ++-
hw/gpio/bcm2835_gpio.c  |  333 ++-
hw/misc/Makefile.objs|2 +
hw/misc/bcm2835_mphi.c  |  163 ++
hw/misc/bcm2835_power.c  |  106 
hw/timer/Makefile.objs  |2 +
hw/timer/bcm2835_st.c|  202 +++
hw/timer/bcm2835_timer.c|  224 +++
hw/usb/Makefile.objs|4 +-
hw/usb/bcm2835_usb.c|  604 +++
hw/usb/bcm2835_usb_regs.h| 1061

++

hw/usb/dev-network.c|2 +-
include/hw/arm/bcm2835.h|  37 ++
include/hw/arm/bcm2835_peripherals.h |  10 +
include/hw/gpio/bcm2835_gpio.h  |5 +
include/hw/intc/bcm2835_control.h|  53 ++
include/hw/intc/bcm2836_control.h|2 +
include/hw/misc/bcm2835_mphi.h  |  28 +
include/hw/misc/bcm2835_power.h  |  22 +
include/hw/timer/bcm2835_st.h|  25 +
include/hw/timer/bcm2835_timer.h|  32 +
include/hw/usb/bcm2835_usb.h|  78 +++
include/qemu/PanelEmu.h  |  53 ++
util/Makefile.objs  |1 +
util/PanelEmu.c  |  293 ++
30 files changed, 3547 insertions(+), 149 deletions(-)
create mode 100644 hw/arm/bcm2835.c
create mode 100644 hw/misc/bcm2835_mphi.c
create mode 100644 hw/misc/bcm2835_power.c
create mode 100644 hw/timer/bcm2835_st.c
create mode 100644 hw/timer/bcm2835_timer.c
create mode 100644 hw/usb/bcm2835_usb.c
create mode 100644 hw/usb/bcm2835_usb_regs.h
create mode 100644 include/hw/arm/bcm2835.h
create mode 100644 include/hw/intc/bcm2835_control.h
create mode 100644 include/hw/misc/bcm2835_mphi.h
create mode 100644 include/hw/misc/bcm2835_power.h
create mode 100644 include/hw/timer/bcm2835_st.h
create mode 100644 include/hw/timer/bcm2835_timer.h
create mode 100644 include/hw/usb/bcm2835_usb.h
create mode 100644 include/qemu/PanelEmu.h
create mode 100644 util/PanelEmu.c

[Qemu-devel] Where did my previous post go?

[jenia.ivlev]

Hello.

I made a post in this group (QEMU). Then I received an email:

Re: [Qemu-devel] Installing Windows with virtio (was: QEMU, increase
graphics memory)



There is no Qemu-devel group in gmane as far as I can see.

How can I find my post and reply to it (to the replies it received ;) )

Thanks
Jenia

[Qemu-devel] [PATCH v9 4/5] shutdown: Add source information to SHUTDOWN and RESET

[Eric Blake]

Time to wire up all the call sites that request a shutdown or
reset to use the enum added in the previous patch.

It would have been less churn to keep the common case with no
arguments as meaning guest-triggered, and only modified the
host-triggered code paths, via a wrapper function, but then we'd
still have to audit that I didn't miss any host-triggered spots;
changing the signature forces us to double-check that I correctly
categorized all callers.

Since command line options can change whether a guest reset request
causes an actual reset vs. a shutdown, it's easy to also add the
information to reset requests.

Signed-off-by: Eric Blake 
Acked-by: David Gibson  [ppc parts]
Reviewed-by: Mark Cave-Ayland  [SPARC part]
Reviewed-by: Cornelia Huck  [s390x parts]

---
v8: rebase later in series
v7: no change
v6: defer event additions to later, add reviews of unchanged portions
v5: drop accidental addition of unrelated files
v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution
v3: retitle again, fix qemu-iotests, use enum rather than raw bool
in all callers
v2: retitle (was "event: Add signal information to SHUTDOWN"),
completely rework to post bool based on whether it is guest-initiated
v1: initial submission, exposing just Unix signals from host
---
 include/sysemu/sysemu.h |  4 ++--
 vl.c| 18 --
 hw/acpi/core.c  |  4 ++--
 hw/arm/highbank.c   |  4 ++--
 hw/arm/integratorcp.c   |  2 +-
 hw/arm/musicpal.c   |  2 +-
 hw/arm/omap1.c  | 10 ++
 hw/arm/omap2.c  |  2 +-
 hw/arm/spitz.c  |  2 +-
 hw/arm/stellaris.c  |  2 +-
 hw/arm/tosa.c   |  2 +-
 hw/i386/pc.c|  2 +-
 hw/i386/xen/xen-hvm.c   |  2 +-
 hw/input/pckbd.c|  4 ++--
 hw/ipmi/ipmi.c  |  4 ++--
 hw/isa/lpc_ich9.c   |  2 +-
 hw/mips/boston.c|  2 +-
 hw/mips/mips_malta.c|  2 +-
 hw/mips/mips_r4k.c  |  4 ++--
 hw/misc/arm_sysctl.c|  8 
 hw/misc/cbus.c  |  2 +-
 hw/misc/macio/cuda.c|  4 ++--
 hw/misc/slavio_misc.c   |  4 ++--
 hw/misc/zynq_slcr.c |  2 +-
 hw/pci-host/apb.c   |  4 ++--
 hw/pci-host/bonito.c|  2 +-
 hw/pci-host/piix.c  |  2 +-
 hw/ppc/e500.c   |  2 +-
 hw/ppc/mpc8544_guts.c   |  2 +-
 hw/ppc/ppc.c|  2 +-
 hw/ppc/ppc405_uc.c  |  2 +-
 hw/ppc/spapr_hcall.c|  2 +-
 hw/ppc/spapr_rtas.c |  4 ++--
 hw/s390x/ipl.c  |  2 +-
 hw/sh4/r2d.c|  2 +-
 hw/timer/etraxfs_timer.c|  2 +-
 hw/timer/m48t59.c   |  4 ++--
 hw/timer/milkymist-sysctl.c |  4 ++--
 hw/timer/pxa2xx_timer.c |  2 +-
 hw/watchdog/watchdog.c  |  2 +-
 hw/xenpv/xen_domainbuild.c  |  2 +-
 hw/xtensa/xtfpga.c  |  2 +-
 kvm-all.c   |  6 +++---
 os-win32.c  |  2 +-
 qmp.c   |  4 ++--
 replay/replay.c |  4 ++--
 target/alpha/sys_helper.c   |  4 ++--
 target/arm/psci.c   |  4 ++--
 target/i386/excp_helper.c   |  2 +-
 target/i386/hax-all.c   |  6 +++---
 target/i386/helper.c|  2 +-
 target/i386/kvm.c   |  2 +-
 target/s390x/helper.c   |  2 +-
 target/s390x/kvm.c  |  4 ++--
 target/s390x/misc_helper.c  |  4 ++--
 target/sparc/int32_helper.c |  2 +-
 ui/sdl.c|  2 +-
 ui/sdl2.c   |  4 ++--
 trace-events|  2 +-
 ui/cocoa.m  |  2 +-
 60 files changed, 98 insertions(+), 98 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 52102fd..e540e6f 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -62,13 +62,13 @@ typedef enum WakeupReason {
 QEMU_WAKEUP_REASON_OTHER,
 } WakeupReason;

-void qemu_system_reset_request(void);
+void qemu_system_reset_request(ShutdownCause reason);
 void qemu_system_suspend_request(void);
 void qemu_register_suspend_notifier(Notifier *notifier);
 void qemu_system_wakeup_request(WakeupReason reason);
 void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
 void qemu_register_wakeup_notifier(Notifier *notifier);
-void qemu_system_shutdown_request(void);
+void qemu_system_shutdown_request(ShutdownCause reason);
 void qemu_system_powerdown_request(void);
 void qemu_register_powerdown_notifier(Notifier *notifier);
 void qemu_system_debug_request(void);
diff --git a/vl.c b/vl.c
index 51ed60f..bc5c1be 100644
--- a/vl.c
+++ b/vl.c
@@ -1724,7 +1724,7 @@ void qemu_system_guest_panicked(GuestPanicInformation 
*info)
 if (!no_shutdown) {
 qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
!!info, info, _abort);
-qemu_system_shutdown_request();
+

[Qemu-devel] [PATCH v9 3/5] shutdown: Preserve shutdown cause through replay

[Eric Blake]

With the recent addition of ShutdownCause, we want to be able to pass
a cause through any shutdown request, and then faithfully replay that
cause when later replaying the same sequence.  The easiest way is to
expand the reply event mechanism to track a series of values for
EVENT_SHUTDOWN, one corresponding to each value of ShutdownCause.

We are free to change the replay stream as needed, since there are
already no guarantees about being able to use a replay stream by
any other version of qemu than the one that generated it.

The cause is not actually fed back until the next patch changes the
signature for requesting a shutdown; a TODO marks that upcoming change.

Yes, this uses the gcc/clang extension of a ranged case label,
but this is not the first time we've used non-C99 constructs.

Signed-off-by: Eric Blake 
Reviewed-by: Pavel Dovgalyuk 

---
v8: hoist earlier in series, tweaks minor enough to keep R-b
v7: rebase to context
v6: new patch
---
 include/sysemu/replay.h  | 3 ++-
 replay/replay-internal.h | 3 ++-
 vl.c | 2 +-
 replay/replay.c  | 7 ---
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index f1c0712..fa14d0e 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -13,6 +13,7 @@
  */

 #include "qapi-types.h"
+#include "sysemu.h"

 /* replay clock kinds */
 enum ReplayClockKind {
@@ -98,7 +99,7 @@ int64_t replay_read_clock(ReplayClockKind kind);
 /* Events */

 /*! Called when qemu shutdown is requested. */
-void replay_shutdown_request(void);
+void replay_shutdown_request(ShutdownCause cause);
 /*! Should be called at check points in the execution.
 These check points are skipped, if they were not met.
 Saves checkpoint in the SAVE mode and validates in the PLAY mode.
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index ed66ed8..3ebb199 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -22,8 +22,9 @@ enum ReplayEvents {
 EVENT_EXCEPTION,
 /* for async events */
 EVENT_ASYNC,
-/* for shutdown request */
+/* for shutdown requests, range allows recovery of ShutdownCause */
 EVENT_SHUTDOWN,
+EVENT_SHUTDOWN_LAST = EVENT_SHUTDOWN + SHUTDOWN_CAUSE__MAX,
 /* for character device write event */
 EVENT_CHAR_WRITE,
 /* for character device read all event */
diff --git a/vl.c b/vl.c
index 5c61d8c..51ed60f 100644
--- a/vl.c
+++ b/vl.c
@@ -1821,8 +1821,8 @@ void qemu_system_killed(int signal, pid_t pid)
 void qemu_system_shutdown_request(void)
 {
 trace_qemu_system_shutdown_request();
-replay_shutdown_request();
 /* TODO - add a parameter to allow callers to specify reason */
+replay_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
 shutdown_requested = SHUTDOWN_CAUSE_HOST_ERROR;
 qemu_notify_event();
 }
diff --git a/replay/replay.c b/replay/replay.c
index f810628..bf94e81 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -49,8 +49,9 @@ bool replay_next_event_is(int event)
 res = true;
 }
 switch (replay_state.data_kind) {
-case EVENT_SHUTDOWN:
+case EVENT_SHUTDOWN ... EVENT_SHUTDOWN_LAST:
 replay_finish_event();
+/* TODO - pass replay_state.data_kind - EVENT_SHUTDOWN as cause */
 qemu_system_shutdown_request();
 break;
 default:
@@ -170,11 +171,11 @@ bool replay_has_interrupt(void)
 return res;
 }

-void replay_shutdown_request(void)
+void replay_shutdown_request(ShutdownCause cause)
 {
 if (replay_mode == REPLAY_MODE_RECORD) {
 replay_mutex_lock();
-replay_put_event(EVENT_SHUTDOWN);
+replay_put_event(EVENT_SHUTDOWN + cause);
 replay_mutex_unlock();
 }
 }
-- 
2.9.4

[Qemu-devel] [PATCH v9 5/5] shutdown: Expose bool cause in SHUTDOWN and RESET events

[Eric Blake]

Libvirt would like to be able to distinguish between a SHUTDOWN
event triggered solely by guest request and one triggered by a
SIGTERM or other action on the host.  While qemu_kill_report() was
already able to give different output to stderr based on whether a
shutdown was triggered by a host signal (but NOT by a host UI event,
such as clicking the X on the window), that information was then
lost to management.  The previous patches improved things to use an
enum throughout all callsites, so now we have something ready to
expose through QMP.

Note that for now, the decision was to expose ONLY a boolean,
rather than promoting ShutdownCause to a QAPI enum; this is because
libvirt has not expressed an interest in anything finer-grained.
We can still add additional details, in a backwards-compatible
manner, if a need later arises (if the addition happens before 2.10,
we can replace the bool with an enum; otherwise, the enum will have
to be in addition to the bool); this patch merely adds a helper
shutdown_caused_by_guest() to map the internal enum into the
external boolean.

Update expected iotest outputs to match the new data (complete
coverage of the affected tests is obtained by -raw, -qcow2, and -nbd).

Here is output from 'virsh qemu-monitor-event --loop' with the
patch installed:

event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
event STOP at 1492639680.732116 for domain fedora_13: 
event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}

Note that libvirt runs qemu with -no-shutdown: the first SHUTDOWN event
was triggered by an action I took directly in the guest (shutdown -h),
at which point qemu stops the vcpus and waits for libvirt to do any
final cleanups; the second SHUTDOWN event is the result of libvirt
sending SIGTERM now that it has completed cleanup.  Libvirt is already
smart enough to only feed the first qemu SHUTDOWN event to the end user
(remember, virsh qemu-monitor-event is a low-level debugging interface
that is explicitly unsupported by libvirt, so it sees things that normal
end users do not); changing qemu to emit SHUTDOWN only once is outside
the scope of this series.

See also https://bugzilla.redhat.com/1384007

Signed-off-by: Eric Blake 

---
v9: silence checkpatch
v8: add shutdown_caused_by_guest(), fix typo in commit message
v7: rebase to context
v6: split out from 'shutdown: Add source information to SHUTDOWN and RESET'
---
 qapi/event.json| 17 +
 include/sysemu/sysemu.h|  5 +
 vl.c   |  8 
 tests/qemu-iotests/071.out |  4 ++--
 tests/qemu-iotests/081.out |  2 +-
 tests/qemu-iotests/087.out | 12 ++--
 tests/qemu-iotests/094.out |  2 +-
 tests/qemu-iotests/117.out |  2 +-
 tests/qemu-iotests/119.out |  2 +-
 tests/qemu-iotests/120.out |  2 +-
 tests/qemu-iotests/140.out |  2 +-
 tests/qemu-iotests/143.out |  2 +-
 tests/qemu-iotests/156.out |  2 +-
 13 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/qapi/event.json b/qapi/event.json
index e80f3f4..6d22b02 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -10,6 +10,10 @@
 # Emitted when the virtual machine has shut down, indicating that qemu is
 # about to exit.
 #
+# @guest: If true, the shutdown was triggered by a guest request (such as
+# a guest-initiated ACPI shutdown request or other hardware-specific action)
+# rather than a host request (such as sending qemu a SIGINT). (since 2.10)
+#
 # Note: If the command-line option "-no-shutdown" has been specified, qemu will
 # not exit, and a STOP event will eventually follow the SHUTDOWN event
 #
@@ -17,11 +21,11 @@
 #
 # Example:
 #
-# <- { "event": "SHUTDOWN",
+# <- { "event": "SHUTDOWN", "data": { "guest": true },
 #  "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
 #
 ##
-{ 'event': 'SHUTDOWN' }
+{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool' } }

 ##
 # @POWERDOWN:
@@ -44,15 +48,20 @@
 #
 # Emitted when the virtual machine is reset
 #
+# @guest: If true, the reset was triggered by a guest request (such as
+# a guest-initiated ACPI reboot request or other hardware-specific action)
+# rather than a host request (such as the QMP command system_reset).
+# (since 2.10)
+#
 # Since: 0.12.0
 #
 # Example:
 #
-# <- { "event": "RESET",
+# <- { "event": "RESET", "data": { "guest": false },
 #  "timestamp": { "seconds": 1267041653, "microseconds": 9518 } }
 #
 ##
-{ 'event': 'RESET' }
+{ 'event': 'RESET', 'data': { 'guest': 'bool' } }

 ##
 # @STOP:
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e540e6f..0ea9593 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -49,6 +49,11 @@ typedef enum ShutdownCause {
 SHUTDOWN_CAUSE__MAX,
 } ShutdownCause;

+static inline bool shutdown_caused_by_guest(ShutdownCause cause)
+{
+return cause >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN;
+}
+
 void vm_start(void);
 int vm_prepare_start(void);
 int vm_stop(RunState state);
diff --git a/vl.c 

[Qemu-devel] [PATCH v9 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request

[Eric Blake]

We want to track why a guest was shutdown; in particular, being able
to tell the difference between a guest request (such as ACPI request)
and host request (such as SIGINT) will prove useful to libvirt.
Since all requests eventually end up changing shutdown_requested in
vl.c, the logical change is to make that value track the reason,
rather than its current 0/1 contents.

Since command-line options control whether a reset request is turned
into a shutdown request instead, the same treatment is given to
reset_requested.

This patch adds an internal enum ShutdownCause that describes reasons
that a shutdown can be requested, and changes qemu_system_reset() to
pass the reason through, although for now nothing is actually changed
with regards to what gets reported.  The enum could be exported via
QAPI at a later date, if deemed necessary, but for now, there has not
been a request to expose that much detail to end clients.

For the most part, we turn 0 into SHUTDOWN_CAUSE_NONE, and 1 into
SHUTDOWN_CAUSE_HOST_ERROR; the only specific case where we have enough
information right now to use a different value is when we are reacting
to a host signal.  It will take a further patch to edit all call-sites
that can trigger a reset or shutdown request to properly pass in any
other reasons; this patch includes TODOs to point such places out.

qemu_system_reset() trades its 'bool report' parameter for a
'ShutdownCause reason', with all non-zero values having the same
effect; this lets us get rid of the weird #defines for VMRESET_*
as synonyms for bools.

Signed-off-by: Eric Blake 

---
v9: one more stray FIXME
v8: s/FIXME/TODO/, include SHUTDOWN_CAUSE__MAX now rather than later,
tweak comment on GUEST_SHUTDOWN to mention suspend
v7: drop 'bool report' from qemu_system_reset(), reorder enum to put
HOST_ERROR == 1, improve commit message
v6: make ShutdownCause internal-only, add SHUTDOWN_CAUSE_NONE so that
comparison to 0 still works, tweak initial FIXME values
v5: no change
v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution
v3: new patch
---
 include/sysemu/sysemu.h | 23 -
 vl.c| 53 ++---
 hw/i386/xen/xen-hvm.c   |  7 +--
 migration/colo.c|  2 +-
 migration/savevm.c  |  2 +-
 5 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 15656b7..52102fd 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -33,8 +33,21 @@ VMChangeStateEntry 
*qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 void vm_state_notify(int running, RunState state);

-#define VMRESET_SILENT   false
-#define VMRESET_REPORT   true
+/* Enumeration of various causes for shutdown. */
+typedef enum ShutdownCause {
+SHUTDOWN_CAUSE_NONE,  /* No shutdown request pending */
+SHUTDOWN_CAUSE_HOST_ERROR,/* An error prevents further use of guest */
+SHUTDOWN_CAUSE_HOST_QMP,  /* Reaction to a QMP command, like 'quit' */
+SHUTDOWN_CAUSE_HOST_SIGNAL,   /* Reaction to a signal, such as SIGINT */
+SHUTDOWN_CAUSE_HOST_UI,   /* Reaction to UI event, like window close */
+SHUTDOWN_CAUSE_GUEST_SHUTDOWN,/* Guest shutdown/suspend request, via
+ ACPI or other hardware-specific means */
+SHUTDOWN_CAUSE_GUEST_RESET,   /* Guest reset request, and command line
+ turns that into a shutdown */
+SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
+ that into a shutdown */
+SHUTDOWN_CAUSE__MAX,
+} ShutdownCause;

 void vm_start(void);
 int vm_prepare_start(void);
@@ -62,10 +75,10 @@ void qemu_system_debug_request(void);
 void qemu_system_vmstop_request(RunState reason);
 void qemu_system_vmstop_request_prepare(void);
 bool qemu_vmstop_requested(RunState *r);
-int qemu_shutdown_requested_get(void);
-int qemu_reset_requested_get(void);
+ShutdownCause qemu_shutdown_requested_get(void);
+ShutdownCause qemu_reset_requested_get(void);
 void qemu_system_killed(int signal, pid_t pid);
-void qemu_system_reset(bool report);
+void qemu_system_reset(ShutdownCause reason);
 void qemu_system_guest_panicked(GuestPanicInformation *info);
 size_t qemu_target_page_size(void);

diff --git a/vl.c b/vl.c
index 7396748..5c61d8c 100644
--- a/vl.c
+++ b/vl.c
@@ -1597,8 +1597,9 @@ void vm_state_notify(int running, RunState state)
 }
 }

-static int reset_requested;
-static int shutdown_requested, shutdown_signal;
+static ShutdownCause reset_requested;
+static ShutdownCause shutdown_requested;
+static int shutdown_signal;
 static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
@@ -1612,19 +1613,19 @@ static NotifierList wakeup_notifiers =
 NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
 static uint32_t 

Re: [Qemu-devel] Changes to Broadcom(BCM) files and Raspberry Pi files. Addition of PanelEmu

[John Bradley via Qemu-devel]

Hi,
That is going to be very difficult as a lot of the changes are interlinked the 
vast majority of the patch is new files. John BradleyTel: 07896 839635Skype: 
flypie125 125B Grove StreetEdge Hill Liverpool L7 7AF 

On Monday, 15 May 2017, 17:54, Alistair Francis 
 wrote:
 

 On Sat, May 13, 2017 at 5:42 PM, John Bradley via Qemu-devel
 wrote:
> From 7f74f048f135d9c9c230a9e90f72451c841c6d35 Mon Sep 17 00:00:00 2001
> From: John Bradley 
> Date: Sat, 13 May 2017 23:07:47 +0100
> Subject: [PATCH] Changes to Broadcom(BCM) files and Raspberry Pi files.
> Addition of PanelEmu
>
> The files add the ability to attach, via TCP, a panel emulator
> The include a unification of several PD Raspberry PI additions
> A modification to dev-network to all circle SDK WWW client to work
> The DummyPanel is not included but available at
> https://github.com/flypie/GDummyPanel.git
>
> Signed-off-by: John Bradley 

Hey John,

Thanks for the patch!

Unfortunately this patch is too long to review, you need to split the
patch up into shorter more readable patches. Otherwise it's too hard
to people to understand what you are changing and why.

There are some details here:
http://wiki.qemu.org/Contribute/SubmitAPatch about how to split up
patches. Each patch applied in order shouldn't break any compilation
or runtime. Generally the flow is to add the logic in earlier patches
and then connect it and switch it on in the later patches.

Try splitting up adding/editing each individual device and send that
our first. That is generally the easiest to review/accept.

Thanks,

Alistair

> ---
> .gitignore                          |  54 ++
> hw/arm/Makefile.objs                |    2 +-
> hw/arm/bcm2835.c                    |  114 
> hw/arm/bcm2835_peripherals.c        |  104 
> hw/arm/bcm2836.c                    |    3 +-
> hw/arm/raspi.c                      |  77 ++-
> hw/gpio/bcm2835_gpio.c              |  333 ++-
> hw/misc/Makefile.objs                |    2 +
> hw/misc/bcm2835_mphi.c              |  163 ++
> hw/misc/bcm2835_power.c              |  106 
> hw/timer/Makefile.objs              |    2 +
> hw/timer/bcm2835_st.c                |  202 +++
> hw/timer/bcm2835_timer.c            |  224 +++
> hw/usb/Makefile.objs                |    4 +-
> hw/usb/bcm2835_usb.c                |  604 +++
> hw/usb/bcm2835_usb_regs.h            | 1061 ++
> hw/usb/dev-network.c                |    2 +-
> include/hw/arm/bcm2835.h            |  37 ++
> include/hw/arm/bcm2835_peripherals.h |  10 +
> include/hw/gpio/bcm2835_gpio.h      |    5 +
> include/hw/intc/bcm2835_control.h    |  53 ++
> include/hw/intc/bcm2836_control.h    |    2 +
> include/hw/misc/bcm2835_mphi.h      |  28 +
> include/hw/misc/bcm2835_power.h      |  22 +
> include/hw/timer/bcm2835_st.h        |  25 +
> include/hw/timer/bcm2835_timer.h    |  32 +
> include/hw/usb/bcm2835_usb.h        |  78 +++
> include/qemu/PanelEmu.h              |  53 ++
> util/Makefile.objs                  |    1 +
> util/PanelEmu.c                      |  293 ++
> 30 files changed, 3547 insertions(+), 149 deletions(-)
> create mode 100644 hw/arm/bcm2835.c
> create mode 100644 hw/misc/bcm2835_mphi.c
> create mode 100644 hw/misc/bcm2835_power.c
> create mode 100644 hw/timer/bcm2835_st.c
> create mode 100644 hw/timer/bcm2835_timer.c
> create mode 100644 hw/usb/bcm2835_usb.c
> create mode 100644 hw/usb/bcm2835_usb_regs.h
> create mode 100644 include/hw/arm/bcm2835.h
> create mode 100644 include/hw/intc/bcm2835_control.h
> create mode 100644 include/hw/misc/bcm2835_mphi.h
> create mode 100644 include/hw/misc/bcm2835_power.h
> create mode 100644 include/hw/timer/bcm2835_st.h
> create mode 100644 include/hw/timer/bcm2835_timer.h
> create mode 100644 include/hw/usb/bcm2835_usb.h
> create mode 100644 include/qemu/PanelEmu.h
> create mode 100644 util/PanelEmu.c
>


   

[Qemu-devel] [PATCH v9 1/5] shutdown: Simplify shutdown_signal

[Eric Blake]

There is no signal 0 (kill(pid, 0) has special semantics to probe whether
a process is alive), rather than actually sending a signal 0).  So we
can use the simpler 0, instead of -1, for our sentinel of whether a
shutdown request due to a signal has happened.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 
Reviewed-by: Markus Armbruster 
Reviewed-by: Alistair Francis 

---
v4-v8: no change
v3: new patch
---
 vl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index ce2b392..7396748 100644
--- a/vl.c
+++ b/vl.c
@@ -1598,7 +1598,7 @@ void vm_state_notify(int running, RunState state)
 }

 static int reset_requested;
-static int shutdown_requested, shutdown_signal = -1;
+static int shutdown_requested, shutdown_signal;
 static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
@@ -1629,7 +1629,7 @@ static int qemu_shutdown_requested(void)

 static void qemu_kill_report(void)
 {
-if (!qtest_driver() && shutdown_signal != -1) {
+if (!qtest_driver() && shutdown_signal) {
 if (shutdown_pid == 0) {
 /* This happens for eg ^C at the terminal, so it's worth
  * avoiding printing an odd message in that case.
@@ -1643,7 +1643,7 @@ static void qemu_kill_report(void)
  shutdown_cmd ? shutdown_cmd : "");
 g_free(shutdown_cmd);
 }
-shutdown_signal = -1;
+shutdown_signal = 0;
 }
 }

-- 
2.9.4

[Qemu-devel] [PATCH v9 0/5] event: Add source information to SHUTDOWN

[Eric Blake]

v6 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01380.html
v7 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01848.html
v8 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03599.html

Since then:
- one more comment fix in patch 2 [Markus]
- format fix in patch 5 [checkpatch]

001/5:[] [--] 'shutdown: Simplify shutdown_signal'
002/5:[0002] [FC] 'shutdown: Prepare for use of an enum in 
reset/shutdown_request'
003/5:[] [--] 'shutdown: Preserve shutdown cause through replay'
004/5:[] [--] 'shutdown: Add source information to SHUTDOWN and RESET'
005/5:[0005] [FC] 'shutdown: Expose bool cause in SHUTDOWN and RESET events'

Eric Blake (5):
  shutdown: Simplify shutdown_signal
  shutdown: Prepare for use of an enum in reset/shutdown_request
  shutdown: Preserve shutdown cause through replay
  shutdown: Add source information to SHUTDOWN and RESET
  shutdown: Expose bool cause in SHUTDOWN and RESET events

 qapi/event.json | 17 ---
 include/sysemu/replay.h |  3 +-
 include/sysemu/sysemu.h | 32 -
 replay/replay-internal.h|  3 +-
 vl.c| 69 ++---
 hw/acpi/core.c  |  4 +--
 hw/arm/highbank.c   |  4 +--
 hw/arm/integratorcp.c   |  2 +-
 hw/arm/musicpal.c   |  2 +-
 hw/arm/omap1.c  | 10 ---
 hw/arm/omap2.c  |  2 +-
 hw/arm/spitz.c  |  2 +-
 hw/arm/stellaris.c  |  2 +-
 hw/arm/tosa.c   |  2 +-
 hw/i386/pc.c|  2 +-
 hw/i386/xen/xen-hvm.c   |  9 --
 hw/input/pckbd.c|  4 +--
 hw/ipmi/ipmi.c  |  4 +--
 hw/isa/lpc_ich9.c   |  2 +-
 hw/mips/boston.c|  2 +-
 hw/mips/mips_malta.c|  2 +-
 hw/mips/mips_r4k.c  |  4 +--
 hw/misc/arm_sysctl.c|  8 +++---
 hw/misc/cbus.c  |  2 +-
 hw/misc/macio/cuda.c|  4 +--
 hw/misc/slavio_misc.c   |  4 +--
 hw/misc/zynq_slcr.c |  2 +-
 hw/pci-host/apb.c   |  4 +--
 hw/pci-host/bonito.c|  2 +-
 hw/pci-host/piix.c  |  2 +-
 hw/ppc/e500.c   |  2 +-
 hw/ppc/mpc8544_guts.c   |  2 +-
 hw/ppc/ppc.c|  2 +-
 hw/ppc/ppc405_uc.c  |  2 +-
 hw/ppc/spapr_hcall.c|  2 +-
 hw/ppc/spapr_rtas.c |  4 +--
 hw/s390x/ipl.c  |  2 +-
 hw/sh4/r2d.c|  2 +-
 hw/timer/etraxfs_timer.c|  2 +-
 hw/timer/m48t59.c   |  4 +--
 hw/timer/milkymist-sysctl.c |  4 +--
 hw/timer/pxa2xx_timer.c |  2 +-
 hw/watchdog/watchdog.c  |  2 +-
 hw/xenpv/xen_domainbuild.c  |  2 +-
 hw/xtensa/xtfpga.c  |  2 +-
 kvm-all.c   |  6 ++--
 migration/colo.c|  2 +-
 migration/savevm.c  |  2 +-
 os-win32.c  |  2 +-
 qmp.c   |  4 +--
 replay/replay.c |  9 +++---
 target/alpha/sys_helper.c   |  4 +--
 target/arm/psci.c   |  4 +--
 target/i386/excp_helper.c   |  2 +-
 target/i386/hax-all.c   |  6 ++--
 target/i386/helper.c|  2 +-
 target/i386/kvm.c   |  2 +-
 target/s390x/helper.c   |  2 +-
 target/s390x/kvm.c  |  4 +--
 target/s390x/misc_helper.c  |  4 +--
 target/sparc/int32_helper.c |  2 +-
 ui/sdl.c|  2 +-
 ui/sdl2.c   |  4 +--
 tests/qemu-iotests/071.out  |  4 +--
 tests/qemu-iotests/081.out  |  2 +-
 tests/qemu-iotests/087.out  | 12 
 tests/qemu-iotests/094.out  |  2 +-
 tests/qemu-iotests/117.out  |  2 +-
 tests/qemu-iotests/119.out  |  2 +-
 tests/qemu-iotests/120.out  |  2 +-
 tests/qemu-iotests/140.out  |  2 +-
 tests/qemu-iotests/143.out  |  2 +-
 tests/qemu-iotests/156.out  |  2 +-
 trace-events|  2 +-
 ui/cocoa.m  |  2 +-
 75 files changed, 196 insertions(+), 150 deletions(-)

-- 
2.9.4

Re: [Qemu-devel] [PATCH v8 0/5] event: Add source information to SHUTDOWN

[Eric Blake]

On 05/15/2017 03:37 PM, no-re...@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 

> Checking PATCH 5/5: shutdown: Expose bool cause in SHUTDOWN and RESET 
> events...
> ERROR: open brace '{' following function declarations go on the next line
> #61: FILE: include/sysemu/sysemu.h:52:
> +static inline bool shutdown_caused_by_guest(ShutdownCause cause) {
> 

Oh fine (I have a hook to run checkpatch locally on every commit; wonder
why it didn't gripe at me). I also missed a FIXME that should have been
TODO in 2/5.   v9 coming up.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-block] [PATCH 01/13] vvfat: fix qemu-img map and qemu-img convert

[Eric Blake]

On 05/15/2017 03:31 PM, Hervé Poussineau wrote:
> - bs->total_sectors is the number of sectors of the whole disk
> - s->sector_count is the number of sectors of the FAT partition
> 
> This fixes the following assert in qemu-img map:
> qemu-img.c:2641: get_block_status: Assertion `nb_sectors' failed.
> 
> This also fixes an infinite loop in qemu-img convert.
> 
> Fixes: 4480e0f924a42e1db8b8cfcac4d0634dd1bb27a0

Wow - broken since 1.2? Not many vvfat users, are there.

Hervé, you might want to work out with Kevin whether to take
co-maintainership over vvfat, in addition to your other areas.

> Fixes: https://bugs.launchpad.net/qemu/+bug/1599539
> Signed-off-by: Hervé Poussineau 

CC: qemu-sta...@nongnu.org

> ---
>  block/vvfat.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Eric Blake 

(I will rebase my work for changing block_status into byte-based on top
of this)

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v8 0/5] event: Add source information to SHUTDOWN

[no-reply]

Hi,

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

Subject: [Qemu-devel] [PATCH v8 0/5] event: Add source information to SHUTDOWN
Message-id: 20170515194149.16288-1-ebl...@redhat.com
Type: series

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

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

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

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

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170515195439.17677-1-ebl...@redhat.com -> 
patchew/20170515195439.17677-1-ebl...@redhat.com
Switched to a new branch 'test'
11c4b62 shutdown: Expose bool cause in SHUTDOWN and RESET events
061189e shutdown: Add source information to SHUTDOWN and RESET
13dc7b9 shutdown: Preserve shutdown cause through replay
99a20d5 shutdown: Prepare for use of an enum in reset/shutdown_request
ab28bb1 shutdown: Simplify shutdown_signal

=== OUTPUT BEGIN ===
Checking PATCH 1/5: shutdown: Simplify shutdown_signal...
Checking PATCH 2/5: shutdown: Prepare for use of an enum in 
reset/shutdown_request...
Checking PATCH 3/5: shutdown: Preserve shutdown cause through replay...
Checking PATCH 4/5: shutdown: Add source information to SHUTDOWN and RESET...
Checking PATCH 5/5: shutdown: Expose bool cause in SHUTDOWN and RESET events...
ERROR: open brace '{' following function declarations go on the next line
#61: FILE: include/sysemu/sysemu.h:52:
+static inline bool shutdown_caused_by_guest(ShutdownCause cause) {

total: 1 errors, 0 warnings, 195 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


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

[Qemu-devel] [PATCH 10/13] vvfat: correctly generate numeric-tail of short file names

[Hervé Poussineau]

More specifically:
- try without numeric-tail only if LFN didn't have invalid short chars
- start at ~1 (instead of ~0)
- handle case if numeric tail is more than one char (ie > 10)

Windows 9x Scandisk doesn't see anymore mismatches between short file names and
long file names for non-ASCII filenames.

Specification: "FAT: General overview of on-disk format" v1.03, page 31
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 67 +++
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 37cfaa85fc..98978df404 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -602,11 +602,14 @@ static uint8_t to_valid_short_char(uint8_t c)
 }
 }
 
-static direntry_t *create_short_filename(BDRVVVFATState *s, long_file_name 
*lfn)
+static direntry_t *create_short_filename(BDRVVVFATState *s, long_file_name 
*lfn,
+ unsigned int directory_start)
 {
 int i, j = 0;
 direntry_t *entry = array_get_next(&(s->directory));
 uint8_t c;
+bool lossy_conversion = false;
+char tail[11];
 
 if (!entry) {
 return NULL;
@@ -621,6 +624,8 @@ static direntry_t *create_short_filename(BDRVVVFATState *s, 
long_file_name *lfn)
 } else if (lfn->name[i + 1] == 0 &&
(lfn->name[i] == '.' || lfn->name[i] == 0)) {
 break;
+} else {
+lossy_conversion = true;
 }
 }
 /* search last extension */
@@ -637,10 +642,37 @@ static direntry_t *create_short_filename(BDRVVVFATState 
*s, long_file_name *lfn)
 entry->name[j++] = c;
 } else if (lfn->name[i + 1] == 0 && lfn->name[i] == 0) {
 break;
+} else {
+lossy_conversion = true;
 }
 }
 }
-return entry;
+
+/* numeric-tail generation */
+for (j = 0; j < 8; j++) {
+if (entry->name[j] == ' ') {
+break;
+}
+}
+for (i = lossy_conversion ? 1 : 0; i < 99; i++) {
+direntry_t *entry1;
+if (i > 0) {
+int len = sprintf(tail, "~%d", i);
+memcpy(entry->name + MIN(j, 8 - len), tail, len);
+}
+for (entry1 = array_get(&(s->directory), directory_start);
+ entry1 < entry; entry1++) {
+if (!is_long_name(entry1) &&
+!memcmp(entry1->name, entry->name, 11)) {
+break; /* found dupe */
+}
+}
+if (entry1 == entry) {
+/* no dupe found */
+return entry;
+}
+}
+return NULL;
 }
 
 /* fat functions */
@@ -754,36 +786,7 @@ static inline direntry_t* 
create_short_and_long_name(BDRVVVFATState* s,
 }
 
 entry_long = create_long_filename(s, filename, );
-entry = create_short_filename(s, );
-
-/* mangle duplicates */
-while(1) {
-direntry_t* entry1=array_get(&(s->directory),directory_start);
-int j;
-
-for(;entry1name,entry->name,11))
-break; /* found dupe */
-if(entry1==entry) /* no dupe found */
-break;
-
-/* use all 8 characters of name */
-if(entry->name[7]==' ') {
-int j;
-for(j=6;j>0 && entry->name[j]==' ';j--)
-entry->name[j]='~';
-}
-
-/* increment number */
-for(j=7;j>0 && entry->name[j]=='9';j--)
-entry->name[j]='0';
-if(j>0) {
-if(entry->name[j]<'0' || entry->name[j]>'9')
-entry->name[j]='0';
-else
-entry->name[j]++;
-}
-}
+entry = create_short_filename(s, , directory_start);
 
 /* calculate checksum; propagate to long name */
 if(entry_long) {
-- 
2.11.0

[Qemu-devel] [PATCH 05/13] vvfat: introduce offset_to_bootsector, offset_to_fat and offset_to_root_dir

[Hervé Poussineau]

- offset_to_bootsector is the number of sectors up to FAT bootsector
- offset_to_fat is the number of sectors up to first File Allocation Table
- offset_to_root_dir is the number of sectors up to root directory sector

Replace first_sectors_number - 1 by offset_to_bootsector.
Replace first_sectors_number by offset_to_fat.
Replace faked_sectors by offset_to_rootdir.

Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 67 +++
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 4f4a63c03f..f60d2a3889 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -320,22 +320,24 @@ static void print_mapping(const struct mapping_t* 
mapping);
 typedef struct BDRVVVFATState {
 CoMutex lock;
 BlockDriverState* bs; /* pointer to parent */
-unsigned int first_sectors_number; /* 1 for a single partition, 0x40 for a 
disk with partition table */
 unsigned char first_sectors[0x40*0x200];
 
 int fat_type; /* 16 or 32 */
 array_t fat,directory,mapping;
 char volume_label[11];
 
+uint32_t offset_to_bootsector; /* 0 for floppy, 0x3f for disk */
+
 unsigned int cluster_size;
 unsigned int sectors_per_cluster;
 unsigned int sectors_per_fat;
 unsigned int sectors_of_root_directory;
 uint32_t last_cluster_of_root_directory;
-unsigned int faked_sectors; /* how many sectors are faked before file data 
*/
 uint32_t sector_count; /* total number of sectors of the partition */
 uint32_t cluster_count; /* total number of clusters of this partition */
 uint32_t max_fat_value;
+uint32_t offset_to_fat;
+uint32_t offset_to_root_dir;
 
 int current_fd;
 mapping_t* current_mapping;
@@ -394,15 +396,15 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int 
heads, int secs)
 partition->attributes=0x80; /* bootable */
 
 /* LBA is used when partition is outside the CHS geometry */
-lba  = sector2CHS(>start_CHS, s->first_sectors_number - 1,
+lba  = sector2CHS(>start_CHS, s->offset_to_bootsector,
  cyls, heads, secs);
 lba |= sector2CHS(>end_CHS,   s->bs->total_sectors - 1,
  cyls, heads, secs);
 
 /*LBA partitions are identified only by start/length_sector_long not by 
CHS*/
-partition->start_sector_long  = cpu_to_le32(s->first_sectors_number - 1);
+partition->start_sector_long  = cpu_to_le32(s->offset_to_bootsector);
 partition->length_sector_long = cpu_to_le32(s->bs->total_sectors
-- s->first_sectors_number + 1);
+- s->offset_to_bootsector);
 
 /* FAT12/FAT16/FAT32 */
 /* DOS uses different types when partition is LBA,
@@ -823,12 +825,12 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 
 static inline uint32_t sector2cluster(BDRVVVFATState* s,off_t sector_num)
 {
-return (sector_num-s->faked_sectors)/s->sectors_per_cluster;
+return (sector_num - s->offset_to_root_dir) / s->sectors_per_cluster;
 }
 
 static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num)
 {
-return s->faked_sectors + s->sectors_per_cluster * cluster_num;
+return s->offset_to_root_dir + s->sectors_per_cluster * cluster_num;
 }
 
 static int init_directories(BDRVVVFATState* s,
@@ -855,6 +857,9 @@ static int init_directories(BDRVVVFATState* s,
 i = 1+s->sectors_per_cluster*0x200*8/s->fat_type;
 s->sectors_per_fat=(s->sector_count+i)/i; /* round up */
 
+s->offset_to_fat = s->offset_to_bootsector + 1;
+s->offset_to_root_dir = s->offset_to_fat + s->sectors_per_fat * 2;
+
 array_init(&(s->mapping),sizeof(mapping_t));
 array_init(&(s->directory),sizeof(direntry_t));
 
@@ -868,7 +873,6 @@ static int init_directories(BDRVVVFATState* s,
 /* Now build FAT, and write back information into directory */
 init_fat(s);
 
-s->faked_sectors=s->first_sectors_number+s->sectors_per_fat*2;
 s->cluster_count=sector2cluster(s, s->sector_count);
 
 mapping = array_get_next(&(s->mapping));
@@ -946,7 +950,8 @@ static int init_directories(BDRVVVFATState* s,
 
 s->current_mapping = NULL;
 
-
bootsector=(bootsector_t*)(s->first_sectors+(s->first_sectors_number-1)*0x200);
+bootsector = (bootsector_t *)(s->first_sectors
+  + s->offset_to_bootsector * 0x200);
 bootsector->jump[0]=0xeb;
 bootsector->jump[1]=0x3e;
 bootsector->jump[2]=0x90;
@@ -957,16 +962,16 @@ static int init_directories(BDRVVVFATState* s,
 bootsector->number_of_fats=0x2; /* number of FATs */
 bootsector->root_entries=cpu_to_le16(s->sectors_of_root_directory*0x10);
 
bootsector->total_sectors16=s->sector_count>0x?0:cpu_to_le16(s->sector_count);
-bootsector->media_type=(s->first_sectors_number>1?0xf8:0xf0); /* media 
descriptor (f8=hd, f0=3.5 fd)*/
+bootsector->media_type = 

[Qemu-devel] [PATCH 01/13] vvfat: fix qemu-img map and qemu-img convert

[Hervé Poussineau]

- bs->total_sectors is the number of sectors of the whole disk
- s->sector_count is the number of sectors of the FAT partition

This fixes the following assert in qemu-img map:
qemu-img.c:2641: get_block_status: Assertion `nb_sectors' failed.

This also fixes an infinite loop in qemu-img convert.

Fixes: 4480e0f924a42e1db8b8cfcac4d0634dd1bb27a0
Fixes: https://bugs.launchpad.net/qemu/+bug/1599539
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index af5153d27d..dfa2a242e1 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2958,8 +2958,7 @@ vvfat_co_pwritev(BlockDriverState *bs, uint64_t offset, 
uint64_t bytes,
 static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
 {
-BDRVVVFATState* s = bs->opaque;
-*n = s->sector_count - sector_num;
+*n = bs->total_sectors - sector_num;
 if (*n > nb_sectors) {
 *n = nb_sectors;
 } else if (*n < 0) {
-- 
2.11.0

[Qemu-devel] [PATCH 13/13] vvfat: change OEM name to 'MSWIN4.1'

[Hervé Poussineau]

According to specification:
"'MSWIN4.1' is the recommanded setting, because it is the setting least likely
to cause compatibility problems. If you want to put something else in here,
that is your option, but the result may be that some FAT drivers might not
recognize the volume."

Specification: "FAT: General overview of on-disk format" v1.03, page 9
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index f96034cda1..b321065060 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1083,7 +1083,7 @@ static int init_directories(BDRVVVFATState* s,
 bootsector->jump[0]=0xeb;
 bootsector->jump[1]=0x3e;
 bootsector->jump[2]=0x90;
-memcpy(bootsector->name,"QEMU",8);
+memcpy(bootsector->name, "MSWIN4.1", 8);
 bootsector->sector_size=cpu_to_le16(0x200);
 bootsector->sectors_per_cluster=s->sectors_per_cluster;
 bootsector->reserved_sectors=cpu_to_le16(1);
-- 
2.11.0

[Qemu-devel] [PATCH 02/13] vvfat: replace tabs by 8 spaces

[Hervé Poussineau]

This was a complete mess. On 2299 indented lines:
- 1329 were with spaces only
- 617 with tabulations only
- 353 with spaces and tabulations

Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 2054 -
 1 file changed, 1027 insertions(+), 1027 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index dfa2a242e1..002bd86e42 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -101,12 +101,12 @@ static inline void* array_get(array_t* array,unsigned int 
index) {
 static inline int array_ensure_allocated(array_t* array, int index)
 {
 if((index + 1) * array->item_size > array->size) {
-   int new_size = (index + 32) * array->item_size;
-   array->pointer = g_realloc(array->pointer, new_size);
-   if (!array->pointer)
-   return -1;
-   array->size = new_size;
-   array->next = index + 1;
+int new_size = (index + 32) * array->item_size;
+array->pointer = g_realloc(array->pointer, new_size);
+if (!array->pointer)
+return -1;
+array->size = new_size;
+array->next = index + 1;
 }
 
 return 0;
@@ -116,7 +116,7 @@ static inline void* array_get_next(array_t* array) {
 unsigned int next = array->next;
 
 if (array_ensure_allocated(array, next) < 0)
-   return NULL;
+return NULL;
 
 array->next = next + 1;
 return array_get(array, next);
@@ -124,15 +124,15 @@ static inline void* array_get_next(array_t* array) {
 
 static inline void* array_insert(array_t* array,unsigned int index,unsigned 
int count) {
 if((array->next+count)*array->item_size>array->size) {
-   int increment=count*array->item_size;
-   array->pointer=g_realloc(array->pointer,array->size+increment);
-   if(!array->pointer)
+int increment=count*array->item_size;
+array->pointer=g_realloc(array->pointer,array->size+increment);
+if(!array->pointer)
 return NULL;
-   array->size+=increment;
+array->size+=increment;
 }
 memmove(array->pointer+(index+count)*array->item_size,
-   array->pointer+index*array->item_size,
-   (array->next-index)*array->item_size);
+array->pointer+index*array->item_size,
+(array->next-index)*array->item_size);
 array->next+=count;
 return array->pointer+index*array->item_size;
 }
@@ -147,12 +147,12 @@ static inline int array_roll(array_t* array,int 
index_to,int index_from,int coun
 int is;
 
 if(!array ||
-   index_to<0 || index_to>=array->next ||
-   index_from<0 || index_from>=array->next)
-   return -1;
+index_to<0 || index_to>=array->next ||
+index_from<0 || index_from>=array->next)
+return -1;
 
 if(index_to==index_from)
-   return 0;
+return 0;
 
 is=array->item_size;
 from=array->pointer+index_from*is;
@@ -161,9 +161,9 @@ static inline int array_roll(array_t* array,int 
index_to,int index_from,int coun
 memcpy(buf,from,is*count);
 
 if(index_to 0);
 assert(index + count <= array->next);
 if(array_roll(array,array->next-1,index,count))
-   return -1;
+return -1;
 array->next -= count;
 return 0;
 }
@@ -217,21 +217,21 @@ typedef struct bootsector_t {
 uint32_t total_sectors;
 union {
 struct {
-   uint8_t drive_number;
-   uint8_t current_head;
-   uint8_t signature;
-   uint32_t id;
-   uint8_t volume_label[11];
-   } QEMU_PACKED fat16;
-   struct {
-   uint32_t sectors_per_fat;
-   uint16_t flags;
-   uint8_t major,minor;
-   uint32_t first_cluster_of_root_directory;
-   uint16_t info_sector;
-   uint16_t backup_boot_sector;
-   uint16_t ignored;
-   } QEMU_PACKED fat32;
+uint8_t drive_number;
+uint8_t current_head;
+uint8_t signature;
+uint32_t id;
+uint8_t volume_label[11];
+} QEMU_PACKED fat16;
+struct {
+uint32_t sectors_per_fat;
+uint16_t flags;
+uint8_t major,minor;
+uint32_t first_cluster_of_root_directory;
+uint16_t info_sector;
+uint16_t backup_boot_sector;
+uint16_t ignored;
+} QEMU_PACKED fat32;
 } u;
 uint8_t fat_type[8];
 uint8_t ignored[0x1c0];
@@ -285,25 +285,25 @@ typedef struct mapping_t {
 /* the clusters of a file may be in any order; this points to the first */
 int first_mapping_index;
 union {
-   /* 

[Qemu-devel] [PATCH 07/13] vvfat: always create . and .. entries at first and in that order

[Hervé Poussineau]

readdir() doesn't always return . and .. entries at first and in that order.
This leads to not creating them at first in the directory, which raises some
errors on file system checking utilities like MS-DOS Scandisk.

Specification: "FAT: General overview of on-disk format" v1.03, page 25

Fixes: https://bugs.launchpad.net/qemu/+bug/1599539
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 348cffe1c4..7da07068b8 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -721,6 +721,12 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 i = mapping->info.dir.first_dir_index =
 first_cluster == 0 ? 0 : s->directory.next;
 
+if (first_cluster != 0) {
+/* create the top entries of a subdirectory */
+(void)create_short_and_long_name(s, i, ".", 1);
+(void)create_short_and_long_name(s, i, "..", 1);
+}
+
 /* actually read the directory, and allocate the mappings */
 while((entry=readdir(dir))) {
 unsigned int length=strlen(dirname)+2+strlen(entry->d_name);
@@ -742,8 +748,11 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 }
 
 /* create directory entry for this file */
-direntry=create_short_and_long_name(s, i, entry->d_name,
-is_dot || is_dotdot);
+if (!is_dot && !is_dotdot) {
+direntry = create_short_and_long_name(s, i, entry->d_name, 0);
+} else {
+direntry = array_get(&(s->directory), is_dot ? i : i + 1);
+}
 direntry->attributes=(S_ISDIR(st.st_mode)?0x10:0x20);
 direntry->reserved[0]=direntry->reserved[1]=0;
 direntry->ctime=fat_datetime(st.st_ctime,1);
-- 
2.11.0

[Qemu-devel] [PATCH 08/13] vvfat: correctly create long names for non-ASCII filenames

[Hervé Poussineau]

Assume that input filename is encoded as UTF-8, so correctly create UTF-16 
encoding.
Reuse long_file_name structure to give back to caller the generated long name.
It will be used in next commit to transform the long file name into short file 
name.

Reference: 
http://stackoverflow.com/questions/7153935/how-to-convert-utf-8-stdstring-to-utf-16-stdwstring
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 132 ++
 1 file changed, 97 insertions(+), 35 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 7da07068b8..5f6356c834 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -357,6 +357,23 @@ typedef struct BDRVVVFATState {
 Error *migration_blocker;
 } BDRVVVFATState;
 
+typedef struct {
+/*
+ * Since the sequence number is at most 0x3f, and the filename
+ * length is at most 13 times the sequence number, the maximal
+ * filename length is 0x3f * 13 bytes.
+ */
+unsigned char name[0x3f * 13 + 1];
+int checksum, len;
+int sequence_number;
+} long_file_name;
+
+static void lfn_init(long_file_name *lfn)
+{
+   lfn->sequence_number = lfn->len = 0;
+   lfn->checksum = 0x100;
+}
+
 /* take the sector position spos and convert it to Cylinder/Head/Sector 
position
  * if the position is outside the specified geometry, fill maximum value for 
CHS
  * and return 1 to signal overflow.
@@ -418,29 +435,90 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int 
heads, int secs)
 
 /* direntry functions */
 
-/* dest is assumed to hold 258 bytes, and pads with 0x up to next multiple 
of 26 */
-static inline int short2long_name(char* dest,const char* src)
-{
-int i;
-int len;
-for(i=0;i<129 && src[i];i++) {
-dest[2*i]=src[i];
-dest[2*i+1]=0;
+/* fills lfn with UTF-16 representation of src filename */
+/* return true if src is valid UTF-8 string, false otherwise */
+static bool filename2long_name(long_file_name *lfn, const char* src)
+{
+uint8_t *dest = lfn->name;
+int i = 0, j;
+int len = 0;
+while (src[i]) {
+uint32_t uni = 0;
+size_t todo;
+uint8_t ch = src[i++];
+if (ch <= 0x7f) {
+uni = ch;
+todo = 0;
+} else if (ch <= 0xbf) {
+return false;
+} else if (ch <= 0xdf) {
+uni = ch & 0x1f;
+todo = 1;
+} else if (ch <= 0xef) {
+uni = ch & 0x0f;
+todo = 2;
+} else if (ch <= 0xf7) {
+uni = ch & 0x07;
+todo = 3;
+} else {
+return false;
+}
+for (j = 0; j < todo; j++) {
+uint8_t ch;
+if (src[i] == '\0') {
+return false;
+}
+ch = src[i++];
+if (ch < 0x80 || ch >= 0xbf) {
+return false;
+}
+uni <<= 6;
+uni += ch & 0x3f;
+}
+if (uni >= 0xd800 && uni <= 0xdfff) {
+return false;
+} else if (uni >= 0x10) {
+return false;
+}
+if (uni <= 0x) {
+dest[len++] = uni & 0xff;
+dest[len++] = uni >> 8;
+} else {
+uint16_t w;
+uni -= 0x1;
+w = (uni >> 10) + 0xd800;
+dest[len++] = w & 0xff;
+dest[len++] = w >> 8;
+w = (uni & 0x3ff) + 0xdc00;
+dest[len++] = w & 0xff;
+dest[len++] = w >> 8;
+}
+}
+dest[len++] = 0;
+dest[len++] = 0;
+while (len % 26 != 0) {
+dest[len++] = 0xff;
 }
-len=2*i;
-dest[2*i]=dest[2*i+1]=0;
-for(i=2*i+2;(i%26);i++)
-dest[i]=0xff;
-return len;
+lfn->len = len;
+return true;
 }
 
-static inline direntry_t* create_long_filename(BDRVVVFATState* s,const char* 
filename)
+static direntry_t *create_long_filename(BDRVVVFATState *s, const char 
*filename,
+long_file_name *lfn)
 {
-char buffer[258];
-int length=short2long_name(buffer,filename),
-number_of_entries=(length+25)/26,i;
+uint8_t *buffer;
+int length, number_of_entries, i;
 direntry_t* entry;
 
+lfn_init(lfn);
+if (!filename2long_name(lfn, filename)) {
+fprintf(stderr, "vvfat: invalid UTF-8 name: %s\n", filename);
+return NULL;
+}
+buffer = lfn->name;
+length = lfn->len;
+number_of_entries = (length + 25) / 26;
+
 for(i=0;idirectory));
 entry->attributes=0xf;
@@ -612,6 +690,7 @@ static inline direntry_t* 
create_short_and_long_name(BDRVVVFATState* s,
 int i,j,long_index=s->directory.next;
 direntry_t* entry = NULL;
 direntry_t* entry_long = NULL;
+long_file_name lfn;
 
 if(is_dot) {
 entry=array_get_next(&(s->directory));
@@ -620,7 +699,7 @@ static inline direntry_t* 

[Qemu-devel] [PATCH 11/13] vvfat: limit number of entries in root directory in FAT12/FAT16

[Hervé Poussineau]

FAT12/FAT16 root directory is two sectors in size, which allows only 512 
directory entries.
Prevent QEMU startup if too much files exist, instead of overflowing root 
directory.

Also introduce variable root_entries, which will be required if we support 
loading
a custom bootsector (bootsect.img).

Fixes: https://bugs.launchpad.net/qemu/+bug/1599539/comments/4
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 98978df404..7b21d6bb21 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -331,8 +331,9 @@ typedef struct BDRVVVFATState {
 unsigned int cluster_size;
 unsigned int sectors_per_cluster;
 unsigned int sectors_per_fat;
-unsigned int sectors_of_root_directory;
 uint32_t last_cluster_of_root_directory;
+/* how many entries are available in root directory (0 for FAT32) */
+uint16_t root_entries;
 uint32_t sector_count; /* total number of sectors of the partition */
 uint32_t cluster_count; /* total number of clusters of this partition */
 uint32_t max_fat_value;
@@ -846,6 +847,12 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 int is_dot=!strcmp(entry->d_name,".");
 int is_dotdot=!strcmp(entry->d_name,"..");
 
+if (first_cluster == 0 && s->directory.next >= s->root_entries - 1) {
+fprintf(stderr, "Too many entries in root directory\n");
+closedir(dir);
+return -2;
+}
+
 if(first_cluster == 0 && (is_dotdot || is_dot))
 continue;
 
@@ -919,15 +926,15 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 memset(direntry,0,sizeof(direntry_t));
 }
 
-/* TODO: if there are more entries, bootsector has to be adjusted! */
-#define ROOT_ENTRIES (0x02 * 0x10 * s->sectors_per_cluster)
-if (mapping_index == 0 && s->directory.next < ROOT_ENTRIES) {
+if (s->fat_type != 32 &&
+mapping_index == 0 &&
+s->directory.next < s->root_entries) {
 /* root directory */
 int cur = s->directory.next;
-array_ensure_allocated(&(s->directory), ROOT_ENTRIES - 1);
-s->directory.next = ROOT_ENTRIES;
+array_ensure_allocated(&(s->directory), s->root_entries - 1);
+s->directory.next = s->root_entries;
 memset(array_get(&(s->directory), cur), 0,
-(ROOT_ENTRIES - cur) * sizeof(direntry_t));
+(s->root_entries - cur) * sizeof(direntry_t));
 }
 
 /* reset the mapping, since s->mapping was possibly realloc()ed */
@@ -992,6 +999,8 @@ static int init_directories(BDRVVVFATState* s,
 /* Now build FAT, and write back information into directory */
 init_fat(s);
 
+/* TODO: if there are more entries, bootsector has to be adjusted! */
+s->root_entries = 0x02 * 0x10 * s->sectors_per_cluster;
 s->cluster_count=sector2cluster(s, s->sector_count);
 
 mapping = array_get_next(&(s->mapping));
@@ -1060,7 +1069,6 @@ static int init_directories(BDRVVVFATState* s,
 }
 
 mapping = array_get(&(s->mapping), 0);
-s->sectors_of_root_directory = mapping->end * s->sectors_per_cluster;
 s->last_cluster_of_root_directory = mapping->end;
 
 /* the FAT signature */
@@ -1079,7 +1087,7 @@ static int init_directories(BDRVVVFATState* s,
 bootsector->sectors_per_cluster=s->sectors_per_cluster;
 bootsector->reserved_sectors=cpu_to_le16(1);
 bootsector->number_of_fats=0x2; /* number of FATs */
-bootsector->root_entries=cpu_to_le16(s->sectors_of_root_directory*0x10);
+bootsector->root_entries = cpu_to_le16(s->root_entries);
 
bootsector->total_sectors16=s->sector_count>0x?0:cpu_to_le16(s->sector_count);
 bootsector->media_type = (s->offset_to_bootsector > 0 ? 0xf8 : 0xf0);
 s->fat.pointer[0] = bootsector->media_type;
-- 
2.11.0

[Qemu-devel] [PATCH 12/13] vvfat: handle KANJI lead byte 0xe5

[Hervé Poussineau]

Specification: "FAT: General overview of on-disk format" v1.03, page 23
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 7b21d6bb21..f96034cda1 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -648,6 +648,9 @@ static direntry_t *create_short_filename(BDRVVVFATState *s, 
long_file_name *lfn,
 }
 }
 }
+if (entry->name[0] == 0xe5) {
+entry->name[0] = 0x05;
+}
 
 /* numeric-tail generation */
 for (j = 0; j < 8; j++) {
@@ -769,8 +772,6 @@ static inline void init_fat(BDRVVVFATState* s)
 
 }
 
-/* TODO: in create_short_filename, 0xe5->0x05 is not yet handled! */
-/* TODO: in parse_short_filename, 0x05->0xe5 is not yet handled! */
 static inline direntry_t* create_short_and_long_name(BDRVVVFATState* s,
 unsigned int directory_start, const char* filename, int is_dot)
 {
@@ -1773,6 +1774,9 @@ static int parse_short_name(BDRVVVFATState* s,
 } else
 lfn->name[i + j + 1] = '\0';
 
+if (lfn->name[0] == 0x05) {
+lfn->name[0] = 0xe5;
+}
 lfn->len = strlen((char*)lfn->name);
 
 return 0;
-- 
2.11.0

[Qemu-devel] [PATCH 06/13] vvfat: fix field names in FAT12/FAT16 boot sector

[Hervé Poussineau]

Specification: "FAT: General overview of on-disk format" v1.03, page 11
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index f60d2a3889..348cffe1c4 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -218,10 +218,12 @@ typedef struct bootsector_t {
 union {
 struct {
 uint8_t drive_number;
-uint8_t current_head;
+uint8_t reserved1;
 uint8_t signature;
 uint32_t id;
 uint8_t volume_label[11];
+uint8_t fat_type[8];
+uint8_t ignored[0x1c0];
 } QEMU_PACKED fat16;
 struct {
 uint32_t sectors_per_fat;
@@ -233,8 +235,6 @@ typedef struct bootsector_t {
 uint16_t ignored;
 } QEMU_PACKED fat32;
 } u;
-uint8_t fat_type[8];
-uint8_t ignored[0x1c0];
 uint8_t magic[2];
 } QEMU_PACKED bootsector_t;
 
@@ -972,13 +972,13 @@ static int init_directories(BDRVVVFATState* s,
 
 /* LATER TODO: if FAT32, this is wrong */
 bootsector->u.fat16.drive_number = s->offset_to_bootsector == 0 ? 0 : 0x80;
-bootsector->u.fat16.current_head=0;
 bootsector->u.fat16.signature=0x29;
 bootsector->u.fat16.id=cpu_to_le32(0xfabe1afd);
 
 memcpy(bootsector->u.fat16.volume_label, s->volume_label,
sizeof(bootsector->u.fat16.volume_label));
-memcpy(bootsector->fat_type,(s->fat_type==12?"FAT12   
":s->fat_type==16?"FAT16   ":"FAT32   "),8);
+memcpy(bootsector->u.fat16.fat_type,
+   s->fat_type == 12 ? "FAT12   " : "FAT16   ", 8);
 bootsector->magic[0]=0x55; bootsector->magic[1]=0xaa;
 
 return 0;
-- 
2.11.0

[Qemu-devel] [PATCH 09/13] vvfat: correctly create base short names for non-ASCII filenames

[Hervé Poussineau]

More specifically, create short name from long name and change blacklist of
invalid chars to whitelist of valid chars.

Windows 9x also now correctly see long file names of filenames containing a 
space,
but Scandisk still complains about mismatch between SFN and LFN.

Specification: "FAT: General overview of on-disk format" v1.03, pages 30/31
Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 84 +++
 1 file changed, 56 insertions(+), 28 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 5f6356c834..37cfaa85fc 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -589,6 +589,60 @@ static void set_begin_of_direntry(direntry_t* direntry, 
uint32_t begin)
 direntry->begin_hi = cpu_to_le16((begin >> 16) & 0x);
 }
 
+static uint8_t to_valid_short_char(uint8_t c)
+{
+if ((c >= '0' && c <= '9') ||
+(c >= 'A' && c <= 'Z') ||
+strchr("$%'-_@~`!(){}^#&", c) != 0) {
+return c;
+} else if (c >= 'a' && c <= 'z') {
+return c - 'a' + 'A';
+} else {
+return 0;
+}
+}
+
+static direntry_t *create_short_filename(BDRVVVFATState *s, long_file_name 
*lfn)
+{
+int i, j = 0;
+direntry_t *entry = array_get_next(&(s->directory));
+uint8_t c;
+
+if (!entry) {
+return NULL;
+}
+memset(entry->name, 0x20, sizeof(entry->name));
+
+/* create short name (keep only valid chars and upcase letters) */
+for (i = 0; i < lfn->len && j < 8; i += 2) {
+c = to_valid_short_char(lfn->name[i]);
+if (lfn->name[i + 1] == 0 && c != 0) {
+entry->name[j++] = c;
+} else if (lfn->name[i + 1] == 0 &&
+   (lfn->name[i] == '.' || lfn->name[i] == 0)) {
+break;
+}
+}
+/* search last extension */
+for (i = lfn->len - 2; i > 0; i -= 2) {
+if (lfn->name[i] == '.' && lfn->name[i + 1] == 0) {
+break;
+}
+}
+if (i != 0) {
+/* create short extension */
+for (; i < lfn->len && j < 11; i += 2) {
+c = to_valid_short_char(lfn->name[i]);
+if (lfn->name[i + 1] == 0 && c != 0) {
+entry->name[j++] = c;
+} else if (lfn->name[i + 1] == 0 && lfn->name[i] == 0) {
+break;
+}
+}
+}
+return entry;
+}
+
 /* fat functions */
 
 static inline uint8_t fat_chksum(const direntry_t* entry)
@@ -687,7 +741,7 @@ static inline void init_fat(BDRVVVFATState* s)
 static inline direntry_t* create_short_and_long_name(BDRVVVFATState* s,
 unsigned int directory_start, const char* filename, int is_dot)
 {
-int i,j,long_index=s->directory.next;
+int long_index = s->directory.next;
 direntry_t* entry = NULL;
 direntry_t* entry_long = NULL;
 long_file_name lfn;
@@ -700,33 +754,7 @@ static inline direntry_t* 
create_short_and_long_name(BDRVVVFATState* s,
 }
 
 entry_long = create_long_filename(s, filename, );
-
-i = strlen(filename);
-for(j = i - 1; j>0  && filename[j]!='.';j--);
-if (j > 0)
-i = (j > 8 ? 8 : j);
-else if (i > 8)
-i = 8;
-
-entry=array_get_next(&(s->directory));
-memset(entry->name, 0x20, sizeof(entry->name));
-memcpy(entry->name, filename, i);
-
-if (j > 0) {
-for (i = 0; i < 3 && filename[j + 1 + i]; i++) {
-entry->name[8 + i] = filename[j + 1 + i];
-}
-}
-
-/* upcase & remove unwanted characters */
-for(i=10;i>=0;i--) {
-if(i==10 || i==7) for(;i>0 && entry->name[i]==' ';i--);
-if(entry->name[i]<=' ' || entry->name[i]>0x7f
-|| strchr(".*?<>|\":/\\[];,+='",entry->name[i]))
-entry->name[i]='_';
-else if(entry->name[i]>='a' && entry->name[i]<='z')
-entry->name[i]+='A'-'a';
-}
+entry = create_short_filename(s, );
 
 /* mangle duplicates */
 while(1) {
-- 
2.11.0

[Qemu-devel] [PATCH 03/13] vvfat: fix typos

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 002bd86e42..57f2489689 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -404,9 +404,9 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int 
heads, int secs)
 /* FAT12/FAT16/FAT32 */
 /* DOS uses different types when partition is LBA,
probably to prevent older versions from using CHS on them */
-partition->fs_type= s->fat_type==12 ? 0x1:
-s->fat_type==16 ? (lba?0xe:0x06):
- /*fat_tyoe==32*/ (lba?0xc:0x0b);
+partition->fs_type = s->fat_type == 12 ? 0x1 :
+ s->fat_type == 16 ? (lba ? 0xe : 0x06) :
+   /*s->fat_type == 32*/ (lba ? 0xc : 0x0b);
 
 real_mbr->magic[0]=0x55; real_mbr->magic[1]=0xaa;
 }
@@ -806,7 +806,7 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 (ROOT_ENTRIES - cur) * sizeof(direntry_t));
 }
 
- /* reget the mapping, since s->mapping was possibly realloc()ed */
+/* reset the mapping, since s->mapping was possibly realloc()ed */
 mapping = array_get(&(s->mapping), mapping_index);
 first_cluster += (s->directory.next - mapping->info.dir.first_dir_index)
 * 0x20 / s->cluster_size;
-- 
2.11.0

[Qemu-devel] [PATCH 00/13] vvfat: misc fixes for read-only mode

[Hervé Poussineau]

Hi,

This patchset fixes some of issues I encountered when trying to use vvfat, and 
fixes
bug #1599539: https://bugs.launchpad.net/qemu/+bug/1599539

Patch 1 fixes a crash when using 'qemu-img convert'.
Patches 2 to 6 are code cleanup. No functionnal changes.
Patches 6 to 12 fix problems detected by disk checking utilities in read-only 
mode.

With these patches, vvfat creates valid FAT volumes and can be used with QEMU 
disk utilities.

Read-write mode is still buggy after this patchset, but at least, I was not
able to crash QEMU anymore.

Note that patch 2 doesn't pass checkpatch.pl, as it changes indentation only.

Hervé

Hervé Poussineau (13):
  vvfat: fix qemu-img map and qemu-img convert
  vvfat: replace tabs by 8 spaces
  vvfat: fix typos
  vvfat: rename useless enumeration values
  vvfat: introduce offset_to_bootsector, offset_to_fat and
offset_to_root_dir
  vvfat: fix field names in FAT12/FAT16 boot sector
  vvfat: always create . and .. entries at first and in that order
  vvfat: correctly create long names for non-ASCII filenames
  vvfat: correctly create base short names for non-ASCII filenames
  vvfat: correctly generate numeric-tail of short file names
  vvfat: limit number of entries in root directory in FAT12/FAT16
  vvfat: handle KANJI lead byte 0xe5
  vvfat: change OEM name to 'MSWIN4.1'

 block/vvfat.c | 2377 ++---
 1 file changed, 1253 insertions(+), 1124 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 04/13] vvfat: rename useless enumeration values

[Hervé Poussineau]

MODE_FAKED and MODE_RENAMED are not and were never used.

Signed-off-by: Hervé Poussineau 
---
 block/vvfat.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 57f2489689..4f4a63c03f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -287,8 +287,7 @@ typedef struct mapping_t {
 union {
 /* offset is
  * - the offset in the file (in clusters) for a file, or
- * - the next cluster of the directory for a directory, and
- * - the address of the buffer for a faked entry
+ * - the next cluster of the directory for a directory
  */
 struct {
 uint32_t offset;
@@ -301,9 +300,13 @@ typedef struct mapping_t {
 /* path contains the full path, i.e. it always starts with s->path */
 char* path;
 
-enum { MODE_UNDEFINED = 0, MODE_NORMAL = 1, MODE_MODIFIED = 2,
-MODE_DIRECTORY = 4, MODE_FAKED = 8,
-MODE_DELETED = 16, MODE_RENAMED = 32 } mode;
+enum {
+MODE_UNDEFINED = 0,
+MODE_NORMAL = 1,
+MODE_MODIFIED = 2,
+MODE_DIRECTORY = 4,
+MODE_DELETED = 8,
+} mode;
 int read_only;
 } mapping_t;
 
-- 
2.11.0

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Richard W.M. Jones]

On Mon, May 15, 2017 at 09:12:54PM +0200, Max Reitz wrote:
> On 2017-05-10 17:57, Richard W.M. Jones wrote:
> > On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote:
> >> Since the last patch in v1 didn't work, I bit the bullet and converted
> >> the whole thing to coroutines (patches 4-6).  This in turns allows a more
> >> elegant solution to wait for CURLStates to get free (patch 7).
> >>
> >> I tested this by lowering CURL_NUM_STATES to 2.  With this change, the
> >> buggy case triggers a couple times while booting a Fedora netinst image.
> > 
> > This series fixes the original bug, so:
> > 
> >   Tested-by: Richard W.M. Jones 
> > 
> > I think the Reported-by in patch 3 should credit Kun Wei for finding
> > the bug, and we should probably mention the BZ too:
> > 
> >   Reported-by: Kun Wei 
> >   Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1447590
> 
> This one is older, though:
> https://bugzilla.redhat.com/show_bug.cgi?id=1437393

Fair enough, it does seem to be the same issue.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

[Qemu-devel] [PATCH] block: Use QDict helpers for --force-share

[Eric Blake]

Fam's addition of --force-share in commits 459571f7 and 335e9937
were developed prior to the addition of QDict scalar insertion
macros, but merged after the general cleanup in commit 46f5ac20.
Patch created mechanically by rerunning:

 spatch --sp-file scripts/coccinelle/qobject.cocci \
--macro-file scripts/cocci-macro-file.h --dir . --in-place

Signed-off-by: Eric Blake 
---

This could go in through Markus' qapi tree (since he took the
original cleanup), a block maintainer (since it only touches block
files), or even the trivial patch queue (since it's so small and
mechanical) - I don't care which, as long as we don't drop it!

 qemu-img.c | 7 +++
 qemu-io.c  | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b506839..60f1784 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -296,7 +296,7 @@ static BlockBackend *img_open_opts(const char *optstr,
 error_report("--force-share/-U conflicts with image options");
 return NULL;
 }
-qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
 }
 blk = blk_new_open(NULL, NULL, options, flags, _err);
 if (!blk) {
@@ -326,7 +326,7 @@ static BlockBackend *img_open_file(const char *filename,
 }

 if (force_share) {
-qdict_put(options, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
 }
 blk = blk_new_open(filename, NULL, options, flags, _err);
 if (!blk) {
@@ -3156,8 +3156,7 @@ static int img_rebase(int argc, char **argv)
 if (!options) {
 options = qdict_new();
 }
-qdict_put(options, BDRV_OPT_FORCE_SHARE,
-  qbool_from_bool(true));
+qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
 }
 bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
 blk_old_backing = blk_new_open(backing_name, NULL,
diff --git a/qemu-io.c b/qemu-io.c
index 34fa8a1..8e38b28 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -76,7 +76,7 @@ static int openfile(char *name, int flags, bool writethrough, 
bool force_share,
 QDECREF(opts);
 return 1;
 }
-qdict_put(opts, BDRV_OPT_FORCE_SHARE, qbool_from_bool(true));
+qdict_put_bool(opts, BDRV_OPT_FORCE_SHARE, true);
 }
 qemuio_blk = blk_new_open(name, NULL, opts, flags, _err);
 if (!qemuio_blk) {
-- 
2.9.4

[Qemu-devel] [PATCH v8 5/5] shutdown: Expose bool cause in SHUTDOWN and RESET events

[Eric Blake]

Libvirt would like to be able to distinguish between a SHUTDOWN
event triggered solely by guest request and one triggered by a
SIGTERM or other action on the host.  While qemu_kill_report() was
already able to give different output to stderr based on whether a
shutdown was triggered by a host signal (but NOT by a host UI event,
such as clicking the X on the window), that information was then
lost to management.  The previous patches improved things to use an
enum throughout all callsites, so now we have something ready to
expose through QMP.

Note that for now, the decision was to expose ONLY a boolean,
rather than promoting ShutdownCause to a QAPI enum; this is because
libvirt has not expressed an interest in anything finer-grained.
We can still add additional details, in a backwards-compatible
manner, if a need later arises (if the addition happens before 2.10,
we can replace the bool with an enum; otherwise, the enum will have
to be in addition to the bool); this patch merely adds a helper
shutdown_caused_by_guest() to map the internal enum into the
external boolean.

Update expected iotest outputs to match the new data (complete
coverage of the affected tests is obtained by -raw, -qcow2, and -nbd).

Here is output from 'virsh qemu-monitor-event --loop' with the
patch installed:

event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
event STOP at 1492639680.732116 for domain fedora_13: 
event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}

Note that libvirt runs qemu with -no-shutdown: the first SHUTDOWN event
was triggered by an action I took directly in the guest (shutdown -h),
at which point qemu stops the vcpus and waits for libvirt to do any
final cleanups; the second SHUTDOWN event is the result of libvirt
sending SIGTERM now that it has completed cleanup.  Libvirt is already
smart enough to only feed the first qemu SHUTDOWN event to the end user
(remember, virsh qemu-monitor-event is a low-level debugging interface
that is explicitly unsupported by libvirt, so it sees things that normal
end users do not); changing qemu to emit SHUTDOWN only once is outside
the scope of this series.

See also https://bugzilla.redhat.com/1384007

Signed-off-by: Eric Blake 

---
v8: add shutdown_caused_by_guest(), fix typo in commit message
v7: rebase to context
v6: split out from 'shutdown: Add source information to SHUTDOWN and RESET'
---
 qapi/event.json| 17 +
 include/sysemu/sysemu.h|  4 
 vl.c   |  8 
 tests/qemu-iotests/071.out |  4 ++--
 tests/qemu-iotests/081.out |  2 +-
 tests/qemu-iotests/087.out | 12 ++--
 tests/qemu-iotests/094.out |  2 +-
 tests/qemu-iotests/117.out |  2 +-
 tests/qemu-iotests/119.out |  2 +-
 tests/qemu-iotests/120.out |  2 +-
 tests/qemu-iotests/140.out |  2 +-
 tests/qemu-iotests/143.out |  2 +-
 tests/qemu-iotests/156.out |  2 +-
 13 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/qapi/event.json b/qapi/event.json
index e80f3f4..6d22b02 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -10,6 +10,10 @@
 # Emitted when the virtual machine has shut down, indicating that qemu is
 # about to exit.
 #
+# @guest: If true, the shutdown was triggered by a guest request (such as
+# a guest-initiated ACPI shutdown request or other hardware-specific action)
+# rather than a host request (such as sending qemu a SIGINT). (since 2.10)
+#
 # Note: If the command-line option "-no-shutdown" has been specified, qemu will
 # not exit, and a STOP event will eventually follow the SHUTDOWN event
 #
@@ -17,11 +21,11 @@
 #
 # Example:
 #
-# <- { "event": "SHUTDOWN",
+# <- { "event": "SHUTDOWN", "data": { "guest": true },
 #  "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
 #
 ##
-{ 'event': 'SHUTDOWN' }
+{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool' } }

 ##
 # @POWERDOWN:
@@ -44,15 +48,20 @@
 #
 # Emitted when the virtual machine is reset
 #
+# @guest: If true, the reset was triggered by a guest request (such as
+# a guest-initiated ACPI reboot request or other hardware-specific action)
+# rather than a host request (such as the QMP command system_reset).
+# (since 2.10)
+#
 # Since: 0.12.0
 #
 # Example:
 #
-# <- { "event": "RESET",
+# <- { "event": "RESET", "data": { "guest": false },
 #  "timestamp": { "seconds": 1267041653, "microseconds": 9518 } }
 #
 ##
-{ 'event': 'RESET' }
+{ 'event': 'RESET', 'data': { 'guest': 'bool' } }

 ##
 # @STOP:
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e540e6f..e505551 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -49,6 +49,10 @@ typedef enum ShutdownCause {
 SHUTDOWN_CAUSE__MAX,
 } ShutdownCause;

+static inline bool shutdown_caused_by_guest(ShutdownCause cause) {
+return cause >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN;
+}
+
 void vm_start(void);
 int vm_prepare_start(void);
 int vm_stop(RunState state);
diff --git a/vl.c b/vl.c
index 

[Qemu-devel] [PATCH v8 4/5] shutdown: Add source information to SHUTDOWN and RESET

[Eric Blake]

Time to wire up all the call sites that request a shutdown or
reset to use the enum added in the previous patch.

It would have been less churn to keep the common case with no
arguments as meaning guest-triggered, and only modified the
host-triggered code paths, via a wrapper function, but then we'd
still have to audit that I didn't miss any host-triggered spots;
changing the signature forces us to double-check that I correctly
categorized all callers.

Since command line options can change whether a guest reset request
causes an actual reset vs. a shutdown, it's easy to also add the
information to reset requests.

Signed-off-by: Eric Blake 
Acked-by: David Gibson  [ppc parts]
Reviewed-by: Mark Cave-Ayland  [SPARC part]
Reviewed-by: Cornelia Huck  [s390x parts]

---
v8: rebase later in series
v7: no change
v6: defer event additions to later, add reviews of unchanged portions
v5: drop accidental addition of unrelated files
v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution
v3: retitle again, fix qemu-iotests, use enum rather than raw bool
in all callers
v2: retitle (was "event: Add signal information to SHUTDOWN"),
completely rework to post bool based on whether it is guest-initiated
v1: initial submission, exposing just Unix signals from host
---
 include/sysemu/sysemu.h |  4 ++--
 vl.c| 18 --
 hw/acpi/core.c  |  4 ++--
 hw/arm/highbank.c   |  4 ++--
 hw/arm/integratorcp.c   |  2 +-
 hw/arm/musicpal.c   |  2 +-
 hw/arm/omap1.c  | 10 ++
 hw/arm/omap2.c  |  2 +-
 hw/arm/spitz.c  |  2 +-
 hw/arm/stellaris.c  |  2 +-
 hw/arm/tosa.c   |  2 +-
 hw/i386/pc.c|  2 +-
 hw/i386/xen/xen-hvm.c   |  2 +-
 hw/input/pckbd.c|  4 ++--
 hw/ipmi/ipmi.c  |  4 ++--
 hw/isa/lpc_ich9.c   |  2 +-
 hw/mips/boston.c|  2 +-
 hw/mips/mips_malta.c|  2 +-
 hw/mips/mips_r4k.c  |  4 ++--
 hw/misc/arm_sysctl.c|  8 
 hw/misc/cbus.c  |  2 +-
 hw/misc/macio/cuda.c|  4 ++--
 hw/misc/slavio_misc.c   |  4 ++--
 hw/misc/zynq_slcr.c |  2 +-
 hw/pci-host/apb.c   |  4 ++--
 hw/pci-host/bonito.c|  2 +-
 hw/pci-host/piix.c  |  2 +-
 hw/ppc/e500.c   |  2 +-
 hw/ppc/mpc8544_guts.c   |  2 +-
 hw/ppc/ppc.c|  2 +-
 hw/ppc/ppc405_uc.c  |  2 +-
 hw/ppc/spapr_hcall.c|  2 +-
 hw/ppc/spapr_rtas.c |  4 ++--
 hw/s390x/ipl.c  |  2 +-
 hw/sh4/r2d.c|  2 +-
 hw/timer/etraxfs_timer.c|  2 +-
 hw/timer/m48t59.c   |  4 ++--
 hw/timer/milkymist-sysctl.c |  4 ++--
 hw/timer/pxa2xx_timer.c |  2 +-
 hw/watchdog/watchdog.c  |  2 +-
 hw/xenpv/xen_domainbuild.c  |  2 +-
 hw/xtensa/xtfpga.c  |  2 +-
 kvm-all.c   |  6 +++---
 os-win32.c  |  2 +-
 qmp.c   |  4 ++--
 replay/replay.c |  4 ++--
 target/alpha/sys_helper.c   |  4 ++--
 target/arm/psci.c   |  4 ++--
 target/i386/excp_helper.c   |  2 +-
 target/i386/hax-all.c   |  6 +++---
 target/i386/helper.c|  2 +-
 target/i386/kvm.c   |  2 +-
 target/s390x/helper.c   |  2 +-
 target/s390x/kvm.c  |  4 ++--
 target/s390x/misc_helper.c  |  4 ++--
 target/sparc/int32_helper.c |  2 +-
 ui/sdl.c|  2 +-
 ui/sdl2.c   |  4 ++--
 trace-events|  2 +-
 ui/cocoa.m  |  2 +-
 60 files changed, 98 insertions(+), 98 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 52102fd..e540e6f 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -62,13 +62,13 @@ typedef enum WakeupReason {
 QEMU_WAKEUP_REASON_OTHER,
 } WakeupReason;

-void qemu_system_reset_request(void);
+void qemu_system_reset_request(ShutdownCause reason);
 void qemu_system_suspend_request(void);
 void qemu_register_suspend_notifier(Notifier *notifier);
 void qemu_system_wakeup_request(WakeupReason reason);
 void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
 void qemu_register_wakeup_notifier(Notifier *notifier);
-void qemu_system_shutdown_request(void);
+void qemu_system_shutdown_request(ShutdownCause reason);
 void qemu_system_powerdown_request(void);
 void qemu_register_powerdown_notifier(Notifier *notifier);
 void qemu_system_debug_request(void);
diff --git a/vl.c b/vl.c
index 4641fdf..808c67b 100644
--- a/vl.c
+++ b/vl.c
@@ -1724,7 +1724,7 @@ void qemu_system_guest_panicked(GuestPanicInformation 
*info)
 if (!no_shutdown) {
 qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
!!info, info, _abort);
-qemu_system_shutdown_request();
+

[Qemu-devel] [PATCH v8 1/5] shutdown: Simplify shutdown_signal

[Eric Blake]

There is no signal 0 (kill(pid, 0) has special semantics to probe whether
a process is alive), rather than actually sending a signal 0).  So we
can use the simpler 0, instead of -1, for our sentinel of whether a
shutdown request due to a signal has happened.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 
Reviewed-by: Markus Armbruster 
Reviewed-by: Alistair Francis 

---
v4-v8: no change
v3: new patch
---
 vl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index ce2b392..7396748 100644
--- a/vl.c
+++ b/vl.c
@@ -1598,7 +1598,7 @@ void vm_state_notify(int running, RunState state)
 }

 static int reset_requested;
-static int shutdown_requested, shutdown_signal = -1;
+static int shutdown_requested, shutdown_signal;
 static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
@@ -1629,7 +1629,7 @@ static int qemu_shutdown_requested(void)

 static void qemu_kill_report(void)
 {
-if (!qtest_driver() && shutdown_signal != -1) {
+if (!qtest_driver() && shutdown_signal) {
 if (shutdown_pid == 0) {
 /* This happens for eg ^C at the terminal, so it's worth
  * avoiding printing an odd message in that case.
@@ -1643,7 +1643,7 @@ static void qemu_kill_report(void)
  shutdown_cmd ? shutdown_cmd : "");
 g_free(shutdown_cmd);
 }
-shutdown_signal = -1;
+shutdown_signal = 0;
 }
 }

-- 
2.9.4

[Qemu-devel] [PATCH v8 3/5] shutdown: Preserve shutdown cause through replay

[Eric Blake]

With the recent addition of ShutdownCause, we want to be able to pass
a cause through any shutdown request, and then faithfully replay that
cause when later replaying the same sequence.  The easiest way is to
expand the reply event mechanism to track a series of values for
EVENT_SHUTDOWN, one corresponding to each value of ShutdownCause.

We are free to change the replay stream as needed, since there are
already no guarantees about being able to use a replay stream by
any other version of qemu than the one that generated it.

The cause is not actually fed back until the next patch changes the
signature for requesting a shutdown; a TODO marks that upcoming change.

Yes, this uses the gcc/clang extension of a ranged case label,
but this is not the first time we've used non-C99 constructs.

Signed-off-by: Eric Blake 
Reviewed-by: Pavel Dovgalyuk 

---
v8: hoist earlier in series, tweaks minor enough to keep R-b
v7: rebase to context
v6: new patch
---
 include/sysemu/replay.h  | 3 ++-
 replay/replay-internal.h | 3 ++-
 vl.c | 2 +-
 replay/replay.c  | 7 ---
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index f1c0712..fa14d0e 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -13,6 +13,7 @@
  */

 #include "qapi-types.h"
+#include "sysemu.h"

 /* replay clock kinds */
 enum ReplayClockKind {
@@ -98,7 +99,7 @@ int64_t replay_read_clock(ReplayClockKind kind);
 /* Events */

 /*! Called when qemu shutdown is requested. */
-void replay_shutdown_request(void);
+void replay_shutdown_request(ShutdownCause cause);
 /*! Should be called at check points in the execution.
 These check points are skipped, if they were not met.
 Saves checkpoint in the SAVE mode and validates in the PLAY mode.
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index ed66ed8..3ebb199 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -22,8 +22,9 @@ enum ReplayEvents {
 EVENT_EXCEPTION,
 /* for async events */
 EVENT_ASYNC,
-/* for shutdown request */
+/* for shutdown requests, range allows recovery of ShutdownCause */
 EVENT_SHUTDOWN,
+EVENT_SHUTDOWN_LAST = EVENT_SHUTDOWN + SHUTDOWN_CAUSE__MAX,
 /* for character device write event */
 EVENT_CHAR_WRITE,
 /* for character device read all event */
diff --git a/vl.c b/vl.c
index 2060038..4641fdf 100644
--- a/vl.c
+++ b/vl.c
@@ -1821,8 +1821,8 @@ void qemu_system_killed(int signal, pid_t pid)
 void qemu_system_shutdown_request(void)
 {
 trace_qemu_system_shutdown_request();
-replay_shutdown_request();
 /* TODO - add a parameter to allow callers to specify reason */
+replay_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
 shutdown_requested = SHUTDOWN_CAUSE_HOST_ERROR;
 qemu_notify_event();
 }
diff --git a/replay/replay.c b/replay/replay.c
index f810628..bf94e81 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -49,8 +49,9 @@ bool replay_next_event_is(int event)
 res = true;
 }
 switch (replay_state.data_kind) {
-case EVENT_SHUTDOWN:
+case EVENT_SHUTDOWN ... EVENT_SHUTDOWN_LAST:
 replay_finish_event();
+/* TODO - pass replay_state.data_kind - EVENT_SHUTDOWN as cause */
 qemu_system_shutdown_request();
 break;
 default:
@@ -170,11 +171,11 @@ bool replay_has_interrupt(void)
 return res;
 }

-void replay_shutdown_request(void)
+void replay_shutdown_request(ShutdownCause cause)
 {
 if (replay_mode == REPLAY_MODE_RECORD) {
 replay_mutex_lock();
-replay_put_event(EVENT_SHUTDOWN);
+replay_put_event(EVENT_SHUTDOWN + cause);
 replay_mutex_unlock();
 }
 }
-- 
2.9.4

[Qemu-devel] [PATCH v8 0/5] event: Add source information to SHUTDOWN

[Eric Blake]

v6 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01380.html
v7 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01848.html

Since then:
- reorder the series [Markus]
- tweak some comments [Markus]
- add helper function in patch 5 [Markus]

001/5:[] [--] 'shutdown: Simplify shutdown_signal'
002/5:[0011] [FC] 'shutdown: Prepare for use of an enum in 
reset/shutdown_request'
003/5:[0009] [FC] 'shutdown: Preserve shutdown cause through replay'
004/5:[0014] [FC] 'shutdown: Add source information to SHUTDOWN and RESET'
005/5:[0010] [FC] 'shutdown: Expose bool cause in SHUTDOWN and RESET events'

Eric Blake (5):
  shutdown: Simplify shutdown_signal
  shutdown: Prepare for use of an enum in reset/shutdown_request
  shutdown: Preserve shutdown cause through replay
  shutdown: Add source information to SHUTDOWN and RESET
  shutdown: Expose bool cause in SHUTDOWN and RESET events

 qapi/event.json | 17 ---
 include/sysemu/replay.h |  3 +-
 include/sysemu/sysemu.h | 31 +++-
 replay/replay-internal.h|  3 +-
 vl.c| 69 ++---
 hw/acpi/core.c  |  4 +--
 hw/arm/highbank.c   |  4 +--
 hw/arm/integratorcp.c   |  2 +-
 hw/arm/musicpal.c   |  2 +-
 hw/arm/omap1.c  | 10 ---
 hw/arm/omap2.c  |  2 +-
 hw/arm/spitz.c  |  2 +-
 hw/arm/stellaris.c  |  2 +-
 hw/arm/tosa.c   |  2 +-
 hw/i386/pc.c|  2 +-
 hw/i386/xen/xen-hvm.c   |  9 --
 hw/input/pckbd.c|  4 +--
 hw/ipmi/ipmi.c  |  4 +--
 hw/isa/lpc_ich9.c   |  2 +-
 hw/mips/boston.c|  2 +-
 hw/mips/mips_malta.c|  2 +-
 hw/mips/mips_r4k.c  |  4 +--
 hw/misc/arm_sysctl.c|  8 +++---
 hw/misc/cbus.c  |  2 +-
 hw/misc/macio/cuda.c|  4 +--
 hw/misc/slavio_misc.c   |  4 +--
 hw/misc/zynq_slcr.c |  2 +-
 hw/pci-host/apb.c   |  4 +--
 hw/pci-host/bonito.c|  2 +-
 hw/pci-host/piix.c  |  2 +-
 hw/ppc/e500.c   |  2 +-
 hw/ppc/mpc8544_guts.c   |  2 +-
 hw/ppc/ppc.c|  2 +-
 hw/ppc/ppc405_uc.c  |  2 +-
 hw/ppc/spapr_hcall.c|  2 +-
 hw/ppc/spapr_rtas.c |  4 +--
 hw/s390x/ipl.c  |  2 +-
 hw/sh4/r2d.c|  2 +-
 hw/timer/etraxfs_timer.c|  2 +-
 hw/timer/m48t59.c   |  4 +--
 hw/timer/milkymist-sysctl.c |  4 +--
 hw/timer/pxa2xx_timer.c |  2 +-
 hw/watchdog/watchdog.c  |  2 +-
 hw/xenpv/xen_domainbuild.c  |  2 +-
 hw/xtensa/xtfpga.c  |  2 +-
 kvm-all.c   |  6 ++--
 migration/colo.c|  2 +-
 migration/savevm.c  |  2 +-
 os-win32.c  |  2 +-
 qmp.c   |  4 +--
 replay/replay.c |  9 +++---
 target/alpha/sys_helper.c   |  4 +--
 target/arm/psci.c   |  4 +--
 target/i386/excp_helper.c   |  2 +-
 target/i386/hax-all.c   |  6 ++--
 target/i386/helper.c|  2 +-
 target/i386/kvm.c   |  2 +-
 target/s390x/helper.c   |  2 +-
 target/s390x/kvm.c  |  4 +--
 target/s390x/misc_helper.c  |  4 +--
 target/sparc/int32_helper.c |  2 +-
 ui/sdl.c|  2 +-
 ui/sdl2.c   |  4 +--
 tests/qemu-iotests/071.out  |  4 +--
 tests/qemu-iotests/081.out  |  2 +-
 tests/qemu-iotests/087.out  | 12 
 tests/qemu-iotests/094.out  |  2 +-
 tests/qemu-iotests/117.out  |  2 +-
 tests/qemu-iotests/119.out  |  2 +-
 tests/qemu-iotests/120.out  |  2 +-
 tests/qemu-iotests/140.out  |  2 +-
 tests/qemu-iotests/143.out  |  2 +-
 tests/qemu-iotests/156.out  |  2 +-
 trace-events|  2 +-
 ui/cocoa.m  |  2 +-
 75 files changed, 195 insertions(+), 150 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v8 2/5] shutdown: Prepare for use of an enum in reset/shutdown_request

[Eric Blake]

We want to track why a guest was shutdown; in particular, being able
to tell the difference between a guest request (such as ACPI request)
and host request (such as SIGINT) will prove useful to libvirt.
Since all requests eventually end up changing shutdown_requested in
vl.c, the logical change is to make that value track the reason,
rather than its current 0/1 contents.

Since command-line options control whether a reset request is turned
into a shutdown request instead, the same treatment is given to
reset_requested.

This patch adds an internal enum ShutdownCause that describes reasons
that a shutdown can be requested, and changes qemu_system_reset() to
pass the reason through, although for now nothing is actually changed
with regards to what gets reported.  The enum could be exported via
QAPI at a later date, if deemed necessary, but for now, there has not
been a request to expose that much detail to end clients.

For the most part, we turn 0 into SHUTDOWN_CAUSE_NONE, and 1 into
SHUTDOWN_CAUSE_HOST_ERROR; the only specific case where we have enough
information right now to use a different value is when we are reacting
to a host signal.  It will take a further patch to edit all call-sites
that can trigger a reset or shutdown request to properly pass in any
other reasons; this patch includes TODOs to point such places out.

qemu_system_reset() trades its 'bool report' parameter for a
'ShutdownCause reason', with all non-zero values having the same
effect; this lets us get rid of the weird #defines for VMRESET_*
as synonyms for bools.

Signed-off-by: Eric Blake 

---
v8: s/FIXME/TODO/, include SHUTDOWN_CAUSE__MAX now rather than later,
tweak comment on GUEST_SHUTDOWN to mention suspend
v7: drop 'bool report' from qemu_system_reset(), reorder enum to put
HOST_ERROR == 1, improve commit message
v6: make ShutdownCause internal-only, add SHUTDOWN_CAUSE_NONE so that
comparison to 0 still works, tweak initial FIXME values
v5: no change
v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution
v3: new patch
---
 include/sysemu/sysemu.h | 23 -
 vl.c| 53 ++---
 hw/i386/xen/xen-hvm.c   |  7 +--
 migration/colo.c|  2 +-
 migration/savevm.c  |  2 +-
 5 files changed, 58 insertions(+), 29 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 15656b7..52102fd 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -33,8 +33,21 @@ VMChangeStateEntry 
*qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 void vm_state_notify(int running, RunState state);

-#define VMRESET_SILENT   false
-#define VMRESET_REPORT   true
+/* Enumeration of various causes for shutdown. */
+typedef enum ShutdownCause {
+SHUTDOWN_CAUSE_NONE,  /* No shutdown request pending */
+SHUTDOWN_CAUSE_HOST_ERROR,/* An error prevents further use of guest */
+SHUTDOWN_CAUSE_HOST_QMP,  /* Reaction to a QMP command, like 'quit' */
+SHUTDOWN_CAUSE_HOST_SIGNAL,   /* Reaction to a signal, such as SIGINT */
+SHUTDOWN_CAUSE_HOST_UI,   /* Reaction to UI event, like window close */
+SHUTDOWN_CAUSE_GUEST_SHUTDOWN,/* Guest shutdown/suspend request, via
+ ACPI or other hardware-specific means */
+SHUTDOWN_CAUSE_GUEST_RESET,   /* Guest reset request, and command line
+ turns that into a shutdown */
+SHUTDOWN_CAUSE_GUEST_PANIC,   /* Guest panicked, and command line turns
+ that into a shutdown */
+SHUTDOWN_CAUSE__MAX,
+} ShutdownCause;

 void vm_start(void);
 int vm_prepare_start(void);
@@ -62,10 +75,10 @@ void qemu_system_debug_request(void);
 void qemu_system_vmstop_request(RunState reason);
 void qemu_system_vmstop_request_prepare(void);
 bool qemu_vmstop_requested(RunState *r);
-int qemu_shutdown_requested_get(void);
-int qemu_reset_requested_get(void);
+ShutdownCause qemu_shutdown_requested_get(void);
+ShutdownCause qemu_reset_requested_get(void);
 void qemu_system_killed(int signal, pid_t pid);
-void qemu_system_reset(bool report);
+void qemu_system_reset(ShutdownCause reason);
 void qemu_system_guest_panicked(GuestPanicInformation *info);
 size_t qemu_target_page_size(void);

diff --git a/vl.c b/vl.c
index 7396748..2060038 100644
--- a/vl.c
+++ b/vl.c
@@ -1597,8 +1597,9 @@ void vm_state_notify(int running, RunState state)
 }
 }

-static int reset_requested;
-static int shutdown_requested, shutdown_signal;
+static ShutdownCause reset_requested;
+static ShutdownCause shutdown_requested;
+static int shutdown_signal;
 static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
@@ -1612,19 +1613,19 @@ static NotifierList wakeup_notifiers =
 NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
 static uint32_t wakeup_reason_mask = ~(1 << 

Re: [Qemu-devel] [Qemu-block] [PATCH v3 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Max Reitz]

On 2017-05-15 12:00, Paolo Bonzini wrote:
> Compared to v2, this silences checkpatch and correctly destroy the mutex on
> exiting from curl_open with an error.
> 
> Paolo
> 
> Paolo Bonzini (7):
>   curl: strengthen assertion in curl_clean_state
>   curl: never invoke callbacks with s->mutex held
>   curl: avoid recursive locking of BDRVCURLState mutex
>   curl: split curl_find_state/curl_init_state
>   curl: convert CURLAIOCB to byte values
>   curl: convert readv to coroutines
>   curl: do not do aio_poll when waiting for a free CURLState
> 
>  block/curl.c | 217 
> +--
>  1 file changed, 121 insertions(+), 96 deletions(-)

Modulo checkpatch:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create

[Max Reitz]

On 2017-05-15 20:41, Max Reitz wrote:
> On 2017-05-12 21:47, John Snow wrote:
>>
>>
>> On 05/12/2017 03:46 PM, Eric Blake wrote:
>>> On 05/12/2017 01:07 PM, Max Reitz wrote:
 On 2017-05-11 20:27, John Snow wrote:
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
>
> Or, rather, force the open of a backing image if one was specified
> for creation. Using a similar -unsafe option as rebase, allow qemu-img
> to ignore the backing file validation if possible.
>
>>>
> +++ b/block.c
> @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const 
> char *fmt,
>  // The size for the image must always be specified, with one 
> exception:
>  // If we are using a backing file, we can obtain the size from there
>  size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> -if (size == -1) {

 "Hang on, why should this be -1 when the defval is 0? Where does the -1
 come from?"
 "..."
 "Oh, the option exists and is set to -1? Why is that?"
 "..."
 "Oh, because this function always sets it itself, and because @img_size
 is set to (uint64_t)-1."
>>>
>>> I had pretty much the same conversation on my v1 review.
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>>

 First, I won't start with how signed integer overflow is
 implementation-defined in C because I hope you have thrashed that out
 with Eric (I hope that "to thrash out" is a good translation for
 "auskaspern" (lit. "to buffoon out").).
>>>
>>> Sounds like a reasonable choice of words, even if I don't speak the
>>> counterpart language to validate your translation.
>>>
>>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>>> wrinkles at you.
> 
> We're not really fine because that conversion happens when the result of
> qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).
> 
>>> I seem to recall that qemu has chosen to use compiler flags and/or
>>> assumptions that we are using 2s-complement arithmetic with sane
>>> behavior (that is, tighter behavior than the bare minimum that C
>>> requires), because it was easier than auditing our code for strict C
>>> compliance on border cases of conversions from unsigned to signed that
>>> trigger undefined behavior.  But again, I don't think it affects this
>>> patch (where our conversion is only from signed to unsigned, and that is
>>> well-defined behavior).
> 
> Right. Which is why I said I won't even start on it, but of course I
> did. O:-)
> 
 Second, well, at least we should put -1 as the default value here, then.
>>>
>>> Indeed, now that two reviewers have tripped on it,
>>> qemu_opt_get_size(,,-1) would be nicer.
>>>

 Not strictly your fault or something that you need to fix, but it is
 just a single line in the vicinity...

 Let me know if you want to address this, for now I'll leave a

 Reviewed-by: Max Reitz 

 here if you don't want to.
>>>
>>> I'm okay whether you want to squash that fix into this patch, or whether
>>> you do it as a separate followup patch.
>>>
>>
>> I had considered the issue separate; but you're welcome to either write
>> a patch or squish it into this one, I'm not going to be picky.
> 
> Yep, it is a separate issue, just related. :-)
> 
> But since you and Eric agree, I've squashed it in and thus I'm more than
> glad to thank you very much and announce this patch as applied to my
> block branch:
> 
> https://github.com/XanClic/qemu/commits/block

...well, so much for that. I'll have to unstage it again because it
breaks a bunch of iotests (41 85 96 118 139 141 144 155 156) due to
failing to acquire image locks. :-/

I suspect this is because the backing file is opened somewhere and
trying to open it breaks now with the locking series applied.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/7] curl: locking cleanups/fixes, coroutine conversion, remove aio_poll

[Max Reitz]

On 2017-05-10 17:57, Richard W.M. Jones wrote:
> On Wed, May 10, 2017 at 04:31:58PM +0200, Paolo Bonzini wrote:
>> Since the last patch in v1 didn't work, I bit the bullet and converted
>> the whole thing to coroutines (patches 4-6).  This in turns allows a more
>> elegant solution to wait for CURLStates to get free (patch 7).
>>
>> I tested this by lowering CURL_NUM_STATES to 2.  With this change, the
>> buggy case triggers a couple times while booting a Fedora netinst image.
> 
> This series fixes the original bug, so:
> 
>   Tested-by: Richard W.M. Jones 
> 
> I think the Reported-by in patch 3 should credit Kun Wei for finding
> the bug, and we should probably mention the BZ too:
> 
>   Reported-by: Kun Wei 
>   Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1447590

This one is older, though:
https://bugzilla.redhat.com/show_bug.cgi?id=1437393

:-)

Max

> A nit pick perhaps but in patch 5 you say "This was broken before for
> disks > 2TB, but now it would break at 4GB.".
> I understand after reading it a few times that you mean it would be
> broken at 4GB, if you hadn't changed size_t -> uint64_t (on 32 bit
> platforms).  Perhaps better to clarify that sentence.
> 
> ---
> 
> I also ran some performance and stability testing.  I used virt-ls for
> this.  The following command will iterate over every file in a remote
> guest image and print an md5sum:
> 
>   LIBGUESTFS_BACKEND=direct \
>   LIBGUESTFS_HV=~/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
>   virt-ls -a http://somehost/rhel-guest-image-7.1-20150224.0.x86_64.qcow2 \
>   -lR --checksum /
> 
> I timed this with and without your patches, but there was no
> significant difference (but note that virt-ls is a fundamentally
> sequential program).
> 
> It didn't crash or hang at any time during my testing.
> 
> Rich.
> 




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] gluster: add support for PREALLOC_MODE_FALLOC

[Niels de Vos]

Add missing support for "preallocation=falloc" to the Gluster block
driver. This change bases its logic on that of block/file-posix.c and
removed the gluster_supports_zerofill() function in favour of an #ifdef
check in an easy to read switch-statement.

Reported-by: Satheesaran Sundaramoorthi 
URL: https://bugzilla.redhat.com/1450759
Signed-off-by: Niels de Vos 
---
 block/gluster.c | 61 +++--
 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 7c76cd0..566166f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -965,11 +965,6 @@ static coroutine_fn int 
qemu_gluster_co_pwrite_zeroes(BlockDriverState *bs,
 return acb.ret;
 }
 
-static inline bool gluster_supports_zerofill(void)
-{
-return 1;
-}
-
 static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset,
 int64_t size)
 {
@@ -977,11 +972,6 @@ static inline int qemu_gluster_zerofill(struct glfs_fd 
*fd, int64_t offset,
 }
 
 #else
-static inline bool gluster_supports_zerofill(void)
-{
-return 0;
-}
-
 static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset,
 int64_t size)
 {
@@ -996,9 +986,10 @@ static int qemu_gluster_create(const char *filename,
 struct glfs *glfs;
 struct glfs_fd *fd;
 int ret = 0;
-int prealloc = 0;
+PreallocMode prealloc;
 int64_t total_size = 0;
 char *tmp = NULL;
+Error *local_err = NULL;
 
 gconf = g_new0(BlockdevOptionsGluster, 1);
 gconf->debug = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
@@ -1026,13 +1017,12 @@ static int qemu_gluster_create(const char *filename,
   BDRV_SECTOR_SIZE);
 
 tmp = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
-if (!tmp || !strcmp(tmp, "off")) {
-prealloc = 0;
-} else if (!strcmp(tmp, "full") && gluster_supports_zerofill()) {
-prealloc = 1;
-} else {
-error_setg(errp, "Invalid preallocation mode: '%s'"
- " or GlusterFS doesn't support zerofill API", tmp);
+prealloc = qapi_enum_parse(PreallocMode_lookup, tmp,
+   PREALLOC_MODE__MAX, PREALLOC_MODE_OFF,
+   _err);
+g_free(tmp);
+if (local_err) {
+error_propagate(errp, local_err);
 ret = -EINVAL;
 goto out;
 }
@@ -1041,21 +1031,46 @@ static int qemu_gluster_create(const char *filename,
 O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | 
S_IWUSR);
 if (!fd) {
 ret = -errno;
-} else {
+goto out;
+}
+
+switch (prealloc) {
+case PREALLOC_MODE_FALLOC:
+if (!glfs_fallocate(fd, 0, 0, total_size)) {
+error_setg(errp, "Could not preallocate data for the new file");
+ret = -errno;
+}
+break;
+#ifdef CONFIG_GLUSTERFS_ZEROFILL
+case PREALLOC_MODE_FULL:
 if (!glfs_ftruncate(fd, total_size)) {
-if (prealloc && qemu_gluster_zerofill(fd, 0, total_size)) {
+if (qemu_gluster_zerofill(fd, 0, total_size)) {
+error_setg(errp, "Could not zerofill the new file");
 ret = -errno;
 }
 } else {
+error_setg(errp, "Could not resize file");
 ret = -errno;
 }
-
-if (glfs_close(fd) != 0) {
+break;
+#endif /* CONFIG_GLUSTERFS_ZEROFILL */
+case PREALLOC_MODE_OFF:
+if (glfs_ftruncate(fd, total_size) != 0) {
 ret = -errno;
+error_setg(errp, "Could not resize file");
 }
+break;
+default:
+ret = -EINVAL;
+error_setg(errp, "Unsupported preallocation mode: %s",
+   PreallocMode_lookup[prealloc]);
+break;
+}
+
+if (glfs_close(fd) != 0) {
+ret = -errno;
 }
 out:
-g_free(tmp);
 qapi_free_BlockdevOptionsGluster(gconf);
 glfs_clear_preopened(glfs);
 return ret;
-- 
2.9.3

Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
> >>  const VMStateDescription vmstate_virtio_ccw_dev = {
> >>  .name = "s390_virtio_ccw_dev",
> >>  .version_id = 1,
> >> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> >>  VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> >>  VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> >>  VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> >> +{
> >> +/*
> >> + * Ugly hack because VirtIODevice does not migrate itself.
> >> + * This also makes legacy via vmstate_save_state possible.
> >> + */
> >> +.name = "virtio/config_vector",
> >> +.info = _info_config_vector,
> >> +},
> > I'm a bit confused - isn't just this bit the bit going
> > through the vdev->load_config hook?
> >
>  Exactly! If you examine the part underscored with == in
>  virtio_ccw_(load|save)_config  (that is what is behind vdev->load_config)
>  you should see that we use vmstate_virtio_ccw_dev (that is this
>  VMStateDescription to implement these function. 
> 
>  Actually virtio_ccw_(load|save)_config won't do anything after patch 9
>  and for new machines since the VirtIOCcwDevice is going to migrate
>  itself via DeviceClass.vmsd like other "normal" devices, and we fall
>  back to the VirtIO specific callbacks only in "legacy mode".
> 
>  I hope this helps!
> >>> Hmm, still confused!
> >>> Why are you changing a Virtio device not to use the same migration
> >>> oddities as all the other virtio devices?
> >>>
> >>> I was assuming we'd have to change the virtio core code to
> >>> solve that for all virtio devices.
> >>>
> >>
> >> You can ask difficult questions ;). First I'm not aware of any
> >> ongoing effort to solve this for all virtio devices by changing
> >> the core, and probably breaking compatibility for all devices
> >> (in a sense I break migration compatibility for all virtio-ccw
> >> devices). To be honest, I have considered that move unlikely,
> >> but the possibility is one of my reasons for seeking an upstream
> >> discussion and having You and Michael on cc.
> > 
> > Well I have been trying to improve it, but that code is particularly
> > nasty; and I don't want to break compatibility.  I had some ideas,
> > for example I was thinking of changing the vdev->load_config to
> > a VMState* and then calling a vmstate_load_state(f, vdc->config,...
> > from virtio_load - but there's some challenging casting and hackery
> > there.
> > 
> >>
> >> Why not use virtio oddities? Because they are oddities. I have
> >> figured, it's a good idea to separate the migration of the 
> >> proxy form the rest: we have two QEMU Device objects and it
> >> should be good practice, that these are migrating themselves via
> >> DeviceClass.vmsd. That's what I get with this patch set, 
> >> for new machine versions (since we can not fix the past), and
> >> with the notable difference of config_vector, because it is
> >> defined as a common infrastructure (struct VirtIODevice) but
> >> ain't migrated as a common virtio infrastructure.
> > 
> > Have you got a bit of a description of your classes/structure - it's
> > a little hard to get my head around.
> > 
> 
> Unfortunately I do not have any extra description besides the comments
> and the commit messages. What exactly do you mean  by 'my
> classes/structure'?  I would like to provide some helpful developer
> documentation on how migration works for s390x. There were voices on the
> internal mailing list too requesting something like that, but I find it
> hard, because for me, the most challenging part was understanding how
> qemu migration works in general and the virtio oddities come next. 

Yes, there are only about 2 people who have the overlap of understanding
migration AND s390 IO.

> Fore example, I still don't understand why is is (virtio) load_config
> called like that, when what it mainly does is loading state of the proxy
> which is basically the reification of the device side of the virtio spec
> calls the transport within QOM. (I say mainly, because of this
> config_vector which resides in core but is migrated by via a callback for
> some strange reason I do not understand).

I think the idea is that virtio_load is trying to act as a generic
save/load with a number of virtual components that are specialised for:
  a) The device (e.g. rng, serial, gpu, net, blk)
  b) The transport (PCI, MMIO, CCW etc)
  c) The virtio queue content
  d) But has a load of core stuff (features, the virtio ring management)

(a) & (b) are very much virtual-function like that doesn't fit that
well 

Re: [Qemu-devel] [PATCH] 9pfs: local: forbid client access to metadata (CVE-2017-7493)

[Greg Kurz]

On Mon, 15 May 2017 12:37:08 -0500
Eric Blake  wrote:

> On 05/15/2017 11:07 AM, Greg Kurz wrote:
> > When using the mapped-file security mode, we shouldn't let the client mess
> > with the metadata. The current code already tries to hide the metadata dir
> > from the client by skipping it in local_readdir(). But the client can still
> > access or modify it through several other operations. This can be used to
> > escalate privileges in the guest.
> > 
> > Affected backend operations are:
> > - local_mknod()
> > - local_mkdir()
> > - local_open2()
> > - local_symlink()
> > - local_link()
> > - local_unlinkat()
> > - local_renameat()
> > - local_rename()
> > - local_name_to_path()
> > 
> > Other operations are safe because they are only passed a fid path, which
> > is computed internally in local_name_to_path().
> > 
> > This patch converts all the functions listed above to fail and return
> > EINVAL when being passed the name of the metadata dir. This may look
> > like a poor choice for errno, but there's no such thing as an illegal
> > path name on Linux and I could not think of anything better.
> > 
> > This fixes CVE-2017-7493.
> > 
> > Reported-by: Leo Gaspard 
> > Signed-off-by: Greg Kurz 
> > ---  
> 
> Reviewed-by: Eric Blake 
> 

Thanks again for your help.

Cheers,

--
Greg


pgpEnCAfwWBzq.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4] qemu-img: Check for backing image if specified during create

[Max Reitz]

On 2017-05-12 21:47, John Snow wrote:
> 
> 
> On 05/12/2017 03:46 PM, Eric Blake wrote:
>> On 05/12/2017 01:07 PM, Max Reitz wrote:
>>> On 2017-05-11 20:27, John Snow wrote:
 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1213786

 Or, rather, force the open of a backing image if one was specified
 for creation. Using a similar -unsafe option as rebase, allow qemu-img
 to ignore the backing file validation if possible.

>>
 +++ b/block.c
 @@ -4275,37 +4275,37 @@ void bdrv_img_create(const char *filename, const 
 char *fmt,
  // The size for the image must always be specified, with one 
 exception:
  // If we are using a backing file, we can obtain the size from there
  size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
 -if (size == -1) {
>>>
>>> "Hang on, why should this be -1 when the defval is 0? Where does the -1
>>> come from?"
>>> "..."
>>> "Oh, the option exists and is set to -1? Why is that?"
>>> "..."
>>> "Oh, because this function always sets it itself, and because @img_size
>>> is set to (uint64_t)-1."
>>
>> I had pretty much the same conversation on my v1 review.
>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01097.html
>>
>>>
>>> First, I won't start with how signed integer overflow is
>>> implementation-defined in C because I hope you have thrashed that out
>>> with Eric (I hope that "to thrash out" is a good translation for
>>> "auskaspern" (lit. "to buffoon out").).
>>
>> Sounds like a reasonable choice of words, even if I don't speak the
>> counterpart language to validate your translation.
>>
>> (uint64_t)-1 is well-defined in C (so I think we're just fine here). But
>> (int64_t)UINT64_MAX is where signed integer overflow does indeed throw
>> wrinkles at you.

We're not really fine because that conversion happens when the result of
qemu_opt_get_size() (a uint64_t) is stored in size (an int64_t).

>> I seem to recall that qemu has chosen to use compiler flags and/or
>> assumptions that we are using 2s-complement arithmetic with sane
>> behavior (that is, tighter behavior than the bare minimum that C
>> requires), because it was easier than auditing our code for strict C
>> compliance on border cases of conversions from unsigned to signed that
>> trigger undefined behavior.  But again, I don't think it affects this
>> patch (where our conversion is only from signed to unsigned, and that is
>> well-defined behavior).

Right. Which is why I said I won't even start on it, but of course I
did. O:-)

>>> Second, well, at least we should put -1 as the default value here, then.
>>
>> Indeed, now that two reviewers have tripped on it,
>> qemu_opt_get_size(,,-1) would be nicer.
>>
>>>
>>> Not strictly your fault or something that you need to fix, but it is
>>> just a single line in the vicinity...
>>>
>>> Let me know if you want to address this, for now I'll leave a
>>>
>>> Reviewed-by: Max Reitz 
>>>
>>> here if you don't want to.
>>
>> I'm okay whether you want to squash that fix into this patch, or whether
>> you do it as a separate followup patch.
>>
> 
> I had considered the issue separate; but you're welcome to either write
> a patch or squish it into this one, I'm not going to be picky.

Yep, it is a separate issue, just related. :-)

But since you and Eric agree, I've squashed it in and thus I'm more than
glad to thank you very much and announce this patch as applied to my
block branch:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v9 01/13] qcow2: Unallocate unmapped zero clusters if no backing file

[Max Reitz]

On 2017-05-13 01:00, John Snow wrote:
> 
> 
> On 05/12/2017 12:06 PM, Max Reitz wrote:
>> On 2017-05-11 16:56, Eric Blake wrote:
>>> [revisiting this older patch version, even though the final version in
>>> today's pull request changed somewhat from this approach]
>>>
>>> On 04/12/2017 04:49 AM, Kevin Wolf wrote:
 Am 11.04.2017 um 03:17 hat Eric Blake geschrieben:
> 'qemu-img map' already coalesces information about unallocated
> clusters (those with status 0) and pure zero clusters (those
> with status BDRV_BLOCK_ZERO and no offset).  Furthermore, all
> qcow2 images with no backing file already report all unallocated
> clusters (in the preallocation sense of clusters with no offset)
> as BDRV_BLOCK_ZERO, regardless of whether the QCOW_OFLAG_ZERO was
> set in that L2 entry (QCOW_OFLAG_ZERO also implies a return of
> BDRV_BLOCK_ALLOCATED, but we intentionally do not expose that bit
> to external users), thanks to generic block layer code in
> bdrv_co_get_block_status().
>
> So, for an image with no backing file, having bdrv_pwrite_zeroes
> mark clusters as unallocated (defer to backing file) rather than
> reads-as-zero (regardless of backing file) makes no difference
> to normal behavior, but may potentially allow for fewer writes to
> the L2 table of a freshly-created image where the L2 table is
> initially written to all-zeroes (although I don't actually know
> if we skip an L2 update and flush when re-writing the same
> contents as already present).

 I don't get this. Allocating a cluster always involves an L2 update, no
 matter whether it was previously unallocated or a zero cluster.
>>>
>>> On IRC, John, Kevin, and I were discussing the current situation with
>>> libvirt NBD storage migration.  When libvirt creates a file on the
>>> destination (rather than the user pre-creating it), it currently
>>> defaults to 0.10 [v2] images, even if the source was a 1.1 image [v3]
>>> (arguably something that could be improved in libvirt, but
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1371749 was closed as not a
>>> bug).
>>>
>>> Therefore, the use case of doing a mirror job to a v2 image, and having
>>> that image become thick even though the source was thin, is happening
>>> more than we'd like
>>> (https://bugzilla.redhat.com/show_bug.cgi?id=1371749).  While Kevin had
>>> a point that in the common case we ALWAYS want to turn an unallocated
>>> cluster into a zero cluster (so that we don't have to audit whether all
>>> callers are properly accounting for the case where a backing image is
>>> added later), our conversation on IRC today conceded that we may want to
>>> introduce a new BDRV_REQ_READ_ZERO_NOP (or some such name) that
>>> particular callers can use to request that if a cluster already reads as
>>> zeroes, the write zero request does NOT have to change it.  Normal guest
>>> operations would not use the flag, but mirroring IS a case that would
>>> use the flag, so that we can end up with thinner mirrors even to 0.10
>>> images.

Reading this again, you could just pass through the write-zero request.
The issue right now is that the qcow2 driver just returns -ENOTSUP for
every write-zero request on v2 images when it could just create a data
cluster and invoke a write-zero request on the data.

Just not suppressing the BDRV_REQ_ZERO_WRITE flag for the generic
fall-back zero write (with bs->supported_write_flags listing it) and
then issuing a bdrv_co_pwrite_zeroes() in qcow2_co_pwritev() works (with
BDRV_REQ_MAY_UNMAP).

(Whether or not to set BDRV_REQ_MAY_UNMAP should be derived from...
Maybe QCOW2_DISCARD_OTHER?)

>>> The other consideration is that on 0.10 images, even if we have to
>>> allocate, right now our allocation is done by way of failing with
>>> -ENOTSUP and falling back to the normal pwrite() of explicit zeroes.  It
>>> may be worth teaching the qcow2 layer to explicitly handle write zeroes,
>>> even on 0.10 images, by allocating a cluster (as needed) but then
>>> telling bs->file to write zeroes (punching a hole as appropriate) so
>>> that the file is still thin.  In fact, it matches the fact that we
>>> already have code that probes whether a qcow2 cluster that reports
>>> BDRV_BLOCK_DATA|BDRV_BLOCK_OFFSET_VALID then probes bs->file to see if
>>> there is a hole there, at which point it can add BDRV_BLOCK_ZERO to the
>>> bdrv_get_block_status.
>>>
>>> I don't know which one of us will tackle patches along these lines, but
>>> wanted to at least capture the gist of the IRC conversation in the
>>> archives for a bit more permanence.
>>
>> Just short ideas:
>>
>> (1) I do consider it a bug if v2 images are created. The BZ wasn't
>> closed for a very good reason, but because nobody answered this question:
>>
>>> is this really a problem?
>>
>> And apparently it is.
>>
>> (2) There is a way of creating zero clusters on v2 images, but we've
>> never done it (yet): You create a single data cluster 

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  wrote:
> 
> >> Forget -b/-i.
> >> 
> >> migration_set_parameter compression_threads 8
> >> 
> >> migrate 
> >> 
> >> We don't use compression_threads at all
> >> 
> >> migrate_set_capability compress
> >> 
> >> migrate 
> >> 
> >> Now, we use compression threads.
> >> 
> >> So, compression_threads parameter is a parameter that is only used when
> >> compress capability is enabled.
> >> 
> >> Same for block migration.  Block_incremental parameter is used only when
> >> block migration capability is setup.  No dependency check needed at all.
> >> 
> >> Or I am losing something obvious here?
> >
> > Ah, you've made up a new rule
> 
> Oops, I thought that was a rule on how things worked O:-)
> 
> >- I don't think it's a bad rule
> > but is it true? Do we always enable a capability before we use a
> > parameter? I don't think so - I think the tls parameters don't have
> > a capability.
> 
> To use tls paramater, we need to use an url with tls, no?
> 
> > My previous rule was just that if it was a bool it was a capability
> > and you can have whatever dependencies you like there - or none.
> 
> Dunno.
> 
> If you think that we can document it (one way or another), I am for it.
> 
> It is really weird that we both thought (reading) the interface that the
> rules are different O:-)

Well I think that's because no one ever defined any rules!  We just
created parameters because we needed something that could take
non-boolean values, but I don't think there's anything more
about the structure of their use that's actually defined.
Similarly, I don't think there's anything defined about the
structure of capabilities.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [Bug 921208] Re: win7/x64 installer hangs on startup with 0x0000005d.

[Thomas Huth]

The patch with CPUID_DE has apparently been included here:
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=b6c5a6f021f485fc36
So has this issue now been fixed in the current version of QEMU?

** Changed in: qemu
   Status: Confirmed => Incomplete

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

Title:
  win7/x64 installer hangs on startup with 0x005d.

Status in QEMU:
  Incomplete
Status in qemu package in Ubuntu:
  Triaged

Bug description:
  hi,

  during booting win7/x64 installer i'm observing a bsod with 0x005d
  ( msdn: unsupported_processor ).

  used command line: qemu-system-x86_64 -m 2048 -hda w7-system.img
  -cdrom win7_x64.iso -boot d

  adding '-machine accel=kvm' instead of default tcg accel helps to
  boot.

  
  installed software:

  qemu-1.0
  linux-3.2.1
  glibc-2.14.1
  gcc-4.6.2

  hw cpu:

  processor   : 0..7
  vendor_id   : GenuineIntel
  cpu family  : 6
  model   : 42
  model name  : Intel(R) Core(TM) i7-2630QM CPU @ 2.00GHz
  stepping: 7
  microcode   : 0x14
  cpu MHz : 1995.739
  cache size  : 6144 KB
  physical id : 0
  siblings: 8
  core id : 3
  cpu cores   : 4
  apicid  : 7
  initial apicid  : 7
  fpu : yes
  fpu_exception   : yes
  cpuid level : 13
  wp  : yes
  flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca 
cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx 
rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology 
nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx est tm2 ssse3 
cx16 xtpr pdcm pcid sse4_1 sse4_2 x2apic popcnt tsc_deadline_timer xsave avx 
lahf_lm ida arat epb xsaveopt pln pts dts tpr_shadow vnmi flexpriority ept vpid
  bogomips: 3992.23
  clflush size: 64
  cache_alignment : 64
  address sizes   : 36 bits physical, 48 bits virtual

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

Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Dr. David Alan Gilbert]

* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> 
> 
> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
> >> As a preparation for switching to a vmstate based migration let us
> >> introduce vmstate entities (e.g. VMStateDescription) for the css entities
> >> to be migrated. Alongside some comments explaining or indicating the not
> >> migration of certain members are introduced too.
> >>
> >> No changes in behavior, we just added some dead code -- which should
> >> rise to life soon.
> >>
> >> Signed-off-by: Halil Pasic 
> >> ---
> >>  hw/s390x/css.c | 276 
> >> +
> >>  include/hw/s390x/css.h |  10 +-
> >>  2 files changed, 285 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index c03bb20..2bda7d0 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -20,29 +20,231 @@
> >>  #include "hw/s390x/css.h"
> >>  #include "trace.h"
> >>  #include "hw/s390x/s390_flic.h"
> >> +#include "hw/s390x/s390-virtio-ccw.h"
> >>  
> 
> [..]
> 
> >> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> >> +VMStateField *field)
> >> +{
> >> +int32_t len;
> >> +IndAddr **ind_addr = pv;
> >> +
> >> +len = qemu_get_be32(f);
> >> +if (len != 0) {
> >> +*ind_addr = get_indicator(qemu_get_be64(f), len);
> >> +} else {
> >> +qemu_get_be64(f);
> >> +*ind_addr = NULL;
> >> +}
> >> +return 0;
> >> +}
> >> +
> >> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> >> +VMStateField *field, QJSON *vmdesc)
> >> +{
> >> +IndAddr *ind_addr = *(IndAddr **) pv;
> >> +
> >> +if (ind_addr != NULL) {
> >> +qemu_put_be32(f, ind_addr->len);
> >> +qemu_put_be64(f, ind_addr->addr);
> >> +} else {
> >> +qemu_put_be32(f, 0);
> >> +qemu_put_be64(f, 0UL);
> >> +}
> >> +return 0;
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_ind_addr = {
> >> +.name = "s390_ind_addr",
> >> +.get = css_get_ind_addr,
> >> +.put = css_put_ind_addr
> >> +};
> > 
> > You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> > declare a temporary struct something like:
> >   struct tmp_ind_addr {
> >  IndAddr *parent;
> >  uint32_t  len;
> >  uint64_t  addr;
> >   }
> > 
> > and then your .get/.put routines turn into pre_save/post_load
> > routines to just setup the len/addr.
> > 
> 
> I don't think this is going to work -- unfortunately! You can see below,
> how this IndAddr* migration stuff is supposed to be used:
> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
> field when describing state which needs and IndAddr* migrated.
> 
> The problem is, we do not know in what state will this field
> be embedded, the pre_save/post_load called by put_tmp/get_tmp
> is however copying the pointer to this state into the parent.
> So instead of having a pointer to IndAddr* in those functions
> and updating it accordingly, I would have to find the IndAddr*
> in some arbitrary state (in our case VirtioCcwDevice) first,
> and I lack information for that.
> 
> If it's hard to follow I can give you the patch I was debugging
> to come to this conclusion. (By the way I ended up with 10
> lines of code more than in this version, and although I think
> it looks nicer, it's simpler only if one knows how WITH_TMP
> works. My plan was to ask you which version do you like more
> and go with that before I realized it ain't gonna work.)
> 

Yes, I see - I've got some similar other cases; the challenge
is it's a custom allocator - 'get_indicator' - and it's used
as fields in a few places.  Hmm.

Dave

> >> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> >> index f1f0d7f..6a451b2 100644
> >> --- a/include/hw/s390x/css.h
> 
> [..]
> 
> >>  
> >> +extern const VMStateInfo vmstate_info_ind_addr;
> >> +
> >> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s)   
> >> \
> >> +VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
> >> +
> >>  IndAddr *get_indicator(hwaddr ind_addr, int len);
> >>  void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
> >>  int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
> >> -- 
> >> 2.10.2
> >>
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> > 
> 
> 
> Cheers,
> Halil
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] qemu-img: Fix leakage of options on error

[Max Reitz]

On 2017-05-15 16:10, Fam Zheng wrote:
> Reported by Coverity.
> 
> Signed-off-by: Fam Zheng 
> ---
>  qemu-img.c | 1 +
>  1 file changed, 1 insertion(+)

Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 0/4] Improve convert and dd commands

[Max Reitz]

On 2017-05-15 18:47, Daniel P. Berrange wrote:
> Update to
> 
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05699.html
>   v2: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00728.html
>   v3: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04391.html
>   v4: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02153.html
>   v5: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04109.html
>   v6: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00215.html
>   v7: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00242.html
>   v8: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg02028.html
>   v9: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03504.html
> 
> This series is in response to Max pointing out that you cannot
> use 'convert' for an encrypted target image.
> 
> The 'convert' and 'dd' commands need to first create the image
> and then open it. The bdrv_create() method takes a set of options
> for creating the image, which let us provide a key-secret for the
> encryption key. When the commands then open the new image, they
> don't provide any options, so the image is unable to be opened
> due to lack of encryption key. It is also not possible to use
> the --image-opts argument to provide structured options in the
> target image name - it must be a plain filename to satisfy the
> bdrv_create() API contract.
> 
> This series addresses these problems to some extent
> 
>  - Adds a new --target-image-opts flag which is used to say
>that the target filename is using structured options.
>It is *only* permitted to use this when -n is also set.
>ie the target image must be pre-created so convert/dd
>don't need to run bdrv_create().
> 
>  - When --target-image-opts is not used, add special case
>code that identifies options passed to bdrv_create()
>named "*key-secret" and adds them to the options used
>to open the new image
> 
> In future it is desirable to make --target-image-opts work even when -n is
> *not* given. This requires considerable work to create a new bdrv_create()
> API impl.
> 
> The first patch fixes a bug in the 'dd' command while the second adds support
> for the missing '--object' arg to 'dd', allowing it to reference secrets when
> opening files.  The last two patches implement the new features described 
> above
> for the 'convert' command.

Thanks,

Reviewed-by: Max Reitz 

...and applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v10 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Eric Blake]

On 05/15/2017 11:47 AM, Daniel P. Berrange wrote:
> The '--image-opts' flag indicates whether the source filename
> includes options. The target filename has to remain in the
> plain filename format though, since it needs to be passed to
> bdrv_create().  When using --skip-create though, it would be
> possible to use image-opts syntax. This adds --target-image-opts
> to indicate that the target filename includes options. Currently
> this mandates use of the --skip-create flag too.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-img-cmds.hx |  4 +--
>  qemu-img.c   | 84 
> ++--
>  qemu-img.texi| 12 ++--
>  3 files changed, 69 insertions(+), 31 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert"  wrote:

>> Forget -b/-i.
>> 
>> migration_set_parameter compression_threads 8
>> 
>> migrate 
>> 
>> We don't use compression_threads at all
>> 
>> migrate_set_capability compress
>> 
>> migrate 
>> 
>> Now, we use compression threads.
>> 
>> So, compression_threads parameter is a parameter that is only used when
>> compress capability is enabled.
>> 
>> Same for block migration.  Block_incremental parameter is used only when
>> block migration capability is setup.  No dependency check needed at all.
>> 
>> Or I am losing something obvious here?
>
> Ah, you've made up a new rule

Oops, I thought that was a rule on how things worked O:-)

>- I don't think it's a bad rule
> but is it true? Do we always enable a capability before we use a
> parameter? I don't think so - I think the tls parameters don't have
> a capability.

To use tls paramater, we need to use an url with tls, no?

> My previous rule was just that if it was a bool it was a capability
> and you can have whatever dependencies you like there - or none.

Dunno.

If you think that we can document it (one way or another), I am for it.

It is really weird that we both thought (reading) the interface that the
rules are different O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH v9 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Max Reitz]

On 2017-05-15 19:43, Max Reitz wrote:
> On 2017-05-15 16:04, Daniel P. Berrange wrote:
>> The qemu-img dd/convert commands will create an image file and
>> then try to open it. Historically it has been possible to open
>> new files without passing any options. With encrypted files
>> though, the *key-secret options are mandatory, so we need to
>> provide those options when opening the newly created file.
>>
>> Reviewed-by: Max Reitz 
>> Reviewed-by: Fam Zheng 
>> Reviewed-by: Eric Blake 
>> Signed-off-by: Daniel P. Berrange 
>> ---
>>  qemu-img.c | 42 +-
>>  1 file changed, 37 insertions(+), 5 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index e0e3d31..dcddded 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -314,15 +314,18 @@ static BlockBackend *img_open_opts(const char *optstr,
>>  }
>>  
>>  static BlockBackend *img_open_file(const char *filename,
>> +   QDict *options,
>> const char *fmt, int flags,
>> bool writethrough, bool quiet,
>> bool force_share)
>>  {
>>  BlockBackend *blk;
>>  Error *local_err = NULL;
>> -QDict *options = qdict_new();
>>  
>>  if (fmt) {
>> +if (!options) {
>> +options = qdict_new();
>> +}
> 
> This is the only place where my attempted rebase and your version
> differ. I think this has to be done unconditionally, because otherwise:
> 
> $ ./qemu-img info -U null-co://
> [1]16327 segmentation fault (core dumped)  ./qemu-img info -U null-co://
> 
> Also, I'm not sure the R-bs apply for this patch any longer.
> 
> (They do for patch 1 because it's just a contextual difference. For
> patch 2, it's a borderline case (I would drop it, but I can understand
> keeping it). For patch 3 it's more than just borderline - I would
> definitely drop the R-b, but the differences are still rather
> mechanical, so it is acceptable to keep it.
> But I think there are too many changes here in this patch to keep the
> R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an
> R-b to this segfault...)

And just saw v10... Maybe I should start working on my inbox back to
front...

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] sockets: Plug memory leak in socket_address_flatten()

[Eric Blake]

On 05/15/2017 11:39 AM, Markus Armbruster wrote:
> socket_address_flatten() leaks a SocketAddress when its argument is
> null.  Happens when opening a ChardevBackend of type 'udp' that is
> configured without a local address.  Screwed up in commit bd269ebc due
> to last minute semantic conflict resolution.  Spotted by Coverity.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  util/qemu-sockets.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

Matches the fix I had proposed here against the v2 pull request:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01825.html

I guess in the confusion of the rebasing, you didn't quite implement it
in v3 the way I had proposed.

> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index d8183f7..b39ae74 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1338,12 +1338,14 @@ char *socket_address_to_string(struct SocketAddress 
> *addr, Error **errp)
>  
>  SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
>  {
> -SocketAddress *addr = g_new(SocketAddress, 1);
> +SocketAddress *addr;
>  
>  if (!addr_legacy) {
>  return NULL;
>  }
>  
> +addr = g_new(SocketAddress, 1);
> +
>  switch (addr_legacy->type) {
>  case SOCKET_ADDRESS_LEGACY_KIND_INET:
>  addr->type = SOCKET_ADDRESS_TYPE_INET;
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v9 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Max Reitz]

On 2017-05-15 16:04, Daniel P. Berrange wrote:
> The qemu-img dd/convert commands will create an image file and
> then try to open it. Historically it has been possible to open
> new files without passing any options. With encrypted files
> though, the *key-secret options are mandatory, so we need to
> provide those options when opening the newly created file.
> 
> Reviewed-by: Max Reitz 
> Reviewed-by: Fam Zheng 
> Reviewed-by: Eric Blake 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-img.c | 42 +-
>  1 file changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e0e3d31..dcddded 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -314,15 +314,18 @@ static BlockBackend *img_open_opts(const char *optstr,
>  }
>  
>  static BlockBackend *img_open_file(const char *filename,
> +   QDict *options,
> const char *fmt, int flags,
> bool writethrough, bool quiet,
> bool force_share)
>  {
>  BlockBackend *blk;
>  Error *local_err = NULL;
> -QDict *options = qdict_new();
>  
>  if (fmt) {
> +if (!options) {
> +options = qdict_new();
> +}

This is the only place where my attempted rebase and your version
differ. I think this has to be done unconditionally, because otherwise:

$ ./qemu-img info -U null-co://
[1]16327 segmentation fault (core dumped)  ./qemu-img info -U null-co://

Also, I'm not sure the R-bs apply for this patch any longer.

(They do for patch 1 because it's just a contextual difference. For
patch 2, it's a borderline case (I would drop it, but I can understand
keeping it). For patch 3 it's more than just borderline - I would
definitely drop the R-b, but the differences are still rather
mechanical, so it is acceptable to keep it.
But I think there are too many changes here in this patch to keep the
R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an
R-b to this segfault...)

Max

>  qdict_put_str(options, "driver", fmt);
>  }
>  
> @@ -344,6 +347,35 @@ static BlockBackend *img_open_file(const char *filename,
>  }
>  
>  
> +static int img_add_key_secrets(void *opaque,
> +   const char *name, const char *value,
> +   Error **errp)
> +{
> +QDict *options = opaque;
> +
> +if (g_str_has_suffix(name, "key-secret")) {
> +qdict_put(options, name, qstring_from_str(value));
> +}
> +
> +return 0;
> +}
> +
> +static BlockBackend *img_open_new_file(const char *filename,
> +   QemuOpts *create_opts,
> +   const char *fmt, int flags,
> +   bool writethrough, bool quiet,
> +   bool force_share)
> +{
> +QDict *options = NULL;
> +
> +options = qdict_new();
> +qemu_opt_foreach(create_opts, img_add_key_secrets, options, 
> _abort);
> +
> +return img_open_file(filename, options, fmt, flags, writethrough, quiet,
> + force_share);
> +}
> +
> +
>  static BlockBackend *img_open(bool image_opts,
>const char *filename,
>const char *fmt, int flags, bool writethrough,
> @@ -364,7 +396,7 @@ static BlockBackend *img_open(bool image_opts,
>  blk = img_open_opts(filename, opts, flags, writethrough, quiet,
>  force_share);
>  } else {
> -blk = img_open_file(filename, fmt, flags, writethrough, quiet,
> +blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet,
>  force_share);
>  }
>  return blk;
> @@ -2286,8 +2318,8 @@ static int img_convert(int argc, char **argv)
>   * That has to wait for bdrv_create to be improved
>   * to allow filenames in option syntax
>   */
> -s.target = img_open_file(out_filename, out_fmt, flags,
> - writethrough, quiet, false);
> +s.target = img_open_new_file(out_filename, opts, out_fmt,
> + flags, writethrough, quiet, false);
>  }
>  if (!s.target) {
>  ret = -1;
> @@ -4351,7 +4383,7 @@ static int img_dd(int argc, char **argv)
>   * with the bdrv_create() call above which does not
>   * support image-opts style.
>   */
> -blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
> +blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR,
>   false, false, false);
>  
>  if (!blk2) {
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert"  wrote:
> >> > * Juan Quintela (quint...@redhat.com) wrote:
> >> >> Markus Armbruster  wrote:
> >> >> > Juan Quintela  writes:
> >> >> >
> >> >> >> Eric Blake  wrote:
> >> >> 
> >> >> 
> >> >> >>> Or is the proposal that we are also going to simplify the QMP 
> >> >> >>> 'migrate'
> >> >> >>> command to get rid of crufty parameters?
> >> >> >>
> >> >> >> I didn't read it that way, but I would not oppose O:-)
> >> >> >>
> >> >> >> Later, Juan.
> >> >> >
> >> >> > I'm not too familiar with this stuff, so please correct my
> >> >> > misunderstandings.
> >> >> >
> >> >> > "Normal" migration configuration is global state, i.e. it applies to 
> >> >> > all
> >> >> > future migrations.
> >> >> >
> >> >> > Except the "migrate" command's flags apply to just the migration 
> >> >> > kicked
> >> >> > off by that command.
> >> >> >
> >> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: 
> >> >> > -i).
> >> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >> >> >
> >> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> >> >> 
> >> >> As qmp command is asynchronous, you can think that -d is *always* on in
> >> >> QMP O:-)
> >> >> 
> >> >> > You'd like to deprecate these flags in favour of "normal" 
> >> >> > configuration.
> >> >> > However, we need to maintain QMP backward compatibility at least for a
> >> >> > while.  HMP backward compatibility is nice to have, but not required.
> >> >> >
> >> >> > First step is to design the new interface you want.  Second step is to
> >> >> > figure out backward compatibility.
> >> >> >
> >> >> > The new interface adds a block migration tri-state (off,
> >> >> > non-incremental, incremental) to global state, default off.  Whether
> >> >> > it's done as two bools or an enum of three values doesn't matter here.
> >> >> 
> >> >> Tristates will complicate it.  I still think that:
> >> >> 
> >> >> - capability: block_migration
> >> >> - parameter: block_shared
> >> >> 
> >> >> Makes more sense, no?
> >> >
> >> > I don't understand what making block_shared a parameter gives you as
> >> > opposed to simply having two capabilities.
> >> >
> >> > (And how did we get 'shared'? We started off with block & incremental)
> >> 
> >> The variables on MigrationParams:
> >> 
> >> struct MigrationParams {
> >> bool blk;
> >> bool shared;
> >> };
> >> 
> >> 
> >> I can move to incremental.  I am not sure which one is clearer.
> >> 
> >> The advantage of having shared as a parameter is that we forget about
> >> all this dependency bussiness.  Is the same than compression_threads
> >> paramter, you setup to whichever value that you want.  But you don't get
> >> compression_threads until you set the compress capability.
> >> 
> >> So, in this case we will have:
> >> 
> >> block capability: Are we using block migration or not
> >> block-incremental parameter: If we are using block migration, are we
> >>   using incremental copying of the block layer?
> >
> > If it's still a boolean why does having it as a parameter remove the
> > dependency?
> 
> Forget -b/-i.
> 
> migration_set_parameter compression_threads 8
> 
> migrate 
> 
> We don't use compression_threads at all
> 
> migrate_set_capability compress
> 
> migrate 
> 
> Now, we use compression threads.
> 
> So, compression_threads parameter is a parameter that is only used when
> compress capability is enabled.
> 
> Same for block migration.  Block_incremental parameter is used only when
> block migration capability is setup.  No dependency check needed at all.
> 
> Or I am losing something obvious here?

Ah, you've made up a new rule - I don't think it's a bad rule
but is it true? Do we always enable a capability before we use a
parameter? I don't think so - I think the tls parameters don't have
a capability.
My previous rule was just that if it was a bool it was a capability
and you can have whatever dependencies you like there - or none.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] 9pfs: local: forbid client access to metadata (CVE-2017-7493)

[Eric Blake]

On 05/15/2017 11:07 AM, Greg Kurz wrote:
> When using the mapped-file security mode, we shouldn't let the client mess
> with the metadata. The current code already tries to hide the metadata dir
> from the client by skipping it in local_readdir(). But the client can still
> access or modify it through several other operations. This can be used to
> escalate privileges in the guest.
> 
> Affected backend operations are:
> - local_mknod()
> - local_mkdir()
> - local_open2()
> - local_symlink()
> - local_link()
> - local_unlinkat()
> - local_renameat()
> - local_rename()
> - local_name_to_path()
> 
> Other operations are safe because they are only passed a fid path, which
> is computed internally in local_name_to_path().
> 
> This patch converts all the functions listed above to fail and return
> EINVAL when being passed the name of the metadata dir. This may look
> like a poor choice for errno, but there's no such thing as an illegal
> path name on Linux and I could not think of anything better.
> 
> This fixes CVE-2017-7493.
> 
> Reported-by: Leo Gaspard 
> Signed-off-by: Greg Kurz 
> ---

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> "Dr. David Alan Gilbert"  wrote:
>> > * Juan Quintela (quint...@redhat.com) wrote:
>> >> Markus Armbruster  wrote:
>> >> > Juan Quintela  writes:
>> >> >
>> >> >> Eric Blake  wrote:
>> >> 
>> >> 
>> >> >>> Or is the proposal that we are also going to simplify the QMP 
>> >> >>> 'migrate'
>> >> >>> command to get rid of crufty parameters?
>> >> >>
>> >> >> I didn't read it that way, but I would not oppose O:-)
>> >> >>
>> >> >> Later, Juan.
>> >> >
>> >> > I'm not too familiar with this stuff, so please correct my
>> >> > misunderstandings.
>> >> >
>> >> > "Normal" migration configuration is global state, i.e. it applies to all
>> >> > future migrations.
>> >> >
>> >> > Except the "migrate" command's flags apply to just the migration kicked
>> >> > off by that command.
>> >> >
>> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >> >
>> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> >> 
>> >> As qmp command is asynchronous, you can think that -d is *always* on in
>> >> QMP O:-)
>> >> 
>> >> > You'd like to deprecate these flags in favour of "normal" configuration.
>> >> > However, we need to maintain QMP backward compatibility at least for a
>> >> > while.  HMP backward compatibility is nice to have, but not required.
>> >> >
>> >> > First step is to design the new interface you want.  Second step is to
>> >> > figure out backward compatibility.
>> >> >
>> >> > The new interface adds a block migration tri-state (off,
>> >> > non-incremental, incremental) to global state, default off.  Whether
>> >> > it's done as two bools or an enum of three values doesn't matter here.
>> >> 
>> >> Tristates will complicate it.  I still think that:
>> >> 
>> >> - capability: block_migration
>> >> - parameter: block_shared
>> >> 
>> >> Makes more sense, no?
>> >
>> > I don't understand what making block_shared a parameter gives you as
>> > opposed to simply having two capabilities.
>> >
>> > (And how did we get 'shared'? We started off with block & incremental)
>> 
>> The variables on MigrationParams:
>> 
>> struct MigrationParams {
>> bool blk;
>> bool shared;
>> };
>> 
>> 
>> I can move to incremental.  I am not sure which one is clearer.
>> 
>> The advantage of having shared as a parameter is that we forget about
>> all this dependency bussiness.  Is the same than compression_threads
>> paramter, you setup to whichever value that you want.  But you don't get
>> compression_threads until you set the compress capability.
>> 
>> So, in this case we will have:
>> 
>> block capability: Are we using block migration or not
>> block-incremental parameter: If we are using block migration, are we
>>   using incremental copying of the block layer?
>
> If it's still a boolean why does having it as a parameter remove the
> dependency?

Forget -b/-i.

migration_set_parameter compression_threads 8

migrate 

We don't use compression_threads at all

migrate_set_capability compress

migrate 

Now, we use compression threads.

So, compression_threads parameter is a parameter that is only used when
compress capability is enabled.

Same for block migration.  Block_incremental parameter is used only when
block migration capability is setup.  No dependency check needed at all.

Or I am losing something obvious here?

Later, Juan.

Re: [Qemu-devel] [PATCH] qemu-img: Fix documentation of convert

[Max Reitz]

On 2017-05-15 12:35, Fam Zheng wrote:
> It got lost in commit a8d16f9ca "qemu-img: Update documentation for -U".
> 
> Reported-by: Max Reitz 
> Signed-off-by: Fam Zheng 
> ---
>  qemu-img-cmds.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks!

Applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> >> Markus Armbruster  wrote:
> >> > Juan Quintela  writes:
> >> >
> >> >> Eric Blake  wrote:
> >> 
> >> 
> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >> >>> command to get rid of crufty parameters?
> >> >>
> >> >> I didn't read it that way, but I would not oppose O:-)
> >> >>
> >> >> Later, Juan.
> >> >
> >> > I'm not too familiar with this stuff, so please correct my
> >> > misunderstandings.
> >> >
> >> > "Normal" migration configuration is global state, i.e. it applies to all
> >> > future migrations.
> >> >
> >> > Except the "migrate" command's flags apply to just the migration kicked
> >> > off by that command.
> >> >
> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >> >
> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> >> 
> >> As qmp command is asynchronous, you can think that -d is *always* on in
> >> QMP O:-)
> >> 
> >> > You'd like to deprecate these flags in favour of "normal" configuration.
> >> > However, we need to maintain QMP backward compatibility at least for a
> >> > while.  HMP backward compatibility is nice to have, but not required.
> >> >
> >> > First step is to design the new interface you want.  Second step is to
> >> > figure out backward compatibility.
> >> >
> >> > The new interface adds a block migration tri-state (off,
> >> > non-incremental, incremental) to global state, default off.  Whether
> >> > it's done as two bools or an enum of three values doesn't matter here.
> >> 
> >> Tristates will complicate it.  I still think that:
> >> 
> >> - capability: block_migration
> >> - parameter: block_shared
> >> 
> >> Makes more sense, no?
> >
> > I don't understand what making block_shared a parameter gives you as
> > opposed to simply having two capabilities.
> >
> > (And how did we get 'shared'? We started off with block & incremental)
> 
> The variables on MigrationParams:
> 
> struct MigrationParams {
> bool blk;
> bool shared;
> };
> 
> 
> I can move to incremental.  I am not sure which one is clearer.
> 
> The advantage of having shared as a parameter is that we forget about
> all this dependency bussiness.  Is the same than compression_threads
> paramter, you setup to whichever value that you want.  But you don't get
> compression_threads until you set the compress capability.
> 
> So, in this case we will have:
> 
> block capability: Are we using block migration or not
> block-incremental parameter: If we are using block migration, are we
>   using incremental copying of the block layer?

If it's still a boolean why does having it as a parameter remove the
dependency?

Dave

> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] 答复： Re: [RFC] virtio-fc: draft idea of virtual fibre channel HBA

[Paolo Bonzini]

Thread necromancy after doing my homework and studying a bunch of specs...

>>> I'd propose to update
>>>
>>> struct virtio_scsi_config
>>> with a field 'u8 initiator_id[8]'
>>>
>>> and
>>>
>>> struct virtio_scsi_req_cmd
>>> with a field 'u8 target_id[8]'
>>>
>>> and do away with the weird LUN remapping qemu has nowadays.
>> Does it mean we dont need to provide '-drive' and '-device scsi-hd'
>> option in qemu command line? so the guest can get its own LUNs
>> through fc switch, right?
>
> No, you still would need that (at least initially).
> But with the modifications above we can add tooling around qemu to
> establish the correct (host) device mappings.
> Without it we
> a) have no idea from the host side which devices should be attached to
> any given guest
> b) have no idea from the guest side what the initiator and target IDs
> are; which will get _really_ tricky if someone decides to use persistent
> reservations from within the guest...
> 
> For handling NPIV proper we would need to update qemu
> a) locate the NPIV host based on the initiator ID from the guest

1) How would the initiator ID (8 bytes) relate to the WWNN/WWPN (2*8
bytes) on the host?  Likewise for the target ID which, as I understand
it, matches the rport's WWNN/WWPN in Linux's FC transport.

2) If the initiator ID is the moral equivalent of a MAC address,
shouldn't it be the host that provides the initiator ID to the host in
the virtio-scsi config space?  (From your proposal, I'd guess it's the
latter, but maybe I am not reading correctly).

3) An initiator ID in virtio-scsi config space is orthogonal to an
initiator IDs in the request.  The former is host->guest, the latter is
guest->host and can be useful to support virtual (nested) NPIV.

> b) stop exposing the devices attached to that NPIV host to the guest

What do you mean exactly?

> c) establish a 'rescan' routine to capture any state changes (LUN
> remapping etc) of the NPIV host.

You'd also need "target add" and "target removed" events.  At this
point, this looks a lot less virtio-scsi and a lot more like virtio-fc
(with a 'cooked' FCP-based format of its own).

At this point, I can think of several ways  to do this, one being SG_IO
in QEMU while the other are more exoteric.

1) use virtio-scsi with userspace passthrough (current solution).

Advantages:
- guests can be stopped/restarted across hosts with different HBAs
- completely oblivious to host HBA driver
- no new guest drivers are needed (well, almost due to above issues)
- out-of-the-box support for live migration, albeit with hacks required
such as Hyper-V's two WWNN/WWPN pairs

Disadvantages:
- no full FCP support
- guest devices exposed as /dev nodes to the host


2) the exact opposite: use the recently added "mediated device
passthrough" (mdev) framework to present a "fake" PCI device to the
guest.  mdev is currently used for vGPU and will also be used by s390
for CCW passthrough.  It lets the host driver take care of device
emulation, and the result is similar to an SR-IOV virtual function but
without requiring SR-IOV in the host.  The PCI device would presumably
reuse in the guest the same driver as the host.

Advantages:
- no new guest drivers are needed
- solution confined entirely within the host driver
- each driver can use its own native 'cooked' format for FC frames

Disadvantages:
- specific to each HBA driver
- guests cannot be stopped/restarted across hosts with different HBAs
- it's still device passthrough, so live migration is a mess (and would
require guest-specific code in QEMU)


3) handle passthrough with a kernel driver.  Under this model, the guest
uses the virtio device, but the passthrough of commands and TMFs is
performed by the host driver.  The host driver grows the option to
present an NPIV vport through a vhost interface (*not* the same thing as
LIO's vhost-scsi target, but a similar API with a different /dev node or
even one node per scsi_host).

We can then choose whether to do it with virtio-scsi or with a new
virtio-fc.

Advantages:
- guests can be stopped/restarted across hosts with different HBAs
- no need to do the "two WWNN/WWPN pairs" hack for live migration,
unlike e.g. Hyper-V
- a bit Rube Goldberg, but the vhost interface can be consumed by any
userspace program, not just by virtual machines

Disadvantages:
- requires a new generalized vhost-scsi (or vhost-fc) layer
- not sure about support for live migration (what to do about in-flight
commands?)

I don't know the Linux code well enough to know if it would require code
specific to each HBA driver.  Maybe just some refactoring.


4) same as (3), but in userspace with a "macvtap" like layer (e.g.,
socket+bind creates an NPIV vport).  This layer can work on some kind of
FCP encapsulation, not the raw thing, and virtio-fc could be designed
according to a similar format for simplicity.

Advantages:
- less dependencies on kernel code
- simplest for live migration
- most flexible for userspace usage

Disadvantages:
- possibly two 

Re: [Qemu-devel] [PULL 0/5] Migration pull request

[Juan Quintela]

Stefan Hajnoczi  wrote:
> On Fri, May 12, 2017 at 05:52:44PM +0200, Juan Quintela wrote:
>> Hi
>> 
>> 
>
> Thanks, applied to my staging tree:
> https://github.com/stefanha/qemu/commits/staging

Hi

As fast as I sent the PULL request, more comments appeared.

What do you preffer?
- drop the whole PULL request?
- drop the patch:  migration: Create block capabilities for shared and enable
  it has zero conflicts with the other patches
- let the PULL alone and I will sent fixes on top of that?

I would preffer option two, but that is up to you.

Sorry for the inconvenience.

Later, Juan.

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

[Halil Pasic]

On 05/10/2017 08:02 PM, Halil Pasic wrote:
> 
> 
> On 04/22/2017 08:23 AM, Gonglei wrote:
>> The virtio crypto device is a virtual crypto device (ie. hardware
>> crypto accelerator card). Currently, the virtio crypto device provides
>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
>>
>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>>
>> VIRTIO-153
>>
>> Signed-off-by: Gonglei 
>> CC: Michael S. Tsirkin 
>> CC: Cornelia Huck 
>> CC: Stefan Hajnoczi 
>> CC: Lingli Deng 
>> CC: Jani Kokkonen 
>> CC: Ola Liljedahl 
>> CC: Varun Sethi 
>> CC: Zeng Xin 
>> CC: Keating Brian 
>> CC: Ma Liang J 
>> CC: Griffin John 
>> CC: Mihai Claudiu Caraman 
>> CC: Halil Pasic 
>> ---
>>  acknowledgements.tex |2 +
>>  content.tex  |2 +
>>  virtio-crypto.tex| 1309 
>> ++
>>  3 files changed, 1313 insertions(+)
>>  create mode 100644 virtio-crypto.tex
>>
>> diff --git a/acknowledgements.tex b/acknowledgements.tex
>> index 53942b0..43b8a9b 100644
>> --- a/acknowledgements.tex
>> +++ b/acknowledgements.tex
>> @@ -26,6 +26,7 @@ Sasha Levin,   Oracle  \newline
>>  Sergey Tverdyshev,  Thales e-Security   \newline
>>  Stefan Hajnoczi,Red Hat \newline
>>  Tom Lyon,   Samya Systems, Inc. \newline
>> +Lei Gong,  Huawei   \newline
>>  \end{oasistitlesection}
>>
>>  The following non-members have provided valuable feedback on this
>> @@ -44,4 +45,5 @@ Patrick Durusau,   Technical Advisory Board, OASIS \newline
>>  Thomas Huth,Red Hat \newline
>>  Yan Vugenfirer, Red Hat / Daynix\newline
>>  Kevin Lo,   MSI \newline
>> +Halil Pasic, IBM  \newline
>>  \end{oasistitlesection}
>> diff --git a/content.tex b/content.tex
>> index 4b45678..ab75f78 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len}, \field{residual},
>>  \field{status_qualifier}, \field{status}, \field{response} and
>>  \field{sense} fields.
>>
>> +\input{virtio-crypto.tex}
>> +
>>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>
>>  Currently there are three device-independent feature bits defined:
>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>> new file mode 100644
>> index 000..2708023
>> --- /dev/null
>> +++ b/virtio-crypto.tex
>> @@ -0,0 +1,1309 @@
>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
>> +
>> +The virtio crypto device is a virtual cryptography device as well as a kind 
>> of
>> +virtual hardware accelerator for virtual machines. The encryption and
>> +decryption requests are placed in any of the data queues and are ultimately 
>> handled by the
>> +backend crypto accelerators. The second kind of queue is the control queue 
>> used to create 
>> +or destroy sessions for symmetric algorithms and will control some advanced
>> +features in the future. The virtio crypto device provides the following 
>> crypto
>> +services: CIPHER, MAC, HASH, and AEAD.
>> +
>> +
>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
>> +
>> +20
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] dataq1
>> +\item[\ldots]
>> +\item[N-1] dataqN
>> +\item[N] controlq
>> +\end{description}
>> +
>> +N is set by \field{max_dataqueues}.
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature 
>> bits}
>> +
>> +VIRTIO_CRYPTO_F_STATELESS_MODE (0) stateless mode is available.
>> +VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE (1) stateless mode is available for 
>> CIPHER service.
>> +VIRTIO_CRYPTO_F_HASH_STATELESS_MODE (2) stateless mode is available for 
>> HASH service.
>> +VIRTIO_CRYPTO_F_MAC_STATELESS_MODE (3) stateless mode is available for MAC 
>> service.
>> +VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE (4) stateless mode is available for 
>> AEAD service.
>> +
>> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto 
>> Device / Feature bits}
>> +
>> +Some crypto feature bits require other crypto feature bits
>> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature 
>> Bits}):
>> +
>> +\begin{description}
>> +\item[VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE] Requires 
>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>> +\item[VIRTIO_CRYPTO_F_HASH_STATELESS_MODE] Requires 
>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>> +\item[VIRTIO_CRYPTO_F_MAC_STATELESS_MODE] Requires 
>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>> +\item[VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE] Requires 
>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>> +\end{description}
>> +
>> +\subsection{Supported crypto services}\label{sec:Device Types / Crypto 
>> 

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Markus Armbruster  wrote:
>> > Juan Quintela  writes:
>> >
>> >> Eric Blake  wrote:
>> 
>> 
>> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>> >>> command to get rid of crufty parameters?
>> >>
>> >> I didn't read it that way, but I would not oppose O:-)
>> >>
>> >> Later, Juan.
>> >
>> > I'm not too familiar with this stuff, so please correct my
>> > misunderstandings.
>> >
>> > "Normal" migration configuration is global state, i.e. it applies to all
>> > future migrations.
>> >
>> > Except the "migrate" command's flags apply to just the migration kicked
>> > off by that command.
>> >
>> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >
>> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> 
>> As qmp command is asynchronous, you can think that -d is *always* on in
>> QMP O:-)
>> 
>> > You'd like to deprecate these flags in favour of "normal" configuration.
>> > However, we need to maintain QMP backward compatibility at least for a
>> > while.  HMP backward compatibility is nice to have, but not required.
>> >
>> > First step is to design the new interface you want.  Second step is to
>> > figure out backward compatibility.
>> >
>> > The new interface adds a block migration tri-state (off,
>> > non-incremental, incremental) to global state, default off.  Whether
>> > it's done as two bools or an enum of three values doesn't matter here.
>> 
>> Tristates will complicate it.  I still think that:
>> 
>> - capability: block_migration
>> - parameter: block_shared
>> 
>> Makes more sense, no?
>
> I don't understand what making block_shared a parameter gives you as
> opposed to simply having two capabilities.
>
> (And how did we get 'shared'? We started off with block & incremental)

The variables on MigrationParams:

struct MigrationParams {
bool blk;
bool shared;
};


I can move to incremental.  I am not sure which one is clearer.

The advantage of having shared as a parameter is that we forget about
all this dependency bussiness.  Is the same than compression_threads
paramter, you setup to whichever value that you want.  But you don't get
compression_threads until you set the compress capability.

So, in this case we will have:

block capability: Are we using block migration or not
block-incremental parameter: If we are using block migration, are we
  using incremental copying of the block layer?


Later, Juan.

Re: [Qemu-devel] [Qemu-devel PATCH 4/5] msf2: Add Smartfusion2 SoC.

[Alistair Francis]

 On Thu, May 11, 2017 at 10:02 PM, Philippe Mathieu-Daudé
 wrote:
> On 05/12/2017 12:17 AM, sundeep subbaraya wrote:
>>
>> Hi Philippe,
>>
>> On Wed, May 10, 2017 at 5:20 PM, Philippe Mathieu-Daudé 
>> wrote:
>>
>>> Hi Subbaraya,
>>>
>>>
>>> On 05/09/2017 01:44 PM, Subbaraya Sundeep wrote:
>>>
 Smartfusion2 SoC has hardened Microcontroller subsystem
 and flash based FPGA fabric. This patch adds support for
 Microcontroller subsystem in the SoC.

 Signed-off-by: Subbaraya Sundeep 
 ---
  default-configs/arm-softmmu.mak |   1 +
  hw/arm/Makefile.objs|   2 +-
  hw/arm/msf2-soc.c   | 188 ++
 ++
  include/hw/arm/msf2-soc.h   |  60 +
  4 files changed, 250 insertions(+), 1 deletion(-)
  create mode 100644 hw/arm/msf2-soc.c
  create mode 100644 include/hw/arm/msf2-soc.h

 diff --git a/default-configs/arm-softmmu.mak
 b/default-configs/arm-softmmu.mak
 index 78d7af0..7062512 100644
 --- a/default-configs/arm-softmmu.mak
 +++ b/default-configs/arm-softmmu.mak
 @@ -122,3 +122,4 @@ CONFIG_ACPI=y
  CONFIG_SMBIOS=y
  CONFIG_ASPEED_SOC=y
  CONFIG_GPIO_KEY=y
 +CONFIG_MSF2=y
 diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
 index 4c5c4ee..ae5e4a3 100644
 --- a/hw/arm/Makefile.objs
 +++ b/hw/arm/Makefile.objs
 @@ -1,7 +1,7 @@
  obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
  obj-$(CONFIG_DIGIC) += digic_boards.o
  obj-y += integratorcp.o mainstone.o musicpal.o nseries.o
 -obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
 +obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o msf2-soc.o

>>>
>>> Not a big deal, but since you added CONFIG_MSF2 why not using it here and
>>> the Makefiles you touched (misc/ssi/timer)?
>>>
>>> obj-$(CONFIG_MSF2) += msf2-soc.o
>>>
>>>   OK. Will change it.
>>
>>
>>>
>>>  obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o

  obj-$(CONFIG_ACPI) += virt-acpi-build.o
  obj-y += netduino2.o
>
> [...]

 +MemoryRegion *system_memory = get_system_memory();
 +MemoryRegion *nvm = g_new(MemoryRegion, 1);
 +MemoryRegion *nvm_alias = g_new(MemoryRegion, 1);
 +MemoryRegion *sram = g_new(MemoryRegion, 1);
 +MemoryRegion *ddr = g_new(MemoryRegion, 1);
 +
 +memory_region_init_ram(nvm, NULL, "MSF2.envm", ENVM_SIZE,
 +   _fatal);

>>>
>>> Maybe you can name it "eNVM" to match the documentation.
>>>
>>> Also envm_size should be a per-model property.
>>>
>>
>> Ok.
>>
>>>
>>> +memory_region_init_alias(nvm_alias, NULL, "MSF2.flash.alias",

 + nvm, 0, ENVM_SIZE);

>>>
>>> Hmmm well this would be the "Cache Matrix Remap" which happens to be
>>> mapped by default to eNVM on cold reset.
>>> Naming it "MSF2.flash.alias" is pretty confusing.
>>>
>>
>> Exactly it is Cache Matrix Remap.
>> AFAIK currently we cannot remap memory during runtime in Qemu.
>> So I handled default remap with alias.
>> Please suggest the name. MSF2.eNVM.alias sounds fine?
>
>
> Hmm Peter, Francis?
>
> Personally I prefer "bus_remap.alias" which is explicit.
>
> "eNVM.alias" is only true on Cold Reset.

Yeah, I'm pretty sure you can't remap memory (unless that is some new
feature I missed).

Creating an alias seems like the right idea (you can even
enable/disable it as needed to pretend we have dynamic remapping).

As for names usually just copy the data sheet. MSF2.eNVM.alias sounds
fine to me.

Thanks,

Alistair

>
>>>
>>> +vmstate_register_ram_global(nvm);

 +
 +memory_region_set_readonly(nvm, true);
 +memory_region_set_readonly(nvm_alias, true);
 +
 +memory_region_add_subregion(system_memory, ENVM_BASE_ADDRESS, nvm);
 +memory_region_add_subregion(system_memory, 0, nvm_alias);
 +
 +memory_region_init_ram(ddr, NULL, "MSF2.ddr", DDR_SIZE,
 +   _fatal);

>>>
>>> Wrong, there is no DDR on this SoC.
>>>
>> DDR controller is there in Smartfusion2 (different from Smartfusion). As
>> you said below this
>> should be in board file.
>
>
> There IS a DDRC in this SoC, but here you are registering a DDR 'ram' memory
> region, not a controller. This SoC can be used without any DDR, enough using
> embedded eNVM and eSRAM.
>
> Now it happens your SoM board provides a DDR chip connected to this SoC.
>
>>>
>>> +vmstate_register_ram_global(ddr);

 +memory_region_add_subregion(system_memory, DDR_BASE_ADDRESS, ddr);
 +
 +memory_region_init_ram(sram, NULL, "MSF2.sram", SRAM_SIZE,
 +   _fatal);

>>>
>>> I'd rather like to see it named "eSRAM" somehow, so there is no confusion
>>> possible with external SRAM a SoM/board can map at 0x6000.
>>>
>>> 

[Qemu-devel] [PATCH v10 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

The qemu-img dd/convert commands will create an image file and
then try to open it. Historically it has been possible to open
new files without passing any options. With encrypted files
though, the *key-secret options are mandatory, so we need to
provide those options when opening the newly created file.

Reviewed-by: Max Reitz 
Reviewed-by: Fam Zheng 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.c | 42 +-
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e0e3d31..0bf941b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -314,14 +314,17 @@ static BlockBackend *img_open_opts(const char *optstr,
 }
 
 static BlockBackend *img_open_file(const char *filename,
+   QDict *options,
const char *fmt, int flags,
bool writethrough, bool quiet,
bool force_share)
 {
 BlockBackend *blk;
 Error *local_err = NULL;
-QDict *options = qdict_new();
 
+if (!options) {
+options = qdict_new();
+}
 if (fmt) {
 qdict_put_str(options, "driver", fmt);
 }
@@ -344,6 +347,35 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+   const char *name, const char *value,
+   Error **errp)
+{
+QDict *options = opaque;
+
+if (g_str_has_suffix(name, "key-secret")) {
+qdict_put(options, name, qstring_from_str(value));
+}
+
+return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+   QemuOpts *create_opts,
+   const char *fmt, int flags,
+   bool writethrough, bool quiet,
+   bool force_share)
+{
+QDict *options = NULL;
+
+options = qdict_new();
+qemu_opt_foreach(create_opts, img_add_key_secrets, options, _abort);
+
+return img_open_file(filename, options, fmt, flags, writethrough, quiet,
+ force_share);
+}
+
+
 static BlockBackend *img_open(bool image_opts,
   const char *filename,
   const char *fmt, int flags, bool writethrough,
@@ -364,7 +396,7 @@ static BlockBackend *img_open(bool image_opts,
 blk = img_open_opts(filename, opts, flags, writethrough, quiet,
 force_share);
 } else {
-blk = img_open_file(filename, fmt, flags, writethrough, quiet,
+blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet,
 force_share);
 }
 return blk;
@@ -2286,8 +2318,8 @@ static int img_convert(int argc, char **argv)
  * That has to wait for bdrv_create to be improved
  * to allow filenames in option syntax
  */
-s.target = img_open_file(out_filename, out_fmt, flags,
- writethrough, quiet, false);
+s.target = img_open_new_file(out_filename, opts, out_fmt,
+ flags, writethrough, quiet, false);
 }
 if (!s.target) {
 ret = -1;
@@ -4351,7 +4383,7 @@ static int img_dd(int argc, char **argv)
  * with the bdrv_create() call above which does not
  * support image-opts style.
  */
-blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
+blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR,
  false, false, false);
 
 if (!blk2) {
-- 
2.9.3

[Qemu-devel] [PATCH v10 2/4] qemu-img: fix --image-opts usage with dd command

[Daniel P. Berrange]

The --image-opts flag can only be used to affect the parsing
of the source image. The target image has to be specified in
the traditional style regardless, since it needs to be passed
to the bdrv_create() API which does not support the new style
opts.

Reviewed-by: Fam Zheng 
Reviewed-by: Max Reitz 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 181f499..4dc1d56 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4316,8 +4316,13 @@ static int img_dd(int argc, char **argv)
 goto out;
 }
 
-blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
-false, false, false);
+/* TODO, we can't honour --image-opts for the target,
+ * since it needs to be given in a format compatible
+ * with the bdrv_create() call above which does not
+ * support image-opts style.
+ */
+blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
+ false, false, false);
 
 if (!blk2) {
 ret = -1;
-- 
2.9.3

[Qemu-devel] [PATCH v10 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

The '--image-opts' flag indicates whether the source filename
includes options. The target filename has to remain in the
plain filename format though, since it needs to be passed to
bdrv_create().  When using --skip-create though, it would be
possible to use image-opts syntax. This adds --target-image-opts
to indicate that the target filename includes options. Currently
this mandates use of the --skip-create flag too.

Signed-off-by: Daniel P. Berrange 
---
 qemu-img-cmds.hx |  4 +--
 qemu-img.c   | 84 ++--
 qemu-img.texi| 12 ++--
 3 files changed, 69 insertions(+), 31 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 469ec12..c298331 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,9 +40,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [-U] [-c] [-p] [-q] [-n] [-f 
fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] 
[-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m 
num_coroutines] [-W] filename [filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
[-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S 
sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] 
output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-U] [-c] [-p] [-q] 
[-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] 
[-B @var{backing_file}][-o @var{options}] [-s @var{snapshot_id_or_name}] [-l 
@var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] 
@var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] 
[-O @var{output_fmt}] [-B @var{backing_file}][-o @var{options}] [-s 
@var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m 
@var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] 
@var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
diff --git a/qemu-img.c b/qemu-img.c
index 4dc1d56..e0e3d31 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -60,6 +60,7 @@ enum {
 OPTION_PATTERN = 260,
 OPTION_FLUSH_INTERVAL = 261,
 OPTION_NO_DRAIN = 262,
+OPTION_TARGET_IMAGE_OPTS = 263,
 };
 
 typedef enum OutputFormat {
@@ -1913,10 +1914,10 @@ static int convert_do_copy(ImgConvertState *s)
 static int img_convert(int argc, char **argv)
 {
 int c, bs_i, flags, src_flags = 0;
-const char *fmt = NULL, *out_fmt = "raw", *cache = "unsafe",
+const char *fmt = NULL, *out_fmt = NULL, *cache = "unsafe",
*src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL,
*out_filename, *out_baseimg_param, *snapshot_name = NULL;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv = NULL, *proto_drv = NULL;
 BlockDriverInfo bdi;
 BlockDriverState *out_bs;
 QemuOpts *opts = NULL, *sn_opts = NULL;
@@ -1924,7 +1925,7 @@ static int img_convert(int argc, char **argv)
 char *options = NULL;
 Error *local_err = NULL;
 bool writethrough, src_writethrough, quiet = false, image_opts = false,
- skip_create = false, progress = false;
+ skip_create = false, progress = false, tgt_image_opts = false;
 int64_t ret = -EINVAL;
 bool force_share = false;
 
@@ -1942,6 +1943,7 @@ static int img_convert(int argc, char **argv)
 {"object", required_argument, 0, OPTION_OBJECT},
 {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
 {"force-share", no_argument, 0, 'U'},
+{"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
 {0, 0, 0, 0}
 };
 c = getopt_long(argc, argv, ":hf:O:B:ce6o:s:l:S:pt:T:qnm:WU",
@@ -2062,9 +2064,16 @@ static int img_convert(int argc, char **argv)
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
+case OPTION_TARGET_IMAGE_OPTS:
+tgt_image_opts = true;
+break;
 }
 }
 
+if (!out_fmt && !tgt_image_opts) {
+out_fmt = "raw";
+}
+
 if (qemu_opts_foreach(_object_opts,
   user_creatable_add_opts_foreach,
   NULL, NULL)) {
@@ -2076,12 +2085,22 @@ static int img_convert(int argc, char **argv)
 goto fail_getopt;
 }
 
+if (tgt_image_opts && !skip_create) {
+error_report("--target-image-opts requires use of -n flag");
+goto fail_getopt;
+}
+
 s.src_num = argc - optind - 1;
 out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
 
 if (options && has_help_option(options)) {
-ret = print_block_option_help(out_filename, out_fmt);
-goto fail_getopt;
+if 

[Qemu-devel] [PATCH v10 1/4] qemu-img: add support for --object with 'dd' command

[Daniel P. Berrange]

The qemu-img dd command added --image-opts support, but missed
the corresponding --object support. This prevented passing
secrets (eg auth passwords) needed by certain disk images.

Reviewed-by: Fam Zheng 
Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index b506839..181f499 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4158,6 +4158,7 @@ static int img_dd(int argc, char **argv)
 };
 const struct option long_options[] = {
 { "help", no_argument, 0, 'h'},
+{ "object", required_argument, 0, OPTION_OBJECT},
 { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
 { "force-share", no_argument, 0, 'U'},
 { 0, 0, 0, 0 }
@@ -4186,6 +4187,15 @@ static int img_dd(int argc, char **argv)
 case 'U':
 force_share = true;
 break;
+case OPTION_OBJECT: {
+QemuOpts *opts;
+opts = qemu_opts_parse_noisily(_object_opts,
+   optarg, true);
+if (!opts) {
+ret = -1;
+goto out;
+}
+}   break;
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
@@ -4230,6 +4240,14 @@ static int img_dd(int argc, char **argv)
 ret = -1;
 goto out;
 }
+
+if (qemu_opts_foreach(_object_opts,
+  user_creatable_add_opts_foreach,
+  NULL, NULL)) {
+ret = -1;
+goto out;
+}
+
 blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
 force_share);
 
-- 
2.9.3

Re: [Qemu-devel] Changes to Broadcom(BCM) files and Raspberry Pi files. Addition of PanelEmu

[Alistair Francis]

On Sat, May 13, 2017 at 5:42 PM, John Bradley via Qemu-devel
 wrote:
> From 7f74f048f135d9c9c230a9e90f72451c841c6d35 Mon Sep 17 00:00:00 2001
> From: John Bradley 
> Date: Sat, 13 May 2017 23:07:47 +0100
> Subject: [PATCH] Changes to Broadcom(BCM) files and Raspberry Pi files.
> Addition of PanelEmu
>
> The files add the ability to attach, via TCP, a panel emulator
> The include a unification of several PD Raspberry PI additions
> A modification to dev-network to all circle SDK WWW client to work
> The DummyPanel is not included but available at
> https://github.com/flypie/GDummyPanel.git
>
> Signed-off-by: John Bradley 

Hey John,

Thanks for the patch!

Unfortunately this patch is too long to review, you need to split the
patch up into shorter more readable patches. Otherwise it's too hard
to people to understand what you are changing and why.

There are some details here:
http://wiki.qemu.org/Contribute/SubmitAPatch about how to split up
patches. Each patch applied in order shouldn't break any compilation
or runtime. Generally the flow is to add the logic in earlier patches
and then connect it and switch it on in the later patches.

Try splitting up adding/editing each individual device and send that
our first. That is generally the easiest to review/accept.

Thanks,

Alistair

> ---
> .gitignore   |   54 ++
> hw/arm/Makefile.objs |2 +-
> hw/arm/bcm2835.c |  114 
> hw/arm/bcm2835_peripherals.c |  104 
> hw/arm/bcm2836.c |3 +-
> hw/arm/raspi.c   |   77 ++-
> hw/gpio/bcm2835_gpio.c   |  333 ++-
> hw/misc/Makefile.objs|2 +
> hw/misc/bcm2835_mphi.c   |  163 ++
> hw/misc/bcm2835_power.c  |  106 
> hw/timer/Makefile.objs   |2 +
> hw/timer/bcm2835_st.c|  202 +++
> hw/timer/bcm2835_timer.c |  224 +++
> hw/usb/Makefile.objs |4 +-
> hw/usb/bcm2835_usb.c |  604 +++
> hw/usb/bcm2835_usb_regs.h| 1061 ++
> hw/usb/dev-network.c |2 +-
> include/hw/arm/bcm2835.h |   37 ++
> include/hw/arm/bcm2835_peripherals.h |   10 +
> include/hw/gpio/bcm2835_gpio.h   |5 +
> include/hw/intc/bcm2835_control.h|   53 ++
> include/hw/intc/bcm2836_control.h|2 +
> include/hw/misc/bcm2835_mphi.h   |   28 +
> include/hw/misc/bcm2835_power.h  |   22 +
> include/hw/timer/bcm2835_st.h|   25 +
> include/hw/timer/bcm2835_timer.h |   32 +
> include/hw/usb/bcm2835_usb.h |   78 +++
> include/qemu/PanelEmu.h  |   53 ++
> util/Makefile.objs   |1 +
> util/PanelEmu.c  |  293 ++
> 30 files changed, 3547 insertions(+), 149 deletions(-)
> create mode 100644 hw/arm/bcm2835.c
> create mode 100644 hw/misc/bcm2835_mphi.c
> create mode 100644 hw/misc/bcm2835_power.c
> create mode 100644 hw/timer/bcm2835_st.c
> create mode 100644 hw/timer/bcm2835_timer.c
> create mode 100644 hw/usb/bcm2835_usb.c
> create mode 100644 hw/usb/bcm2835_usb_regs.h
> create mode 100644 include/hw/arm/bcm2835.h
> create mode 100644 include/hw/intc/bcm2835_control.h
> create mode 100644 include/hw/misc/bcm2835_mphi.h
> create mode 100644 include/hw/misc/bcm2835_power.h
> create mode 100644 include/hw/timer/bcm2835_st.h
> create mode 100644 include/hw/timer/bcm2835_timer.h
> create mode 100644 include/hw/usb/bcm2835_usb.h
> create mode 100644 include/qemu/PanelEmu.h
> create mode 100644 util/PanelEmu.c
>

[Qemu-devel] [PATCH v10 0/4] Improve convert and dd commands

[Daniel P. Berrange]

Update to

  v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05699.html
  v2: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00728.html
  v3: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04391.html
  v4: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02153.html
  v5: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04109.html
  v6: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00215.html
  v7: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00242.html
  v8: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg02028.html
  v9: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03504.html

This series is in response to Max pointing out that you cannot
use 'convert' for an encrypted target image.

The 'convert' and 'dd' commands need to first create the image
and then open it. The bdrv_create() method takes a set of options
for creating the image, which let us provide a key-secret for the
encryption key. When the commands then open the new image, they
don't provide any options, so the image is unable to be opened
due to lack of encryption key. It is also not possible to use
the --image-opts argument to provide structured options in the
target image name - it must be a plain filename to satisfy the
bdrv_create() API contract.

This series addresses these problems to some extent

 - Adds a new --target-image-opts flag which is used to say
   that the target filename is using structured options.
   It is *only* permitted to use this when -n is also set.
   ie the target image must be pre-created so convert/dd
   don't need to run bdrv_create().

 - When --target-image-opts is not used, add special case
   code that identifies options passed to bdrv_create()
   named "*key-secret" and adds them to the options used
   to open the new image

In future it is desirable to make --target-image-opts work even when -n is
*not* given. This requires considerable work to create a new bdrv_create()
API impl.

The first patch fixes a bug in the 'dd' command while the second adds support
for the missing '--object' arg to 'dd', allowing it to reference secrets when
opening files.  The last two patches implement the new features described above
for the 'convert' command.

Changed in v10:

 - Fixed crash in qemu-img when locking flag is set, without
   fmt being set (seen when running iotest 153).

Changed in v9:

 - Rebased against Kevin's "block" branch with Fam's patch
   "qemu-img: Fix documentation of convert" applied first

Changed in v8:

 - Readd accidentally dropped check for compression (Max)
 - Fix indentation of variable declaration (Max)
 - Fix goto jump target (Max)

Changed in v7:

 - Drop the (accidentally included) revert patch (Eric)

Changed in v6:

 - Fix misc typos (Fam)
 - Resolve messy conflicts wrt max/block-next (Max)

Changed in v5:

 - Fix return value (Max)
 - Misc doc changes (Max)
 - Use error_abort (Max)

Changed in v4:

 - Refactor img_open_new_file in terms of img_open_file (Kevin)

Changed in v3:

 - Drop all patches affecting the 'dd' command except for the clear bug fix
   and the --object support. They can be re-considered once dd is rewritten
   to run ontop of convert.
 - Use consistent return/goto style in dd command (Max)
 - Fix error reporting when using compressed image and skip-create (Max)
 - Unconditionally create QDict when open files (Max)

Changed in v2:

 - Replace dd -n flag with support for conv=nocreat,notrunc
 - Misc typos (Eric, Fam)


Daniel P. Berrange (4):
  qemu-img: add support for --object with 'dd' command
  qemu-img: fix --image-opts usage with dd command
  qemu-img: introduce --target-image-opts for 'convert' command
  qemu-img: copy *key-secret opts when opening newly created files

 qemu-img-cmds.hx |   4 +-
 qemu-img.c   | 147 +++
 qemu-img.texi|  12 -
 3 files changed, 128 insertions(+), 35 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH] sockets: Plug memory leak in socket_address_flatten()

[Markus Armbruster]

socket_address_flatten() leaks a SocketAddress when its argument is
null.  Happens when opening a ChardevBackend of type 'udp' that is
configured without a local address.  Screwed up in commit bd269ebc due
to last minute semantic conflict resolution.  Spotted by Coverity.

Signed-off-by: Markus Armbruster 
---
 util/qemu-sockets.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f7..b39ae74 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1338,12 +1338,14 @@ char *socket_address_to_string(struct SocketAddress 
*addr, Error **errp)
 
 SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
 {
-SocketAddress *addr = g_new(SocketAddress, 1);
+SocketAddress *addr;
 
 if (!addr_legacy) {
 return NULL;
 }
 
+addr = g_new(SocketAddress, 1);
+
 switch (addr_legacy->type) {
 case SOCKET_ADDRESS_LEGACY_KIND_INET:
 addr->type = SOCKET_ADDRESS_TYPE_INET;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> Markus Armbruster  wrote:
> > Juan Quintela  writes:
> >
> >> Eric Blake  wrote:
> 
> 
> >>> Or is the proposal that we are also going to simplify the QMP 'migrate'
> >>> command to get rid of crufty parameters?
> >>
> >> I didn't read it that way, but I would not oppose O:-)
> >>
> >> Later, Juan.
> >
> > I'm not too familiar with this stuff, so please correct my
> > misunderstandings.
> >
> > "Normal" migration configuration is global state, i.e. it applies to all
> > future migrations.
> >
> > Except the "migrate" command's flags apply to just the migration kicked
> > off by that command.
> >
> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> > !blk && inc makes no sense and is silently treated like !blk && !inc.
> >
> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
> 
> As qmp command is asynchronous, you can think that -d is *always* on in
> QMP O:-)
> 
> > You'd like to deprecate these flags in favour of "normal" configuration.
> > However, we need to maintain QMP backward compatibility at least for a
> > while.  HMP backward compatibility is nice to have, but not required.
> >
> > First step is to design the new interface you want.  Second step is to
> > figure out backward compatibility.
> >
> > The new interface adds a block migration tri-state (off,
> > non-incremental, incremental) to global state, default off.  Whether
> > it's done as two bools or an enum of three values doesn't matter here.
> 
> Tristates will complicate it.  I still think that:
> 
> - capability: block_migration
> - parameter: block_shared
> 
> Makes more sense, no?

I don't understand what making block_shared a parameter gives you as
opposed to simply having two capabilities.

(And how did we get 'shared'? We started off with block & incremental)

Dave

> If block_migration is not enabled, we ignore the shared parameter.  We
> already do that for other parameters.
> 
> > If the new interface isn't used, the old one still needs to work.  If it
> > is used, the old one either has to do "the right thing", or fail
> > cleanly.
> >
> > We approximate "new interface isn't used" by "block migration is off in
> > global state".  When it is off, the migration command needs to honor its
> > two flags for compatibility.  It must leave block migration off in
> > global state.  Yes, this will complicate the implementation until we
> > actually remove the deprecated flags.  Par for the backward compatility
> > course.
> >
> > When block migration isn't off in global state, we can either
> >
> > * let the flags take precedence over the global state (one
> >   interpretation of "do the right thing"), or
> >
> > * reject flags that conflict with global state (another interpretation),
> >   or
> >
> > * reject *all* flags (fail cleanly).
> >
> > The last one looks perfectly servicable to me.
> 
> Yeap,  I think that makes sense.  If you use capabilities, parameters,
> old interface don't work at all.
> 
> We still have a problem that is what happens if the user does:
> 
> migrate -b 
> migrate_cancel (or error)
> migrate  (without -b)
> 
> With current patches, it will still use -b.  Fixing it requires still
> anding more code.  But I think that this use case is so weird what we
> should not even care about it.
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> Eric Blake  wrote:


>>> Or is the proposal that we are also going to simplify the QMP 'migrate'
>>> command to get rid of crufty parameters?
>>
>> I didn't read it that way, but I would not oppose O:-)
>>
>> Later, Juan.
>
> I'm not too familiar with this stuff, so please correct my
> misunderstandings.
>
> "Normal" migration configuration is global state, i.e. it applies to all
> future migrations.
>
> Except the "migrate" command's flags apply to just the migration kicked
> off by that command.
>
> QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
> !blk && inc makes no sense and is silently treated like !blk && !inc.
>
> There's a third flag "detach" (HMP: -d), but it does nothing in QMP.

As qmp command is asynchronous, you can think that -d is *always* on in
QMP O:-)

> You'd like to deprecate these flags in favour of "normal" configuration.
> However, we need to maintain QMP backward compatibility at least for a
> while.  HMP backward compatibility is nice to have, but not required.
>
> First step is to design the new interface you want.  Second step is to
> figure out backward compatibility.
>
> The new interface adds a block migration tri-state (off,
> non-incremental, incremental) to global state, default off.  Whether
> it's done as two bools or an enum of three values doesn't matter here.

Tristates will complicate it.  I still think that:

- capability: block_migration
- parameter: block_shared

Makes more sense, no?

If block_migration is not enabled, we ignore the shared parameter.  We
already do that for other parameters.

> If the new interface isn't used, the old one still needs to work.  If it
> is used, the old one either has to do "the right thing", or fail
> cleanly.
>
> We approximate "new interface isn't used" by "block migration is off in
> global state".  When it is off, the migration command needs to honor its
> two flags for compatibility.  It must leave block migration off in
> global state.  Yes, this will complicate the implementation until we
> actually remove the deprecated flags.  Par for the backward compatility
> course.
>
> When block migration isn't off in global state, we can either
>
> * let the flags take precedence over the global state (one
>   interpretation of "do the right thing"), or
>
> * reject flags that conflict with global state (another interpretation),
>   or
>
> * reject *all* flags (fail cleanly).
>
> The last one looks perfectly servicable to me.

Yeap,  I think that makes sense.  If you use capabilities, parameters,
old interface don't work at all.

We still have a problem that is what happens if the user does:

migrate -b 
migrate_cancel (or error)
migrate  (without -b)

With current patches, it will still use -b.  Fixing it requires still
anding more code.  But I think that this use case is so weird what we
should not even care about it.

Later, Juan.

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

On Mon, 15 May 2017 18:11:27 +0200
Cédric Le Goater  wrote:

> >>> +int smt = kvmppc_smt_threads();
> >>> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
> >>
> >> may be we should reintroduce nr_servers at the machine level ? 
> >>  
> > 
> > I had reintroduced it but then I realized it was only used in this
> > function.  
> 
> nr_servers is also used when the device tree is populated with the 
> interrupt controller nodes. No big deal.
> 

Ah yes you're right. Maybe this can be factored out in a followup
patch.

> C. 
> 



pgpcr5oSf6V7q.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

On Mon, 15 May 2017 18:09:04 +0200
Cédric Le Goater  wrote:

> On 05/15/2017 03:16 PM, Greg Kurz wrote:
> > On Mon, 15 May 2017 14:22:32 +0200
> > Cédric Le Goater  wrote:
> >   
> >> On 05/15/2017 01:40 PM, Greg Kurz wrote:  
> >>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> >>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> >>> is an improvement since we no longer allocate ICP objects that will
> >>> never be used. But it has the side-effect of breaking migration of
> >>> older machine types from older QEMU versions.
> >>>
> >>> This patch introduces a compat flag in the sPAPR machine class so
> >>> that all pseries machine up to 2.9 go on with the previous behavior
> >>> of pre-allocating ICP objects.
> >>
> >> I think this is a quite elegant way to a handle the migration 
> >> regression. Thanks for taking care of it.
> >>
> >> Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
> >> instead of the OBJECT(cpu)  ? 
> >>  
> > 
> > Do you mean to reparent unconditionally to OBJECT(spapr) for all
> > machine versions ?   
> 
> only in the case of smc->must_pre_allocate_icps
> 
> > I'm not sure this would be beneficial, but I might be missing 
> > something...  
> 
> I think that we would not need to allocate the legacy_icps array. 
> Parenting the icp object to the spapr machine should be enough. 
> I might be wrong. my expertise on the migration stream is very 
> basic.
> 

I don't think this would work because an older QEMU would still
send state for objects that don't exist in the destination.

> C.
>  



pgpQEvqVArg1Y.pgp
Description: OpenPGP digital signature