Re: [Qemu-devel] [PATCH v3 2/4] target-tilegx: Add single floating point implementation

[Chen Gang]

On 12/21/15 23:01, Richard Henderson wrote:
> On 12/20/2015 07:30 AM, Chen Gang wrote:
>> And we have to still check TILEGX_F_CALC_CVT, for they are really two
>> different format: TILEGX_F_CALC_CVT has no HBIT, but TILEGX_F_CALC_NCVT
>> has HBIT (which we need process it specially).
> 
> The both do, in that you re-normalize to produce that HBIT.
> That's the whole point.
> 

Oh, yes.

But all together, we want to normalize the float value in fsingle_pack2,
so we can not use float64_to_float32(), it assumes the input is already
normalized (if we can let the input normalized, we will return directly).

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] ivshmem: remove redundant assignment, fix crash with msi=off

[P J P]

+-- On Mon, 21 Dec 2015, marcandre.lur...@redhat.com wrote --+
| diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
| index 7d14222..dcfc8cc 100644
| --- a/hw/misc/ivshmem.c
| +++ b/hw/misc/ivshmem.c
| @@ -355,12 +355,9 @@ static CharDriverState* 
create_eventfd_chr_device(IVShmemState *s,
|int vector)
|  {
|  /* create a event character device based on the passed eventfd */
| -PCIDevice *pdev = PCI_DEVICE(s);
|  int eventfd = event_notifier_get_fd(n);
|  CharDriverState *chr;
|  
| -s->msi_vectors[vector].pdev = pdev;
| -
|  chr = qemu_chr_open_eventfd(eventfd);
|  
|  if (chr == NULL) {

Looks okay.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [Qemu-block] Jobs 2.0 QAPI [RFC]

[John Snow]

On 12/21/2015 11:58 AM, Alberto Garcia wrote:
> On Thu 17 Dec 2015 01:50:08 AM CET, John Snow  wrote:
>> In working through a prototype to enable multiple block jobs. A few
>> problem spots in our API compatibility become apparent.
>>
>> In a nutshell, old Blockjobs rely on the "device" to identify the job,
>> which implies:
>>
>> 1) A job is always attached to a device / the root node of a device
>> 2) There can only be one job per device
>> 3) A job can only affect one device at a time
>> 4) Once created, a job will always be associated with that device.
>>
>> All four of these are wrong and something we need to change, so
>> principally the "device" addressing scheme for Jobs needs to go and we
>> need a new ID addressing scheme instead.
> 
> Out of curiosity, do you have specific examples of block jobs that are
> affected by these problems?
> 

The motivating problem is multiple block jobs. Multiple block jobs are
hard to implement in a generic way (instead of per-job) because the API
is not suited well for it.

We need to refer to jobs by ID instead of "device," but while we're at
it Jeff Cody is working on a more universal/fine-grained op blocker
permission system. (See his RFC discussion thread for more details.)

The two can be co-developed to form a new jobs API.

> For the intermediate block-stream functionality I was having problems
> because of 1), so I extended the 'device' parameter to identify
> arbitrary node names as well.
> 
> Just to make things clear: your proposal looks good to me, I'm only
> wondering whether you're simply looking for a cleaner API or you have a
> use case that you cannot fulfill because of the way block jobs work
> now...
> 

The cleaner interface is definitely the larger motivator.

> Thanks!
> 
> Berto
> 

However, better flexibility also plays a part. Say we have two devices:

[drive0]: [X] --> [Y] --> [Z]
[drive1]: [A] --> [B]

In theory, we should be able to commit Z into Y into X while we
simultaneously perform a backup from X to A. We definitely can't do that
now.

There may be some better use cases -- backups, fleecing and other
read-only operations in particular have a high likelihood of being able
to run concurrently with other operations.


We definitely *can* just extend the old API to allow for these kinds of
things, but since it represents a new paradigm of job manipulation, it's
easier to just extend the block jobs api into a new "jobs" API and allow
the system to expand to other subsystems.


Thanks,
--js

Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Sunday, 20 December 2015 14:58
> On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
>  wrote:
> > The SD spec for ACMD41 says that a zero argument is an "inquiry"
> > ACMD41, which does not start initialisation and is used only for
> > retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
> > first sends an inquiry (zero) ACMD41. If that first request returns an
> > OCR value with the power up bit (0x8000) set, it assumes the card
> > is ready and continues, leaving the card in the wrong state. (My
> > assumption is that this works on hardware, because no real card is
> > immediately powered up upon reset.)
> >
> > This change models a delay of 0.5ms from the first ACMD41 to the power
> > being up. However, it also immediately sets the power on upon seeing a
> > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should
> > also account for guests that simply delay after card reset and then
> > issue an ACMD41 that they expect will succeed.
[...]
> Can this be done in a non-rPI specific way by just kicking off the
> timer on card reset?

No :( I tried that first, but there is no value for the timeout that works 
reliably and isn't huge. UEFI doesn't reset the card itself (it relies on the 
hardware reset), and there can be a large/variable delay after power on before 
its gets to the SD init (particularly for debug builds, which do lots of 
printing to the serial port).

BTW, this is generic to UEFI; I don't believe it's Pi-specific.

Andrew

Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure

[P J P]

+-- On Mon, 21 Dec 2015, Miao Yan wrote --+
| So return 1 on device activation failure instead of -1;
| 
| Signed-off-by: Miao Yan 
| ---
|  hw/net/vmxnet3.c | 2 +-
|  1 file changed, 1 insertion(+), 1 deletion(-)
| 
| diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
| index e168285..9185408 100644
| --- a/hw/net/vmxnet3.c
| +++ b/hw/net/vmxnet3.c
| @@ -1652,7 +1652,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State 
*s)
|  
|  switch (s->last_command) {
|  case VMXNET3_CMD_ACTIVATE_DEV:
| -ret = (s->device_active) ? 0 : -1;
| +ret = (s->device_active) ? 0 : 1;
|  VMW_CFPRN("Device active: %" PRIx64, ret);
|  break;

  It seems okay. Considering that the function returns 'uint64_t', -1 would 
become an extremely large value. I wonder if that is intended.

If '1' indicates the error, the 'default:' case in the same switch needs to be 
updated too.

  default:
VMW_WRPRN("Received request for unknown command: %x", s->last_command); 
ret = -1;   
break;


Why does the function return 'uint64_t' type? All return values from other 
cases seem to be within uint32_t type.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v14 Resend 10/13] pci: add pci device pre-post reset callbacks for host bus reset

[Alex Williamson]

On Fri, 2015-12-18 at 11:29 +0800, Chen Fan wrote:
> On 12/18/2015 04:31 AM, Alex Williamson wrote:
> > On Thu, 2015-12-17 at 09:41 +0800, Cao jin wrote:
> > > From: Chen Fan 
> > > 
> > > Particularly, For vfio devices, Once need to recovery devices
> > > by bus reset such as AER, we always need to reset the host bus
> > > to recovery the devices under the bus, so we need to add pci
> > > device
> > > callbacks to specify to do host bus reset.
> > > 
> > > Signed-off-by: Chen Fan 
> > > Reviewed-by: Michael S. Tsirkin 
> > > ---
> > >   hw/pci/pci.c | 18 ++
> > >   hw/pci/pci_bridge.c  |  9 +
> > >   hw/vfio/pci.c| 26 ++
> > >   hw/vfio/pci.h|  2 ++
> > >   include/hw/pci/pci.h |  7 +++
> > >   5 files changed, 62 insertions(+)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index f6ca6ef..64fa2cc 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -247,6 +247,24 @@ static void pci_do_device_reset(PCIDevice
> > > *dev)
> > >   msix_reset(dev);
> > >   }
> > >   
> > > +void pci_device_pre_reset(PCIBus *bus, PCIDevice *dev, void
> > > *unused)
> > > +{
> > > +PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(dev);
> > > +
> > > +if (dc->pre_reset) {
> > > +dc->pre_reset(dev);
> > > +}
> > > +}
> > > +
> > > +void pci_device_post_reset(PCIBus *bus, PCIDevice *dev, void
> > > *unused)
> > > +{
> > > +PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(dev);
> > > +
> > > +if (dc->post_reset) {
> > > +dc->post_reset(dev);
> > > +}
> > > +}
> > > +
> > >   /*
> > >    * This function is called on #RST and FLR.
> > >    * FLR if PCI_EXP_DEVCTL_BCR_FLR is set
> > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > index 40c97b1..ddb76ab 100644
> > > --- a/hw/pci/pci_bridge.c
> > > +++ b/hw/pci/pci_bridge.c
> > > @@ -267,8 +267,17 @@ void pci_bridge_write_config(PCIDevice *d,
> > >   
> > >   newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > >   if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> > > +/*
> > > + * Notify all vfio-pci devices under the bus
> > > + * should do physical bus reset.
> > > + */
> > > +PCIBus *sec_bus = pci_bridge_get_sec_bus(s);
> > > +pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> > > +pci_device_pre_reset, NULL);
> > >   /* Trigger hot reset on 0->1 transition. */
> > >   qbus_reset_all(>sec_bus.qbus);
> > > +pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> > > +pci_device_post_reset, NULL);
> > >   }
> > >   }
> > >   
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index e17dc89..df32618 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -39,6 +39,7 @@
> > >   
> > >   static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
> > >   static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool
> > > enabled);
> > > +static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single);
> > >   
> > >   /*
> > >    * Disabling BAR mmaping can be slow, but toggling it around
> > > INTx
> > > can
> > > @@ -1888,6 +1889,8 @@ static int
> > > vfio_check_host_bus_reset(VFIOPCIDevice *vdev)
> > >   /* List all affected devices by bus reset */
> > >   devices = >devices[0];
> > >   
> > > +vdev->single_depend_dev = (info->count == 1);
> > > +
> > >   /* Verify that we have all the groups required */
> > >   for (i = 0; i < info->count; i++) {
> > >   PCIHostDeviceAddress host;
> > > @@ -2029,10 +2032,26 @@ static int
> > > vfio_check_bus_reset(NotifierWithReturn *n, void *opaque)
> > >   return vfio_check_host_bus_reset(vdev);
> > >   }
> > >   
> > > +static void vfio_aer_pre_reset(PCIDevice *pdev)
> > > +{
> > > +VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > +
> > > +vdev->aer_reset = true;
> > > +vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
> > > +}
> > Doesn't this lead to multiple host bus resets per guest bus reset
> > in
> > many cases?  It looks like we'll do it once per vfio-pci device,
> > even
> > if those devices are on the same host bus.  That's a 1 second
> > operation
> > per device.  Can we avoid that?  Maybe some sort of sequence ID
> > could
> > help a device figure out whether it's already been reset as part of
> > a
> > dependent device for this particular guest bus reset.  Thanks,
> That's right, I missed this case, but I don't understand the scenario
> how to
> use a sequence ID to mark the device if been reset. can you detail it
> ?

I don't really have a concrete idea for a sequence ID, it was just a
thought that maybe if each bus reset had a sequence ID then devices
could know whether they've already been reset for that sequence ID.
 The basic problem we have is that reset callbacks are per device and
it's 

[Qemu-devel] [PATCH v2 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts

[Andrew Baumann]

This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC
controller, where the card insert bit is documented as unimplemented
(always reads zero, doesn't generate interrupts) but is in fact
observed on hardware as set at power on, but is cleared (and remains
clear) on subsequent controller resets.

Signed-off-by: Andrew Baumann 
Reviewed-by: Peter Crosthwaite 
---
 hw/sd/sdhci.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dd83e89..61f919b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s)
  * initialization */
 memset(>sdmasysad, 0, (uintptr_t)>capareg - 
(uintptr_t)>sdmasysad);
 
-sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+if (!s->noeject_quirk) {
+sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+}
 s->data_count = 0;
 s->stopped_state = sdhc_not_stopped;
 }
@@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = {
 VMSTATE_UINT16(data_count, SDHCIState),
 VMSTATE_UINT64(admasysaddr, SDHCIState),
 VMSTATE_UINT8(stopped_state, SDHCIState),
+VMSTATE_BOOL(noeject_quirk, SDHCIState),
 VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz),
 VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
 VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
@@ -1276,6 +1279,7 @@ static Property sdhci_sysbus_properties[] = {
 DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
 SDHC_CAPAB_REG_DEFAULT),
 DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.5.3

[Qemu-devel] [PATCH v2 1/3] sd: sdhci: Delete over-zealous power check

[Andrew Baumann]

From: Peter Crosthwaite 

This check was conditionalising SD card operation on the card being
powered by the SDHCI host controller. It is however possible
(particularly in embedded systems) for the power control of the SD card
to be managed outside of SDHCI. This can be as trivial as hard-wiring
the SD slot VCC to a constant power-rail.

This means the guest SDHCI can validly opt-out of the SDHCI power
control feature while still using the card. So delete this check to
allow operation of the card with SDHCI power control.

This is needed for at least Xilinx Zynq and Raspberry Pi, and
also makes Freescale i.MX25 work for me. The digilent Zybo board
has a public schematic which shows SD VCC hardwiring:

http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf
bottom of page 3.

