[Qemu-devel] [PATCH] uas: Fix response iu struct definition

[Hans de Goede]

This patch mirrors a patch to the Linux uas kernel driver which I've just
submitted. It looks like the qemu uas struct definitions were taken from
the Linux kernel driver, and have inherited the same mistake.

Besides fixing the response iu struct, the patch also drops the add_info
parameter from the usb_uas_queue_response() function, it is always 0 anyways,
and expressing 3 zero-bytes as a function argument is a bit hard.

Below is the long explanation for this change taken from the kernel commit:

The response iu struct before this patch has a size of 7 bytes, which is weird
since all other iu-s are explictly padded to a multiple of 4 bytes.

Submitting a 7 byte bulk transfer to the status endpoint of a real uasp device
when expecting a response iu results in an USB babble error, as the device
actually sends 8 bytes.

Up on closer reading of the UAS spec:
http://www.t10.org/cgi-bin/ac.pl?t=f&f=uas2r00.pdf

The reason for this becomes clear, the 2 entries in "Table 17 — RESPONSE IU"
are numbered 4 and 6, looking at other iu definitions in the spec, esp.
multi-byte fields, this indicates that the ADDITIONAL RESPONSE INFORMATION
field is not a 2 byte field as one might assume at a first look, but is
a multi-byte field containing 3 bytes.

This also aligns with the SCSI Architecture Model 4 spec, which UAS is based
on which states in paragraph "7.1 Task management function procedure calls"
that the "Additional Response Information" output argument for a Task
management function procedure call is 3 bytes.

Last but not least I've verified this by sending a logical unit reset task
management call with an invalid lun to an actual uasp device, and received
back a response-iu with byte 6 being 0, and byte 7 being 9, which is the
responce code for an invalid iu, which confirms that the response code is
being reported in byte 7 of the response iu rather then in byte 6.

Signed-off-by: Hans de Goede 
---
 hw/usb/dev-uas.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 5884035..82a47be 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -76,7 +76,7 @@ typedef struct {
 } QEMU_PACKED  uas_ui_sense;
 
 typedef struct {
-uint16_t   add_response_info;
+uint8_tadd_response_info[3];
 uint8_tresponse_code;
 } QEMU_PACKED  uas_ui_response;
 
@@ -392,14 +392,12 @@ static void usb_uas_queue_status(UASDevice *uas, 
UASStatus *st, int length)
 }
 }
 
-static void usb_uas_queue_response(UASDevice *uas, uint16_t tag,
-   uint8_t code, uint16_t add_info)
+static void usb_uas_queue_response(UASDevice *uas, uint16_t tag, uint8_t code)
 {
 UASStatus *st = usb_uas_alloc_status(uas, UAS_UI_RESPONSE, tag);
 
 trace_usb_uas_response(uas->dev.addr, tag, code);
 st->status.response.response_code = code;
-st->status.response.add_response_info = cpu_to_be16(add_info);
 usb_uas_queue_status(uas, st, sizeof(uas_ui_response));
 }
 
@@ -768,32 +766,32 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui)
 if (req && req->dev == dev) {
 scsi_req_cancel(req->req);
 }
-usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE, 0);
+usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE);
 break;
 
 case UAS_TMF_LOGICAL_UNIT_RESET:
 trace_usb_uas_tmf_logical_unit_reset(uas->dev.addr, tag, lun);
 qdev_reset_all(&dev->qdev);
-usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE, 0);
+usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE);
 break;
 
 default:
 trace_usb_uas_tmf_unsupported(uas->dev.addr, tag, ui->task.function);
-usb_uas_queue_response(uas, tag, UAS_RC_TMF_NOT_SUPPORTED, 0);
+usb_uas_queue_response(uas, tag, UAS_RC_TMF_NOT_SUPPORTED);
 break;
 }
 return;
 
 invalid_tag:
-usb_uas_queue_response(uas, tag, UAS_RC_INVALID_INFO_UNIT, 0);
+usb_uas_queue_response(uas, tag, UAS_RC_INVALID_INFO_UNIT);
 return;
 
 overlapped_tag:
-usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG, 0);
+usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG);
 return;
 
 incorrect_lun:
-usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN, 0);
+usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN);
 }
 
 static void usb_uas_handle_data(USBDevice *dev, USBPacket *p)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 0/7] qdev and blockdev refcount leak fixes

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 04:44:18PM +0100, Andreas Färber wrote:
> Am 30.10.2013 14:54, schrieb Stefan Hajnoczi:
> > v3:
> >  * I lost track of this patch, now I'm pushing it again
> 
> Part of this series is in a pending qom-next pull of mine (on a
> different base), which Anthony didn't merge due to some questions or
> problems, and during Hackathon he disappeared and so far hasn't followed
> up further. :(
> 
> I can surely send a rebased PULL v2 now that the block fix I think is
> in, but not sure if that is all to do...

Ah, that explains what happened.

Anyway this v3 series should be useful to you because it resolves
conflicts with qemu.git/master.

Stefan

Re: [Qemu-devel] [PATCH v3 2/2] smbios: Set system manufacturer, product & version by default

[Eduardo Habkost]

On Wed, Oct 30, 2013 at 09:22:38PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin"  writes:
> 
> > On Wed, Oct 30, 2013 at 04:18:37PM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin"  writes:
> >> 
> >> > On Wed, Oct 30, 2013 at 12:29:16PM -0200, Eduardo Habkost wrote:
> >> >> On Wed, Oct 30, 2013 at 04:18:16PM +0200, Michael S. Tsirkin wrote:
> >> >> > On Wed, Oct 30, 2013 at 01:56:40PM +0100, arm...@redhat.com wrote:
> >> >> > > From: Markus Armbruster 
> >> >> > > 
> >> >> > > Currently, we get SeaBIOS defaults: manufacturer Bochs, product 
> >> >> > > Bochs,
> >> >> > > no version.  Best SeaBIOS can do, but we can provide better 
> >> >> > > defaults:
> >> >> > > manufacturer QEMU, product & version taken from QEMUMachine desc and
> >> >> > > name.
> >> >> > > 
> >> >> > > Take care to do this only for new machine types, of course.
> >> >> > > 
> >> >> > > Signed-off-by: Markus Armbruster 
> >> >> > 
> >> >> > I feel applying this one would be a mistake.
> >> >> > 
> >> >> > Machine desc is for human readers.
> >> >> > For example, it currently says "Standard PC (Q35 + ICH9, 2009)"
> >> >> > but if we add a variant with IDE compatibility mode we will
> >> >> > likely want to
> >> >> > tweak it to say "Standard PC (Q35 + ICH9/AHCI mode, 2009)"
> >> >> > and add another one saying ""Standard PC (Q35 + ICH9/compat mode,
> >> >> > 2009)".
> >> >> > 
> >> >> > In other words we want the ability to tweak
> >> >> > description retroactively, and exposing it to guest will
> >> >> > break this ability.
> >> >> > 
> >> >> > So we really need a new field not tied to the human description.
> >> >> > 
> >> >> 
> >> >> You have a point, but if we do that one day, then we can add a new
> >> >> smbios-specific field and set it for each of the existing machine-types
> >> >> so they keep the same ABI. This patch doesn't make us unable to do that
> >> >> in the future.
> >> >
> >> > We'll likely forget and just break guest ABI.
> >> > So we really need a unit test for this, too.
> >> 
> >> More tests are good, but we I think we got bigger fish to fry than
> >> writing tests to catch changes that are so obviously foolish as messing
> >> with old machine type's QEMUMachine.
> >
> > You "messed with" old machine type's QEMUMachine in your cleanup
> > patches too, didn't you?
> 
> I've occasionally touched QEMUMachine initializers in cleanup series,
> but nothing as frivolous as changing strings.  And I can't find anything
> as frivolous as that in git.  We *are* careful and conservative there.

Actually, we changed .desc for old machine types before. See commit
a0dba644c139907ccf6735c505fbd254010d6938.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] QEMU 1.6 and drive discard parameter

[Amos Kong]

On Thu, Oct 31, 2013 at 04:07:15PM +0800, Osier Yang wrote:
> CC to Amos.
> 
> On 30/10/13 16:19, whitearchey wrote:
> >
> >In QEMU 1.6 parameters of 'drive' option were removed:
> >
> >QemuOptsList qemu_drive_opts = {
> >.name = "drive",
> >.head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head),
> >.desc = {
> >/*
> > * no elements => accept any params
> > * validation will happen later
> > */
> >{ /* end of list */ }
> >},
> >};
> >
> >But libvirt still checks for QEMU_CAPS_DRIVE_DISCARD using QMP
> >query-command-line-options:
> >
> >static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
> >{ "machine", "mem-merge", QEMU_CAPS_MEM_MERGE },
> >{ "drive", "discard", QEMU_CAPS_DRIVE_DISCARD },
> >{ "realtime", "mlock", QEMU_CAPS_MLOCK },
> >};
> >...
> >qemuMonitorGetCommandLineOptionParameters(mon,
> >virQEMUCapsCommandLine[i].option, &values)
> >
> >So, when I try to use discard option in domain xml I get this error:
> >
> >error : qemuBuildDriveStr:3986 : unsupported configuration:
> >discard is not supported by this QEMU binary
> >
> 
> It's a qemu problem, the command "query-command-line-options" should
> keep working
> after the structures were changed for any option, in this case, all
> the option descs were
> moved to "qemu_common_drive_opts" instead.

{ 'execute': 'query-command-line-options', 'arguments': { 'option': 'drive' } }
 
{
"return": [
{
"parameters": [
],
"option": "drive"
}
]
}

It returns a NULL parameters list, that's true, some error handling
should be done by libvirt.

> Regards,
> Osier

-- 
Amos.

Re: [Qemu-devel] [PATCH v3 3/2] smbios: Decouple system product from QEMUMachine

[Eduardo Habkost]

On Thu, Oct 31, 2013 at 06:51:50AM +0100, Markus Armbruster wrote:
> Michael Tsirkin doesn't trust us to keep values of QEMUMachine member
> product stable in the future.  Use copies instead, and in a way that
> makes it obvious that they're guest ABI.
> 
> Note that we can be trusted to keep values of member name, because
> that has always been ABI.
> 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 07/10] piix3 piix4: Clean up use of cannot_instantiate_with_device_add_yet

[Marcel Apfelbaum]

On Wed, 2013-10-30 at 17:28 +0100, arm...@redhat.com wrote:
> From: Markus Armbruster 
> 
> A PIIX3/PIIX4 southbridge has multiple functions.  We model each
> function as a separate qdev.  Two of them need some special wiring set
> up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
> and the SMBus controller at 01.3.
> 
> The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
> always had cannot_instantiate_with_device_add_yet set, but there is no
> obvious reason why device_add could not work for them.  Drop it.
Question:
piix3-ide (and probably piix4-ide) has io ports hard coded in 
pci_piix_init_ports.
PIIX3/PIIX4 already has a piix3-ide that uses the io ports.
Adding more piix3-ide devices would work? 
What am I missing?

Another question:
It seems that piix3-ide it was meant to be a function within PIIX3,
if so, we need a cannot_instantiate_with_device_add_ever?

Hope it helped,
Marcel

> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/acpi/piix4.c|  6 +-
>  hw/ide/piix.c  |  3 ---
>  hw/isa/piix4.c |  6 +-
>  hw/pci-host/piix.c | 12 ++--
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index c29a703..368a76b 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -508,9 +508,13 @@ static void piix4_pm_class_init(ObjectClass *klass, void 
> *data)
>  k->revision = 0x03;
>  k->class_id = PCI_CLASS_BRIDGE_OTHER;
>  dc->desc = "PM";
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->vmsd = &vmstate_acpi;
>  dc->props = piix4_pm_properties;
> +/*
> + * Reason: part of PIIX4 southbridge, needs to be wired up,
> + * e.g. by mips_malta_init()
> + */
> +dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo piix4_pm_info = {
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 27b08e1..9b5960b 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -248,7 +248,6 @@ static void piix3_ide_class_init(ObjectClass *klass, void 
> *data)
>  k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>  k->class_id = PCI_CLASS_STORAGE_IDE;
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo piix3_ide_info = {
> @@ -267,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass, 
> void *data)
>  k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
>  k->class_id = PCI_CLASS_STORAGE_IDE;
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->unplug = pci_piix3_xen_ide_unplug;
>  }
>  
> @@ -289,7 +287,6 @@ static void piix4_ide_class_init(ObjectClass *klass, void 
> *data)
>  k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
>  k->class_id = PCI_CLASS_STORAGE_IDE;
>  set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  }
>  
>  static const TypeInfo piix4_ide_info = {
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index d9dac61..def6fe3 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -113,8 +113,12 @@ static void piix4_class_init(ObjectClass *klass, void 
> *data)
>  k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
>  k->class_id = PCI_CLASS_BRIDGE_ISA;
>  dc->desc = "ISA bridge";
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->vmsd = &vmstate_piix4;
> +/*
> + * Reason: part of PIIX4 southbridge, needs to be wired up,
> + * e.g. by mips_malta_init()
> + */
> +dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo piix4_info = {
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 8089fd6..1526fd4 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -644,7 +644,6 @@ static void piix3_class_init(ObjectClass *klass, void 
> *data)
>  
>  dc->desc= "ISA bridge";
>  dc->vmsd= &vmstate_piix3;
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  k->no_hotplug   = 1;
>  k->init = piix3_initfn;
>  k->config_write = piix3_write_config;
> @@ -652,6 +651,11 @@ static void piix3_class_init(ObjectClass *klass, void 
> *data)
>  /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
>  k->device_id= PCI_DEVICE_ID_INTEL_82371SB_0;
>  k->class_id = PCI_CLASS_BRIDGE_ISA;
> +/*
> + * Reason: part of PIIX3 southbridge, needs to be wired up by
> + * pc_piix.c's pc_init1()
> + */
> +dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo piix3_info = {
> @@ -668,7 +672,6 @@ static void piix3_xen_class_init(ObjectClass *klass, void 
> *data)
>  
>  dc->desc= "ISA bridge";
>  dc->vmsd= &vmstate_pi

Re: [Qemu-devel] [PATCH v3 07/10] piix3 piix4: Clean up use of cannot_instantiate_with_device_add_yet

[Markus Armbruster]

Marcel Apfelbaum  writes:

> On Wed, 2013-10-30 at 17:28 +0100, arm...@redhat.com wrote:
>> From: Markus Armbruster 
>> 
>> A PIIX3/PIIX4 southbridge has multiple functions.  We model each
>> function as a separate qdev.  Two of them need some special wiring set
>> up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
>> and the SMBus controller at 01.3.
>> 
>> The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
>> always had cannot_instantiate_with_device_add_yet set, but there is no
>> obvious reason why device_add could not work for them.  Drop it.
> Question:
> piix3-ide (and probably piix4-ide) has io ports hard coded in
> pci_piix_init_ports.
> PIIX3/PIIX4 already has a piix3-ide that uses the io ports.
> Adding more piix3-ide devices would work? 
> What am I missing?

If I understand physical hardware correctly, the southbridge routes the
legacy ISA IDE ports to the first suitable device.  We don't model that
correctly, and end up with all IDE controller device models claiming
them.

There's a similar case in PATCH 09/10: i8042.  It also has hardcoded ISA
ports.  Nevertheless, I drop cannot_instantiate_with_device_add_yet with
the following rationale:

* i8042: drop, even though its I/O base is hardcoded (because you
  could conceivably still add one to a board that has none) [...]

Same argument applies here: I figure you could add a piix3-ide to a
board that has no IDE controller.

General rule: when in doubt, cannot_instantiate_with_device_add_yet
stays off, because that's the way Anthony wants it (if I understand him
correctly).

> Another question:
> It seems that piix3-ide it was meant to be a function within PIIX3,

Correct.

> if so, we need a cannot_instantiate_with_device_add_ever?

I agree with you that there's something missing here, but I don't think
it's cannot_instantiate_with_device_add_ever :)

It would be nice if we'd model PCI functions and PCI devices properly.
If we did, we wouldn't let users plug a function into a PCI bus.
Instead, we'd let them combine functions into devices, and plug devices
into slots[*].  But it's not what we have now.

In the current state of affairs, we approximate "combine functions into
devices" by letting users plug a bunch of function device models into
the same PCI slot, with different function addresses.  That requires
cannot_instantiate_with_device_add_ever = false.

Users generally don't plug southbridge IDE controller functions, because
a board with PCI generally has a southbridge containing one already.
But as long as a user *could* plug it successfully into *some* board,
cannot_instantiate_with_device_add_ever stays off as per the general
rule above.

Previous discussion:
https://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg02000.html

> Hope it helped,

Thinking reviewers are always appreciated!


[*] Then hot plug of multi-function devices could be made to work.

Re: [Qemu-devel] [PATCH] import kvm-unittest in QEMU source tree

[Gleb Natapov]

On Wed, Oct 30, 2013 at 04:06:19PM -0700, Andrew Jones wrote:
> On Wed, Oct 16, 2013 at 10:03:37PM +0300, Michael S. Tsirkin wrote:
> > This simply imports kvm-unittest git into qemu source tree.
> > 
> > We can next work on making make check run it
> > automatically.
> > 
> > Squashed 'kvm-unittest/' content from commit 2bc0e29
> > 
> > git-subtree-dir: kvm-unittest
> > git-subtree-split: 2bc0e29ee4447bebcd3b90053881f59265306adc
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > 
> > ---
> > 
> > Gleb, Paolo, any objections to this?  I really want a small guest for
> > running ACPI tests during make check, and kvm-unittest seems to fit the
> > bill.
> > 
> > Ability to test e.g. PCI with this in-tree would be very benefitial.
> >
> 
> Sorry for slow reply, I just noticed this mail now.
> 
> So far qemu is a dependency for kvm-unit-tests (both x86 and arm use it),
> but at some point that could change (if another arch doesn't want to use
> qemu). So if that happens, then it won't make much sense for the tests to
> live in qemu. Thus I'm not 100% in favor of moving them. However, if the
> consensus is to move them, then I have two comments on this patch.
> 
Do not worry, we are far from such consensus :)

> 1) There are a couple pendings patches on the kvm list that tidy up the
> kvm-unit-tests repo - removing lots of the files. That should be
> committed first to avoid importing a bunch of files right before deleting
> them.
> 
> 2) The name shouldn't change from kvm-unit-tests to kvm-unittest.
> 
> drew

--
Gleb.

[Qemu-devel] [PATCH] RFC: spapr: introduce smt_cpu_index

[Alexey Kardashevskiy]

This is not really a patch to accept or review, this is a conversation
starter.

Cc: Badari Pulavarty  
Cc: Paul Mackerras 
Cc: David Gibson 
Cc: Benjamin Herrenschmidt 
Cc: Andreas Färber 
Signed-off-by: Alexey Kardashevskiy 

---

Normall CPUState::cpu_index is used to pick the right CPU for various
operations. However default consecutive numbering does not always work
for PPC64.

For example, on POWER7 (which supports 4 threads per core),
"-smp 8,threads=4" should create CPUs with indexes 0,1,2,3,4,5,6,7 and
"-smp 8,threads=1" should create CPUs with indexes 0,4,8,12,16,20,24,28.

These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX
and used to call KVM VCPU's ioctl.
In order to achieve this, kvmppc_fixup_cpu() was introduced. Roughly,
it multiplies cpu_index by the number of threads per core.

The approach used to work till we hit NUMA bug. For example, if to run
QEMU with:

-smp 16,sockets=16,cores=1,threads=1
-numa node,nodeid=0,cpus=0-7,mem=500
-numa node,nodeid=1,cpus=8-15,mem=500

then CPUs 0 1 4 5 6 7 8 9 10 11 12 13 14 15 get assigned to node#0 and
only CPUs 2 and 3 are assigned to node#1. This happens because
kvmppc_fixup_cpu() does change cpu_index but does not change NUMA binding.

We could fix kvmppc_fixup_cpu() to update NUMA binding but the whole idea
of hacking cpu_index does not seem right as (for example) with
"-smp 8,threads=1" the user would expect that QEMU Monitor's "cpu"
command would accept indexes 0, 1, 2,.. but this is not true as
it accepts 0, 4, 8,...

And there is an existing mechanism to solve my issue which allows having
architecture specific CPU indexes via kvm_arch_vcpu_id(). However it is:

1) KVM only and does not have empty stub for TCG;

2) Reverse converter is needed to get CPUState from KVM's CPU index
(which does not exist) as sPAPR will use the KVM CPU index in various
hypercalls.

So I could:
1. fix kvmppc_fixup_cpu() to update NUMA binding;
2. Apply the patch below and make sure it does not break x86/ARM/s390
with and without KVM (does not it?);
3. Move kvm_arch_vcpu_id() + new kvm_arch_get_vcpu_by_id() out of KVM part
and make it platform specific with or without KVM.

What is the correct approach here? Thanks.



---
 hw/intc/xics_kvm.c  | 14 +++---
 hw/ppc/spapr.c  |  9 +
 hw/ppc/spapr_hcall.c|  2 +-
 hw/ppc/spapr_rtas.c |  4 ++--
 include/sysemu/kvm.h|  1 +
 kvm-stub.c  | 10 ++
 target-ppc/cpu.h|  3 +++
 target-ppc/kvm.c| 24 +---
 target-ppc/translate_init.c |  1 +
 9 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c203646..c96ca4d 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -65,7 +65,7 @@ static void icp_get_kvm_state(ICPState *ss)
 ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, ®);
 if (ret != 0) {
 error_report("Unable to retrieve KVM interrupt controller state"
-" for CPU %d: %s", ss->cs->cpu_index, strerror(errno));
+" for CPU %ld: %s", kvm_arch_vcpu_id(ss->cs), strerror(errno));
 exit(1);
 }
 
@@ -97,7 +97,7 @@ static int icp_set_kvm_state(ICPState *ss, int version_id)
 ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, ®);
 if (ret != 0) {
 error_report("Unable to restore KVM interrupt controller state (0x%"
-PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
+PRIx64 ") for CPU %ld: %s", state, kvm_arch_vcpu_id(ss->cs),
 strerror(errno));
 return ret;
 }
@@ -313,9 +313,9 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU 
*cpu)
 KVMXICSState *icpkvm = KVM_XICS(icp);
 
 cs = CPU(cpu);
-ss = &icp->ss[cs->cpu_index];
+ss = &icp->ss[kvm_arch_vcpu_id(cs)];
 
-assert(cs->cpu_index < icp->nr_servers);
+assert(kvm_arch_vcpu_id(cs) < icp->nr_servers);
 if (icpkvm->kernel_xics_fd == -1) {
 abort();
 }
@@ -325,15 +325,15 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU 
*cpu)
 struct kvm_enable_cap xics_enable_cap = {
 .cap = KVM_CAP_IRQ_XICS,
 .flags = 0,
-.args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
+.args = {icpkvm->kernel_xics_fd, kvm_arch_vcpu_id(cs), 0, 0},
 };
 
 ss->cs = cs;
 
 ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
 if (ret < 0) {
-error_report("Unable to connect CPU%d to kernel XICS: %s",
-cs->cpu_index, strerror(errno));
+error_report("Unable to connect CPU%ld to kernel XICS: %s",
+kvm_arch_vcpu_id(cs), strerror(errno));
 exit(1);
 }
 }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f76b355..1e00982 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -206,19 +206,20 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment 
*sp

Re: [Qemu-devel] virtio-net performance on mach-virt.

[Paolo Bonzini]

Il 31/10/2013 04:32, Giridhar Maruthy ha scritto:
> On 30 October 2013 22:12, Paolo Bonzini  wrote:
>> Il 30/10/2013 15:40, Giridhar Maruthy ha scritto:
>>> Hi All,
>>>
>>> I tried to measure the network performance of guest (mach-virt) with
>>> virtio-net on an ARM host platform (Samsung exynos). The qemu version
>>> is 1.6.50.
>>>
>>> I found that with 1GbE NIC on the host, the host iperf gave a speed of
>>> 847Mbits/sec when a local dhcp server was used as a iperf server.
>>>
>>> But the guest gave a speed of 478Mbits/sec which is around 56% compared to 
>>> host.
>>>
>>> What is the typical guest network efficiency compared to host with 
>>> virtio-net?
>>> Any ideas would be helpful.
>>
>> Were you using vhost?
> 
> Yes, I did use vhost in the host kernel configuration and passed
> vhost=on in qemu, but there is no difference in the performance of
> guest.