Signed-off-by: Peter Crosthwaite 
Reviewed-by: Sai Pavan Boddu 
[AB: Add Pi to list of devices fixed in commit message]
Signed-off-by: Andrew Baumann 
---
 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8612760..bc39d48 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -831,7 +831,7 @@ static void sdhci_data_transfer(void *opaque)
 
 static bool sdhci_can_issue_command(SDHCIState *s)
 {
-if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) ||
+if (!SDHC_CLOCK_IS_ON(s->clkcon) ||
 (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) &&
 ((s->cmdreg & SDHC_CMD_DATA_PRESENT) ||
 ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY &&
-- 
2.5.3

[Qemu-devel] [PATCH v2 2/3] sdhci: don't raise a command index error for an unexpected response

[Andrew Baumann]

This deletes a block of code that raised a command index error if a
command returned response data, but the guest did not set the
appropriate bits in the response register to handle such a response. I
cannot find any documentation that suggests the controller should
behave in this way, the error code doesn't make sense (command index
error is defined for the case where the index in a response does not
match that of the issued command), and in at least one case (CMD23
issued by UEFI on Raspberry Pi 2), actual hardware does not do this.

Signed-off-by: Andrew Baumann 
Reviewed-by: Peter Crosthwaite 
---
 hw/sd/sdhci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index bc39d48..dd83e89 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -243,9 +243,6 @@ static void sdhci_send_command(SDHCIState *s)
 (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
 s->norintsts |= SDHC_NIS_TRSCMP;
 }
-} else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) {
-s->errintsts |= SDHC_EIS_CMDIDX;
-s->norintsts |= SDHC_NIS_ERR;
 }
 
 if (s->norintstsen & SDHC_NISEN_CMDCMP) {
-- 
2.5.3

[Qemu-devel] [Bug 893208] Re: qemu on ARM hosts can't boot i386 image

[PeteVine]

Still present in 2.5.

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

Title:
  qemu on ARM hosts can't boot i386 image

Status in QEMU:
  New
Status in Linaro QEMU:
  New

Bug description:
  If you apply some workarounds for bug 870990, bug 883133 and bug
  883136 QEMU still cannot boot the i386
  debian_squeeze_i386_standard.qcow2 image from
  http://people.debian.org/~aurel32/qemu/i386/ -- grub starts to boot
  but something causes the system to reset just before display of the
  blue-background grub menu, so we go round in a loop forever. This
  image boots OK on i386 hosted qemu so this indicates some kind of ARM-
  host specific bug.

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

Re: [Qemu-devel] [Qemu-block] [PATCH COLO-Frame v12 25/38] qmp event: Add event notification for COLO error

[John Snow]

On 12/19/2015 05:02 AM, Markus Armbruster wrote:
> Copying qemu-block because this seems related to generalising block jobs
> to background jobs.
> 
> zhanghailiang  writes:
> 
>> If some errors happen during VM's COLO FT stage, it's important to notify 
>> the users
>> of this event. Together with 'colo_lost_heartbeat', users can intervene in 
>> COLO's
>> failover work immediately.
>> If users don't want to get involved in COLO's failover verdict,
>> it is still necessary to notify users that we exited COLO mode.
>>
>> Cc: Markus Armbruster 
>> Cc: Michael Roth 
>> Signed-off-by: zhanghailiang 
>> Signed-off-by: Li Zhijian 
>> ---
>> v11:
>> - Fix several typos found by Eric
>>
>> Signed-off-by: zhanghailiang 
>> ---
>>  docs/qmp-events.txt | 17 +
>>  migration/colo.c| 11 +++
>>  qapi-schema.json| 16 
>>  qapi/event.json | 17 +
>>  4 files changed, 61 insertions(+)
>>
>> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
>> index d2f1ce4..19f68fc 100644
>> --- a/docs/qmp-events.txt
>> +++ b/docs/qmp-events.txt
>> @@ -184,6 +184,23 @@ Example:
>>  Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR
>>  event.
>>  
>> +COLO_EXIT
>> +-
>> +
>> +Emitted when VM finishes COLO mode due to some errors happening or
>> +at the request of users.
> 
> How would the event's recipient distinguish between "due to error" and
> "at the user's request"?
> 
>> +
>> +Data:
>> +
>> + - "mode": COLO mode, primary or secondary side (json-string)
>> + - "reason":  the exit reason, internal error or external request. 
>> (json-string)
>> + - "error": error message (json-string, operation)
>> +
>> +Example:
>> +
>> +{"timestamp": {"seconds": 2032141960, "microseconds": 417172},
>> + "event": "COLO_EXIT", "data": {"mode": "primary", "reason": "request" } }
>> +
> 
> Pardon my ignorance again...  Does "VM finishes COLO mode" means have
> some kind of COLO background job, and it just finished for whatever
> reason?
> 
> If yes, this COLO job could be an instance of the general background job
> concept we're trying to grow from the existing block job concept.
> 
> I'm not asking you to rebase your work onto the background job
> infrastructure, not least for the simple reason that it doesn't exist,
> yet.  But I think it would be fruitful to compare your COLO job
> management QMP interface with the one we have for block jobs.  Not only
> may that avoid unnecessary inconsistency, it could also help shape the
> general background job interface.
> 

Yes. The "background job" concept doesn't exist in a formal way outside
of the block layer yet, but we're looking to expand it as we re-tool the
block jobs themselves.

It may be the case that the COLO commands and events need to go in as
they are now, but later we can bring them back into the generalized job
infrastructure.

> Quick overview of the block job QMP interface:
> 
> * Commands to create a job: block-commit, block-stream, drive-mirror,
>   drive-backup.
> 
> * Get information on jobs: query-block-jobs
> 
> * Pause a job: block-job-pause
> 
> * Resume a job: block-job-resume
> 
> * Cancel a job: block-job-cancel
> 
> * Block job completion events: BLOCK_JOB_COMPLETED, BLOCK_JOB_CANCELLED
> 
> * Block job error event: BLOCK_JOB_ERROR
> 
> * Block job synchronous completion: event BLOCK_JOB_READY and command
>   block-job-complete
> 

The block-agnostic version of these commands would likely be:

query-jobs
job-pause
job-resume
job-cancel
job-complete

Events: JOB_COMPLETED, JOB_CANCELLED, JOB_ERROR, JOB_READY.


It looks like COLO_EXIT would be an instance of JOB_COMPLETED, and if it
occurred due to an error, we'd also see JOB_ERROR emitted.

>>  DEVICE_DELETED
>>  --
>>  
>> diff --git a/migration/colo.c b/migration/colo.c
>> index d1dd4e1..d06c14f 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -18,6 +18,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/sockets.h"
>>  #include "migration/failover.h"
>> +#include "qapi-event.h"
>>  
>>  /* colo buffer */
>>  #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
>> @@ -349,6 +350,11 @@ static void colo_process_checkpoint(MigrationState *s)
>>  out:
>>  if (ret < 0) {
>>  error_report("%s: %s", __func__, strerror(-ret));
>> +qapi_event_send_colo_exit(COLO_MODE_PRIMARY, COLO_EXIT_REASON_ERROR,
>> +  true, strerror(-ret), NULL);
>> +} else {
>> +qapi_event_send_colo_exit(COLO_MODE_PRIMARY, 
>> COLO_EXIT_REASON_REQUEST,
>> +  false, NULL, NULL);
>>  }
>>  
>>  qsb_free(buffer);
>> @@ -516,6 +522,11 @@ out:
>>  if (ret < 0) {
>>  error_report("colo incoming thread will exit, detect error: %s",
>>   

Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Kevin O'Connor]

On Mon, Dec 21, 2015 at 09:41:32AM +, Gonglei (Arei) wrote:
> When the gurb of OS is booting, then the softirq and C function send_disk_op()
> may use extra stack of SeaBIOS. If we inject a NMI, romlayout.S: 
> irqentry_extrastack
> is invoked, and the extra stack will be used again. And the stack of first 
> calling
> will be broken, so that the SeaBIOS stuck. 
> 
> You can easily reproduce the problem.
> 
> 1. start on guest
> 2. reset the guest
> 3. inject a NMI when the guest show the grub surface
> 4. then the guest stuck

Does the SeaBIOS patch below help?  I'm not familiar with how to
"inject a NMI" - can you describe the process in more detail?

-Kevin


--- a/src/romlayout.S
+++ b/src/romlayout.S
@@ -548,7 +548,9 @@ entry_post:
 ENTRY_INTO32 _cfunc32flat_handle_post   // Normal entry point
 
 ORG 0xe2c3
-IRQ_ENTRY 02
+.global entry_02
+entry_02:
+ENTRY handle_02  // NMI handler does not switch onto extra stack
 
 ORG 0xe3fe
 .global entry_13_official

Re: [Qemu-devel] [PATCH 1/8] bcm2835_sbm: add BCM2835 mailboxes

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 3:15 PM, Andrew Baumann
 wrote:
> Hi Peter,
>
> Thanks for the review!
>
>> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
>> Sent: Monday, 21 December 2015 14:49
>> On Thu, Dec 3, 2015 at 10:01 PM, Andrew Baumann
>>  wrote:
>> > This adds the system mailboxes which are used to communicate with a
>> > number of GPU peripherals on Pi/Pi2.
>> >
>> > Signed-off-by: Andrew Baumann 
>> > ---
>> >  default-configs/arm-softmmu.mak  |   1 +
>> >  hw/misc/Makefile.objs|   1 +
>> >  hw/misc/bcm2835_sbm.c| 280 
>>
>> >  include/hw/arm/bcm2835_arm_control.h | 481
>> +++
>>
>> Do we need this file as of this patch?
>
> Yes, for ARM_MC_IHAVEDATAIRQEN and other related defines.
>
> [...]
>> > +static void bcm2835_sbm_update(BCM2835SbmState *s)
>>
>> What does Sbm stand for? If it is acronym is should be all caps in camel 
>> case.
>
> I'm not sure -- it comes from Gregory's code; maybe he can comment? Where 
> this device gets instantiated, there is a comment of
>
> /* Semaphores / Doorbells / Mailboxes */
>
> ... but only mailboxes are implemented here. I'm guessing maybe it's intended 
> as "system mailboxes". I can rename it.
>
>
>> > +{
>> > +int set;
>>
>> bool.
>>
>> > +int done, n;
>> > +uint32_t value;
>> > +
>> > +/* Avoid unwanted recursive calls */
>> > +s->mbox_irq_disabled = 1;
>> > +
>> > +/* Get pending responses and put them in the vc->arm mbox */
>> > +done = 0;
>> > +while (!done) {
>> > +done = 1;
>> > +if (s->mbox[0].status & ARM_MS_FULL) {
>> > +/* vc->arm mbox full, exit */
>>
>> break here.
>>
>> > +} else {
>>
>> so you can drop the else and get back a level of indent.
>>
>> > +for (n = 0; n < MBOX_CHAN_COUNT; n++) {
>> > +if (s->available[n]) {
>> > +value = ldl_phys(>mbox_as, n<<4);
>> > +if (value != MBOX_INVALID_DATA) {
>> > +mbox_push(>mbox[0], value);
>> > +} else {
>> > +/* Hmmm... */
>>
>> Needs more comment :)
>
> Gregory -- help! :)
>
> [...]
>> > +s->available[irq] = level;
>> > +if (!s->mbox_irq_disabled) {
>>
>> I don't think you need this. IO devices are not thread safe and
>> core-code knows it. So you don't need to worry about interruption and
>> re-entry of your IRQ handler.
>
> I will have to put a debugger on this to confirm, but I think the issue is 
> that we are using a private memory region and GPIOs to route mailbox data and 
> interrupts to the relevant sub peripherals. The ldl_phys invokes another 
> device emulation (such as bcm2835_property.c in this series), which may cause 
> it to signal an interrupt back to us via qemu_set_irq which will recursively 
> run our handler. We want that IRQ to be recorded but "squashed".
>
> [...]
>> > +
>> > +switch (offset) {
>> > +case 0x80:  /* MAIL0_READ */
>> > +case 0x84:
>> > +case 0x88:
>> > +case 0x8c:
>>
>> case 0x80..0x8c
>
> Woah! Is that standard C?
>

Yes, its probably one of the more recent language standards though.
QEMU does use to more modern features liberally.

> [...]
>> > --- /dev/null
>> > +++ b/include/hw/arm/bcm2835_arm_control.h
>> > @@ -0,0 +1,481 @@
>> > +/*
>> > + *  linux/arch/arm/mach-bcm2708/arm_control.h
> [...]
>> When you have a regular structure like this, you should collapse it
>> down using some arithmatic:
>
> Notice that this file comes from Linux. I know it's not pretty, but can we 
> please keep it as-is, for comparison purposes? I'm not sure there's much 
> value in cleaning it up locally...
>

It looks very autogenerated and seems pretty nasty on the repetition.

As implementers of the hardware, it is much rarer to need these
repetitious defs than the software users on the other side. "Do
something specific with CPU#3's Mbox#5" is going to appear in
software, but hardware implementers generally don't have a choice to
implement things specifically and it usually ends up being looped and
the exploded defs are never used. If there are only a handful of
genuinely single defs needed, can they be fished out?

Regards,
Peter

>> > --- /dev/null
>> > +++ b/include/hw/misc/bcm2835_sbm.h
>> > @@ -0,0 +1,37 @@
>> > +/*
>> > + * Raspberry Pi emulation (c) 2012 Gregory Estrade
>> > + * This code is licensed under the GNU GPLv2 and later.
>> > + */
>> > +
>> > +#ifndef BCM2835_SBM_H
>> > +#define BCM2835_SBM_H
>> > +
>> > +#include "hw/sysbus.h"
>> > +#include "exec/address-spaces.h"
>> > +#include "hw/arm/bcm2835_mbox.h"
>> > +
>> > +#define TYPE_BCM2835_SBM "bcm2835_sbm"
>> > +#define BCM2835_SBM(obj) \
>> > +OBJECT_CHECK(BCM2835SbmState, (obj), TYPE_BCM2835_SBM)
>> > +
>> > +typedef struct {
>> > +MemoryRegion *mbox_mr;
>> > +AddressSpace mbox_as;

[Qemu-devel] [PATCH v2 07/14] qapi: add json output visitor

[Eric Blake]

We have several places that want to go from qapi to JSON; right now,
they have to create an intermediate QObject to do the work.  That
also has the drawback that the JSON formatting of a QDict will
rearrange keys (according to a deterministic, but unpredictable,
hash), when humans have an easier time if dicts are produced in
the same order as the qapi type.

For these reasons, it is time to add a new JSON output visitor.
This patch just adds the basic visitor and tests that it works;
later patches will add pretty-printing, and convert clients to
use the visitor.

Design choices: Unlike the QMP output visitor, the JSON visitor
refuses to visit a required string with a NULL value (abort), as
well as a non-finite number (raises an error message).  Reusing
QString to grow the contents means that we easily share code with
both qobject-json.c and qjson.c; although it might be nice to
enhance things to take an optional output callback function so
that the output can truly be streamed instead of collected in
memory.

Signed-off-by: Eric Blake 

---
v2: rebase to qapi subset E v8; add test of error outputting
infinity; use unsigned depth
---
 include/qapi/json-output-visitor.h |  25 +++
 qapi/Makefile.objs |   2 +-
 qapi/json-output-visitor.c | 203 ++
 tests/.gitignore   |   1 +
 tests/Makefile |   4 +
 tests/test-json-output-visitor.c   | 418 +
 6 files changed, 652 insertions(+), 1 deletion(-)
 create mode 100644 include/qapi/json-output-visitor.h
 create mode 100644 qapi/json-output-visitor.c
 create mode 100644 tests/test-json-output-visitor.c

diff --git a/include/qapi/json-output-visitor.h 
b/include/qapi/json-output-visitor.h
new file mode 100644
index 000..5be5a13
--- /dev/null
+++ b/include/qapi/json-output-visitor.h
@@ -0,0 +1,25 @@
+/*
+ * JSON Output Visitor
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef JSON_OUTPUT_VISITOR_H
+#define JSON_OUTPUT_VISITOR_H
+
+#include "qapi/visitor.h"
+
+typedef struct JsonOutputVisitor JsonOutputVisitor;
+
+JsonOutputVisitor *json_output_visitor_new(void);
+void json_output_visitor_cleanup(JsonOutputVisitor *v);
+void json_output_visitor_reset(JsonOutputVisitor *v);
+
+char *json_output_get_string(JsonOutputVisitor *v);
+Visitor *json_output_get_visitor(JsonOutputVisitor *v);
+
+#endif
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 2278970..b60e11b 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -1,6 +1,6 @@
 util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
 util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
 util-obj-y += string-input-visitor.o string-output-visitor.o
-util-obj-y += opts-visitor.o
+util-obj-y += opts-visitor.o json-output-visitor.o
 util-obj-y += qmp-event.o
 util-obj-y += qapi-util.o
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
new file mode 100644
index 000..8183b85
--- /dev/null
+++ b/qapi/json-output-visitor.c
@@ -0,0 +1,203 @@
+/*
+ * Convert QAPI to JSON
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qapi/json-output-visitor.h"
+#include "qapi/visitor-impl.h"
+#include "qemu/queue.h"
+#include "qemu-common.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qobject-json.h"
+
+struct JsonOutputVisitor {
+Visitor visitor;
+QString *str;
+bool comma;
+unsigned int depth;
+};
+
+static JsonOutputVisitor *to_jov(Visitor *v)
+{
+return container_of(v, JsonOutputVisitor, visitor);
+}
+
+static void json_output_name(JsonOutputVisitor *jov, const char *name)
+{
+if (!jov->str) {
+jov->str = qstring_new();
+}
+if (jov->comma) {
+qstring_append(jov->str, ", ");
+} else {
+jov->comma = true;
+}
+if (name && jov->depth) {
+qstring_append_json_string(jov->str, name);
+qstring_append(jov->str, ": ");
+}
+}
+
+static bool json_output_start_struct(Visitor *v, const char *name, void **obj,
+ size_t unused, Error **errp)
+{
+JsonOutputVisitor *jov = to_jov(v);
+json_output_name(jov, name);
+qstring_append(jov->str, "{");
+jov->comma = false;
+jov->depth++;
+return false;
+}
+
+static void json_output_end_struct(Visitor *v)
+{
+JsonOutputVisitor *jov = to_jov(v);
+assert(jov->depth);
+jov->depth--;
+qstring_append(jov->str, "}");
+jov->comma = true;
+}
+
+static bool json_output_start_list(Visitor *v, const char *name,
+   GenericList **listp, size_t size,
+   Error **errp)
+{
+JsonOutputVisitor *jov 

[Qemu-devel] [PATCH v2 02/14] qapi: Improve use of qmp/types.h

[Eric Blake]

'qobject-json.h' is not a QObject subtype; include this file
directly in .c files that are using it, rather than abusing
qmp/types.h for that purpose.

Meanwhile, for files that include a list of individual QObject
subtypes, it's easier to just use qmp/types.h for that purpose.

Signed-off-by: Eric Blake 

---
v2: no change
---
 hw/pci/pcie_aer.c  | 1 +
 include/qapi/qmp/types.h   | 1 -
 monitor.c  | 6 +-
 qapi/qmp-dispatch.c| 1 +
 qobject/json-parser.c  | 7 +--
 qobject/qobject-json.c | 6 +-
 qobject/qobject.c  | 7 +--
 tests/check-qobject-json.c | 7 +--
 tests/test-qmp-input-strict.c  | 1 +
 tests/test-qmp-input-visitor.c | 1 +
 tests/test-qmp-output-visitor.c| 1 +
 tests/test-visitor-serialization.c | 1 +
 12 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 98d2c18..dd5588c 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -20,6 +20,7 @@

 #include "sysemu/sysemu.h"
 #include "qapi/qmp/types.h"
+#include "qapi/qmp/qobject-json.h"
 #include "monitor/monitor.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pcie.h"
diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
index 9109eda..f21ecf4 100644
--- a/include/qapi/qmp/types.h
+++ b/include/qapi/qmp/types.h
@@ -20,6 +20,5 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qobject-json.h"

 #endif /* QEMU_OBJECTS_H */
diff --git a/monitor.c b/monitor.c
index 1dfd359..ed6a548 100644
--- a/monitor.c
+++ b/monitor.c
@@ -50,12 +50,8 @@
 #include "qemu/acl.h"
 #include "sysemu/tpm.h"
 #include "qapi/qmp/qerror.h"
-#include "qapi/qmp/qint.h"
-#include "qapi/qmp/qfloat.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qbool.h"
-#include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qobject-json.h"
+#include "qapi/qmp/types.h"
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/json-parser.h"
 #include 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index f36933d..4b79bf4 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -14,6 +14,7 @@
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/json-parser.h"
+#include "qapi/qmp/qobject-json.h"
 #include "qapi-types.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 6ab98a7..441c6e9 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -14,12 +14,7 @@
 #include 

 #include "qemu-common.h"
-#include "qapi/qmp/qstring.h"
-#include "qapi/qmp/qint.h"
-#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qfloat.h"
-#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/types.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-lexer.h"
 #include "qapi/qmp/json-streamer.h"
diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
index 8fc65a4..cc96ff6 100644
--- a/qobject/qobject-json.c
+++ b/qobject/qobject-json.c
@@ -15,11 +15,7 @@
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/qobject-json.h"
-#include "qapi/qmp/qint.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qbool.h"
-#include "qapi/qmp/qfloat.h"
-#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/types.h"

 typedef struct JSONParsingState
 {
diff --git a/qobject/qobject.c b/qobject/qobject.c
index a3ef14e..0c468d8 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -8,12 +8,7 @@
  */

 #include "qemu-common.h"
-#include "qapi/qmp/qbool.h"
-#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qfloat.h"
-#include "qapi/qmp/qint.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qstring.h"
+#include "qapi/qmp/types.h"

 static void (*qdestroy[QTYPE__MAX])(QObject *) = {
 [QTYPE_NONE] = NULL,   /* No such object exists */
diff --git a/tests/check-qobject-json.c b/tests/check-qobject-json.c
index 9c4e53a..46d4edf 100644
--- a/tests/check-qobject-json.c
+++ b/tests/check-qobject-json.c
@@ -12,12 +12,7 @@
  */
 #include 

-#include "qapi/qmp/qstring.h"
-#include "qapi/qmp/qint.h"
-#include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qlist.h"
-#include "qapi/qmp/qfloat.h"
-#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/types.h"
 #include "qapi/qmp/qobject-json.h"

 #include "qemu-common.h"
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 4db35dd..b929ec7 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -19,6 +19,7 @@
 #include "test-qapi-types.h"
 #include "test-qapi-visit.h"
 #include "qapi/qmp/types.h"
+#include "qapi/qmp/qobject-json.h"
 #include "test-qmp-introspect.h"
 #include "qmp-introspect.h"
 #include "qapi-visit.h"
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 7f9191c..5e30fd8 100644
--- a/tests/test-qmp-input-visitor.c
+++ 

[Qemu-devel] [PATCH v2 13/14] qapi: Support pretty printing in JSON output visitor

[Eric Blake]

Similar to pretty printing in the QObject visitor.  The rickiest
parts are the fact that during type_any(), we have to coordinate
with QObject to also print pretty; and the fact that the testsuite
now has to honor parameterization on whether pretty printing is
enabled.

Signed-off-by: Eric Blake 

---
v2: rebase to earlier changes
---
 include/qapi/json-output-visitor.h |   2 +-
 migration/savevm.c |   2 +-
 qapi/json-output-visitor.c |  25 +-
 tests/test-json-output-visitor.c   | 160 ++---
 4 files changed, 136 insertions(+), 53 deletions(-)

diff --git a/include/qapi/json-output-visitor.h 
b/include/qapi/json-output-visitor.h
index 5be5a13..e5e16e6 100644
--- a/include/qapi/json-output-visitor.h
+++ b/include/qapi/json-output-visitor.h
@@ -15,7 +15,7 @@

 typedef struct JsonOutputVisitor JsonOutputVisitor;

-JsonOutputVisitor *json_output_visitor_new(void);
+JsonOutputVisitor *json_output_visitor_new(bool pretty);
 void json_output_visitor_cleanup(JsonOutputVisitor *v);
 void json_output_visitor_reset(JsonOutputVisitor *v);

diff --git a/migration/savevm.c b/migration/savevm.c
index a739b8b..9a05a3d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1077,7 +1077,7 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool 
iterable_only)
 return;
 }

-vmdesc_jov = json_output_visitor_new();
+vmdesc_jov = json_output_visitor_new(false);
 vmdesc = json_output_get_visitor(vmdesc_jov);
 visit_start_struct(vmdesc, NULL, NULL, 0, _abort);
 tmp_i = TARGET_PAGE_SIZE;
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
index 8183b85..0c5be38 100644
--- a/qapi/json-output-visitor.c
+++ b/qapi/json-output-visitor.c
@@ -18,6 +18,7 @@
 struct JsonOutputVisitor {
 Visitor visitor;
 QString *str;
+bool pretty;
 bool comma;
 unsigned int depth;
 };
@@ -33,10 +34,13 @@ static void json_output_name(JsonOutputVisitor *jov, const 
char *name)
 jov->str = qstring_new();
 }
 if (jov->comma) {
-qstring_append(jov->str, ", ");
+qstring_append(jov->str, jov->pretty ? "," : ", ");
 } else {
 jov->comma = true;
 }
+if (jov->pretty && jov->depth) {
+qstring_append_format(jov->str, "\n%*s", 4 * jov->depth, "");
+}
 if (name && jov->depth) {
 qstring_append_json_string(jov->str, name);
 qstring_append(jov->str, ": ");
@@ -59,6 +63,9 @@ static void json_output_end_struct(Visitor *v)
 JsonOutputVisitor *jov = to_jov(v);
 assert(jov->depth);
 jov->depth--;
+if (jov->pretty) {
+qstring_append_format(jov->str, "\n%*s", 4 * jov->depth, "");
+}
 qstring_append(jov->str, "}");
 jov->comma = true;
 }
@@ -86,6 +93,9 @@ static void json_output_end_list(Visitor *v)
 JsonOutputVisitor *jov = to_jov(v);
 assert(jov->depth);
 jov->depth--;
+if (jov->pretty) {
+qstring_append_format(jov->str, "\n%*s", 4 * jov->depth, "");
+}
 qstring_append(jov->str, "]");
 jov->comma = true;
 }
@@ -135,7 +145,15 @@ static void json_output_type_any(Visitor *v, const char 
*name, QObject **obj,
  Error **errp)
 {
 JsonOutputVisitor *jov = to_jov(v);
-QString *str = qobject_to_json(*obj);
+QString *str;
+
+if (jov->pretty) {
+char *prefix = g_strdup_printf("%*s", 4 * jov->depth, "");
+str = qobject_to_json_pretty_prefix(*obj, prefix);
+g_free(prefix);
+} else {
+str = qobject_to_json(*obj);
+}
 assert(str);
 json_output_name(jov, name);
 qstring_append(jov->str, qstring_get_str(str));
@@ -179,11 +197,12 @@ void json_output_visitor_cleanup(JsonOutputVisitor *v)
 g_free(v);
 }

-JsonOutputVisitor *json_output_visitor_new(void)
+JsonOutputVisitor *json_output_visitor_new(bool pretty)
 {
 JsonOutputVisitor *v;

 v = g_malloc0(sizeof(*v));
+v->pretty = pretty;

 v->visitor.start_struct = json_output_start_struct;
 v->visitor.end_struct = json_output_end_struct;
diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c
index f884a03..0ddb7da 100644
--- a/tests/test-json-output-visitor.c
+++ b/tests/test-json-output-visitor.c
@@ -22,9 +22,10 @@ typedef struct TestOutputVisitorData {
 } TestOutputVisitorData;

 static void visitor_output_setup(TestOutputVisitorData *data,
- const void *unused)
+ const void *arg)
 {
-data->jov = json_output_visitor_new();
+const bool *pretty = arg;
+data->jov = json_output_visitor_new(*pretty);
 g_assert(data->jov);

 data->ov = json_output_get_visitor(data->jov);
@@ -157,8 +158,9 @@ static void test_visitor_out_struct(TestOutputVisitorData 
*data,
 }

 static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
-   const void *unused)
+   

[Qemu-devel] [PATCH v2 06/14] qapi: Add qstring_append_format()

[Eric Blake]

Back in commit 764c1ca (Nov 2009), we added qstring_append_int().
However, it did not see any use until commit 190c882 (Jan 2015).
Furthermore, it has a rather limited use case - to print anything
else, callers still have to format into a temporary buffer, unless
we want to introduce an explosion of new qstring_append_* methods
for each useful type to print.

A much better approach is to add a wrapper that merges printf
behavior onto qstring_append, via the new qstring_append_format()
(and its vararg counterpart).  In fact, with that in place, we
no longer need qstring_append_int().

Other immediate uses for the new function include simplifying
two existing clients of qstring_append() on a just-formatted
buffer, and the fact that we can take advantage of printf width
manipulations for more efficient indentation.

Signed-off-by: Eric Blake 

---
v2: also simplify qstring_append_json_string(), add assertion that
format is well-formed
---
 include/qapi/qmp/qstring.h |  7 ++-
 qjson.c|  2 +-
 qobject/qobject-json.c | 25 +
 qobject/qstring.c  | 37 +
 4 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index cab1faf..1b1e130 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -14,6 +14,8 @@
 #define QSTRING_H

 #include 
+#include 
+#include "qemu/compiler.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/error.h"

@@ -29,9 +31,12 @@ QString *qstring_from_str(const char *str);
 QString *qstring_from_substr(const char *str, int start, int end);
 size_t qstring_get_length(const QString *qstring);
 const char *qstring_get_str(const QString *qstring);
-void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
+void qstring_append_format(QString *qstring, const char *fmt, ...)
+GCC_FMT_ATTR(2, 3);
+void qstring_append_vformat(QString *qstring, const char *fmt, va_list ap)
+GCC_FMT_ATTR(2, 0);
 void qstring_append_json_string(QString *qstring, const char *raw);
 void qstring_append_json_number(QString *qstring, double number, Error **errp);
 QString *qobject_to_qstring(const QObject *obj);
diff --git a/qjson.c b/qjson.c
index 25afaff..8c93c1b 100644
--- a/qjson.c
+++ b/qjson.c
@@ -70,7 +70,7 @@ void json_end_array(QJSON *json)
 void json_prop_int(QJSON *json, const char *name, int64_t val)
 {
 json_emit_element(json, name);
-qstring_append_int(json->str, val);
+qstring_append_format(json->str, "%" PRId64, val);
 }

 void json_prop_str(QJSON *json, const char *name, const char *str)
diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
index dbe0ab8..3a7b26b 100644
--- a/qobject/qobject-json.c
+++ b/qobject/qobject-json.c
@@ -79,16 +79,13 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent);
 static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
 {
 ToJsonIterState *s = opaque;
-int j;

 if (s->count) {
 qstring_append(s->str, s->pretty ? "," : ", ");
 }

 if (s->pretty) {
-qstring_append(s->str, "\n");
-for (j = 0 ; j < s->indent ; j++)
-qstring_append(s->str, "");
+qstring_append_format(s->str, "\n%*s", 4 * s->indent, "");
 }

 qstring_append_json_string(s->str, key);
@@ -101,16 +98,13 @@ static void to_json_dict_iter(const char *key, QObject 
*obj, void *opaque)
 static void to_json_list_iter(QObject *obj, void *opaque)
 {
 ToJsonIterState *s = opaque;
-int j;

 if (s->count) {
 qstring_append(s->str, s->pretty ? "," : ", ");
 }

 if (s->pretty) {
-qstring_append(s->str, "\n");
-for (j = 0 ; j < s->indent ; j++)
-qstring_append(s->str, "");
+qstring_append_format(s->str, "\n%*s", 4 * s->indent, "");
 }

 to_json(obj, s->str, s->pretty, s->indent);
@@ -125,10 +119,7 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent)
 break;
 case QTYPE_QINT: {
 QInt *val = qobject_to_qint(obj);
-char buffer[1024];
-
-snprintf(buffer, sizeof(buffer), "%" PRId64, qint_get_int(val));
-qstring_append(str, buffer);
+qstring_append_format(str, "%" PRId64, qint_get_int(val));
 break;
 }
 case QTYPE_QSTRING: {
@@ -147,10 +138,7 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent)
 qstring_append(str, "{");
 qdict_iter(val, to_json_dict_iter, );
 if (pretty) {
-int j;
-qstring_append(str, "\n");
-for (j = 0 ; j < indent ; j++)
-qstring_append(str, "");
+qstring_append_format(str, "\n%*s", 4 * indent, "");
 }
 qstring_append(str, "}");
 break;
@@ -166,10 +154,7 @@ 

[Qemu-devel] [PATCH v2 04/14] qapi: Factor out JSON number formatting

[Eric Blake]

Pull out a new qstring_append_json_number() helper, so that all
JSON output producers can use a consistent style for printing
floating point without duplicating code (since we are doing more
data massaging than a simple printf format can handle).

Address one FIXME by adding an Error parameter and warning the
caller if the requested number cannot be represented in JSON;
but add another FIXME in its place because we have no way to
report the problem higher up the stack.

Signed-off-by: Eric Blake 

---
v2: minor wording tweaks
---
 include/qapi/qmp/qstring.h |  4 +++-
 qobject/qobject-json.c | 24 +++-
 qobject/qstring.c  | 37 -
 3 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 1a938f6..cab1faf 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -1,7 +1,7 @@
 /*
  * QString Module
  *
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009, 2015 Red Hat Inc.
  *
  * Authors:
  *  Luiz Capitulino 
@@ -15,6 +15,7 @@

 #include 
 #include "qapi/qmp/qobject.h"
+#include "qapi/error.h"

 typedef struct QString {
 QObject base;
@@ -32,6 +33,7 @@ void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
 void qstring_append_json_string(QString *qstring, const char *raw);
+void qstring_append_json_number(QString *qstring, double number, Error **errp);
 QString *qobject_to_qstring(const QObject *obj);
 void qstring_destroy_obj(QObject *obj);

diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
index 1557ec6..dbe0ab8 100644
--- a/qobject/qobject-json.c
+++ b/qobject/qobject-json.c
@@ -176,27 +176,9 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent)
 }
 case QTYPE_QFLOAT: {
 QFloat *val = qobject_to_qfloat(obj);
-char buffer[1024];
-int len;
-
-/* FIXME: snprintf() is locale dependent; but JSON requires
- * numbers to be formatted as if in the C locale. */
-/* FIXME: This risks printing Inf or NaN, which are not valid
- * JSON values. */
-/* FIXME: the default precision of %f may be insufficient to
- * tell this number apart from others. */
-len = snprintf(buffer, sizeof(buffer), "%f", qfloat_get_double(val));
-while (len > 0 && buffer[len - 1] == '0') {
-len--;
-}
-
-if (len && buffer[len - 1] == '.') {
-buffer[len - 1] = 0;
-} else {
-buffer[len] = 0;
-}
-
-qstring_append(str, buffer);
+/* FIXME: no way to report invalid JSON to caller, so for now
+ * we just ignore it */
+qstring_append_json_number(str, qfloat_get_double(val), NULL);
 break;
 }
 case QTYPE_QBOOL: {
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 2882c47..b06f290 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -1,7 +1,7 @@
 /*
  * QString Module
  *
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009, 2015 Red Hat Inc.
  *
  * Authors:
  *  Luiz Capitulino 
@@ -13,6 +13,7 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qstring.h"
 #include "qemu-common.h"
+#include 

 /**
  * qstring_new(): Create a new empty QString
@@ -165,6 +166,40 @@ void qstring_append_json_string(QString *qstring, const 
char *raw)
 }

 /**
+ * qstring_append_json_number(): Append a JSON number to a QString.
+ * Set @errp if the number is not representable in JSON, but append the
+ * output anyway (callers can then choose to ignore the warning).
+ */
+void qstring_append_json_number(QString *qstring, double number, Error **errp)
+{
+char buffer[1024];
+int len;
+
+/* JSON does not allow Inf or NaN; append it but set errp */
+if (!isfinite(number)) {
+error_setg(errp, "Non-finite number %f is not valid JSON", number);
+}
+
+/* FIXME: snprintf() is locale dependent; but JSON requires
+ * numbers to be formatted as if in the C locale. */
+/* FIXME: the default precision of %f may be insufficient to
+ * tell this number apart from others. */
+len = snprintf(buffer, sizeof(buffer), "%f", number);
+assert(len > 0 && len < sizeof(buffer));
+while (len > 0 && buffer[len - 1] == '0') {
+len--;
+}
+
+if (len && buffer[len - 1] == '.') {
+buffer[len - 1] = 0;
+} else {
+buffer[len] = 0;
+}
+
+qstring_append(qstring, buffer);
+}
+
+/**
  * qobject_to_qstring(): Convert a QObject to a QString
  */
 QString *qobject_to_qstring(const QObject *obj)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Monday, 21 December 2015 13:42
> On Mon, Dec 21, 2015 at 1:31 PM, Andrew Baumann
>  wrote:
> > This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC
> > controller, where the card insert bit is documented as unimplemented
> > (always reads zero, doesn't generate interrupts) but is in fact
> > observed on hardware as set at power on, but is cleared (and remains
> > clear) on subsequent controller resets.
> > @@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = {
> >  VMSTATE_UINT16(data_count, SDHCIState),
> >  VMSTATE_UINT64(admasysaddr, SDHCIState),
> >  VMSTATE_UINT8(stopped_state, SDHCIState),
> > +VMSTATE_BOOL(noeject_quirk, SDHCIState),
> 
> Sorry, one small thing I missed, static props should not be in the
> VMSD. I think you just need to drop the VMSTATE_ addition here.
> Otherwise you would need a VMSD version bump.

Yes we can drop the VMSTATE addition here.

> Is the patch missing the corresponding header change to add the new field?

Aargh. You're right, sorry, my mistake -- I've been breaking up all the changes 
into different branches to get them submitted, and missed the header file here. 
Will resend.

Andrew

Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 2:25 PM, Andrew Baumann
 wrote:
>> From: qemu-devel-bounces+andrew.baumann=microsoft@nongnu.org
>> [mailto:qemu-devel-
>> bounces+andrew.baumann=microsoft@nongnu.org] On Behalf Of
>> Peter Crosthwaite
>> Sent: Monday, 21 December 2015 13:46
>> On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
>>  wrote:
>> > The SD spec for ACMD41 says that a zero argument is an "inquiry"
>> > ACMD41, which does not start initialisation and is used only for
>> > retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
>> > first sends an inquiry (zero) ACMD41. If that first request returns an
>> > OCR value with the power up bit (0x8000) set, it assumes the card
>> > is ready and continues, leaving the card in the wrong state. (My
>> > assumption is that this works on hardware, because no real card is
>> > immediately powered up upon reset.)
>> >
>> > This change models a delay of 0.5ms from the first ACMD41 to the power
>> > being up. However, it also immediately sets the power on upon seeing a
>> > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should
>> > also account for guests that simply delay after card reset and then
>> > issue an ACMD41 that they expect will succeed.
> [...]
>> > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
>> >  .fields = (VMStateField[]) {
>> >  VMSTATE_UINT32(mode, SDState),
>> >  VMSTATE_INT32(state, SDState),
>> > +VMSTATE_UINT32(ocr, SDState),
>> > +VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
>>
>> If you change the VMSTATE layout, you need to bump the version. As
>> this is very common code, it may have stricter version bump
>> requirements. Last I knew however, there was a way to add new fields
>> at the end of VMSD without breaking backwards compatibility. Peter or
>> Juan may know more.
>
> I'll admit that I didn't think about these issues when adding the fields, but 
> after a (quick) look at vmstate_save_state() and vmstate_load_state(), they 
> seem to be using named fields in a json format, so I don't think the order of 
> fields should matter if we are adding new ones. However, if we want to be 
> able to migrate sd instances across across this change, then we'll need to 
> arrange for the OCR to appear already powered-on if we're coming from a 
> previous version. Does VMState have a way to do that? Essentially I just need 
> to specify a default value for the ocr field if coming from an old vmstate 
> version <= 1 that differs from the value set in sd_reset().
>

You can open code post_load logic as a callback, and I think you have
access to the image version from there.

Regards,
Peter

> Andrew

[Qemu-devel] [PATCH v2 14/14] qemu-img: Use new JSON output formatter

[Eric Blake]

Now that we can pretty-print straight to JSON from a visitor,
we can eliminate the temporary conversion into QObject inside
qemu-img.

The changes to qemu-iotests 043 expected output demonstrates
the fact that output is now done in qapi declaration order,
rather than QDict hash order.

Signed-off-by: Eric Blake 

---
v2: Drop RFC, adjust expected output of 043; rebase to 'name' motion

Overlaps with Fam's qemu-img edits, although he has expressed
interest in getting this one in first.
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01756.html
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01806.html
---
 qemu-img.c | 69 +++---
 tests/qemu-iotests/043.out | 22 +++
 2 files changed, 40 insertions(+), 51 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d9a2c74..59d3944 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -22,9 +22,9 @@
  * THE SOFTWARE.
  */
 #include "qapi-visit.h"
-#include "qapi/qmp-output-visitor.h"
+#include "qapi/json-output-visitor.h"
 #include "qapi/qmp/qerror.h"
-#include "qapi/qmp/qobject-json.h"
+#include "qapi/qmp/qstring.h"
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "qemu/error-report.h"
@@ -375,19 +375,15 @@ fail:

 static void dump_json_image_check(ImageCheck *check, bool quiet)
 {
-Error *local_err = NULL;
-QString *str;
-QmpOutputVisitor *ov = qmp_output_visitor_new();
-QObject *obj;
-visit_type_ImageCheck(qmp_output_get_visitor(ov), NULL, ,
-  _err);
-obj = qmp_output_get_qobject(ov);
-str = qobject_to_json_pretty(obj);
-assert(str != NULL);
-qprintf(quiet, "%s\n", qstring_get_str(str));
-qobject_decref(obj);
-qmp_output_visitor_cleanup(ov);
-QDECREF(str);
+char *str;
+JsonOutputVisitor *ov = json_output_visitor_new(true);
+visit_type_ImageCheck(json_output_get_visitor(ov), NULL,
+  , _abort);
+str = json_output_get_string(ov);
+assert(str);
+qprintf(quiet, "%s\n", str);
+g_free(str);
+json_output_visitor_cleanup(ov);
 }

 static void dump_human_image_check(ImageCheck *check, bool quiet)
@@ -1926,35 +1922,28 @@ static void dump_snapshots(BlockDriverState *bs)

 static void dump_json_image_info_list(ImageInfoList *list)
 {
-Error *local_err = NULL;
-QString *str;
-QmpOutputVisitor *ov = qmp_output_visitor_new();
-QObject *obj;
-visit_type_ImageInfoList(qmp_output_get_visitor(ov), NULL, ,
- _err);
-obj = qmp_output_get_qobject(ov);
-str = qobject_to_json_pretty(obj);
-assert(str != NULL);
-printf("%s\n", qstring_get_str(str));
-qobject_decref(obj);
-qmp_output_visitor_cleanup(ov);
-QDECREF(str);
+char *str;
+JsonOutputVisitor *ov = json_output_visitor_new(true);
+visit_type_ImageInfoList(json_output_get_visitor(ov), NULL,
+ , _abort);
+str = json_output_get_string(ov);
+assert(str);
+printf("%s\n", str);
+json_output_visitor_cleanup(ov);
+g_free(str);
 }

 static void dump_json_image_info(ImageInfo *info)
 {
-Error *local_err = NULL;
-QString *str;
-QmpOutputVisitor *ov = qmp_output_visitor_new();
-QObject *obj;
-visit_type_ImageInfo(qmp_output_get_visitor(ov), NULL, , _err);
-obj = qmp_output_get_qobject(ov);
-str = qobject_to_json_pretty(obj);
-assert(str != NULL);
-printf("%s\n", qstring_get_str(str));
-qobject_decref(obj);
-qmp_output_visitor_cleanup(ov);
-QDECREF(str);
+char *str;
+JsonOutputVisitor *ov = json_output_visitor_new(true);
+visit_type_ImageInfo(json_output_get_visitor(ov), NULL, ,
+ _abort);
+str = json_output_get_string(ov);
+assert(str);
+printf("%s\n", str);
+json_output_visitor_cleanup(ov);
+g_free(str);
 }

 static void dump_human_image_info_list(ImageInfoList *list)
diff --git a/tests/qemu-iotests/043.out b/tests/qemu-iotests/043.out
index b37d2a3..41697a2 100644
--- a/tests/qemu-iotests/043.out
+++ b/tests/qemu-iotests/043.out
@@ -40,29 +40,29 @@ cluster_size: 65536
 == finite chain of length 3 (json) ==
 [
 {
-"virtual-size": 134217728,
 "filename": "TEST_DIR/t.IMGFMT",
-"cluster-size": 65536,
 "format": "IMGFMT",
-"full-backing-filename": "TEST_DIR/t.IMGFMT.2.base",
+"dirty-flag": false,
+"virtual-size": 134217728,
+"cluster-size": 65536,
 "backing-filename": "TEST_DIR/t.IMGFMT.2.base",
-"dirty-flag": false
+"full-backing-filename": "TEST_DIR/t.IMGFMT.2.base",
 },
 {
-"virtual-size": 134217728,
 "filename": "TEST_DIR/t.IMGFMT.2.base",
-"cluster-size": 65536,
 "format": "IMGFMT",
-"full-backing-filename": "TEST_DIR/t.IMGFMT.1.base",
+"dirty-flag": false,
+"virtual-size": 134217728,
+"cluster-size": 65536,
  

Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Andrew Baumann]

> From: qemu-devel-bounces+andrew.baumann=microsoft@nongnu.org
> [mailto:qemu-devel-
> bounces+andrew.baumann=microsoft@nongnu.org] On Behalf Of
> Peter Crosthwaite
> Sent: Monday, 21 December 2015 13:46
> On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
>  wrote:
> > The SD spec for ACMD41 says that a zero argument is an "inquiry"
> > ACMD41, which does not start initialisation and is used only for
> > retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
> > first sends an inquiry (zero) ACMD41. If that first request returns an
> > OCR value with the power up bit (0x8000) set, it assumes the card
> > is ready and continues, leaving the card in the wrong state. (My
> > assumption is that this works on hardware, because no real card is
> > immediately powered up upon reset.)
> >
> > This change models a delay of 0.5ms from the first ACMD41 to the power
> > being up. However, it also immediately sets the power on upon seeing a
> > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should
> > also account for guests that simply delay after card reset and then
> > issue an ACMD41 that they expect will succeed.
[...]
> > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
> >  .fields = (VMStateField[]) {
> >  VMSTATE_UINT32(mode, SDState),
> >  VMSTATE_INT32(state, SDState),
> > +VMSTATE_UINT32(ocr, SDState),
> > +VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
> 
> If you change the VMSTATE layout, you need to bump the version. As
> this is very common code, it may have stricter version bump
> requirements. Last I knew however, there was a way to add new fields
> at the end of VMSD without breaking backwards compatibility. Peter or
> Juan may know more.

I'll admit that I didn't think about these issues when adding the fields, but 
after a (quick) look at vmstate_save_state() and vmstate_load_state(), they 
seem to be using named fields in a json format, so I don't think the order of 
fields should matter if we are adding new ones. However, if we want to be able 
to migrate sd instances across across this change, then we'll need to arrange 
for the OCR to appear already powered-on if we're coming from a previous 
version. Does VMState have a way to do that? Essentially I just need to specify 
a default value for the ocr field if coming from an old vmstate version <= 1 
that differs from the value set in sd_reset().

Andrew

[Qemu-devel] [PATCH v2 12/14] qapi: Add qobject_to_json_pretty_prefix()

[Eric Blake]

The next patch will add pretty indentation to the JSON visitor.
But in order to support pretty output in the type_any() callback,
we need to prefix every line of the QObject visitor by the current
indentation in the JSON visitor.  Hence, a new function
qobject_to_json_pretty_indent(), and the old function becomes a
thin wrapper to the expanded behavior.

While at it, change 'pretty' to be a bool, to match its usage.

Note that the new simple_pretty() test is a bit sensitive to our
current notion of prettiness, as well as to the hash ordering in
QDict (most of the tests in check-qobject-json intentionally do
not compare the original string to the round-trip string, because
we liberally accept more input forms than the canonical form that
we output).

Signed-off-by: Eric Blake 

---
v2: no change
---
 include/qapi/qmp/qobject-json.h |  1 +
 qobject/qobject-json.c  | 40 ++---
 tests/check-qobject-json.c  | 65 +
 3 files changed, 95 insertions(+), 11 deletions(-)

diff --git a/include/qapi/qmp/qobject-json.h b/include/qapi/qmp/qobject-json.h
index ee4d31a..120266b 100644
--- a/include/qapi/qmp/qobject-json.h
+++ b/include/qapi/qmp/qobject-json.h
@@ -25,5 +25,6 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap) 
GCC_FMT_ATTR(1, 0);

 QString *qobject_to_json(const QObject *obj);
 QString *qobject_to_json_pretty(const QObject *obj);
+QString *qobject_to_json_pretty_prefix(const QObject *obj, const char *prefix);

 #endif /* QJSON_H */
diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
index 3a7b26b..6d0030e 100644
--- a/qobject/qobject-json.c
+++ b/qobject/qobject-json.c
@@ -69,12 +69,14 @@ QObject *qobject_from_jsonf(const char *string, ...)
 typedef struct ToJsonIterState
 {
 int indent;
-int pretty;
+bool pretty;
 int count;
 QString *str;
+const char *prefix;
 } ToJsonIterState;

-static void to_json(const QObject *obj, QString *str, int pretty, int indent);
+static void to_json(const QObject *obj, QString *str, bool pretty, int indent,
+const char *prefix);

 static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
 {
@@ -85,13 +87,13 @@ static void to_json_dict_iter(const char *key, QObject 
*obj, void *opaque)
 }

 if (s->pretty) {
-qstring_append_format(s->str, "\n%*s", 4 * s->indent, "");
+qstring_append_format(s->str, "\n%s%*s", s->prefix, 4 * s->indent, "");
 }

 qstring_append_json_string(s->str, key);

 qstring_append(s->str, ": ");
-to_json(obj, s->str, s->pretty, s->indent);
+to_json(obj, s->str, s->pretty, s->indent, s->prefix);
 s->count++;
 }

@@ -104,14 +106,15 @@ static void to_json_list_iter(QObject *obj, void *opaque)
 }

 if (s->pretty) {
-qstring_append_format(s->str, "\n%*s", 4 * s->indent, "");
+qstring_append_format(s->str, "\n%s%*s", s->prefix, 4 * s->indent, "");
 }

-to_json(obj, s->str, s->pretty, s->indent);
+to_json(obj, s->str, s->pretty, s->indent, s->prefix);
 s->count++;
 }

-static void to_json(const QObject *obj, QString *str, int pretty, int indent)
+static void to_json(const QObject *obj, QString *str, bool pretty, int indent,
+const char *prefix)
 {
 switch (qobject_type(obj)) {
 case QTYPE_QNULL:
@@ -135,10 +138,11 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent)
 s.str = str;
 s.indent = indent + 1;
 s.pretty = pretty;
+s.prefix = prefix;
 qstring_append(str, "{");
 qdict_iter(val, to_json_dict_iter, );
 if (pretty) {
-qstring_append_format(str, "\n%*s", 4 * indent, "");
+qstring_append_format(str, "\n%s%*s", prefix, 4 * indent, "");
 }
 qstring_append(str, "}");
 break;
@@ -151,10 +155,11 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent)
 s.str = str;
 s.indent = indent + 1;
 s.pretty = pretty;
+s.prefix = prefix;
 qstring_append(str, "[");
 qlist_iter(val, (void *)to_json_list_iter, );
 if (pretty) {
-qstring_append_format(str, "\n%*s", 4 * indent, "");
+qstring_append_format(str, "\n%s%*s", prefix, 4 * indent, "");
 }
 qstring_append(str, "]");
 break;
@@ -185,7 +190,7 @@ QString *qobject_to_json(const QObject *obj)
 {
 QString *str = qstring_new();

-to_json(obj, str, 0, 0);
+to_json(obj, str, false, 0, "");

 return str;
 }
@@ -194,7 +199,20 @@ QString *qobject_to_json_pretty(const QObject *obj)
 {
 QString *str = qstring_new();

-to_json(obj, str, 1, 0);
+to_json(obj, str, true, 0, "");
+
+return str;
+}
+
+/*
+ * Pretty-print JSON representing obj, inserting prefix after every newline.
+ * Note that prefix is NOT applied before the first character.
+ */
+QString 

[Qemu-devel] [PATCH v2 00/14] Add qapi-to-JSON output visitor

[Eric Blake]

Prerequisites:
+ my qapi cleanups subset E v8:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03863.html

I wrote this series for several reasons:
1. I've been doing a lot of churn in the qapi visitor interfaces
lately; adding a new visitor is good proof whether the changes
still make sense
2. Alexander ended up writing his own simple JSON generator as
qjson.c in his work for vmstate self-description, rather than
reusing existing code, because the QObject JSON generator does
not have an easy entry point
3. Fam commented while trying to enhance 'qemu-img map' that we
are rather wasteful in that there is no way to go directly from
a qapi type to JSON without an intermediate QObject creation

v2 notes:
Fix bugs so that 'make check' and qemu-iotests pass at all points.
Rebase on top of qapi cleanups subset E v8.

Promote out of RFC.  However, in this version, I'm still
presenting two alternative solutions: either patch 8 (keep the
qjson.h interface) or patches 10-11 (completely drop qjson.h,
by inlining its contents but adding more glue in vmstate.c).
Patch 9 is a revert to show that the two alternatives are
orthogonal; the final series won't need a revert.  I'd welcome
opinions on which alternative to stick with.

Backport diffstat (renames of 1 and 14 confuse the stats):

001/14:[down] 'qapi: Rename (one) qjson.h to qobject-json.h'
002/14:[] [--] 'qapi: Improve use of qmp/types.h'
003/14:[] [--] 'qapi: Factor out JSON string escaping'
004/14:[0002] [FC] 'qapi: Factor out JSON number formatting'
005/14:[] [--] 'qapi: Use qstring_append_chr() where appropriate'
006/14:[0016] [FC] 'qapi: Add qstring_append_format()'
007/14:[0063] [FC] 'qapi: add json output visitor'
008/14:[0008] [FC] 'qjson: Simplify by using json-output-visitor'
009/14:[down] 'Revert "qjson: Simplify by using json-output-visitor"'
010/14:[down] 'vmstate: use new JSON output visitor'
011/14:[down] 'qjson: Remove unused file'
012/14:[] [--] 'qapi: Add qobject_to_json_pretty_prefix()'
013/14:[0004] [FC] 'qapi: Support pretty printing in JSON output visitor'
014/14:[down] 'qemu-img: Use new JSON output formatter'

v1 (RFC) notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01760.html

Eric Blake (14):
  qapi: Rename (one) qjson.h to qobject-json.h
  qapi: Improve use of qmp/types.h
  qapi: Factor out JSON string escaping
  qapi: Factor out JSON number formatting
  qapi: Use qstring_append_chr() where appropriate
  qapi: Add qstring_append_format()
  qapi: add json output visitor
  qjson: Simplify by using json-output-visitor
  Revert "qjson: Simplify by using json-output-visitor"
  vmstate: use new JSON output visitor
  qjson: Remove unused file
  qapi: Add qobject_to_json_pretty_prefix()
  qapi: Support pretty printing in JSON output visitor
  qemu-img: Use new JSON output formatter

 MAINTAINERS   |   2 +-
 Makefile.objs |   1 -
 balloon.c |   2 +-
 block.c   |   2 +-
 block/archipelago.c   |   2 +-
 block/nbd.c   |   2 +-
 block/quorum.c|   2 +-
 blockjob.c|   2 +-
 hw/core/qdev.c|   2 +-
 hw/misc/pvpanic.c |   2 +-
 hw/net/virtio-net.c   |   2 +-
 hw/pci/pcie_aer.c |   1 +
 include/migration/vmstate.h   |   4 +-
 include/qapi/json-output-visitor.h|  25 ++
 include/qapi/qmp/{qjson.h => qobject-json.h}  |   1 +
 include/qapi/qmp/qstring.h|  12 +-
 include/qapi/qmp/types.h  |   1 -
 include/qjson.h   |  29 --
 migration/savevm.c|  66 ++--
 migration/vmstate.c   |  63 ++--
 monitor.c |   8 +-
 qapi/Makefile.objs|   2 +-
 qapi/json-output-visitor.c| 222 
 qapi/qmp-dispatch.c   |   1 +
 qapi/qmp-event.c  |   2 +-
 qemu-img.c|  69 ++--
 qga/main.c|   2 +-
 qjson.c   | 129 ---
 qobject/Makefile.objs |   3 +-
 qobject/json-parser.c |  14 +-
 qobject/{qjson.c => qobject-json.c}   | 147 ++--
 qobject/qobject.c |   7 +-
 qobject/qstring.c | 117 ++-
 target-s390x/kvm.c|   2 +-
 tests/.gitignore  |   3 +-
 tests/Makefile|  14 +-
 tests/{check-qjson.c => check-qobject-json.c} |  86 -
 tests/libqtest.c  |   2 +-
 

[Qemu-devel] [PATCH v2 10/14] vmstate: use new JSON output visitor

[Eric Blake]

Rather than using a QJSON object and converting the QString result
to a char *, we can use the new JSON output visitor and get directly
to a char *.

The conversions are a bit tricky in place (in places, we have to
copy an integer to an int64_t temporary to get the right pointer for
visit_type_int(); and for several strings, we have to copy to a
temporary variable so we can take an address ([] is not the
same as *) and cast away const), but overall still fairly
mechanical.

Suggested-by: Paolo Bonzini 
Signed-off-by: Eric Blake 

---
v2: new patch

This is part of alternative 2, along with patch 11.  See also patch 8
for alternative 1.
---
 include/migration/vmstate.h |  4 +--
 migration/savevm.c  | 66 -
 migration/vmstate.c | 63 ++-
 tests/Makefile  |  2 +-
 4 files changed, 84 insertions(+), 51 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7267e38..101742e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -29,7 +29,7 @@
 #ifndef CONFIG_USER_ONLY
 #include 
 #endif
-#include 
+#include "qapi/json-output-visitor.h"

 typedef void SaveStateHandler(QEMUFile *f, void *opaque);
 typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
@@ -814,7 +814,7 @@ void loadvm_free_handlers(MigrationIncomingState *mis);
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id);
 void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
-void *opaque, QJSON *vmdesc);
+void *opaque, Visitor *vmdesc);

 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);

diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..a739b8b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -646,27 +646,32 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, 
int version_id)
 return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
 }

-static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON 
*vmdesc)
+static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
+   Visitor *vmdesc)
 {
 int64_t old_offset, size;
+const char *tmp;

 old_offset = qemu_ftell_fast(f);
 se->ops->save_state(f, se->opaque);
 size = qemu_ftell_fast(f) - old_offset;

 if (vmdesc) {
-json_prop_int(vmdesc, "size", size);
-json_start_array(vmdesc, "fields");
-json_start_object(vmdesc, NULL);
-json_prop_str(vmdesc, "name", "data");
-json_prop_int(vmdesc, "size", size);
-json_prop_str(vmdesc, "type", "buffer");
-json_end_object(vmdesc);
-json_end_array(vmdesc);
+visit_type_int(vmdesc, "size", , _abort);
+visit_start_list(vmdesc, "fields", NULL, 0, _abort);
+visit_start_struct(vmdesc, NULL, NULL, 0, _abort);
+tmp = "data";
+visit_type_str(vmdesc, "name", (char **), _abort);
+visit_type_int(vmdesc, "size", , _abort);
+tmp = "buffer";
+visit_type_str(vmdesc, "type", (char **), _abort);
+visit_check_struct(vmdesc, _abort);
+visit_end_struct(vmdesc);
+visit_end_list(vmdesc);
 }
 }

-static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
+static void vmstate_save(QEMUFile *f, SaveStateEntry *se, Visitor *vmdesc)
 {
 trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
 if (!se->vmsd) {
@@ -886,7 +891,7 @@ void qemu_savevm_state_header(QEMUFile *f)

 if (!savevm_state.skip_configuration) {
 qemu_put_byte(f, QEMU_VM_CONFIGURATION);
-vmstate_save_state(f, _configuration, _state, 0);
+vmstate_save_state(f, _configuration, _state, NULL);
 }

 }
@@ -1028,11 +1033,15 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)

 void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
 {
-QJSON *vmdesc;
+JsonOutputVisitor *vmdesc_jov;
+Visitor *vmdesc;
+char *vmdesc_str;
 int vmdesc_len;
 SaveStateEntry *se;
 int ret;
 bool in_postcopy = migration_in_postcopy(migrate_get_current());
+int64_t tmp_i;
+char *tmp_s;

 trace_savevm_state_complete_precopy();

@@ -1068,9 +1077,12 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, 
bool iterable_only)
 return;
 }