What's your command line?

Paolo

Re: [Qemu-devel] [libvirt] QEMU 1.6 and drive discard parameter

[Amos Kong]

CC Kevin, Paolo

On Thu, Oct 31, 2013 at 04:35:43PM +0800, Amos Kong wrote:
> On Thu, Oct 31, 2013 at 04:07:15PM +0800, Osier Yang wrote:
> > CC to Amos.
> > 
> > On 30/10/13 16:19, whitearchey wrote:
> > >
> > >In QEMU 1.6 parameters of 'drive' option were removed:
> > >
> > >QemuOptsList qemu_drive_opts = {
> > >.name = "drive",
> > >.head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head),
> > >.desc = {
> > >/*
> > > * no elements => accept any params
> > > * validation will happen later
> > > */
> > >{ /* end of list */ }
> > >},
> > >};
> > >
> > >But libvirt still checks for QEMU_CAPS_DRIVE_DISCARD using QMP
> > >query-command-line-options:
> > >
> > >static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
> > >{ "machine", "mem-merge", QEMU_CAPS_MEM_MERGE },
> > >{ "drive", "discard", QEMU_CAPS_DRIVE_DISCARD },
> > >{ "realtime", "mlock", QEMU_CAPS_MLOCK },
> > >};
> > >...
> > >qemuMonitorGetCommandLineOptionParameters(mon,
> > >virQEMUCapsCommandLine[i].option, &values)
> > >
> > >So, when I try to use discard option in domain xml I get this error:
> > >
> > >error : qemuBuildDriveStr:3986 : unsupported configuration:
> > >discard is not supported by this QEMU binary
> > >
> > 
> > It's a qemu problem, the command "query-command-line-options" should
> > keep working
> > after the structures were changed for any option, in this case, all
> > the option descs were
> > moved to "qemu_common_drive_opts" instead.
> 
> { 'execute': 'query-command-line-options', 'arguments': { 'option': 'drive' } 
> }
>  
> {
> "return": [
> {
> "parameters": [
> ],
> "option": "drive"
> }
> ]
> }
> 
> It returns a NULL parameters list, that's true, some error handling
> should be done by libvirt.

Currently we have three QemuOptsList (qemu_common_drive_opts,
qemu_legacy_drive_opts, and qemu_drive_opts)

only qemu_drive_opts is added to vm_config_groups[]

| commit 492fdc6fbe17b2d45878e813e980f782ac260c30
| Author: Kevin Wolf 
| Date:   Wed Jun 19 13:44:17 2013 +0200
| 
| Revert "block: Disable driver-specific options for 1.5"

This patch removed the items in qemu_drive_opts .desc

We query commandline options by checking information in vm_config_groups[],
so we can only get a NULL parameter list now.

Another issue is we also can't query legacy options of -drive.

--

Can we fix this problem by copy desc items of qemu_legacy_drive_opts and
qemu_common_drive_opts to qemu_drive_opts?

-- 
Amos.

Re: [Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 12:32:12PM +0100, Paolo Bonzini wrote:
> Il 28/10/2013 20:01, Michael S. Tsirkin ha scritto:
> > From: Alexander Graf 
> > 
> > When AHCI executes an asynchronous IDE command, it checked DRDY without
> > checking either DRQ or BSY.  This sometimes caused interrupt to be sent
> > before command is actually completed.
> > 
> > This resulted in a race condition: if guest then managed to access the
> > device before command has completed, it would hang waiting for an
> > interrupt.
> > This was observed with windows 7 guests.
> > 
> > To fix, check for DRQ or BSY in additiona to DRDY, if set,
> > the command is asynchronous so delay the interrupt until
> > asynchronous done callback is invoked.
> > 
> > Reported-by: Michael S. Tsirkin 
> > Reviewed-by: Michael S. Tsirkin 
> > Tested-by: Michael S. Tsirkin 
> > Signed-off-by: Michael S. Tsirkin 
> > 
> > ---
> >  hw/ide/ahci.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index a8be62c..fbea9e8 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
> >  /* We're ready to process the command in FIS byte 2. */
> >  ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
> >  
> > -if (s->dev[port].port.ifs[0].status & READY_STAT) {
> > +if ((s->dev[port].port.ifs[0].status & 
> > (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
> > +READY_STAT) {
> >  ahci_write_fis_d2h(&s->dev[port], cmd_fis);
> >  }
> >  }
> > 
> 
> While the patch fixes the symptom, I think it is only a bandaid.
> 
> There is no reason why the async_cmd_done should be restricted to
> asynchronous commands.  If synchronous commands are made to go through
> the async_cmd_done callback, you'll automatically get the D2H FIS
> written for all commands.

I suggested this to Kevin offline but he prefers it like this.

> It's good for 1.7, but let's revisit it for 1.8.
> 
> Paolo

Fine with me.

Re: [Qemu-devel] [PATCH repost] ahci: fix win7 hang on boot

[Paolo Bonzini]

Il 28/10/2013 20:01, Michael S. Tsirkin ha scritto:
> From: Alexander Graf 
> 
> When AHCI executes an asynchronous IDE command, it checked DRDY without
> checking either DRQ or BSY.  This sometimes caused interrupt to be sent
> before command is actually completed.
> 
> This resulted in a race condition: if guest then managed to access the
> device before command has completed, it would hang waiting for an
> interrupt.
> This was observed with windows 7 guests.
> 
> To fix, check for DRQ or BSY in additiona to DRDY, if set,
> the command is asynchronous so delay the interrupt until
> asynchronous done callback is invoked.
> 
> Reported-by: Michael S. Tsirkin 
> Reviewed-by: Michael S. Tsirkin 
> Tested-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> 
> ---
>  hw/ide/ahci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index a8be62c..fbea9e8 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
>  /* We're ready to process the command in FIS byte 2. */
>  ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
>  
> -if (s->dev[port].port.ifs[0].status & READY_STAT) {
> +if ((s->dev[port].port.ifs[0].status & 
> (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
> +READY_STAT) {
>  ahci_write_fis_d2h(&s->dev[port], cmd_fis);
>  }
>  }
> 

While the patch fixes the symptom, I think it is only a bandaid.

There is no reason why the async_cmd_done should be restricted to
asynchronous commands.  If synchronous commands are made to go through
the async_cmd_done callback, you'll automatically get the D2H FIS
written for all commands.

It's good for 1.7, but let's revisit it for 1.8.

Paolo

Re: [Qemu-devel] [PATCH 00/13] target-openrisc: More optimizations and corrections

[Jia Liu]

Hi Sebastian,

On Wed, Oct 30, 2013 at 5:22 AM, Sebastian Macke  wrote:
> On 29/10/2013 2:15 PM, Max Filippov wrote:
>>
>> On Tue, Oct 29, 2013 at 11:04 PM, Sebastian Macke 
>> wrote:
>>>
>>> Hi,
>>>
>>> This is the second part of the patches to make the openrisc target faster
>>> and more reliable.
>>
>> Hi Sebastian,
>>
>> this series doesn't apply cleanly to the current qemu git head,
>> what tree is it based on?
>>
>
> It is based on the current master tree but depends on the patches I send on
> the 21st October.
> They got accepted by the maintainer but are not in the master tree right
> now.

Your previous patch set separate SR is a little different from Arch
spec, you said
it will speed up some instructions, I think maybe it is a good reason
and acceptable.
But this one, going a little more too far, maybe you should consider on
the comment from Max, Peter and Richard.

More people comment your patch, make your code more better :)

Regards,
Jia

Re: [Qemu-devel] How to add qemu-system-$(target) as a dependency for qtests

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 05:09:26PM +0100, Paolo Bonzini wrote:
> Il 30/10/2013 16:07, Stefan Hajnoczi ha scritto:
> > I came across this little tests/Makefile annoyance:
> > If you modify QEMU code and then run "make check" it will not rebuild QEMU.
> > 
> > This can be confusing during development when you expect changing the
> > code and rerunning the tests to pass :).
> > 
> > I played with tests/Makefile but was unable to add the right
> > dependency.  We need something that makes all libqtest tests depend on
> > $(TARGET)-softmmu/qemu-system-$(TARGET) at "make check" time.
> > 
> > This way QEMU gets rebuilt if "make check" will execute the QEMU binary.
> > 
> > Any ideas?
> 
> Does this work?
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index fa4c9f0..f3f78ee 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -212,7 +212,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
>  # gtester tests, possibly with verbose output
>  
>  .PHONY: $(patsubst %, check-qtest-%, $(QTEST_TARGETS))
> -$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: 
> $(check-qtest-y)
> +$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: 
> $(check-qtest-y) subdir-%-softmmu
>   $(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
>   $(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
>   MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \

Yes, it does!

Yesterday I tried the same approach with "%-softmmu/qemu-system-%".  The
'%' is only expanded once so it failed.

Stefan

Re: [Qemu-devel] [PATCH v3 10/10] qdev: Do not let the user try to device_add when it cannot work

[Marcel Apfelbaum]

On Wed, 2013-10-30 at 17:28 +0100, arm...@redhat.com wrote:
> From: Markus Armbruster 
> 
> Such devices have always been unavailable and omitted from the list of
> available devices shown by device_add help.  Until commit 18b6dad
> silently broke the former, setting up nasty traps for unwary users,
> like this one:
> 
> $ qemu-system-x86_64 -nodefaults -monitor stdio -display none
> QEMU 1.6.50 monitor - type 'help' for more information
> (qemu) device_add apic
> Segmentation fault (core dumped)
> 
> I call that a regression.  Fix it.

Seems OK to me.
Reviewed-by: Marcel Apfelbaum 

> 
> Signed-off-by: Markus Armbruster 
> ---
>  qdev-monitor.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 36f6f09..d3d87a3 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -477,13 +477,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>  }
>  }
>  
> -if (!obj) {
> +k = DEVICE_CLASS(obj);
> +
> +if (!k || k->cannot_instantiate_with_device_add_yet) {
>  qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
>  return NULL;
>  }
>  
> -k = DEVICE_CLASS(obj);
> -
>  /* find bus */
>  path = qemu_opt_get(opts, "bus");
>  if (path != NULL) {

Re: [Qemu-devel] [PATCH v3 09/10] isa: Clean up use of cannot_instantiate_with_device_add_yet

[Marcel Apfelbaum]

On Wed, 2013-10-30 at 17:28 +0100, arm...@redhat.com wrote:
> From: Markus Armbruster 
> 
> Drop it when there's no obvious reason why device_add could not work.
> Else keep and document why.
> 
> * isa-fdc: drop
> 
> * i8042: drop, even though its I/O base is hardcoded (because you
>   could conceivably still add one to a board that has none), and even
>   though PC board code wires up the A20 line (because that wiring is
>   optional)
> 
> * port92: keep because it needs additional wiring by port92_init()
> 
> * mc146818rtc: keep because it needs to be wired up by rtc_init()
> 
> * m48t59_isa: keep because needs to be wired up by m48t59_init_isa()
> 
> * isa-pit, kvm-pit: keep (in their abstract base pic-common) because
>   the PIT needs additional wiring by board code, depending on HPET
>   presence
> 
> * pcspk: keep because of pointer property pit, and because realize
>   sets global pcspk_state
> 
> * vmmouse: keep because of pointer property ps2_mouse
> 
> * vmport: keep because realize sets global port_state
> 
> * isa-i8259, kvm-i8259: keep (in their abstract base pic-common),
>   because the PICs' IRQ input lines are set up by board code, and the
>   wiring of the slave to the master is hard-coded in device model code

I agree with the intend of this patch, but I am not familiar
with this code, so I am not comfortable reviewing it, anyone else?

Thanks,
Marcel 

> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/audio/pcspk.c| 3 ++-
>  hw/block/fdc.c  | 1 -
>  hw/i386/pc.c| 7 ++-
>  hw/input/pckbd.c| 1 -
>  hw/input/vmmouse.c  | 3 ++-
>  hw/intc/i8259_common.c  | 8 +++-
>  hw/misc/vmport.c| 3 ++-
>  hw/timer/i8254_common.c | 7 ++-
>  hw/timer/m48t59.c   | 3 ++-
>  hw/timer/mc146818rtc.c  | 3 ++-
>  10 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index 8e3e178..f980d66 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -192,8 +192,9 @@ static void pcspk_class_initfn(ObjectClass *klass, void 
> *data)
>  
>  dc->realize = pcspk_realizefn;
>  set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->props = pcspk_properties;
> +/* Reason: pointer property "pit", realize sets global pcspk_state */
> +dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo pcspk_info = {
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 86f4920..592b58f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2234,7 +2234,6 @@ static void isabus_fdc_class_init(ObjectClass *klass, 
> void *data)
>  
>  dc->realize = isabus_fdc_realize;
>  dc->fw_name = "fdc";
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->reset = fdctrl_external_reset_isa;
>  dc->vmsd = &vmstate_isa_fdc;
>  dc->props = isa_fdc_properties;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index fe33843..20402ba 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -544,10 +544,15 @@ static void port92_class_initfn(ObjectClass *klass, 
> void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->realize = port92_realizefn;
>  dc->reset = port92_reset;
>  dc->vmsd = &vmstate_port92_isa;
> +/*
> + * Reason: unlike ordinary ISA devices, this one needs additional
> + * wiring: its A20 output line needs to be wired up by
> + * port92_init().
> + */
> +dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo port92_info = {
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index dee31a6..655b8c5 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -522,7 +522,6 @@ static void i8042_class_initfn(ObjectClass *klass, void 
> *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
>  dc->realize = i8042_realizefn;
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->vmsd = &vmstate_kbd_isa;
>  }
>  
> diff --git a/hw/input/vmmouse.c b/hw/input/vmmouse.c
> index 600e4a2..6a50533 100644
> --- a/hw/input/vmmouse.c
> +++ b/hw/input/vmmouse.c
> @@ -282,10 +282,11 @@ static void vmmouse_class_initfn(ObjectClass *klass, 
> void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>  
>  dc->realize = vmmouse_realizefn;
> -dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why 
> */
>  dc->reset = vmmouse_reset;
>  dc->vmsd = &vmstate_vmmouse;
>  dc->props = vmmouse_properties;
> +/* Reason: pointer property "ps2_mouse" */
> +dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo vmmouse_info = {
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index 2acdbfe..9d29399 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
>

Re: [Qemu-devel] [PATCH] RFC: spapr: introduce smt_cpu_index

[Paolo Bonzini]

Il 31/10/2013 10:28, Alexey Kardashevskiy ha scritto:
> This is not really a patch to accept or review, this is a conversation
> starter.
> 
> Cc: Badari Pulavarty  
> Cc: Paul Mackerras 
> Cc: David Gibson 
> Cc: Benjamin Herrenschmidt 
> Cc: Andreas Färber 
> Signed-off-by: Alexey Kardashevskiy 
> 
> ---
> 
> Normall CPUState::cpu_index is used to pick the right CPU for various
> operations. However default consecutive numbering does not always work
> for PPC64.
> 
> For example, on POWER7 (which supports 4 threads per core),
> "-smp 8,threads=4" should create CPUs with indexes 0,1,2,3,4,5,6,7 and
> "-smp 8,threads=1" should create CPUs with indexes 0,4,8,12,16,20,24,28.
> 
> These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX
> and used to call KVM VCPU's ioctl.

I think these two uses should be separated, even if they use the same
formula.  Define a PPC-specific concept, used by TCG as well, and make
kvm_arch_vcpu_id use it.

In other words:

- the hw/ppc/spapr* changes should not use kvm_arch_vcpu_id and
kvm_arch_get_vcpu_by_id, but something like ppc_get_vcpu_dt_id and
ppc_get_vcpu_by_dt_id.

- the comment for the new field should be something like /* The CPU
index used in the device tree.  KVM uses this index too.  */

- kvm_arch_vcpu_id can be a simple wrapper for ppc_get_vcpu_dt_id; no
need for the new kvm_arch_get_vcpu_by_id, and no kvm-stub.c changes
should be necessary either.

- everything else seems fine.

> 2. Apply the patch below and make sure it does not break x86/ARM/s390
> with and without KVM (does not it?);

If you do it right (i.e. do not touch kvm-stub.c) it should "obviously"
not break anything outside PPC.

Paolo

Re: [Qemu-devel] [PATCH v8 00/19] VHDX log replay and write support, .bdrv_create()

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 10:44:37AM -0400, Jeff Cody wrote:
> This patch series contains the initial VHDX log parsing, replay,
> write support, and image creation.
> 
> === v8 changes ===
> https://github.com/codyprime/qemu-kvm-jtc/tree/vhdx-write-v7-upstream
> 
> Rebased to latest qemu/master
> 
> Patch 10/19: * Added comments for bdrv_flush() (Stefan)
> 
> Patch 11/19: * Added qemu_iovec_destroy(&hd_qiov) (Stefan)
>  * On certain _writev errors, restore BAT cache (Stefan)
> 
> Patch 16/19: * Replaced fprintf(stderr,...) with error_setg_errno() (Stefan)
> 
> Patch 18/19: * Added filter for block_state_zero in qemu-iotest/common.rc
> 
> Patch 19/19: * Moved log replay test name to 068 (part of rebase to master)
> 
> === v7 changes ===
> https://github.com/codyprime/qemu-kvm-jtc/tree/vhdx-write-v7-upstream
> 
> Rebased to latest qemu/master (picked up vhdx r/o tests, migration blocker)
> 
> Patch  8/19: * validate log descriptor_count (Stefan)
>  * fix typos in comments (Stefan)
>  * Removed unneccessary initialization (Stefan)
>  * Replay log prior to metadata (Stefan)
>  * In vhdx_log_flush(), call bdrv_flush() prior to zeroing
>out the log guid in the header.
>  * In vhdx_close(), set freed pointers to NULL
>  
> Patch  9/19: * correct logic for region overlap (Stefan)
>  
> Patch 10/19: * add missing goto exit in error case (Stefan)
>  * add bdrv_flush() to ensure data is stable on disk (Stefan)
> 
> Patch 11/19: * fixed typos in comments (Stefan)
>  * QEMU coding style changes (Stefan)
>  * rename bat_entry to bat_entry_le for clarity (Stefan)
>  * Add PAYLOAD_BLOCK_ZERO explicit zero padding for
>protocols that do not support zero init (Stefan)
>  * rename PAYLOAD_BLOCK_FULL_PRESENT to 
>PAYLOAD_BLOCK_FULLY_PRESENT (Stefan)
> 
> Patch 13/19: * Fixed typo in commit message (Stefan)
> 
> Patch 19/19: * New, adds qemu-io test for log replay of data sector
> 
> v6 Patch 17/20: * Dropped (already upstream)
> v6 Patch 18/20: * Dropped (already upstream)
> 
> 
> 
> === v6 changes ===
> https://github.com/codyprime/qemu-kvm-jtc/tree/vhdx-write-v6-upstream
> 
> Rebased to latest qemu/master:
> 
> Patch 16/20: .bdrv_create() propagates Error, and bdrv_unref() used
>  instead of bdrv_delete().
> 
> Patch 17 & 18 are already included in another series:
> [PATCH v3 0/3] qemu-iotests with sample images, vhdx test, cleanup
> 
> They are included here to provide a base for patches 19 & 20.  If the above
> series is applied before this series, then patches 17 and 18 can be ignored.
> 
> Patch 19/20: In qemu-io tests _make_test_img(), filter out vhdx-specific
>  options for .bdrv_create().
> 
> Patch 20/20: Add VHDX write test case to 064.
> 
> 
> === v5 changes ===
> 
> v5 is also available for testing from:
> https://github.com/codyprime/qemu-kvm-jtc/tree/vhdx-write-v5-upstream
> 
> Most of the patches from v4 -> v5 are the same, but there are a few 
> differences
> and a few new patches.  Here is a summary of which patches are different 
> and/or
> new:
> 
> Patch highlights:
> 
> Patch 7  just some minor code movement, in prep for changes in patch 8
> 
> Patch 8  incorporates review feedback from Stefan, for the previous Patch 7
>  in v4.
> 
> Patch 9  adds region checking for log, region table, and metadata tables, per
>  suggestion from Stefan.
> 
> Patch 10 minor change from changes made in 8/16 (vhdx_guid_is_zero() is gone)
> 
> Patch 12 is just some minor housekeeping, to get rid of bit shifting that
>  doesn't need to happen.
> 
> 
> 
> === v4 changes ===  
> 
> v4 patches are available from github as well, on branch 
> vhdx-write-v4-upstream:
> https://github.com/codyprime/qemu-kvm-jtc/tree/vhdx-write-v4-upstream
> https://github.com/codyprime/qemu-kvm-jtc.git
> 
> Those in the midst of reviewing v3, don't fear - the only changes with v4 is
> the addition of patches on the end of the series (patches 10-13).  These
> patches enable creating VHDX images.  Image files created have been
> (briefly & lightly) tested on Hyper-V running on Windows Server 2012.
> 
> Some of the new patches could be squashed with earlier patches in the series,
> but I refrained from doing so, since some of the patches have already been
> reviewed, and others are in the midst of review.  I want to make it as easy
> as possible on those currently reviewing. There is nothing critical
> that needs to be pushed into the earlier patches.
> 
> New patches:
> 
> Patch 10:  Breaks out some more endian translation functions
> (likely squashable into patch 5)
> 
> Patch 11:  Break out some operations into seperate helper functions
> 
> Patch 12:  More comment typos and header fixes in vhdx.h
> (likely squashable into patch 1)
> 
> Patch 13:  Adds .bdrv_create() for vhdx.

Re: [Qemu-devel] [PATCH v3 07/10] piix3 piix4: Clean up use of cannot_instantiate_with_device_add_yet

[Marcel Apfelbaum]

On Thu, 2013-10-31 at 11:29 +0100, Markus Armbruster wrote:
> Marcel Apfelbaum  writes:
> 
> > On Wed, 2013-10-30 at 17:28 +0100, arm...@redhat.com wrote:
> >> From: Markus Armbruster 
> >> 
> >> A PIIX3/PIIX4 southbridge has multiple functions.  We model each
> >> function as a separate qdev.  Two of them need some special wiring set
> >> up in pc_init1() or mips_malta_init() to work: the ISA bridge at 01.0,
> >> and the SMBus controller at 01.3.
> >> 
> >> The IDE controller at 01.1 (piix3-ide, piix3-ide-xen, piix4-ide) has
> >> always had cannot_instantiate_with_device_add_yet set, but there is no
> >> obvious reason why device_add could not work for them.  Drop it.
> > Question:
> > piix3-ide (and probably piix4-ide) has io ports hard coded in
> > pci_piix_init_ports.
> > PIIX3/PIIX4 already has a piix3-ide that uses the io ports.
> > Adding more piix3-ide devices would work? 
> > What am I missing?
> 
> If I understand physical hardware correctly, the southbridge routes the
> legacy ISA IDE ports to the first suitable device.  We don't model that
> correctly, and end up with all IDE controller device models claiming
> them.
> 
> There's a similar case in PATCH 09/10: i8042.  It also has hardcoded ISA
> ports.  Nevertheless, I drop cannot_instantiate_with_device_add_yet with
> the following rationale:
> 
> * i8042: drop, even though its I/O base is hardcoded (because you
>   could conceivably still add one to a board that has none) [...]
> 
> Same argument applies here: I figure you could add a piix3-ide to a
> board that has no IDE controller.
> 
> General rule: when in doubt, cannot_instantiate_with_device_add_yet
> stays off, because that's the way Anthony wants it (if I understand him
> correctly).
> 
> > Another question:
> > It seems that piix3-ide it was meant to be a function within PIIX3,
> 
> Correct.
> 
> > if so, we need a cannot_instantiate_with_device_add_ever?
> 
> I agree with you that there's something missing here, but I don't think
> it's cannot_instantiate_with_device_add_ever :)
> 
> It would be nice if we'd model PCI functions and PCI devices properly.
> If we did, we wouldn't let users plug a function into a PCI bus.
> Instead, we'd let them combine functions into devices, and plug devices
> into slots[*].  But it's not what we have now.
> 
> In the current state of affairs, we approximate "combine functions into
> devices" by letting users plug a bunch of function device models into
> the same PCI slot, with different function addresses.  That requires
> cannot_instantiate_with_device_add_ever = false.
> 
> Users generally don't plug southbridge IDE controller functions, because
> a board with PCI generally has a southbridge containing one already.
> But as long as a user *could* plug it successfully into *some* board,
> cannot_instantiate_with_device_add_ever stays off as per the general
> rule above.
Thanks for the explanation, with this rule in mind:

Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

> 
> Previous discussion:
> https://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg02000.html
> 
> > Hope it helped,
> 
> Thinking reviewers are always appreciated!
> 
> 
> [*] Then hot plug of multi-function devices could be made to work.

Re: [Qemu-devel] [PATCH v8 16/19] block: vhdx - add .bdrv_create() support

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 10:44:53AM -0400, Jeff Cody wrote:
> +if (image_size > VHDX_MAX_IMAGE_SIZE) {
> +error_setg_errno(errp, EINVAL, "Image size too large; max of 
> 64TB\n");

Error messages should not include '\n'.

Perhaps this can be fixed when merging the patch.

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Edgar E. Iglesias]

On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
> 
>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 
> 10:03:24 -0700)
> 
> are available in the git repository at:
> 
> 
>   git://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20131025
> 
> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
> 
>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 
> 18:27:07 +0100)


Applied, thanks all.

Cheers,
Edgar


> 
> 
> target-arm queue: a couple of trivial features to improve support
> for some guest emulation cases, notably running UEFI images:
>  * support VBAR (vector base address register)
>  * allow running without specifying a kernel (ie just running
>an image from flash)
> Plus some bugfixes.
> 
> 
> Alex Bennée (1):
>   integrator: fix Linux boot failure by emulating dbg region
> 
> Alvise Rigo (2):
>   target-arm: sort TCG cpreg list by KVM-style 64 bit ID number
>   target-arm: fix sorting issue of KVM cpreg list
> 
> Nathan Rossi (1):
>   target-arm: Add CP15 VBAR support
> 
> Peter Maydell (2):
>   hw/arm/boot: Make user not specifying a kernel not an error
>   hw/arm: Tidy up conditional calls to arm_load_kernel
> 
>  default-configs/arm-softmmu.mak|1 +
>  hw/arm/boot.c  |6 +-
>  hw/arm/integratorcp.c  |2 +
>  hw/arm/omap_sx1.c  |   10 ++--
>  hw/arm/palm.c  |   10 ++--
>  hw/arm/z2.c|   12 ++--
>  hw/misc/Makefile.objs  |1 +
>  hw/misc/arm_integrator_debug.c |   99 
> 
>  include/hw/misc/arm_integrator_debug.h |   18 ++
>  target-arm/cpu.h   |1 +
>  target-arm/helper.c|   33 ++-
>  target-arm/kvm.c   |8 ++-
>  12 files changed, 176 insertions(+), 25 deletions(-)
>  create mode 100644 hw/misc/arm_integrator_debug.c
>  create mode 100644 include/hw/misc/arm_integrator_debug.h
> 

Re: [Qemu-devel] [PATCH] import kvm-unittest in QEMU source tree

[Anthony Liguori]

On Thu, Oct 31, 2013 at 8:48 AM, Gleb Natapov  wrote:
> On Wed, Oct 30, 2013 at 04:06:19PM -0700, Andrew Jones wrote:
>> On Wed, Oct 16, 2013 at 10:03:37PM +0300, Michael S. Tsirkin wrote:
>> > This simply imports kvm-unittest git into qemu source tree.
>> >
>> > We can next work on making make check run it
>> > automatically.
>> >
>> > Squashed 'kvm-unittest/' content from commit 2bc0e29
>> >
>> > git-subtree-dir: kvm-unittest
>> > git-subtree-split: 2bc0e29ee4447bebcd3b90053881f59265306adc
>> >
>> > Signed-off-by: Michael S. Tsirkin 
>> >
>> > ---
>> >
>> > Gleb, Paolo, any objections to this?  I really want a small guest for
>> > running ACPI tests during make check, and kvm-unittest seems to fit the
>> > bill.
>> >
>> > Ability to test e.g. PCI with this in-tree would be very benefitial.
>> >
>>
>> Sorry for slow reply, I just noticed this mail now.
>>
>> So far qemu is a dependency for kvm-unit-tests (both x86 and arm use it),
>> but at some point that could change (if another arch doesn't want to use
>> qemu). So if that happens, then it won't make much sense for the tests to
>> live in qemu. Thus I'm not 100% in favor of moving them. However, if the
>> consensus is to move them, then I have two comments on this patch.
>>
> Do not worry, we are far from such consensus :)

I don't think kvm-unit-tests belong in QEMU either.  They are tied to
the kernel side of things, not QEMU and many of the tests don't make
sense for TCG.

If you want to execute guest code within QEMU's make check, we can
copy the basic infrastructure in kvm-unit-tests.  It's so simple that
a "fork" isn't really much of a concern to me.

Regards,

Anthony Liguori

>> 1) There are a couple pendings patches on the kvm list that tidy up the
>> kvm-unit-tests repo - removing lots of the files. That should be
>> committed first to avoid importing a bunch of files right before deleting
>> them.
>>
>> 2) The name shouldn't change from kvm-unit-tests to kvm-unittest.
>>
>> drew
>
> --
> Gleb.

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Hi,

Am 31.10.2013 15:02, schrieb Edgar E. Iglesias:
> On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
>> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
>>
>>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 
>> 10:03:24 -0700)
>>
>> are available in the git repository at:
>>
>>
>>   git://git.linaro.org/people/pmaydell/qemu-arm.git 
>> tags/pull-target-arm-20131025
>>
>> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
>>
>>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 
>> 18:27:07 +0100)
> 
> 
> Applied, thanks all.

Edgar, there is no merge commit in qemu.git despite this being a signed
pull. Do you maybe need to upgrade your version of git?

Peter, since I had picked up the first two patches into my still pending
qom-next pull, as per the QEMU Summit discussion those patches should've
gotten an Acked-by.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:18 PM, Andreas Färber  wrote:
> Hi,
>
> Am 31.10.2013 15:02, schrieb Edgar E. Iglesias:
>> On Fri, Oct 25, 2013 at 07:07:23PM +0100, Peter Maydell wrote:
>>> The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:
>>>
>>>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging 
>>> (2013-10-18 10:03:24 -0700)
>>>
>>> are available in the git repository at:
>>>
>>>
>>>   git://git.linaro.org/people/pmaydell/qemu-arm.git 
>>> tags/pull-target-arm-20131025
>>>
>>> for you to fetch changes up to 71c903cc3b78fc563122fe40c5cadd050068b91a:
>>>
>>>   integrator: fix Linux boot failure by emulating dbg region (2013-10-25 
>>> 18:27:07 +0100)
>>
>>
>> Applied, thanks all.
>
> Edgar, there is no merge commit in qemu.git despite this being a signed
> pull. Do you maybe need to upgrade your version of git?

Need to add:

[merge]
 ff = false

To your git config to prevent fast forwards on merging.

Regards,

Anthony Liguori

> Peter, since I had picked up the first two patches into my still pending
> qom-next pull, as per the QEMU Summit discussion those patches should've
> gotten an Acked-by.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Re: [Qemu-devel] [PATCH] import kvm-unittest in QEMU source tree

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 03:17:57PM +0100, Anthony Liguori wrote:
> On Thu, Oct 31, 2013 at 8:48 AM, Gleb Natapov  wrote:
> > On Wed, Oct 30, 2013 at 04:06:19PM -0700, Andrew Jones wrote:
> >> On Wed, Oct 16, 2013 at 10:03:37PM +0300, Michael S. Tsirkin wrote:
> >> > This simply imports kvm-unittest git into qemu source tree.
> >> >
> >> > We can next work on making make check run it
> >> > automatically.
> >> >
> >> > Squashed 'kvm-unittest/' content from commit 2bc0e29
> >> >
> >> > git-subtree-dir: kvm-unittest
> >> > git-subtree-split: 2bc0e29ee4447bebcd3b90053881f59265306adc
> >> >
> >> > Signed-off-by: Michael S. Tsirkin 
> >> >
> >> > ---
> >> >
> >> > Gleb, Paolo, any objections to this?  I really want a small guest for
> >> > running ACPI tests during make check, and kvm-unittest seems to fit the
> >> > bill.
> >> >
> >> > Ability to test e.g. PCI with this in-tree would be very benefitial.
> >> >
> >>
> >> Sorry for slow reply, I just noticed this mail now.
> >>
> >> So far qemu is a dependency for kvm-unit-tests (both x86 and arm use it),
> >> but at some point that could change (if another arch doesn't want to use
> >> qemu). So if that happens, then it won't make much sense for the tests to
> >> live in qemu. Thus I'm not 100% in favor of moving them. However, if the
> >> consensus is to move them, then I have two comments on this patch.
> >>
> > Do not worry, we are far from such consensus :)
> 
> I don't think kvm-unit-tests belong in QEMU either.  They are tied to
> the kernel side of things, not QEMU and many of the tests don't make
> sense for TCG.
> 
> If you want to execute guest code within QEMU's make check, we can
> copy the basic infrastructure in kvm-unit-tests.  It's so simple that
> a "fork" isn't really much of a concern to me.
> 
> Regards,
> 
> Anthony Liguori

For now I'm only testing the bios so it's even simpler,
I just put 16 bit code in the boot sector.


> >> 1) There are a couple pendings patches on the kvm list that tidy up the
> >> kvm-unit-tests repo - removing lots of the files. That should be
> >> committed first to avoid importing a bunch of files right before deleting
> >> them.
> >>
> >> 2) The name shouldn't change from kvm-unit-tests to kvm-unittest.
> >>
> >> drew
> >
> > --
> > Gleb.

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
> Hi All,
> 
> I know it's been a long time since this thread. But qemu 1.7 is
> releasing, do you have any consensus on this?
> 
> Thanks.


I think the biggest issue is the new PANICKED state.
Guests already have simple ways to halt the CPU,
and actually do.  I think a new state was a mistake.
So how about the following? Does it break anything?
(Untested).

Signed-off-by: Michael S. Tsirkin 

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 226e298..2055afc 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -51,7 +51,6 @@ static void handle_event(int event)
 
 if (event & PVPANIC_PANICKED) {
 panicked_mon_event("pause");
-vm_stop(RUN_STATE_GUEST_PANICKED);
 return;
 }
 }

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 14:18, Andreas Färber  wrote:
> Peter, since I had picked up the first two patches into my still pending
> qom-next pull, as per the QEMU Summit discussion those patches should've
> gotten an Acked-by.

Hmm? I don't recall this part of the discussion. If you want the
patches to have an Acked-by from you you need to send mail
to the list with an Acked-by line.

thanks
-- PMM

Re: [Qemu-devel] pvpanic plans?

[Eric Blake]

On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
> On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
>> Hi All,
>>
>> I know it's been a long time since this thread. But qemu 1.7 is
>> releasing, do you have any consensus on this?
>>
>> Thanks.
> 
> 
> I think the biggest issue is the new PANICKED state.
> Guests already have simple ways to halt the CPU,
> and actually do.  I think a new state was a mistake.
> So how about the following? Does it break anything?
> (Untested).
> 
> Signed-off-by: Michael S. Tsirkin 
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 226e298..2055afc 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -51,7 +51,6 @@ static void handle_event(int event)
>  
>  if (event & PVPANIC_PANICKED) {
>  panicked_mon_event("pause");
> -vm_stop(RUN_STATE_GUEST_PANICKED);

Don't you still need to halt the guest on a panic event, for management
to have a chance to choose what to do about the panic?  I'm suspecting
this patch does break things.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:32 PM, Eric Blake  wrote:
> On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
>> On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
>>> Hi All,
>>>
>>> I know it's been a long time since this thread. But qemu 1.7 is
>>> releasing, do you have any consensus on this?
>>>
>>> Thanks.
>>
>>
>> I think the biggest issue is the new PANICKED state.
>> Guests already have simple ways to halt the CPU,
>> and actually do.  I think a new state was a mistake.
>> So how about the following? Does it break anything?
>> (Untested).
>>
>> Signed-off-by: Michael S. Tsirkin 
>>
>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>> index 226e298..2055afc 100644
>> --- a/hw/misc/pvpanic.c
>> +++ b/hw/misc/pvpanic.c
>> @@ -51,7 +51,6 @@ static void handle_event(int event)
>>
>>  if (event & PVPANIC_PANICKED) {
>>  panicked_mon_event("pause");
>> -vm_stop(RUN_STATE_GUEST_PANICKED);
>
> Don't you still need to halt the guest on a panic event, for management
> to have a chance to choose what to do about the panic?  I'm suspecting
> this patch does break things.

I would be happy to apply a patch that just reverted the whole dang
mess of this device.

Regards,

Anthony Liguori

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 15:31, schrieb Peter Maydell:
> On 31 October 2013 14:18, Andreas Färber  wrote:
>> Peter, since I had picked up the first two patches into my still pending
>> qom-next pull, as per the QEMU Summit discussion those patches should've
>> gotten an Acked-by.
> 
> Hmm? I don't recall this part of the discussion. If you want the
> patches to have an Acked-by from you you need to send mail
> to the list with an Acked-by line.

No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
needs to be explicitly sent as reply but that "looks okay" should in
exactly such a case where sender=submaintainer should be recorded as
Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PULL v2 00/56] QOM devices patch queue 2013-10-31

[Andreas Färber]

Hello Anthony,

This is my updated QOM devices patch queue. Please pull or provide more details
of what is not working in your build environment.

v2 is rebased, dropping two ARM patches, and the observed SD segfault had been
fixed in 794cbc26eb94ce13c75d105eea9ff0afff56e2c2.
Patches are not manually changed, thus intentionally not resent.

Thanks,
Andreas

Cc: Anthony Liguori 

Cc: Peter Maydell 
Cc: Mian M. Hamayun 
Cc: Paolo Bonzini 
Cc: Michael S. Tsirkin 
Cc: Stefan Hajnoczi 
Cc: Edgar E. Iglesias 

The following changes since commit b86160555f8d1fe11d6bcec393e08e645d7e1e8d:

  integrator: fix Linux boot failure by emulating dbg region (2013-10-31 
14:00:16 +0100)

are available in the git repository at:

  git://github.com/afaerber/qemu-cpu.git tags/qom-devices-for-anthony

for you to fetch changes up to 30a423024ffaa28920c7e36c17855d7bd0061f09:

  pcmcia/pxa2xx: QOM'ify PXA2xxPCMCIAState (2013-10-31 15:12:30 +0100)


QOM device refactorings

* QTest coverage for all machines
* QOM realize for Milkymist UART
* QOM realize for ARM MPCore
* device_add bug fixes and cleanups
* QOM API extensions and cleanups
* QOM for PCMCIA/MicroDrive (last legacy IDE device)


Andreas Färber (49):
  mips_mipssim: Silence BIOS loading warning for qtest
  puv3: Turn puv3_load_kernel() into a no-op for qtest without -kernel
  mainstone: Don't enforce use of -pflash for qtest
  gumstix: Don't enforce use of -pflash for qtest
  z2: Don't enforce use of -pflash for qtest
  palm: Don't enforce loading ROM or kernel for qtest
  omap_sx1: Don't enforce use of kernel or flash for qtest
  exynos4_boards: Silence lack of -smp 2 warning for qtest
  armv7m: Don't enforce use of kernel for qtest
  axis_dev88: Don't enforce use of kernel for qtest
  mcf5208: Don't enforce use of kernel for qtest
  an5206: Don't enforce use of kernel for qtest
  milkymist: Suppress -kernel/-bios/-drive error for qtest
  shix: Drop debug output
  shix: Don't require firmware presence for qtest
  leon3: Don't enforce use of -bios with qtest
  qtest: Prepare QOM machine tests
  a9mpcore: Split off instance_init
  arm_gic: Extract headers hw/intc/arm_gic{,_common}.h
  a9mpcore: Embed GICState
  a9scu: QOM cleanups
  a9mpcore: Embed A9SCUState
  arm_mptimer: Convert to QOM realize
  a9mpcore: Embed ARMMPTimerState
  a9mpcore: Convert to QOM realize
  a9mpcore: Prepare for QOM embedding
  a15mpcore: Split off instance_init
  a15mpcore: Embed GICState
  a15mpcore: Convert to QOM realize
  a15mpcore: Prepare for QOM embedding
  a9scu: Build only once
  arm11mpcore: Fix typo in MemoryRegion name
  arm11mpcore: Drop unused fields
  arm11mpcore: Create container MemoryRegion in instance_init
  arm11mpcore: Split off SCU device
  arm11mpcore: Convert ARM11MPCorePriveState to QOM realize
  realview_gic: Convert to QOM realize
  realview_gic: Prepare for QOM embedding
  arm11mpcore: Convert mpcore_rirq_state to QOM realize
  arm11mpcore: Prepare for QOM embedding
  arm11mpcore: Split off RealView MPCore
  qdev-monitor: Clean up qdev_device_add() variable naming
  qdev-monitor: Avoid qdev as variable name
  qdev-monitor: Inline qdev_init() for device_add
  pxa: Fix typo "dettach"
  pcmcia: QOM'ify PCMCIACardState and MicroDriveState
  microdrive: Coding Style cleanups
  ide: Drop ide_init2_with_non_qdev_drives()
  pcmcia/pxa2xx: QOM'ify PXA2xxPCMCIAState

Antony Pavlov (1):
  milkymist-uart: Use Device::realize instead of SysBusDevice::init

Igor Mammedov (2):
  qdev-monitor: Fix crash when device_add is called with abstract driver
  qom: Include error.h directly in object.h

Michael S. Tsirkin (2):
  qom: Clean up struct Error references
  qom: Add pointer to int property helpers

Stefan Hajnoczi (2):
  qdev-monitor: Unref device when device_add fails
  qdev: Drop misleading qdev_free() function

 default-configs/arm-softmmu.mak  |   1 +
 hw/Makefile.objs |   1 +
 hw/acpi/piix4.c  |   2 +-
 hw/arm/armv7m.c  |  25 +--
 hw/arm/exynos4_boards.c  |   3 +-
 hw/arm/gumstix.c |  11 +-
 hw/arm/mainstone.c   |   5 +-
 hw/arm/omap_sx1.c|   3 +-
 hw/arm/palm.c|   3 +-
 hw/arm/z2.c  |   5 +-
 hw/block/tc58128.c   |  10 +-
 hw/char/milkymist-uart.c |  24 +--
 hw/core/qdev.c   |  12 +-
 hw/cpu/Makefile.objs |   1 +
 hw/cpu/a15mpcore.c   |  81 -
 hw/cpu/

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Il 31/10/2013 15:32, Eric Blake ha scritto:
> On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
>> On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
>>> Hi All,
>>> 
>>> I know it's been a long time since this thread. But qemu 1.7 is
>>> releasing, do you have any consensus on this?
>> 
>> I think the biggest issue is the new PANICKED state. Guests 
>> already have simple ways to halt the CPU, and actually do.  I 
>> think a new state was a mistake. So how about the following?
>> Does it break anything? (Untested).
> 
> Don't you still need to halt the guest on a panic event, for 
> management to have a chance to choose what to do about the panic? 
> I'm suspecting this patch does break things.

Yes, it does.  But I think that, once we make the pvpanic device is
optional, to a large extent there is no bug.  Adding the pvpanic
device to the VM will make libvirt obey  instead of the
in-guest setting, and that's it.

Two months have passed and no casualties have been reported due to
pvpanic.  Let's just remove the auto-pvpanic from all machine types in
1.7 (yes, that's backwards incompatible in a strict sense), document
it in the release notes, and hope that the old QEMU versions with
mandatory pvpanic die of old age.

All the advantages/disadvantages from my original messages still
apply.  Let's ignore the disadvantages and just KISS.

Paolo
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJScmuVAAoJEBvWZb6bTYbyS6MP/Rb9japkUmjKy+bUW2Bf5vHD
hOrCL/20LC17OdIdzOngjfcY6OyPrtZ6dtjzvSjsWoxjW+QNuTwLM5h+0kOmvlM/
UCqX9MZiKFysVphnwIFy2fzTKmAVw5WqUlS17XMsiKxpvCquy5Fss5PWB3k3orMD
kRCoTTmQDID95T38OqezvzDaRjNvoHXOTuWFOhbzWt2OFUwg1WDy1GLKV/pSx0/o
GS2/RYo5iQkEtZk0muj0fmWEZX1K2nfaYmVQh39LJhFrWQLabHskYz1+n/OTmwQT
3DiUkfeqrxLahlyHbIOguLjLY8gH2fZa0gzHEB7D0b1aiuqQmsxM3ELdl8pJJzex
1ZGKt9X1u58v/SvfDHW14uCMAiv8jgQDnDOa6pgi7DezBQym+90+RnzC2Z5BcHIp
hsPEB0Bc47REPu69GOSj7XQ1uan5yQS38jSv8D11nJEW8VfXiV4smOZhivrEOicR
mdYYV6BNc985vcVOmvmkTJ5VkUOKeMzMDAJkSDqN6P0fQKTOJpCtJag3TcjHVRB4
ORXUrdO9NuICGjwB75T86INkxEsXFaw1aHIKpcxMk2PN6u/Zc+n7GXf65ReGorbL
2QUrepzUUt+mTYXJha9h7gEudRnixe5zK8AgqtSHAJPKP++LrFz4lrmzCHC6e3V1
6K2AhZl4EjBJaf6AMy70
=GBBt
-END PGP SIGNATURE-

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber  wrote:
> Am 31.10.2013 15:31, schrieb Peter Maydell:
>> On 31 October 2013 14:18, Andreas Färber  wrote:
>>> Peter, since I had picked up the first two patches into my still pending
>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>> gotten an Acked-by.
>>
>> Hmm? I don't recall this part of the discussion. If you want the
>> patches to have an Acked-by from you you need to send mail
>> to the list with an Acked-by line.
>
> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
> needs to be explicitly sent as reply but that "looks okay" should in
> exactly such a case where sender=submaintainer should be recorded as
> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.

Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
make people infer your Acked-bys.

And adding tags is a nice-to-have.  There is no "rule" stating that
you must include everyone that appears on the mailing list.  But I
expect that maintainers try to

Regards,

Anthony Liguori

> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 08:32:43AM -0600, Eric Blake wrote:
> On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
> > On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
> >> Hi All,
> >>
> >> I know it's been a long time since this thread. But qemu 1.7 is
> >> releasing, do you have any consensus on this?
> >>
> >> Thanks.
> > 
> > 
> > I think the biggest issue is the new PANICKED state.
> > Guests already have simple ways to halt the CPU,
> > and actually do.  I think a new state was a mistake.
> > So how about the following? Does it break anything?
> > (Untested).
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > 
> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> > index 226e298..2055afc 100644
> > --- a/hw/misc/pvpanic.c
> > +++ b/hw/misc/pvpanic.c
> > @@ -51,7 +51,6 @@ static void handle_event(int event)
> >  
> >  if (event & PVPANIC_PANICKED) {
> >  panicked_mon_event("pause");
> > -vm_stop(RUN_STATE_GUEST_PANICKED);
> 
> Don't you still need to halt the guest on a panic event, for management
> to have a chance to choose what to do about the panic?

Guest can just call hlt to do this. Most guests do this on a panic
already.

> I'm suspecting
> this patch does break things.

http://xkcd.com/1172/

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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 15:39, schrieb Anthony Liguori:
> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber  wrote:
>> Am 31.10.2013 15:31, schrieb Peter Maydell:
>>> On 31 October 2013 14:18, Andreas Färber  wrote:
 Peter, since I had picked up the first two patches into my still pending
 qom-next pull, as per the QEMU Summit discussion those patches should've
 gotten an Acked-by.
>>>
>>> Hmm? I don't recall this part of the discussion. If you want the
>>> patches to have an Acked-by from you you need to send mail
>>> to the list with an Acked-by line.
>>
>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>> needs to be explicitly sent as reply but that "looks okay" should in
>> exactly such a case where sender=submaintainer should be recorded as
>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
> 
> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
> make people infer your Acked-bys.

Yes, that's in the minutes. And yes, that's what I got as answer there.
Please reply to the minutes if you think otherwise.

I brought up exactly this situation where I am contributor to CPU and
submaintainer of CPU and often not getting Reviewed-bys but if at all,
such as from Paolo recently, some verbal "looks OK" for a series. I was
told that that should be turned into an Acked-by on the patches to
satisfy your criteria that contributors may not just send patches as
pull without Reviewed-by.

> And adding tags is a nice-to-have.  There is no "rule" stating that
> you must include everyone that appears on the mailing list.  But I
> expect that maintainers try to

Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
we discussed whether a submaintainer must add a Reviewed-by then and
what to do if author==submaintainer. If you dropped that thought, then
fine with me.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] pvpanic plans?

[Eric Blake]

On 10/31/2013 08:47 AM, Michael S. Tsirkin wrote:

>>>  if (event & PVPANIC_PANICKED) {
>>>  panicked_mon_event("pause");
>>> -vm_stop(RUN_STATE_GUEST_PANICKED);
>>
>> Don't you still need to halt the guest on a panic event, for management
>> to have a chance to choose what to do about the panic?
> 
> Guest can just call hlt to do this. Most guests do this on a panic
> already.

On the one hand, the fact that the guest already has to inform the host
means we are already trusting the guest behavior on a panic.  On the
other hand, assuming that the guest will ALWAYS halt after triggering a
panic is putting a lot more trust in the guest, compared to qemu
explicitly halting the guest so that management has a chance to choose
to dump the guest's state at the moment the panic was flagged.

The biggest argument for either removing all auto-pvpanic, or reverting
pvpanic altogether, is that no one seems to be actively using pvpanic in
the field yet.  I wish we could get more feedback from Fujitsu as the
original patch authors on what they are looking for in a working
solution, rather than repeatedly second-guessing everything downstream
and delaying the eradication of the buggy behavior even longer.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 03:39:17PM +0100, Paolo Bonzini wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
> 
> Il 31/10/2013 15:32, Eric Blake ha scritto:
> > On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
> >> On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
> >>> Hi All,
> >>> 
> >>> I know it's been a long time since this thread. But qemu 1.7 is
> >>> releasing, do you have any consensus on this?
> >> 
> >> I think the biggest issue is the new PANICKED state. Guests 
> >> already have simple ways to halt the CPU, and actually do.  I 
> >> think a new state was a mistake. So how about the following?
> >> Does it break anything? (Untested).
> > 
> > Don't you still need to halt the guest on a panic event, for 
> > management to have a chance to choose what to do about the panic? 
> > I'm suspecting this patch does break things.
> 
> Yes, it does.

What does it break exactly?

> But I think that, once we make the pvpanic device is
> optional, to a large extent there is no bug.  Adding the pvpanic
> device to the VM will make libvirt obey  instead of the
> in-guest setting, and that's it.
> 
> Two months have passed and no casualties have been reported due to
> pvpanic.  Let's just remove the auto-pvpanic from all machine types in
> 1.7 (yes, that's backwards incompatible in a strict sense), document
> it in the release notes, and hope that the old QEMU versions with
> mandatory pvpanic die of old age.

Nod. I'm fine with that.

> All the advantages/disadvantages from my original messages still
> apply.  Let's ignore the disadvantages and just KISS.
> 
> Paolo

I think we still need to do get rid of the PANICKED state somehow.
If we can't replace it with RUNNING state, let's replace it with PAUSED.

For example, you can't continue from panicked for some reason.
You can't do a reset.
But you can pause and then continue.


> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJScmuVAAoJEBvWZb6bTYbyS6MP/Rb9japkUmjKy+bUW2Bf5vHD
> hOrCL/20LC17OdIdzOngjfcY6OyPrtZ6dtjzvSjsWoxjW+QNuTwLM5h+0kOmvlM/
> UCqX9MZiKFysVphnwIFy2fzTKmAVw5WqUlS17XMsiKxpvCquy5Fss5PWB3k3orMD
> kRCoTTmQDID95T38OqezvzDaRjNvoHXOTuWFOhbzWt2OFUwg1WDy1GLKV/pSx0/o
> GS2/RYo5iQkEtZk0muj0fmWEZX1K2nfaYmVQh39LJhFrWQLabHskYz1+n/OTmwQT
> 3DiUkfeqrxLahlyHbIOguLjLY8gH2fZa0gzHEB7D0b1aiuqQmsxM3ELdl8pJJzex
> 1ZGKt9X1u58v/SvfDHW14uCMAiv8jgQDnDOa6pgi7DezBQym+90+RnzC2Z5BcHIp
> hsPEB0Bc47REPu69GOSj7XQ1uan5yQS38jSv8D11nJEW8VfXiV4smOZhivrEOicR
> mdYYV6BNc985vcVOmvmkTJ5VkUOKeMzMDAJkSDqN6P0fQKTOJpCtJag3TcjHVRB4
> ORXUrdO9NuICGjwB75T86INkxEsXFaw1aHIKpcxMk2PN6u/Zc+n7GXf65ReGorbL
> 2QUrepzUUt+mTYXJha9h7gEudRnixe5zK8AgqtSHAJPKP++LrFz4lrmzCHC6e3V1
> 6K2AhZl4EjBJaf6AMy70
> =GBBt
> -END PGP SIGNATURE-

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber  wrote:
> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber  wrote:
>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
 On 31 October 2013 14:18, Andreas Färber  wrote:
> Peter, since I had picked up the first two patches into my still pending
> qom-next pull, as per the QEMU Summit discussion those patches should've
> gotten an Acked-by.

 Hmm? I don't recall this part of the discussion. If you want the
 patches to have an Acked-by from you you need to send mail
 to the list with an Acked-by line.
>>>
>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>> needs to be explicitly sent as reply but that "looks okay" should in
>>> exactly such a case where sender=submaintainer should be recorded as
>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>
>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>> make people infer your Acked-bys.
>
> Yes, that's in the minutes. And yes, that's what I got as answer there.
> Please reply to the minutes if you think otherwise.

I

> I brought up exactly this situation where I am contributor to CPU and
> submaintainer of CPU and often not getting Reviewed-bys but if at all,
> such as from Paolo recently, some verbal "looks OK" for a series. I was
> told that that should be turned into an Acked-by on the patches to
> satisfy your criteria that contributors may not just send patches as
> pull without Reviewed-by.
>
>> And adding tags is a nice-to-have.  There is no "rule" stating that
>> you must include everyone that appears on the mailing list.  But I
>> expect that maintainers try to
>
> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
> we discussed whether a submaintainer must add a Reviewed-by then and
> what to do if author==submaintainer. If you dropped that thought, then
> fine with me.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
>> > Yes, it does.
> What does it break exactly?

The point of a panicked event is to examine the guest at a particular
moment in time (e.g. host-initiated crash dump).  If you let the guest
run, it may reboot and prevent you from getting a meaningful dump.

>> > But I think that, once we make the pvpanic device is
>> > optional, to a large extent there is no bug.  Adding the pvpanic
>> > device to the VM will make libvirt obey  instead of the
>> > in-guest setting, and that's it.
>> > 
>> > Two months have passed and no casualties have been reported due to
>> > pvpanic.  Let's just remove the auto-pvpanic from all machine types in
>> > 1.7 (yes, that's backwards incompatible in a strict sense), document
>> > it in the release notes, and hope that the old QEMU versions with
>> > mandatory pvpanic die of old age.
> 
> Nod. I'm fine with that.
> 
> I think we still need to do get rid of the PANICKED state somehow.
> If we can't replace it with RUNNING state, let's replace it with PAUSED.
> 
> For example, you can't continue from panicked for some reason.
> You can't do a reset.  But you can pause and then continue.

We need to keep the PANICKED state, but we can make it a normal
"resumable" state.

Basically it's patches 1 and 2 at
http://permalink.gmane.org/gmane.comp.emulators.qemu/229131.  Rebasing
will fix the problem highlighted in the commit message of patch 2.

Paolo

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber  wrote:
> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber  wrote:
>>> Am 31.10.2013 15:31, schrieb Peter Maydell:
 On 31 October 2013 14:18, Andreas Färber  wrote:
> Peter, since I had picked up the first two patches into my still pending
> qom-next pull, as per the QEMU Summit discussion those patches should've
> gotten an Acked-by.

 Hmm? I don't recall this part of the discussion. If you want the
 patches to have an Acked-by from you you need to send mail
 to the list with an Acked-by line.
>>>
>>> No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
>>> needs to be explicitly sent as reply but that "looks okay" should in
>>> exactly such a case where sender=submaintainer should be recorded as
>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>
>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>> make people infer your Acked-bys.
>
> Yes, that's in the minutes. And yes, that's what I got as answer there.
> Please reply to the minutes if you think otherwise.

I explicitly said that Acked-bys are useless too.

The minutes say that you said the kernel treats "Acked-bys" as "looks
good".  You did say that.  At no point did a "rule" get made though.

> I brought up exactly this situation where I am contributor to CPU and
> submaintainer of CPU and often not getting Reviewed-bys but if at all,
> such as from Paolo recently, some verbal "looks OK" for a series. I was
> told that that should be turned into an Acked-by on the patches to
> satisfy your criteria that contributors may not just send patches as
> pull without Reviewed-by.

I think you misunderstood.

I don't care about Acked-bys.  They are useless.

A third of patches are being committed with Reviewed-bys.  There are
certainly many cases where patches are going in from submaintainers
that have been reviewed which comes implicitly with Signed-off-by.

But I worry that we're not reviewing enough on list and that there are
patches from maintainers going in through maintainer trees that aren't
getting outside review.

There's no immediate action for this other than we should all try to
review more patches on list to prevent the above situation.

>> And adding tags is a nice-to-have.  There is no "rule" stating that
>> you must include everyone that appears on the mailing list.  But I
>> expect that maintainers try to
>
> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
> we discussed whether a submaintainer must add a Reviewed-by then and
> what to do if author==submaintainer. If you dropped that thought, then
> fine with me.

Yes, patches should get reviewed.  I hope this is obvious to all of us :-)

I also suggested that I have tooling that people can use to simplify
adding collected Reviewed-bys on the list.

But none of this has anything to do with inferred Acked-bys.  I'll go
a step further and say that I would be very unhappy if anyone every
added any kind of tag to a patch with my name on it that I didn't send
myself.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] e1000 patch for osx

[Gabriel L. Somlo]

On Wed, Oct 30, 2013 at 6:31 PM, jacek burghardt wrote:
> I got this error
> hw/net/e1000.c: In function 'set_phy_ctrl':
> hw/net/e1000.c:209:10: warning: implicit declaration of function 'set_ics'
> [-Wimplicit-function-declaration]

That's because set_ics needs to be declared before being used in
set_phy_ctrl(). A full patch which builds on the current qemu git
master would look like below.

That being said, now it builds, but doesn't actually make osx
(10.6 in my case) start with a working network interface. I still
need to bounce the link (via "set_link foo off; set_link foo on")
from the qemu monitor command line in order to get network access
from osx.

--Gabriel

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 70a59fd..7c2dd9f 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -186,6 +186,9 @@ e1000_link_up(E1000State *s)
 s->phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
 }
 
+//FIXME: maybe move set_phy_ctrl *after* set_ics instead ?
+static void set_ics(E1000State *s, int index, uint32_t val);
+
 static void
 set_phy_ctrl(E1000State *s, int index, uint16_t val)
 {
@@ -203,6 +206,12 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
 DBGOUT(PHY, "Start link auto negotiation\n");
 timer_mod(s->autoneg_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 
500);
 }
+
+if (val & 0x8000) {
+val &= 0x7fff;
+set_ics(s, 0, E1000_ICR_LSC);
+}
+s->phy_reg[PHY_CTRL] = val;
 }
 
 static void
@@ -445,8 +454,9 @@ set_mdic(E1000State *s, int index, uint32_t val)
 } else {
 if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
 phyreg_writeops[addr](s, index, data);
+} else {
+s->phy_reg[addr] = data;
 }