-vmdesc = qjson_new();
-json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE);
-json_start_array(vmdesc, "devices");
+vmdesc_jov = json_output_visitor_new();
+vmdesc = json_output_get_visitor(vmdesc_jov);
+visit_start_struct(vmdesc, NULL, NULL, 0, _abort);
+tmp_i = TARGET_PAGE_SIZE;
+visit_type_int(vmdesc, "page_size", _i, _abort);
+visit_start_list(vmdesc, "devices", NULL, 0, _abort);
 QTAILQ_FOREACH(se, 

[Qemu-devel] [PATCH v2 05/14] qapi: Use qstring_append_chr() where appropriate

[Eric Blake]

No need to create a temporary buffer, when we already have a
function available for our needs.

Signed-off-by: Eric Blake 
---
 qobject/json-parser.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 441c6e9..fc5510e 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -204,12 +204,7 @@ static QString *qstring_from_escaped_str(JSONParserContext 
*ctxt,
 goto out;
 }
 } else {
-char dummy[2];
-
-dummy[0] = *ptr++;
-dummy[1] = 0;
-
-qstring_append(str, dummy);
+qstring_append_chr(str, *ptr++);
 }
 }

-- 
2.4.3

[Qemu-devel] [PATCH v2 03/14] qapi: Factor out JSON string escaping

[Eric Blake]

Pull out a new qstring_append_json_string() helper, so that all
JSON output producers can use the same output escaping rules.

While it appears that vmstate's use of the simpler qjson.c
formatter is not currently encountering any string that needs
escapes to be valid JSON, it is better to be safe than sorry.

Signed-off-by: Eric Blake 

---
v2: no change
---
 include/qapi/qmp/qstring.h |  1 +
 qjson.c|  9 +++
 qobject/qobject-json.c | 58 ++---
 qobject/qstring.c  | 59 ++
 4 files changed, 65 insertions(+), 62 deletions(-)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index df7df55..1a938f6 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -31,6 +31,7 @@ const char *qstring_get_str(const QString *qstring);
 void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
+void qstring_append_json_string(QString *qstring, const char *raw);
 QString *qobject_to_qstring(const QObject *obj);
 void qstring_destroy_obj(QObject *obj);

diff --git a/qjson.c b/qjson.c
index e478802..25afaff 100644
--- a/qjson.c
+++ b/qjson.c
@@ -36,9 +36,8 @@ static void json_emit_element(QJSON *json, const char *name)
 }

 if (name) {
-qstring_append(json->str, "\"");
-qstring_append(json->str, name);
-qstring_append(json->str, "\" : ");
+qstring_append_json_string(json->str, name);
+qstring_append(json->str, " : ");
 }
 }

@@ -77,9 +76,7 @@ void json_prop_int(QJSON *json, const char *name, int64_t val)
 void json_prop_str(QJSON *json, const char *name, const char *str)
 {
 json_emit_element(json, name);
-qstring_append_chr(json->str, '"');
-qstring_append(json->str, str);
-qstring_append_chr(json->str, '"');
+qstring_append_json_string(json->str, str);
 }

 const char *qjson_get_str(QJSON *json)
diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
index cc96ff6..1557ec6 100644
--- a/qobject/qobject-json.c
+++ b/qobject/qobject-json.c
@@ -79,7 +79,6 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent);
 static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
 {
 ToJsonIterState *s = opaque;
-QString *qkey;
 int j;

 if (s->count) {
@@ -92,9 +91,7 @@ static void to_json_dict_iter(const char *key, QObject *obj, 
void *opaque)
 qstring_append(s->str, "");
 }

-qkey = qstring_from_str(key);
-to_json(QOBJECT(qkey), s->str, s->pretty, s->indent);
-QDECREF(qkey);
+qstring_append_json_string(s->str, key);

 qstring_append(s->str, ": ");
 to_json(obj, s->str, s->pretty, s->indent);
@@ -136,58 +133,7 @@ static void to_json(const QObject *obj, QString *str, int 
pretty, int indent)
 }
 case QTYPE_QSTRING: {
 QString *val = qobject_to_qstring(obj);
-const char *ptr;
-int cp;
-char buf[16];
-char *end;
-
-ptr = qstring_get_str(val);
-qstring_append(str, "\"");
-
-for (; *ptr; ptr = end) {
-cp = mod_utf8_codepoint(ptr, 6, );
-switch (cp) {
-case '\"':
-qstring_append(str, "\\\"");
-break;
-case '\\':
-qstring_append(str, "");
-break;
-case '\b':
-qstring_append(str, "\\b");
-break;
-case '\f':
-qstring_append(str, "\\f");
-break;
-case '\n':
-qstring_append(str, "\\n");
-break;
-case '\r':
-qstring_append(str, "\\r");
-break;
-case '\t':
-qstring_append(str, "\\t");
-break;
-default:
-if (cp < 0) {
-cp = 0xFFFD; /* replacement character */
-}
-if (cp > 0x) {
-/* beyond BMP; need a surrogate pair */
-snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
- 0xD800 + ((cp - 0x1) >> 10),
- 0xDC00 + ((cp - 0x1) & 0x3FF));
-} else if (cp < 0x20 || cp >= 0x7F) {
-snprintf(buf, sizeof(buf), "\\u%04X", cp);
-} else {
-buf[0] = cp;
-buf[1] = 0;
-}
-qstring_append(str, buf);
-}
-};
-
-qstring_append(str, "\"");
+qstring_append_json_string(str, qstring_get_str(val));
 break;
 }
 case QTYPE_QDICT: {
diff --git a/qobject/qstring.c b/qobject/qstring.c
index f44c5c4..2882c47 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -106,6 

Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
 wrote:
> The SD spec for ACMD41 says that a zero argument is an "inquiry"
> ACMD41, which does not start initialisation and is used only for
> retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
> first sends an inquiry (zero) ACMD41. If that first request returns an
> OCR value with the power up bit (0x8000) set, it assumes the card
> is ready and continues, leaving the card in the wrong state. (My
> assumption is that this works on hardware, because no real card is
> immediately powered up upon reset.)
>
> This change models a delay of 0.5ms from the first ACMD41 to the power
> being up. However, it also immediately sets the power on upon seeing a
> non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should
> also account for guests that simply delay after card reset and then
> issue an ACMD41 that they expect will succeed.
>
> [1] 
> https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c#L279
> (This is the loop starting with "We need to wait for the MMC or SD
> card is ready")
>
> Signed-off-by: Andrew Baumann 
> ---
> Obviously this is a bug that should be fixed in EDK2. However, this
> initialisation appears to have been around for quite a while in EDK2
> (in various forms), and the fact that it has obviously worked with so
> many real SD/MMC cards makes me think that it would be pragmatic to
> have the workaround in QEMU as well.
>
> You might argue that the delay timer should start on sd_reset(), and
> not the first ACMD41. However, that doesn't work reliably with UEFI,
> because a large delay often elapses between the two (particularly in
> debug builds that do lots of printing to the serial port). If the
> timer fires too early, we'll still hit the bug, but we also don't want
> to set a huge timeout value, because some guests may depend on it
> expiring.
>
>  hw/sd/sd.c | 45 -
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1a24933..1011785 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -33,6 +33,7 @@
>  #include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/timer.h"
>
>  //#define DEBUG_SD 1
>
> @@ -43,7 +44,9 @@ do { fprintf(stderr, "SD: " fmt , ## __VA_ARGS__); } while 
> (0)
>  #define DPRINTF(fmt, ...) do {} while(0)
>  #endif
>
> -#define ACMD41_ENQUIRY_MASK 0x00ff
> +#define ACMD41_ENQUIRY_MASK 0x00ff
> +#define OCR_POWER_UP0x8000
> +#define OCR_POWER_DELAY (get_ticks_per_sec() / 2000) /* 0.5ms */
>
>  typedef enum {
>  sd_r0 = 0,/* no response */
> @@ -80,6 +83,7 @@ struct SDState {
>  uint32_t mode;/* current card mode, one of SDCardModes */
>  int32_t state;/* current card state, one of SDCardStates */
>  uint32_t ocr;
> +QEMUTimer *ocr_power_timer;
>  uint8_t scr[8];
>  uint8_t cid[16];
>  uint8_t csd[16];
> @@ -194,8 +198,17 @@ static uint16_t sd_crc16(void *message, size_t width)
>
>  static void sd_set_ocr(SDState *sd)
>  {
> -/* All voltages OK, card power-up OK, Standard Capacity SD Memory Card */
> -sd->ocr = 0x8000;
> +/* All voltages OK, Standard Capacity SD Memory Card, not yet powered up 
> */
> +sd->ocr = 0x0000;
> +}
> +
> +static void sd_ocr_powerup(void *opaque)
> +{
> +SDState *sd = opaque;
> +
> +/* Set powered up bit in OCR */
> +assert(!(sd->ocr & OCR_POWER_UP));
> +sd->ocr |= OCR_POWER_UP;
>  }
>
>  static void sd_set_scr(SDState *sd)
> @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
>  .fields = (VMStateField[]) {
>  VMSTATE_UINT32(mode, SDState),
>  VMSTATE_INT32(state, SDState),
> +VMSTATE_UINT32(ocr, SDState),
> +VMSTATE_TIMER_PTR(ocr_power_timer, SDState),

If you change the VMSTATE layout, you need to bump the version. As
this is very common code, it may have stricter version bump
requirements. Last I knew however, there was a way to add new fields
at the end of VMSD without breaking backwards compatibility. Peter or
Juan may know more.

Regards,
Peter

>  VMSTATE_UINT8_ARRAY(cid, SDState, 16),
>  VMSTATE_UINT8_ARRAY(csd, SDState, 16),
>  VMSTATE_UINT16(rca, SDState),
> @@ -493,6 +508,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>  sd->spi = is_spi;
>  sd->enable = true;
>  sd->blk = blk;
> +sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, 
> sd);
>  sd_reset(sd);
>  if (sd->blk) {
>  /* Attach dev if not already attached.  (This call ignores an
> @@ -1295,12 +1311,31 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>  }
>  switch (sd->state) {
>  case sd_idle_state:
> +/* If it's the first ACMD41 since reset, we need to decide
> +

[Qemu-devel] [PATCH v2 0/3] sdhci patches to enable Raspberry Pi

[Andrew Baumann]

This is a series of three tweaks needed to enable the generic sdhci
controller to emulate Raspberry Pi (bcm2835/2836), and boot Linux and
Windows.

There was some discussion of these changes in the following thread:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01401.html

v2 is functionally equivalent to v1; the only changes are to commit
messages and review metadata.

Cheers,
Andrew

Andrew Baumann (2):
  sdhci: don't raise a command index error for an unexpected response
  sdhci: add optional quirk property to disable card insertion/removal
interrupts

Peter Crosthwaite (1):
  sd: sdhci: Delete over-zealous power check

 hw/sd/sdhci.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.5.3

[Qemu-devel] [PATCH v2 01/14] qapi: Rename (one) qjson.h to qobject-json.h

[Eric Blake]

We have two different JSON visitors in the tree; and having both
named 'qjson.h' can cause include confusion.  Rename the qapi
version.

Why did I pick that one?  A later patch plans on deleting the
top-level qjson.c once we have a native JSON output visitor; we
could have renamed that one for less overall churn.  On the other
hand, all of the QObject subtypes have their own qFOO.c file, but
qjson.c makes it sound like we have a QTYPE_JSON subclass of
QObject; the new name of qobject-json makes it obvious that the
file is used for conversions between QObject and JSON, and not a
QObject subtype.

Kill trailing whitespace in the renamed tests/check-qobject-json.c
to keep checkpatch.pl happy.

Signed-off-by: Eric Blake 
Reviewed-by: Paolo Bonzini 

---
v2: retitle, enhance commit message, rebase to master
---
 MAINTAINERS   |  2 +-
 balloon.c |  2 +-
 block.c   |  2 +-
 block/archipelago.c   |  2 +-
 block/nbd.c   |  2 +-
 block/quorum.c|  2 +-
 blockjob.c|  2 +-
 hw/core/qdev.c|  2 +-
 hw/misc/pvpanic.c |  2 +-
 hw/net/virtio-net.c   |  2 +-
 include/qapi/qmp/{qjson.h => qobject-json.h}  |  0
 include/qapi/qmp/types.h  |  2 +-
 monitor.c |  2 +-
 qapi/qmp-event.c  |  2 +-
 qemu-img.c|  2 +-
 qga/main.c|  2 +-
 qobject/Makefile.objs |  3 ++-
 qobject/{qjson.c => qobject-json.c}   |  2 +-
 target-s390x/kvm.c|  2 +-
 tests/.gitignore  |  2 +-
 tests/Makefile|  8 
 tests/{check-qjson.c => check-qobject-json.c} | 14 +++---
 tests/libqtest.c  |  2 +-
 ui/spice-core.c   |  2 +-
 vl.c  |  2 +-
 25 files changed, 34 insertions(+), 33 deletions(-)
 rename include/qapi/qmp/{qjson.h => qobject-json.h} (100%)
 rename qobject/{qjson.c => qobject-json.c} (99%)
 rename tests/{check-qjson.c => check-qobject-json.c} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 55a0fd8..81fd039 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1155,7 +1155,7 @@ X: include/qapi/qmp/dispatch.h
 F: tests/check-qdict.c
 F: tests/check-qfloat.c
 F: tests/check-qint.c
-F: tests/check-qjson.c
+F: tests/check-qobject-json.c
 F: tests/check-qlist.c
 F: tests/check-qstring.c
 T: git git://repo.or.cz/qemu/qmp-unstable.git queue/qmp
diff --git a/balloon.c b/balloon.c
index 0f45d1b..5983b4f 100644
--- a/balloon.c
+++ b/balloon.c
@@ -31,7 +31,7 @@
 #include "trace.h"
 #include "qmp-commands.h"
 #include "qapi/qmp/qerror.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qobject-json.h"

 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
diff --git a/block.c b/block.c
index 411edbf..0b7eb00 100644
--- a/block.c
+++ b/block.c
@@ -30,7 +30,7 @@
 #include "qemu/module.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qobject-json.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
 #include "qemu/notify.h"
diff --git a/block/archipelago.c b/block/archipelago.c
index 855655c..80a1bb5 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -56,7 +56,7 @@
 #include "qemu/thread.h"
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qstring.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qobject-json.h"
 #include "qemu/atomic.h"

 #include 
diff --git a/block/nbd.c b/block/nbd.c
index 416f42b..ef53083 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -32,7 +32,7 @@
 #include "qemu/module.h"
 #include "qemu/sockets.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qobject-json.h"
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qstring.h"

diff --git a/block/quorum.c b/block/quorum.c
index 6793f12..a64b40d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -18,7 +18,7 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qint.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qobject-json.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi-event.h"
diff --git a/blockjob.c b/blockjob.c
index 80adb9d..84361f7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -31,7 +31,7 @@
 #include "block/block_int.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qmp/qerror.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qobject-json.h"
 #include "qemu/coroutine.h"
 #include "qmp-commands.h"
 #include "qemu/timer.h"
diff --git a/hw/core/qdev.c 

[Qemu-devel] [PATCH v2 08/14] qjson: Simplify by using json-output-visitor

[Eric Blake]

Instead of rolling our own limited JSON outputter, we can just
wrap the more full-featured JSON output Visitor.

This slightly changes the output (different spacing), but the
result is still equivalent JSON contents.

Signed-off-by: Eric Blake 

---
v2: rebase to earlier changes

This is alternative 1.  See also patches 10-11 for alternative 2.
---
 qjson.c | 61 ++---
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/qjson.c b/qjson.c
index 8c93c1b..8874bad 100644
--- a/qjson.c
+++ b/qjson.c
@@ -11,103 +11,86 @@
  *
  */

-#include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
+#include "qapi/json-output-visitor.h"

 struct QJSON {
 Object obj;
-QString *str;
-bool omit_comma;
+JsonOutputVisitor *jov;
+char *str;
 };

 #define QJSON(obj) OBJECT_CHECK(QJSON, (obj), TYPE_QJSON)

-static void json_emit_element(QJSON *json, const char *name)
-{
-/* Check whether we need to print a , before an element */
-if (json->omit_comma) {
-json->omit_comma = false;
-} else {
-qstring_append(json->str, ", ");
-}
-
-if (name) {
-qstring_append_json_string(json->str, name);
-qstring_append(json->str, " : ");
-}
-}
-
 void json_start_object(QJSON *json, const char *name)
 {
-json_emit_element(json, name);
-qstring_append(json->str, "{ ");
-json->omit_comma = true;
+Visitor *v = json_output_get_visitor(json->jov);
+visit_start_struct(v, name, NULL, 0, _abort);
 }

 void json_end_object(QJSON *json)
 {
-qstring_append(json->str, " }");
-json->omit_comma = false;
+Visitor *v = json_output_get_visitor(json->jov);
+visit_check_struct(v, _abort);
+visit_end_struct(v);
 }

 void json_start_array(QJSON *json, const char *name)
 {
-json_emit_element(json, name);
-qstring_append(json->str, "[ ");
-json->omit_comma = true;
+Visitor *v = json_output_get_visitor(json->jov);
+visit_start_list(v, name, NULL, 0, _abort);
 }

 void json_end_array(QJSON *json)
 {
-qstring_append(json->str, " ]");
-json->omit_comma = false;
+Visitor *v = json_output_get_visitor(json->jov);
+visit_end_list(v);
 }

 void json_prop_int(QJSON *json, const char *name, int64_t val)
 {
-json_emit_element(json, name);
-qstring_append_format(json->str, "%" PRId64, val);
+Visitor *v = json_output_get_visitor(json->jov);
+visit_type_int(v, name, , _abort);
 }

 void json_prop_str(QJSON *json, const char *name, const char *str)
 {
-json_emit_element(json, name);
-qstring_append_json_string(json->str, str);
+Visitor *v = json_output_get_visitor(json->jov);
+visit_type_str(v, name, (char **), _abort);
 }

 const char *qjson_get_str(QJSON *json)
 {
-return qstring_get_str(json->str);
+return json->str;
 }

 QJSON *qjson_new(void)
 {
 QJSON *json = QJSON(object_new(TYPE_QJSON));
+json_start_object(json, NULL);
 return json;
 }

 void qjson_finish(QJSON *json)
 {
 json_end_object(json);
+json->str = json_output_get_string(json->jov);
 }

 static void qjson_initfn(Object *obj)
 {
 QJSON *json = QJSON(obj);
-
-json->str = qstring_from_str("{ ");
-json->omit_comma = true;
+json->jov = json_output_visitor_new();
 }

 static void qjson_finalizefn(Object *obj)
 {
 QJSON *json = QJSON(obj);
-
-qobject_decref(QOBJECT(json->str));
+g_free(json->str);
 }

 static const TypeInfo qjson_type_info = {
-- 
2.4.3

[Qemu-devel] [PATCH v2 11/14] qjson: Remove unused file

[Eric Blake]

Now that we have a JSON output visitor, and the previous patch
fixed the only client of vmstate to use it, we no longer need the
simpler QJSON object doing the same thing.

Signed-off-by: Eric Blake 

---
v2: new patch

This is part of alternative 2, along with patch 10.  See also patch 8
for alternative 1.
---
 Makefile.objs   |   1 -
 include/qjson.h |  29 -
 qjson.c | 126 
 3 files changed, 156 deletions(-)
 delete mode 100644 include/qjson.h
 delete mode 100644 qjson.c

diff --git a/Makefile.objs b/Makefile.objs
index dac2c02..dbae7f8 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -51,7 +51,6 @@ common-obj-$(CONFIG_LINUX) += fsdev/
 common-obj-y += migration/
 common-obj-y += qemu-char.o #aio.o
 common-obj-y += page_cache.o
-common-obj-y += qjson.o

 common-obj-$(CONFIG_SPICE) += spice-qemu-char.o

diff --git a/include/qjson.h b/include/qjson.h
deleted file mode 100644
index 7c54fdf..000
--- a/include/qjson.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/*
- * QEMU JSON writer
- *
- * Copyright Alexander Graf
- *
- * Authors:
- *  Alexander Graf 
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-#ifndef QEMU_QJSON_H
-#define QEMU_QJSON_H
-
-#define TYPE_QJSON "QJSON"
-typedef struct QJSON QJSON;
-
-QJSON *qjson_new(void);
-void json_prop_str(QJSON *json, const char *name, const char *str);
-void json_prop_int(QJSON *json, const char *name, int64_t val);
-void json_end_array(QJSON *json);
-void json_start_array(QJSON *json, const char *name);
-void json_end_object(QJSON *json);
-void json_start_object(QJSON *json, const char *name);
-const char *qjson_get_str(QJSON *json);
-void qjson_finish(QJSON *json);
-
-#endif /* QEMU_QJSON_H */
diff --git a/qjson.c b/qjson.c
deleted file mode 100644
index 8c93c1b..000
--- a/qjson.c
+++ /dev/null
@@ -1,126 +0,0 @@
-/*
- * QEMU JSON writer
- *
- * Copyright Alexander Graf
- *
- * Authors:
- *  Alexander Graf 
- *
- * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-struct QJSON {
-Object obj;
-QString *str;
-bool omit_comma;
-};
-
-#define QJSON(obj) OBJECT_CHECK(QJSON, (obj), TYPE_QJSON)
-
-static void json_emit_element(QJSON *json, const char *name)
-{
-/* Check whether we need to print a , before an element */
-if (json->omit_comma) {
-json->omit_comma = false;
-} else {
-qstring_append(json->str, ", ");
-}
-
-if (name) {
-qstring_append_json_string(json->str, name);
-qstring_append(json->str, " : ");
-}
-}
-
-void json_start_object(QJSON *json, const char *name)
-{
-json_emit_element(json, name);
-qstring_append(json->str, "{ ");
-json->omit_comma = true;
-}
-
-void json_end_object(QJSON *json)
-{
-qstring_append(json->str, " }");
-json->omit_comma = false;
-}
-
-void json_start_array(QJSON *json, const char *name)
-{
-json_emit_element(json, name);
-qstring_append(json->str, "[ ");
-json->omit_comma = true;
-}
-
-void json_end_array(QJSON *json)
-{
-qstring_append(json->str, " ]");
-json->omit_comma = false;
-}
-
-void json_prop_int(QJSON *json, const char *name, int64_t val)
-{
-json_emit_element(json, name);
-qstring_append_format(json->str, "%" PRId64, val);
-}
-
-void json_prop_str(QJSON *json, const char *name, const char *str)
-{
-json_emit_element(json, name);
-qstring_append_json_string(json->str, str);
-}
-
-const char *qjson_get_str(QJSON *json)
-{
-return qstring_get_str(json->str);
-}
-
-QJSON *qjson_new(void)
-{
-QJSON *json = QJSON(object_new(TYPE_QJSON));
-return json;
-}
-
-void qjson_finish(QJSON *json)
-{
-json_end_object(json);
-}
-
-static void qjson_initfn(Object *obj)
-{
-QJSON *json = QJSON(obj);
-
-json->str = qstring_from_str("{ ");
-json->omit_comma = true;
-}
-
-static void qjson_finalizefn(Object *obj)
-{
-QJSON *json = QJSON(obj);
-
-qobject_decref(QOBJECT(json->str));
-}
-
-static const TypeInfo qjson_type_info = {
-.name = TYPE_QJSON,
-.parent = TYPE_OBJECT,
-.instance_size = sizeof(QJSON),
-.instance_init = qjson_initfn,
-.instance_finalize = qjson_finalizefn,
-};
-
-static void qjson_register_types(void)
-{
-type_register_static(_type_info);
-}
-
-type_init(qjson_register_types)
-- 
2.4.3

[Qemu-devel] [PATCH v2 09/14] Revert "qjson: Simplify by using json-output-visitor"

[Eric Blake]

This reverts commit 5859ad241516eed8cb9ba60889efa0ed47648b38.

The revert is here only to show the difference between two
alternatives, the final series will have just one choice of
patch 8, or of patches 10-11

---
 qjson.c | 61 +++--
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/qjson.c b/qjson.c
index 8874bad..8c93c1b 100644
--- a/qjson.c
+++ b/qjson.c
@@ -11,86 +11,103 @@
  *
  */

+#include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
-#include "qapi/json-output-visitor.h"

 struct QJSON {
 Object obj;
-JsonOutputVisitor *jov;
-char *str;
+QString *str;
+bool omit_comma;
 };

 #define QJSON(obj) OBJECT_CHECK(QJSON, (obj), TYPE_QJSON)

+static void json_emit_element(QJSON *json, const char *name)
+{
+/* Check whether we need to print a , before an element */
+if (json->omit_comma) {
+json->omit_comma = false;
+} else {
+qstring_append(json->str, ", ");
+}
+
+if (name) {
+qstring_append_json_string(json->str, name);
+qstring_append(json->str, " : ");
+}
+}
+
 void json_start_object(QJSON *json, const char *name)
 {
-Visitor *v = json_output_get_visitor(json->jov);
-visit_start_struct(v, name, NULL, 0, _abort);
+json_emit_element(json, name);
+qstring_append(json->str, "{ ");
+json->omit_comma = true;
 }

 void json_end_object(QJSON *json)
 {
-Visitor *v = json_output_get_visitor(json->jov);
-visit_check_struct(v, _abort);
-visit_end_struct(v);
+qstring_append(json->str, " }");
+json->omit_comma = false;
 }

 void json_start_array(QJSON *json, const char *name)
 {
-Visitor *v = json_output_get_visitor(json->jov);
-visit_start_list(v, name, NULL, 0, _abort);
+json_emit_element(json, name);
+qstring_append(json->str, "[ ");
+json->omit_comma = true;
 }

 void json_end_array(QJSON *json)
 {
-Visitor *v = json_output_get_visitor(json->jov);
-visit_end_list(v);
+qstring_append(json->str, " ]");
+json->omit_comma = false;
 }

 void json_prop_int(QJSON *json, const char *name, int64_t val)
 {
-Visitor *v = json_output_get_visitor(json->jov);
-visit_type_int(v, name, , _abort);
+json_emit_element(json, name);
+qstring_append_format(json->str, "%" PRId64, val);
 }

 void json_prop_str(QJSON *json, const char *name, const char *str)
 {
-Visitor *v = json_output_get_visitor(json->jov);
-visit_type_str(v, name, (char **), _abort);
+json_emit_element(json, name);
+qstring_append_json_string(json->str, str);
 }

 const char *qjson_get_str(QJSON *json)
 {
-return json->str;
+return qstring_get_str(json->str);
 }

 QJSON *qjson_new(void)
 {
 QJSON *json = QJSON(object_new(TYPE_QJSON));
-json_start_object(json, NULL);
 return json;
 }

 void qjson_finish(QJSON *json)
 {
 json_end_object(json);
-json->str = json_output_get_string(json->jov);
 }

 static void qjson_initfn(Object *obj)
 {
 QJSON *json = QJSON(obj);
-json->jov = json_output_visitor_new();
+
+json->str = qstring_from_str("{ ");
+json->omit_comma = true;
 }

 static void qjson_finalizefn(Object *obj)
 {
 QJSON *json = QJSON(obj);
-g_free(json->str);
+
+qobject_decref(QOBJECT(json->str));
 }

 static const TypeInfo qjson_type_info = {
-- 
2.4.3

[Qemu-devel] [PATCH v3 0/3] sdhci patches to enable Raspberry Pi

[Andrew Baumann]

This is a series of three tweaks needed to enable the generic sdhci
controller to emulate Raspberry Pi (bcm2835/2836), and boot Linux and
Windows.

There was some discussion of these changes in the following thread:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01401.html

v2 is functionally equivalent to v1; the only changes are to commit
messages and review metadata. v3 fixes two bugs in patch 3/3 (details
there).

Cheers,
Andrew

Andrew Baumann (2):
  sdhci: don't raise a command index error for an unexpected response
  sdhci: add optional quirk property to disable card insertion/removal
interrupts

Peter Crosthwaite (1):
  sd: sdhci: Delete over-zealous power check

 hw/sd/sdhci.c | 10 +-
 include/hw/sd/sdhci.h |  1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.5.3

[Qemu-devel] [PATCH v3 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts

[Andrew Baumann]

This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC
controller, where the card insert bit is documented as unimplemented
(always reads zero, doesn't generate interrupts) but is in fact
observed on hardware as set at power on, but is cleared (and remains
clear) on subsequent controller resets.

Signed-off-by: Andrew Baumann 
Reviewed-by: Peter Crosthwaite 
---

Notes:
v3: add missing field in header, drop needless change to vmstate

 hw/sd/sdhci.c | 5 -
 include/hw/sd/sdhci.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dd83e89..7acb4d7 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s)
  * initialization */
 memset(>sdmasysad, 0, (uintptr_t)>capareg - 
(uintptr_t)>sdmasysad);
 
-sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+if (!s->noeject_quirk) {
+sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+}
 s->data_count = 0;
 s->stopped_state = sdhc_not_stopped;
 }
@@ -1276,6 +1278,7 @@ static Property sdhci_sysbus_properties[] = {
 DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
 SDHC_CAPAB_REG_DEFAULT),
 DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index e78d938..ffd1f80 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -77,6 +77,7 @@ typedef struct SDHCIState {
 uint32_t buf_maxsz;
 uint16_t data_count;   /* current element in FIFO buffer */
 uint8_t  stopped_state;/* Current SDHC state */
+bool noeject_quirk;/* Quirk to disable card insert/remove interrupts */
 /* Buffer Data Port Register - virtual access point to R and W buffers */
 /* Software Reset Register - always reads as 0 */
 /* Force Event Auto CMD12 Error Interrupt Reg - write only */
-- 
2.5.3

[Qemu-devel] [PATCH v3 1/3] sd: sdhci: Delete over-zealous power check

[Andrew Baumann]

From: Peter Crosthwaite 

This check was conditionalising SD card operation on the card being
powered by the SDHCI host controller. It is however possible
(particularly in embedded systems) for the power control of the SD card
to be managed outside of SDHCI. This can be as trivial as hard-wiring
the SD slot VCC to a constant power-rail.

This means the guest SDHCI can validly opt-out of the SDHCI power
control feature while still using the card. So delete this check to
allow operation of the card with SDHCI power control.

This is needed for at least Xilinx Zynq and Raspberry Pi, and
also makes Freescale i.MX25 work for me. The digilent Zybo board
has a public schematic which shows SD VCC hardwiring:

http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf
bottom of page 3.

Signed-off-by: Peter Crosthwaite 
Reviewed-by: Sai Pavan Boddu 
[AB: Add Pi to list of devices fixed in commit message]
Signed-off-by: Andrew Baumann 
---
 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8612760..bc39d48 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -831,7 +831,7 @@ static void sdhci_data_transfer(void *opaque)
 
 static bool sdhci_can_issue_command(SDHCIState *s)
 {
-if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) ||
+if (!SDHC_CLOCK_IS_ON(s->clkcon) ||
 (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) &&
 ((s->cmdreg & SDHC_CMD_DATA_PRESENT) ||
 ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY &&
-- 
2.5.3

Re: [Qemu-devel] [PATCH] qmp: return err msg when powerdown a vm when it isn't in running state

[Qinghua Jin]

Thanks for your help, i'll resubmit the patch right away.


At 2015-12-22 01:55:35, "P J P"  wrote:
>+-- On Mon, 21 Dec 2015, Qinghua Jin wrote --+
>| -void qmp_system_powerdown(Error **erp)
>| +void qmp_system_powerdown(Error **errp)
>|  {
>| +if (!runstate_is_running()) {
>| +error_setg(errp, "Can't powerdown the Virtual Machine when it isn't 
>running");
>| +return;
>| +}
>|  qemu_system_powerdown_request();
>|  }
>
> - Maybe direct call to 'if (!runstate_check(RUN_STATE_RUNNING))' is better?  
>   runstate_is_running too invokes the same. Not sure why are there two
>   functions 'runstate_is_running' & 'runstate_check'.
> - Can't -> Can not
> - "...the Virtual Machine.." -> "...virtual machine.." (not capitalised)
> - "...when it isn't.." -> "as it is not..."
> - OR maybe just say -> "Virtual machine is not running"
>
>--
>Prasad J Pandit / Red Hat Product Security Team
>47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v5 RFC] spec: add qcow2 bitmaps extension specification

[Max Reitz]

On 21.12.2015 16:25, Vladimir Sementsov-Ogievskiy wrote:
> The new feature for qcow2: storing bitmaps.
> 
> Only bitmaps, relative to the virtual disk, stored in qcow2 file, should
> be stored in this qcow2 file.
> 
> Strings started from +# are RFC-strings, not to be commited of course
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> 
> v5:
> 
> - 'Dirty bitmaps' renamed to 'Bitmaps', as we may have several types of
>   bitmaps.
> - rewordings
> - move upper bounds to "Notes about Qemu limits"
> - s/should/must somewhere. (but not everywhere)
> - move name_size field closer to name itself in bitmap header
> - add extra data area to bitmap header
> - move bitmap data description to separate section
> 
> 
> 
>  docs/specs/qcow2.txt | 160 
> ++-
>  1 file changed, 159 insertions(+), 1 deletion(-)

Looks good! :-)

Some comments below, but I think the general design is good now.

> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 121dfc8..3d557ee 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -103,7 +103,19 @@ in the description of a field.
>  write to an image with unknown auto-clear features if it
>  clears the respective bits from this field first.
>  
> -Bits 0-63:  Reserved (set to 0)
> +Bit 0:  Bitmaps extension bit.
> +This bit is responsible for Bitmaps extension
> +consistency.
> +
> +If it is set, but there is no Bitmaps
> +extension, this should be considered as an
> +error.
> +
> +If it is not set, but there is a Bitmaps
> +extension, its data should be considered as
> +inconsistent.
> +
> +Bits 1-63:  Reserved (set to 0)
>  
>   96 -  99:  refcount_order
>  Describes the width of a reference count block entry 
> (width
> @@ -123,6 +135,7 @@ be stored. Each extension has a structure like the 
> following:
>  0x - End of the header extension area
>  0xE2792ACA - Backing file format name
>  0x6803f857 - Feature name table
> +0x23852875 - Bitmaps extension
>  other  - Unknown header extension, can be safely
>   ignored
>  
> @@ -166,6 +179,34 @@ the header extension data. Each entry look like this:
>  terminated if it has full length)
>  
>  
> +== Bitmaps extension ==
> +
> +Bitmaps extension is an optional header extension. It provides an ability to
> +store virtual disk related bitmaps in a qcow2 image. For now there is only 
> one
> +type of such bitmaps: Dirty Tracking Bitmap, which just tracks virtual disk
> +changes from some moment.
> +
> +The data of the extension should be considered as consistent only if
> +corresponding auto-clear feature bit is set (see autoclear_features above).
> +
> +The fields of Bitmaps extension are:
> +
> +  0 -  3:  nb_bitmaps
> +   The number of bitmaps contained in the image. Must be
> +   greater or equal to 1.
> +
> +   Note: Qemu currently only supports up to 65535 bitmaps per
> +   image.
> +
> +  4 -  7:  bitmap_directory_size
> +   Size of the Bitmap Directory in bytes. It must be equal to
> +   sum of sizes of all (nb_bitmaps) bitmap headers.

I'd rather write this as: "Size of the Bitmap Directory in bytes, i.e.
the cumulative size of all (nb_bitmaps) bitmap headers."

("It must" sounds like it's an additional restriction while it's
actually just an explanation.)

> +
> +  8 - 15:  bitmap_directory_offset
> +   Offset into the image file at which the Bitmap Directory
> +   starts. Must be aligned to a cluster boundary.
> +
> +
>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference 
> count
> @@ -360,3 +401,120 @@ Snapshot table entry:
>  
>  variable:   Padding to round up the snapshot table entry size to the
>  next multiple of 8.
> +
> +
> +== Bitmaps ==
> +
> +The feature supports storing bitmaps in a qcow2 image. All bitmaps are 
> related
> +to the virtual disk, stored in this image.
> +
> +=== Bitmap Directory ===
> +
> +Each bitmap saved in the image is described in a Bitmap Directory entry. 
> Bitmap
> +Directory is a contiguous area in the image file, whose starting offset and
> +length are given by the header extension fields bitmap_directory_offset and
> +bitmap_directory_size. The entries of the bitmap directory have 

Re: [Qemu-devel] [PATCH 1/8] bcm2835_sbm: add BCM2835 mailboxes

[Peter Crosthwaite]

On Thu, Dec 3, 2015 at 10:01 PM, Andrew Baumann
 wrote:
> This adds the system mailboxes which are used to communicate with a
> number of GPU peripherals on Pi/Pi2.
>
> Signed-off-by: Andrew Baumann 
> ---
>  default-configs/arm-softmmu.mak  |   1 +
>  hw/misc/Makefile.objs|   1 +
>  hw/misc/bcm2835_sbm.c| 280 

>  include/hw/arm/bcm2835_arm_control.h | 481 
> +++

Do we need this file as of this patch?

>  include/hw/arm/bcm2835_mbox.h|  19 ++
>  include/hw/misc/bcm2835_sbm.h|  37 +++
>  6 files changed, 819 insertions(+)
>  create mode 100644 hw/misc/bcm2835_sbm.c
>  create mode 100644 include/hw/arm/bcm2835_arm_control.h
>  create mode 100644 include/hw/arm/bcm2835_mbox.h
>  create mode 100644 include/hw/misc/bcm2835_sbm.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index d9b90a5..a9f82a1 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -79,6 +79,7 @@ CONFIG_TUSB6010=y
>  CONFIG_IMX=y
>  CONFIG_MAINSTONE=y
>  CONFIG_NSERIES=y
> +CONFIG_RASPI=y
>  CONFIG_REALVIEW=y
>  CONFIG_ZAURUS=y
>  CONFIG_ZYNQ=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index aeb6b7d..b9379f2 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -34,6 +34,7 @@ obj-$(CONFIG_OMAP) += omap_gpmc.o
>  obj-$(CONFIG_OMAP) += omap_l4.o
>  obj-$(CONFIG_OMAP) += omap_sdrc.o
>  obj-$(CONFIG_OMAP) += omap_tap.o
> +obj-$(CONFIG_RASPI) += bcm2835_sbm.o
>  obj-$(CONFIG_SLAVIO) += slavio_misc.o
>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>  obj-$(CONFIG_ZYNQ) += zynq-xadc.o
> diff --git a/hw/misc/bcm2835_sbm.c b/hw/misc/bcm2835_sbm.c
> new file mode 100644
> index 000..a5c9324
> --- /dev/null
> +++ b/hw/misc/bcm2835_sbm.c
> @@ -0,0 +1,280 @@
> +/*
> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> + * This code is licensed under the GNU GPLv2 and later.
> + */
> +
> +#include "hw/misc/bcm2835_sbm.h"
> +#include "hw/arm/bcm2835_arm_control.h"
> +
> +static void mbox_update_status(BCM2835Mbox *mb)
> +{
> +if (mb->count == 0) {
> +mb->status |= ARM_MS_EMPTY;
> +} else {
> +mb->status &= ~ARM_MS_EMPTY;
> +}
> +if (mb->count == MBOX_SIZE) {
> +mb->status |= ARM_MS_FULL;
> +} else {
> +mb->status &= ~ARM_MS_FULL;
> +}

The start-from-scratch approach makes this shorter:

mb->status &= ~(ARM_MS_EMPTY | ARM_MS_FULL);
if (mb->count == 0) {
mb->status |= ARM_MS_EMPTY;
}
if (mb->count == MBOX_SIZE) {
mb->status |= ARM_MS_FULL;
}

> +}
> +
> +static void mbox_init(BCM2835Mbox *mb)

This looks like a reset function.

> +{
> +int n;

Blank line here.

> +mb->count = 0;
> +mb->config = 0;
> +for (n = 0; n < MBOX_SIZE; n++) {
> +mb->reg[n] = MBOX_INVALID_DATA;
> +}
> +mbox_update_status(mb);
> +}
> +
> +static uint32_t mbox_pull(BCM2835Mbox *mb, int index)
> +{
> +int n;
> +uint32_t val;
> +
> +assert(mb->count > 0);
> +assert(index < mb->count);
> +
> +val = mb->reg[index];
> +for (n = index + 1; n < mb->count; n++) {
> +mb->reg[n - 1] = mb->reg[n];
> +}
> +mb->count--;
> +mb->reg[mb->count] = MBOX_INVALID_DATA;
> +
> +mbox_update_status(mb);
> +
> +return val;
> +}
> +
> +static void mbox_push(BCM2835Mbox *mb, uint32_t val)
> +{
> +assert(mb->count < MBOX_SIZE);
> +mb->reg[mb->count++] = val;
> +mbox_update_status(mb);
> +}
> +
> +static void bcm2835_sbm_update(BCM2835SbmState *s)

What does Sbm stand for? If it is acronym is should be all caps in camel case.

> +{
> +int set;

bool.

> +int done, n;
> +uint32_t value;
> +
> +/* Avoid unwanted recursive calls */
> +s->mbox_irq_disabled = 1;
> +
> +/* Get pending responses and put them in the vc->arm mbox */
> +done = 0;
> +while (!done) {
> +done = 1;
> +if (s->mbox[0].status & ARM_MS_FULL) {
> +/* vc->arm mbox full, exit */

break here.

> +} else {

so you can drop the else and get back a level of indent.

> +for (n = 0; n < MBOX_CHAN_COUNT; n++) {
> +if (s->available[n]) {
> +value = ldl_phys(>mbox_as, n<<4);
> +if (value != MBOX_INVALID_DATA) {
> +mbox_push(>mbox[0], value);
> +} else {
> +/* Hmmm... */

Needs more comment :)

> +}
> +done = 0;
> +break;
> +}
> +}
> +}
> +}
> +
> +/* Try to push pending requests from the arm->vc mbox */
> +/* TODO (?) */
> +
> +/* Re-enable calls from the IRQ routine */
> +s->mbox_irq_disabled = 0;
> +
> +/* Update ARM IRQ status */
> +set = 0;
> +if (s->mbox[0].status & ARM_MS_EMPTY) {
> +

[Qemu-devel] [PATCH v3 2/3] sdhci: don't raise a command index error for an unexpected response

[Andrew Baumann]

This deletes a block of code that raised a command index error if a
command returned response data, but the guest did not set the
appropriate bits in the response register to handle such a response. I
cannot find any documentation that suggests the controller should
behave in this way, the error code doesn't make sense (command index
error is defined for the case where the index in a response does not
match that of the issued command), and in at least one case (CMD23
issued by UEFI on Raspberry Pi 2), actual hardware does not do this.

Signed-off-by: Andrew Baumann 
Reviewed-by: Peter Crosthwaite 
---
 hw/sd/sdhci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index bc39d48..dd83e89 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -243,9 +243,6 @@ static void sdhci_send_command(SDHCIState *s)
 (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
 s->norintsts |= SDHC_NIS_TRSCMP;
 }
-} else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) {
-s->errintsts |= SDHC_EIS_CMDIDX;
-s->norintsts |= SDHC_NIS_ERR;
 }
 
 if (s->norintstsen & SDHC_NISEN_CMDCMP) {
-- 
2.5.3

Re: [Qemu-devel] [PATCH 1/8] bcm2835_sbm: add BCM2835 mailboxes

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Monday, 21 December 2015 15:33
> On Mon, Dec 21, 2015 at 3:15 PM, Andrew Baumann
>  wrote:
> >> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> >> Sent: Monday, 21 December 2015 14:49
> >> On Thu, Dec 3, 2015 at 10:01 PM, Andrew Baumann
> >>  wrote:
> >> > +
> >> > +switch (offset) {
> >> > +case 0x80:  /* MAIL0_READ */
> >> > +case 0x84:
> >> > +case 0x88:
> >> > +case 0x8c:
> >>
> >> case 0x80..0x8c
> >
> > Woah! Is that standard C?
> >
> 
> Yes, its probably one of the more recent language standards though.
> QEMU does use to more modern features liberally.

Notwithstanding that it's widely used in QEMU (and a nice feature too), and not 
that I have a problem with adopting it, but just because I'm a pedant: this is 
definitely not in C99 (per qemu/HACKING). It doesn't appears in C11 either (at 
least not the final draft). It's a gcc extension:
http://gcc.gnu.org/onlinedocs/gcc/Case-Ranges.html

> > [...]
> >> > --- /dev/null
> >> > +++ b/include/hw/arm/bcm2835_arm_control.h
> >> > @@ -0,0 +1,481 @@
> >> > +/*
> >> > + *  linux/arch/arm/mach-bcm2708/arm_control.h
> > [...]
> >> When you have a regular structure like this, you should collapse it
> >> down using some arithmatic:
> >
> > Notice that this file comes from Linux. I know it's not pretty, but can we
> please keep it as-is, for comparison purposes? I'm not sure there's much
> value in cleaning it up locally...
> >
> 
> It looks very autogenerated and seems pretty nasty on the repetition.
> 
> As implementers of the hardware, it is much rarer to need these
> repetitious defs than the software users on the other side. "Do
> something specific with CPU#3's Mbox#5" is going to appear in
> software, but hardware implementers generally don't have a choice to
> implement things specifically and it usually ends up being looped and
> the exploded defs are never used. If there are only a handful of
> genuinely single defs needed, can they be fished out?

I think so. I'll do that then.

Andrew

Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 1:04 PM, Andrew Baumann
 wrote:
>> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
>> Sent: Sunday, 20 December 2015 14:58
>> On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
>>  wrote:
>> > The SD spec for ACMD41 says that a zero argument is an "inquiry"
>> > ACMD41, which does not start initialisation and is used only for
>> > retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
>> > first sends an inquiry (zero) ACMD41. If that first request returns an
>> > OCR value with the power up bit (0x8000) set, it assumes the card
>> > is ready and continues, leaving the card in the wrong state. (My
>> > assumption is that this works on hardware, because no real card is
>> > immediately powered up upon reset.)
>> >
>> > This change models a delay of 0.5ms from the first ACMD41 to the power
>> > being up. However, it also immediately sets the power on upon seeing a
>> > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should
>> > also account for guests that simply delay after card reset and then
>> > issue an ACMD41 that they expect will succeed.
> [...]
>> Can this be done in a non-rPI specific way by just kicking off the
>> timer on card reset?
>
> No :( I tried that first, but there is no value for the timeout that works 
> reliably and isn't huge. UEFI doesn't reset the card itself (it relies on the 
> hardware reset), and there can be a large/variable delay after power on 
> before its gets to the SD init (particularly for debug builds, which do lots 
> of printing to the serial port).
>
> BTW, this is generic to UEFI; I don't believe it's Pi-specific.
>

Ok fair enough,

This way works and handles pretty much every case. There is one more
review comment RE the VMSD ill make on original mail.

Regards,
Peter

> Andrew

Re: [Qemu-devel] [PATCH v2 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 1:31 PM, Andrew Baumann
 wrote:
> This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC
> controller, where the card insert bit is documented as unimplemented
> (always reads zero, doesn't generate interrupts) but is in fact
> observed on hardware as set at power on, but is cleared (and remains
> clear) on subsequent controller resets.
>
> Signed-off-by: Andrew Baumann 
> Reviewed-by: Peter Crosthwaite 
> ---
>  hw/sd/sdhci.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index dd83e89..61f919b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s)
>   * initialization */
>  memset(>sdmasysad, 0, (uintptr_t)>capareg - 
> (uintptr_t)>sdmasysad);
>
> -sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +if (!s->noeject_quirk) {
> +sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +}
>  s->data_count = 0;
>  s->stopped_state = sdhc_not_stopped;
>  }
> @@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = {
>  VMSTATE_UINT16(data_count, SDHCIState),
>  VMSTATE_UINT64(admasysaddr, SDHCIState),
>  VMSTATE_UINT8(stopped_state, SDHCIState),
> +VMSTATE_BOOL(noeject_quirk, SDHCIState),

Sorry, one small thing I missed, static props should not be in the
VMSD. I think you just need to drop the VMSTATE_ addition here.
Otherwise you would need a VMSD version bump.

Is the patch missing the corresponding header change to add the new field?

Regards,
Peter

>  VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, 
> buf_maxsz),
>  VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
>  VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
> @@ -1276,6 +1279,7 @@ static Property sdhci_sysbus_properties[] = {
>  DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>  SDHC_CAPAB_REG_DEFAULT),
>  DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> +DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false),
>  DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 2.5.3
>

Re: [Qemu-devel] [PATCH 1/8] bcm2835_sbm: add BCM2835 mailboxes

[Andrew Baumann]

Hi Peter,

Thanks for the review!

> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Monday, 21 December 2015 14:49
> On Thu, Dec 3, 2015 at 10:01 PM, Andrew Baumann
>  wrote:
> > This adds the system mailboxes which are used to communicate with a
> > number of GPU peripherals on Pi/Pi2.
> >
> > Signed-off-by: Andrew Baumann 
> > ---
> >  default-configs/arm-softmmu.mak  |   1 +
> >  hw/misc/Makefile.objs|   1 +
> >  hw/misc/bcm2835_sbm.c| 280 
> 
> >  include/hw/arm/bcm2835_arm_control.h | 481
> +++
> 
> Do we need this file as of this patch?

Yes, for ARM_MC_IHAVEDATAIRQEN and other related defines.

[...]
> > +static void bcm2835_sbm_update(BCM2835SbmState *s)
> 
> What does Sbm stand for? If it is acronym is should be all caps in camel case.

I'm not sure -- it comes from Gregory's code; maybe he can comment? Where this 
device gets instantiated, there is a comment of 

/* Semaphores / Doorbells / Mailboxes */

... but only mailboxes are implemented here. I'm guessing maybe it's intended 
as "system mailboxes". I can rename it.


> > +{
> > +int set;
> 
> bool.
> 
> > +int done, n;
> > +uint32_t value;
> > +
> > +/* Avoid unwanted recursive calls */
> > +s->mbox_irq_disabled = 1;
> > +
> > +/* Get pending responses and put them in the vc->arm mbox */
> > +done = 0;
> > +while (!done) {
> > +done = 1;
> > +if (s->mbox[0].status & ARM_MS_FULL) {
> > +/* vc->arm mbox full, exit */
> 
> break here.
> 
> > +} else {
> 
> so you can drop the else and get back a level of indent.
> 
> > +for (n = 0; n < MBOX_CHAN_COUNT; n++) {
> > +if (s->available[n]) {
> > +value = ldl_phys(>mbox_as, n<<4);
> > +if (value != MBOX_INVALID_DATA) {
> > +mbox_push(>mbox[0], value);
> > +} else {
> > +/* Hmmm... */
> 
> Needs more comment :)

Gregory -- help! :)

[...]
> > +s->available[irq] = level;
> > +if (!s->mbox_irq_disabled) {
> 
> I don't think you need this. IO devices are not thread safe and
> core-code knows it. So you don't need to worry about interruption and
> re-entry of your IRQ handler.

I will have to put a debugger on this to confirm, but I think the issue is that 
we are using a private memory region and GPIOs to route mailbox data and 
interrupts to the relevant sub peripherals. The ldl_phys invokes another device 
emulation (such as bcm2835_property.c in this series), which may cause it to 
signal an interrupt back to us via qemu_set_irq which will recursively run our 
handler. We want that IRQ to be recorded but "squashed".

[...]
> > +
> > +switch (offset) {
> > +case 0x80:  /* MAIL0_READ */
> > +case 0x84:
> > +case 0x88:
> > +case 0x8c:
> 
> case 0x80..0x8c

Woah! Is that standard C?

[...]
> > --- /dev/null
> > +++ b/include/hw/arm/bcm2835_arm_control.h
> > @@ -0,0 +1,481 @@
> > +/*
> > + *  linux/arch/arm/mach-bcm2708/arm_control.h
[...]
> When you have a regular structure like this, you should collapse it
> down using some arithmatic:

Notice that this file comes from Linux. I know it's not pretty, but can we 
please keep it as-is, for comparison purposes? I'm not sure there's much value 
in cleaning it up locally...

> > --- /dev/null
> > +++ b/include/hw/misc/bcm2835_sbm.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> > + * This code is licensed under the GNU GPLv2 and later.
> > + */
> > +
> > +#ifndef BCM2835_SBM_H
> > +#define BCM2835_SBM_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/arm/bcm2835_mbox.h"
> > +
> > +#define TYPE_BCM2835_SBM "bcm2835_sbm"
> > +#define BCM2835_SBM(obj) \
> > +OBJECT_CHECK(BCM2835SbmState, (obj), TYPE_BCM2835_SBM)
> > +
> > +typedef struct {
> > +MemoryRegion *mbox_mr;
> > +AddressSpace mbox_as;
> 
> I can't see where these two fields are used. A quick scan shows that
> the SbmState's copy is uses for all DMA ops. If these are needed,
> mbox_init should pass a pointer to the the SbmState then this saves
> the pointer and navigates as needed back to the container structure to
> get the AS.

You're right. They're uninitialised/unused detritus from a previous refactor. 
I'll remove them.

Thanks,
Andrew

Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Gonglei (Arei)]

Dear Kevin,

> -Original Message-
> From: Kevin O'Connor [mailto:ke...@koconnor.net]
> Sent: Sunday, December 20, 2015 10:33 PM
> To: Gonglei (Arei)
> Cc: Xulei (Stone); Paolo Bonzini; qemu-devel; seab...@seabios.org;
> Huangweidong (C); k...@vger.kernel.org; Radim Krcmar
> Subject: Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy
> problem on qemu-kvm platform
> 
> On Sun, Dec 20, 2015 at 09:49:54AM +, Gonglei (Arei) wrote:
> > > From: Kevin O'Connor [mailto:ke...@koconnor.net]
> > > Sent: Saturday, December 19, 2015 11:12 PM
> > > On Sat, Dec 19, 2015 at 12:03:15PM +, Gonglei (Arei) wrote:
> > > > Maybe the root cause is not NMI but INTR, so yield() can open hardware
> > > interrupt,
> > > > And then execute interrupt handler, but the interrupt handler make the
> > > SeaBIOS
> > > > stack broken, so that the BSP can't execute the instruction and occur
> > > exception,
> > > > VM_EXIT to Kmod, which is an infinite loop. But I don't have any proofs
> except
> > > > the surface phenomenon.
> > >
> > > I can't see any reason why allowing interrupts at this location would
> > > be a problem.
> > >
> > Does it have any relationship with *extra stack* of SeaBIOS?
> 
> None that I can see.  Also, the kvm trace seems to show the code
> trying to execute at rip=0x03 - that will crash long before the extra
> stack is used.
> 
When the gurb of OS is booting, then the softirq and C function send_disk_op()
may use extra stack of SeaBIOS. If we inject a NMI, romlayout.S: 
irqentry_extrastack
is invoked, and the extra stack will be used again. And the stack of first 
calling
will be broken, so that the SeaBIOS stuck. 

You can easily reproduce the problem.

1. start on guest
2. reset the guest
3. inject a NMI when the guest show the grub surface
4. then the guest stuck

If we disabled extra stack by setting

 CONFIG_ENTRY_EXTRASTACK=n

Then the problem is gone.

Besides, I have another thought:

Is it possible when one cpu is using the extra stack, but other cpus (APs)
still be waked up by hardware interrupt after yield() or br->flags = F_IF 
and used the extra stack again?


Regards,
-Gonglei

> > > > Kevin, can we drop yield() in smp_setup() ?
> > >
> > > It's possible to eliminate this instance of yield, but I think it
> > > would just push the crash to the next time interrupts are enabled.
> > >
> > Perhaps. I'm not sure.
> >
> > > > Is it really useful and allowable for SeaBIOS? Maybe for other
> components?
> > > > I'm not sure. Because we found that when SeaBIOS is booting, if we 
> > > > inject
> a
> > > > NMI by QMP, the guest will *stuck*. And the kvm tracing log is the same
> with
> > > > the current problem.
> > >
> > > If you apply the patches you had to prevent that NMI crash problem,
> > > does it also prevent the above crash?
> > >
> > Yes, but we cannot prevent the NMI injection (though I'll submit some
> patches to
> > forbid users' NMI injection after NMI_EN disabled by RTC bit7 of port 0x70).
> >
> 
> -Kevin

Re: [Qemu-devel] [PATCH] qmp: return err msg when powerdown a vm when it isn't in running state

[P J P]

+-- On Mon, 21 Dec 2015, Qinghua Jin wrote --+
| -void qmp_system_powerdown(Error **erp)
| +void qmp_system_powerdown(Error **errp)
|  {
| +if (!runstate_is_running()) {
| +error_setg(errp, "Can't powerdown the Virtual Machine when it isn't 
running");
| +return;
| +}
|  qemu_system_powerdown_request();
|  }

 - Maybe direct call to 'if (!runstate_check(RUN_STATE_RUNNING))' is better?  
   runstate_is_running too invokes the same. Not sure why are there two
   functions 'runstate_is_running' & 'runstate_check'.
 - Can't -> Can not
 - "...the Virtual Machine.." -> "...virtual machine.." (not capitalised)
 - "...when it isn't.." -> "as it is not..."
 - OR maybe just say -> "Virtual machine is not running"

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] scsi: initialise info object with appropriate size

[P J P]

+-- On Mon, 21 Dec 2015, Paolo Bonzini wrote --+
| I can add the Cc to the commit message as well.  For now it's enough to
| send a message in Cc so that the qemu-stable people notice it.

Okay, great! Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v4 RFC] spec: add qcow2-dirty-bitmaps specification

[Vladimir Sementsov-Ogievskiy]

On 14.12.2015 23:45, John Snow wrote:


On 12/14/2015 03:05 PM, Max Reitz wrote:

On 14.12.2015 18:43, Vladimir Sementsov-Ogievskiy wrote:

The new feature for qcow2: storing dirty bitmaps.

Only dirty bitmaps relative to this qcow2 image should be stored in it.

Strings started from +# are RFC-strings, not to be commited of course.


Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

  docs/specs/qcow2.txt | 151 ++-
  1 file changed, 150 insertions(+), 1 deletion(-)

Overall: Looks better to me. Good enough for me to ACK it, but I still
have some issues with it.

Let's evaluate the main point of critique I had: I really want this not
to be qemu-specific but potentially useful to all programs.

Pretty good: You do implicitly describe what a (dirty) bitmap looks like
by describing how to obtain the bit offset of a certain byte guest
offset. So it's not an opaque binary data dump anymore.

(Why only "pretty good"? I find the description to be a bit too
"implicit", I think a separate section describing the bitmap structure
would be better.)

Good: The bitmap actually describes the qcow2 file.

Not so good: While now any program knows how to read the bitmap and that
it does refer to this qcow2 file, it's interpretation is not so easy
still. Generally, a dirty bitmap has some reference point, that is the
state of the disk when the bitmap was cleared or created. For instance,
for incremental backups, whenever you create a backup based on a dirty
bitmap, the dirty bitmap is cleared and the backup target is then said
reference point.
I think it would be nice to put that reference point (i.e. the name of
an image file that contains the clean image) into the dirty bitmap
header, if possible.


This starts to get a little spooky, because QEMU doesn't necessarily
know where (or what) the reference is. QEMU doesn't even know where the
last incremental is.

It might be hard to store something meaningful here.

I suppose the management application could do it itself if it wants to.


(Note: I won't comment on orthography, because I feel like that is
something a native speaker should do. O:-))


diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 121dfc8..3c89580 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -103,7 +103,17 @@ in the description of a field.
  write to an image with unknown auto-clear features if it
  clears the respective bits from this field first.
  
-Bits 0-63:  Reserved (set to 0)

+Bit 0:  Dirty bitmaps bit.
+This bit is responsible for Dirty bitmaps
+extension consistency.
+If it is set, but there is no Dirty bitmaps
+extensions, this should be considered as an
+error.
+If it is not set, but there is a Dirty bitmaps
+extension, its data should be considered as
+inconsistent.
+
+Bits 1-63:  Reserved (set to 0)
  
   96 -  99:  refcount_order

  Describes the width of a reference count block entry 
(width
@@ -123,6 +133,7 @@ be stored. Each extension has a structure like the 
following:
  0x - End of the header extension area
  0xE2792ACA - Backing file format name
  0x6803f857 - Feature name table
+0x23852875 - Dirty bitmaps
  other  - Unknown header extension, can be safely
   ignored
  
@@ -166,6 +177,31 @@ the header extension data. Each entry look like this:

  terminated if it has full length)
  
  
+== Dirty bitmaps ==

+
+Dirty bitmaps is an optional header extension. It provides an ability to store
+dirty bitmaps in a qcow2 image. The data of this extension should be considered
+as consistent only if corresponding auto-clear feature bit is set (see
+autoclear_features above).
+The fields of Dirty bitmaps extension are:
+
+  0 -  3:  nb_dirty_bitmaps
+   The number of dirty bitmaps contained in the image. Valid
+   values: 1 - 65535.

Again, I don't see a reason for why we should impose a strict upper
limit here. I'd prefer "Note that qemu currently only supports up to
65535 dirty bitmaps per image."


After discussing this with Eric, I agree.

It's hard to present justification for arbitrary limits if our only
concern is "That's just too many."

Let's list the pragmatic limit we intend to impose in QEMU, but allow
the format to use the fill width of this field.


+# Let's be strict, the feature should be deleted with deleting last bitmap.
+
+  4 -  7:  dirty_bitmap_directory_size

[Qemu-devel] ODP: [PATCH 02/12] Added reset-pin emulation in model.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:04, Peter Crosthwaite pisze:
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 38 +-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index bfd493f..bcb66a5 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -258,6 +258,8 @@ typedef struct Flash {
>>  uint8_t cmd_in_progress;
>>  uint64_t cur_addr;
>>  bool write_enable;
>> +bool initialized;
>> +uint8_t reset_pin;
>>
>>  int64_t dirty_page;
>>
>> @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s)
>>  }
>>  }
>>
>> +static void reset_memory(Flash *s)
>> +{
>> +s->cmd_in_progress = NOP;
>> +s->cur_addr = 0;
>> +s->len = 0;
>> +s->needed_bytes = 0;
>> +s->pos = 0;
>> +s->state = STATE_IDLE;
>> +s->write_enable = false;
>> +
>> +DB_PRINT_L(0, "Reset done.\n");
>> +}
>> +
>>  static void decode_new_cmd(Flash *s, uint32_t value)
>>  {
>>  s->cmd_in_progress = value;
>> @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss)
>>  s->size = s->pi->sector_size * s->pi->n_sectors;
>>  s->dirty_page = -1;
>>
>> +s->initialized = false;
>> +
>
> Init functions should never set runtime state.
>
>>  /* FIXME use a qdev drive property instead of drive_get_next() */
>>  dinfo = drive_get_next(IF_MTD);
>>
>> @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque)
>>  flash_sync_dirty((Flash *)opaque, -1);
>>  }
>>
>> +static void m25p80_reset(DeviceState *d)
>> +{
>> +Flash *s = M25P80(d);
>> +
>> +if (s->reset_pin || !s->initialized) {
>> +reset_memory(s);
>> +s->initialized = true;
>
> Bus I think the real issue here is that is trying to model something
> other than a power-on-reset. Looks like a a soft reset. the dc->reset
> should be a power-on-reset and not be conditional on anything. Only
> the non-volatile state (the storage data) should persist.
>
> Is your board using qemu_system_reset_request() by any chance for
> something that is not supposed to reset the whole board? If this is
> the case, you need to limit the scope of that reset logic and just set
> your board up to not use the centralized all-devices reset (which is
> pretty broken for these modern boards/SoCs that have multiple reset
> domains).
>
> Regards,
> Peter
That is not exactly my case.
I want to emulate device that have a RESET signal functionality (eg. n25q128 
has such option).
The other think that I want to have is possibility to use this signal by qemu 
model
(eg. pin is connected on board to reset IC, or it is unused).
The use case: qemu boots us powered-up board, then I issue 
qemu_system_reset_request().
Then if the pin is connected (propeti is set, flash wil also reset, in other 
case will not).
I don't see why it against reset signal implementation.

Regards,
Marcin

>
>> +}
>> +}
>> +
>> +static Property m25p80_properties[] = {
>> +DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0),
>> +DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static const VMStateDescription vmstate_m25p80 = {
>>  .name = "xilinx_spi",
>> -.version_id = 1,
>> +.version_id = 2,
>>  .minimum_version_id = 1,
>>  .pre_save = m25p80_pre_save,
>>  .fields = (VMStateField[]) {
>> @@ -664,6 +696,8 @@ static const VMStateDescription vmstate_m25p80 = {
>>  VMSTATE_UINT8(cmd_in_progress, Flash),
>>  VMSTATE_UINT64(cur_addr, Flash),
>>  VMSTATE_BOOL(write_enable, Flash),
>> +VMSTATE_BOOL(initialized, Flash),
>> +VMSTATE_UINT8(reset_pin, Flash),
>>  VMSTATE_END_OF_LIST()
>>  }
>>  };
>> @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, void 
>> *data)
>>  k->set_cs = m25p80_cs;
>>  k->cs_polarity = SSI_CS_LOW;
>>  dc->vmsd = _m25p80;
>> +dc->reset = _reset;
>> +dc->props = m25p80_properties;
>>  mc->pi = data;
>>  }
>>
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] ODP: [PATCH 04/12] Changed variable type to allow serial eeprom emulation (changing 0->1).

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:23, Peter Crosthwaite pisze:
> Your commit message subject line should be a little shorter and
> lengthier explanations of the patch content go here as a paragraph.
> You should also have subsystem prefixes to patch subject lines. This
> patch would be something like:
>
> block: m25p80: widen flags variable
>
> Extend the width of the flags variable to support the already existing
> (but unused) WR_1 flag, which is above the range of 8 bits. This is
> turn allows support of EEPROM emulation which requires the WR_1
> feature.
Thanks,
Marcin
>
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite 
>
> Regards,
> Peter
>
>> ---
>>  hw/block/m25p80.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 5e07b57..fbbfd1d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -60,7 +60,7 @@ typedef struct FlashPartInfo {
>>  uint32_t sector_size;
>>  uint32_t n_sectors;
>>  uint32_t page_size;
>> -uint8_t flags;
>> +uint16_t flags;
>>  } FlashPartInfo;
>>
>>  /* adapted from linux */
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] ODP: [PATCH 10/12] Support for quad commands.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:52, Peter Crosthwaite pisze:
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Pawel Lenkow 
>
> Same comments as for earlier patches. Need to clarify authorship and
> add subsystem prefix and commit paragraph.
>
>> ---
>>  hw/block/m25p80.c | 38 --
>>  1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 6fc55a3..25ec666 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -255,19 +255,24 @@ typedef enum {
>>  BULK_ERASE = 0xc7,
>>
>>  READ = 0x3,
>> +READ4 = 0x13,
>>  FAST_READ = 0xb,
>>  DOR = 0x3b,
>>  QOR = 0x6b,
>>  DIOR = 0xbb,
>>  QIOR = 0xeb,
>> +QIOR4 = 0xec,
>>
>>  PP = 0x2,
>> +PP4 = 0x12,
>>  DPP = 0xa2,
>>  QPP = 0x32,
>>
>>  ERASE_4K = 0x20,
>> +ERASE4_4K = 0x21,
>>  ERASE_32K = 0x52,
>>  ERASE_SECTOR = 0xd8,
>> +ERASE4_SECTOR = 0xdc,
>>
>>  EN_4BYTE_ADDR = 0xB7,
>>  EX_4BYTE_ADDR = 0xE9,
>> @@ -307,6 +312,7 @@ typedef struct Flash {
>>  bool write_enable;
>>  bool four_bytes_address_mode;
>>  bool reset_enable;
>> +bool quad_enable;
>
> What is this used for?
>
>>  bool initialized;
>>  uint8_t reset_pin;
>>  uint8_t ear;
>> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD 
>> cmd)
>>
>>  switch (cmd) {
>>  case ERASE_4K:
>> +case ERASE4_4K:
>>  len = 4 << 10;
>>  capa_to_assert = ER_4K;
>>  break;
>> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD 
>> cmd)
>>  capa_to_assert = ER_32K;
>>  break;
>>  case ERASE_SECTOR:
>> +case ERASE4_SECTOR:
>>  len = s->pi->sector_size;
>>  break;
>>  case BULK_ERASE:
>> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>>
>>  static inline int is_4bytes(Flash *s)
>>  {
>> +   switch (s->cmd_in_progress) {
>> +   case PP4:
>> +   case READ4:
>> +   case QIOR4:
>> +   case ERASE4_4K:
>> +   case ERASE4_SECTOR:
>> +   return 1;
>> +   default:
>> return s->four_bytes_address_mode;
>> }
>>  }
>> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
>>  case DPP:
>>  case QPP:
>>  case PP:
>> +case PP4:
>>  s->state = STATE_PAGE_PROGRAM;
>>  break;
>>  case READ:
>> +case READ4:
>>  case FAST_READ:
>>  case DOR:
>>  case QOR:
>>  case DIOR:
>>  case QIOR:
>> +case QIOR4:
>>  s->state = STATE_READ;
>>  break;
>>  case ERASE_4K:
>> +case ERASE4_4K:
>>  case ERASE_32K:
>>  case ERASE_SECTOR:
>> +case ERASE4_SECTOR:
>>  flash_erase(s, s->cur_addr, s->cmd_in_progress);
>>  break;
>>  case WRSR:
>> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
>>  s->state = STATE_IDLE;
>>  s->write_enable = false;
>>  s->reset_enable = false;
>> +s->quad_enable = false;
>>
>>  DB_PRINT_L(0, "Reset done.\n");
>>  }
>> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
>>  static void decode_new_cmd(Flash *s, uint32_t value)
>>  {
>>  s->cmd_in_progress = value;
>> +int i;
>
> I can't see where i is used?
Should be in patch 09/12.
I will correct this.
Thanks,
Marcin
>
>
>>  DB_PRINT_L(0, "decoded new command:%x\n", value);
>>
>>  switch (value) {
>>
>>  case ERASE_4K:
>> +case ERASE4_4K:
>>  case ERASE_32K:
>>  case ERASE_SECTOR:
>> +case ERASE4_SECTOR:
>>  case READ:
>> +case READ4:
>>  case DPP:
>>  case QPP:
>>  case PP:
>> -s->needed_bytes = 3;
>> +case PP4:
>> +s->needed_bytes = is_4bytes(s) ? 4 : 3;
>>  s->pos = 0;
>>  s->len = 0;
>>  s->state = STATE_COLLECTING_DATA;
>> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  s->len = 0;
>>  s->state = STATE_COLLECTING_DATA;
>>  break;
>> -
>> +case QIOR4:
>> +/* 4 address + 1 dummy */
>> +s->needed_bytes = 5;
>> +s->pos = 0;
>> +s->len = 0;
>> +s->state = STATE_COLLECTING_DATA;
>> +break;
>
> Blank line needed here. The blank is omitted between between logical
> groupings of commands (such as read-write pairs or different types of
> the same op) but there is no grouping between the data RW ops and
> WRSR.
>
> Regards,
> Peter
>
>>  case WRSR:
>>  if (s->write_enable) {
>>  s->needed_bytes = 1;
>> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>  VMSTATE_BOOL(four_bytes_address_mode, Flash),
>>  VMSTATE_UINT8(ear, Flash),
>>  VMSTATE_BOOL(reset_enable, Flash),
>> +VMSTATE_BOOL(quad_enable, Flash),
>>  VMSTATE_BOOL(initialized, Flash),
>> 

[Qemu-devel] [Bug 1528239] [NEW] Unable to debug PIE binaries with QEMU gdb stub.

[Maxim Ostapenko]

Public bug reported:

The issue occurs on current trunk:

max@max:~/build/qemu$ cat test.c
#include 

int main() {
  printf("Hello, world!\n");
  return 0;
}

max@max:~/build/qemu$ gcc test.c -fPIC -pie -o bad.x
max@max:~/build/qemu$ ./x86_64-linux-user/qemu-x86_64 -g 1234 bad.x 
.


max@max:~/build/qemu$ gdb
GNU gdb (Ubuntu 7.7.1-0ubuntu5~14.04.2) 7.7.1

(gdb) file bad.x
Reading symbols from bad.x...(no debugging symbols found)...done.
(gdb) b main
Breakpoint 1 at 0x779
(gdb) target remote localhost:1234
Remote debugging using localhost:1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...warning: the debug 
information found in "/lib64/ld-2.19.so" does not match 
"/lib64/ld-linux-x86-64.so.2" (CRC mismatch).

Reading symbols from /usr/lib/debug//lib/x86_64-linux-gnu/ld-2.19.so...done.
done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
Error in re-setting breakpoint 1: Cannot access memory at address 0x775
Error in re-setting breakpoint 1: Cannot access memory at address 0x775
0x004000a042d0 in _start () from /lib64/ld-linux-x86-64.so.2
(gdb) c
Continuing.
[Inferior 1 (Remote target) exited normally]
(gdb) 


max@max:~/build/qemu$ cat config.log
# Configured with: '/home/max/src/qemu/configure' 
'--prefix=/home/max/install/qemu' 
'--target-list=arm-linux-user,aarch64-linux-user,x86_64-linux-user' '--static'


W/O QEMU or -pie flag breakpoint on main works fine.

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  Unable to debug PIE binaries with QEMU gdb stub.

Status in QEMU:
  New

Bug description:
  The issue occurs on current trunk:

  max@max:~/build/qemu$ cat test.c
  #include 

  int main() {
printf("Hello, world!\n");
return 0;
  }

  max@max:~/build/qemu$ gcc test.c -fPIC -pie -o bad.x
  max@max:~/build/qemu$ ./x86_64-linux-user/qemu-x86_64 -g 1234 bad.x 
  .

  
  max@max:~/build/qemu$ gdb
  GNU gdb (Ubuntu 7.7.1-0ubuntu5~14.04.2) 7.7.1
  

  (gdb) file bad.x
  Reading symbols from bad.x...(no debugging symbols found)...done.
  (gdb) b main
  Breakpoint 1 at 0x779
  (gdb) target remote localhost:1234
  Remote debugging using localhost:1234
  Reading symbols from /lib64/ld-linux-x86-64.so.2...warning: the debug 
information found in "/lib64/ld-2.19.so" does not match 
"/lib64/ld-linux-x86-64.so.2" (CRC mismatch).

  Reading symbols from /usr/lib/debug//lib/x86_64-linux-gnu/ld-2.19.so...done.
  done.
  Loaded symbols for /lib64/ld-linux-x86-64.so.2
  Error in re-setting breakpoint 1: Cannot access memory at address 0x775
  Error in re-setting breakpoint 1: Cannot access memory at address 0x775
  0x004000a042d0 in _start () from /lib64/ld-linux-x86-64.so.2
  (gdb) c
  Continuing.
  [Inferior 1 (Remote target) exited normally]
  (gdb) 

  
  max@max:~/build/qemu$ cat config.log
  # Configured with: '/home/max/src/qemu/configure' 
'--prefix=/home/max/install/qemu' 
'--target-list=arm-linux-user,aarch64-linux-user,x86_64-linux-user' '--static'

  
  W/O QEMU or -pie flag breakpoint on main works fine.

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

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/21/2015 06:08 PM, Marcel Apfelbaum wrote:

On 12/21/2015 04:59 AM, Cao jin wrote:




[...]


Another question: because some of this series is CCed to
qemu-trivial(which means: reviewed-by?) by other maintainer, so next
time, do I need to send the whole series with "v2", or the rest?


Hi,

Since the patches are not related, the ones cc-ed to qemu-trivial will
be taken by the maintainer of trivial patches,
for the rest you should prepare a V2 to be reviewed by the corresponding
sub-tree maintainer.

CC to qemu-trivial does not mean "reviewed-by", it just implies the
patch is simple enough to go through the trivial tree and does not need
to go through the sub-tree maintainer.



Got it, thanks:)


Thanks,
Marcel




[...]

.



--
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 02/12] Added reset-pin emulation in model.

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
> From: Marcin Krzeminski 
>
> Signed-off-by: Marcin Krzeminski 
> ---
>  hw/block/m25p80.c | 38 +-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index bfd493f..bcb66a5 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -258,6 +258,8 @@ typedef struct Flash {
>  uint8_t cmd_in_progress;
>  uint64_t cur_addr;
>  bool write_enable;
> +bool initialized;
> +uint8_t reset_pin;
>
>  int64_t dirty_page;
>
> @@ -430,6 +432,19 @@ static void complete_collecting_data(Flash *s)
>  }
>  }
>
> +static void reset_memory(Flash *s)
> +{
> +s->cmd_in_progress = NOP;
> +s->cur_addr = 0;
> +s->len = 0;
> +s->needed_bytes = 0;
> +s->pos = 0;
> +s->state = STATE_IDLE;
> +s->write_enable = false;
> +
> +DB_PRINT_L(0, "Reset done.\n");
> +}
> +
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>  s->cmd_in_progress = value;
> @@ -620,6 +635,8 @@ static int m25p80_init(SSISlave *ss)
>  s->size = s->pi->sector_size * s->pi->n_sectors;
>  s->dirty_page = -1;
>
> +s->initialized = false;
> +

Init functions should never set runtime state.

>  /* FIXME use a qdev drive property instead of drive_get_next() */
>  dinfo = drive_get_next(IF_MTD);
>
> @@ -650,9 +667,24 @@ static void m25p80_pre_save(void *opaque)
>  flash_sync_dirty((Flash *)opaque, -1);
>  }
>
> +static void m25p80_reset(DeviceState *d)
> +{
> +Flash *s = M25P80(d);
> +
> +if (s->reset_pin || !s->initialized) {
> +reset_memory(s);
> +s->initialized = true;

Bus I think the real issue here is that is trying to model something
other than a power-on-reset. Looks like a a soft reset. the dc->reset
should be a power-on-reset and not be conditional on anything. Only
the non-volatile state (the storage data) should persist.

Is your board using qemu_system_reset_request() by any chance for
something that is not supposed to reset the whole board? If this is
the case, you need to limit the scope of that reset logic and just set
your board up to not use the centralized all-devices reset (which is
pretty broken for these modern boards/SoCs that have multiple reset
domains).

Regards,
Peter

> +}
> +}
> +
> +static Property m25p80_properties[] = {
> +DEFINE_PROP_UINT8("reset-pin", Flash, reset_pin, 0),
> +DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static const VMStateDescription vmstate_m25p80 = {
>  .name = "xilinx_spi",
> -.version_id = 1,
> +.version_id = 2,
>  .minimum_version_id = 1,
>  .pre_save = m25p80_pre_save,
>  .fields = (VMStateField[]) {
> @@ -664,6 +696,8 @@ static const VMStateDescription vmstate_m25p80 = {
>  VMSTATE_UINT8(cmd_in_progress, Flash),
>  VMSTATE_UINT64(cur_addr, Flash),
>  VMSTATE_BOOL(write_enable, Flash),
> +VMSTATE_BOOL(initialized, Flash),
> +VMSTATE_UINT8(reset_pin, Flash),
>  VMSTATE_END_OF_LIST()
>  }
>  };
> @@ -679,6 +713,8 @@ static void m25p80_class_init(ObjectClass *klass, void 
> *data)
>  k->set_cs = m25p80_cs;
>  k->cs_polarity = SSI_CS_LOW;
>  dc->vmsd = _m25p80;
> +dc->reset = _reset;
> +dc->props = m25p80_properties;
>  mc->pi = data;
>  }
>
> --
> 2.5.0
>
>

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

[Pavel Fedin]

 Hello!

> Yes, we can use  KVM_EXIT_REG_IO/MSR_IO for Hyper-V SynIC MSRS's changes
> and can even use only one MSR value . So union inside struct
> kvm_hyperv_exit is excessive.
> 
> But we still need Vcpu exit to handle VMBus hypercalls by QEMU to
> emulate VMBus devices inside QEMU.
> 
> And currently we are going to extend struct kvm_hyperv_exit
> to store Hyper-V VMBus hypercall parameters.

 Hm... Hypercalls, you say?
 We already have KVM_EXIT_HYPERCALL. Documentation says it's currently unused. 
Is it a leftover from ia64 KVM? Could we reuse it for
the purpose?

> but could we replace Hyper-V VMBus hypercall and it's parameters
> by KVM_EXIT_REG_IO/MSR_IO too?

 It depends. Can i read about these hypercalls somewhere? Is there any 
documentation?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

[Qemu-devel] ODP: [PATCH 05/12] Added support for serial eeproms - AT25128A/AT25256A

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:28, Peter Crosthwaite pisze:
> Subsystem prefix needed in commit subject (block: m25p80). Bring the
> mention of specific parts to the paragraph to shorten the subject.
I accidentally removed word m25p80 from all patches subject.
Sorry.
>
>
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index fbbfd1d..1a547ae 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -94,6 +94,11 @@ static const FlashPartInfo known_devices[] = {
>>
>>  { INFO("at45db081d",  0x1f2500,  0,  64 << 10,  16, ER_4K) },
>>
>> +/* Atmel EEPROMS - it is assumed, that don't care bit in command */
>
> Multi-line comment should not stop and start again (no */).
Ok.
Thanks,
Marcin
>
>
> Otherwise,
>
> Reviewed-by: Peter Crosthwaite 
>
> Regards,
> Peter
>
>> +/* is set to 0. Block protection is not supported */
>> +{ INFO("at25128a-nonjedec", 0x0, 0, 1, 131072, WR_1) },
>> +{ INFO("at25256a-nonjedec", 0x0, 0, 1, 262144, WR_1) },
>> +
>>  /* EON -- en25xxx */
>>  { INFO("en25f32", 0x1c3116,  0,  64 << 10,  64, ER_4K) },
>>  { INFO("en25p32", 0x1c2016,  0,  64 << 10,  64, 0) },
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [PATCH v4 5/5] kvm/x86: Hyper-V kvm exit

[Andrey Smetanin]

On 12/21/2015 04:28 PM, Pavel Fedin wrote:

  Hello!


Yes, we can use  KVM_EXIT_REG_IO/MSR_IO for Hyper-V SynIC MSRS's changes
and can even use only one MSR value . So union inside struct
kvm_hyperv_exit is excessive.

But we still need Vcpu exit to handle VMBus hypercalls by QEMU to
emulate VMBus devices inside QEMU.

And currently we are going to extend struct kvm_hyperv_exit
to store Hyper-V VMBus hypercall parameters.


  Hm... Hypercalls, you say?
  We already have KVM_EXIT_HYPERCALL. Documentation says it's currently unused. 
Is it a leftover from ia64 KVM? Could we reuse it for
the purpose?


but could we replace Hyper-V VMBus hypercall and it's parameters
by KVM_EXIT_REG_IO/MSR_IO too?


  It depends. Can i read about these hypercalls somewhere? Is there any 
documentation?
I don't know about a documentation, but you can look at the code of 
Hyper-V hypercall handling inside KVM:


https://github.com/torvalds/linux/blob/master/arch/x86/kvm/hyperv.c#L346

The code simply decodes hypercall parameters from vcpu registers then 
handle hypercall code in switch and encode return code inside vcpu 
registers. Probably encode and decode of hypercall parameters/return 
code can be done in QEMU so we need only some exit with parameter that 
this is Hyper-V hypercall and probably KVM_EXIT_HYPERCALL is good for it.


But KVM_EXIT_HYPERCALL is not used inside KVM/QEMU so requires
implementation.



Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

[Qemu-devel] ODP: [PATCH 08/12] Support for N25Q256A/N25Q512A

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 11:29, Peter Crosthwaite pisze:
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index f0f637e..a41c2f1 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -214,6 +214,8 @@ static const FlashPartInfo known_devices[] = {
>>
>>  /* Numonyx -- n25q128 */
>
> Comment needs update to qXXX or just drop the part name altogether for:
>
> /* Numonyx */
Will be dropped.
Thanks,
Marcin
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite 
>
> Regards,
> Peter
>
>>  { INFO("n25q128",  0x20ba18,  0,  64 << 10, 256, 0) },
>> +{ INFO("n25q256a", 0x20ba19,  0,  64 << 10, 512, ER_4K) },
>> +{ INFO("n25q512a", 0x20ba20,  0,  64 << 10, 1024, ER_4K) },
>>  };
>>
>>  typedef enum {
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] ODP: [PATCH 06/12] 4byte address mode support added.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:35, Peter Crosthwaite pisze:
> Commit subject subsystem prefix (block: m25p80:) needed.
>
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 31 ---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 1a547ae..6d5d90d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -237,6 +237,9 @@ typedef enum {
>>  ERASE_32K = 0x52,
>>  ERASE_SECTOR = 0xd8,
>>
>> +EN_4BYTE_ADDR = 0xB7,
>> +EX_4BYTE_ADDR = 0xE9,
>> +
>>  RESET_ENABLE = 0x66,
>>  RESET_MEMORY = 0x99,
>>
>> @@ -267,6 +270,7 @@ typedef struct Flash {
>>  uint8_t cmd_in_progress;
>>  uint64_t cur_addr;
>>  bool write_enable;
>> +bool four_bytes_address_mode;
>>  bool reset_enable;
>>  bool initialized;
>>  uint8_t reset_pin;
>> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t 
>> data)
>>  s->dirty_page = page;
>>  }
>>
>> +static inline int is_4bytes(Flash *s)
>> +{
>> +   return s->four_bytes_address_mode;
>> +   }
>> +}
>> +
>>  static void complete_collecting_data(Flash *s)
>>  {
>> -s->cur_addr = s->data[0] << 16;
>> -s->cur_addr |= s->data[1] << 8;
>> -s->cur_addr |= s->data[2];
>> +if (is_4bytes(s)) {
>> +s->cur_addr = s->data[0] << 24;
>> +s->cur_addr |= s->data[1] << 16;
>> +s->cur_addr |= s->data[2] << 8;
>> +s->cur_addr |= s->data[3];
>> +} else {
>> +s->cur_addr = s->data[0] << 16;
>> +s->cur_addr |= s->data[1] << 8;
>> +s->cur_addr |= s->data[2];
>> +}
>
> Avoid the duped code with a loop:
>
> s->cur_addr = 0;
> for (i = 0; i < (is_4bytes(s) ? 4 : 3); i++) {
> s->cur_addr <<= 8;
> s->cur_addr |= s->data[i];
> }
Ok.
Thanks,
Marcin
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite 
>
> Regards,
> Peter
>
>>
>>  s->state = STATE_IDLE;
>>
>> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s)
>>  {
>>  s->cmd_in_progress = NOP;
>>  s->cur_addr = 0;
>> +s->four_bytes_address_mode = false;
>>  s->len = 0;
>>  s->needed_bytes = 0;
>>  s->pos = 0;
>> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  break;
>>  case NOP:
>>  break;
>> +case EN_4BYTE_ADDR:
>> +s->four_bytes_address_mode = true;
>> +break;
>> +case EX_4BYTE_ADDR:
>> +s->four_bytes_address_mode = false;
>> +break;
>>  case RESET_ENABLE:
>>  s->reset_enable = true;
>>  break;
>> @@ -715,6 +739,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>  VMSTATE_UINT8(cmd_in_progress, Flash),
>>  VMSTATE_UINT64(cur_addr, Flash),
>>  VMSTATE_BOOL(write_enable, Flash),
>> +VMSTATE_BOOL(four_bytes_address_mode, Flash),
>>  VMSTATE_BOOL(reset_enable, Flash),
>>  VMSTATE_BOOL(initialized, Flash),
>>  VMSTATE_UINT8(reset_pin, Flash),
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [PATCH 1/1] qga: guest-set-user-password - added ability to create new user

[Eric Blake]

On 12/20/2015 11:40 PM, Denis V. Lunev wrote:
> From: Yuri Pudgorodskiy 
> 
> Added optional 'create' flag to guest-set-user-password command.
> When it is specified, a new user will be created if it is not
> exists yet.

s/is not exists/does not exist/

> 
> The option to the existing command is added as password for newly created
> user should be set as specified.
> 
> Signed-off-by: Yuri Pudgorodskiy 
> Signed-off-by: Denis V. Lunev 
> CC: Michael Roth 
> ---
>  qga/commands-posix.c | 36 
>  qga/commands-win32.c | 25 -
>  qga/qapi-schema.json |  3 ++-
>  3 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c

> @@ -1993,6 +1995,40 @@ void qmp_guest_set_user_password(const char *username,
>  goto out;
>  }
>  
> +/* create new user if requested */
> +if (has_create && create) {
> +pid = fork();
> +if (pid == 0) {
> +char *str = g_shell_quote(username);
> +char *cmd = g_strdup_printf("id %s || useradd -m %s", str, str);

useradd is Linux-specific; should we be trying harder to make this
command portable to all POSIX-y guests?

> +setsid();
> +reopen_fd_to_null(0);
> +reopen_fd_to_null(1);
> +reopen_fd_to_null(2);
> +execle("/bin/sh", "sh", "-c", cmd, NULL, environ);

By redirecting stderr to /dev/null, you've lost any error messages that
useradd tries to report...

> +_exit(EXIT_FAILURE);
> +} else if (pid < 0) {
> +error_setg_errno(errp, errno, "failed to create child process");
> +goto out;
> +}
> +
> +ga_wait_child(pid, , _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
> +
> +if (!WIFEXITED(status)) {
> +error_setg(errp, "child process has terminated abnormally");
> +goto out;
> +}
> +
> +if (WEXITSTATUS(status)) {
> +error_setg(errp, "child process has failed to add new user");

...and replaced it with a less-helpful message. Should you try harder to
pass through the real reason for failure?

> +++ b/qga/qapi-schema.json
> @@ -787,6 +787,7 @@
>  # @username: the user account whose password to change
>  # @password: the new password entry string, base64 encoded
>  # @crypted: true if password is already crypt()d, false if raw
> +# @create: #optional user will be created if not exists (since 2.6)

s/if not exists/if it does not exist/

may want to mention that it defaults to false

>  #
>  # If the @crypted flag is true, it is the caller's responsibility
>  # to ensure the correct crypt() encryption scheme is used. This
> @@ -806,7 +807,7 @@
>  # Since 2.3
>  ##
>  { 'command': 'guest-set-user-password',
> -  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
> +  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool', 
> '*create': 'bool' } }

Long line; please wrap to keep things under 80 columns.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] ODP: [PATCH 12/12] Read flag status register command support added.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:54, Peter Crosthwaite pisze:
> Subsystem prefix needed and reverse commit subject to start with verb:
>
> "block: m25p80: Add Read Flag Status command"
>
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite 
>
> Thanks to you and Pawel for this work.
Thanks for review and sorry for many technical issues in patches.

Thanks,
Marcin
>
>
> Regards,
> Peter
>
>> ---
>>  hw/block/m25p80.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index fadd6ec..ef05ad3 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -258,6 +258,7 @@ typedef enum {
>>  WREN = 0x6,
>>  JEDEC_READ = 0x9f,
>>  BULK_ERASE = 0xc7,
>> +READ_FSR = 0x70,
>>
>>  READ = 0x3,
>>  READ4 = 0x13,
>> @@ -667,6 +668,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  s->state = STATE_READING_DATA;
>>  break;
>>
>> +case READ_FSR:
>> +s->data[0] = (1<<7); /*Indicates flash is ready */
>> +s->pos = 0;
>> +s->len = 1;
>> +s->state = STATE_READING_DATA;
>> +break;
>> +
>>  case JEDEC_READ:
>>  DB_PRINT_L(0, "populated jedec code\n");
>>  for (i = 0; i < s->pi->id_len; i++) {
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [Xen-devel] new barrier type for paravirt (was Re: [PATCH] virtio_ring: use smp_store_mb)

[Stefano Stabellini]

On Mon, 21 Dec 2015, David Vrabel wrote:
> On 20/12/15 09:25, Michael S. Tsirkin wrote:
> > 
> > I noticed that drivers/xen/xenbus/xenbus_comms.c uses
> > full memory barriers to communicate with the other side.
> > For example:
> > 
> > /* Must write data /after/ reading the consumer index.  * */
> > mb();
> > 
> > memcpy(dst, data, avail);
> > data += avail;
> > len -= avail;
> > 
> > /* Other side must not see new producer until data is * 
> > there. */
> > wmb();
> > intf->req_prod += avail;
> > 
> > /* Implies mb(): other side will see the updated producer. 
> > */
> > notify_remote_via_evtchn(xen_store_evtchn);
> > 
> > To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
> > would be sufficient, so mb() and wmb() here are only needed if
> > a non-SMP guest runs on an SMP host.
> > 
> > Is my analysis correct?
> 
> For x86, yes.
> 
> For arm/arm64 I think so, but would prefer one of the Xen arm
> maintainers to confirm.  In particular, whether inner-shareable barriers
> are sufficient for memory shared with the hypervisor.

inner-shareable barriers are certainly OK. In this case there would be
also a switch from dsb to dmb barriers, which I also think should be OK.

What about all the mb() and wmb() in RING_PUSH_REQUESTS and
RING_PUSH_RESPONSES in include/xen/interface/io/ring.h ?


> > So what I'm suggesting is something like the below patch,
> > except instead of using virtio directly, a new set of barriers
> > that behaves identically for SMP and non-SMP guests will be introduced.
> > 
> > And of course the weak barriers flag is not needed for Xen -
> > that's a virtio only thing.
> > 
> > For example:
> > 
> > smp_pv_wmb()
> > smp_pv_rmb()
> > smp_pv_mb()
> 
> The smp_ prefix doesn't make a lot of sense to me here since these
> barriers are going to be the same whether the kernel is SMP or not.

[Qemu-devel] ODP: [PATCH 03/12] Reset enable and reset memory commands support.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:18, Peter Crosthwaite pisze:
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 15 +++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index bcb66a5..5e07b57 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -231,6 +231,10 @@ typedef enum {
>>  ERASE_4K = 0x20,
>>  ERASE_32K = 0x52,
>>  ERASE_SECTOR = 0xd8,
>> +
>> +RESET_ENABLE = 0x66,
>> +RESET_MEMORY = 0x99,
>> +
>
> Extra blank not needed.
Ok.
>
>
>>  } FlashCMD;
>>
>>  typedef enum {
>> @@ -258,6 +262,7 @@ typedef struct Flash {
>>  uint8_t cmd_in_progress;
>>  uint64_t cur_addr;
>>  bool write_enable;
>> +bool reset_enable;
>>  bool initialized;
>>  uint8_t reset_pin;
>>
>> @@ -441,6 +446,7 @@ static void reset_memory(Flash *s)
>>  s->pos = 0;
>>  s->state = STATE_IDLE;
>>  s->write_enable = false;
>> +s->reset_enable = false;
>>
>>  DB_PRINT_L(0, "Reset done.\n");
>>  }
>> @@ -554,6 +560,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  break;
>>  case NOP:
>>  break;
>> +case RESET_ENABLE:
>> +s->reset_enable = true;
>> +break;
>> +case RESET_MEMORY:
>> +if (s->reset_enable) {
>> +reset_memory(s);
>> +}
>> +break;
>
> Reading an n25q256 datasheet, the RESET_ENABLE state is cancelled by
> commands other that RESET_ENABLE or by bouncing the CS. A way to
> implement this would be something like
>
> if (value != RESET_MEMORY) {
> s->reset_enable = false;
> }
>
> before the switch of value as well as clearing reset_enable in m25p80_cs.
>
> Regards,
> Peter
Agree.

Thanks,
Marcin
>
>>  default:
>>  qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>>  break;
>> @@ -696,6 +710,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>  VMSTATE_UINT8(cmd_in_progress, Flash),
>>  VMSTATE_UINT64(cur_addr, Flash),
>>  VMSTATE_BOOL(write_enable, Flash),
>> +VMSTATE_BOOL(reset_enable, Flash),
>>  VMSTATE_BOOL(initialized, Flash),
>>  VMSTATE_UINT8(reset_pin, Flash),
>>  VMSTATE_END_OF_LIST()
>> --
>> 2.5.0
>>
>>
>
>

[Qemu-devel] ODP: [PATCH 07/12] Added support for extend address mode commands.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:41, Peter Crosthwaite pisze:
> Subsystem prefix.
>
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Marcin Krzeminski 
>> ---
>>  hw/block/m25p80.c | 28 
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 6d5d90d..f0f637e 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -47,6 +47,9 @@
>>   */
>>  #define WR_1 0x100
>>
>> +/* 16 MiB max in 3 byte address mode */
>> +#define MAX_3BYTES_SIZE 0x100
>> +
>>  typedef struct FlashPartInfo {
>>  const char *part_name;
>>  /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
>> @@ -240,6 +243,9 @@ typedef enum {
>>  EN_4BYTE_ADDR = 0xB7,
>>  EX_4BYTE_ADDR = 0xE9,
>>
>> +EXTEND_ADDR_READ = 0xC8,
>> +EXTEND_ADDR_WRITE = 0xC5,
>> +
>>  RESET_ENABLE = 0x66,
>>  RESET_MEMORY = 0x99,
>>
>> @@ -274,6 +280,7 @@ typedef struct Flash {
>>  bool reset_enable;
>>  bool initialized;
>>  uint8_t reset_pin;
>> +uint8_t ear;
>>
>>  int64_t dirty_page;
>>
>> @@ -426,6 +433,8 @@ static void complete_collecting_data(Flash *s)
>>  s->cur_addr = s->data[0] << 16;
>>  s->cur_addr |= s->data[1] << 8;
>>  s->cur_addr |= s->data[2];
>> +
>> +s->cur_addr += (s->ear&0x3)*MAX_3BYTES_SIZE;
>
> The reset of the file puts spaces around all arithmetic operators so
> the same should be for the & and * here.
>
> Following on for the loop suggestion in prev. patch this would then be
> in its own if for !is_4byte after the loop.
Ok.
Thanks,
Marcin
>
> Otherwise:
>
> Reviewed-by: Peter Crosthwaite 
>
> Regards,
> Peter
>
>>  }
>>
>>  s->state = STATE_IDLE;
>> @@ -454,6 +463,9 @@ static void complete_collecting_data(Flash *s)
>>  s->write_enable = false;
>>  }
>>  break;
>> +case EXTEND_ADDR_WRITE:
>> +s->ear = s->data[0];
>> +break;
>>  default:
>>  break;
>>  }
>> @@ -463,6 +475,7 @@ static void reset_memory(Flash *s)
>>  {
>>  s->cmd_in_progress = NOP;
>>  s->cur_addr = 0;
>> +s->ear = 0;
>>  s->four_bytes_address_mode = false;
>>  s->len = 0;
>>  s->needed_bytes = 0;
>> @@ -589,6 +602,20 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  case EX_4BYTE_ADDR:
>>  s->four_bytes_address_mode = false;
>>  break;
>> +case EXTEND_ADDR_READ:
>> +s->data[0] = s->ear;
>> +s->pos = 0;
>> +s->len = 1;
>> +s->state = STATE_READING_DATA;
>> +break;
>> +case EXTEND_ADDR_WRITE:
>> +if (s->write_enable) {
>> +s->needed_bytes = 1;
>> +s->pos = 0;
>> +s->len = 0;
>> +s->state = STATE_COLLECTING_DATA;
>> +}
>> +break;
>>  case RESET_ENABLE:
>>  s->reset_enable = true;
>>  break;
>> @@ -740,6 +767,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>  VMSTATE_UINT64(cur_addr, Flash),
>>  VMSTATE_BOOL(write_enable, Flash),
>>  VMSTATE_BOOL(four_bytes_address_mode, Flash),
>> +VMSTATE_UINT8(ear, Flash),
>>  VMSTATE_BOOL(reset_enable, Flash),
>>  VMSTATE_BOOL(initialized, Flash),
>>  VMSTATE_UINT8(reset_pin, Flash),
>> --
>> 2.5.0
>>
>>
>
>

Re: [Qemu-devel] [PATCH 1/1] qga: guest-set-user-password - added ability to create new user

[Denis V. Lunev]

On 12/21/2015 05:00 PM, Eric Blake wrote:

On 12/20/2015 11:40 PM, Denis V. Lunev wrote:

From: Yuri Pudgorodskiy 

Added optional 'create' flag to guest-set-user-password command.
When it is specified, a new user will be created if it is not
exists yet.

s/is not exists/does not exist/


The option to the existing command is added as password for newly created
user should be set as specified.

Signed-off-by: Yuri Pudgorodskiy 
Signed-off-by: Denis V. Lunev 
CC: Michael Roth 
---
  qga/commands-posix.c | 36 
  qga/commands-win32.c | 25 -
  qga/qapi-schema.json |  3 ++-
  3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
@@ -1993,6 +1995,40 @@ void qmp_guest_set_user_password(const char *username,
  goto out;
  }
  
+/* create new user if requested */

+if (has_create && create) {
+pid = fork();
+if (pid == 0) {
+char *str = g_shell_quote(username);
+char *cmd = g_strdup_printf("id %s || useradd -m %s", str, str);

useradd is Linux-specific; should we be trying harder to make this
command portable to all POSIX-y guests?

this code is inside #if defined(__linux__)
as implemented by the original author.



+setsid();
+reopen_fd_to_null(0);
+reopen_fd_to_null(1);
+reopen_fd_to_null(2);
+execle("/bin/sh", "sh", "-c", cmd, NULL, environ);

By redirecting stderr to /dev/null, you've lost any error messages that
useradd tries to report...


+_exit(EXIT_FAILURE);
+} else if (pid < 0) {
+error_setg_errno(errp, errno, "failed to create child process");
+goto out;
+}
+
+ga_wait_child(pid, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+if (!WIFEXITED(status)) {
+error_setg(errp, "child process has terminated abnormally");
+goto out;
+}
+
+if (WEXITSTATUS(status)) {
+error_setg(errp, "child process has failed to add new user");

...and replaced it with a less-helpful message. Should you try harder to
pass through the real reason for failure?


+++ b/qga/qapi-schema.json
@@ -787,6 +787,7 @@
  # @username: the user account whose password to change
  # @password: the new password entry string, base64 encoded
  # @crypted: true if password is already crypt()d, false if raw
+# @create: #optional user will be created if not exists (since 2.6)

s/if not exists/if it does not exist/

may want to mention that it defaults to false


  #
  # If the @crypted flag is true, it is the caller's responsibility
  # to ensure the correct crypt() encryption scheme is used. This
@@ -806,7 +807,7 @@
  # Since 2.3
  ##
  { 'command': 'guest-set-user-password',
-  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
+  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool', 
'*create': 'bool' } }

Long line; please wrap to keep things under 80 columns.



the rest is clear. We will try.

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

[Xiao Guangrong]

On 12/10/2015 11:11 AM, Xiao Guangrong wrote:


New version, new week, and unfortunate new ping... :(


Ping again to see what happened...

[Qemu-devel] [Bug 1528214] [NEW] qemu 1.7.0 vhost_net crash

[wang liming]

Public bug reported:

i find the crash in  /var/crash 
the crash content is :
<4>Pid: 6949, comm: qemu-system-x86 Not tainted 2.6.32-431.el6.x86_64 #1 
Powerleader PR2530G2/SC612DI-8F
<4>RIP: 0010:[]  [] fput+0x9/0x30
<4>RSP: 0018:88015b601d98  EFLAGS: 00010292
<4>RAX: 0382 RBX: 881e4659 RCX: 01c3
<4>RDX:  RSI: 881e46590130 RDI: 
<4>RBP: 88015b601d98 R08: 881e46598518 R09: 
<4>R10:  R11: 0246 R12: 881e46590010
<4>R13:  R14: 880c29812748 R15: 
<4>FS:  7f6a74d20700() GS:8810b884() knlGS:
<4>CS:  0010 DS: 002b ES: 002b CR0: 8005003b
<4>CR2: 0030 CR3: 001c544cc000 CR4: 001427e0
<4>DR0:  DR1:  DR2: 
<4>DR3:  DR6: 0ff0 DR7: 0400
<4>Process qemu-system-x86 (pid: 6949, threadinfo 88015b60, task 
880c1ed9c040)
<4>Stack:
<4> 88015b601e58 a02ac3c8 881e4659 
<4> 881e46590080 881e46590078 88015b601e38 0286
<4>  0001 88015b601e58 0282
<4>Call Trace:
<4> [] vhost_net_ioctl+0x328/0x5d0 [vhost_net]
<4> [] vfs_ioctl+0x22/0xa0
<4> [] do_vfs_ioctl+0x84/0x580
<4> [] ? __fput+0x1a1/0x210
<4> [] sys_ioctl+0x81/0xa0
<4> [] ? __audit_syscall_exit+0x25e/0x290
<4> [] system_call_fastpath+0x16/0x1b
<4>Code: fe ff ff 31 d2 48 89 de 83 cf ff ff d0 e9 da fe ff ff 48 89 df e8 28 
64 04 00 e9 bb fe ff ff 0f 1f 00 55 48 89 e5 0f 1f 44 00 00  48 ff 4f 30 0f 
94 c0 84 c0 75 0b c9 c3 66 0f 1f
84 00 00 00 
<1>RIP  [] fput+0x9/0x30
<4> RSP 
<4>CR2: 0030

how the bug occure

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  qemu 1.7.0 vhost_net crash

Status in QEMU:
  New

Bug description:
  i find the crash in  /var/crash 
  the crash content is :
  <4>Pid: 6949, comm: qemu-system-x86 Not tainted 2.6.32-431.el6.x86_64 #1 
Powerleader PR2530G2/SC612DI-8F
  <4>RIP: 0010:[]  [] fput+0x9/0x30
  <4>RSP: 0018:88015b601d98  EFLAGS: 00010292
  <4>RAX: 0382 RBX: 881e4659 RCX: 01c3
  <4>RDX:  RSI: 881e46590130 RDI: 
  <4>RBP: 88015b601d98 R08: 881e46598518 R09: 
  <4>R10:  R11: 0246 R12: 881e46590010
  <4>R13:  R14: 880c29812748 R15: 
  <4>FS:  7f6a74d20700() GS:8810b884() 
knlGS:
  <4>CS:  0010 DS: 002b ES: 002b CR0: 8005003b
  <4>CR2: 0030 CR3: 001c544cc000 CR4: 001427e0
  <4>DR0:  DR1:  DR2: 
  <4>DR3:  DR6: 0ff0 DR7: 0400
  <4>Process qemu-system-x86 (pid: 6949, threadinfo 88015b60, task 
880c1ed9c040)
  <4>Stack:
  <4> 88015b601e58 a02ac3c8 881e4659 
  <4> 881e46590080 881e46590078 88015b601e38 0286
  <4>  0001 88015b601e58 0282
  <4>Call Trace:
  <4> [] vhost_net_ioctl+0x328/0x5d0 [vhost_net]
  <4> [] vfs_ioctl+0x22/0xa0
  <4> [] do_vfs_ioctl+0x84/0x580
  <4> [] ? __fput+0x1a1/0x210
  <4> [] sys_ioctl+0x81/0xa0
  <4> [] ? __audit_syscall_exit+0x25e/0x290
  <4> [] system_call_fastpath+0x16/0x1b
  <4>Code: fe ff ff 31 d2 48 89 de 83 cf ff ff d0 e9 da fe ff ff 48 89 df e8 28 
64 04 00 e9 bb fe ff ff 0f 1f 00 55 48 89 e5 0f 1f 44 00 00  48 ff 4f 30 0f 
94 c0 84 c0 75 0b c9 c3 66 0f 1f
  84 00 00 00 
  <1>RIP  [] fput+0x9/0x30
  <4> RSP 
  <4>CR2: 0030

  how the bug occure

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

[Qemu-devel] [Bug 1528214] Re: qemu 1.7.0 vhost_net crash

[wang liming]

my envionment is  centos6.5
and libvirt version is 1.2.14

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

Title:
  qemu 1.7.0 vhost_net crash

Status in QEMU:
  New

Bug description:
  i find the crash in  /var/crash 
  the crash content is :
  <4>Pid: 6949, comm: qemu-system-x86 Not tainted 2.6.32-431.el6.x86_64 #1 
Powerleader PR2530G2/SC612DI-8F
  <4>RIP: 0010:[]  [] fput+0x9/0x30
  <4>RSP: 0018:88015b601d98  EFLAGS: 00010292
  <4>RAX: 0382 RBX: 881e4659 RCX: 01c3
  <4>RDX:  RSI: 881e46590130 RDI: 
  <4>RBP: 88015b601d98 R08: 881e46598518 R09: 
  <4>R10:  R11: 0246 R12: 881e46590010
  <4>R13:  R14: 880c29812748 R15: 
  <4>FS:  7f6a74d20700() GS:8810b884() 
knlGS:
  <4>CS:  0010 DS: 002b ES: 002b CR0: 8005003b
  <4>CR2: 0030 CR3: 001c544cc000 CR4: 001427e0
  <4>DR0:  DR1:  DR2: 
  <4>DR3:  DR6: 0ff0 DR7: 0400
  <4>Process qemu-system-x86 (pid: 6949, threadinfo 88015b60, task 
880c1ed9c040)
  <4>Stack:
  <4> 88015b601e58 a02ac3c8 881e4659 
  <4> 881e46590080 881e46590078 88015b601e38 0286
  <4>  0001 88015b601e58 0282
  <4>Call Trace:
  <4> [] vhost_net_ioctl+0x328/0x5d0 [vhost_net]
  <4> [] vfs_ioctl+0x22/0xa0
  <4> [] do_vfs_ioctl+0x84/0x580
  <4> [] ? __fput+0x1a1/0x210
  <4> [] sys_ioctl+0x81/0xa0
  <4> [] ? __audit_syscall_exit+0x25e/0x290
  <4> [] system_call_fastpath+0x16/0x1b
  <4>Code: fe ff ff 31 d2 48 89 de 83 cf ff ff d0 e9 da fe ff ff 48 89 df e8 28 
64 04 00 e9 bb fe ff ff 0f 1f 00 55 48 89 e5 0f 1f 44 00 00  48 ff 4f 30 0f 
94 c0 84 c0 75 0b c9 c3 66 0f 1f
  84 00 00 00 
  <1>RIP  [] fput+0x9/0x30
  <4> RSP 
  <4>CR2: 0030

  how the bug occure

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

[Qemu-devel] ODP: [PATCH 09/12] Support for 6Bytes jdec.

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 11:47, Peter Crosthwaite pisze:
> You need to add more notes in commit messages about what you have
> changed. Specifically here, you need to say something about the
> arrayification of the Jedec ID. You are also correcting terminology by
> changing ext_jedec to ext_id which shuld be mentioned.
>
> On Wed, Dec 16, 2015 at 4:57 AM,   wrote:
>> From: Marcin Krzeminski 
>>
>> Signed-off-by: Pawel Lenkow 
>
> Who is the Author? The patch authorship is Marcin, but the signature
> is Pawel. The original author should be the author (the From:) as well
> as have the Signed-off-by. Extra people (who resend, queue or commit
> the patch) add their SOB. So if this is Pawel's code you need to do a
> git commit --amend --author with Pawel's name. But either way, as
> sender your own (Marcin) SoB should be here.
My understanding was that SOB is enough to identify an author.
In all this patches whee Pawel is SOB, he is also the author.
I will correct it.
Thanks,
Marcin
>
>
>> ---
>>  hw/block/m25p80.c | 61 
>> +--
>>  1 file changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index a41c2f1..6fc55a3 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -50,12 +50,17 @@
>>  /* 16 MiB max in 3 byte address mode */
>>  #define MAX_3BYTES_SIZE 0x100
>>
>> +#define SPI_NOR_MAX_ID_LEN6
>> +
>>  typedef struct FlashPartInfo {
>>  const char *part_name;
>> -/* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */
>> -uint32_t jedec;
>> -/* extended jedec code */
>> -uint16_t ext_jedec;
>> +/*
>> + * This array stores the ID bytes.
>
> Comment should be of form
>
> /* some text wrapping
>  * to multiple lines
>  */
>
> for consistency with surrounding code.
>
>> + * The first three bytes are the JEDIC ID.
>
> "JEDEC"
>
>> + * JEDEC ID zero means "no ID" (mostly older chips).
>> + */
>> +uint8_t id[SPI_NOR_MAX_ID_LEN];
>> +uint8_t id_len;
>
> This arrayification is a very nice change.
>
>>  /* there is confusion between manufacturers as to what a sector is. In 
>> this
>>   * device model, a "sector" is the size that is erased by the 
>> ERASE_SECTOR
>>   * command (opcode 0xd8).
>> @@ -67,11 +72,33 @@ typedef struct FlashPartInfo {
>>  } FlashPartInfo;
>>
>>  /* adapted from linux */
>> -
>
> Keep this blank.
>
>> -#define INFO(_part_name, _jedec, _ext_jedec, _sector_size, _n_sectors, 
>> _flags)\
>> -.part_name = (_part_name),\
>> -.jedec = (_jedec),\
>> -.ext_jedec = (_ext_jedec),\
>> +/* Used when the "_ext_id" is two bytes at most */
>> +#define INFO(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, 
>> _flags)\
>> +.part_name = _part_name,\
>> +.id = {\
>> +((_jedec_id) >> 16) & 0xff,\
>> +((_jedec_id) >> 8) & 0xff,\
>> +(_jedec_id) & 0xff,\
>> +((_ext_id) >> 8) & 0xff,\
>> +(_ext_id) & 0xff,\
>> +  },\
>> +.id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),\
>> +.sector_size = (_sector_size),\
>> +.n_sectors = (_n_sectors),\
>> +.page_size = 256,\
>> +.flags = (_flags),
>> +
>> +#define INFO6(_part_name, _jedec_id, _ext_id, _sector_size, _n_sectors, 
>> _flags)\
>
> I am not sure we need the extra macro. Can you just make the 6th byte
> an extra _flags flag and use the original INFO()? If you do need the
> extra macro, one macro should call the other, or both should call a a
> common macro (accepting _id_len as a macro arg) to avoid the dup of
> the majority of the macro body.
>
> Regards,
> Peter
>
>> +.part_name = _part_name,\
>> +.id = {\
>> +((_jedec_id) >> 16) & 0xff,\
>> +((_jedec_id) >> 8) & 0xff,\
>> +(_jedec_id) & 0xff,\
>> +((_ext_id) >> 16) & 0xff,\
>> +((_ext_id) >> 8) & 0xff,\
>> +(_ext_id) & 0xff,\
>> +  },\
>> +.id_len = 6,\
>>  .sector_size = (_sector_size),\
>>  .n_sectors = (_n_sectors),\
>>  .page_size = 256,\
>> @@ -519,7 +546,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  break;
>>
>>  case DIOR:
>> -switch ((s->pi->jedec >> 16) & 0xFF) {
>> +switch (s->pi->id[0]) {
>>  case JEDEC_WINBOND:
>>  case JEDEC_SPANSION:
>>  s->needed_bytes = 4;
>> @@ -534,7 +561,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>  break;
>>
>>  case QIOR:
>> -switch ((s->pi->jedec >> 16) & 0xFF) {
>> +switch (s->pi->id[0]) {
>>  case JEDEC_WINBOND:
>>  case JEDEC_SPANSION:
>>  s->needed_bytes = 6;
>> @@ -573,16 +600,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>
>>  case JEDEC_READ:
>>  DB_PRINT_L(0, "populated jedec code\n");
>> -s->data[0] = (s->pi->jedec >> 16) & 0xff;
>> -   

Re: [Qemu-devel] [PATCH v3 18/24] vmdk: Clean up "Invalid extent lines" error message

[Fam Zheng]

On Fri, 12/18 16:35, Markus Armbruster wrote:
> vmdk_parse_extents() reports parse errors like this:
> 
> error_setg(errp, "Invalid extent lines:\n%s", p);
> 
> where p points to the beginning of the malformed line in the image
> descriptor.  This results in a multi-line error message
> 
> Invalid extent lines:
> 
> 
> 
> Error messages should not have newlines embedded.  Since the remaining
> text is not helpful, we can simply report:
> 
> Invalid extent line: 
> 
> Cc: Fam Zheng 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
> ---
>  block/vmdk.c   | 20 +---
>  tests/qemu-iotests/059.out |  4 +---
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 08fa3f3..2b5cb00 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -780,7 +780,7 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>  char access[11];
>  char type[11];
>  char fname[512];
> -const char *p;
> +const char *p, *np;
>  int64_t sectors = 0;
>  int64_t flat_offset;
>  char *extent_path;
> @@ -805,19 +805,16 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>  continue;
>  } else if (!strcmp(type, "FLAT")) {
>  if (matches != 5 || flat_offset < 0) {
> -error_setg(errp, "Invalid extent lines: \n%s", p);
> -return -EINVAL;
> +goto invalid;
>  }
>  } else if (!strcmp(type, "VMFS")) {
>  if (matches == 4) {
>  flat_offset = 0;
>  } else {
> -error_setg(errp, "Invalid extent lines:\n%s", p);
> -return -EINVAL;
> +goto invalid;
>  }
>  } else if (matches != 4) {
> -error_setg(errp, "Invalid extent lines:\n%s", p);
> -return -EINVAL;
> +goto invalid;
>  }
>  
>  if (sectors <= 0 ||
> @@ -883,6 +880,15 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>  extent->type = g_strdup(type);
>  }
>  return 0;
> +
> +invalid:
> +np = next_line(p);
> +assert(np != p);
> +if (np[-1] == '\n') {
> +np--;
> +}
> +error_setg(errp, "Invalid extent line: %.*s", (int)(np - p), p);
> +return -EINVAL;
>  }
>  
>  static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index d28df5b..9d506cb 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2038,9 +2038,7 @@ Format specific information:
>  format: FLAT
>  
>  === Testing malformed VMFS extent description line ===
> -qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent lines:
> -RW 12582912 VMFS "dummy.IMGFMT" 1
> -
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 
> 12582912 VMFS "dummy.IMGFMT" 1
>  
>  === Testing truncated sparse ===
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=107374182400 
> subformat=monolithicSparse
> -- 
> 2.4.3
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v2] linux-user/mmap.c: Always zero MAP_ANONYMOUS memory inmmap_frag()

[Chen Gang]

> From:  "Laurent Vivier";;
> 
> Le 21/12/2015 03:33, cheng...@emindsoft.com.cn a écrit :
>> From: Chen Gang 
>>
>> When mapping MAP_ANONYMOUS memory fragments, still need notice about to
>> set it zero, or it will cause issues.
> 
> Perhaps you can explain in the commit message why this page is not
> already filled by zeros ?
> 

In fact, I don't know. when I debug related issues under sw_64 host (
almost the same as alpha) for i386 target, I found it. The host is 8KB
page, but the guest is 4KB page. So I guess:

 - Firstly, qemu allocate one !MAP_ANONYMOUS 8KB page, so it is not
   zeroed.

 - Then qemu want one MAP_ANONYMOUS fragment, just can use the the left
   room of the page above. It merges and sets the related prot, but
   forgot to reset zero for MAP_ANONYMOUS fragment.

 - For normal host 4KB and guest also 4KB page, it is rarely happen (
   may never happen, I guess). But for host 8KB and guest 4KB pages, it
   often occurs.

Our qemu, at present, let i386 use 8KB page by force, but it can not
work. Our qemu members told me to use softmmu, or we would meet many
various issues.  But at present, I do not meet softmmu related issues:

 - I can chroot with binfmt_misc successfully by using sw_64 host i386
   static qemu, and most i386 programs (e.g. gcc, vi, xclac...) are OK
   (I build i386 wine under sw_64 host i386 chroot environments).

 - After fix this issue, some wine programs can run successfully, e.g.
   cmd.exe, clock.exe, winver.exe (they almost like windows own exe),
   but initialization is very slow in qemu mmap_find_vma_reserved().

 - Now, I am just analyzing and fixing the issue about notepad.exe, is
   it related with softmmu? I am not sure.

When I really meet softmmu related issue, I have to solve it, although
it may spend much time resources.


>> Signed-off-by: Chen Gang 
>> ---
>>  linux-user/mmap.c |4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index 7b459d5..29fe646 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -186,10 +186,12 @@ static int mmap_frag(abi_ulong real_start,
>>  if (prot_new != (prot1 | PROT_WRITE))
>>  mprotect(host_start, qemu_host_page_size, prot_new);
>>  } else {
>> -/* just update the protection */
>>  if (prot_new != prot1) {
>>  mprotect(host_start, qemu_host_page_size, prot_new);
>>  }
>> +if ((prot_new & PROT_WRITE) && ((flags & MAP_PRIVATE) || (fd == 
>> -1))) {
> 
> According to manpage, for MAP_ANONYMOUS, fd can be ignored.
> Why do you check if the page is MAP_PRIVATE or not ?
> 

For me, we can remove them, originally I only worry about MAP_SHARED and
fd according to the man page (it mentions MAP_SHARED and fd):

  MAP_ANONYMOUS
The mapping is not backed by any file; its contents are initialized
to zero. The fd and offset arguments are ignored; however, some
implementations require fd to be -1  if MAP_ANONYMOUS (or MAP_ANON)
is specified, and portable applications should ensure this.  The use
of MAP_ANONYMOUS in conjunction with MAP_SHARED is supported on
Linux only since kernel 2.4.


Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH v3 2/4] target-tilegx: Add single floating point implementation

[Richard Henderson]

On 12/21/2015 10:54 AM, Chen Gang wrote:

The both do, in that you re-normalize to produce that HBIT.
That's the whole point.



Oh, yes.

But all together, we want to normalize the float value in fsingle_pack2,
so we can not use float64_to_float32()...


Of course not.  I told you that you couldn't.


r~

[Qemu-devel] [PATCH v2] xen-pvdevice: convert to realize()

[Cao jin]

Signed-off-by: Cao jin 
---
 hw/i386/xen/xen_pvdevice.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
index c218947..9abcf25 100644
--- a/hw/i386/xen/xen_pvdevice.c
+++ b/hw/i386/xen/xen_pvdevice.c
@@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int xen_pv_init(PCIDevice *pci_dev)
+static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
 {
 XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
 uint8_t *pci_conf;
 
 /* device-id property must always be supplied */
-if (d->device_id == 0x)
-   return -1;
+if (d->device_id == 0x) {
+error_setg(errp, "Device ID invalid, it must always be supplied");
+return;
+}
 
 pci_conf = pci_dev->config;
 
@@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
 
 pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
  >mmio);
-
-return 0;
 }
 
 static Property xen_pv_props[] = {
@@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->init = xen_pv_init;
+k->realize = xen_pv_realize;
 k->class_id = PCI_CLASS_SYSTEM_OTHER;
 dc->desc = "Xen PV Device";
 dc->props = xen_pv_props;
-- 
2.1.0

[Qemu-devel] [PATCH v2 08/11] iotests: 038: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/038 | 5 -
 tests/qemu-iotests/038.out | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/038 b/tests/qemu-iotests/038
index cfaf00a..34fe698 100755
--- a/tests/qemu-iotests/038
+++ b/tests/qemu-iotests/038
@@ -48,6 +48,9 @@ size=128M
 echo
 echo "== creating backing file for COW tests =="
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
+
 _make_test_img $size
 
 function backing_io()
@@ -68,7 +71,7 @@ function backing_io()
 
 backing_io 0 256 write | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 
-mv "$TEST_IMG" "$TEST_IMG.base"
+TEST_IMG="$TEST_IMG_SAVE"
 
 _make_test_img -b "$TEST_IMG.base" 6G
 
diff --git a/tests/qemu-iotests/038.out b/tests/qemu-iotests/038.out
index ecb656e..0bdfb19 100644
--- a/tests/qemu-iotests/038.out
+++ b/tests/qemu-iotests/038.out
@@ -1,7 +1,7 @@
 QA output created by 038
 
 == creating backing file for COW tests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 65536
-- 
2.4.3

[Qemu-devel] [PATCH v2 06/11] iotests: 034: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/034 | 6 +-
 tests/qemu-iotests/034.out | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/034 b/tests/qemu-iotests/034
index 69c7858..c769dd8 100755
--- a/tests/qemu-iotests/034
+++ b/tests/qemu-iotests/034
@@ -51,9 +51,13 @@ size=128M
 echo
 echo "== creating backing file for COW tests =="
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
+
 _make_test_img $size
 $QEMU_IO -c "write -P 0x55 0 1M" "$TEST_IMG" | _filter_qemu_io
-mv "$TEST_IMG" "$TEST_IMG.base"
+
+TEST_IMG="$TEST_IMG_SAVE"
 
 _make_test_img -b "$TEST_IMG.base" 6G
 
diff --git a/tests/qemu-iotests/034.out b/tests/qemu-iotests/034.out
index 34fda80..0764ead 100644
--- a/tests/qemu-iotests/034.out
+++ b/tests/qemu-iotests/034.out
@@ -1,7 +1,7 @@
 QA output created by 034
 
 == creating backing file for COW tests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
 wrote 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944 
backing_file=TEST_DIR/t.IMGFMT.base
-- 
2.4.3

[Qemu-devel] [PATCH v2 04/11] iotests: 024: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/024 | 10 --
 tests/qemu-iotests/024.out |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 9bf99e1..2c2d148 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -60,16 +60,22 @@ CLUSTER_SIZE=65536
 echo "Creating backing file"
 echo
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base_old"
+
 _make_test_img 1G
 io_pattern writev 0 $CLUSTER_SIZE $((2 * CLUSTER_SIZE)) 8 0x11
-mv "$TEST_IMG" "$TEST_IMG.base_old"
+
+TEST_IMG="$TEST_IMG_SAVE.base_new"
 
 echo "Creating new backing file"
 echo
 
 _make_test_img 1G
 io_pattern writev 0 $((2 * CLUSTER_SIZE)) $((4 * CLUSTER_SIZE)) 4 0x22
-mv "$TEST_IMG" "$TEST_IMG.base_new"
+
+
+TEST_IMG="$TEST_IMG_SAVE"
 
 echo "Creating COW image"
 echo
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index 9b9ef3a..33cfaf5 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -1,7 +1,7 @@
 QA output created by 024
 Creating backing file
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+Formatting 'TEST_DIR/t.IMGFMT.base_old', fmt=IMGFMT size=1073741824
 === IO: pattern 0x11
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -21,7 +21,7 @@ wrote 65536/65536 bytes at offset 917504
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Creating new backing file
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+Formatting 'TEST_DIR/t.IMGFMT.base_new', fmt=IMGFMT size=1073741824
 === IO: pattern 0x22
 wrote 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.4.3

[Qemu-devel] [PATCH v2 03/11] iotests: 020: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/020 | 7 +--
 tests/qemu-iotests/020.out | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index 2f258dc..6625b55 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -56,6 +56,9 @@ fi
 
 TEST_OFFSETS="0 4294967296"
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
+
 _make_test_img 6G
 
 echo "Filling base image"
@@ -73,7 +76,7 @@ _check_test_img
 echo "Creating test image with backing file"
 echo
 
-mv "$TEST_IMG" "$TEST_IMG.base"
+TEST_IMG="$TEST_IMG_SAVE"
 _make_test_img -b "$TEST_IMG.base" 6G
 
 echo "Filling test image"
@@ -89,7 +92,7 @@ done
 _check_test_img
 
 $QEMU_IMG commit "$TEST_IMG"
-mv "$TEST_IMG.base" "$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
 
 echo "Reading from the backing file"
 echo
diff --git a/tests/qemu-iotests/020.out b/tests/qemu-iotests/020.out
index 134aa29..42f6c1b 100644
--- a/tests/qemu-iotests/020.out
+++ b/tests/qemu-iotests/020.out
@@ -1,5 +1,5 @@
 QA output created by 020
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=6442450944
 Filling base image
 
 === IO: pattern 0
-- 
2.4.3

[Qemu-devel] [PATCH v2 01/11] iotests: 018: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/018 | 8 
 tests/qemu-iotests/018.out | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/018 b/tests/qemu-iotests/018
index d8a7d43..07b2de9 100755
--- a/tests/qemu-iotests/018
+++ b/tests/qemu-iotests/018
@@ -66,8 +66,8 @@ _check_test_img
 echo "Creating test image with backing file"
 echo
 
-TEST_IMG=$TEST_IMG_SAVE
-_make_test_img -b "$TEST_IMG.base" 6G
+TEST_IMG="$TEST_IMG_SAVE.orig"
+_make_test_img -b "$TEST_IMG_SAVE.base" 6G
 
 echo "Filling test image"
 echo
@@ -81,8 +81,8 @@ for offset in $TEST_OFFSETS; do
 done
 _check_test_img
 
-mv "$TEST_IMG" "$TEST_IMG.orig"
-$QEMU_IMG convert -O $IMGFMT "$TEST_IMG.orig" "$TEST_IMG"
+TEST_IMG="$TEST_IMG_SAVE"
+$QEMU_IMG convert -f $IMGFMT -O $IMGFMT "$TEST_IMG.orig" "$TEST_IMG"
 
 echo "Reading"
 echo
diff --git a/tests/qemu-iotests/018.out b/tests/qemu-iotests/018.out
index d66bd63..5df9667 100644
--- a/tests/qemu-iotests/018.out
+++ b/tests/qemu-iotests/018.out
@@ -269,7 +269,7 @@ wrote 65536/65536 bytes at offset 4295032832
 No errors were found on the image.
 Creating test image with backing file
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944 
backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=6442450944 
backing_file=TEST_DIR/t.IMGFMT.base
 Filling test image
 
 === IO: pattern 1
-- 
2.4.3

Re: [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform

[Xulei (Stone)]

Hi, Kevin,
Can you tell how to reset/reboot this VM, if it goes to the handle_hwpic1()
on its booting procedure? I mean, usually, SeaBIOS would not go to 
handle_hwpic routine. But in my test case, SeaBIOS calls handle_hwpic when
KVM injects a #UD expcetion (not irq) and  SeaBIOS will loop to handle this
if KVM persistently injects exception.
 
Now, i just wish to reset/reboot this VM if it is fall into handle_hwpic. I
tried follwing patch and it seems not work. What can i do to force 
reset/reboot? 

@@ -7,10 +7,11 @@
  #include "biosvar.h" // SET_IVT
  #include "config.h" // CONFIG_*
  #include "output.h" // dprintf
  #include "pic.h" // pic_*
+ #include "hw/ps2port.h"
 
 u16
 pic_irqmask_read(void)
 {
 if (!CONFIG_HARDWARE_IRQ)
@@ -103,10 +104,11 @@ pic_isr2_read(void)
 void VISIBLE16
 handle_hwpic1(struct bregs *regs)
 {
 dprintf(DEBUG_ISR_hwpic1, "handle_hwpic1 irq=%x\n", pic_isr1_read());
 pic_eoi1();
+i8042_reboot();
 }

useful information:
kmod ftrace:
<...>-31509 [035] 154753.180077: kvm_exit: reason EXCEPTION_NMI rip 0x3 info 0 
8306
<...>-31509 [035] 154753.180077: kvm_emulate_insn: 0:3:f0 53 (real)
<...>-31509 [035] 154753.180077: kvm_inj_exception: #UD (0x0)
<...>-31509 [035] 154753.180077: kvm_entry: vcpu 0

bad SeaBIOS log:
[2015-12-17 12:37:30] In 32bit resume
[2015-12-17 12:37:30] =Attempting a hard reboot
[2015-12-17 12:37:30] SeaBIOS (version 
rel-1.8.1-0-g4adadbd-20151217_104405-linux-emBwNn)
[2015-12-17 12:37:30] No Xen hypervisor found.
[2015-12-17 12:37:30] Running on QEMU (i440fx)
[2015-12-17 12:37:30] Running on KVM
[2015-12-17 12:37:30] RamSize: 0x8000 [cmos]
[2015-12-17 12:37:30] Relocating init from 0x000db230 to 0x7ffad360 (size 76768)
[2015-12-17 12:37:30] Found QEMU fw_cfg
[2015-12-17 12:37:30] RamBlock: addr 0x len 0x8000 
[e820]
[2015-12-17 12:37:30] Moving pm_base to 0x600
[2015-12-17 12:37:30] boot order:
[2015-12-17 12:37:30] 1: /pci@i0cf8/ide@1,1/drive@0/disk@0
[2015-12-17 12:37:30] 2: HALT
[2015-12-17 12:37:30] maininit
[2015-12-17 12:37:30] platform_hardware_setup
[2015-12-17 12:37:30] init pic
[2015-12-17 12:37:30] pic_setup
[2015-12-17 12:37:30] pic_reset
[2015-12-17 12:37:30] enable_hwirq
[2015-12-17 12:37:30] CPU Mhz=3304
[2015-12-17 12:37:30] enable_hwirq
[2015-12-17 12:37:30] enable_hwirq
[2015-12-17 12:37:30] === PCI bus & bridge init ===
[2015-12-17 12:37:30] PCI: pci_bios_init_bus_rec bus = 0x0
[2015-12-17 12:37:30] === PCI device probing ===
[2015-12-17 12:37:30] Found 6 PCI devices (max PCI bus is 00)
[2015-12-17 12:37:30] === PCI new allocation pass #1 ===
[2015-12-17 12:37:30] PCI: check devices
[2015-12-17 12:37:30] === PCI new allocation pass #2 ===
[2015-12-17 12:37:30] PCI: IO: c000 - c02f
[2015-12-17 12:37:30] PCI: 32: 8000 - fec0
[2015-12-17 12:37:30] PCI: map device bdf=00:01.2  bar 4, addr c000, size 
0020 [io]
[2015-12-17 12:37:30] PCI: map device bdf=00:01.1  bar 4, addr c020, size 
0010 [io]
[2015-12-17 12:37:30] PCI: map device bdf=00:02.0  bar 6, addr febe, size 
0001 [mem]
[2015-12-17 12:37:30] PCI: map device bdf=00:02.0  bar 1, addr febf, size 
1000 [mem]
[2015-12-17 12:37:30] PCI: map device bdf=00:02.0  bar 0, addr fc00, size 
0200 [prefmem]
[2015-12-17 12:37:30] PCI: init bdf=00:00.0 id=8086:1237
[2015-12-17 12:37:30] PCI: init bdf=00:01.0 id=8086:7000
[2015-12-17 12:37:30] PIIX3/PIIX4 init: elcr=00 0c
[2015-12-17 12:37:30] PCI: init bdf=00:01.1 id=8086:7010
[2015-12-17 12:37:30] PCI: init bdf=00:01.2 id=8086:7020
[2015-12-17 12:37:30] PCI: init bdf=00:01.3 id=8086:7113
[2015-12-17 12:37:30] Using pmtimer, ioport 0x608
[2015-12-17 12:37:30] PCI: init bdf=00:02.0 id=1013:00b8
[2015-12-17 12:37:30] PCI: Using 00:02.0 for primary VGA
[2015-12-17 12:37:30] handle_hshamanpnd:dl leae_p_sismcmp_p:i: d a=ap3  
 <<=== everytime stuck, AP setup log seems abnormal!
[2015-12-17 12:37:30] ièf[cf_^ifd_=f3
[2015-12-17 12:37:30] èf[f^f_f]fÃÍ^XË<90>Found 4 cpu(s) max supported 4 cpu(s)
[2015-12-17 12:37:30] Copying PIR from 0x7ffbea18 to 0x000f5700
[2015-12-17 12:37:30] Copying MPTABLE from 0x6e30/7ffa42c0 to 0x000f55e0
[2015-12-17 12:37:30] Copying SMBIOS entry point from 0x6e11 to 0x000f55c0
[2015-12-17 12:37:31] Scan for VGA option rom
[2015-12-17 12:37:31] Running option rom at c000:0003
[2015-12-17 12:37:31] Start SeaVGABIOS (version 
rel-1.8.1-0-g4adadbd-20150316_085902-nilsson.home.kraxel.org)
[2015-12-17 12:37:31] enter vga_post:
[2015-12-17 12:37:31]a=0010  b=  c=  d= ds= 
es=f000 ss=
[2015-12-17 12:37:31]   si= di=57e0 bp= sp=6dbe cs=f000 
ip=d1fb  f=
[2015-12-17 12:37:31] cirrus init
[2015-12-17 12:37:31] cirrus init 2
[2015-12-17 12:37:31] Attempting to allocate VGA stack via pmm call to 
f000:d2a0   <<== here stuck, loop handle PIC irq0
[2015-12-17 12:37:35] handle_hwpic1 irq=0
[2015-12-17 12:37:35] handle_hwpic1 irq=0

[Qemu-devel] [PATCH v2] qmp: return err msg when powerdown a vm when it isn't in running state

[Qinghua Jin]

This is an update of patch previously show in msg:

  http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03817.html

When send system_powerdown to QMP when the vm isn't in RUN_STATE_RUNNING,
it will be ignored by system. So reply a err msg with the situation.

Signed-off-by: Qinghua Jin 
---
 qmp.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/qmp.c b/qmp.c
index 0a1fa19..384df56 100644
--- a/qmp.c
+++ b/qmp.c
@@ -114,8 +114,13 @@ void qmp_system_reset(Error **errp)
 qemu_system_reset_request();
 }
 
-void qmp_system_powerdown(Error **erp)
+void qmp_system_powerdown(Error **errp)
 {
+if (!runstate_check(RUN_STATE_RUNNING)) {
+error_setg(errp,
+   "Can not powerdown virtual machine as it is not running");
+return;
+}
 qemu_system_powerdown_request();
 }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/3] net/vmxnet3: return 1 on device activation failure

[Miao Yan]

2015-12-22 2:15 GMT+08:00 P J P :
> +-- On Mon, 21 Dec 2015, Miao Yan wrote --+
> | So return 1 on device activation failure instead of -1;
> |
> | Signed-off-by: Miao Yan 
> | ---
> |  hw/net/vmxnet3.c | 2 +-
> |  1 file changed, 1 insertion(+), 1 deletion(-)
> |
> | diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> | index e168285..9185408 100644
> | --- a/hw/net/vmxnet3.c
> | +++ b/hw/net/vmxnet3.c
> | @@ -1652,7 +1652,7 @@ static uint64_t 
> vmxnet3_get_command_status(VMXNET3State *s)
> |
> |  switch (s->last_command) {
> |  case VMXNET3_CMD_ACTIVATE_DEV:
> | -ret = (s->device_active) ? 0 : -1;
> | +ret = (s->device_active) ? 0 : 1;
> |  VMW_CFPRN("Device active: %" PRIx64, ret);
> |  break;
>
>   It seems okay. Considering that the function returns 'uint64_t', -1 would
> become an extremely large value. I wonder if that is intended.
>
> If '1' indicates the error, the 'default:' case in the same switch needs to be
> updated too.


'1' indicates an error on device activation. Not sure
about the 'unknown command' case.


>
>   default:
> VMW_WRPRN("Received request for unknown command: %x", 
> s->last_command);
> ret = -1;
> break;
>
>
> Why does the function return 'uint64_t' type? All return values from other
> cases seem to be within uint32_t type.

Linux driver uses VXMNET3_READ_BAR1_REG() which calls readl().
That should be an indication that the driver expects 32bit values.
But the prototype in MemoryRegionOps requires uint64_t.


>
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

[Qemu-devel] [PATCH v2 2/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command

[Miao Yan]

VMXNET3_CMD_GET_DID_LO should return PCI ID of the device
and VMXNET3_CMD_GET_DID_HI should return vmxnet3 revision ID.

This behavior can be observed by the following steps:

1) run a Linux distro on esxi server
2) modify vmxnet3 Linux driver to read DID_HI and DID_LO:

  VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_LO);
  lo =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);

  VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, VMXNET3_CMD_GET_DID_HI);
  high =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
  pr_info("vmxnet3 DID lo: 0x%x, high: 0x%x\n", lo, high);

The kernel log will have something like the following message:

  [ 7005.70] vmxnet3 DID lo: 0x7b0, high: 0x1

Signed-off-by: Miao Yan 
---
Changes in v2: 
 - update vmxnet3_handle_command not to print error when issuing these commands

 hw/net/vmxnet3.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 9185408..cddbf6d 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1640,6 +1640,14 @@ static void vmxnet3_handle_command(VMXNET3State *s, 
uint64_t cmd)
   "adaptive ring info flags");
 break;
 
+case VMXNET3_CMD_GET_DID_LO:
+VMW_CBPRN("Set: Get lower part of device ID");
+break;
+
+case VMXNET3_CMD_GET_DID_HI:
+VMW_CBPRN("Set: Get upper part of device ID");
+break;
+
 default:
 VMW_CBPRN("Received unknown command: %" PRIx64, cmd);
 break;
@@ -1683,6 +1691,14 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State 
*s)
 ret = VMXNET3_DISABLE_ADAPTIVE_RING;
 break;
 
+case VMXNET3_CMD_GET_DID_LO:
+ret = PCI_DEVICE_ID_VMWARE_VMXNET3;
+break;
+
+case VMXNET3_CMD_GET_DID_HI:
+ret = VMXNET3_DEVICE_REVISION;
+break;
+
 default:
 VMW_WRPRN("Received request for unknown command: %x", s->last_command);
 ret = -1;
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/4] net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO

[Miao Yan]

VMXNET3_CMD_GET_DEV_EXTRA_INFO should return 0 for emulation
mode

This behavior can be observed by the following steps:

1) run a Linux distro on esxi server
2) modify vmxnet3 Linux driver to read the register:

  VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD, 
VMXNET3_CMD_GET_DEV_EXTRA_INFO);
  ret =  VMXNET3_READ_BAR1_REG(adapter, VMXNET3_REG_CMD);
  pr_info("vmxnet3 dev_info: 0x%x\n", ret);

The kernel log will have some like the following message:

  [ 7005.70] vmxnet3 dev_info: 0x0

Signed-off-by: Miao Yan 
---
Changes in v2: 
 - update vmxnet3_handle_command not to print error when issuing these commands

 hw/net/vmxnet3.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index cddbf6d..b8bc360 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1648,6 +1648,10 @@ static void vmxnet3_handle_command(VMXNET3State *s, 
uint64_t cmd)
 VMW_CBPRN("Set: Get upper part of device ID");
 break;
 
+case VMXNET3_CMD_GET_DEV_EXTRA_INFO:
+VMW_CBPRN("Set: Get device extra info");
+break;
+
 default:
 VMW_CBPRN("Received unknown command: %" PRIx64, cmd);
 break;
@@ -1667,6 +1671,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State 
*s)
 case VMXNET3_CMD_RESET_DEV:
 case VMXNET3_CMD_QUIESCE_DEV:
 case VMXNET3_CMD_GET_QUEUE_STATUS:
+case VMXNET3_CMD_GET_DEV_EXTRA_INFO:
 ret = 0;
 break;
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 1/4] net/vmxnet3: return 1 on device activation failure

[Miao Yan]

When reading device status, 0 means device is successfully
activated and 1 means error.

This behavior can be observed by the following steps:

1) run a Linux distro on esxi server
2) modify vmxnet3 Linux driver to give it an invalid
   address to 'adapter->shared_pa' which is the
   shared memory for guest/host communication