-s->phy_reg[addr] = data;
 }
 }
 s->mac_reg[MDIC] = val | E1000_MDIC_READY;

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
> >> > Yes, it does.
> > What does it break exactly?
> 
> The point of a panicked event is to examine the guest at a particular
> moment in time (e.g. host-initiated crash dump).  If you let the guest
> run, it may reboot and prevent you from getting a meaningful dump.

Well we trust guest anyway, so I think we can trust it to call halt.


> >> > But I think that, once we make the pvpanic device is
> >> > optional, to a large extent there is no bug.  Adding the pvpanic
> >> > device to the VM will make libvirt obey  instead of the
> >> > in-guest setting, and that's it.
> >> > 
> >> > Two months have passed and no casualties have been reported due to
> >> > pvpanic.  Let's just remove the auto-pvpanic from all machine types in
> >> > 1.7 (yes, that's backwards incompatible in a strict sense), document
> >> > it in the release notes, and hope that the old QEMU versions with
> >> > mandatory pvpanic die of old age.
> > 
> > Nod. I'm fine with that.
> > 
> > I think we still need to do get rid of the PANICKED state somehow.
> > If we can't replace it with RUNNING state, let's replace it with PAUSED.
> > 
> > For example, you can't continue from panicked for some reason.
> > You can't do a reset.  But you can pause and then continue.
> 
> We need to keep the PANICKED state, but we can make it a normal
> "resumable" state.

If it's resumable how is it different from PAUSED?

> Basically it's patches 1 and 2 at
> http://permalink.gmane.org/gmane.comp.emulators.qemu/229131.  Rebasing
> will fix the problem highlighted in the commit message of patch 2.
> 
> Paolo

Looks like all transitions from paused state should be allowed from panicked
state. So why keep it separate?

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 08:49:08AM -0600, Eric Blake wrote:
> On 10/31/2013 08:47 AM, Michael S. Tsirkin wrote:
> 
> >>>  if (event & PVPANIC_PANICKED) {
> >>>  panicked_mon_event("pause");
> >>> -vm_stop(RUN_STATE_GUEST_PANICKED);
> >>
> >> Don't you still need to halt the guest on a panic event, for management
> >> to have a chance to choose what to do about the panic?
> > 
> > Guest can just call hlt to do this. Most guests do this on a panic
> > already.
> 
> On the one hand, the fact that the guest already has to inform the host
> means we are already trusting the guest behavior on a panic.  On the
> other hand, assuming that the guest will ALWAYS halt after triggering a
> panic is putting a lot more trust in the guest, compared to qemu
> explicitly halting the guest so that management has a chance to choose
> to dump the guest's state at the moment the panic was flagged.

I wouldn't call it *a lot* more trust. And again, this is guest policy:
if you want to do hlt from driver because you think it's safer, go for it.

> The biggest argument for either removing all auto-pvpanic, or reverting
> pvpanic altogether, is that no one seems to be actively using pvpanic in
> the field yet.  I wish we could get more feedback from Fujitsu as the
> original patch authors on what they are looking for in a working
> solution, rather than repeatedly second-guessing everything downstream
> and delaying the eradication of the buggy behavior even longer.


With my patch we have a benign device that merely reports io writes
on the monitor. No code -> no bugs.


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

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 14:36, Andreas Färber  wrote:
> Am 31.10.2013 15:31, schrieb Peter Maydell:
>> On 31 October 2013 14:18, Andreas Färber  wrote:
>>> Peter, since I had picked up the first two patches into my still pending
>>> qom-next pull, as per the QEMU Summit discussion those patches should've
>>> gotten an Acked-by.
>>
>> Hmm? I don't recall this part of the discussion. If you want the
>> patches to have an Acked-by from you you need to send mail
>> to the list with an Acked-by line.
>
> No, I added a Signed-off-by.

I checked my mail and the only thing I can find in reply to those
patches is a note from you saying you added them to your queue.

> It was clearly stated that a Reviewed-by
> needs to be explicitly sent as reply but that "looks okay" should in
> exactly such a case where sender=submaintainer should be recorded as
> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.

...but you're not the submaintainer here so I don't think this applies.

The point about the kernel practice as I understood it was that
the kernel folks treat acked-by at about the same level of review as
"looks ok to me" (ie, very little), not that there's some obligation to
treat any informal 'looks ok' note as an acked-by. I'm in full agreement
with Anthony that if you want a tag to appear you should send it
properly.

-- PMM

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
> On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
>> Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
> Yes, it does.
>>> What does it break exactly?
>>
>> The point of a panicked event is to examine the guest at a particular
>> moment in time (e.g. host-initiated crash dump).  If you let the guest
>> run, it may reboot and prevent you from getting a meaningful dump.
> 
> Well we trust guest anyway, so I think we can trust it to call halt.

No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
configuration.

> But I think that, once we make the pvpanic device is
> optional, to a large extent there is no bug.  Adding the pvpanic
> device to the VM will make libvirt obey  instead of the
> in-guest setting, and that's it.
>
> Two months have passed and no casualties have been reported due to
> pvpanic.  Let's just remove the auto-pvpanic from all machine types in
> 1.7 (yes, that's backwards incompatible in a strict sense), document
> it in the release notes, and hope that the old QEMU versions with
> mandatory pvpanic die of old age.
>>>
>>> Nod. I'm fine with that.
>>>
>>> I think we still need to do get rid of the PANICKED state somehow.
>>> If we can't replace it with RUNNING state, let's replace it with PAUSED.
>>>
>>> For example, you can't continue from panicked for some reason.
>>> You can't do a reset.  But you can pause and then continue.
>>
>> We need to keep the PANICKED state, but we can make it a normal
>> "resumable" state.
> 
> If it's resumable how is it different from PAUSED?

If the guest panics while for some reason libvirtd went down, libvirt
can see what happened.  It is similar to the "I/O error" state in this
respect.

> Looks like all transitions from paused state should be allowed from panicked
> state. So why keep it separate?

Because you can poll for the state instead of watching an event.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 04:26:13PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
> > On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
> >> Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
> > Yes, it does.
> >>> What does it break exactly?
> >>
> >> The point of a panicked event is to examine the guest at a particular
> >> moment in time (e.g. host-initiated crash dump).  If you let the guest
> >> run, it may reboot and prevent you from getting a meaningful dump.
> > 
> > Well we trust guest anyway, so I think we can trust it to call halt.
> 
> No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
> configuration.
>
> > But I think that, once we make the pvpanic device is
> > optional, to a large extent there is no bug.  Adding the pvpanic
> > device to the VM will make libvirt obey  instead of the
> > in-guest setting, and that's it.
> >
> > Two months have passed and no casualties have been reported due to
> > pvpanic.  Let's just remove the auto-pvpanic from all machine types in
> > 1.7 (yes, that's backwards incompatible in a strict sense), document
> > it in the release notes, and hope that the old QEMU versions with
> > mandatory pvpanic die of old age.
> >>>
> >>> Nod. I'm fine with that.
> >>>
> >>> I think we still need to do get rid of the PANICKED state somehow.
> >>> If we can't replace it with RUNNING state, let's replace it with PAUSED.
> >>>
> >>> For example, you can't continue from panicked for some reason.
> >>> You can't do a reset.  But you can pause and then continue.
> >>
> >> We need to keep the PANICKED state, but we can make it a normal
> >> "resumable" state.
> > 
> > If it's resumable how is it different from PAUSED?
> 
> If the guest panics while for some reason libvirtd went down, libvirt
> can see what happened.  It is similar to the "I/O error" state in this
> respect.
> 
> > Looks like all transitions from paused state should be allowed from panicked
> > state. So why keep it separate?
> 
> Because you can poll for the state instead of watching an event.
> 
> Paolo

I see. Maybe it was a mistake to use a separate runtime state for
this, but oh well.

So it should be exactly like paused, we can just find all transitions
from PAUSED and it should be same for PANICKED?
Why isn't DEBUG allowed from PAUSED but allowed from PANICKED then?
Maybe it should be allowed for PAUSED?

-- 
MST

[Qemu-devel] [PULL 04/30] qemu-img: add special exit code if bdrv_check is not supported

[Kevin Wolf]

From: Peter Lieven 

currently it is not possible to distinguish by exitcode if there
has been an error or if bdrv_check is not supported by the image
format. Change the exitcode from 1 to 63 for the latter case.

Signed-off-by: Peter Lieven 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 926f0a0..bf3fb4f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -607,7 +607,7 @@ static int img_check(int argc, char **argv)
 if (output_format == OFORMAT_HUMAN) {
 error_report("This image format does not support checks");
 }
-ret = 1;
+ret = 63;
 goto fail;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PULL v2 00/30] Block patches

[Kevin Wolf]

The following changes since commit fc8ead74674b7129e8f31c2595c76658e5622197:

  Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2013-10-18 
10:03:24 -0700)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git tags/for-anthony

for you to fetch changes up to f4c129a38a5430b7342a7a23f53a22831154612f:

  vmdk: Implment bdrv_get_specific_info (2013-10-31 14:44:43 +0100)


Block patches for 1.7.0-rc0 (v2)


Alexander Graf (1):
  ahci: fix win7 hang on boot

Eric Blake (1):
  qapi: fix documentation example

Fam Zheng (4):
  qemu-iotests: drop duplicated "create_image"
  qemu-iotests: prefill some data to test image
  qapi: Add optional field 'compressed' to ImageInfo
  vmdk: Implment bdrv_get_specific_info

Kevin Wolf (5):
  exec: Fix bounce buffer allocation in address_space_map()
  ide-test: Check what happens with bus mastering disabled
  tests: Multiboot mmap test case
  block: Avoid unecessary drv->bdrv_getlength() calls
  qemu-iotests: Fix 051 reference output

Liu Yuan (2):
  sheepdog: explicitly set copies as type uint8_t
  sheepdog: pass copy_policy in the request

MORITA Kazutaka (8):
  sheepdog: check return values of qemu_co_recv/send correctly
  sheepdog: handle vdi objects in resend_aio_req
  sheepdog: reload inode outside of resend_aioreq
  coroutine: add co_aio_sleep_ns() to allow sleep in block drivers
  sheepdog: try to reconnect to sheepdog after network error
  sheepdog: make add_aio_request and send_aioreq void functions
  sheepdog: cancel aio requests if possible
  sheepdog: check simultaneous create in resend_aioreq

Max Reitz (6):
  qcow2: Restore total_sectors value in save_vmstate
  qcow2: Unset zero_beyond_eof in save_vmstate
  qemu-iotests: Test for loading VM state from qcow2
  qcow2: Flush image after creation
  block: Don't copy backing file name on error
  qemu-iotests: Test case for backing file deletion

Peter Lieven (2):
  qemu-img: add special exit code if bdrv_check is not supported
  block/vpc: check that the image has not been truncated

Thibaut LAURENT (1):
  block: Disable BDRV_O_COPY_ON_READ for the backing file

 block.c |  14 +-
 block/qcow2.c   |  19 +++
 block/raw-posix.c   |   9 +-
 block/raw-win32.c   |   4 +-
 block/raw_bsd.c |   1 +
 block/sheepdog.c| 352 
 block/vmdk.c|  68 -
 block/vpc.c |   7 +
 docs/qapi-code-gen.txt  |   2 +-
 exec.c  |   4 +-
 hw/ide/ahci.c   |   3 +-
 include/block/block_int.h   |   3 +
 include/block/coroutine.h   |   9 ++
 qapi-schema.json|  28 +++-
 qemu-coroutine-sleep.c  |  14 ++
 qemu-img.c  |   2 +-
 tests/ide-test.c|  26 
 tests/multiboot/Makefile|  18 +++
 tests/multiboot/libc.c  | 139 +
 tests/multiboot/libc.h  |  61 
 tests/multiboot/link.ld |  19 +++
 tests/multiboot/mmap.c  |  56 +++
 tests/multiboot/mmap.out|  93 
 tests/multiboot/multiboot.h |  66 +
 tests/multiboot/run_test.sh |  81 ++
 tests/multiboot/start.S |  51 +++
 tests/qemu-iotests/030  |   4 +
 tests/qemu-iotests/040  |  14 +-
 tests/qemu-iotests/051.out  |   2 +-
 tests/qemu-iotests/059  |   2 +-
 tests/qemu-iotests/059.out  |   5 +-
 tests/qemu-iotests/068  |  65 
 tests/qemu-iotests/068.out  |  11 ++
 tests/qemu-iotests/069  |  59 
 tests/qemu-iotests/069.out  |   8 +
 tests/qemu-iotests/group|   2 +
 36 files changed, 1157 insertions(+), 164 deletions(-)
 create mode 100644 tests/multiboot/Makefile
 create mode 100644 tests/multiboot/libc.c
 create mode 100644 tests/multiboot/libc.h
 create mode 100644 tests/multiboot/link.ld
 create mode 100644 tests/multiboot/mmap.c
 create mode 100644 tests/multiboot/mmap.out
 create mode 100644 tests/multiboot/multiboot.h
 create mode 100755 tests/multiboot/run_test.sh
 create mode 100644 tests/multiboot/start.S
 create mode 100755 tests/qemu-iotests/068
 create mode 100644 tests/qemu-iotests/068.out
 create mode 100755 tests/qemu-iotests/069
 create mode 100644 tests/qemu-iotests/069.out

[Qemu-devel] [PULL 02/30] qcow2: Restore total_sectors value in save_vmstate

[Kevin Wolf]

From: Max Reitz 

Since df2a6f29a5, bdrv_co_do_writev increases the total_sectors value of
a growable block devices on writes after the current end. This leads to
the virtual disk apparently growing in qcow2_save_vmstate, which in turn
affects the disk size captured by the internal snapshot taken directly
afterwards through e.g. the HMP savevm command. Such a "grown" snapshot
cannot be loaded after reopening the qcow2 image, since its disk size
differs from the actual virtual disk size (writing a VM state does not
actually increase the virtual disk size).

Fix this by restoring total_sectors at the end of qcow2_save_vmstate.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index c1abaff..4a3e8b4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1939,6 +1939,7 @@ static int qcow2_save_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov,
   int64_t pos)
 {
 BDRVQcowState *s = bs->opaque;
+int64_t total_sectors = bs->total_sectors;
 int growable = bs->growable;
 int ret;
 
@@ -1947,6 +1948,11 @@ static int qcow2_save_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov,
 ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
 bs->growable = growable;
 
+/* bdrv_co_do_writev will have increased the total_sectors value to include
+ * the VM state - the VM state is however not an actual part of the block
+ * device, therefore, we need to restore the old value. */
+bs->total_sectors = total_sectors;
+
 return ret;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PULL 01/30] qapi: fix documentation example

[Kevin Wolf]

From: Eric Blake 

The QMP wire format uses "", not '', around strings.

* docs/qapi-code-gen.txt: Fix typo.

Signed-off-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 docs/qapi-code-gen.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 91f44d0..0728f36 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -164,7 +164,7 @@ This example allows using both of the following example 
objects:
  { "file": "my_existing_block_device_id" }
  { "file": { "driver": "file",
  "readonly": false,
- 'filename': "/tmp/mydisk.qcow2" } }
+ "filename": "/tmp/mydisk.qcow2" } }
 
 
 === Commands ===
-- 
1.8.1.4

[Qemu-devel] [PULL 05/30] block/vpc: check that the image has not been truncated

[Kevin Wolf]

From: Peter Lieven 

this adds a check that a dynamic VHD file has not been
accidently truncated (e.g. during transfer or upload).

Signed-off-by: Peter Lieven 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vpc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index b5dca39..627d11c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -260,6 +260,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 }
 
+if (s->free_data_block_offset > bdrv_getlength(bs->file)) {
+error_setg(errp, "block-vpc: free_data_block_offset points after "
+ "the end of file. The image has been truncated.");
+ret = -EINVAL;
+goto fail;
+}
+
 s->last_bitmap_offset = (int64_t) -1;
 
 #ifdef CACHE
-- 
1.8.1.4

[Qemu-devel] [PULL 03/30] qcow2: Unset zero_beyond_eof in save_vmstate

[Kevin Wolf]

From: Max Reitz 

Saving the VM state is done using bdrv_pwrite. This function may perform
a read-modify-write, which in this case results in data being read from
beyond the end of the virtual disk. Since we are actually trying to
access an area which is not a part of the virtual disk, zero_beyond_eof
has to be set to false before performing the partial write, otherwise
the VM state may become corrupted.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 4a3e8b4..01269f9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1941,12 +1941,15 @@ static int qcow2_save_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov,
 BDRVQcowState *s = bs->opaque;
 int64_t total_sectors = bs->total_sectors;
 int growable = bs->growable;
+bool zero_beyond_eof = bs->zero_beyond_eof;
 int ret;
 
 BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
 bs->growable = 1;
+bs->zero_beyond_eof = false;
 ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
 bs->growable = growable;
+bs->zero_beyond_eof = zero_beyond_eof;
 
 /* bdrv_co_do_writev will have increased the total_sectors value to include
  * the VM state - the VM state is however not an actual part of the block
-- 
1.8.1.4

[Qemu-devel] [PULL 07/30] qcow2: Flush image after creation

[Kevin Wolf]

From: Max Reitz 

Opening the qcow2 image with BDRV_O_NO_FLUSH prevents any flushes during
the image creation. This means that the image has not yet been flushed
to disk when qemu-img create exits. This flush is delayed until the next
operation on the image involving opening it without BDRV_O_NO_FLUSH and
closing (or directly flushing) it. For large images and/or images with a
small cluster size and preallocated metadata, this flush may take a
significant amount of time and may occur unexpectedly.

Reopening the image without BDRV_O_NO_FLUSH right before the end of
qcow2_create2() results in hoisting the potentially costly flush into
the image creation, which is expected to take some time (whereas
successive image operations may be not).

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Benoit Canet 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 01269f9..6e5d98d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1584,6 +1584,16 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 }
 }
 
+bdrv_close(bs);
+
+/* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
+ret = bdrv_open(bs, filename, NULL,
+BDRV_O_RDWR | BDRV_O_CACHE_WB, drv, &local_err);
+if (error_is_set(&local_err)) {
+error_propagate(errp, local_err);
+goto out;
+}
+
 ret = 0;
 out:
 bdrv_unref(bs);
-- 
1.8.1.4

[Qemu-devel] [PULL 21/30] sheepdog: handle vdi objects in resend_aio_req

[Kevin Wolf]

From: MORITA Kazutaka 

The current resend_aio_req() doesn't work when the request is against
vdi objects.  This fixes the problem.

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index c6e57f0..ddb8bfb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1197,11 +1197,15 @@ static int coroutine_fn resend_aioreq(BDRVSheepdogState 
*s, AIOReq *aio_req)
 return ret;
 }
 
-aio_req->oid = vid_to_data_oid(s->inode.vdi_id,
-   data_oid_to_idx(aio_req->oid));
+if (is_data_obj(aio_req->oid)) {
+aio_req->oid = vid_to_data_oid(s->inode.vdi_id,
+   data_oid_to_idx(aio_req->oid));
+} else {
+aio_req->oid = vid_to_vdi_oid(s->inode.vdi_id);
+}
 
 /* check whether this request becomes a CoW one */
-if (acb->aiocb_type == AIOCB_WRITE_UDATA) {
+if (acb->aiocb_type == AIOCB_WRITE_UDATA && is_data_obj(aio_req->oid)) {
 int idx = data_oid_to_idx(aio_req->oid);
 AIOReq *areq;
 
@@ -1229,8 +1233,15 @@ static int coroutine_fn resend_aioreq(BDRVSheepdogState 
*s, AIOReq *aio_req)
 create = true;
 }
 out:
-return add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov,
-   create, acb->aiocb_type);
+if (is_data_obj(aio_req->oid)) {
+return add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov,
+   create, acb->aiocb_type);
+} else {
+struct iovec iov;
+iov.iov_base = &s->inode;
+iov.iov_len = sizeof(s->inode);
+return add_aio_request(s, aio_req, &iov, 1, false, AIOCB_WRITE_UDATA);
+}
 }
 
 /* TODO Convert to fine grained options */
-- 
1.8.1.4

[Qemu-devel] [PULL 24/30] sheepdog: try to reconnect to sheepdog after network error

[Kevin Wolf]

From: MORITA Kazutaka 

This introduces a failed request queue and links all the inflight
requests to the list after network error happens.  After QEMU
reconnects to the sheepdog server successfully, the sheepdog block
driver will retry all the requests in the failed queue.

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 80 ++--
 1 file changed, 66 insertions(+), 14 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 5311fb1..fd1447e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -304,6 +304,8 @@ struct SheepdogAIOCB {
 };
 
 typedef struct BDRVSheepdogState {
+BlockDriverState *bs;
+
 SheepdogInode inode;
 
 uint32_t min_dirty_data_idx;
@@ -323,8 +325,11 @@ typedef struct BDRVSheepdogState {
 Coroutine *co_recv;
 
 uint32_t aioreq_seq_num;
+
+/* Every aio request must be linked to either of these queues. */
 QLIST_HEAD(inflight_aio_head, AIOReq) inflight_aio_head;
 QLIST_HEAD(pending_aio_head, AIOReq) pending_aio_head;
+QLIST_HEAD(failed_aio_head, AIOReq) failed_aio_head;
 } BDRVSheepdogState;
 
 static const char * sd_strerror(int err)
@@ -611,6 +616,8 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState 
*s, AIOReq *aio_req,
enum AIOCBState aiocb_type);
 static int coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req);
 static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char 
*tag);
+static int get_sheep_fd(BDRVSheepdogState *s);
+static void co_write_request(void *opaque);
 
 static AIOReq *find_pending_req(BDRVSheepdogState *s, uint64_t oid)
 {
@@ -652,6 +659,51 @@ static void coroutine_fn 
send_pending_req(BDRVSheepdogState *s, uint64_t oid)
 }
 }
 
+static coroutine_fn void reconnect_to_sdog(void *opaque)
+{
+BDRVSheepdogState *s = opaque;
+AIOReq *aio_req, *next;
+
+qemu_aio_set_fd_handler(s->fd, NULL, NULL, NULL);
+close(s->fd);
+s->fd = -1;
+
+/* Wait for outstanding write requests to be completed. */
+while (s->co_send != NULL) {
+co_write_request(opaque);
+}
+
+/* Try to reconnect the sheepdog server every one second. */
+while (s->fd < 0) {
+s->fd = get_sheep_fd(s);
+if (s->fd < 0) {
+DPRINTF("Wait for connection to be established\n");
+co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME,
+10ULL);
+}
+};
+
+/*
+ * Now we have to resend all the request in the inflight queue.  However,
+ * resend_aioreq() can yield and newly created requests can be added to the
+ * inflight queue before the coroutine is resumed.  To avoid mixing them, 
we
+ * have to move all the inflight requests to the failed queue before
+ * resend_aioreq() is called.
+ */
+QLIST_FOREACH_SAFE(aio_req, &s->inflight_aio_head, aio_siblings, next) {
+QLIST_REMOVE(aio_req, aio_siblings);
+QLIST_INSERT_HEAD(&s->failed_aio_head, aio_req, aio_siblings);
+}
+
+/* Resend all the failed aio requests. */
+while (!QLIST_EMPTY(&s->failed_aio_head)) {
+aio_req = QLIST_FIRST(&s->failed_aio_head);
+QLIST_REMOVE(aio_req, aio_siblings);
+QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
+resend_aioreq(s, aio_req);
+}
+}
+
 /*
  * Receive responses of the I/O requests.
  *
@@ -668,15 +720,11 @@ static void coroutine_fn aio_read_response(void *opaque)
 SheepdogAIOCB *acb;
 uint64_t idx;
 
-if (QLIST_EMPTY(&s->inflight_aio_head)) {
-goto out;
-}
-
 /* read a header */
 ret = qemu_co_recv(fd, &rsp, sizeof(rsp));
 if (ret != sizeof(rsp)) {
 error_report("failed to get the header, %s", strerror(errno));
-goto out;
+goto err;
 }
 
 /* find the right aio_req from the inflight aio list */
@@ -687,7 +735,7 @@ static void coroutine_fn aio_read_response(void *opaque)
 }
 if (!aio_req) {
 error_report("cannot find aio_req %x", rsp.id);
-goto out;
+goto err;
 }
 
 acb = aio_req->aiocb;
@@ -727,7 +775,7 @@ static void coroutine_fn aio_read_response(void *opaque)
 aio_req->iov_offset, rsp.data_length);
 if (ret != rsp.data_length) {
 error_report("failed to get the data, %s", strerror(errno));
-goto out;
+goto err;
 }
 break;
 case AIOCB_FLUSH_CACHE:
@@ -761,10 +809,9 @@ static void coroutine_fn aio_read_response(void *opaque)
 if (s->inode.vdi_id == oid_to_vid(aio_req->oid)) {
 ret = reload_inode(s, 0, "");
 if (ret < 0) {
-goto out;
+goto err;
 }
 }
-
 if (is_data_obj(aio_req->oid)) {
 aio_req->oid = vid_to_data_oid(s->inode.vdi_id,
 

[Qemu-devel] [PULL 20/30] sheepdog: check return values of qemu_co_recv/send correctly

[Kevin Wolf]

From: MORITA Kazutaka 

If qemu_co_recv/send doesn't return the specified length, it means
that an error happened.

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 9f0757b..c6e57f0 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -494,13 +494,13 @@ static coroutine_fn int send_co_req(int sockfd, 
SheepdogReq *hdr, void *data,
 int ret;
 
 ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
-if (ret < sizeof(*hdr)) {
+if (ret != sizeof(*hdr)) {
 error_report("failed to send a req, %s", strerror(errno));
 return ret;
 }
 
 ret = qemu_co_send(sockfd, data, *wlen);
-if (ret < *wlen) {
+if (ret != *wlen) {
 error_report("failed to send a req, %s", strerror(errno));
 }
 
@@ -546,7 +546,7 @@ static coroutine_fn void do_co_req(void *opaque)
 qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, co);
 
 ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
-if (ret < sizeof(*hdr)) {
+if (ret != sizeof(*hdr)) {
 error_report("failed to get a rsp, %s", strerror(errno));
 ret = -errno;
 goto out;
@@ -558,7 +558,7 @@ static coroutine_fn void do_co_req(void *opaque)
 
 if (*rlen) {
 ret = qemu_co_recv(sockfd, data, *rlen);
-if (ret < *rlen) {
+if (ret != *rlen) {
 error_report("failed to get the data, %s", strerror(errno));
 ret = -errno;
 goto out;
@@ -669,7 +669,7 @@ static void coroutine_fn aio_read_response(void *opaque)
 
 /* read a header */
 ret = qemu_co_recv(fd, &rsp, sizeof(rsp));
-if (ret < 0) {
+if (ret != sizeof(rsp)) {
 error_report("failed to get the header, %s", strerror(errno));
 goto out;
 }
@@ -720,7 +720,7 @@ static void coroutine_fn aio_read_response(void *opaque)
 case AIOCB_READ_UDATA:
 ret = qemu_co_recvv(fd, acb->qiov->iov, acb->qiov->niov,
 aio_req->iov_offset, rsp.data_length);
-if (ret < 0) {
+if (ret != rsp.data_length) {
 error_report("failed to get the data, %s", strerror(errno));
 goto out;
 }
@@ -1064,7 +1064,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState 
*s, AIOReq *aio_req,
 
 /* send a header */
 ret = qemu_co_send(s->fd, &hdr, sizeof(hdr));
-if (ret < 0) {
+if (ret != sizeof(hdr)) {
 qemu_co_mutex_unlock(&s->lock);
 error_report("failed to send a req, %s", strerror(errno));
 return -errno;
@@ -1072,7 +1072,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState 
*s, AIOReq *aio_req,
 
 if (wlen) {
 ret = qemu_co_sendv(s->fd, iov, niov, aio_req->iov_offset, wlen);
-if (ret < 0) {
+if (ret != wlen) {
 qemu_co_mutex_unlock(&s->lock);
 error_report("failed to send a data, %s", strerror(errno));
 return -errno;
-- 
1.8.1.4

[Qemu-devel] [PULL 08/30] exec: Fix bounce buffer allocation in address_space_map()

[Kevin Wolf]

This fixes a regression introduced by commit e3127ae0c, which kept the
allocation size of the bounce buffer limited to one page in order to
avoid unbounded allocations (as explained in the commit message of
6d16c2f88), but broke the reporting of the shortened bounce buffer to
the caller. The caller therefore assumes that the full requested size
was provided and causes memory corruption when writing beyond the end of
the actually allocated buffer.

Signed-off-by: Kevin Wolf 
---
 exec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 2e31ffc..b453713 100644
--- a/exec.c
+++ b/exec.c
@@ -2099,7 +2099,9 @@ void *address_space_map(AddressSpace *as,
 if (bounce.buffer) {
 return NULL;
 }
-bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
+/* Avoid unbounded allocations */
+l = MIN(l, TARGET_PAGE_SIZE);
+bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, l);
 bounce.addr = addr;
 bounce.len = l;
 
-- 
1.8.1.4

[Qemu-devel] [PULL 10/30] tests: Multiboot mmap test case

[Kevin Wolf]

This adds a test case for Multiboot memory map in the tests/multiboot
directory, where future i386 test kernels can be dropped. Because this
requires an x86 build host and an installed 32 bit libgcc, the test is
not part of a regular 'make check'.

The reference output for the test is verified against test runs of the
same multiboot kernel booted by some GRUB 0.97.

Signed-off-by: Kevin Wolf 
---
 tests/multiboot/Makefile|  18 ++
 tests/multiboot/libc.c  | 139 
 tests/multiboot/libc.h  |  61 +++
 tests/multiboot/link.ld |  19 ++
 tests/multiboot/mmap.c  |  56 ++
 tests/multiboot/mmap.out|  93 +
 tests/multiboot/multiboot.h |  66 +
 tests/multiboot/run_test.sh |  81 ++
 tests/multiboot/start.S |  51 
 9 files changed, 584 insertions(+)
 create mode 100644 tests/multiboot/Makefile
 create mode 100644 tests/multiboot/libc.c
 create mode 100644 tests/multiboot/libc.h
 create mode 100644 tests/multiboot/link.ld
 create mode 100644 tests/multiboot/mmap.c
 create mode 100644 tests/multiboot/mmap.out
 create mode 100644 tests/multiboot/multiboot.h
 create mode 100755 tests/multiboot/run_test.sh
 create mode 100644 tests/multiboot/start.S

diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
new file mode 100644
index 000..34cdd81
--- /dev/null
+++ b/tests/multiboot/Makefile
@@ -0,0 +1,18 @@
+CC=gcc
+CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin
+ASFLAGS=-m32
+
+LD=ld
+LDFLAGS=-melf_i386 -T link.ld
+LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
+
+all: mmap.elf
+
+mmap.elf: start.o mmap.o libc.o
+   $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
+
+%.o: %.c
+   $(CC) $(CCFLAGS) -c -o $@ $^
+
+%.o: %.S
+   $(CC) $(ASFLAGS) -c -o $@ $^
diff --git a/tests/multiboot/libc.c b/tests/multiboot/libc.c
new file mode 100644
index 000..05abbd9
--- /dev/null
+++ b/tests/multiboot/libc.c
@@ -0,0 +1,139 @@
+/*
+ * Copyright (c) 2013 Kevin Wolf 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "libc.h"
+
+static void print_char(char c)
+{
+outb(0xe9, c);
+}
+
+static void print_str(char *s)
+{
+while (*s) {
+print_char(*s++);
+}
+}
+
+static void print_num(uint64_t value, int base)
+{
+char digits[] = "0123456789abcdef";
+char buf[32] = { 0 };
+int i = sizeof(buf) - 2;
+
+do {
+buf[i--] = digits[value % base];
+value /= base;
+} while (value);
+
+print_str(&buf[i + 1]);
+}
+
+void printf(const char *fmt, ...)
+{
+va_list ap;
+uint64_t val;
+char *str;
+int base;
+int has_long;
+int alt_form;
+
+va_start(ap, fmt);
+
+for (; *fmt; fmt++) {
+if (*fmt != '%') {
+print_char(*fmt);
+continue;
+}
+fmt++;
+
+if (*fmt == '#') {
+fmt++;
+alt_form = 1;
+} else {
+alt_form = 0;
+}
+
+if (*fmt == 'l') {
+fmt++;
+if (*fmt == 'l') {
+fmt++;
+has_long = 2;
+} else {
+has_long = 1;
+}
+} else {
+has_long = 0;
+}
+
+switch (*fmt) {
+case 'x':
+case 'p':
+base = 16;
+goto convert_number;
+case 'd':
+case 'i':
+case 'u':
+base = 10;
+goto convert_number;
+case 'o':
+base = 8;
+goto convert_number;
+
+convert_number:
+switch (has_long) {
+case 0:
+val = va_arg(ap, unsigned int);
+break;
+case 1:
+val = va_arg(ap, unsigned long);
+break;
+case 2:
+val = va_arg(ap, unsi

[Qemu-devel] [PULL 25/30] sheepdog: make add_aio_request and send_aioreq void functions

[Kevin Wolf]

From: MORITA Kazutaka 

These functions no longer return errors.  We can make them void
functions and simplify the codes.

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 66 +++-
 1 file changed, 17 insertions(+), 49 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fd1447e..8790494 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -611,10 +611,10 @@ static int do_req(int sockfd, SheepdogReq *hdr, void 
*data,
 return srco.ret;
 }
 
-static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
+static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
struct iovec *iov, int niov, bool create,
enum AIOCBState aiocb_type);
-static int coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req);
+static void coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req);
 static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char 
*tag);
 static int get_sheep_fd(BDRVSheepdogState *s);
 static void co_write_request(void *opaque);
@@ -640,22 +640,14 @@ static void coroutine_fn 
send_pending_req(BDRVSheepdogState *s, uint64_t oid)
 {
 AIOReq *aio_req;
 SheepdogAIOCB *acb;
-int ret;
 
 while ((aio_req = find_pending_req(s, oid)) != NULL) {
 acb = aio_req->aiocb;
 /* move aio_req from pending list to inflight one */
 QLIST_REMOVE(aio_req, aio_siblings);
 QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
-ret = add_aio_request(s, aio_req, acb->qiov->iov,
-  acb->qiov->niov, false, acb->aiocb_type);
-if (ret < 0) {
-error_report("add_aio_request is failed");
-free_aio_req(s, aio_req);
-if (!acb->nr_pending) {
-sd_finish_aiocb(acb);
-}
-}
+add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov, false,
+acb->aiocb_type);
 }
 }
 
@@ -818,11 +810,8 @@ static void coroutine_fn aio_read_response(void *opaque)
 } else {
 aio_req->oid = vid_to_vdi_oid(s->inode.vdi_id);
 }
-ret = resend_aioreq(s, aio_req);
-if (ret == SD_RES_SUCCESS) {
-goto out;
-}
-/* fall through */
+resend_aioreq(s, aio_req);
+goto out;
 default:
 acb->ret = -EIO;
 error_report("%s", sd_strerror(rsp.result));
@@ -1071,7 +1060,7 @@ out:
 return ret;
 }
 
-static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
+static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
struct iovec *iov, int niov, bool create,
enum AIOCBState aiocb_type)
 {
@@ -1149,8 +1138,6 @@ out:
 qemu_aio_set_fd_handler(s->fd, co_read_response, NULL, s);
 s->co_send = NULL;
 qemu_co_mutex_unlock(&s->lock);
-
-return 0;
 }
 
 static int read_write_object(int fd, char *buf, uint64_t oid, uint8_t copies,
@@ -1253,7 +1240,7 @@ out:
 return ret;
 }
 
-static int coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req)
+static void coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req)
 {
 SheepdogAIOCB *acb = aio_req->aiocb;
 bool create = false;
@@ -1278,7 +1265,7 @@ static int coroutine_fn resend_aioreq(BDRVSheepdogState 
*s, AIOReq *aio_req)
 DPRINTF("simultaneous CoW to %" PRIx64 "\n", aio_req->oid);
 QLIST_REMOVE(aio_req, aio_siblings);
 QLIST_INSERT_HEAD(&s->pending_aio_head, aio_req, aio_siblings);
-return SD_RES_SUCCESS;
+return;
 }
 }
 
@@ -1288,13 +1275,13 @@ static int coroutine_fn resend_aioreq(BDRVSheepdogState 
*s, AIOReq *aio_req)
 }
 out:
 if (is_data_obj(aio_req->oid)) {
-return add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov,
-   create, acb->aiocb_type);
+add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov, create,
+acb->aiocb_type);
 } else {
 struct iovec iov;
 iov.iov_base = &s->inode;
 iov.iov_len = sizeof(s->inode);
-return add_aio_request(s, aio_req, &iov, 1, false, AIOCB_WRITE_UDATA);
+add_aio_request(s, aio_req, &iov, 1, false, AIOCB_WRITE_UDATA);
 }
 }
 
@@ -1697,7 +1684,6 @@ static int sd_truncate(BlockDriverState *bs, int64_t 
offset)
  */
 static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
 {
-int ret;
 BDRVSheepdogState *s = acb->common.bs->opaque;
 struct iovec iov;
 AIOReq *aio_req;
@@ -1719,18 +1705,13 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB 
*acb)
 aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
 

[Qemu-devel] [PULL 30/30] vmdk: Implment bdrv_get_specific_info

[Kevin Wolf]

From: Fam Zheng 

Implement .bdrv_get_specific_info to return the extent information.

Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 block/vmdk.c   | 68 +-
 qapi-schema.json   | 24 +++-
 tests/qemu-iotests/059 |  2 +-
 tests/qemu-iotests/059.out |  5 ++--
 4 files changed, 93 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 32ec8b7..a7ebd0f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -106,6 +106,7 @@ typedef struct VmdkExtent {
 uint32_t l2_cache_counts[L2_CACHE_SIZE];
 
 int64_t cluster_sectors;
+char *type;
 } VmdkExtent;
 
 typedef struct BDRVVmdkState {
@@ -113,11 +114,13 @@ typedef struct BDRVVmdkState {
 uint64_t desc_offset;
 bool cid_updated;
 bool cid_checked;
+uint32_t cid;
 uint32_t parent_cid;
 int num_extents;
 /* Extent array with num_extents entries, ascend ordered by address */
 VmdkExtent *extents;
 Error *migration_blocker;
+char *create_type;
 } BDRVVmdkState;
 
 typedef struct VmdkMetaData {
@@ -214,6 +217,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
 g_free(e->l1_table);
 g_free(e->l2_cache);
 g_free(e->l1_backup_table);
+g_free(e->type);
 if (e->file != bs->file) {
 bdrv_unref(e->file);
 }
@@ -534,6 +538,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
 uint32_t l1_size, l1_entry_sectors;
 VMDK4Header header;
 VmdkExtent *extent;
+BDRVVmdkState *s = bs->opaque;
 int64_t l1_backup_offset = 0;
 
 ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
@@ -549,6 +554,10 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
 }
 }
 
+if (!s->create_type) {
+s->create_type = g_strdup("monolithicSparse");
+}
+
 if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) {
 /*
  * The footer takes precedence over the header, so read it in. The
@@ -709,6 +718,8 @@ static int vmdk_parse_extents(const char *desc, 
BlockDriverState *bs,
 int64_t flat_offset;
 char extent_path[PATH_MAX];
 BlockDriverState *extent_file;
+BDRVVmdkState *s = bs->opaque;
+VmdkExtent *extent;
 
 while (*p) {
 /* parse extent line:
@@ -751,7 +762,6 @@ static int vmdk_parse_extents(const char *desc, 
BlockDriverState *bs,
 /* save to extents array */
 if (!strcmp(type, "FLAT") || !strcmp(type, "VMFS")) {
 /* FLAT extent */
-VmdkExtent *extent;
 
 ret = vmdk_add_extent(bs, extent_file, true, sectors,
 0, 0, 0, 0, 0, &extent, errp);
@@ -766,10 +776,12 @@ static int vmdk_parse_extents(const char *desc, 
BlockDriverState *bs,
 bdrv_unref(extent_file);
 return ret;
 }
+extent = &s->extents[s->num_extents - 1];
 } else {
 error_setg(errp, "Unsupported extent type '%s'", type);
 return -ENOTSUP;
 }
+extent->type = g_strdup(type);
 next_line:
 /* move to next line */
 while (*p) {
@@ -817,6 +829,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int 
flags,
 ret = -ENOTSUP;
 goto exit;
 }
+s->create_type = g_strdup(ct);
 s->desc_offset = 0;
 ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
 exit:
@@ -843,6 +856,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, 
int flags,
 if (ret) {
 goto fail;
 }
+s->cid = vmdk_read_cid(bs, 0);
 s->parent_cid = vmdk_read_cid(bs, 1);
 qemu_co_mutex_init(&s->lock);
 
@@ -855,6 +869,8 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, 
int flags,
 return 0;
 
 fail:
+g_free(s->create_type);
+s->create_type = NULL;
 vmdk_free_extents(bs);
 return ret;
 }
@@ -1766,6 +1782,7 @@ static void vmdk_close(BlockDriverState *bs)
 BDRVVmdkState *s = bs->opaque;
 
 vmdk_free_extents(bs);
+g_free(s->create_type);
 
 migrate_del_blocker(s->migration_blocker);
 error_free(s->migration_blocker);
@@ -1827,6 +1844,54 @@ static int vmdk_has_zero_init(BlockDriverState *bs)
 return 1;
 }
 
+static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
+{
+ImageInfo *info = g_new0(ImageInfo, 1);
+
+*info = (ImageInfo){
+.filename = g_strdup(extent->file->filename),
+.format   = g_strdup(extent->type),
+.virtual_size = extent->sectors * BDRV_SECTOR_SIZE,
+.compressed   = extent->compressed,
+.has_compressed   = extent->compressed,
+.cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE,
+.has_cluster_size = !extent->flat,
+};
+
+return info;
+}
+
+static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
+{
+int i;
+BDRVVmdkState *s = bs->opaque;
+ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific,

[Qemu-devel] [PULL 15/30] block: Disable BDRV_O_COPY_ON_READ for the backing file

[Kevin Wolf]

From: Thibaut LAURENT 

Since commit 0ebd24e0a203cf2852c310b59fbe050190dc6c8c,
bdrv_open_common will throw an error when trying to open a file
read-only with the BDRV_O_COPY_ON_READ flag set.
Although BDRV_O_RDWR is unset for the backing files,
BDRV_O_COPY_ON_READ is still passed on if copy-on-read was requested
for the drive. Let's unset this flag too before opening the backing
file, or bdrv_open_common will fail.

Signed-off-by: Thibaut LAURENT 
Reviewed-by: Benoit Canet 
Reviewed-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 block.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 366999b..61795fe 100644
--- a/block.c
+++ b/block.c
@@ -999,7 +999,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 }
 
 /* backing files always opened read-only */
-back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
+back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
+BDRV_O_COPY_ON_READ);
 
 ret = bdrv_open(bs->backing_hd,
 *backing_filename ? backing_filename : NULL, options,
-- 
1.8.1.4

[Qemu-devel] [PULL 11/30] block: Don't copy backing file name on error

[Kevin Wolf]

From: Max Reitz 

bdrv_open_backing_file() tries to copy the backing file name using
pstrcpy directly after calling bdrv_open() to open the backing file
without checking whether that was actually successful. If it was not,
ps->backing_hd->file will probably be NULL and qemu will crash.

Fix this by moving pstrcpy after checking whether bdrv_open() succeeded.

Signed-off-by: Max Reitz 
Reviewed-by: Benoit Canet 
Reviewed-by: Amos Kong 
Signed-off-by: Kevin Wolf 
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index fd05a80..366999b 100644
--- a/block.c
+++ b/block.c
@@ -1004,8 +1004,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 ret = bdrv_open(bs->backing_hd,
 *backing_filename ? backing_filename : NULL, options,
 back_flags, back_drv, &local_err);
-pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-bs->backing_hd->file->filename);
 if (ret < 0) {
 bdrv_unref(bs->backing_hd);
 bs->backing_hd = NULL;
@@ -1013,6 +1011,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 error_propagate(errp, local_err);
 return ret;
 }
+pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+bs->backing_hd->file->filename);
 return 0;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PULL 16/30] block: Avoid unecessary drv->bdrv_getlength() calls

[Kevin Wolf]

The block layer generally keeps the size of an image cached in
bs->total_sectors so that it doesn't have to perform expensive
operations to get the size whenever it needs it.

This doesn't work however when using a backend that can change its size
without qemu being aware of it, i.e. passthrough of removable media like
CD-ROMs or floppy disks. For this reason, the caching is disabled when a
removable device is used.

It is obvious that checking whether the _guest_ device has removable
media isn't the right thing to do when we want to know whether the size
of the host backend can change. To make things worse, non-top-level
BlockDriverStates never have any device attached, which makes qemu
assume they are removable, so drv->bdrv_getlength() is always called on
the protocol layer. In the case of raw-posix, this causes unnecessary
lseek() system calls, which turned out to be rather expensive.

This patch completely changes the logic and disables bs->total_sectors
caching only for certain block driver types, for which a size change is
expected: host_cdrom and host_floppy on POSIX, host_device on win32; also
the raw format in case it sits on top of one of these protocols, but in
the common case the nested bdrv_getlength() call on the protocol driver
will use the cache again and avoid an expensive drv->bdrv_getlength()
call.

Signed-off-by: Kevin Wolf 
Reviewed-by: Paolo Bonzini 
---
 block.c   | 7 ---
 block/raw-posix.c | 9 ++---
 block/raw-win32.c | 4 +++-
 block/raw_bsd.c   | 1 +
 include/block/block_int.h | 3 +++
 5 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 61795fe..58efb5b 100644
--- a/block.c
+++ b/block.c
@@ -2869,9 +2869,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
 if (!drv)
 return -ENOMEDIUM;
 
-if (bdrv_dev_has_removable_media(bs)) {
-if (drv->bdrv_getlength) {
-return drv->bdrv_getlength(bs);
+if (drv->has_variable_length) {
+int ret = refresh_total_sectors(bs, bs->total_sectors);
+if (ret < 0) {
+return ret;
 }
 }
 return bs->total_sectors * BDRV_SECTOR_SIZE;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6f03fbf..f6d48bb 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1715,7 +1715,8 @@ static BlockDriver bdrv_host_floppy = {
 .bdrv_aio_flush= raw_aio_flush,
 
 .bdrv_truncate  = raw_truncate,
-.bdrv_getlength= raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
@@ -1824,7 +1825,8 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_aio_flush= raw_aio_flush,
 
 .bdrv_truncate  = raw_truncate,
-.bdrv_getlength = raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
@@ -1951,7 +1953,8 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_aio_flush= raw_aio_flush,
 
 .bdrv_truncate  = raw_truncate,
-.bdrv_getlength = raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 676b570..2741e4d 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -616,7 +616,9 @@ static BlockDriver bdrv_host_device = {
 .bdrv_aio_writev= raw_aio_writev,
 .bdrv_aio_flush = raw_aio_flush,
 
-.bdrv_getlength= raw_getlength,
+.bdrv_getlength  = raw_getlength,
+.has_variable_length = true,
+
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 };
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 0078c1b..2265dcc 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -178,6 +178,7 @@ static BlockDriver bdrv_raw = {
 .bdrv_co_get_block_status = &raw_co_get_block_status,
 .bdrv_truncate= &raw_truncate,
 .bdrv_getlength   = &raw_getlength,
+.has_variable_length  = true,
 .bdrv_get_info= &raw_get_info,
 .bdrv_is_inserted = &raw_is_inserted,
 .bdrv_media_changed   = &raw_media_changed,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a48731d..1666066 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -156,8 +156,11 @@ struct BlockDriver {
 
 const char *protocol_name;
 int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
+
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
+bool has_variable_length;
 int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
+
 int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
  const uint8_t *buf, int nb_sectors);
 
-- 
1.8.1.4

[Qemu-devel] [PULL 12/30] sheepdog: explicitly set copies as type uint8_t

[Kevin Wolf]

From: Liu Yuan 

'copies' is actually uint8_t since day one, but request headers and some helper
functions parameterize it as uint32_t for unknown reasons and effectively
reserve 24 bytes for possible future use. This patch explicitly set the correct
for copies and reserve the left bytes.

This is a preparation patch that allow passing copy_policy in request header.

Cc: Kevin Wolf 
Cc: Stefan Hajnoczi 
Signed-off-by: Liu Yuan 
Acked-by: MORITA Kazutaka 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 5f81c93..b8a2985 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -125,8 +125,8 @@ typedef struct SheepdogObjReq {
 uint32_t data_length;
 uint64_t oid;
 uint64_t cow_oid;
-uint32_t copies;
-uint32_t rsvd;
+uint8_t copies;
+uint8_t reserved[7];
 uint64_t offset;
 } SheepdogObjReq;
 
@@ -138,7 +138,8 @@ typedef struct SheepdogObjRsp {
 uint32_t id;
 uint32_t data_length;
 uint32_t result;
-uint32_t copies;
+uint8_t copies;
+uint8_t reserved[3];
 uint32_t pad[6];
 } SheepdogObjRsp;
 
@@ -151,7 +152,8 @@ typedef struct SheepdogVdiReq {
 uint32_t data_length;
 uint64_t vdi_size;
 uint32_t vdi_id;
-uint32_t copies;
+uint8_t copies;
+uint8_t reserved[3];
 uint32_t snapid;
 uint32_t pad[3];
 } SheepdogVdiReq;
@@ -1081,7 +1083,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState 
*s, AIOReq *aio_req,
 return 0;
 }
 
-static int read_write_object(int fd, char *buf, uint64_t oid, int copies,
+static int read_write_object(int fd, char *buf, uint64_t oid, uint8_t copies,
  unsigned int datalen, uint64_t offset,
  bool write, bool create, uint32_t cache_flags)
 {
@@ -1129,7 +1131,7 @@ static int read_write_object(int fd, char *buf, uint64_t 
oid, int copies,
 }
 }
 
-static int read_object(int fd, char *buf, uint64_t oid, int copies,
+static int read_object(int fd, char *buf, uint64_t oid, uint8_t copies,
unsigned int datalen, uint64_t offset,
uint32_t cache_flags)
 {
@@ -1137,7 +1139,7 @@ static int read_object(int fd, char *buf, uint64_t oid, 
int copies,
  false, cache_flags);
 }
 
-static int write_object(int fd, char *buf, uint64_t oid, int copies,
+static int write_object(int fd, char *buf, uint64_t oid, uint8_t copies,
 unsigned int datalen, uint64_t offset, bool create,
 uint32_t cache_flags)
 {
-- 
1.8.1.4

[Qemu-devel] [PATCH 0/7] block: qemu-iotests, more quoting fixes

[Jeff Cody]

There are still problems with the qemu-iotests, if the pathname of the working
directory contains spaces.  This series fixes most of them.  To verify,
 ./check -qcow2 was run from a pathname with spaces (e.g., renamed ~/qemu-kvm
to "~/qemu-kvm test/").

The following tests still fail due to pathnames with spaces, and
are not addressed in this series: 059, 067.


Jeff Cody (7):
  block: qemu-iotests, add quotes to $TEST_IMG usage io pattern tests
  block: qemu-iotests, fix _make_test_img() to work with spaced
pathnames
  block: qemu-iotests, add quotes to $TEST_IMG.base usage in 017
  block: qemu-iotests, add quotes to $TEST_IMG usage in 019
  block: qemu-iotests, removes duplicate double quotes in 039
  block: qemu-iotests, add quotes to $TEST_IMG usage for 051
  block: qemu-iotests, add quotes to $TEST_IMG usage in 061

 tests/qemu-iotests/017|  2 +-
 tests/qemu-iotests/019|  6 +++---
 tests/qemu-iotests/039|  2 +-
 tests/qemu-iotests/051|  8 
 tests/qemu-iotests/061|  6 +++---
 tests/qemu-iotests/common.pattern | 12 ++--
 tests/qemu-iotests/common.rc  | 13 +++--
 7 files changed, 29 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] block: qemu-iotests, add quotes to $TEST_IMG usage in 019

[Jeff Cody]

There were still instances of $TEST_IMG not being properly quoted.
This was in the usage of a string built up for a 'for' loop; modify
the loop so we can quote $TEST_IMG properly.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/019 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
index cd3582c..5bb18d0 100755
--- a/tests/qemu-iotests/019
+++ b/tests/qemu-iotests/019
@@ -90,12 +90,12 @@ mv "$TEST_IMG" "$TEST_IMG.orig"
 # Test the conversion twice: One test with the old-style -B option and another
 # one with -o backing_file
 
-for backing_option in "-B $TEST_IMG.base" "-o backing_file=$TEST_IMG.base"; do
+for backing_option in "-B " "-o backing_file="; do
 
 echo
-echo Testing conversion with $backing_option | _filter_testdir | 
_filter_imgfmt
+echo Testing conversion with $backing_option$TEST_IMG.base | 
_filter_testdir | _filter_imgfmt
 echo
-$QEMU_IMG convert -O $IMGFMT $backing_option "$TEST_IMG.orig" "$TEST_IMG"
+$QEMU_IMG convert -O $IMGFMT $backing_option"$TEST_IMG.base" 
"$TEST_IMG.orig" "$TEST_IMG"
 
 echo "Checking if backing clusters are allocated when they shouldn't"
 echo
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] block: qemu-iotests, fix _make_test_img() to work with spaced pathnames

[Jeff Cody]

_make_test_img() currently works with spaced pathnames only when not
specifying a backing file.  This fixes it so that the backing file
argument is properly quoted.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/common.rc | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4e82604..d24de2c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -111,6 +111,8 @@ _make_test_img()
 local image_size=$*
 local optstr=""
 local img_name=""
+local use_backing=0
+local backing_file=""
 
 if [ -n "$TEST_IMG_FILE" ]; then
 img_name=$TEST_IMG_FILE
@@ -123,7 +125,8 @@ _make_test_img()
 fi
 
 if [ "$1" = "-b" ]; then
-extra_img_options="$1 $2"
+use_backing=1
+backing_file=$2
 image_size=$3
 fi
 if [ \( "$IMGFMT" = "qcow2" -o "$IMGFMT" = "qed" \) -a -n "$CLUSTER_SIZE" 
]; then
@@ -135,7 +138,13 @@ _make_test_img()
 fi
 
 # XXX(hch): have global image options?
-$QEMU_IMG create -f $IMGFMT $extra_img_options $img_name $image_size 2>&1 
| \
+(
+ if [ $use_backing = 1 ]; then
+$QEMU_IMG create -f $IMGFMT $extra_img_options -b "$backing_file" 
"$img_name" $image_size 2>&1
+ else
+$QEMU_IMG create -f $IMGFMT $extra_img_options "$img_name" $image_size 
2>&1
+ fi
+) | \
 sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
 -e "s#$TEST_DIR#TEST_DIR#g" \
 -e "s#$IMGFMT#IMGFMT#g" \
-- 
1.8.3.1

[Qemu-devel] [PULL 17/30] qemu-iotests: Fix 051 reference output

[Kevin Wolf]

Commit 684b254 forgot to update it.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/051.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 2839e32..15deef6 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -24,7 +24,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo block
 ide0-hd0: TEST_DIR/t.qcow2 (qcow2)
 Backing file: TEST_DIR/t.qcow2.orig (chain depth: 1)
- [not inserted](qemu) qququiquit
+(qemu) qququiquit
 
 
 === Enable and disable lazy refcounting on the command line, plus some invalid 
values ===
-- 
1.8.1.4

[Qemu-devel] [PULL 13/30] sheepdog: pass copy_policy in the request

[Kevin Wolf]

From: Liu Yuan 

Currently copy_policy isn't used. Recent sheepdog supports erasure coding, which
make use of copy_policy internally, but require client explicitly passing
copy_policy from base inode to newly creately inode for snapshot related
operations.

If connected sheep daemon doesn't utilize copy_policy, passing it to sheep
daemon is just one extra null effect operation. So no compatibility problem.

With this patch, sheepdog can provide erasure coded volume for QEMU VM.

Cc: Kevin Wolf 
Cc: Stefan Hajnoczi 
Signed-off-by: Liu Yuan 
Acked-by: MORITA Kazutaka 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index b8a2985..9f0757b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -126,7 +126,8 @@ typedef struct SheepdogObjReq {
 uint64_t oid;
 uint64_t cow_oid;
 uint8_t copies;
-uint8_t reserved[7];
+uint8_t copy_policy;
+uint8_t reserved[6];
 uint64_t offset;
 } SheepdogObjReq;
 
@@ -139,7 +140,8 @@ typedef struct SheepdogObjRsp {
 uint32_t data_length;
 uint32_t result;
 uint8_t copies;
-uint8_t reserved[3];
+uint8_t copy_policy;
+uint8_t reserved[2];
 uint32_t pad[6];
 } SheepdogObjRsp;
 
@@ -153,7 +155,8 @@ typedef struct SheepdogVdiReq {
 uint64_t vdi_size;
 uint32_t vdi_id;
 uint8_t copies;
-uint8_t reserved[3];
+uint8_t copy_policy;
+uint8_t reserved[2];
 uint32_t snapid;
 uint32_t pad[3];
 } SheepdogVdiReq;
@@ -1346,7 +1349,8 @@ out:
 }
 
 static int do_sd_create(BDRVSheepdogState *s, char *filename, int64_t vdi_size,
-uint32_t base_vid, uint32_t *vdi_id, int snapshot)
+uint32_t base_vid, uint32_t *vdi_id, int snapshot,
+uint8_t copy_policy)
 {
 SheepdogVdiReq hdr;
 SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
@@ -1376,6 +1380,7 @@ static int do_sd_create(BDRVSheepdogState *s, char 
*filename, int64_t vdi_size,
 
 hdr.data_length = wlen;
 hdr.vdi_size = vdi_size;
+hdr.copy_policy = copy_policy;
 
 ret = do_req(fd, (SheepdogReq *)&hdr, buf, &wlen, &rlen);
 
@@ -1528,7 +1533,8 @@ static int sd_create(const char *filename, 
QEMUOptionParameter *options,
 bdrv_unref(bs);
 }
 
-ret = do_sd_create(s, vdi, vdi_size, base_vid, &vid, 0);
+/* TODO: allow users to specify copy number */
+ret = do_sd_create(s, vdi, vdi_size, base_vid, &vid, 0, 0);
 if (!prealloc || ret) {
 goto out;
 }
@@ -1718,7 +1724,7 @@ static int sd_create_branch(BDRVSheepdogState *s)
  */
 deleted = sd_delete(s);
 ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &vid,
-   !deleted);
+   !deleted, s->inode.copy_policy);
 if (ret) {
 goto out;
 }
@@ -2008,7 +2014,7 @@ static int sd_snapshot_create(BlockDriverState *bs, 
QEMUSnapshotInfo *sn_info)
 }
 
 ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, 
&new_vid,
-   1);
+   1, s->inode.copy_policy);
 if (ret < 0) {
 error_report("failed to create inode for snapshot. %s",
  strerror(errno));
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/7] block: qemu-iotests, add quotes to $TEST_IMG.base usage in 017