This will trigger device activation failure and kernel
log will have the following message:

   [ 7138.403256] vmxnet3 :03:00.0 eth1: Failed to activate dev: error 1

So return 1 on device activation failure instead of -1;

Signed-off-by: Miao Yan 
---
 hw/net/vmxnet3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index e168285..9185408 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1652,7 +1652,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State 
*s)
 
 switch (s->last_command) {
 case VMXNET3_CMD_ACTIVATE_DEV:
-ret = (s->device_active) ? 0 : -1;
+ret = (s->device_active) ? 0 : 1;
 VMW_CFPRN("Device active: %" PRIx64, ret);
 break;
 
-- 
1.9.1

Re: [Qemu-devel] 回复： 回复： someconfusion on qemu i/o pocess and the qcow2format

[Stefan Hajnoczi]

On Mon, Dec 21, 2015 at 10:48:07AM +0800, 浩樊啊 wrote:
> I want to change the queue_size of virtio-blk ring from 128 to 1024, so I  
> change here:
> s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
> but the vm will not work. Are any thing i missed ?

I think 1024 should work in theory (it's the max supported virtqueue
size).  You'd have to debug it to figure out why.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 10/10] iotests: 095: Use TEST_IMG override instead of "mv"

[Fam Zheng]

On Fri, 12/18 19:18, Max Reitz wrote:
> On 16.12.2015 10:54, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng 
> > ---
> >  tests/qemu-iotests/095 | 12 ++--
> >  tests/qemu-iotests/095.out |  8 
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/095 b/tests/qemu-iotests/095
> > index 6630181..cbe61bc 100755
> > --- a/tests/qemu-iotests/095
> > +++ b/tests/qemu-iotests/095
> > @@ -50,17 +50,16 @@ _supported_os Linux
> >  size_smaller=5M
> >  size_larger=100M
> >  
> > -_make_test_img $size_smaller
> > -mv "${TEST_IMG}" "${TEST_IMG}.base"
> > +TEST_IMG="$TEST_IMG.base _make_test_img $size_smaller"
> 
> I think this should be:
> 
> TEST_IMG="$TEST_IMG.base" _make_test_img $size_smaller

Yes, it's a regex search-replace error. Will fix.

> 
> >  
> > -_make_test_img -b "${TEST_IMG}.base" $size_larger
> > -mv "${TEST_IMG}" "${TEST_IMG}.snp1"
> > +TEST_IMG="$TEST_IMG.snp1 _make_test_img -b "${TEST_IMG}.base" $size_larger"
> 
> TEST_IMG="$TEST_IMG.snp1" _make_test_img -b "${TEST_IMG}.base" \
> $size_larger
> 
> (Also, you could strip the {} because the inconsistent usage looks a bit
> strange inside of a single line.)

Okay.

> 
> >  
> >  _make_test_img -b "${TEST_IMG}.snp1" $size_larger
> >  
> >  echo
> >  echo "=== Base image info before commit and resize ==="
> > -TEST_IMG="${TEST_IMG}.base" _img_info
> > +TEST_IMG="${TEST_IMG}.base" _img_info | \
> > +sed -e 's/^cluster_size: .*$/cluster_size: XXX/'
> 
> How about using _filter_img_info instead?

Will drop this change and do it in a separate patch.

Thanks!

Fam

Re: [Qemu-devel] [PATCH v5 RFC] spec: add qcow2 bitmaps extension specification

[Vladimir Sementsov-Ogievskiy]

On 22.12.2015 02:56, Max Reitz wrote:

On 21.12.2015 16:25, Vladimir Sementsov-Ogievskiy wrote:

The new feature for qcow2: storing bitmaps.

Only bitmaps, relative to the virtual disk, stored in qcow2 file, should
be stored in this qcow2 file.

Strings started from +# are RFC-strings, not to be commited of course

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---


v5:

- 'Dirty bitmaps' renamed to 'Bitmaps', as we may have several types of
   bitmaps.
- rewordings
- move upper bounds to "Notes about Qemu limits"
- s/should/must somewhere. (but not everywhere)
- move name_size field closer to name itself in bitmap header
- add extra data area to bitmap header
- move bitmap data description to separate section



  docs/specs/qcow2.txt | 160 ++-
  1 file changed, 159 insertions(+), 1 deletion(-)

Looks good! :-)


Thank you! )



Some comments below, but I think the general design is good now.


diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 121dfc8..3d557ee 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -103,7 +103,19 @@ in the description of a field.
  write to an image with unknown auto-clear features if it
  clears the respective bits from this field first.
  
-Bits 0-63:  Reserved (set to 0)

+Bit 0:  Bitmaps extension bit.
+This bit is responsible for Bitmaps extension
+consistency.
+
+If it is set, but there is no Bitmaps
+extension, this should be considered as an
+error.
+
+If it is not set, but there is a Bitmaps
+extension, its data should be considered as
+inconsistent.
+
+Bits 1-63:  Reserved (set to 0)
  
   96 -  99:  refcount_order

  Describes the width of a reference count block entry 
(width
@@ -123,6 +135,7 @@ be stored. Each extension has a structure like the 
following:
  0x - End of the header extension area
  0xE2792ACA - Backing file format name
  0x6803f857 - Feature name table
+0x23852875 - Bitmaps extension
  other  - Unknown header extension, can be safely
   ignored
  
@@ -166,6 +179,34 @@ the header extension data. Each entry look like this:

  terminated if it has full length)
  
  
+== Bitmaps extension ==

+
+Bitmaps extension is an optional header extension. It provides an ability to
+store virtual disk related bitmaps in a qcow2 image. For now there is only one
+type of such bitmaps: Dirty Tracking Bitmap, which just tracks virtual disk
+changes from some moment.
+
+The data of the extension should be considered as consistent only if
+corresponding auto-clear feature bit is set (see autoclear_features above).
+
+The fields of Bitmaps extension are:
+
+  0 -  3:  nb_bitmaps
+   The number of bitmaps contained in the image. Must be
+   greater or equal to 1.
+
+   Note: Qemu currently only supports up to 65535 bitmaps per
+   image.
+
+  4 -  7:  bitmap_directory_size
+   Size of the Bitmap Directory in bytes. It must be equal to
+   sum of sizes of all (nb_bitmaps) bitmap headers.