[Jeff Cody]

$TEST_IMG.base is used unquoted. Add quotes so that pathnames with
spaces are supported.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/017 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index 45f2c0b..aba3faf 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -66,7 +66,7 @@ echo "Creating test image with backing file"
 echo
 
 TEST_IMG=$TEST_IMG_SAVE
-_make_test_img -b $TEST_IMG.base 6G
+_make_test_img -b "$TEST_IMG.base" 6G
 
 echo "Filling test image"
 echo
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] block: qemu-iotests, removes duplicate double quotes in 039

[Jeff Cody]

Test 039 had $TEST_IMG with duplicate double quotes - remove duplicate.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/039 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index f85b4ce..8bade92 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -54,7 +54,7 @@ echo "== Checking that image is clean on shutdown =="
 IMGOPTS="compat=1.1,lazy_refcounts=on"
 _make_test_img $size
 
-$QEMU_IO -c "write -P 0x5a 0 512" ""$TEST_IMG"" | _filter_qemu_io
+$QEMU_IO -c "write -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
 
 # The dirty bit must not be set
 ./qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
-- 
1.8.3.1

[Qemu-devel] [PULL 18/30] qemu-iotests: drop duplicated "create_image"

[Kevin Wolf]

From: Fam Zheng 

There's a same common function in iotests.py

Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/040 | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index aad535a..a2e18c5 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -54,22 +54,12 @@ class ImageCommitTestCase(iotests.QMPTestCase):
 
 self.assert_no_active_commit()
 
-def create_image(self, name, size):
-file = open(name, 'w')
-i = 0
-while i < size:
-sector = struct.pack('>l504xl', i / 512, i / 512)
-file.write(sector)
-i = i + 512
-file.close()
-
-
 class TestSingleDrive(ImageCommitTestCase):
 image_len = 1 * 1024 * 1024
 test_len = 1 * 1024 * 256
 
 def setUp(self):
-self.create_image(backing_img, TestSingleDrive.image_len)
+iotests.create_image(backing_img, TestSingleDrive.image_len)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, mid_img)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
mid_img, test_img)
 qemu_io('-c', 'write -P 0xab 0 524288', backing_img)
@@ -167,7 +157,7 @@ class TestRelativePaths(ImageCommitTestCase):
 except OSError as exception:
 if exception.errno != errno.EEXIST:
 raise
-self.create_image(self.backing_img_abs, TestRelativePaths.image_len)
+iotests.create_image(self.backing_img_abs, TestRelativePaths.image_len)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
self.backing_img_abs, self.mid_img_abs)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
self.mid_img_abs, self.test_img)
 qemu_img('rebase', '-u', '-b', self.backing_img, self.mid_img_abs)
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/7] block: qemu-iotests, add quotes to $TEST_IMG usage for 051

[Jeff Cody]

There were still a couple of instances of unquoted usage of
$TEST_IMG and $TEST_IMG.orig.  Quoted these so they will not fail
on pathnames with spaces in them.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/051 | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 356c375..0a4971d 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -64,9 +64,9 @@ function run_qemu()
 size=128M
 
 _make_test_img $size
-cp $TEST_IMG $TEST_IMG.orig
-mv $TEST_IMG $TEST_IMG.base
-_make_test_img -b $TEST_IMG.base $size
+cp "$TEST_IMG" "$TEST_IMG.orig"
+mv "$TEST_IMG" "$TEST_IMG.base"
+_make_test_img -b "$TEST_IMG.base" $size
 
 echo
 echo === Unknown option ===
@@ -81,7 +81,7 @@ echo
 echo === Overriding backing file ===
 echo
 
-echo "info block" | run_qemu -drive 
file=$TEST_IMG,driver=qcow2,backing.file.filename=$TEST_IMG.orig -nodefaults
+echo "info block" | run_qemu -drive 
file="$TEST_IMG",driver=qcow2,backing.file.filename="$TEST_IMG.orig" -nodefaults
 
 echo
 echo === Enable and disable lazy refcounting on the command line, plus some 
invalid values ===
-- 
1.8.3.1

[Qemu-devel] [PULL 19/30] qemu-iotests: Test case for backing file deletion

[Kevin Wolf]

From: Max Reitz 

Add a test case for trying to open an image file where it is impossible
to open its backing file (in this case, because it was deleted). When
doing this, qemu (or qemu-io in this case) should not crash but rather
print an appropriate error message.

Signed-off-by: Max Reitz 
Reviewed-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/069 | 59 ++
 tests/qemu-iotests/069.out |  8 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 68 insertions(+)
 create mode 100755 tests/qemu-iotests/069
 create mode 100644 tests/qemu-iotests/069.out

diff --git a/tests/qemu-iotests/069 b/tests/qemu-iotests/069
new file mode 100755
index 000..3042803
--- /dev/null
+++ b/tests/qemu-iotests/069
@@ -0,0 +1,59 @@
+#!/bin/bash
+#
+# Test case for deleting a backing file
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=mre...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt cow qed qcow qcow2 vmdk
+_supported_proto generic
+_supported_os Linux
+
+IMG_SIZE=128K
+
+echo
+echo "=== Creating an image with a backing file and deleting that file ==="
+echo
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE
+_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
+rm -f "$TEST_IMG.base"
+# Just open the image and close it right again (this should print an error 
message)
+$QEMU_IO -c quit "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/069.out b/tests/qemu-iotests/069.out
new file mode 100644
index 000..3648814
--- /dev/null
+++ b/tests/qemu-iotests/069.out
@@ -0,0 +1,8 @@
+QA output created by 069
+
+=== Creating an image with a backing file and deleting that file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=131072 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 
backing_file='TEST_DIR/t.IMGFMT.base' 
+qemu-io: can't open device TEST_DIR/t.IMGFMT: Could not open file: No such 
file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3ca9cba..c57ff35 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -74,3 +74,4 @@
 066 rw auto
 067 rw auto
 068 rw auto
+069 rw auto
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/7] block: qemu-iotests, add quotes to $TEST_IMG usage in 061

[Jeff Cody]

When creating images with backing files in the test, the backing
file argument was not quoted properly.  This caused the test to fail
when run from a pathname with a space.  Pass the backing argument in
with the -b option to _make_test_img, so it can be properly quoted.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/061 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index fa9319d..e42f9bd 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -163,7 +163,7 @@ echo "=== Testing zero expansion on backed image ==="
 echo
 IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 64M
 $QEMU_IO -c "write -P 0x2a 0 128k" "$TEST_IMG.base" | _filter_qemu_io
-IMGOPTS="compat=1.1,backing_file=$TEST_IMG.base" _make_test_img 64M
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 64M
 $QEMU_IO -c "read -P 0x2a 0 128k" -c "write -z 0 64k" "$TEST_IMG" | 
_filter_qemu_io
 $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
 _check_test_img
@@ -174,7 +174,7 @@ echo "=== Testing zero expansion on backed inactive 
clusters ==="
 echo
 IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 64M
 $QEMU_IO -c "write -P 0x2a 0 128k" "$TEST_IMG.base" | _filter_qemu_io
-IMGOPTS="compat=1.1,backing_file=$TEST_IMG.base" _make_test_img 64M
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 64M
 $QEMU_IO -c "write -z 0 64k" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG snapshot -c foo "$TEST_IMG"
 $QEMU_IO -c "write -P 0x42 0 128k" "$TEST_IMG" | _filter_qemu_io
@@ -190,7 +190,7 @@ echo "=== Testing zero expansion on backed image with 
shared L2 table ==="
 echo
 IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 64M
 $QEMU_IO -c "write -P 0x2a 0 128k" "$TEST_IMG.base" | _filter_qemu_io
-IMGOPTS="compat=1.1,backing_file=$TEST_IMG.base" _make_test_img 64M
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 64M
 $QEMU_IO -c "write -z 0 128k" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG snapshot -c foo "$TEST_IMG"
 $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
-- 
1.8.3.1

[Qemu-devel] [PULL 22/30] sheepdog: reload inode outside of resend_aioreq

[Kevin Wolf]

From: MORITA Kazutaka 

This prepares for using resend_aioreq() after reconnecting to the
sheepdog server.

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 33 +++--
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index ddb8bfb..5311fb1 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -227,6 +227,11 @@ static inline uint64_t data_oid_to_idx(uint64_t oid)
 return oid & (MAX_DATA_OBJS - 1);
 }
 
+static inline uint32_t oid_to_vid(uint64_t oid)
+{
+return (oid & ~VDI_BIT) >> VDI_SPACE_SHIFT;
+}
+
 static inline uint64_t vid_to_vdi_oid(uint32_t vid)
 {
 return VDI_BIT | ((uint64_t)vid << VDI_SPACE_SHIFT);
@@ -605,7 +610,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState 
*s, AIOReq *aio_req,
struct iovec *iov, int niov, bool create,
enum AIOCBState aiocb_type);
 static int coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req);
-
+static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char 
*tag);
 
 static AIOReq *find_pending_req(BDRVSheepdogState *s, uint64_t oid)
 {
@@ -753,6 +758,19 @@ static void coroutine_fn aio_read_response(void *opaque)
 case SD_RES_SUCCESS:
 break;
 case SD_RES_READONLY:
+if (s->inode.vdi_id == oid_to_vid(aio_req->oid)) {
+ret = reload_inode(s, 0, "");
+if (ret < 0) {
+goto out;
+}
+}
+
+if (is_data_obj(aio_req->oid)) {
+aio_req->oid = vid_to_data_oid(s->inode.vdi_id,
+   data_oid_to_idx(aio_req->oid));
+} else {
+aio_req->oid = vid_to_vdi_oid(s->inode.vdi_id);
+}
 ret = resend_aioreq(s, aio_req);
 if (ret == SD_RES_SUCCESS) {
 goto out;
@@ -1190,19 +1208,6 @@ static int coroutine_fn resend_aioreq(BDRVSheepdogState 
*s, AIOReq *aio_req)
 {
 SheepdogAIOCB *acb = aio_req->aiocb;
 bool create = false;
-int ret;
-
-ret = reload_inode(s, 0, "");
-if (ret < 0) {
-return ret;
-}
-
-if (is_data_obj(aio_req->oid)) {
-aio_req->oid = vid_to_data_oid(s->inode.vdi_id,
-   data_oid_to_idx(aio_req->oid));
-} else {
-aio_req->oid = vid_to_vdi_oid(s->inode.vdi_id);
-}
 
 /* check whether this request becomes a CoW one */
 if (acb->aiocb_type == AIOCB_WRITE_UDATA && is_data_obj(aio_req->oid)) {
-- 
1.8.1.4

[Qemu-devel] [PULL 28/30] qemu-iotests: prefill some data to test image

[Kevin Wolf]

From: Fam Zheng 

Case 030 occasionally fails because of block job compltes too fast to be
captured by script, and 'unexpected qmp event' of job completion causes
the test failure.

Simply fill in some data to the test image to make this false alarm less
likely to happen.

(For other benefits to prefill data to test image, see also commit
ab68cdfaa).

Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/030 | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index ae56f3b..d0f96ea 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -388,7 +388,9 @@ class TestStreamStop(iotests.QMPTestCase):
 
 def setUp(self):
 qemu_img('create', backing_img, str(TestStreamStop.image_len))
+qemu_io('-c', 'write -P 0x1 0 32M', backing_img)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, test_img)
+qemu_io('-c', 'write -P 0x1 32M 32M', test_img)
 self.vm = iotests.VM().add_drive(test_img)
 self.vm.launch()
 
@@ -414,7 +416,9 @@ class TestSetSpeed(iotests.QMPTestCase):
 
 def setUp(self):
 qemu_img('create', backing_img, str(TestSetSpeed.image_len))
+qemu_io('-c', 'write -P 0x1 0 32M', backing_img)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, test_img)
+qemu_io('-c', 'write -P 0x1 32M 32M', test_img)
 self.vm = iotests.VM().add_drive(test_img)
 self.vm.launch()
 
-- 
1.8.1.4

[Qemu-devel] [PULL 29/30] qapi: Add optional field 'compressed' to ImageInfo

[Kevin Wolf]

From: Fam Zheng 

Signed-off-by: Fam Zheng 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 qapi-schema.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 60f3fd1..add97e2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -256,6 +256,8 @@
 #
 # @encrypted: #optional true if the image is encrypted
 #
+# @compressed: #optional true if the image is compressed (Since 1.7)
+#
 # @backing-filename: #optional name of the backing file
 #
 # @full-backing-filename: #optional full path of the backing file
@@ -276,7 +278,7 @@
 { 'type': 'ImageInfo',
   'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
'*actual-size': 'int', 'virtual-size': 'int',
-   '*cluster-size': 'int', '*encrypted': 'bool',
+   '*cluster-size': 'int', '*encrypted': 'bool', '*compressed': 'bool',
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
'*backing-image': 'ImageInfo',
-- 
1.8.1.4

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 04:56:07PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 16:45, Michael S. Tsirkin ha scritto:
> > On Thu, Oct 31, 2013 at 04:26:13PM +0100, Paolo Bonzini wrote:
> >> Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
> >>> On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
>  Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
> >>> Yes, it does.
> > What does it break exactly?
> 
>  The point of a panicked event is to examine the guest at a particular
>  moment in time (e.g. host-initiated crash dump).  If you let the guest
>  run, it may reboot and prevent you from getting a meaningful dump.
> >>>
> >>> Well we trust guest anyway, so I think we can trust it to call halt.
> >>
> >> No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
> >> configuration.
> >>
> >>> But I think that, once we make the pvpanic device is
> >>> optional, to a large extent there is no bug.  Adding the pvpanic
> >>> device to the VM will make libvirt obey  instead of the
> >>> in-guest setting, and that's it.
> >>>
> >>> Two months have passed and no casualties have been reported due to
> >>> pvpanic.  Let's just remove the auto-pvpanic from all machine types in
> >>> 1.7 (yes, that's backwards incompatible in a strict sense), document
> >>> it in the release notes, and hope that the old QEMU versions with
> >>> mandatory pvpanic die of old age.
> >
> > Nod. I'm fine with that.
> >
> > I think we still need to do get rid of the PANICKED state somehow.
> > If we can't replace it with RUNNING state, let's replace it with PAUSED.
> >
> > For example, you can't continue from panicked for some reason.
> > You can't do a reset.  But you can pause and then continue.
> 
>  We need to keep the PANICKED state, but we can make it a normal
>  "resumable" state.
> >>>
> >>> If it's resumable how is it different from PAUSED?
> >>
> >> If the guest panics while for some reason libvirtd went down, libvirt
> >> can see what happened.  It is similar to the "I/O error" state in this
> >> respect.
> >>
> >>> Looks like all transitions from paused state should be allowed from 
> >>> panicked
> >>> state. So why keep it separate?
> >>
> >> Because you can poll for the state instead of watching an event.
> > 
> > I see. Maybe it was a mistake to use a separate runtime state for
> > this, but oh well.
> 
> Yes, we should have had a list of "reasons" why a guest is stopped (I/O
> error, panic, gdb, ...) and a command to clear one or more of them;
> there can be paused/running/waiting-for-migration/... states, but many
> of the states we have are not necessarily in mutual exclusion.
> 
> But we cannot really choose now.
> 
> > So it should be exactly like paused, we can just find all transitions
> > from PAUSED and it should be same for PANICKED?
> > Why isn't DEBUG allowed from PAUSED but allowed from PANICKED then?
> > Maybe it should be allowed for PAUSED?
> 
> PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
> reverted if the panicked state is removed from runstate_needs_reset.
> 
> Paolo

Okay so let's drop the code duplication and explicitly make
them the same?

Signed-off-by: Michael S. Tsirkin 


diff --git a/vl.c b/vl.c
index 46c29c4..e12d317 100644
--- a/vl.c
+++ b/vl.c
@@ -638,10 +638,6 @@ static const RunStateTransition runstate_transitions_def[] 
= {
 { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
 { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
 
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
-
 { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
@@ -660,6 +656,12 @@ static void runstate_init(void)
 
 for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
 runstate_valid_transitions[p->from][p->to] = true;
+/* Panicked state is same as paused, we only made it different so
+ * management can detect a panic.
+ */
+if (p->from == RUN_STATE_PAUSED) {
+runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = true;
+}
 }
 }
 
@@ -686,8 +688,7 @@ int runstate_is_running(void)
 bool runstate_needs_reset(void)
 {
 return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-runstate_check(RUN_STATE_SHUTDOWN) ||
-runstate_check(RUN_STATE_GUEST_PANICKED);
+runstate_check(RUN_STATE_SHUTDOWN);
 }
 
 StatusInfo *qmp_query_status(Error **errp)

Re: [Qemu-devel] [PATCH v5 2/2] vmdk: Implment bdrv_get_specific_info

[Eric Blake]

On 10/30/2013 08:06 PM, Fam Zheng wrote:
> Implement .bdrv_get_specific_info to return the extent information.
> 
> Signed-off-by: Fam Zheng 
> 
> ---
> v5: Changed 3 lines:
> - Save cid in vmdk_open. (Kevin)
> - Report parent_cid in vmdk_get_specific_info.
> - Rename create_type to create-type in qapi-schema.json.
> 

> +++ b/qapi-schema.json
> @@ -225,6 +225,27 @@
>} }
>  
>  ##
> +# @ImageInfoSpecificVmdk:
> +#
> +# @create_type: The create type of VMDK image

Mismatch...

> +#
> +# @cid: Content id of image
> +#
> +# @parent-cid: Parent VMDK image's cid
> +#
> +# @extents: List of extent files
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'ImageInfoSpecificVmdk',
> +  'data': {
> +  'create-type': 'str',

...from the actual spelling.

> +  'cid': 'int',
> +  'parent-cid': 'int',
> +  'extents': ['ImageInfo']
> +  } }
> +

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PULL 30/30] vmdk: Implment bdrv_get_specific_info

[Eric Blake]

On 10/31/2013 09:48 AM, Kevin Wolf wrote:
> From: Fam Zheng 
> 
> Implement .bdrv_get_specific_info to return the extent information.
> 
> Signed-off-by: Fam Zheng 
> Signed-off-by: Kevin Wolf 
> ---

> +++ b/qapi-schema.json
> @@ -225,6 +225,27 @@
>} }
>  
>  ##
> +# @ImageInfoSpecificVmdk:
> +#
> +# @create_type: The create type of VMDK image

If it's not too late, can we fix this typo without having to do it in a
followup patch? s/_/-/

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto:
>> PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
>> reverted if the panicked state is removed from runstate_needs_reset.
> 
> Okay so let's drop the code duplication and explicitly make
> them the same?
> 
> Signed-off-by: Michael S. Tsirkin 
> 
> 
> diff --git a/vl.c b/vl.c
> index 46c29c4..e12d317 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -638,10 +638,6 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>  { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>  { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>  
> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> -
>  { RUN_STATE_MAX, RUN_STATE_MAX },
>  };
>  
> @@ -660,6 +656,12 @@ static void runstate_init(void)
>  
>  for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
>  runstate_valid_transitions[p->from][p->to] = true;
> +/* Panicked state is same as paused, we only made it different so
> + * management can detect a panic.
> + */
> +if (p->from == RUN_STATE_PAUSED) {
> +runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = 
> true;

It makes only sense to me if you do that for IO_ERROR and WATCHDOG as
well, and perhaps there are others I'm missing.  Just add a comment
before runstate_transitions_def's entries for PANICKED, IO_ERROR and
WATCHDOG.

But again, it is somewhat separate from the issue at hand, which is to
finally make pvpanic usable and hopefully before 1.7.

Paolo

> +}
>  }
>  }
>  
> @@ -686,8 +688,7 @@ int runstate_is_running(void)
>  bool runstate_needs_reset(void)
>  {
>  return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -runstate_check(RUN_STATE_SHUTDOWN) ||
> -runstate_check(RUN_STATE_GUEST_PANICKED);
> +runstate_check(RUN_STATE_SHUTDOWN);
>  }
>  
>  StatusInfo *qmp_query_status(Error **errp)
> 

Re: [Qemu-devel] [PULL 30/30] vmdk: Implment bdrv_get_specific_info

[Kevin Wolf]

Am 31.10.2013 um 17:13 hat Eric Blake geschrieben:
> On 10/31/2013 09:48 AM, Kevin Wolf wrote:
> > From: Fam Zheng 
> > 
> > Implement .bdrv_get_specific_info to return the extent information.
> > 
> > Signed-off-by: Fam Zheng 
> > Signed-off-by: Kevin Wolf 
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -225,6 +225,27 @@
> >} }
> >  
> >  ##
> > +# @ImageInfoSpecificVmdk:
> > +#
> > +# @create_type: The create type of VMDK image
> 
> If it's not too late, can we fix this typo without having to do it in a
> followup patch? s/_/-/

Okay, I'll fix that quickly, but I'm not going send a v3 on the list.
Let's just hope that nobody's looking. :-)

Kevin


pgprfdhQckl9k.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/7] block: qemu-iotests, fix _make_test_img() to work with spaced pathnames

[Eric Blake]

On 10/31/2013 09:57 AM, Jeff Cody wrote:
> _make_test_img() currently works with spaced pathnames only when not
> specifying a backing file.  This fixes it so that the backing file
> argument is properly quoted.
> 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/common.rc | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

As a slight optimization, you could avoid a subshell:

>  # XXX(hch): have global image options?
> -$QEMU_IMG create -f $IMGFMT $extra_img_options $img_name $image_size 
> 2>&1 | \
> +(
> + if [ $use_backing = 1 ]; then
> +$QEMU_IMG create -f $IMGFMT $extra_img_options -b "$backing_file" 
> "$img_name" $image_size 2>&1
> + else
> +$QEMU_IMG create -f $IMGFMT $extra_img_options "$img_name" 
> $image_size 2>&1
> + fi
> +) | \

$QEMU_IMG create -f $IMGFMT $extra_img_options \
  ${backing_file:+-b "$backing_file"} "$img_name" $image_size 2>&1 |

But as not everyone is a shell guru to recognize the ${var+:expansion}
trick, I'm fine with the version you proposed.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto:
> >> PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
> >> reverted if the panicked state is removed from runstate_needs_reset.
> > 
> > Okay so let's drop the code duplication and explicitly make
> > them the same?
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > 
> > 
> > diff --git a/vl.c b/vl.c
> > index 46c29c4..e12d317 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -638,10 +638,6 @@ static const RunStateTransition 
> > runstate_transitions_def[] = {
> >  { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
> >  { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
> >  
> > -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> > -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> > -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> > -
> >  { RUN_STATE_MAX, RUN_STATE_MAX },
> >  };
> >  
> > @@ -660,6 +656,12 @@ static void runstate_init(void)
> >  
> >  for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
> >  runstate_valid_transitions[p->from][p->to] = true;
> > +/* Panicked state is same as paused, we only made it different so
> > + * management can detect a panic.
> > + */
> > +if (p->from == RUN_STATE_PAUSED) {
> > +runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = 
> > true;
> 
> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as
> well, and perhaps there are others I'm missing.  Just add a comment
> before runstate_transitions_def's entries for PANICKED, IO_ERROR and
> WATCHDOG.
> 
> But again, it is somewhat separate from the issue at hand, which is to
> finally make pvpanic usable and hopefully before 1.7.
> 
> Paolo

The issue is that you can't continue from panicked state.
You should be able to do that without going through paused.

> > +}
> >  }
> >  }
> >  
> > @@ -686,8 +688,7 @@ int runstate_is_running(void)
> >  bool runstate_needs_reset(void)
> >  {
> >  return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> > -runstate_check(RUN_STATE_SHUTDOWN) ||
> > -runstate_check(RUN_STATE_GUEST_PANICKED);
> > +runstate_check(RUN_STATE_SHUTDOWN);
> >  }
> >  
> >  StatusInfo *qmp_query_status(Error **errp)
> > 