I'd rather write this as: "Size of the Bitmap Directory in bytes, i.e.
the cumulative size of all (nb_bitmaps) bitmap headers."

("It must" sounds like it's an additional restriction while it's
actually just an explanation.)


+
+  8 - 15:  bitmap_directory_offset
+   Offset into the image file at which the Bitmap Directory
+   starts. Must be aligned to a cluster boundary.
+
+
  == Host cluster management ==
  
  qcow2 manages the allocation of host clusters by maintaining a reference count

@@ -360,3 +401,120 @@ Snapshot table entry:
  
  variable:   Padding to round up the snapshot table entry size to the

  next multiple of 8.
+
+
+== Bitmaps ==
+
+The feature supports storing bitmaps in a qcow2 image. All bitmaps are related
+to the virtual disk, stored in this image.
+
+=== Bitmap Directory ===
+
+Each bitmap saved in the image is described in a Bitmap Directory entry. Bitmap
+Directory is a contiguous area in the image file, whose starting offset and
+length are given by the header extension fields bitmap_directory_offset and
+bitmap_directory_size. The entries of the bitmap directory have variable
+length, depending on the length of the bitmap name and extra data. These
+entries are also called bitmap headers.
+
+Bitmap Directory Entry:
+

[Qemu-devel] [PATCH v2 10/11] iotests: 095: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/095 | 6 ++
 tests/qemu-iotests/095.out | 4 ++--
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/095 b/tests/qemu-iotests/095
index 6630181..57a730e 100755
--- a/tests/qemu-iotests/095
+++ b/tests/qemu-iotests/095
@@ -50,11 +50,9 @@ _supported_os Linux
 size_smaller=5M
 size_larger=100M
 