Re: [Qemu-devel] [PATCH 1/7] block: qemu-iotests, add quotes to $TEST_IMG usage io pattern tests

[Eric Blake]

On 10/31/2013 09:57 AM, Jeff Cody wrote:
> The usage of $TEST_IMG was not properly quoted everywhere in
> common.pattern.
> 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/common.pattern | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/7] block: qemu-iotests, add quotes to $TEST_IMG.base usage in 017

[Eric Blake]

On 10/31/2013 09:57 AM, Jeff Cody wrote:
> $TEST_IMG.base is used unquoted. Add quotes so that pathnames with
> spaces are supported.
> 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/017 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto:
> >> PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
> >> reverted if the panicked state is removed from runstate_needs_reset.
> > 
> > Okay so let's drop the code duplication and explicitly make
> > them the same?
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > 
> > 
> > diff --git a/vl.c b/vl.c
> > index 46c29c4..e12d317 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -638,10 +638,6 @@ static const RunStateTransition 
> > runstate_transitions_def[] = {
> >  { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
> >  { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
> >  
> > -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> > -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> > -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> > -
> >  { RUN_STATE_MAX, RUN_STATE_MAX },
> >  };
> >  
> > @@ -660,6 +656,12 @@ static void runstate_init(void)
> >  
> >  for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
> >  runstate_valid_transitions[p->from][p->to] = true;
> > +/* Panicked state is same as paused, we only made it different so
> > + * management can detect a panic.
> > + */
> > +if (p->from == RUN_STATE_PAUSED) {
> > +runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = 
> > true;
> 
> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as
> well,

Yea, let's do that.

> and perhaps there are others I'm missing.
>  Just add a comment
> before runstate_transitions_def's entries for PANICKED, IO_ERROR and
> WATCHDOG.

comments don't compile :)

> But again, it is somewhat separate from the issue at hand, which is to
> finally make pvpanic usable and hopefully before 1.7.
> 
> Paolo
> 
> > +}
> >  }
> >  }
> >  
> > @@ -686,8 +688,7 @@ int runstate_is_running(void)
> >  bool runstate_needs_reset(void)
> >  {
> >  return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> > -runstate_check(RUN_STATE_SHUTDOWN) ||
> > -runstate_check(RUN_STATE_GUEST_PANICKED);
> > +runstate_check(RUN_STATE_SHUTDOWN);
> >  }
> >  
> >  StatusInfo *qmp_query_status(Error **errp)
> > 

Re: [Qemu-devel] [PATCH 5/7] block: qemu-iotests, removes duplicate double quotes in 039

[Eric Blake]

On 10/31/2013 09:57 AM, Jeff Cody wrote:
> Test 039 had $TEST_IMG with duplicate double quotes - remove duplicate.
> 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/039 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 


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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PULL 23/30] coroutine: add co_aio_sleep_ns() to allow sleep in block drivers

[Kevin Wolf]

From: MORITA Kazutaka 

This helper function behaves similarly to co_sleep_ns(), but the
sleeping coroutine will be resumed when using qemu_aio_wait().

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 include/block/coroutine.h |  9 +
 qemu-coroutine-sleep.c| 14 ++
 2 files changed, 23 insertions(+)

diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 4232569..4d5c0cf 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -216,6 +216,15 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
 void coroutine_fn co_sleep_ns(QEMUClockType type, int64_t ns);
 
 /**
+ * Yield the coroutine for a given duration
+ *
+ * Behaves similarly to co_sleep_ns(), but the sleeping coroutine will be
+ * resumed when using qemu_aio_wait().
+ */
+void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
+  int64_t ns);
+
+/**
  * Yield until a file descriptor becomes readable
  *
  * Note that this function clobbers the handlers for the file descriptor.
diff --git a/qemu-coroutine-sleep.c b/qemu-coroutine-sleep.c
index f6db978..ad78fba 100644
--- a/qemu-coroutine-sleep.c
+++ b/qemu-coroutine-sleep.c
@@ -13,6 +13,7 @@
 
 #include "block/coroutine.h"
 #include "qemu/timer.h"
+#include "block/aio.h"
 
 typedef struct CoSleepCB {
 QEMUTimer *ts;
@@ -37,3 +38,16 @@ void coroutine_fn co_sleep_ns(QEMUClockType type, int64_t ns)
 timer_del(sleep_cb.ts);
 timer_free(sleep_cb.ts);
 }
+
+void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
+  int64_t ns)
+{
+CoSleepCB sleep_cb = {
+.co = qemu_coroutine_self(),
+};
+sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb);
+timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns);
+qemu_coroutine_yield();
+timer_del(sleep_cb.ts);
+timer_free(sleep_cb.ts);
+}
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 7/7] block: qemu-iotests, add quotes to $TEST_IMG usage in 061

[Eric Blake]

On 10/31/2013 09:57 AM, Jeff Cody wrote:
> When creating images with backing files in the test, the backing
> file argument was not quoted properly.  This caused the test to fail
> when run from a pathname with a space.  Pass the backing argument in
> with the -b option to _make_test_img, so it can be properly quoted.
> 
> Signed-off-by: Jeff Cody 
> ---
>  tests/qemu-iotests/061 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PULL 27/30] sheepdog: check simultaneous create in resend_aioreq

[Kevin Wolf]

From: MORITA Kazutaka 

After reconnection happens, all the inflight requests are moved to the
failed request list.  As a result, sd_co_rw_vector() can send another
create request before resend_aioreq() resends a create request from
the failed list.

This patch adds a helper function check_simultaneous_create() and
checks simultaneous create requests more strictly in resend_aioreq().

Signed-off-by: MORITA Kazutaka 
Tested-by: Liu Yuan 
Reviewed-by: Liu Yuan 
Signed-off-by: Kevin Wolf 
---
 block/sheepdog.c | 64 
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index eebb5fe..ef387de 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1288,6 +1288,29 @@ out:
 return ret;
 }
 
+/* Return true if the specified request is linked to the pending list. */
+static bool check_simultaneous_create(BDRVSheepdogState *s, AIOReq *aio_req)
+{
+AIOReq *areq;
+QLIST_FOREACH(areq, &s->inflight_aio_head, aio_siblings) {
+if (areq != aio_req && areq->oid == aio_req->oid) {
+/*
+ * Sheepdog cannot handle simultaneous create requests to the same
+ * object, so we cannot send the request until the previous request
+ * finishes.
+ */
+DPRINTF("simultaneous create to %" PRIx64 "\n", aio_req->oid);
+aio_req->flags = 0;
+aio_req->base_oid = 0;
+QLIST_REMOVE(aio_req, aio_siblings);
+QLIST_INSERT_HEAD(&s->pending_aio_head, aio_req, aio_siblings);
+return true;
+}
+}
+
+return false;
+}
+
 static void coroutine_fn resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req)
 {
 SheepdogAIOCB *acb = aio_req->aiocb;
@@ -1296,29 +1319,19 @@ static void coroutine_fn 
resend_aioreq(BDRVSheepdogState *s, AIOReq *aio_req)
 /* check whether this request becomes a CoW one */
 if (acb->aiocb_type == AIOCB_WRITE_UDATA && is_data_obj(aio_req->oid)) {
 int idx = data_oid_to_idx(aio_req->oid);
-AIOReq *areq;
 
-if (s->inode.data_vdi_id[idx] == 0) {
-create = true;
-goto out;
-}
 if (is_data_obj_writable(&s->inode, idx)) {
 goto out;
 }
 
-/* link to the pending list if there is another CoW request to
- * the same object */
-QLIST_FOREACH(areq, &s->inflight_aio_head, aio_siblings) {
-if (areq != aio_req && areq->oid == aio_req->oid) {
-DPRINTF("simultaneous CoW to %" PRIx64 "\n", aio_req->oid);
-QLIST_REMOVE(aio_req, aio_siblings);
-QLIST_INSERT_HEAD(&s->pending_aio_head, aio_req, aio_siblings);
-return;
-}
+if (check_simultaneous_create(s, aio_req)) {
+return;
 }
 
-aio_req->base_oid = vid_to_data_oid(s->inode.data_vdi_id[idx], idx);
-aio_req->flags |= SD_FLAG_CMD_COW;
+if (s->inode.data_vdi_id[idx]) {
+aio_req->base_oid = vid_to_data_oid(s->inode.data_vdi_id[idx], 
idx);
+aio_req->flags |= SD_FLAG_CMD_COW;
+}
 create = true;
 }
 out:
@@ -1945,27 +1958,14 @@ static int coroutine_fn sd_co_rw_vector(void *p)
 }
 
 aio_req = alloc_aio_req(s, acb, oid, len, offset, flags, old_oid, 
done);
+QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
 
 if (create) {
-AIOReq *areq;
-QLIST_FOREACH(areq, &s->inflight_aio_head, aio_siblings) {
-if (areq->oid == oid) {
-/*
- * Sheepdog cannot handle simultaneous create
- * requests to the same object.  So we cannot send
- * the request until the previous request
- * finishes.
- */
-aio_req->flags = 0;
-aio_req->base_oid = 0;
-QLIST_INSERT_HEAD(&s->pending_aio_head, aio_req,
-  aio_siblings);
-goto done;
-}
+if (check_simultaneous_create(s, aio_req)) {
+goto done;
 }
 }
 
-QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
 add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov, create,
 acb->aiocb_type);
 done:
-- 
1.8.1.4

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 17:26, Michael S. Tsirkin ha scritto:
> On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote:
>> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto:
 PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
 reverted if the panicked state is removed from runstate_needs_reset.
>>>
>>> Okay so let's drop the code duplication and explicitly make
>>> them the same?
>>>
>>> Signed-off-by: Michael S. Tsirkin 
>>>
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 46c29c4..e12d317 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -638,10 +638,6 @@ static const RunStateTransition 
>>> runstate_transitions_def[] = {
>>>  { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>>>  { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>>>  
>>> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
>>> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
>>> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
>>> -
>>>  { RUN_STATE_MAX, RUN_STATE_MAX },
>>>  };
>>>  
>>> @@ -660,6 +656,12 @@ static void runstate_init(void)
>>>  
>>>  for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
>>>  runstate_valid_transitions[p->from][p->to] = true;
>>> +/* Panicked state is same as paused, we only made it different so
>>> + * management can detect a panic.
>>> + */
>>> +if (p->from == RUN_STATE_PAUSED) {
>>> +runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = 
>>> true;
>>
>> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as
>> well, and perhaps there are others I'm missing.  Just add a comment
>> before runstate_transitions_def's entries for PANICKED, IO_ERROR and
>> WATCHDOG.
>>
>> But again, it is somewhat separate from the issue at hand, which is to
>> finally make pvpanic usable and hopefully before 1.7.
>>
>> Paolo
> 
> The issue is that you can't continue from panicked state.
> You should be able to do that without going through paused.

Yes, that's what my patch (posted the link before) does:

-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
+{ RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
 { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },


Comments don't compile, but are also easier to understand than code.
Special logic in runstate_init is unnecessarily complicated, for a table
that hardly sees any change.  English works better, whoever modifies the
table has it under their eyes.

Paolo

[Qemu-devel] [PATCH 1/7] block: qemu-iotests, add quotes to $TEST_IMG usage io pattern tests

[Jeff Cody]

The usage of $TEST_IMG was not properly quoted everywhere in
common.pattern.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/common.pattern | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/common.pattern 
b/tests/qemu-iotests/common.pattern
index 00e0f60..ddfbca1 100644
--- a/tests/qemu-iotests/common.pattern
+++ b/tests/qemu-iotests/common.pattern
@@ -28,7 +28,7 @@ function do_is_allocated() {
 }
 
 function is_allocated() {
-do_is_allocated "$@" | $QEMU_IO $TEST_IMG | _filter_qemu_io
+do_is_allocated "$@" | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }
 
 function do_io() {
@@ -46,18 +46,18 @@ function do_io() {
 }
 
 function io_pattern() {
-do_io "$@" | $QEMU_IO $TEST_IMG | _filter_qemu_io
+do_io "$@" | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }
 
 function io() {
 local start=$2
 local pattern=$(( (start >> 9) % 256 ))
 
-do_io "$@" $pattern | $QEMU_IO $TEST_IMG | _filter_qemu_io
+do_io "$@" $pattern | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }
 
 function io_zero() {
-do_io "$@" 0 | $QEMU_IO $TEST_IMG | _filter_qemu_io
+do_io "$@" 0 | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 }
 
 function io_test() {
@@ -117,8 +117,8 @@ function io_test2() {
 echo === Clusters to be compressed [3]
 io_pattern writev $((offset + 8 * $cluster_size)) $cluster_size $((9 * 
$cluster_size)) $num 165
 
-mv $TEST_IMG $TEST_IMG.orig
-$QEMU_IMG convert -f $IMGFMT -O $IMGFMT -c $TEST_IMG.orig $TEST_IMG
+mv "$TEST_IMG" "$TEST_IMG.orig"
+$QEMU_IMG convert -f $IMGFMT -O $IMGFMT -c "$TEST_IMG.orig" "$TEST_IMG"
 
 # Write the used clusters
 echo === Used clusters [1]
-- 
1.8.3.1

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 05:38:40PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 17:26, Michael S. Tsirkin ha scritto:
> > On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote:
> >> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto:
>  PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
>  reverted if the panicked state is removed from runstate_needs_reset.
> >>>
> >>> Okay so let's drop the code duplication and explicitly make
> >>> them the same?
> >>>
> >>> Signed-off-by: Michael S. Tsirkin 
> >>>
> >>>
> >>> diff --git a/vl.c b/vl.c
> >>> index 46c29c4..e12d317 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -638,10 +638,6 @@ static const RunStateTransition 
> >>> runstate_transitions_def[] = {
> >>>  { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
> >>>  { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
> >>>  
> >>> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> >>> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> >>> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> >>> -
> >>>  { RUN_STATE_MAX, RUN_STATE_MAX },
> >>>  };
> >>>  
> >>> @@ -660,6 +656,12 @@ static void runstate_init(void)
> >>>  
> >>>  for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; 
> >>> p++) {
> >>>  runstate_valid_transitions[p->from][p->to] = true;
> >>> +/* Panicked state is same as paused, we only made it different so
> >>> + * management can detect a panic.
> >>> + */
> >>> +if (p->from == RUN_STATE_PAUSED) {
> >>> +runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] 
> >>> = true;
> >>
> >> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as
> >> well, and perhaps there are others I'm missing.  Just add a comment
> >> before runstate_transitions_def's entries for PANICKED, IO_ERROR and
> >> WATCHDOG.
> >>
> >> But again, it is somewhat separate from the issue at hand, which is to
> >> finally make pvpanic usable and hopefully before 1.7.
> >>
> >> Paolo
> > 
> > The issue is that you can't continue from panicked state.
> > You should be able to do that without going through paused.
> 
> Yes, that's what my patch (posted the link before) does:
> 
> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> +{ RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
>  { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> -{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> 
> 
> Comments don't compile, but are also easier to understand than code.
> Special logic in runstate_init is unnecessarily complicated, for a table
> that hardly sees any change.  English works better, whoever modifies the
> table has it under their eyes.
> 
> Paolo

But code duplication is bad. I think IO error for example
is broken in that you can't pause but can run then pause.
Seems strange.
Internal error has same bug as panicked.

So it's the same bug for a bunch of states, let's just
have a way to say "this is same as paused".
How's this?

diff --git a/vl.c b/vl.c
index 46c29c4..4388c95 100644
--- a/vl.c
+++ b/vl.c
@@ -593,12 +593,6 @@ static const RunStateTransition runstate_transitions_def[] 
= {
 { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
 { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
 
-{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
-{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
-
-{ RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
-{ RUN_STATE_IO_ERROR, RUN_STATE_FINISH_MIGRATE },
-
 { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
 { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
 
@@ -635,16 +629,17 @@ static const RunStateTransition 
runstate_transitions_def[] = {
 { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
 { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
 
-{ RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
-{ RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
-
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
-{ RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
-
 { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
+static const RunState runstate_paused[] = {
+{ RUN_STATE_GUEST_PANICKED},
+{ RUN_STATE_IO_ERROR},
+{ RUN_STATE_INTERNAL_ERROR},
+{ RUN_STATE_WATCHDOG},
+{ RUN_STATE_MAX },
+};
+
 static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX];
 
 bool runstate_check(RunState state)
@@ -655,12 +650,21 @@ bool runstate_check(RunState state)
 static void runstate_init(void)
 {
 const RunStateTransition *p;
+const RunState *i;
 
 memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
 
 for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
 runstate_valid_transitions[p->from][p->to] = true;
 }
+/* Allow two-way transitions between identical states */
+for (i = &runstate_paused[0]; *p != RUN_STATE_MAX; p++) {
+runstate_valid_transitions[*i][RUN_STATE_PAUSED] = true;
+r

[Qemu-devel] [PULL 14/30] ahci: fix win7 hang on boot

[Kevin Wolf]

From: Alexander Graf 

When AHCI executes an asynchronous IDE command, it checked DRDY without
checking either DRQ or BSY.  This sometimes caused interrupt to be sent
before command is actually completed.

This resulted in a race condition: if guest then managed to access the
device before command has completed, it would hang waiting for an
interrupt.
This was observed with windows 7 guests.

To fix, check for DRQ or BSY in additiona to DRDY, if set,
the command is asynchronous so delay the interrupt until
asynchronous done callback is invoked.

Reported-by: Michael S. Tsirkin 
Reviewed-by: Michael S. Tsirkin 
Tested-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
Signed-off-by: Kevin Wolf 
---
 hw/ide/ahci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a8be62c..fbea9e8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
 /* We're ready to process the command in FIS byte 2. */
 ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
 
-if (s->dev[port].port.ifs[0].status & READY_STAT) {
+if ((s->dev[port].port.ifs[0].status & 
(READY_STAT|DRQ_STAT|BUSY_STAT)) ==
+READY_STAT) {
 ahci_write_fis_d2h(&s->dev[port], cmd_fis);
 }
 }
-- 
1.8.1.4

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 17:48, Michael S. Tsirkin ha scritto:
> But code duplication is bad.

So should we make a table of IO_ERROR-like states to avoid code
duplication?  You have to draw a line somewhere...

> I think IO error for example
> is broken in that you can't pause but can run then pause.
> Seems strange.

"cont" moves you out of IO_ERROR.  IO_ERROR is already a non-running
state (all states except RUNNING are non-running), "stop" is a no-op in
non-running states.  I don't like it that much either, but it works.

Paolo

Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Andreas Färber]

Am 31.10.2013 16:04, schrieb Anthony Liguori:
> On Thu, Oct 31, 2013 at 3:45 PM, Andreas Färber  wrote:
>> Am 31.10.2013 15:39, schrieb Anthony Liguori:
>>> On Thu, Oct 31, 2013 at 3:36 PM, Andreas Färber  wrote:
 Am 31.10.2013 15:31, schrieb Peter Maydell:
> On 31 October 2013 14:18, Andreas Färber  wrote:
>> Peter, since I had picked up the first two patches into my still pending
>> qom-next pull, as per the QEMU Summit discussion those patches should've
>> gotten an Acked-by.
>
> Hmm? I don't recall this part of the discussion. If you want the
> patches to have an Acked-by from you you need to send mail
> to the list with an Acked-by line.

 No, I added a Signed-off-by. It was clearly stated that a Reviewed-by
 needs to be explicitly sent as reply but that "looks okay" should in
 exactly such a case where sender=submaintainer should be recorded as
 Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>
>>> Nope.  If you want there to be an Acked-by, say "Acked-by:".  Don't
>>> make people infer your Acked-bys.
>>
>> Yes, that's in the minutes. And yes, that's what I got as answer there.
>> Please reply to the minutes if you think otherwise.
> 
> I explicitly said that Acked-bys are useless too.
> 
> The minutes say that you said the kernel treats "Acked-bys" as "looks
> good".  You did say that.

I *asked* about what to do with my QEMU CPU patches that only get a
"looks okay" and got only positive answers for whether that should be an
Acked-by and no objection, including none from you.
I certainly said nothing at all about the kernel.

>  At no point did a "rule" get made though.

The new rule you made was: no patch without Reviewed-by.
Peter sending that PULL and Edgar merging it both violate that rule.
No objection against a particular patch function-wise.

Point is, had Peter ping'ed me before sending out that pull, he would've
actually gotten a Reviewed-by from me, thereby satisfying your rule! He
didn't, ignoring that he himself had actually told me to queue the
patches before his vacation, for which obviously I reviewed and tested them.

Maybe there's no obligation for picking up tags, but then again you
can't go ahead and do statistics over incompletely recorded tags.

Regards,
Andreas

>> I brought up exactly this situation where I am contributor to CPU and
>> submaintainer of CPU and often not getting Reviewed-bys but if at all,
>> such as from Paolo recently, some verbal "looks OK" for a series. I was
>> told that that should be turned into an Acked-by on the patches to
>> satisfy your criteria that contributors may not just send patches as
>> pull without Reviewed-by.
> 
> I think you misunderstood.
> 
> I don't care about Acked-bys.  They are useless.
> 
> A third of patches are being committed with Reviewed-bys.  There are
> certainly many cases where patches are going in from submaintainers
> that have been reviewed which comes implicitly with Signed-off-by.
> 
> But I worry that we're not reviewing enough on list and that there are
> patches from maintainers going in through maintainer trees that aren't
> getting outside review.
> 
> There's no immediate action for this other than we should all try to
> review more patches on list to prevent the above situation.
> 
>>> And adding tags is a nice-to-have.  There is no "rule" stating that
>>> you must include everyone that appears on the mailing list.  But I
>>> expect that maintainers try to
>>
>> Again, at QEMU Summit you pushed for making Reviewed-by a must-have and
>> we discussed whether a submaintainer must add a Reviewed-by then and
>> what to do if author==submaintainer. If you dropped that thought, then
>> fine with me.
> 
> Yes, patches should get reviewed.  I hope this is obvious to all of us :-)
> 
> I also suggested that I have tooling that people can use to simplify
> adding collected Reviewed-bys on the list.
> 
> But none of this has anything to do with inferred Acked-bys.  I'll go
> a step further and say that I would be very unhappy if anyone every
> added any kind of tag to a patch with my name on it that I didn't send
> myself.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> Regards,
>> Andreas
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PULL 06/30] qemu-iotests: Test for loading VM state from qcow2

[Kevin Wolf]

From: Max Reitz 

Add a test for saving a VM state from a qcow2 image and loading it back
(with having restarted qemu in between); this should work without any
problems.

Signed-off-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/068 | 65 ++
 tests/qemu-iotests/068.out | 11 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 77 insertions(+)
 create mode 100755 tests/qemu-iotests/068
 create mode 100644 tests/qemu-iotests/068.out

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
new file mode 100755
index 000..b72e555
--- /dev/null
+++ b/tests/qemu-iotests/068
@@ -0,0 +1,65 @@
+#!/bin/bash
+#
+# Test case for loading a saved VM state from a qcow2 image
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=mre...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+IMGOPTS="compat=1.1"
+IMG_SIZE=128K
+
+echo
+echo "=== Saving and reloading a VM state to/from a qcow2 image ==="
+echo
+_make_test_img $IMG_SIZE
+# Give qemu some time to boot before saving the VM state
+bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\
+$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\
+_filter_qemu
+# Now try to continue from that VM state (this should just work)
+echo quit |\
+$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" -loadvm 0 |\
+_filter_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out
new file mode 100644
index 000..abe35a9
--- /dev/null
+++ b/tests/qemu-iotests/068.out
@@ -0,0 +1,11 @@
+QA output created by 068
+
+=== Saving and reloading a VM state to/from a qcow2 image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) 
ssasavsavesavevsavevmsavevm
 savevm 0
+(qemu) qququiquit
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qququiquit
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 13c5500..3ca9cba 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -73,3 +73,4 @@
 065 rw auto
 066 rw auto
 067 rw auto
+068 rw auto
-- 
1.8.1.4