-_make_test_img $size_smaller
-mv "${TEST_IMG}" "${TEST_IMG}.base"
+TEST_IMG="$TEST_IMG.base" _make_test_img $size_smaller
 
-_make_test_img -b "${TEST_IMG}.base" $size_larger
-mv "${TEST_IMG}" "${TEST_IMG}.snp1"
+TEST_IMG="$TEST_IMG.snp1" _make_test_img -b "$TEST_IMG.base" $size_larger
 
 _make_test_img -b "${TEST_IMG}.snp1" $size_larger
 
diff --git a/tests/qemu-iotests/095.out b/tests/qemu-iotests/095.out
index 2360061..61a2057 100644
--- a/tests/qemu-iotests/095.out
+++ b/tests/qemu-iotests/095.out
@@ -1,6 +1,6 @@
 QA output created by 095
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=5242880
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 
backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=5242880
+Formatting 'TEST_DIR/t.IMGFMT.snp1', fmt=IMGFMT size=104857600 
backing_file=TEST_DIR/t.IMGFMT.base
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 
backing_file=TEST_DIR/t.IMGFMT.snp1
 
 === Base image info before commit and resize ===
-- 
2.4.3

[Qemu-devel] [PATCH v2 02/11] iotests: 019: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/019 | 13 +++--
 tests/qemu-iotests/019.out |  4 ++--
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
index f5ecbf5..0937b5c 100755
--- a/tests/qemu-iotests/019
+++ b/tests/qemu-iotests/019
@@ -54,6 +54,9 @@ _unsupported_imgopts "subformat=monolithicFlat" \
 TEST_OFFSETS="0 4294967296"
 CLUSTER_SIZE=65536
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
+
 _make_test_img 6G
 
 echo "Filling base image"
@@ -71,8 +74,8 @@ _check_test_img
 echo "Creating test image with backing file"
 echo
 
-mv "$TEST_IMG" "$TEST_IMG.base"
-_make_test_img -b "$TEST_IMG.base" 6G
+TEST_IMG="$TEST_IMG_SAVE.orig"
+_make_test_img -b "$TEST_IMG_SAVE.base" 6G
 
 echo "Filling test image"
 echo
@@ -86,9 +89,7 @@ for offset in $TEST_OFFSETS; do
 done
 _check_test_img
 
-mv "$TEST_IMG" "$TEST_IMG.orig"
-
-
+TEST_IMG="$TEST_IMG_SAVE"
 
 # Test the conversion twice: One test with the old-style -B option and another
 # one with -o backing_file
@@ -98,7 +99,7 @@ for backing_option in "-B " "-o backing_file="; do
 echo
 echo Testing conversion with $backing_option"$TEST_IMG.base" | 
_filter_testdir | _filter_imgfmt
 echo
-$QEMU_IMG convert -O $IMGFMT $backing_option"$TEST_IMG.base" 
"$TEST_IMG.orig" "$TEST_IMG"
+$QEMU_IMG convert -f $IMGFMT -O $IMGFMT $backing_option"$TEST_IMG.base" 
"$TEST_IMG.orig" "$TEST_IMG"
 
 echo "Checking if backing clusters are allocated when they shouldn't"
 echo
diff --git a/tests/qemu-iotests/019.out b/tests/qemu-iotests/019.out
index 615450a..0124264 100644
--- a/tests/qemu-iotests/019.out
+++ b/tests/qemu-iotests/019.out
@@ -1,5 +1,5 @@
 QA output created by 019
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=6442450944
 Filling base image
 
 === IO: pattern 42
@@ -269,7 +269,7 @@ wrote 65536/65536 bytes at offset 4296015872
 No errors were found on the image.
 Creating test image with backing file
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=6442450944 
backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=6442450944 
backing_file=TEST_DIR/t.IMGFMT.base
 Filling test image
 
 === IO: pattern 43
-- 
2.4.3

[Qemu-devel] [PATCH v2 05/11] iotests: 028: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/028 | 5 -
 tests/qemu-iotests/028.out | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index a1f4423..009510d 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -60,6 +60,9 @@ base_size=$(( image_size - 1024 * 1024 * 1024 ))
 
 offset=$(( base_size - 32 * 1024 ))
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
+
 _make_test_img $base_size
 
 echo "Filling base image"
@@ -73,7 +76,7 @@ _check_test_img
 echo "Creating test image with backing file"
 echo
 
-mv "$TEST_IMG" "$TEST_IMG.base"
+TEST_IMG="$TEST_IMG_SAVE"
 _make_test_img -b "$TEST_IMG.base" $image_size
 
 echo "Filling test image"
diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
index 29c9972..279029d 100644
--- a/tests/qemu-iotests/028.out
+++ b/tests/qemu-iotests/028.out
@@ -1,5 +1,5 @@
 QA output created by 028
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=3221227008
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=3221227008
 Filling base image
 
 === IO: pattern 195
-- 
2.4.3

[Qemu-devel] [PATCH v2 00/11] iotests: Clean up "mv $TEST_IMG $TEST_IMG.XXX"

[Fam Zheng]

v2: Add Max's rev-by in patches 1-9.
Fix quote bugs in patch 10 and split out _img_info filtering patch 11.

Commit 794d00f71d fixed two "mv" commands into the TEST_IMG override approach.
There are still more occasions of "mv", this series fixes them.

The benefit is it drops the assumption that the created image is a single file,
which is not true for VMDK or the coming QBM format.  On top of this, those
formats can be added to these cases later.

The patches stems from QBM work, but I will also take a look at adding
applicable VMDK subformats once merged.

Please review.

Fam

Fam Zheng (11):
  iotests: 018: Use TEST_IMG override instead of "mv"
  iotests: 019: Use TEST_IMG override instead of "mv"
  iotests: 020: Use TEST_IMG override instead of "mv"
  iotests: 024: Use TEST_IMG override instead of "mv"
  iotests: 028: Use TEST_IMG override instead of "mv"
  iotests: 034: Use TEST_IMG override instead of "mv"
  iotests: 037: Use TEST_IMG override instead of "mv"
  iotests: 038: Use TEST_IMG override instead of "mv"
  iotests: 050: Use TEST_IMG override instead of "mv"
  iotests: 095: Use TEST_IMG override instead of "mv"
  iotests: 095: Filter _img_info output

 tests/qemu-iotests/018 |  8 
 tests/qemu-iotests/018.out |  2 +-
 tests/qemu-iotests/019 | 13 +++--
 tests/qemu-iotests/019.out |  4 ++--
 tests/qemu-iotests/020 |  7 +--
 tests/qemu-iotests/020.out |  2 +-
 tests/qemu-iotests/024 | 10 --
 tests/qemu-iotests/024.out |  4 ++--
 tests/qemu-iotests/028 |  5 -
 tests/qemu-iotests/028.out |  2 +-
 tests/qemu-iotests/034 |  6 +-
 tests/qemu-iotests/034.out |  2 +-
 tests/qemu-iotests/037 |  5 -
 tests/qemu-iotests/037.out |  2 +-
 tests/qemu-iotests/038 |  5 -
 tests/qemu-iotests/038.out |  2 +-
 tests/qemu-iotests/050 |  9 +++--
 tests/qemu-iotests/050.out |  4 ++--
 tests/qemu-iotests/095 | 10 --
 tests/qemu-iotests/095.out |  6 ++
 20 files changed, 66 insertions(+), 42 deletions(-)

-- 
2.4.3

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/21/2015 11:49 PM, Paolo Bonzini wrote:



On 20/12/2015 12:38, Cao jin wrote:


+object_unref(OBJECT(ds));
+object_unref(OBJECT(bds));
+object_unref(OBJECT(bus));


I think these should be object_unparent, not unref.



But, it seems these 3 objects isn`t added as a child-property via
object_property_add_child() during creation, so OBJECT(ds)->parent(so
does the other 2) will be NULL, and so object_unparent will do nothing?


qdev_init_nofail adds them (qdev_init_nofail -> object_property_set_bool
-> device_set_realized -> object_property_add_child).

If you haven't reached qdev_init_nofail, you should indeed unref ds and
bds instead.  However, the bus should be unparented because pci_bus_new
makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
object_property_add_child).



Yes...that`s true.

and @Marcel, I think maybe this is final decision?


Paolo


.



--
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Cao jin]

Hi

On 12/22/2015 09:24 AM, Cao jin wrote:



On 12/21/2015 11:15 PM, Stefano Stabellini wrote:

On Fri, 18 Dec 2015, Cao jin wrote:

[...]


This doesn't even compile: you are missing a ';'

Please at least build test patches.



Yup...sorry for the silly mistake...but weird, I did build and didn`t
got error...



I test with:
$ readelf -s qemu-system-x86_64 | grep xen_pv_*
got nothing, which make me realized that I don`t have xen support on my 
computer... maybe that`s why I didn`t got compile error





[...]

--
Yours Sincerely,

Cao Jin

[Qemu-devel] [PATCH v2 0/4] correct some register return values for vxmnet3

[Miao Yan]

Qemu vmxnet3 emulation doesn't recognize VMXNET3_CMD_GET_DID_LO,
VMXNET3_CMD_GET_DID_HI and VMXNET3_CMD_GET_DEV_EXTRA_INFO command and 
returns -1 on all of them.

This patchset makes them return correct values.

Changes in v2:
 - return 0 on unknown command

Miao Yan (4):
  net/vmxnet3: return 1 on device activation failure
  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DID_* command
  net/vmxnet3: return correct value for VMXNET3_CMD_GET_DEV_EXTRA_INFO
  net/vmxnet3: return 0 on unknown command

 hw/net/vmxnet3.c | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 4/4] net/vmxnet3: return 0 on unknown command

[Miao Yan]

Return 0 on unknown command, this is what esxi behaves.

Signed-off-by: Miao Yan 
---
 hw/net/vmxnet3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index b8bc360..a429405 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1706,7 +1706,7 @@ static uint64_t vmxnet3_get_command_status(VMXNET3State 
*s)
 
 default:
 VMW_WRPRN("Received request for unknown command: %x", s->last_command);
-ret = -1;
+ret = 0;
 break;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Cao jin]

On 12/21/2015 11:15 PM, Stefano Stabellini wrote:

On Fri, 18 Dec 2015, Cao jin wrote:

Signed-off-by: Cao jin 
---
  hw/i386/xen/xen_pvdevice.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
index c218947..a6c93d0 100644
--- a/hw/i386/xen/xen_pvdevice.c
+++ b/hw/i386/xen/xen_pvdevice.c
@@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
  .endianness = DEVICE_LITTLE_ENDIAN,
  };

-static int xen_pv_init(PCIDevice *pci_dev)
+static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
  {
  XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
  uint8_t *pci_conf;

  /* device-id property must always be supplied */
-if (d->device_id == 0x)
-   return -1;
+if (d->device_id == 0x) {
+error_setg(errp, "Device ID invalid, it must always be supplied")


This doesn't even compile: you are missing a ';'

Please at least build test patches.



Yup...sorry for the silly mistake...but weird, I did build and didn`t 
got error...





+   return;


I realize that there was a tab before there, but please use spaces for
indentation.



surprised that I didn`t recognized the tab...V2 is coming soon.




+}

  pci_conf = pci_dev->config;

@@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)

  pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
   >mmio);
-
-return 0;
  }

  static Property xen_pv_props[] = {
@@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void 
*data)
  DeviceClass *dc = DEVICE_CLASS(klass);
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

-k->init = xen_pv_init;
+k->realize = xen_pv_realize;
  k->class_id = PCI_CLASS_SYSTEM_OTHER;
  dc->desc = "Xen PV Device";
  dc->props = xen_pv_props;
--
2.1.0







.



--
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH v2] qmp: return err msg when powerdown a vm when it isn't in running state

[Fam Zheng]

On Tue, 12/22 09:42, Qinghua Jin wrote:
> This is an update of patch previously show in msg:
> 
>   http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03817.html

The patch looks good to me, thanks for submitting the patch! There is only one
minor comment:

Patch revision note should go after the --- line (or the cover letter if you
have one) so that it won't be commited into the git log (because it wouldn't be
informative):

When send system_powerdown to QMP when the vm isn't in RUN_STATE_RUNNING,
it will be ignored by system. So reply a err msg with the situation.

Signed-off-by: Qinghua Jin 
---

This is an update of patch previously show in msg:

  http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03817.html

 qmp.c | 7 ++-

> 
> When send system_powerdown to QMP when the vm isn't in RUN_STATE_RUNNING,
> it will be ignored by system. So reply a err msg with the situation.
> 
> Signed-off-by: Qinghua Jin 
> ---
>  qmp.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/qmp.c b/qmp.c
> index 0a1fa19..384df56 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -114,8 +114,13 @@ void qmp_system_reset(Error **errp)
>  qemu_system_reset_request();
>  }
>  
> -void qmp_system_powerdown(Error **erp)
> +void qmp_system_powerdown(Error **errp)
>  {
> +if (!runstate_check(RUN_STATE_RUNNING)) {
> +error_setg(errp,
> +   "Can not powerdown virtual machine as it is not running");
> +return;
> +}
>  qemu_system_powerdown_request();
>  }
>  
> -- 
> 2.5.0
> 
> 
> 

Re: [Qemu-devel] [PATCH v2] qmp: return err msg when powerdown a vm when it isn't in running state

[Qinghua Jin]

Thanks for helping me, i'll use the patch format suggested by you.


At 2015-12-22 10:06:17, "Fam Zheng"  wrote:
>On Tue, 12/22 09:42, Qinghua Jin wrote:
>> This is an update of patch previously show in msg:
>> 
>>   http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03817.html
>
>The patch looks good to me, thanks for submitting the patch! There is only one
>minor comment:
>
>Patch revision note should go after the --- line (or the cover letter if you
>have one) so that it won't be commited into the git log (because it wouldn't be
>informative):
>
>When send system_powerdown to QMP when the vm isn't in RUN_STATE_RUNNING,
>it will be ignored by system. So reply a err msg with the situation.
>
>Signed-off-by: Qinghua Jin 
>---
>
>This is an update of patch previously show in msg:
>
>  http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03817.html
>
> qmp.c | 7 ++-
>
>> 
>> When send system_powerdown to QMP when the vm isn't in RUN_STATE_RUNNING,
>> it will be ignored by system. So reply a err msg with the situation.
>> 
>> Signed-off-by: Qinghua Jin 
>> ---
>>  qmp.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/qmp.c b/qmp.c
>> index 0a1fa19..384df56 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -114,8 +114,13 @@ void qmp_system_reset(Error **errp)
>>  qemu_system_reset_request();
>>  }
>>  
>> -void qmp_system_powerdown(Error **erp)
>> +void qmp_system_powerdown(Error **errp)
>>  {
>> +if (!runstate_check(RUN_STATE_RUNNING)) {
>> +error_setg(errp,
>> +   "Can not powerdown virtual machine as it is not 
>> running");
>> +return;
>> +}
>>  qemu_system_powerdown_request();
>>  }
>>  
>> -- 
>> 2.5.0
>> 
>> 
>> 

[Qemu-devel] [PATCH v2 07/11] iotests: 037: Use TEST_IMG override instead of "mv"

[Fam Zheng]

Signed-off-by: Fam Zheng 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/037 | 5 -
 tests/qemu-iotests/037.out | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/037 b/tests/qemu-iotests/037
index 9171d8c..5862451 100755
--- a/tests/qemu-iotests/037
+++ b/tests/qemu-iotests/037
@@ -51,6 +51,9 @@ size=128M
 echo
 echo "== creating backing file for COW tests =="
 
+TEST_IMG_SAVE="$TEST_IMG"
+TEST_IMG="$TEST_IMG.base"
+
 _make_test_img $size
 
 function backing_io()
@@ -71,7 +74,7 @@ function backing_io()
 
 backing_io 0 256 write | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
 
-mv "$TEST_IMG" "$TEST_IMG.base"
+TEST_IMG="$TEST_IMG_SAVE"
 
 _make_test_img -b "$TEST_IMG.base" 6G
 
diff --git a/tests/qemu-iotests/037.out b/tests/qemu-iotests/037.out
index 55b30fd..cd6710c 100644
--- a/tests/qemu-iotests/037.out
+++ b/tests/qemu-iotests/037.out
@@ -1,7 +1,7 @@
 QA output created by 037
 
 == creating backing file for COW tests ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 512/512 bytes at offset 512
-- 
2.4.3

[Qemu-devel] [PATCH v3] qmp: return err msg when powerdown a vm when it isn't in running state

[Qinghua Jin]

When send system_powerdown to QMP when the vm isn't in RUN_STATE_RUNNING,
it will be ignored by system. So reply a err msg with the situation.

Signed-off-by: Qinghua Jin 
---

This is an update of the patch as per Fam Zheng's suggestion:
  http://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03949.html

 qmp.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/qmp.c b/qmp.c
index 0a1fa19..384df56 100644
--- a/qmp.c
+++ b/qmp.c
@@ -114,8 +114,13 @@ void qmp_system_reset(Error **errp)
 qemu_system_reset_request();
 }
 
-void qmp_system_powerdown(Error **erp)
+void qmp_system_powerdown(Error **errp)
 {
+if (!runstate_check(RUN_STATE_RUNNING)) {
+error_setg(errp,
+   "Can not powerdown virtual machine as it is not running");
+return;
+}
 qemu_system_powerdown_request();
 }
 
-- 
2.5.0