Re: [Qemu-devel] [virtio-dev] Re: [PATCH v5] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

> -Original Message-
> From: virtio-...@lists.oasis-open.org [mailto:virtio-...@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Sent: Friday, July 29, 2016 1:34 PM
> To: Zeng, Xin
> Cc: Gonglei (Arei); qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org;
> Ola Liljedahl; Keating, Brian A; Hanweidong (Randy); Luonengjun; Huangpeng
> (Peter); Griffin, John; Ma, Liang J; Stefan Hajnoczi; Cornelia Huck; Varun 
> Sethi;
> Jani Kokkonen; Lingli Deng; Huangweidong (C)
> Subject: [virtio-dev] Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio 
> crypto
> device specification
> 
> On Thu, Jul 28, 2016 at 05:28:33AM +, Zeng, Xin wrote:
> > On Thursday, July 28, 2016 10:51 AM  Gonglei (Arei) Wrote:
> > > > > Changes from v4:
> > > > >  - introduce crypto services into virtio crypto device. The services
> > > > >currently defined are CIPHER, MAC, HASH, AEAD, KDF, ASYM,
> > > > PRIMITIVE.
> > > > >  - define a unified crypto request format that is consisted of
> > > > >general header + service specific request,  Where 'general header'
> is for
> > > > all
> > > > >crypto request,  'service specific request' is composed of
> > > > >operation parameter + input data + output data in generally.
> > > > >operation parameter is algorithm-specific parameters,
> > > > >input data is the data should be operated ,
> > > > >output data is the "operation result + result buffer".
> > > > >  - redefine the algorithms and structure based on above crypto
> services.
> > > > >  - rearrange the title and subtitle
> > > > >  - Only support CIPHER, MAC, HASH and AEAD crypto services, and Xin
> will
> > > > >focus KDF, ASYM and PRIMITIVE services.
> > > > >  - Some other corresponding fixes.
> > > > >  - Make a formal patch using tex type.
> > > > >
> > > > > Changes from v3:
> > > > >  - Don't use enum is the spec but macros in specific structures.
> [Michael &
> > > > Stefan]
> > > > >  - Add two complete structures for session creation and closing, so
> that
> > > > >   the spec is clear on how to lay out the request.  [Stefan]
> > > > >  - Definite the crypto operation request with assigned structure, in 
> > > > > this
> > > way,
> > > > >   each data request only occupies *one entry* of the Vring descriptor
> > > table,
> > > > >   which *improves* the *throughput* of data transferring.
> > > > >
> > > > > Changes from v2:
> > > > >  - Reserve virtio device ID 20 for crypto device. [Cornelia]
> > > > >  - Drop all feature bits, those capabilities are offered by the 
> > > > > device all
> the
> > > > time.  [Stefan & Cornelia]
> > > > >  - Add a new section 1.4.2 for driver requirements. [Stefan]
> > > > >  - Use definite type definition instead of enum type in some 
> > > > > structure.
> > > > [Stefan]
> > > > >  - Add virtio_crypto_cipher_alg definition. [Stefan]
> > > > >  - Add a "Device requirements" section as using MUST. [Stefan]
> > > > >  - Some grammar nits fixes and typo fixes. [Stefan & Cornelia]
> > > > >  - Add one VIRTIO_CRYPTO_S_STARTED status for the driver as the flag
> of
> > > > virtio-crypto device started and can work now.
> > > > >
> > > > > Great thanks for Stefan and Cornelia!
> > > > >
> > > > > Changes from v1:
> > > > >  - Drop the feature bit definition for each algorithm, and using 
> > > > > config
> > > space
> > > > instead  [Cornelia]
> > > > >  - Add multiqueue support and add corresponding feature bit
> > > > >  - Update Encryption process and header definition
> > > > >  - Add session operation process and add corresponding header
> > > description
> > > > >  - Other better description in order to fit for virtio spec  [Michael]
> > > > >  - Some other trivial fixes.
> > > >
> > > > OK I will let people who understand crypto comment on this.
> > >
> > > Excellently, thanks!
> > >
> > > > Down the road before we release this we'll need to link confirmance
> > > clauses
> > > > from confirmance section. Can be a patch on top, no big deal.
> > > >
> > >
> > > Sorry, where is the confirmance section and what's confirmance clauses?
> 
> I meant conformance :) The stuff in conformance.tex
> 
Okay, I got it, I'll add a separate patch :)


Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

> -Original Message-
> From: Stefan Hajnoczi [mailto:stefa...@gmail.com]
> Sent: Friday, July 29, 2016 9:49 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org; Ola Liljedahl;
> Keating Brian; Michael S . Tsirkin; Zeng Xin; Hanweidong (Randy); Luonengjun;
> Huangpeng (Peter); Griffin John; Ma Liang J; Stefan Hajnoczi; Cornelia Huck;
> Varun Sethi; Jani Kokkonen; Lingli Deng
> Subject: Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device
> specification
> 
> On Wed, Jul 27, 2016 at 06:08:23PM +0800, Gonglei wrote:
> > +The first driver-read-only field, \field{version} specifies the virtio 
> > crypto??s
> > +version, which is reserved for back-compatibility in future.It??s currently
> 
> Here...
> 
> > +Step2: Execute the detail encryption operation:
> > +\begin{enumerate}
> > +\item The driver fills out the encrypt requests;
> > +\item Put the requests into dataq and kick the virtqueue;
> > +\item The device executes the encryption operation according the requests??
> arguments;
> 
> ...and here there are invalid characters.
> 
> The email was sent with charset UTF-8 but these bytes are not valid
> UTF-8.
> 
> Please check your text editor and git-send-email(1) configuration so
> that you send patches with the correct charset.
> 
> Stefan

Sure, I'll check it before the next version submitted.

Thanks for your reminder. :)

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Eric Blake]

On 07/28/2016 01:07 AM, Prerna Saxena wrote:
> From: Prerna Saxena 
> 
> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
> 

> +
> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)

Rather than make this parenthetical, I would go with:

The payload should be set to zero on success or non-zero on failure,
unless the message already has an explicit reply body.

> +
> +This indicates to QEMU that the requested operation has deterministically
> +been met or not. Today, QEMU is expected to terminate the main vhost-user

Reads awkwardly; maybe:

The response payload gives QEMU a deterministic indication of the result
of the command.

> +loop upon receiving such errors. In future, qemu could be taught to be more
> +resilient for selective requests.
> +
> +For the message types that already solicit a reply from the client, the
> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set 
> brings
> +no behaviourial change. (See the 'Communication' section for details.)

s/behaviourial/behavioural/ (or if the document widely favors US
spelling, behavioral)


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Eric Blake]

On 07/29/2016 09:31 AM, Felipe Franciosi wrote:
> Heya,
> 
> On 29 Jul 2016, at 13:47, Marc-André Lureau 
> > wrote:
> 
> Hi
> 
> On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena 
> > wrote:
> From: Prerna Saxena 
> >

Wow, your mailer's poor choice of quoting style makes it VERY difficult
to read your reply.


> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)
> 
> Unless/unless (for consistency, the rest of the document doesn't use
> Upper-case inside parentheses)
> 
> Actually, if the sentence starts inside the parenthesis it should be capital.
> See rule 2a:
> http://www.grammarbook.com/punctuation/parens.asp
> 
> Prerna's text looks correct to me. If it's wrong in other places we should 
> probably fix it there separately.
> 

Based on the number of '>' inserted by my mailer, it appears that you
wrote all four of the above paragraphs. In reality, Prerna wrote the
first paragraph (quoted from the patch, Marc-André wrote the second, and
you wrote the third and fourth.  In fact, your mailer ACTIVELY stripped
the '>' that was already present in Marc-André's mail when he quoted
Prerna, rather than the usual paradigm of adding yet another layer of '>'.

You really need to get that fixed.  We should not have to hunt and
compare multiple emails in order to determine what you are adding to the
conversation, nor mis-attribute it to the wrong author.


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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [ANNOUNCE] QEMU 2.7.0-rc1 is now available

[Michael Roth]

Hello,

On behalf of the QEMU Team, I'd like to announce the availability of the
second release candidate for the QEMU 2.7 release.  This release is meant
for testing purposes and should not be used in a production environment.

http://wiki.qemu.org/download/qemu-2.7.0-rc1.tar.bz2

You can help improve the quality of the QEMU 2.7 release by testing this
release and reporting bugs on Launchpad:

https://bugs.launchpad.net/qemu/

The release plan, as well a documented known issues for release
candidates, are available at:

http://wiki.qemu.org/Planning/2.7

Please add entries to the ChangeLog for the 2.7 release below:

http://wiki.qemu.org/ChangeLog/2.7

Re: [Qemu-devel] [virtio-dev] Re: [PATCH v2 repost 4/7] virtio-balloon: speed up inflate/deflate process

[Dave Hansen]

On 07/28/2016 02:51 PM, Michael S. Tsirkin wrote:
>> > If 1MB is too big, how about 512K, or 256K?  32K seems too small.
>> > 
> It's only small because it makes you rescan the free list.
> So maybe you should do something else.
> I looked at it a bit. Instead of scanning the free list, how about
> scanning actual page structures? If page is unused, pass it to host.
> Solves the problem of rescanning multiple times, does it not?

FWIW, I think the new data structure needs some work.

Before, we had a potentially very long list of 4k areas.  Now, we've
just got a very large bitmap.  The bitmap might not even be very dense
if we are ballooning relatively few things.

Can I suggest an alternate scheme?  I think you actually need a hybrid
scheme that has bitmaps but also allows more flexibility in the pfn
ranges.  The payload could be a number of records each containing 3 things:

pfn, page order, length of bitmap (maybe in powers of 2)

Each record is followed by the bitmap.  Or, if the bitmap length is 0,
immediately followed by another record.  A bitmap length of 0 implies a
bitmap with the least significant bit set.  Page order specifies how
many pages each bit represents.

This scheme could easily encode the new data structure you are proposing
by just setting pfn=0, order=0, and a very long bitmap length.  But, it
could handle sparse bitmaps much better *and* represent large pages much
more efficiently.

There's plenty of space to fit a whole record in 64 bits.

[Qemu-devel] [PATCH for-2.7 v6 2/2] vhost-user: Attempt to fix a race with set_mem_table.

[Prerna Saxena]

From: Prerna Saxena 

The set_mem_table command currently does not seek a reply. Hence, there is
no easy way for a remote application to notify to QEMU when it finished
setting up memory, or if there were errors doing so.

As an example:
(1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net
application). SET_MEM_TABLE does not require a reply according to the spec.
(2) Qemu commits the memory to the guest.
(3) Guest issues an I/O operation over a new memory region which was configured 
on (1).
(4) The application has not yet remapped the memory, but it sees the I/O 
request.
(5) The application cannot satisfy the request because it does not know about 
those GPAs.

While a guaranteed fix would require a protocol extension (committed 
separately),
a best-effort workaround for existing applications is to send a GET_FEATURES
message before completing the vhost_user_set_mem_table() call.
Since GET_FEATURES requires a reply, an application that processes vhost-user
messages synchronously would probably have completed the SET_MEM_TABLE before 
replying.

Signed-off-by: Prerna Saxena 
---
 hw/virtio/vhost-user.c | 125 ++---
 1 file changed, 67 insertions(+), 58 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 521a5db..53c37a6 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -254,64 +254,6 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, 
uint64_t base,
 return 0;
 }
 
-static int vhost_user_set_mem_table(struct vhost_dev *dev,
-struct vhost_memory *mem)
-{
-int fds[VHOST_MEMORY_MAX_NREGIONS];
-int i, fd;
-size_t fd_num = 0;
-bool reply_supported = virtio_has_feature(dev->protocol_features,
-  VHOST_USER_PROTOCOL_F_REPLY_ACK);
-
-VhostUserMsg msg = {
-.request = VHOST_USER_SET_MEM_TABLE,
-.flags = VHOST_USER_VERSION,
-};
-
-if (reply_supported) {
-msg.flags |= VHOST_USER_NEED_REPLY_MASK;
-}
-
-for (i = 0; i < dev->mem->nregions; ++i) {
-struct vhost_memory_region *reg = dev->mem->regions + i;
-ram_addr_t offset;
-MemoryRegion *mr;
-
-assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
- );
-fd = memory_region_get_fd(mr);
-if (fd > 0) {
-msg.payload.memory.regions[fd_num].userspace_addr = 
reg->userspace_addr;
-msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
-msg.payload.memory.regions[fd_num].guest_phys_addr = 
reg->guest_phys_addr;
-msg.payload.memory.regions[fd_num].mmap_offset = offset;
-assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
-fds[fd_num++] = fd;
-}
-}
-
-msg.payload.memory.nregions = fd_num;
-
-if (!fd_num) {
-error_report("Failed initializing vhost-user memory map, "
- "consider using -object memory-backend-file share=on");
-return -1;
-}
-
-msg.size = sizeof(msg.payload.memory.nregions);
-msg.size += sizeof(msg.payload.memory.padding);
-msg.size += fd_num * sizeof(VhostUserMemoryRegion);
-
-vhost_user_write(dev, , fds, fd_num);
-
-if (reply_supported) {
-return process_message_reply(dev, msg.request);
-}
-
-return 0;
-}
-
 static int vhost_user_set_vring_addr(struct vhost_dev *dev,
  struct vhost_vring_addr *addr)
 {
@@ -514,6 +456,73 @@ static int vhost_user_get_features(struct vhost_dev *dev, 
uint64_t *features)
 return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
 }
 
+static int vhost_user_set_mem_table(struct vhost_dev *dev,
+struct vhost_memory *mem)
+{
+int fds[VHOST_MEMORY_MAX_NREGIONS];
+int i, fd;
+size_t fd_num = 0;
+uint64_t features;
+bool reply_supported = virtio_has_feature(dev->protocol_features,
+  VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+VhostUserMsg msg = {
+.request = VHOST_USER_SET_MEM_TABLE,
+.flags = VHOST_USER_VERSION,
+};
+
+if (reply_supported) {
+msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+}
+
+for (i = 0; i < dev->mem->nregions; ++i) {
+struct vhost_memory_region *reg = dev->mem->regions + i;
+ram_addr_t offset;
+MemoryRegion *mr;
+
+assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
+mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
+ );
+fd = memory_region_get_fd(mr);
+if (fd > 0) {
+msg.payload.memory.regions[fd_num].userspace_addr
+ = 

[Qemu-devel] [PATCH for-2.7 v6 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Prerna Saxena]

From: Prerna Saxena 

This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.

If negotiated, client applications should send a u64 payload in
response to any message that contains the "need_reply" bit set
on the message flags. Setting the payload to "zero" indicates the
command finished successfully. Likewise, setting it to "non-zero"
indicates an error.

Currently implemented only for SET_MEM_TABLE.

Reviewed-by: Marc-André Lureau 
Signed-off-by: Prerna Saxena 
---
 docs/specs/vhost-user.txt | 26 ++
 hw/virtio/vhost-user.c| 32 
 2 files changed, 58 insertions(+)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 777c49c..57a8357 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
  * Flags: 32-bit bit field:
- Lower 2 bits are the version (currently 0x01)
- Bit 2 is the reply flag - needs to be sent on each reply from the slave
+   - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
+ details.
  * Size - 32-bit size of the payload
 
 
@@ -126,6 +128,8 @@ the ones that do:
  * VHOST_GET_VRING_BASE
  * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
 
+[ Also see the section on REPLY_ACK protocol extension. ]
+
 There are several messages that the master sends with file descriptors passed
 in the ancillary data:
 
@@ -254,6 +258,7 @@ Protocol features
 #define VHOST_USER_PROTOCOL_F_MQ 0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD  1
 #define VHOST_USER_PROTOCOL_F_RARP   2
+#define VHOST_USER_PROTOCOL_F_REPLY_ACK  3
 
 Message types
 -
@@ -464,3 +469,24 @@ Message types
   is present in VHOST_USER_GET_PROTOCOL_FEATURES.
   The first 6 bytes of the payload contain the mac address of the guest to
   allow the vhost user backend to construct and broadcast the fake RARP.
+
+VHOST_USER_PROTOCOL_F_REPLY_ACK:
+---
+The original vhost-user specification only demands replies for certain
+commands. This differs from the vhost protocol implementation where commands
+are sent over an ioctl() call and block until the client has completed.
+
+With this protocol extension negotiated, the sender (QEMU) can set the
+"need_reply" [Bit 3] flag to any command. This indicates that
+the client MUST respond with a Payload VhostUserMsg indicating success or
+failure. The payload should be set to zero on success or non-zero on failure,
+unless the message already has an explicit reply body.
+
+This indicates to QEMU that the requested operation has deterministically
+been met or not. Today, QEMU is expected to terminate the main vhost-user
+loop upon receiving such errors. In future, qemu could be taught to be more
+resilient for selective requests.
+
+For the message types that already solicit a reply from the client, the
+presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
+no behaviourial change. (See the 'Communication' section for details.)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..521a5db 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
 VHOST_USER_PROTOCOL_F_MQ = 0,
 VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
 VHOST_USER_PROTOCOL_F_RARP = 2,
+VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
 
 VHOST_USER_PROTOCOL_F_MAX
 };
@@ -84,6 +85,7 @@ typedef struct VhostUserMsg {
 
 #define VHOST_USER_VERSION_MASK (0x3)
 #define VHOST_USER_REPLY_MASK   (0x1<<2)
+#define VHOST_USER_NEED_REPLY_MASK  (0x1 << 3)
 uint32_t flags;
 uint32_t size; /* the following payload size */
 union {
@@ -158,6 +160,25 @@ fail:
 return -1;
 }
 
+static int process_message_reply(struct vhost_dev *dev,
+ VhostUserRequest request)
+{
+VhostUserMsg msg;
+
+if (vhost_user_read(dev, ) < 0) {
+return -1;
+}
+
+if (msg.request != request) {
+error_report("Received unexpected msg type."
+ "Expected %d received %d",
+ request, msg.request);
+return -1;
+}
+
+return msg.payload.u64 ? -1 : 0;
+}
+
 static bool vhost_user_one_time_request(VhostUserRequest request)
 {
 switch (request) {
@@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
 int fds[VHOST_MEMORY_MAX_NREGIONS];
 int i, fd;
 size_t fd_num = 0;
+bool reply_supported = virtio_has_feature(dev->protocol_features,
+  VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
 VhostUserMsg msg = {
 .request = VHOST_USER_SET_MEM_TABLE,
 .flags = VHOST_USER_VERSION,
 };
 
+if (reply_supported) {
+msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+}
+
 for (i = 0; i < dev->mem->nregions; 

[Qemu-devel] [PATCH for-2.7 v6 0/2] vhost-user: Extend protocol to receive replies on any command.

[Prerna Saxena]

From: Prerna Saxena 

*** BLURB HERE ***
vhost-user: Extend protocol to receive replies on any command.

The current vhost-user protocol requires the client to send reply to only a
few commands. For the remaining commands, it is impossible for QEMU to know the
status of the requested operation -- ie, did it succeed? If so, by what time?

This is inconvenient, and can also lead to races. As an example:

(1) Qemu sends a SET_MEM_TABLE to the backend (eg, a vhost-user net 
application).Note that SET_MEM_TABLE does not require a reply according to the 
spec.
(2) Qemu commits the memory to the guest.
(3) Guest issues an I/O operation over a new memory region which was configured 
on (1).
(4) The application hasn't yet remapped the memory, but it sees the I/O request.
(5) The application cannot satisfy the request because it does not know about 
those GPAs.

Note that the kernel implementation does not suffer from this limitation since 
messages are sent via an ioctl(). The ioctl() blocks until the backend (eg. 
vhost-net) completes the command and returns (with an error code).

Changing the behaviour of current vhost-user commands would break existing 
applications.
Patch 1 introduces a protocol extension, VHOST_USER_PROTOCOL_F_REPLY_ACK. This
feature, if negotiated, allows QEMU to request a reply to any message by setting
the newly introduced "need_reply" flag. The application must then respond to 
qemu
by providing a status about the requested operation.

Patch 2 adds a workaround for the race described above for clients that do not 
support REPLY_ACK
feature. It introduces  a get_features command to be sent before returning from 
set_mem_table. While this is not a complete fix, it will help client 
applications that strictly process messagesin order.

Changelog:
--
Changes v5.1 -> v6:
1) Patch 1 : fixed some minor indentation issues and a really tiny 
documentation chang
2) Patch 2 : unchanged.

Changes v5->v5.1 :
1) Patch 1 : no change
2) Patch 2 : fixes a tiny typo I'd accidentally introduced while creating v5 
from v4. The code itself is unchanged from v4.

Changes v4->v5:
1) Patch 1 :
* Reword 'response' to 'reply' on public demand.
* Documentation is more concise.
Patch 2 : unchanged

Changes v3->v4:
1) Rearranged code in PATCH 1 to offset compiler warnings about missing 
declaration of vhost_user_read(). Fixed by moving process_message_reply() after 
definition of vhost_user_read()
2) Fixed minor suggestions in writeup for this protocol extension.

Changes v2->v3:
1) Swapped the patch numbers 1 & 2 from the previous series.
2) Patch 1 (previously patch 2 in v2): addresses MarcAndre's review comments 
and renames function 'process_message_response' to 'process_message_reply'
3) Patch 2 (ie patch 1 in v2) : Unchanged from v2.

Changes v1->v2:
1) Patch 1 : Ask for get_features before returning from set_mem_table(new).
2) Patch 2 : * Improve documentation.
  * Abstract out commonly used operations in the form of a function, 
process_message_response(). Also implement this only for SET_MEM_TABLE.

References:
v1 : https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07152.html
v2 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00048.html
v3 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01598.html
v4 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06173.html
v5 : https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06338.html
v5.1:https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06359.html 

Prerna Saxena (2):
  vhost-user: Introduce a new protocol feature REPLY_ACK.
  vhost-user: Attempt to fix a race with set_mem_table.

 docs/specs/vhost-user.txt |  26 +
 hw/virtio/vhost-user.c| 135 ++
 2 files changed, 114 insertions(+), 47 deletions(-)

-- 
1.8.1.2

Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Felipe Franciosi]

Heya,

On 29 Jul 2016, at 13:47, Marc-André Lureau 
> wrote:

Hi

On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena 
> wrote:
From: Prerna Saxena 
>

This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.

If negotiated, client applications should send a u64 payload in
response to any message that contains the "need_reply" bit set
on the message flags. Setting the payload to "zero" indicates the
command finished successfully. Likewise, setting it to "non-zero"
indicates an error.

Currently implemented only for SET_MEM_TABLE.

Signed-off-by: Prerna Saxena 
>
---
docs/specs/vhost-user.txt | 26 ++
hw/virtio/vhost-user.c| 32 
2 files changed, 58 insertions(+)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 777c49c..54b5c8f 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
 * Flags: 32-bit bit field:
   - Lower 2 bits are the version (currently 0x01)
   - Bit 2 is the reply flag - needs to be sent on each reply from the slave
+   - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
+ details.
 * Size - 32-bit size of the payload


@@ -126,6 +128,8 @@ the ones that do:
 * VHOST_GET_VRING_BASE
 * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)

+[ Also see the section on REPLY_ACK protocol extension. ]
+
There are several messages that the master sends with file descriptors passed
in the ancillary data:

@@ -254,6 +258,7 @@ Protocol features
#define VHOST_USER_PROTOCOL_F_MQ 0
#define VHOST_USER_PROTOCOL_F_LOG_SHMFD  1
#define VHOST_USER_PROTOCOL_F_RARP   2
+#define VHOST_USER_PROTOCOL_F_REPLY_ACK  3

Message types
-
@@ -464,3 +469,24 @@ Message types
  is present in VHOST_USER_GET_PROTOCOL_FEATURES.
  The first 6 bytes of the payload contain the mac address of the guest to
  allow the vhost user backend to construct and broadcast the fake RARP.
+
+VHOST_USER_PROTOCOL_F_REPLY_ACK:
+---
+The original vhost-user specification only demands replies for certain
+commands. This differs from the vhost protocol implementation where commands
+are sent over an ioctl() call and block until the client has completed.
+
+With this protocol extension negotiated, the sender (QEMU) can set the
+"need_reply" [Bit 3] flag to any command. This indicates that
+the client MUST respond with a Payload VhostUserMsg indicating success or
+failure. The payload should be set to zero on success or non-zero on failure.
+(Unless the message already has an explicit reply body)

Unless/unless (for consistency, the rest of the document doesn't use
Upper-case inside parentheses)

Actually, if the sentence starts inside the parenthesis it should be capital.
See rule 2a:
http://www.grammarbook.com/punctuation/parens.asp

Prerna's text looks correct to me. If it's wrong in other places we should 
probably fix it there separately.


+
+This indicates to QEMU that the requested operation has deterministically
+been met or not. Today, QEMU is expected to terminate the main vhost-user
+loop upon receiving such errors. In future, qemu could be taught to be more
+resilient for selective requests.
+
+For the message types that already solicit a reply from the client, the
+presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings
+no behaviourial change. (See the 'Communication' section for details.)

See/see

Same as my comment above.

Cheers,
Felipe


diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..86e7ae0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
VHOST_USER_PROTOCOL_F_MQ = 0,
VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
VHOST_USER_PROTOCOL_F_RARP = 2,
+VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,

VHOST_USER_PROTOCOL_F_MAX
};
@@ -84,6 +85,7 @@ typedef struct VhostUserMsg {

#define VHOST_USER_VERSION_MASK (0x3)
#define VHOST_USER_REPLY_MASK   (0x1<<2)
+#define VHOST_USER_NEED_REPLY_MASK   (0x1 << 3)

You could align it, and use the same style as the line above

uint32_t flags;
uint32_t size; /* the following payload size */
union {
@@ -158,6 +160,25 @@ fail:
return -1;
}

+static int process_message_reply(struct vhost_dev *dev,
+VhostUserRequest request)

bad indentation

+{
+VhostUserMsg msg;
+
+if (vhost_user_read(dev, ) < 0) {
+return 0;

return -1

+}
+
+if (msg.request != request) {
+error_report("Received unexpected msg type."
+"Expected %d received %d",
+request, msg.request);

bad 

Re: [Qemu-devel] [RFC PATCH 0/3] add nvdimm support on AArch64 virt platform

[Peter Maydell]

On 26 July 2016 at 07:32, kwangwoo@sk.com  wrote:
> NVDIMM stands for Non-Volatile DIMM which has DIMM form factor like PC-DIMM
> (Memory Module used in PC) and can be used like a memory, but the stored data
> is preserved on power failure. So it can be used as a memory, a storage, or
> backup memory on power failure.
>
> NVDIMM-N is already out in the market and Intel, HPE, and other companies
> actively developing related software. In Linux kernel, several usages exist
> like PMEM, BLK-aperture, and mixed mode. They are actively under developing.
>
> In QEMU, NVDIMM has been emulated on i386 platform. My patch series tries to
> enable the environment in AArch64 platform. Related Linux kernel patches were
> posted into the mailing list and they are under review/revise process 
> currently.

Thanks. I think I've now made my comments on this series (mostly
pretty minor ones). I'd appreciate it if QEMU devs with knowledge about
the NVDIMM emulation code we already have did a review pass on the
code as well...

-- PMM

Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt: add hotplug memory support

[Peter Maydell]

On 20 July 2016 at 01:49, Kwangwoo Lee  wrote:
> Add hotplug memory feature on aarch64 virt platfom. This patch is
> required to emulate a DIMM type memory like NVDIMM.
>
> Signed-off-by: Kwangwoo Lee 
> ---
>  default-configs/aarch64-softmmu.mak |   1 +
>  hw/arm/virt.c   | 110 
> 
>  include/hw/arm/virt.h   |   3 +
>  3 files changed, 114 insertions(+)
>
> diff --git a/default-configs/aarch64-softmmu.mak 
> b/default-configs/aarch64-softmmu.mak
> index 2449483..5790cd2 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -7,3 +7,4 @@ CONFIG_AUX=y
>  CONFIG_DDC=y
>  CONFIG_DPCD=y
>  CONFIG_XLNX_ZYNQMP=y
> +CONFIG_MEM_HOTPLUG=y
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a193b5a..f7ff411 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -58,6 +58,8 @@
>  #include "hw/smbios/smbios.h"
>  #include "qapi/visitor.h"
>  #include "standard-headers/linux/input.h"
> +#include "hw/mem/pc-dimm.h"
> +#include "hw/acpi/acpi.h"
>
>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -91,6 +93,7 @@ typedef struct {
>  bool secure;
>  bool highmem;
>  int32_t gic_version;
> +MemoryHotplugState hotplug_memory;
>  } VirtMachineState;
>
>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> @@ -1376,6 +1379,40 @@ static void machvirt_init(MachineState *machine)
>  guest_info_state->machine_done.notify = virt_guest_info_machine_done;
>  qemu_add_machine_init_done_notifier(_info_state->machine_done);
>
> +/* initialize hotplug memory address space */
> +if (machine->ram_size < machine->maxram_size) {
> +ram_addr_t hotplug_mem_size = machine->maxram_size - 
> machine->ram_size;
> +
> +if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
> +error_report("unsupported amount of memory slots: %"PRIu64,

"number of"

> + machine->ram_slots);
> +exit(EXIT_FAILURE);
> +}
> +
> +if (QEMU_ALIGN_UP(machine->maxram_size,
> + TARGET_PAGE_SIZE) != machine->maxram_size) {
> +error_report("maximum memory size must by aligned to multiple of 
> "

"must be"

> + "%d bytes", TARGET_PAGE_SIZE);
> +exit(EXIT_FAILURE);
> +}
> +
> +vms->hotplug_memory.base =
> + ROUND_UP(vbi->memmap[VIRT_MEM].base + machine->ram_size,
> +  ARCH_VIRT_HOTPLUG_MEM_ALIGN);
> +
> +if ((vms->hotplug_memory.base + hotplug_mem_size) <
> +hotplug_mem_size) {

This expression is confusing. Is it trying to test for overflow?
When can that happen?

> +error_report("unsupported amount of maximum memory: " 
> RAM_ADDR_FMT,
> + machine->maxram_size);
> +exit(EXIT_FAILURE);
> +}
> +
> +memory_region_init(>hotplug_memory.mr, OBJECT(vms),
> +   "hotplug-memory", hotplug_mem_size);
> +memory_region_add_subregion(sysmem, vms->hotplug_memory.base,
> +>hotplug_memory.mr);
> +}
> +
>  vbi->bootinfo.ram_size = machine->ram_size;
>  vbi->bootinfo.kernel_filename = machine->kernel_filename;
>  vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -1448,9 +1485,75 @@ static void virt_set_gic_version(Object *obj, const 
> char *value, Error **errp)
>  }
>  }
>
> +static void virt_dimm_plug(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> +VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +PCDIMMDevice *dimm = PC_DIMM(dev);
> +PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +MemoryRegion *mr = ddc->get_memory_region(dimm);
> +Error *local_err = NULL;
> +uint64_t align = memory_region_get_alignment(mr);
> +
> +pc_dimm_memory_plug(dev, >hotplug_memory, mr, align, _err);
> +if (local_err) {
> +goto out;
> +}
> +
> +out:
> +error_propagate(errp, local_err);
> +}
> +
> +static void virt_dimm_unplug(HotplugHandler *hotplug_dev,
> +   DeviceState *dev, Error **errp)
> +{
> +VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +PCDIMMDevice *dimm = PC_DIMM(dev);
> +PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> +MemoryRegion *mr = ddc->get_memory_region(dimm);
> +Error *local_err = NULL;
> +
> +pc_dimm_memory_unplug(dev, >hotplug_memory, mr);
> +object_unparent(OBJECT(dev));
> +
> +error_propagate(errp, local_err);
> +}
> +
> +static void virt_machinedevice_plug_cb(HotplugHandler *hotplug_dev,
> +  DeviceState *dev, Error **errp)
> +{
> +if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +virt_dimm_plug(hotplug_dev, dev, errp);
> +} else {
> +

Re: [Qemu-devel] [PATCH v9 8/8] docs: Add a generic loader explanation document

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> Signed-off-by: Alistair Francis 
> ---
> V9:
>  - Clarify the image loading options
> V8:
>  - Improve documentation
> V6:
>  - Fixup documentation
> V4:
>  - Re-write to be more comprehensive
>
>  docs/generic-loader.txt | 63 
> +
>  1 file changed, 63 insertions(+)
>  create mode 100644 docs/generic-loader.txt
>
> diff --git a/docs/generic-loader.txt b/docs/generic-loader.txt
> new file mode 100644
> index 000..16c11ee
> --- /dev/null
> +++ b/docs/generic-loader.txt
> @@ -0,0 +1,63 @@
> +Copyright (c) 2016 Xilinx Inc.
> +
> +This work is licensed under the terms of the GNU GPL, version 2 or later.  
> See
> +the COPYING file in the top-level directory.
> +
> +
> +The 'loader' device allows the user to load multiple images or values into
> +QEMU at startup.
> +
> +Loading Memory Values
> +-
> +The loader device allows memory values to be set from the command line. This
> +can be done by following the syntax below:
> +
> +-device loader,addr=,data=,data-len=
> +-device loader,addr=,cpu-num=
> +
> +  - The address to store the data or the value to use as the
> +  CPU's PC.
> +  - The value to be written to the address. The maximum size of
> +  the data is 8 bytes.
> +  - The length of the data in bytes. This argument must be
> +  included if the data argument is.
> +   - Set to true if the data to be stored on the guest should be
> +  written as big endian data. The default is to write little
> +  endian data.
> +   - This will cause the CPU to be reset and the PC to be set to
> +  the value of addr.
> +
> +For all values both hex and decimal values are allowed. By default the values
> +will be parsed as decimal. To use hex values the user should prefix the 
> number
> +with a '0x'.
> +
> +An example of loading value 0x800e to address 0xfd1a0104 is:
> +-device loader,addr=0xfd1a0104,data=0x800e,data-len=4
> +
> +Loading Files
> +-
> +The loader device also allows files to be loaded into memory. This can be 
> done
> +similarly to setting memory values. The syntax is shown below:
> +
> +-device loader,file=,addr=,cpu-num=,force-raw=
> +
> +  - A file to be loaded into memory
> +  - The addr in memory that the file should be loaded. This is
> +  ignored if you are using an ELF (unless force-raw is true).
> +  This is required if you aren't loading an ELF.
> +   - This specifies the CPU that should be used. This is an
> +  optional argument and will cause the CPU's PC to be set to
> +  where the image is stored or in the case of an ELF file to
> +  the value in the header. This option should only be used
> +  for the boot image.
> +  This will also cause the image to be written to the 
> specified
> +  CPUs address space.
> + - Forces the file to be treated as a raw image. This can be
> +  used to specify the load address of ELF files.
> +
> +For all values both hex and decimal values are allowed. By default the values
> +will be parsed as decimal. To use hex values the user should prefix the 
> number
> +with a '0x'.
> +
> +An example of loading an ELF file which CPU0 will boot is shown below:
> +-device loader,file=./images/boot.elf,cpu-num=0

With this interface, you can specify a file to be loaded to CPU 2's
address space (via file=whatever,cpu-num=2) but you can't specify
a data value to be loaded to CPU 2's address space (because
addr=a,data=x,data-len=y,cpu-num=2 isn't valid). I think we could
usefully make that syntax do that.

This is probably most clearly documented by completely splitting
 -device loader,addr=,cpu-num=
   (set the PC)

from
 -device 
loader,addr=,data=,data-len=[,data-be=][,cpu-num=]
   (load raw data values)

in the documentation in the same way that loader,file is split.

(I have a feeling we've been circling around on this option syntax
over the last few revisions so apologies if I've been inconsistent.)

You also can't specify a file to be loaded which doesn't set the
CPU PC, but I'm not sure how important that is. We can always add
it later with a no-set-pc option flag if we need it I guess.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 6/8] loader: Add AddressSpace loading support to targphys

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> Add a new function load_image_targphys_as() that allows the caller
> to specify an AddressSpace to use when loading a targphys. The
> original load_image_targphys() function doesn't have any change in
> functionality.
>
> Signed-off-by: Alistair Francis 
> ---
>
>  hw/core/loader.c| 10 --
>  include/hw/loader.h |  5 +
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 861dbc2..31a2d4a 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -133,10 +133,16 @@ ssize_t read_targphys(const char *name,
>  return did;
>  }
>
> -/* return the size or -1 if error */
>  int load_image_targphys(const char *filename,
>  hwaddr addr, uint64_t max_sz)
>  {
> +return load_image_targphys_as(filename, addr, max_sz, NULL);
> +}
> +
> +/* return the size or -1 if error */
> +int load_image_targphys_as(const char *filename,
> +   hwaddr addr, uint64_t max_sz, AddressSpace *as)
> +{
>  int size;
>
>  size = get_image_size(filename);
> @@ -144,7 +150,7 @@ int load_image_targphys(const char *filename,
>  return -1;
>  }
>  if (size > 0) {
> -rom_add_file_fixed(filename, addr, -1);
> +rom_add_file_fixed_as(filename, addr, -1, as);
>  }
>  return size;
>  }
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index ede98f6..1a9053f 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -16,6 +16,9 @@ int load_image(const char *filename, uint8_t *addr); /* 
> deprecated */
>  ssize_t load_image_size(const char *filename, void *addr, size_t size);
>  int load_image_targphys(const char *filename, hwaddr,
>  uint64_t max_sz);
> +int load_image_targphys_as(const char *filename,
> +   hwaddr addr, uint64_t max_sz, AddressSpace *as);
> +

Again, code changes are fine but could we have a doc comment?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 5/8] loader: Add AddressSpace loading support to uImages

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> Add a new function load_uimage_as() that allows the caller to
> specify an AddressSpace to use when loading the uImage. The
> original load_uimage() function doesn't have any change in
> functionality.
>
> Signed-off-by: Alistair Francis 
> ---
>
>  hw/core/loader.c| 17 +
>  include/hw/loader.h |  6 ++
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0f69894..861dbc2 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -581,7 +581,7 @@ static ssize_t gunzip(void *dst, size_t dstlen, uint8_t 
> *src,
>  static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr 
> *loadaddr,
>  int *is_linux, uint8_t image_type,
>  uint64_t (*translate_fn)(void *, uint64_t),
> -void *translate_opaque)
> +void *translate_opaque, AddressSpace *as)
>  {
>  int fd;
>  int size;
> @@ -682,7 +682,7 @@ static int load_uboot_image(const char *filename, hwaddr 
> *ep, hwaddr *loadaddr,
>  hdr->ih_size = bytes;
>  }
>
> -rom_add_blob_fixed(filename, data, hdr->ih_size, address);
> +rom_add_blob_fixed_as(filename, data, hdr->ih_size, address, as);
>
>  ret = hdr->ih_size;
>
> @@ -698,14 +698,23 @@ int load_uimage(const char *filename, hwaddr *ep, 
> hwaddr *loadaddr,
>  void *translate_opaque)
>  {
>  return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> -translate_fn, translate_opaque);
> +translate_fn, translate_opaque, NULL);
> +}
> +
> +int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> +   int *is_linux,
> +   uint64_t (*translate_fn)(void *, uint64_t),
> +   void *translate_opaque, AddressSpace *as)
> +{
> +return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> +translate_fn, translate_opaque, as);
>  }
>
>  /* Load a ramdisk.  */
>  int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
>  {
>  return load_uboot_image(filename, NULL, , NULL, IH_TYPE_RAMDISK,
> -NULL, NULL);
> +NULL, NULL, NULL);
>  }
>
>  /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 36a16cc..ede98f6 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -108,6 +108,10 @@ int load_uimage(const char *filename, hwaddr *ep,
>  hwaddr *loadaddr, int *is_linux,
>  uint64_t (*translate_fn)(void *, uint64_t),
>  void *translate_opaque);
> +int load_uimage_as(const char *filename, hwaddr *ep,
> +   hwaddr *loadaddr, int *is_linux,
> +   uint64_t (*translate_fn)(void *, uint64_t),
> +   void *translate_opaque, AddressSpace *as);

The code changes are good, but could we have a doc comment for
this new function, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 4/8] loader: Add AddressSpace loading support to ELFs

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> Add a new function load_elf_as() that allows the caller to specify an
> AddressSpace to use when loading the ELF. The original load_elf()
> function doesn't have any change in functionality.
>
> Signed-off-by: Alistair Francis 
> ---
> V8:
>  - Introduce an RFC version of AddressSpace support
>
>  hw/core/loader.c | 16 ++--
>  include/hw/elf_ops.h |  5 +++--
>  include/hw/loader.h  | 16 +++-
>  3 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index a024133..0f69894 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -417,6 +417,18 @@ int load_elf(const char *filename, uint64_t 
> (*translate_fn)(void *, uint64_t),
>   uint64_t *highaddr, int big_endian, int elf_machine,
>   int clear_lsb, int data_swab)
>  {
> +return load_elf_as(filename, translate_fn, translate_opaque, pentry,
> +   lowaddr, highaddr, big_endian, elf_machine, clear_lsb,
> +   data_swab, NULL);
> +}
> +
> +/* return < 0 if error, otherwise the number of bytes loaded in memory */
> +int load_elf_as(const char *filename,
> +uint64_t (*translate_fn)(void *, uint64_t),
> +void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> +uint64_t *highaddr, int big_endian, int elf_machine,
> +int clear_lsb, int data_swab, AddressSpace *as)
> +{
>  int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>  uint8_t e_ident[EI_NIDENT];
>
> @@ -455,11 +467,11 @@ int load_elf(const char *filename, uint64_t 
> (*translate_fn)(void *, uint64_t),
>  if (e_ident[EI_CLASS] == ELFCLASS64) {
>  ret = load_elf64(filename, fd, translate_fn, translate_opaque, 
> must_swab,
>   pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> - data_swab);
> + data_swab, as);
>  } else {
>  ret = load_elf32(filename, fd, translate_fn, translate_opaque, 
> must_swab,
>   pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> - data_swab);
> + data_swab, as);
>  }
>
>   fail:
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 1339677..3b8c9e9 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -263,7 +263,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>void *translate_opaque,
>int must_swab, uint64_t *pentry,
>uint64_t *lowaddr, uint64_t *highaddr,
> -  int elf_machine, int clear_lsb, int data_swab)
> +  int elf_machine, int clear_lsb, int data_swab,
> +  AddressSpace *as)
>  {
>  struct elfhdr ehdr;
>  struct elf_phdr *phdr = NULL, *ph;
> @@ -405,7 +406,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>  snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
>
>  /* rom_add_elf_program() seize the ownership of 'data' */
> -rom_add_elf_program(label, data, file_size, mem_size, addr, 
> NULL);
> +rom_add_elf_program(label, data, file_size, mem_size, addr, as);
>
>  total_size += mem_size;
>  if (addr < low)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index a701423..36a16cc 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -45,7 +45,7 @@ int load_image_gzipped(const char *filename, hwaddr addr, 
> uint64_t max_sz);
>  #define ELF_LOAD_WRONG_ENDIAN -4
>  const char *load_elf_strerror(int error);
>
> -/** load_elf:
> +/** load_elf_as:
>   * @filename: Path of ELF file
>   * @translate_fn: optional function to translate load addresses
>   * @translate_opaque: opaque data passed to @translate_fn
> @@ -59,6 +59,8 @@ const char *load_elf_strerror(int error);
>   * @data_swab: Set to order of byte swapping for data. 0 for no swap, 1
>   * for swapping bytes within halfwords, 2 for bytes within
>   * words and 3 for within doublewords.
> + * @as: The AddressSpace to load the ELF to. The value of 
> address_space_memory
> + *  is used if nothing is supplied here.

"if @as is NULL".

>   *
>   * Load an ELF file's contents to the emulated system's address space.
>   * Clients may optionally specify a callback to perform address
> @@ -73,6 +75,16 @@ const char *load_elf_strerror(int error);
>   * machine type.
>   */
>
> +int load_elf_as(const char *filename,
> +uint64_t (*translate_fn)(void *, uint64_t),
> +void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> +uint64_t *highaddr, int big_endian, int elf_machine,
> +

Re: [Qemu-devel] [PATCH v9 2/8] loader: Use the specified MemoryRegion

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> Prevously the specified MemoryRegion was ignored during the rom register
> reset. This patch uses the rom MemoryRegion is avaliable.

"if available"

>
> Signed-off-by: Alistair Francis 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

[Qemu-devel] no mouse events with gtk,gl=on

[Rob Herring]

I'm running current master of QEMU using virgl and display backend
"gtk,gl=on". I get no guest mouse events unless I force CONFIG_GTK_GL
off or make the following patch. Key events work fine. I'm on ubuntu
16.04 with GTK 3.18 (ubuntu 15.10/GTK 3.16 also had the same issue).
I've had this problem for some time, but just getting around to trying
to debug it further.

@@ -1373,8 +1378,8 @@ static void gd_grab_devices(VirtualConsole *vc, bool grab,
 }
 if (grab) {
 GdkWindow *win = gtk_widget_get_window(vc->gfx.drawing_area);
-gdk_device_grab(dev, win, GDK_OWNERSHIP_NONE, FALSE,
-mask, cursor, GDK_CURRENT_TIME);
+gdk_device_grab(dev, win, GDK_OWNERSHIP_NONE, TRUE,
+0, cursor, GDK_CURRENT_TIME);
 } else {
 gdk_device_ungrab(dev, GDK_CURRENT_TIME);
 }

Any ideas or suggestions how to debug this further?

Rob

Re: [Qemu-devel] [PATCH v2 2/2] vfio : add aer process

[Alex Williamson]

On Tue, 19 Jul 2016 15:32:43 +0800
Zhou Jie  wrote:

> From: Chen Fan 
> 
> During aer err occurs and resume do following to
> protect device from being accessed.
> 1. Make config space read only.
> 2. Disable INTx/MSI Interrupt.
> 3. Do nothing for bar regions.
> 
> Signed-off-by: Zhou Jie 
> ---
>  drivers/vfio/pci/vfio_pci.c | 30 ++
>  drivers/vfio/pci/vfio_pci_private.h |  2 ++
>  include/uapi/linux/vfio.h   |  2 ++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 2d12b03..dd96b60 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -318,6 +318,7 @@ static int vfio_pci_open(void *device_data)
>   return -ENODEV;
>  
>   mutex_lock(_lock);
> + init_completion(>aer_error_completion);
>  
>   if (!vdev->refcnt) {
>   ret = vfio_pci_enable(vdev);
> @@ -571,6 +572,16 @@ static long vfio_pci_ioctl(void *device_data,
>   struct vfio_pci_device *vdev = device_data;
>   unsigned long minsz;
>  
> + if (vdev->aer_error_in_progress && (cmd == VFIO_DEVICE_SET_IRQS ||
> + cmd == VFIO_DEVICE_RESET || cmd == VFIO_DEVICE_PCI_HOT_RESET)) {
> + int ret;
> + ret = wait_for_completion_interruptible(
> + >aer_error_completion);
> + if (ret) {
> + return ret;
> + }

No brackets necessary.

> + }
> +
>   if (cmd == VFIO_DEVICE_GET_INFO) {
>   struct vfio_device_info info;
>  
> @@ -587,6 +598,10 @@ static long vfio_pci_ioctl(void *device_data,
>   if (vdev->reset_works)
>   info.flags |= VFIO_DEVICE_FLAGS_RESET;
>  
> + info.flags |= VFIO_DEVICE_FLAGS_AERPROCESS;
> + if (vdev->aer_error_in_progress)
> + info.flags |= VFIO_DEVICE_FLAGS_INAERPROCESS;
> +
>   info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions;
>   info.num_irqs = VFIO_PCI_NUM_IRQS;
>  
> @@ -996,6 +1011,14 @@ static ssize_t vfio_pci_rw(void *device_data, char 
> __user *buf,
>  
>   switch (index) {
>   case VFIO_PCI_CONFIG_REGION_INDEX:
> + if (vdev->aer_error_in_progress && iswrite) {
> + int ret;
> + ret = wait_for_completion_interruptible(
> + >aer_error_completion);
> + if (ret) {
> + return ret;
> + }
> + }
>   return vfio_pci_config_rw(vdev, buf, count, ppos, iswrite);
>  
>   case VFIO_PCI_ROM_REGION_INDEX:
> @@ -1226,6 +1249,10 @@ static pci_ers_result_t 
> vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  
>   mutex_lock(>igate);
>  
> + vdev->aer_error_in_progress = true;
> + vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE |
> + VFIO_IRQ_SET_ACTION_TRIGGER,
> + vdev->irq_type, 0, 0, NULL);
>   if (vdev->err_trigger)
>   eventfd_signal(vdev->err_trigger, 1);
>  
> @@ -1252,6 +1279,9 @@ static void vfio_pci_aer_resume(struct pci_dev *pdev)
>   }
>  
>   mutex_lock(>igate);
> +
> + vdev->aer_error_in_progress = false;
> + complete_all(>aer_error_completion);
>   if (vdev->resume_trigger)
>   eventfd_signal(vdev->resume_trigger, 1);
>  
> diff --git a/drivers/vfio/pci/vfio_pci_private.h 
> b/drivers/vfio/pci/vfio_pci_private.h
> index 80d4ddd..2f151f5 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -84,6 +84,8 @@ struct vfio_pci_device {
>   boolhas_vga;
>   boolneeds_reset;
>   boolnointx;
> + boolaer_error_in_progress;
> + struct completion   aer_error_completion;
>   struct pci_saved_state  *pci_saved_state;
>   int refcnt;
>   struct eventfd_ctx  *err_trigger;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 34ab138..276ce50 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -198,6 +198,8 @@ struct vfio_device_info {
>  #define VFIO_DEVICE_FLAGS_PCI(1 << 1)/* vfio-pci device */
>  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
>  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3) /* vfio-amba device */
> +#define VFIO_DEVICE_FLAGS_AERPROCESS  (1 << 4)   /* support aer error 
> progress */
> +#define VFIO_DEVICE_FLAGS_INAERPROCESS  (1 << 5)/* status in aer error 
> progress */
>   __u32   num_regions;/* Max region index + 1 */
>   __u32   num_irqs;   /* Max IRQ index + 1 */
>  };

Clearly this has only been tested for a single instance of an AER error

Re: [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier

[Alex Williamson]

On Tue, 19 Jul 2016 15:52:45 +0800
Zhou Jie  wrote:

> From: Chen Fan 

An empty commit log is unacceptable for all but the most trivial
patches.

There's also no sign-off on this patch.

I also don't know why we need this since you previously found that for
QEMU, ordering of error versus resume notifications is not guaranteed,
which is why I thought we went with a status flag within the struct
vfio_device_info.  I'm not adding an interrupt to the user that has
no users.  This was not part of the most recent discussion we had about
this, so I'm lost why this patch exists. Thanks,

Alex

> ---
>  drivers/vfio/pci/vfio_pci.c | 28 +++-
>  drivers/vfio/pci/vfio_pci_intrs.c   | 18 ++
>  drivers/vfio/pci/vfio_pci_private.h |  1 +
>  include/uapi/linux/vfio.h   |  1 +
>  4 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 188b1ff..2d12b03 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -363,7 +363,8 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device 
> *vdev, int irq_type)
>  
>   return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
>   }
> - } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX) {
> + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX ||
> +irq_type == VFIO_PCI_RESUME_IRQ_INDEX) {
>   if (pci_is_pcie(vdev->pdev))
>   return 1;
>   } else if (irq_type == VFIO_PCI_REQ_IRQ_INDEX) {
> @@ -731,6 +732,7 @@ static long vfio_pci_ioctl(void *device_data,
>   case VFIO_PCI_REQ_IRQ_INDEX:
>   break;
>   case VFIO_PCI_ERR_IRQ_INDEX:
> + case VFIO_PCI_RESUME_IRQ_INDEX:
>   if (pci_is_pcie(vdev->pdev))
>   break;
>   /* pass thru to return error */
> @@ -1234,8 +1236,32 @@ static pci_ers_result_t 
> vfio_pci_aer_err_detected(struct pci_dev *pdev,
>   return PCI_ERS_RESULT_CAN_RECOVER;
>  }
>  
> +static void vfio_pci_aer_resume(struct pci_dev *pdev)
> +{
> + struct vfio_pci_device *vdev;
> + struct vfio_device *device;
> +
> + device = vfio_device_get_from_dev(>dev);
> + if (device == NULL)
> + return;
> +
> + vdev = vfio_device_data(device);
> + if (vdev == NULL) {
> + vfio_device_put(device);
> + return;
> + }
> +
> + mutex_lock(>igate);
> + if (vdev->resume_trigger)
> + eventfd_signal(vdev->resume_trigger, 1);
> +
> + mutex_unlock(>igate);
> + vfio_device_put(device);
> +}
> +
>  static const struct pci_error_handlers vfio_err_handlers = {
>   .error_detected = vfio_pci_aer_err_detected,
> + .resume = vfio_pci_aer_resume,
>  };
>  
>  static struct pci_driver vfio_pci_driver = {
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c 
> b/drivers/vfio/pci/vfio_pci_intrs.c
> index 15ecfc9..3a01a62 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -617,6 +617,16 @@ static int vfio_pci_set_err_trigger(struct 
> vfio_pci_device *vdev,
>   return vfio_pci_set_ctx_trigger_single(>err_trigger, flags, data);
>  }
>  
> +static int vfio_pci_set_resume_trigger(struct vfio_pci_device *vdev,
> + unsigned index, unsigned start,
> + unsigned count, uint32_t flags, void *data)
> +{
> + if (index != VFIO_PCI_RESUME_IRQ_INDEX)
> + return -EINVAL;
> +
> + return vfio_pci_set_ctx_trigger_single(>resume_trigger, flags, 
> data);
> +}
> +
>  static int vfio_pci_set_req_trigger(struct vfio_pci_device *vdev,
>   unsigned index, unsigned start,
>   unsigned count, uint32_t flags, void *data)
> @@ -676,6 +686,14 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_device 
> *vdev, uint32_t flags,
>   break;
>   }
>   break;
> + case VFIO_PCI_RESUME_IRQ_INDEX:
> + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
> + case VFIO_IRQ_SET_ACTION_TRIGGER:
> + if (pci_is_pcie(vdev->pdev))
> + func = vfio_pci_set_resume_trigger;
> + break;
> + }
> + break;
>   }
>  
>   if (!func)
> diff --git a/drivers/vfio/pci/vfio_pci_private.h 
> b/drivers/vfio/pci/vfio_pci_private.h
> index 016c14a..80d4ddd 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -88,6 +88,7 @@ struct vfio_pci_device {
>   int refcnt;
>   struct eventfd_ctx  *err_trigger;
>   struct eventfd_ctx  *req_trigger;
> + struct eventfd_ctx  *resume_trigger;
>  };
>  
>  #define is_intx(vdev) (vdev->irq_type 

[Qemu-devel] [PATCH] Fix bsd-user build errors after 8642c1b81e0418df066a7960a7426d85a923a253

[Sean Bruno]

  LINK  sparc-bsd-user/qemu-sparc
bsd-user/main.o: In function `cpu_loop':
/home/sbruno/bsd/qemu/bsd-user/main.c:515: undefined reference to 
`cpu_sparc_exec'
c++: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[1]: *** [Makefile:197: qemu-sparc] Error 1
gmake: *** [Makefile:204: subdir-sparc-bsd-user] Error 2

  LINK  i386-bsd-user/qemu-i386
bsd-user/main.o: In function `cpu_loop':
/home/sbruno/bsd/qemu/bsd-user/main.c:174: undefined reference to `cpu_x86_exec'
c++: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[1]: *** [Makefile:197: qemu-i386] Error 1
gmake: *** [Makefile:204: subdir-i386-bsd-user] Error 2

Signed-off-by:  Sean Bruno 
---
 bsd-user/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 315ba1d..bbba43f 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -171,7 +171,7 @@ void cpu_loop(CPUX86State *env)
 //target_siginfo_t info;
 
 for(;;) {
-trapnr = cpu_x86_exec(cs);
+trapnr = cpu_exec(cs);
 switch(trapnr) {
 case 0x80:
 /* syscall from int $0x80 */
@@ -512,7 +512,7 @@ void cpu_loop(CPUSPARCState *env)
 //target_siginfo_t info;
 
 while (1) {
-trapnr = cpu_sparc_exec(cs);
+trapnr = cpu_exec(cs);
 
 switch (trapnr) {
 #ifndef TARGET_SPARC64
-- 
2.9.2

Re: [Qemu-devel] [PATCH v9 1/8] loader: Allow ELF loader to auto-detect the ELF arch

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> If the caller didn't specify an architecture for the ELF machine
> the load_elf() function will auto detect it based on the ELF file.
>
> Signed-off-by: Alistair Francis 
> ---
> V9:
>  - Update documentation
> V8:
>  - Move into load_elf64/load_elf32
> V7:
>  - Fix typo
>
>  include/hw/elf_ops.h | 5 +
>  include/hw/loader.h  | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index f510e7e..db70c11 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -280,6 +280,11 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>  glue(bswap_ehdr, SZ)();
>  }
>
> +if (elf_machine < 1) {
> +/* The caller didn't specify an ARCH, we can figure it out */
> +elf_machine = ehdr.e_machine;
> +}
> +
>  switch (elf_machine) {
>  case EM_PPC64:
>  if (ehdr.e_machine != EM_PPC64) {
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 4879b63..fd540fc 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -68,6 +68,9 @@ const char *load_elf_strerror(int error);
>   * load will fail if the target ELF does not match. Some architectures
>   * have some architecture-specific behaviours that come into effect when
>   * their particular values for @elf_machine are set.
> + * If no @elf_machine is provided the machine will default to the value
> + * in the ELFs header and no checks will be carried out against the
> + * machine type.
>   */

The argument is mandatory, you can't not provide it.
Should we make this "if @elf_machine is EM_NONE then the
machine type will be read from the ELF header" ? (EM_NONE is 0).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 3/8] loader: Allow a custom AddressSpace when loading ROMs

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis  wrote:
> When loading ROMs allow the caller to specify an AddressSpace to use for
> the load.
>
> Signed-off-by: Alistair Francis 
> ---
> V9:
>  - Fixup the ROM ordering
>  - Don't allow address space and memory region to be specified
> V8:
>  - Introduce an RFC version of AddressSpace loading support
>
>  hw/core/loader.c | 39 ---
>  include/hw/elf_ops.h |  2 +-
>  include/hw/loader.h  | 10 ++
>  3 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6b61f29..a024133 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -777,6 +777,7 @@ struct Rom {
>
>  uint8_t *data;
>  MemoryRegion *mr;
> +AddressSpace *as;
>  int isrom;
>  char *fw_dir;
>  char *fw_file;
> @@ -796,12 +797,15 @@ static void rom_insert(Rom *rom)
>  hw_error ("ROM images must be loaded at startup\n");
>  }
>
> -/* list is ordered by load address */
> +/* List is ordered by load address in the same address space */
>  QTAILQ_FOREACH(item, , next) {
> -if (rom->addr >= item->addr)
> -continue;
> -QTAILQ_INSERT_BEFORE(item, rom, next);
> -return;
> +if (rom->addr >= item->addr && rom->as == item->as) {
> +QTAILQ_INSERT_AFTER(, item, rom, next);
> +return;
> +} else if (rom->addr <= item->addr && rom->as == item->as) {
> +QTAILQ_INSERT_BEFORE(item, rom, next);
> +return;
> +}
>  }
>  QTAILQ_INSERT_TAIL(, rom, next);

This seems a somewhat confusing way of writing this. I think you
should define a comparison function and then just replace the
current "rom->addr >= item->addr" with "rom_order_compare(rom, item) >= 0".
Then it's clear what the comparison you're using to define the
sorted order is and that the loop will put things in in sorted
position.

>  }
> @@ -833,16 +837,25 @@ static void *rom_set_mr(Rom *rom, Object *owner, const 
> char *name)
>
>  int rom_add_file(const char *file, const char *fw_dir,
>   hwaddr addr, int32_t bootindex,
> - bool option_rom, MemoryRegion *mr)
> + bool option_rom, MemoryRegion *mr,
> + AddressSpace *as)
>  {
>  MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>  Rom *rom;
>  int rc, fd = -1;
>  char devpath[100];
>
> +if (as && mr) {
> +fprintf(stderr, "Specifying an Address Space and Memory Region is " \
> +"not valid when loading a rom\n");

Some day we'll fix this up to use Errors, but that day need not be today.

> +/* We haven't allocated anything so we don't need any cleanup */
> +return -1;
> +}
> +
>  rom = g_malloc0(sizeof(*rom));
>  rom->name = g_strdup(file);
>  rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
> +rom->as = as;
>  if (rom->path == NULL) {
>  rom->path = g_strdup(file);
>  }
> @@ -969,7 +982,7 @@ MemoryRegion *rom_add_blob(const char *name, const void 
> *blob, size_t len,
>   * memory ownership of "data", so we don't have to allocate and copy the 
> buffer.
>   */
>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
> -size_t romsize, hwaddr addr)
> +size_t romsize, hwaddr addr, AddressSpace *as)
>  {
>  Rom *rom;
>
> @@ -979,18 +992,19 @@ int rom_add_elf_program(const char *name, void *data, 
> size_t datasize,
>  rom->datasize = datasize;
>  rom->romsize  = romsize;
>  rom->data = data;
> +rom->as   = as;
>  rom_insert(rom);
>  return 0;
>  }
>
>  int rom_add_vga(const char *file)
>  {
> -return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
> +return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
>  }
>
>  int rom_add_option(const char *file, int32_t bootindex)
>  {
> -return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
> +return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
>  }
>
>  static void rom_reset(void *unused)
> @@ -1008,7 +1022,8 @@ static void rom_reset(void *unused)
>  void *host = memory_region_get_ram_ptr(rom->mr);
>  memcpy(host, rom->data, rom->datasize);
>  } else {
> -cpu_physical_memory_write_rom(_space_memory,
> +cpu_physical_memory_write_rom(rom->as ? rom->as :
> +_space_memory,

Should we just make rom->as be _space_memory if the caller
doesn't provide one rather than having it be NULL and then special
casing what NULL means? (This will also avoid potentially odd
effects with some of the ROMs in the list having _space_memory
for an explicitly specified AS and some having NULL for the same
AS but implicitly specified, which is otherwise a 

[Qemu-devel] [PATCH v14 7/9] target-avr: adding instruction translation

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 target-avr/Makefile.objs|2 +-
 target-avr/translate-inst.c | 2622 +++
 target-avr/translate.c  |1 -
 target-avr/translate.h  |1 +
 4 files changed, 2624 insertions(+), 2 deletions(-)
 create mode 100644 target-avr/translate-inst.c

diff --git a/target-avr/Makefile.objs b/target-avr/Makefile.objs
index 2a10104..fdafcd5 100644
--- a/target-avr/Makefile.objs
+++ b/target-avr/Makefile.objs
@@ -18,6 +18,6 @@
 #  
 #
 
-obj-y   += translate.o cpu.o helper.o
+obj-y   += translate.o cpu.o helper.o translate-inst.o
 obj-y   += gdbstub.o
 obj-$(CONFIG_SOFTMMU) += machine.o
diff --git a/target-avr/translate-inst.c b/target-avr/translate-inst.c
new file mode 100644
index 000..d1dce42
--- /dev/null
+++ b/target-avr/translate-inst.c
@@ -0,0 +1,2622 @@
+/*
+ *  QEMU AVR CPU
+ *
+ *  Copyright (c) 2016 Michael Rolnik
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, see
+ *  
+ */
+
+#include "translate.h"
+#include "translate-inst.h"
+#include "qemu/bitops.h"
+
+static void gen_add_CHf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+TCGv t3 = tcg_temp_new_i32();
+
+tcg_gen_and_tl(t1, Rd, Rr); /*  t1 = Rd & Rr  */
+tcg_gen_andc_tl(t2, Rd, R); /*  t2 = Rd & ~R  */
+tcg_gen_andc_tl(t3, Rr, R); /*  t3 = Rr & ~R  */
+tcg_gen_or_tl(t1, t1, t2);  /*  t1 = t1 | t2 | t3  */
+tcg_gen_or_tl(t1, t1, t3);
+
+tcg_gen_shri_tl(cpu_Cf, t1, 7); /*  Cf = t1(7)  */
+tcg_gen_shri_tl(cpu_Hf, t1, 3); /*  Hf = t1(3)  */
+tcg_gen_andi_tl(cpu_Hf, cpu_Hf, 1);
+
+tcg_temp_free_i32(t3);
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+static void gen_add_Vf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+
+/*  t1 = Rd & Rr & ~R | ~Rd & ~Rr & R = (Rd ^ R) & ~(Rd ^ Rr)   */
+tcg_gen_xor_tl(t1, Rd, R);
+tcg_gen_xor_tl(t2, Rd, Rr);
+tcg_gen_andc_tl(t1, t1, t2);
+
+tcg_gen_shri_tl(cpu_Vf, t1, 7); /*  Vf = t1(7)  */
+
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+static void gen_sub_CHf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+TCGv t3 = tcg_temp_new_i32();
+
+/*  Cf & Hf  */
+tcg_gen_not_tl(t1, Rd); /*  t1 = ~Rd  */
+tcg_gen_and_tl(t2, t1, Rr); /*  t2 = ~Rd & Rr  */
+tcg_gen_or_tl(t3, t1, Rr);  /*  t3 = (~Rd | Rr) & R  */
+tcg_gen_and_tl(t3, t3, R);
+tcg_gen_or_tl(t2, t2, t3);  /*  t2 = ~Rd & Rr | ~Rd & R | R & Rr  
*/
+tcg_gen_shri_tl(cpu_Cf, t2, 7); /*  Cf = t2(7)  */
+tcg_gen_shri_tl(cpu_Hf, t2, 3); /*  Hf = t2(3)  */
+tcg_gen_andi_tl(cpu_Hf, cpu_Hf, 1);
+
+tcg_temp_free_i32(t3);
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+static void gen_sub_Vf(TCGv R, TCGv Rd, TCGv Rr)
+{
+TCGv t1 = tcg_temp_new_i32();
+TCGv t2 = tcg_temp_new_i32();
+
+/*  Vf  */
+/*  t1 = Rd & ~Rr & ~R | ~Rd & Rr & R  = (Rd ^ R) & (Rd ^ R)*/
+tcg_gen_xor_tl(t1, Rd, R);
+tcg_gen_xor_tl(t2, Rd, Rr);
+tcg_gen_and_tl(t1, t1, t2);
+tcg_gen_shri_tl(cpu_Vf, t1, 7); /*  Vf = t1(7)  */
+
+tcg_temp_free_i32(t2);
+tcg_temp_free_i32(t1);
+}
+
+static void gen_NSf(TCGv R)
+{
+tcg_gen_shri_tl(cpu_Nf, R, 7);  /*  Nf = R(7)  */
+tcg_gen_xor_tl(cpu_Sf, cpu_Nf, cpu_Vf); /*  Sf = Nf ^ Vf  */
+}
+
+static void gen_ZNSf(TCGv R)
+{
+tcg_gen_mov_tl(cpu_Zf, R);  /*  Zf = R  */
+tcg_gen_shri_tl(cpu_Nf, R, 7);  /*  Nf = R(7)  */
+tcg_gen_xor_tl(cpu_Sf, cpu_Nf, cpu_Vf); /*  Sf = Nf ^ Vf  */
+}
+
+static void gen_push_ret(CPUAVRState *env, int ret)
+{
+if (avr_feature(env, AVR_FEATURE_1_BYTE_PC)) {
+
+TCGv t0 = tcg_const_i32((ret & 0xff));
+
+tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_UB);
+tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
+
+tcg_temp_free_i32(t0);
+} else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
+
+TCGv t0 = tcg_const_i32((ret & 0x00));
+
+tcg_gen_subi_tl(cpu_sp, cpu_sp, 1);
+tcg_gen_qemu_st_tl(t0, cpu_sp, MMU_DATA_IDX, MO_BEUW);

[Qemu-devel] [PATCH v14 8/9] target-avr: instruction decoder generator

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 target-avr/cpugen/CMakeLists.txt   |  38 +++
 target-avr/cpugen/README.md|  17 ++
 target-avr/cpugen/cpu/avr.yaml | 214 ++
 target-avr/cpugen/src/CMakeLists.txt   |  63 
 target-avr/cpugen/src/cpugen.cpp   | 458 +
 target-avr/cpugen/src/utils.cpp|  27 ++
 target-avr/cpugen/src/utils.h  |  79 +
 target-avr/cpugen/xsl/decode.c.xsl | 103 +++
 target-avr/cpugen/xsl/translate-inst.h.xsl | 118 
 target-avr/cpugen/xsl/utils.xsl| 108 +++
 10 files changed, 1225 insertions(+)
 create mode 100644 target-avr/cpugen/CMakeLists.txt
 create mode 100644 target-avr/cpugen/README.md
 create mode 100644 target-avr/cpugen/cpu/avr.yaml
 create mode 100644 target-avr/cpugen/src/CMakeLists.txt
 create mode 100644 target-avr/cpugen/src/cpugen.cpp
 create mode 100644 target-avr/cpugen/src/utils.cpp
 create mode 100644 target-avr/cpugen/src/utils.h
 create mode 100644 target-avr/cpugen/xsl/decode.c.xsl
 create mode 100644 target-avr/cpugen/xsl/translate-inst.h.xsl
 create mode 100644 target-avr/cpugen/xsl/utils.xsl

diff --git a/target-avr/cpugen/CMakeLists.txt b/target-avr/cpugen/CMakeLists.txt
new file mode 100644
index 000..ded391c
--- /dev/null
+++ b/target-avr/cpugen/CMakeLists.txt
@@ -0,0 +1,38 @@
+cmake_minimum_required(VERSION 2.8)
+
+project(cpugen)
+
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -ggdb -g3")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
+
+set(Boost_USE_STATIC_LIBS   ON)
+find_package(
+Boost 1.60.0
+REQUIRED
+COMPONENTS
+system
+regex)
+#set(BUILD_SHARED_LIBS   OFF)
+#set(BUILD_STATIC_LIBS   ON)
+add_subdirectory(tinyxml2)
+add_subdirectory(yaml-cpp)
+
+include_directories(
+${CMAKE_CURRENT_SOURCE_DIR}
+${CMAKE_CURRENT_SOURCE_DIR}/..
+${CMAKE_CURRENT_SOURCE_DIR}/../yaml-cpp/include
+${Boost_INCLUDE_DIRS}
+)
+
+add_executable(
+cpugen
+src/cpugen.cpp
+src/utils.cpp
+)
+
+target_link_libraries(
+cpugen
+yaml-cpp
+tinyxml2
+${Boost_LIBRARIES}
+)
diff --git a/target-avr/cpugen/README.md b/target-avr/cpugen/README.md
new file mode 100644
index 000..f0caa8b
--- /dev/null
+++ b/target-avr/cpugen/README.md
@@ -0,0 +1,17 @@
+# CPUGEN
+## How to build
+within ```cpugen``` directory do
+```
+git clone https://github.com/leethomason/tinyxml2
+git clone https://github.com/jbeder/yaml-cpp
+mkdir build
+cd build
+cmake ..
+make
+```
+## How to use
+```
+cpugen ../cpu/avr.yaml
+xsltproc ../xsl/decode.c.xsl output.xml > ../../decode.c
+xsltproc ../xsl/translate-inst.h.xsl output.xml > ../../translate-inst.h
+```
diff --git a/target-avr/cpugen/cpu/avr.yaml b/target-avr/cpugen/cpu/avr.yaml
new file mode 100644
index 000..a626859
--- /dev/null
+++ b/target-avr/cpugen/cpu/avr.yaml
@@ -0,0 +1,214 @@
+cpu:
+name: avr
+instructions:
+- ADC:
+opcode: 0001 11 hRr[1] Rd[5] lRr[4]
+- ADD:
+opcode:  11 hRr[1] Rd[5] lRr[4]
+- ADIW:
+opcode: 1001 0110 hImm[2] Rd[2] lImm[4]
+- AND:
+opcode: 0010 00 hRr[1] Rd[5] lRr[4]
+- ANDI:
+opcode: 0111 hImm[4] Rd[4] lImm[4]
+- ASR:
+opcode: 1001 010 Rd[5] 0101
+- BCLR:
+opcode: 1001 0100 1 Bit[3] 1000
+- BLD:
+opcode:  100 Rd[5] 0 Bit[3]
+- BRBC:
+opcode:  01 Imm[7] Bit[3]
+- BRBS:
+opcode:  00 Imm[7] Bit[3]
+- BREAK:
+opcode: 1001 0101 1001 1000
+- BSET:
+opcode: 1001 0100 0 Bit[3] 1000
+- BST:
+opcode:  101 Rd[5] 0 Bit[3]
+- CALL:
+opcode: 1001 010 hImm[5] 111 lImm[17]
+- CBI:
+opcode: 1001 1000 Imm[5] Bit[3]
+- COM:
+opcode: 1001 010 Rd[5] 
+- CP:
+opcode: 0001 01 hRr[1] Rd[5] lRr[4]
+- CPC:
+opcode:  01 hRr[1] Rd[5] lRr[4]
+- CPI:
+opcode: 0011 hImm[4] Rd[4] lImm[4]
+- CPSE:
+opcode: 0001 00 hRr[1] Rd[5] lRr[4]
+- DEC:
+opcode: 1001 010 Rd[5] 1010
+- DES:
+opcode: 1001 0100 Imm[4] 1011
+- EICALL:
+opcode: 1001 0101 0001 1001
+- EIJMP:
+opcode: 1001 0100 0001 1001
+- ELPM1:
+opcode: 1001 0101 1101 1000
+- ELPM2:
+opcode: 1001 000 Rd[5] 0110
+- ELPMX:
+opcode: 1001 000 Rd[5] 0111
+- EOR:
+opcode: 0010 01 hRr[1] Rd[5] lRr[4]
+- FMUL:
+opcode:  0011 0 Rd[3] 1 Rr[3]
+- FMULS:
+opcode:  0011 1 Rd[3] 0 Rr[3]
+- FMULSU:
+opcode:  0011 1 Rd[3] 1 Rr[3]
+- ICALL:
+opcode: 1001 0101  1001
+- IJMP:
+

[Qemu-devel] [PATCH v14 4/9] target-avr: adding instructions encodings

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 target-avr/translate-inst.h | 805 
 1 file changed, 805 insertions(+)
 create mode 100644 target-avr/translate-inst.h

diff --git a/target-avr/translate-inst.h b/target-avr/translate-inst.h
new file mode 100644
index 000..9dc22a0
--- /dev/null
+++ b/target-avr/translate-inst.h
@@ -0,0 +1,805 @@
+/*
+ *  QEMU AVR CPU
+ *
+ *  Copyright (c) 2016 Michael Rolnik
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, see
+ *  
+ */
+
+#ifndef AVR_TRANSLATE_INST_H_
+#define AVR_TRANSLATE_INST_H_
+
+typedef struct DisasContext DisasContext;
+
+int avr_translate_NOP(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+
+int avr_translate_MOVW(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t MOVW_Rr(uint32_t opcode)
+{
+return extract32(opcode, 0, 4);
+}
+
+static inline uint32_t MOVW_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 4);
+}
+
+int avr_translate_MULS(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t MULS_Rr(uint32_t opcode)
+{
+return extract32(opcode, 0, 4);
+}
+
+static inline uint32_t MULS_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 4);
+}
+
+int avr_translate_MULSU(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t MULSU_Rr(uint32_t opcode)
+{
+return extract32(opcode, 0, 3);
+}
+
+static inline uint32_t MULSU_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 3);
+}
+
+int avr_translate_FMUL(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t FMUL_Rr(uint32_t opcode)
+{
+return extract32(opcode, 0, 3);
+}
+
+static inline uint32_t FMUL_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 3);
+}
+
+int avr_translate_FMULS(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t FMULS_Rr(uint32_t opcode)
+{
+return extract32(opcode, 0, 3);
+}
+
+static inline uint32_t FMULS_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 3);
+}
+
+int avr_translate_FMULSU(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t FMULSU_Rr(uint32_t opcode)
+{
+return extract32(opcode, 0, 3);
+}
+
+static inline uint32_t FMULSU_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 3);
+}
+
+int avr_translate_CPC(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t CPC_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 5);
+}
+
+static inline uint32_t CPC_Rr(uint32_t opcode)
+{
+return (extract32(opcode, 9, 1) << 4) |
+(extract32(opcode, 0, 4));
+}
+
+int avr_translate_SBC(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t SBC_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 5);
+}
+
+static inline uint32_t SBC_Rr(uint32_t opcode)
+{
+return (extract32(opcode, 9, 1) << 4) |
+(extract32(opcode, 0, 4));
+}
+
+int avr_translate_ADD(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t ADD_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 5);
+}
+
+static inline uint32_t ADD_Rr(uint32_t opcode)
+{
+return (extract32(opcode, 9, 1) << 4) |
+(extract32(opcode, 0, 4));
+}
+
+int avr_translate_AND(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t AND_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 5);
+}
+
+static inline uint32_t AND_Rr(uint32_t opcode)
+{
+return (extract32(opcode, 9, 1) << 4) |
+(extract32(opcode, 0, 4));
+}
+
+int avr_translate_EOR(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t EOR_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 5);
+}
+
+static inline uint32_t EOR_Rr(uint32_t opcode)
+{
+return (extract32(opcode, 9, 1) << 4) |
+(extract32(opcode, 0, 4));
+}
+
+int avr_translate_OR(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t OR_Rd(uint32_t opcode)
+{
+return extract32(opcode, 4, 5);
+}
+
+static inline uint32_t OR_Rr(uint32_t opcode)
+{
+return (extract32(opcode, 9, 1) << 4) |
+(extract32(opcode, 0, 4));
+}
+
+int avr_translate_MOV(CPUAVRState *env, DisasContext* ctx, uint32_t opcode);
+static inline uint32_t MOV_Rd(uint32_t opcode)
+{
+return 

[Qemu-devel] [PATCH v14 6/9] target-avr: adding helpers for IN, OUT, SLEEP, WBR & unsupported instructions

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 target-avr/cpu.h   |  13 ++-
 target-avr/helper.c| 288 ++---
 target-avr/helper.h|   8 +-
 target-avr/translate.c |   9 ++
 4 files changed, 300 insertions(+), 18 deletions(-)

diff --git a/target-avr/cpu.h b/target-avr/cpu.h
index 9ef8c41..85c48f1 100644
--- a/target-avr/cpu.h
+++ b/target-avr/cpu.h
@@ -146,6 +146,7 @@ struct CPUAVRState {
 uint32_t sp; /* 16 bits */
 
 uint64_t intsrc; /* interrupt sources */
+bool fullacc;/* CPU/MEM if true MEM only otherwise */
 
 uint32_t features;
 
@@ -188,12 +189,22 @@ int avr_cpu_handle_mmu_fault(CPUState *cpu, vaddr 
address, int rw,
 int avr_cpu_memory_rw_debug(CPUState *cs, vaddr address, uint8_t *buf,
 int len, bool is_write);
 
+enum {
+TB_FLAGS_FULL_ACCESS = 1,
+};
 static inline void cpu_get_tb_cpu_state(CPUAVRState *env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *pflags)
 {
+uint32_t flags = 0;
+
 *pc = env->pc_w * 2;
 *cs_base = 0;
-*pflags = 0;
+
+if (env->fullacc) {
+flags |= TB_FLAGS_FULL_ACCESS;
+}
+
+*pflags = flags;
 }
 
 static inline int cpu_interrupts_enabled(CPUAVRState *env)
diff --git a/target-avr/helper.c b/target-avr/helper.c
index 1e5d97d..9635d38 100644
--- a/target-avr/helper.c
+++ b/target-avr/helper.c
@@ -28,6 +28,7 @@
 #include "exec/cpu_ldst.h"
 #include "qemu/host-utils.h"
 #include "exec/helper-proto.h"
+#include "exec/ioport.h"
 
 bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
@@ -42,14 +43,14 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 cs->exception_index = EXCP_RESET;
 cc->do_interrupt(cs);
 
-cs->interrupt_request   &= ~CPU_INTERRUPT_RESET;
+cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
 
 ret = true;
 }
 }
 if (interrupt_request & CPU_INTERRUPT_HARD) {
 if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
-int index = ctz32(env->intsrc);
+int index = ctz32(env->intsrc);
 cs->exception_index = EXCP_INT(index);
 cc->do_interrupt(cs);
 
@@ -64,13 +65,13 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
 
 void avr_cpu_do_interrupt(CPUState *cs)
 {
-AVRCPU *cpu = AVR_CPU(cs);
-CPUAVRState*env = >env;
+AVRCPU *cpu = AVR_CPU(cs);
+CPUAVRState *env = >env;
 
 uint32_t ret = env->pc_w;
 int vector = 0;
 int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
-int base = 0;/* TODO: where to get it */
+int base = 0; /* TODO: where to get it */
 
 if (cs->exception_index == EXCP_RESET) {
 vector = 0;
@@ -79,18 +80,18 @@ void avr_cpu_do_interrupt(CPUState *cs)
 }
 
 if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
-stb_phys(cs->as, env->sp--, (ret & 0xff));
-stb_phys(cs->as, env->sp--, (ret & 0x00ff00) >>  8);
-stb_phys(cs->as, env->sp--, (ret & 0xff) >> 16);
+cpu_stb_data(env, env->sp--, (ret & 0xff));
+cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >>  8);
+cpu_stb_data(env, env->sp--, (ret & 0xff) >> 16);
 } else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
-stb_phys(cs->as, env->sp--, (ret & 0xff));
-stb_phys(cs->as, env->sp--, (ret & 0x00ff00) >>  8);
+cpu_stb_data(env, env->sp--, (ret & 0xff));
+cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >>  8);
 } else {
-stb_phys(cs->as, env->sp--, (ret & 0xff));
+cpu_stb_data(env, env->sp--, (ret & 0xff));
 }
 
 env->pc_w = base + vector * size;
-env->sregI = 0;/*  clear Global Interrupt Flag */
+env->sregI = 0; /* clear Global Interrupt Flag */
 
 cs->exception_index = -1;
 }
@@ -108,7 +109,7 @@ hwaddr avr_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
 
 int avr_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw, int mmu_idx)
 {
-/*  currently it's assumed that this will never happen */
+/* currently it's assumed that this will never happen */
 cs->exception_index = EXCP_DEBUG;
 cpu_dump_state(cs, stderr, fprintf, 0);
 return 1;
@@ -125,15 +126,55 @@ void tlb_fill(CPUState *cs, target_ulong vaddr, 
MMUAccessType access_type,
 vaddr &= TARGET_PAGE_MASK;
 
 if (mmu_idx == MMU_CODE_IDX) {
-paddr = PHYS_BASE_CODE + vaddr;
+paddr = PHYS_BASE_CODE + vaddr - VIRT_BASE_CODE;
 prot = PAGE_READ | PAGE_EXEC;
 } else {
-paddr = PHYS_BASE_DATA + vaddr;
-prot = PAGE_READ | PAGE_WRITE;
+#if VIRT_BASE_REGS == 0
+if (vaddr < VIRT_BASE_REGS + AVR_REGS) {
+#else
+if (VIRT_BASE_REGS <= vaddr && vaddr < VIRT_BASE_REGS + SIZE_REGS) {
+#endif
+/*
+ * this is a write into CPU registers, exit and rebuilt this TB
+ * to 

[Qemu-devel] [PATCH v14 5/9] target-avr: adding AVR interrupt handling

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 target-avr/helper.c | 59 -
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/target-avr/helper.c b/target-avr/helper.c
index 3984fc8..1e5d97d 100644
--- a/target-avr/helper.c
+++ b/target-avr/helper.c
@@ -31,11 +31,68 @@
 
 bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
-return false;
+CPUClass *cc = CPU_GET_CLASS(cs);
+AVRCPU *cpu = AVR_CPU(cs);
+CPUAVRState *env = >env;
+
+bool ret = false;
+
+if (interrupt_request & CPU_INTERRUPT_RESET) {
+if (cpu_interrupts_enabled(env)) {
+cs->exception_index = EXCP_RESET;
+cc->do_interrupt(cs);
+
+cs->interrupt_request   &= ~CPU_INTERRUPT_RESET;
+
+ret = true;
+}
+}
+if (interrupt_request & CPU_INTERRUPT_HARD) {
+if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
+int index = ctz32(env->intsrc);
+cs->exception_index = EXCP_INT(index);
+cc->do_interrupt(cs);
+
+env->intsrc &= env->intsrc - 1; /* clear the interrupt */
+cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
+
+ret = true;
+}
+}
+return ret;
 }
 
 void avr_cpu_do_interrupt(CPUState *cs)
 {
+AVRCPU *cpu = AVR_CPU(cs);
+CPUAVRState*env = >env;
+
+uint32_t ret = env->pc_w;
+int vector = 0;
+int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
+int base = 0;/* TODO: where to get it */
+
+if (cs->exception_index == EXCP_RESET) {
+vector = 0;
+} else if (env->intsrc != 0) {
+vector = ctz32(env->intsrc) + 1;
+}
+
+if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
+stb_phys(cs->as, env->sp--, (ret & 0xff));
+stb_phys(cs->as, env->sp--, (ret & 0x00ff00) >>  8);
+stb_phys(cs->as, env->sp--, (ret & 0xff) >> 16);
+} else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
+stb_phys(cs->as, env->sp--, (ret & 0xff));
+stb_phys(cs->as, env->sp--, (ret & 0x00ff00) >>  8);
+} else {
+stb_phys(cs->as, env->sp--, (ret & 0xff));
+}
+
+env->pc_w = base + vector * size;
+env->sregI = 0;/*  clear Global Interrupt Flag */
+
+cs->exception_index = -1;
 }
 
 int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
-- 
2.4.9 (Apple Git-60)

[Qemu-devel] [PATCH v14 3/9] target-avr: adding a sample AVR board

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 MAINTAINERS  |   6 +++
 hw/avr/Makefile.objs |  21 +
 hw/avr/sample.c  | 117 +++
 3 files changed, 144 insertions(+)
 create mode 100644 hw/avr/Makefile.objs
 create mode 100644 hw/avr/sample.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d1439a8..1435040 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -110,6 +110,12 @@ F: disas/arm.c
 F: disas/arm-a64.cc
 F: disas/libvixl/
 
+AVR
+M: Michael Rolnik 
+S: Maintained
+F: target-avr/
+F: hw/avr/
+
 CRIS
 M: Edgar E. Iglesias 
 S: Maintained
diff --git a/hw/avr/Makefile.objs b/hw/avr/Makefile.objs
new file mode 100644
index 000..8f537c9
--- /dev/null
+++ b/hw/avr/Makefile.objs
@@ -0,0 +1,21 @@
+#
+#  QEMU AVR CPU
+#
+#  Copyright (c) 2016 Michael Rolnik
+#
+#  This library is free software; you can redistribute it and/or
+#  modify it under the terms of the GNU Lesser General Public
+#  License as published by the Free Software Foundation; either
+#  version 2.1 of the License, or (at your option) any later version.
+#
+#  This library is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+#  Lesser General Public License for more details.
+#
+#  You should have received a copy of the GNU Lesser General Public
+#  License along with this library; if not, see
+#  
+#
+
+obj-y += sample.o
diff --git a/hw/avr/sample.c b/hw/avr/sample.c
new file mode 100644
index 000..2c1cd84
--- /dev/null
+++ b/hw/avr/sample.c
@@ -0,0 +1,117 @@
+/*
+ * QEMU AVR CPU
+ *
+ * Copyright (c) 2016 Michael Rolnik
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+/*
+ *  NOTE:
+ *  This is not a real AVR board !!! This is an example !!!
+ *
+ *This example can be used to build a real AVR board.
+ *
+ *  This example board loads provided binary file into flash memory and
+ *  executes it from 0x address in the code memory space.
+ *
+ *  This example does not implement/install any AVR specific on board
+ *  devices except SampleIO device which is an example as well.
+ *
+ *  Currently used for AVR CPU validation
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "hw/hw.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/qtest.h"
+#include "ui/console.h"
+#include "hw/boards.h"
+#include "hw/devices.h"
+#include "hw/loader.h"
+#include "qemu/error-report.h"
+#include "exec/address-spaces.h"
+#include "include/hw/sysbus.h"
+
+#define VIRT_BASE_FLASH 0x
+#define VIRT_BASE_ISRAM 0x0100
+#define VIRT_BASE_EXMEM 0x1100
+#define VIRT_BASE_EEPROM 0x
+
+#define SIZE_FLASH 0x0002
+#define SIZE_ISRAM 0x1000
+#define SIZE_EXMEM 0x0001
+#define SIZE_EEPROM 0x1000
+#define SIZE_IOREG SIZE_REGS
+
+#define PHYS_BASE_FLASH (PHYS_BASE_CODE)
+
+#define PHYS_BASE_ISRAM (PHYS_BASE_DATA)
+#define PHYS_BASE_EXMEM (PHYS_BASE_ISRAM + SIZE_ISRAM)
+#define PHYS_BASE_EEPROM (PHYS_BASE_EXMEM + SIZE_EXMEM)
+
+#define PHYS_BASE_IOREG (PHYS_BASE_REGS + 0x20)
+
+static void sample_init(MachineState *machine)
+{
+MemoryRegion *address_space_mem = get_system_memory();
+
+MemoryRegion *ram;
+MemoryRegion *flash;
+unsigned ram_size = SIZE_ISRAM + SIZE_EXMEM;
+
+AVRCPU *cpu_avr ATTRIBUTE_UNUSED;
+
+ram = g_new(MemoryRegion, 1);
+flash = g_new(MemoryRegion, 1);
+
+cpu_avr = cpu_avr_init("avr5");
+
+memory_region_allocate_system_memory(ram, NULL, "avr.ram", ram_size);
+memory_region_add_subregion(address_space_mem, PHYS_BASE_ISRAM, ram);
+
+memory_region_init_rom(flash, NULL, "avr.flash", SIZE_FLASH, _fatal);
+memory_region_add_subregion(address_space_mem, PHYS_BASE_FLASH, flash);
+vmstate_register_ram_global(flash);
+
+const char *firmware = NULL;
+const char *filename;
+
+if (machine->firmware) {
+firmware = machine->firmware;
+}
+
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, firmware);
+if (!filename) {
+error_report("Could not find flash image file '%s'", firmware);
+exit(1);
+}
+
+

[Qemu-devel] [PATCH v14 1/9] target-avr: AVR cores support is added.

[Michael Rolnik]

1. basic CPU structure
2. registers
3. no instructions
4. saving sreg, rampD, rampX, rampY, rampD, eind in HW representation

Signed-off-by: Michael Rolnik 
---
 arch_init.c |   2 +
 configure   |   5 +
 default-configs/avr-softmmu.mak |  21 +++
 include/disas/bfd.h |   6 +
 include/sysemu/arch_init.h  |   1 +
 target-avr/Makefile.objs|  23 +++
 target-avr/cpu-qom.h|  85 +++
 target-avr/cpu.c| 306 
 target-avr/cpu.h| 185 
 target-avr/gdbstub.c|  86 +++
 target-avr/helper.c |  88 
 target-avr/helper.h |  22 +++
 target-avr/machine.c| 116 +++
 target-avr/translate.c  | 282 
 target-avr/translate.h  | 116 +++
 15 files changed, 1344 insertions(+)
 create mode 100644 default-configs/avr-softmmu.mak
 create mode 100644 target-avr/Makefile.objs
 create mode 100644 target-avr/cpu-qom.h
 create mode 100644 target-avr/cpu.c
 create mode 100644 target-avr/cpu.h
 create mode 100644 target-avr/gdbstub.c
 create mode 100644 target-avr/helper.c
 create mode 100644 target-avr/helper.h
 create mode 100644 target-avr/machine.c
 create mode 100644 target-avr/translate.c
 create mode 100644 target-avr/translate.h

diff --git a/arch_init.c b/arch_init.c
index fa05973..be6e6de 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -80,6 +80,8 @@ int graphic_depth = 32;
 #define QEMU_ARCH QEMU_ARCH_UNICORE32
 #elif defined(TARGET_TRICORE)
 #define QEMU_ARCH QEMU_ARCH_TRICORE
+#elif defined(TARGET_AVR)
+#define QEMU_ARCH QEMU_ARCH_AVR
 #endif
 
 const uint32_t arch_type = QEMU_ARCH;
diff --git a/configure b/configure
index f57fcc6..c4d58b4 100755
--- a/configure
+++ b/configure
@@ -5641,6 +5641,8 @@ case "$target_name" in
   x86_64)
 TARGET_BASE_ARCH=i386
   ;;
+  avr)
+  ;;
   alpha)
   ;;
   arm|armeb)
@@ -5837,6 +5839,9 @@ disas_config() {
 
 for i in $ARCH $TARGET_BASE_ARCH ; do
   case "$i" in
+  avr)
+disas_config "AVR"
+  ;;
   alpha)
 disas_config "ALPHA"
   ;;
diff --git a/default-configs/avr-softmmu.mak b/default-configs/avr-softmmu.mak
new file mode 100644
index 000..003465d
--- /dev/null
+++ b/default-configs/avr-softmmu.mak
@@ -0,0 +1,21 @@
+#
+#  QEMU AVR CPU
+#
+#  Copyright (c) 2016 Michael Rolnik
+#
+#  This library is free software; you can redistribute it and/or
+#  modify it under the terms of the GNU Lesser General Public
+#  License as published by the Free Software Foundation; either
+#  version 2.1 of the License, or (at your option) any later version.
+#
+#  This library is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+#  Lesser General Public License for more details.
+#
+#  You should have received a copy of the GNU Lesser General Public
+#  License along with this library; if not, see
+#  
+#
+
+# Default configuration for avr-softmmu
diff --git a/include/disas/bfd.h b/include/disas/bfd.h
index 8a3488c..8ccd108 100644
--- a/include/disas/bfd.h
+++ b/include/disas/bfd.h
@@ -213,6 +213,12 @@ enum bfd_architecture
 #define bfd_mach_m32r  0  /* backwards compatibility */
   bfd_arch_mn10200,/* Matsushita MN10200 */
   bfd_arch_mn10300,/* Matsushita MN10300 */
+  bfd_arch_avr,   /* Atmel AVR microcontrollers.  */
+#define bfd_mach_avr1  1
+#define bfd_mach_avr2  2
+#define bfd_mach_avr3  3
+#define bfd_mach_avr4  4
+#define bfd_mach_avr5  5
   bfd_arch_cris,   /* Axis CRIS */
 #define bfd_mach_cris_v0_v10   255
 #define bfd_mach_cris_v32  32
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index d690dfa..8c75777 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -23,6 +23,7 @@ enum {
 QEMU_ARCH_UNICORE32 = (1 << 14),
 QEMU_ARCH_MOXIE = (1 << 15),
 QEMU_ARCH_TRICORE = (1 << 16),
+QEMU_ARCH_AVR = (1 << 17),
 };
 
 extern const uint32_t arch_type;
diff --git a/target-avr/Makefile.objs b/target-avr/Makefile.objs
new file mode 100644
index 000..2a10104
--- /dev/null
+++ b/target-avr/Makefile.objs
@@ -0,0 +1,23 @@
+#
+#  QEMU AVR CPU
+#
+#  Copyright (c) 2016 Michael Rolnik
+#
+#  This library is free software; you can redistribute it and/or
+#  modify it under the terms of the GNU Lesser General Public
+#  License as published by the Free Software Foundation; either
+#  version 2.1 of the License, or (at your option) any later version.
+#
+#  This library is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

[Qemu-devel] [PATCH v14 2/9] target-avr: adding AVR CPU features/flavors

[Michael Rolnik]

Signed-off-by: Michael Rolnik 
---
 target-avr/cpu.c | 319 +--
 target-avr/cpu.h |  52 -
 target-avr/machine.c |  28 ++---
 3 files changed, 373 insertions(+), 26 deletions(-)

diff --git a/target-avr/cpu.c b/target-avr/cpu.c
index c5ee2e9..fa15727 100644
--- a/target-avr/cpu.c
+++ b/target-avr/cpu.c
@@ -203,9 +203,300 @@ static void avr_cpu_class_init(ObjectClass *oc, void 
*data)
 dc->cannot_destroy_with_object_finalize_yet = true;
 }
 
-static void avr_any_initfn(Object *obj)
+static void avr_avr1_initfn(Object *obj)
 {
-/* Set cpu feature flags */
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+}
+
+static void avr_avr2_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+}
+
+static void avr_avr25_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_LPMX);
+avr_set_feature(env, AVR_FEATURE_MOVW);
+}
+
+static void avr_avr3_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_JMP_CALL);
+}
+
+static void avr_avr31_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_RAMPZ);
+avr_set_feature(env, AVR_FEATURE_ELPM);
+avr_set_feature(env, AVR_FEATURE_JMP_CALL);
+}
+
+static void avr_avr35_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_JMP_CALL);
+avr_set_feature(env, AVR_FEATURE_LPMX);
+avr_set_feature(env, AVR_FEATURE_MOVW);
+}
+
+static void avr_avr4_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_LPMX);
+avr_set_feature(env, AVR_FEATURE_MOVW);
+avr_set_feature(env, AVR_FEATURE_MUL);
+}
+
+static void avr_avr5_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+avr_set_feature(env, AVR_FEATURE_ADIW_SBIW);
+avr_set_feature(env, AVR_FEATURE_SRAM);
+avr_set_feature(env, AVR_FEATURE_BREAK);
+
+avr_set_feature(env, AVR_FEATURE_2_BYTE_PC);
+avr_set_feature(env, AVR_FEATURE_2_BYTE_SP);
+avr_set_feature(env, AVR_FEATURE_JMP_CALL);
+avr_set_feature(env, AVR_FEATURE_LPMX);
+avr_set_feature(env, AVR_FEATURE_MOVW);
+avr_set_feature(env, AVR_FEATURE_MUL);
+}
+
+static void avr_avr51_initfn(Object *obj)
+{
+AVRCPU *cpu = AVR_CPU(obj);
+CPUAVRState *env = >env;
+
+avr_set_feature(env, AVR_FEATURE_LPM);
+avr_set_feature(env, AVR_FEATURE_IJMP_ICALL);
+

[Qemu-devel] [PATCH v14 0/9] 8bit AVR cores

[Michael Rolnik]

This series of patches adds 8bit AVR cores to QEMU.
All instruction, except BREAK/DES/SPM/SPMX, are implemented. Not fully tested 
yet.
However I was able to execute simple code with functions. e.g fibonacci 
calculation.
This series of patches include a non real, sample board.
No fuses support yet. PC is set to 0 at reset.

the patches include the following
1. just a basic 8bit AVR CPU, without instruction decoding or translation
2. CPU features which allow define the following 8bit AVR cores
 avr1
 avr2 avr25
 avr3 avr31 avr35
 avr4
 avr5 avr51
 avr6
 xmega2 xmega4 xmega5 xmega6 xmega7
3. a definition of sample machine with SRAM, FLASH and CPU which allows to 
execute simple code
4. encoding for all AVR instructions
5. interrupt handling
6. helpers for IN, OUT, SLEEP, WBR & unsupported instructions
7. a decoder which given an opcode decides what istruction it is
8. translation of AVR instruction into TCG
9. all features together

changes since v3
1. rampD/X/Y/Z registers are encoded as 0x00ff (instead of 0x00ff) for 
faster address manipulaton
2. ffs changed to ctz32
3. duplicate code removed at avr_cpu_do_interrupt
4. using andc instead of not + and
5. fixing V flag calculation in varios instructions
6. freeing local variables in PUSH
7. tcg_const_local_i32 -> tcg_const_i32
8. using sextract32 instead of my implementation
9. fixing BLD instruction
10.xor(r) instead of 0xff - r at COM
11.fixing MULS/MULSU not to modify inputs' content
12.using SUB for NEG
13.fixing tcg_gen_qemu_ld/st call in XCH

changes since v4
1. target is now defined as big endian in order to optimize push_ret/pop_ret
2. all style warnings are fixed
3. adding cpu_set/get_sreg functions
4. simplifying gen_goto_tb as there is no real paging
5. env->pc -> env->pc_w
6. making flag dump more compact
7. more spacing
8. renaming CODE/DATA_INDEX -> MMU_CODE/DATA_IDX
9. removing avr_set_feature
10. SPL/SPH set bug fix
11. switching stb_phys to cpu_stb_data
12. cleaning up avr_decode
13. saving sreg, rampD/X/Y/Z, eind in HW format (savevm)
14. saving CPU features (savevm)

changes since v5
1. BLD bug fix
2. decoder generator is added

chages since v6
1. using cpu_get_sreg/cpu_set_sreg in 
avr_cpu_gdb_read_register/avr_cpu_gdb_write_register
2. configure the target as little endian because otherwise GDB does not work
3. fixing and testing gen_push_ret/gen_pop_ret

changes since v7
1. folding back v6 
2. logging at helper_outb and helper_inb are done for non supported yet 
registers only
3. MAINTAINERS updated

changes since v8
1. removing hw/avr from hw/Makefile.obj as it should not be built for all
2. making linux compilable
3. testing on
a. Mac, Apple LLVM version 7.0.0
b. Ubuntu 12.04, gcc 4.9.2
c. Fedora 23, gcc 5.3.1
4. folding back some patches
5. translation bug fixes for ORI, CPI, XOR instructions
6. propper handling of cpu register writes though memory

changes since v9
1. removing forward declarations of static functions
2. disabling debug prints
3. switching to case range instead of if else if ...
4. LD/ST IN/OUT accessing CPU maintainder registers are not routed to any device
5. commenst about sample board and sample IO device added
6. sample board description is more descriptive now
7. memory_region_allocate_system_memory is used to create RAM
8. now there are helper_fullrd & helper_fullwr when LD/ST try to access 
registers

changes since v10
1. movig back fullwr & fullrd into the commit where outb and inb were introduced
2. changing tlb_fill function signature
3. adding empty line between functions
4. adding newline on the last line of the file
5. using tb->flags to generae full access ST/LD instructions
6. fixing SBRC bug
7. folding back 10th commit
8. whenever a new file is introduced it's added to Makefile.objs

changes since v11
1. updating to v2.7.0-rc
2. removing assignment to env->fullacc from gen_intermediate_code

changes since v12
1. fixing spacing
2. fixing get/put_segment functions
3. removing target-avr/machine.h file
4. VMSTATE_SINGLE_TEST -> VMSTATE_SINGLE
5. comment spelling
6. removing hw/avr/sample_io.c
7. char const* -> const char*
8. proper ram allocation
9. fixing breakpoint functionality.
10.env1 -> env
11.fixing avr_cpu_gdb_write_register & avr_cpu_gdb_read_register functions
12.any cpu is removed
12.feature bits are not saved into vm state

changes since v13
1. rebasing to v2.7.0-rc1


Michael Rolnik (9):
  target-avr: AVR cores support is added.
  target-avr: adding AVR CPU features/flavors
  target-avr: adding a sample AVR board
  target-avr: adding instructions encodings
  target-avr: adding AVR interrupt handling
  target-avr: adding helpers for IN, OUT, SLEEP, WBR & unsupported
instructions
  target-avr: adding instruction translation
  target-avr: instruction decoder generator
  target-avr: adding instruction decoder

 MAINTAINERS|6 +
 arch_init.c|2 +
 configure 

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 29 July 2016 at 07:54, Andrew Jones  wrote:
> OK, so this property will be exposed to all ARM cpu types, and if a user
> turns it on, then it will stay on for all types, except when using KVM
> with an aarch64 cpu type, and KVM doesn't support it. This could mislead
> users to believe they'll get a pmu, by simply adding pmu=on, even when
> they can't. I think we'd ideally keep has_pmu, and the current code that
> sets it, and then add code like
>
>  if (enable_pmu && !has_pmu) {
>error_report("Warning: ...")
>  }
>
> somewhere.

I think we should probably follow the existing model used by
has_el3, where the property only exists if it's valid to set it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 29 July 2016 at 16:08, Wei Huang  wrote:
>
>
> On 07/29/2016 02:57 AM, Peter Maydell wrote:
>> On 28 July 2016 at 17:38, Wei Huang  wrote:
>>> This patch adds a pmu=[on/off] option to enable/disable vpmu support
>>> in guest vm. There are several reasons to justify this option. First
>>> vpmu can be problematic for cross-migration between different SoC as
>>> perf counters is architecture-dependent. It is more flexible to
>>> have an option to turn it on/off. Secondly it matches the -cpu pmu
>>> option in libivrt. This patch has been tested on both DT/ACPI modes.
>>
>>
>> What particular two systems are you trying to migrate between?
>
> One example: APM's Mustang has 5 perf counters while AMD's Seattle has 7
> counters.

Right; this is a cross-implementation migration, which isn't
supposed to work at the moment, and more will be required for
it to work than just pmu on/off.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio-console: set frontend open permanently for console devs

[Daniel P. Berrange]

On Fri, Jul 29, 2016 at 12:17:43PM +0100, Daniel P. Berrange wrote:
> The virtio-console.c file handles both serial consoles
> and interactive consoles, since they're backed by the
> same device model.
> 
> Since serial devices are expected to be reliable and
> need to notify the guest when the backend is opened
> or closed, the virtio-console.c file wires up support
> for chardev events. This affects both serial consoles
> and interactive consoles, using a network connection
> based chardev backend such as 'socket', but not when
> using a PTY based backend or plain 'file' backends.
> 
> When a device is open, however, the behaviour is
> different - if the backend chardev returns EAGAIN or
> a short write, the serial console will block and
> setup a watch to poll for writability, ensuring no
> data is lost.  The interactive consoles meanwhile
> will simply discard data.
> 
> This means that the interactive consoles have different
> blocking behaviour depending on whether the chardev is
> open or not. If open, data may be discarded if not
> consumed, where as if closed, data will always be queued
> pending an open.
> 
> This behaviour is unhelpful in general - applications
> outputting messages on the guest console should not be
> blocked simply because no client is conencted to the host
> side.
> 
> Consider for example, configuring a x86_64 guest with a
> plain UART serial port
> 
>   -chardev 
> socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on
>   -device isa-serial,chardev=charserial1,id=serial1
> 
> vs a s390 guest which has to use the virtio-console port
> 
>   -chardev 
> socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on
>   -device virtconsole,chardev=charconsole1,id=console1
> 
> The isa-serial one gets data written to the log regardless
> of whether a client is connected, while the virtioconsole
> one only gets data written to the log when a client is
> connected.
> 
> This patch changes the behaviour so that virtconsole
> devices work in same way as other traditional console
> devices. Specifically, the frontend will now be marked
> as permanently open, so data flows regardless of the
> backend status.
> 
> NB, the behaviour of virtserialport devices is *not*
> changed, only virtconsole.
> 
> Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  hw/char/virtio-console.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> NB If there is a concern about backends compatibility with
> this change, we could instead add a boolean property to
> the virtio-console device 'explicit-open' which controls
> whether the virtconsole device has the old behaviour or
> the new behaviour and default to old. Personally I think
> it is fine to just change behaviour for virtconsole
> unconditionally though


Ignore this patch for now. Looking around the users of chardev
code I've noticed more inconsistency. So I want to analyse the
broad usage and report on semantics across all, before proposing
potential multiple fixes.


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

Re: [Qemu-devel] VFIO mdev with vIOMMU

[Alex Williamson]

On Thu, 28 Jul 2016 23:47:58 +
"Tian, Kevin"  wrote:

> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Thursday, July 28, 2016 11:42 PM
> > 
> > On Thu, 28 Jul 2016 10:15:24 +
> > "Tian, Kevin"  wrote:
> >   
> > > Hi, Alex,
> > >
> > > Along with recent enhancement on virtual IOMMU (vIOMMU) in Qemu, I'm
> > > thinking whether there is any issue for mdev to cope with vIOMMU. I
> > > know today VFIO device only works with PowerPC IOMMU (note someone
> > > is enabling VFIO device with virtual VT-d but looks not complete yet), but
> > > it's always good to do architecture discussion earlier. :-)
> > >
> > > VFIO mdev framework maintains a GPA->HPA mapping, which are queried
> > > by vendor specific mdev device model for emulation purpose. For example,
> > > guest GPU PTEs may need be translated into shadow GPU PTEs, where
> > > GPA->HPA conversion is required.
> > >
> > > When a virtual IOMMU is exposed to the guest, IOVA may be used as DMA
> > > address by the guest, which means guest PTE now contains IOVA instead
> > > of GPA then device model would like to know IOVA->HPA mapping. After
> > > checking current vIOMMU logic within Qemu, looks it's not a problem.
> > > vIOMMU is expected to notify any IOVA change to VFIO and kernel VFIO
> > > driver does receive map requests for IOVA regions. Thus the mapping
> > > structure that VFIO maintains does be IOVA->HPA mapping as required
> > > by device model.
> > >
> > > In this manner looks no further change is required on proposed mdev
> > > framework to support vIOMMU. The only thing that I'm unsure is how
> > > Qemu guarantees to map IOVA vs. GPA exclusively. I checked that
> > > vfio_listener_region_add initiates map request for normal memory
> > > regions (which is GPA), and then vfio_iommu_map_notify will send
> > > map request for IOVA region which is notified through IOMMU notifier.
> > > I don't think VFIO can cope both GPA/IOVA map requests simultaneously,
> > > since VFIO doesn't maintain multiple address spaces on one device. It's
> > > not a mdev specific question, but I definitely missed some key points
> > > here since it's assumed to be working for PowerPC already...  
> > 
> > I prefer not to distinguish GPA vs IOVA, the device always operates in
> > the IOVA space.  Without a vIOMMU, it just happens to be an identity map
> > into the GPA space.  Think about how this works on real hardware, when
> > VT-d is not enabled, there's no translation, IOVA = GPA.  The device
> > interacts directly with system memory, same as the default case in
> > QEMU now.  When VT-d is enabled, the device is placed into an IOMMU
> > domain and the IOVA space is now restricted to the translations defined
> > within that domain.  The same is expected to happen with QEMU, all of
> > the GPA mapped IOVA space is removed via vfio_listener_region_del() and
> > a new IOMMU region is added, enabling the vfio_iommu_map_notify  
> 
> Ha, it is the info I'm looking for. Can you help point to me where above 
> logic is implemented? I only saw the latter part about adding a new IOMMU
> region...
> 
> And suppose we also have logic to do the vice versa - when guest disables
> IOMMU then all IOVA mappings will be deleted and then GPA mapped IOVA
> space will be replayed?

Since we've never actually seen this work with an assigned device, I
may have misspoken on how it actually works.  I think the above is how
it *should* work though.  As it is, vfio_initfn() calls
pci_device_iommu_address_space(), this is what looks for iommu address
spaces for a device and when not found makes use of
address_space_memory.  So at this point we are either strictly using
the iommu notifier or strictly using address_space_memory.  This seems
hugely inefficient to me, but we don't have any mechanisms to change a
device's address space.  IMHO it really seems like we need
dynamic address space aliasing support for an iommu like VT-d.  So if
VT-d emulation actually worked with the iommu notifier today, I believe
each vfio device will be placed into separate containers and the iommu
replay mechanism would populate whatever initial translations are
configured for the device, including translations to
address_space_memory if the iommu is disabled or configured for
passthrough.  It would be through notifier events that this would need
to be de-populated and re-populated with discrete entries if the device
is configured to a more fine grained domain.

I have my doubts whether QEMU iommu support is really properly designed
to handle an iommu like VT-d, it seems better equipped for simple
window-based iommus.  Thanks,

Alex

[Qemu-devel] [PATCH v3] ui/cocoa.m: Make a better about dialog

[Programmingkid]

The about dialog in QEMU on Mac OS X is very plain and unhelpful. This patch
makes the about dialog look a lot better and have some descriptive information
on what version of QEMU the user is running.

Signed-off-by: John Arbuckle 
---
version 3 changes:
Removed buffer related code

version 2 changes:
Added QEMU version to the version label

 ui/cocoa.m | 107 +++--
 1 file changed, 104 insertions(+), 3 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 36c6bf0..e17401b 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -34,6 +34,7 @@
 #include "qmp-commands.h"
 #include "sysemu/blockdev.h"
 #include 
+#include "qemu-version.h"
 
 #ifndef MAC_OS_X_VERSION_10_5
 #define MAC_OS_X_VERSION_10_5 1050
@@ -63,7 +64,7 @@ typedef struct {
 int bitsPerPixel;
 } QEMUScreen;
 
-NSWindow *normalWindow;
+NSWindow *normalWindow, *about_window;
 static DisplayChangeListener *dcl;
 static int last_buttons;
 
@@ -670,7 +671,9 @@ QemuCocoaView *cocoaView;
 case NSLeftMouseUp:
 mouse_event = true;
 if (!isMouseGrabbed && [self screenContainsPoint:p]) {
-[self grabMouse];
+if([[self window] isKeyWindow]) {
+[self grabMouse];
+}
 }
 break;
 case NSRightMouseUp:
@@ -824,6 +827,8 @@ QemuCocoaView *cocoaView;
 - (void)changeDeviceMedia:(id)sender;
 - (BOOL)verifyQuit;
 - (void)openDocumentation:(NSString *)filename;
+- (IBAction) do_about_menu_item: (id) sender;
+- (void)make_about_window;
 @end
 
 @implementation QemuCocoaAppController
@@ -876,6 +881,7 @@ QemuCocoaView *cocoaView;
 supportedImageFileTypes = [NSArray arrayWithObjects: @"img", @"iso", 
@"dmg",
  @"qcow", @"qcow2", @"cloop", @"vmdk", @"cdr",
   nil];
+[self make_about_window];
 }
 return self;
 }
@@ -1138,6 +1144,101 @@ QemuCocoaView *cocoaView;
 }
 }
 
+/* The action method for the About menu item */
+- (IBAction) do_about_menu_item: (id) sender
+{
+[about_window makeKeyAndOrderFront: nil];
+}
+
+/* Create and display the about dialog */
+- (void)make_about_window
+{
+/* Make the window */
+int x = 0, y = 0, about_width = 400, about_height = 200;
+NSRect window_rect = NSMakeRect(x, y, about_width, about_height);
+about_window = [[NSWindow alloc] initWithContentRect:window_rect
+styleMask:NSTitledWindowMask | NSClosableWindowMask |
+NSMiniaturizableWindowMask
+backing:NSBackingStoreBuffered
+defer:NO];
+[about_window setTitle: @"About"];
+[about_window setReleasedWhenClosed: NO];
+[about_window center];
+NSView *superView = [about_window contentView];
+
+/* Create the dimensions of the picture */
+int picture_width = 80, picture_height = 80;
+x = (about_width - picture_width)/2;
+y = about_height - picture_height - 10;
+NSRect picture_rect = NSMakeRect(x, y, picture_width, picture_height);
+
+/* Get the path to the QEMU binary */
+NSString *binary_name = [NSString stringWithCString: gArgv[0]
+  encoding: NSASCIIStringEncoding];
+binary_name = [binary_name lastPathComponent];
+NSString *program_path = [[NSString alloc] initWithFormat: @"%@/%@",
+[[NSBundle mainBundle] bundlePath], binary_name];
+
+/* Make the picture of QEMU */
+NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
+ picture_rect];
+NSImage *qemu_image = [[NSWorkspace sharedWorkspace] iconForFile:
+ program_path];
+[picture_view setImage: qemu_image];
+[picture_view setImageScaling: NSScaleToFit];
+[superView addSubview: picture_view];
+
+/* Make the name label */
+x = 0;
+y = y - 25;
+int name_width = about_width, name_height = 20;
+NSRect name_rect = NSMakeRect(x, y, name_width, name_height);
+NSTextField *name_label = [[NSTextField alloc] initWithFrame: name_rect];
+[name_label setEditable: NO];
+[name_label setBezeled: NO];
+[name_label setDrawsBackground: NO];
+[name_label setAlignment: NSCenterTextAlignment];
+NSString *qemu_name = [[NSString alloc] initWithCString: gArgv[0]
+encoding: NSASCIIStringEncoding];
+qemu_name = [qemu_name lastPathComponent];
+[name_label setStringValue: qemu_name];
+[superView addSubview: name_label];
+
+/* Set the version label's attributes */
+x = 0;
+y = 50;
+int version_width = about_width, version_height = 20;
+NSRect version_rect = NSMakeRect(x, y, version_width, version_height);
+NSTextField *version_label = [[NSTextField alloc] initWithFrame:
+  

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Wei Huang]

On 07/29/2016 02:57 AM, Peter Maydell wrote:
> On 28 July 2016 at 17:38, Wei Huang  wrote:
>> This patch adds a pmu=[on/off] option to enable/disable vpmu support
>> in guest vm. There are several reasons to justify this option. First
>> vpmu can be problematic for cross-migration between different SoC as
>> perf counters is architecture-dependent. It is more flexible to
>> have an option to turn it on/off. Secondly it matches the -cpu pmu
>> option in libivrt. This patch has been tested on both DT/ACPI modes.
> 
> 
> What particular two systems are you trying to migrate between?

One example: APM's Mustang has 5 perf counters while AMD's Seattle has 7
counters.

> In general we don't support migrating between different CPU
> types at the moment, so the PMU sholud be the same on both ends.
> 
> (If we ever do get to supporting cross-cpu-type migration
> then it will probably involve a very long and detailed command
> line to specify exactly a whole lot of things like pmu yes/no,
> number of hw breakpoints/watchpoints, and everything else that
> can differ between implementations.)
> 
> That said, I don't have any objection to making the PMU
> presence controllable (especially if we have similar
> control on x86).
> 
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -579,8 +579,9 @@ struct ARMCPU {
>>  bool powered_off;
>>  /* CPU has security extension */
>>  bool has_el3;
>> -/* CPU has PMU (Performance Monitor Unit) */
>> -bool has_pmu;
>> +
>> +/* CPU has vPMU (Performance Monitor Unit) support */
>> +bool enable_pmu;
> 
> Why rename the flag? has_foo is what we use for other features,
> as you can see in the context of this bit of the patch.

I will fix it. Maybe follow the suggestion Drew's suggestion, keeping
has_pmu and add another option for turning it on/off.

> 
>>
>>  /* CPU has memory protection unit */
>>  bool has_mpu;
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Wei Huang]

On 07/29/2016 01:54 AM, Andrew Jones wrote:
> On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote:
>> This patch adds a pmu=[on/off] option to enable/disable vpmu support
>> in guest vm. There are several reasons to justify this option. First
>> vpmu can be problematic for cross-migration between different SoC as
>> perf counters is architecture-dependent. It is more flexible to
>> have an option to turn it on/off. Secondly it matches the -cpu pmu
>> option in libivrt. This patch has been tested on both DT/ACPI modes.
>>
>> Signed-off-by: Wei Huang 
>> ---
>>  hw/arm/virt-acpi-build.c |  2 +-
>>  hw/arm/virt.c|  2 +-
>>  target-arm/cpu.c |  1 +
>>  target-arm/cpu.h |  5 +++--
>>  target-arm/kvm64.c   | 10 +-
>>  5 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 28fc59c..dc5f66d 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
>> VirtGuestInfo *guest_info)
>>  gicc->uid = i;
>>  gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>>  
>> -if (armcpu->has_pmu) {
>> +if (armcpu->enable_pmu) {
>>  gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>>  }
>>  }
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index a193b5a..6aea901 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, 
>> int gictype)
>>  
>>  CPU_FOREACH(cpu) {
>>  armcpu = ARM_CPU(cpu);
>> -if (!armcpu->has_pmu ||
>> +if (!armcpu->enable_pmu ||
>>  !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
>>  return;
>>  }
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index ce8b8f4..f7daf81 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>  };
>>  
>>  static Property arm_cpu_properties[] = {
>> +DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true),
> 
> x86's pmu property defaults to off. I'm not sure if it's necessary to
> have a consistent default between x86 and arm in order for libvirt to
> be able to use it in the same way. We should confirm with libvirt
> people. Anyway, I think I'd prefer we default off here, and then we
> can default on in machine code for configurations that we want it by
> default (only AArch64 KVM). Or, maybe we don't want it by default at
> all? Possibly we should only set it on by default for virt-2.6, and
> then, from 2.7 on, require users to opt-in to the feature. It makes
> sense to require opting-in to features that can cause problems with
> migration.

This option is default=off on x86. I agree that it is a compatibility
related issue which can break migration. So it is reasonable to turn it
off on ARM as well. Let us wait for libvirt people to voice out.

> 
>>  DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>>  DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
>>  DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 76d824d..f2341c0 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -579,8 +579,9 @@ struct ARMCPU {
>>  bool powered_off;
>>  /* CPU has security extension */
>>  bool has_el3;
>> -/* CPU has PMU (Performance Monitor Unit) */
>> -bool has_pmu;
>> +
>> +/* CPU has vPMU (Performance Monitor Unit) support */
>> +bool enable_pmu;
>>  
>>  /* CPU has memory protection unit */
>>  bool has_mpu;
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index 5faa76c..ca21670 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -501,11 +501,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>  if (!arm_feature(>env, ARM_FEATURE_AARCH64)) {
>>  cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
>>  }
>> -if (kvm_irqchip_in_kernel() &&
>> -kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
>> -cpu->has_pmu = true;
>> -cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
>> -}
>> +
>> +/* enable vPMU based on KVM mode and hardware capability */
>> +cpu->enable_pmu &= (kvm_irqchip_in_kernel() &&
>> +kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3));
> 
> nit: the () aren't necessary
> 

Will fix

>> +cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3;
>>  
>>  /* Do KVM_ARM_VCPU_INIT ioctl */
>>  ret = kvm_arm_vcpu_init(cs);
>> -- 
>> 2.4.11
>>
>>
> 
> OK, so this property will be exposed to all ARM cpu types, and if a user
> turns it on, then it will stay on for all types, except when using KVM
> with an aarch64 cpu type, and KVM doesn't support it. This could mislead
> users to believe they'll get a pmu, by simply 

[Qemu-devel] [Bug 1586756] Re: "-serial unix:" option of qemu-system-arm is broken in qemu 2.6.0

[Daniel Berrange]

Ok, now it makes more sense. You are using the socket chardev in TCP
outgoing client mode, which used to be blocking by default. In 2.6 this
switched to match TCP server mode,  in wich the incoming client was
always in non-blocking mode.

IOW, this arm code was already broken in 2.5 just depending on which
chardev config was used.

So, we'll need to put a fix in pl011_write.  I'm unsure whether use
writeall() is the correct behaviour - I'll come to other serial
backends.

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

Title:
  "-serial unix:" option of qemu-system-arm is broken in qemu 2.6.0

Status in QEMU:
  Incomplete

Bug description:
  I found a bug of "-serial unix:PATH_TO_SOCKET" in qemu 2.6.0 (qemu 2.5.1 
works fine).
  Occasionally, a part of the output of qemu disappears in the bug.

  It looks like following commit is the cause:

  char: ensure all clients are in non-blocking mode (Author: Daniel P. Berrange 
)
  
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=64c800f808748522727847b9cdc73412f22dffb9

  In this commit, UNIX socket is set to non-blocking mode, but 
qemu_chr_fe_write function doesn't handle EAGAIN.
  You should fix code like that:

  ---
  diff --git a/qemu-char.c b/qemu-char.c
  index b597ee1..0361d78 100644
  --- a/qemu-char.c
  +++ b/qemu-char.c
  @@ -270,6 +270,7 @@ static int qemu_chr_fe_write_buffer(CharDriverState *s, 
const uint8_t *buf, int
   int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
   {
   int ret;
  +int offset = 0;
   
   if (s->replay && replay_mode == REPLAY_MODE_PLAY) {
   int offset;
  @@ -280,7 +281,21 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t 
*buf, int len)
   }
   
   qemu_mutex_lock(>chr_write_lock);
  -ret = s->chr_write(s, buf, len);
  +
  +while (offset < len) {
  +retry:
  +ret = s->chr_write(s, buf, len);
  +if (ret < 0 && errno == EAGAIN) {
  +g_usleep(100);
  +goto retry;
  +}
  +
  +if (ret <= 0) {
  +break;
  +}
  +
  +offset += ret;
  +}
   
   if (ret > 0) {
   qemu_chr_fe_write_log(s, buf, ret);
  ---

  Or please do "git revert 64c800f808748522727847b9cdc73412f22dffb9".

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

[Qemu-devel] [Bug 1586756] Re: "-serial unix:" option of qemu-system-arm is broken in qemu 2.6.0

[redcap97]

** Summary changed:

- "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0
+ "-serial unix:" option of qemu-system-arm is broken in qemu 2.6.0

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

Title:
  "-serial unix:" option of qemu-system-arm is broken in qemu 2.6.0

Status in QEMU:
  Incomplete

Bug description:
  I found a bug of "-serial unix:PATH_TO_SOCKET" in qemu 2.6.0 (qemu 2.5.1 
works fine).
  Occasionally, a part of the output of qemu disappears in the bug.

  It looks like following commit is the cause:

  char: ensure all clients are in non-blocking mode (Author: Daniel P. Berrange 
)
  
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=64c800f808748522727847b9cdc73412f22dffb9

  In this commit, UNIX socket is set to non-blocking mode, but 
qemu_chr_fe_write function doesn't handle EAGAIN.
  You should fix code like that:

  ---
  diff --git a/qemu-char.c b/qemu-char.c
  index b597ee1..0361d78 100644
  --- a/qemu-char.c
  +++ b/qemu-char.c
  @@ -270,6 +270,7 @@ static int qemu_chr_fe_write_buffer(CharDriverState *s, 
const uint8_t *buf, int
   int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
   {
   int ret;
  +int offset = 0;
   
   if (s->replay && replay_mode == REPLAY_MODE_PLAY) {
   int offset;
  @@ -280,7 +281,21 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t 
*buf, int len)
   }
   
   qemu_mutex_lock(>chr_write_lock);
  -ret = s->chr_write(s, buf, len);
  +
  +while (offset < len) {
  +retry:
  +ret = s->chr_write(s, buf, len);
  +if (ret < 0 && errno == EAGAIN) {
  +g_usleep(100);
  +goto retry;
  +}
  +
  +if (ret <= 0) {
  +break;
  +}
  +
  +offset += ret;
  +}
   
   if (ret > 0) {
   qemu_chr_fe_write_log(s, buf, ret);
  ---

  Or please do "git revert 64c800f808748522727847b9cdc73412f22dffb9".

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

[Qemu-devel] [Bug 1586756] Re: "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0

[redcap97]

I wrote small code which reproduces this issue.

https://bitbucket.org/redcap97/puts-hello-x8-armv7-kernel/downloads
/puts-HELLO-x8

Above binary outputs "HELLO!" 8 times to UART.

# Please execute in terminal
socat unix-listen:a.sock stdout | tee actual

# Please execute in another terminal
qemu-system-arm -M vexpress-a9 -nographic -kernel puts-HELLO-x8 -serial 
unix:a.sock

# Check results
yes 'HELLO!' | head -n 8 > expected
diff -u expected actual

Occasionally, a part of the output of qemu disappears.


Source code of the binary
https://bitbucket.org/redcap97/puts-hello-x8-armv7-kernel/src

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

Title:
  "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0

Status in QEMU:
  Incomplete

Bug description:
  I found a bug of "-serial unix:PATH_TO_SOCKET" in qemu 2.6.0 (qemu 2.5.1 
works fine).
  Occasionally, a part of the output of qemu disappears in the bug.

  It looks like following commit is the cause:

  char: ensure all clients are in non-blocking mode (Author: Daniel P. Berrange 
)
  
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=64c800f808748522727847b9cdc73412f22dffb9

  In this commit, UNIX socket is set to non-blocking mode, but 
qemu_chr_fe_write function doesn't handle EAGAIN.
  You should fix code like that:

  ---
  diff --git a/qemu-char.c b/qemu-char.c
  index b597ee1..0361d78 100644
  --- a/qemu-char.c
  +++ b/qemu-char.c
  @@ -270,6 +270,7 @@ static int qemu_chr_fe_write_buffer(CharDriverState *s, 
const uint8_t *buf, int
   int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
   {
   int ret;
  +int offset = 0;
   
   if (s->replay && replay_mode == REPLAY_MODE_PLAY) {
   int offset;
  @@ -280,7 +281,21 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t 
*buf, int len)
   }
   
   qemu_mutex_lock(>chr_write_lock);
  -ret = s->chr_write(s, buf, len);
  +
  +while (offset < len) {
  +retry:
  +ret = s->chr_write(s, buf, len);
  +if (ret < 0 && errno == EAGAIN) {
  +g_usleep(100);
  +goto retry;
  +}
  +
  +if (ret <= 0) {
  +break;
  +}
  +
  +offset += ret;
  +}
   
   if (ret > 0) {
   qemu_chr_fe_write_log(s, buf, ret);
  ---

  Or please do "git revert 64c800f808748522727847b9cdc73412f22dffb9".

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

[Qemu-devel] [Bug 1586756] Re: "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0

[redcap97]

> Looking at the code in 2.5.0, the qemu_chr_fe_write method would see
EAGAIN too, because the tcp_chr_accept() method would always set the
newly connected client to non-blocking mode. So thre's no obvious change
in behaviour between 2.5 and 2.6 wrt to blocking sockets.

It looks like tcp_chr_accept function isn't called in above command.
tcp_chr_wait_connected function is called instead.

So the socket is blocking mode and doesn't return EAGAIN in Qemu 2.5.0.

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

Title:
  "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0

Status in QEMU:
  Incomplete

Bug description:
  I found a bug of "-serial unix:PATH_TO_SOCKET" in qemu 2.6.0 (qemu 2.5.1 
works fine).
  Occasionally, a part of the output of qemu disappears in the bug.

  It looks like following commit is the cause:

  char: ensure all clients are in non-blocking mode (Author: Daniel P. Berrange 
)
  
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=64c800f808748522727847b9cdc73412f22dffb9

  In this commit, UNIX socket is set to non-blocking mode, but 
qemu_chr_fe_write function doesn't handle EAGAIN.
  You should fix code like that:

  ---
  diff --git a/qemu-char.c b/qemu-char.c
  index b597ee1..0361d78 100644
  --- a/qemu-char.c
  +++ b/qemu-char.c
  @@ -270,6 +270,7 @@ static int qemu_chr_fe_write_buffer(CharDriverState *s, 
const uint8_t *buf, int
   int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
   {
   int ret;
  +int offset = 0;
   
   if (s->replay && replay_mode == REPLAY_MODE_PLAY) {
   int offset;
  @@ -280,7 +281,21 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t 
*buf, int len)
   }
   
   qemu_mutex_lock(>chr_write_lock);
  -ret = s->chr_write(s, buf, len);
  +
  +while (offset < len) {
  +retry:
  +ret = s->chr_write(s, buf, len);
  +if (ret < 0 && errno == EAGAIN) {
  +g_usleep(100);
  +goto retry;
  +}
  +
  +if (ret <= 0) {
  +break;
  +}
  +
  +offset += ret;
  +}
   
   if (ret > 0) {
   qemu_chr_fe_write_log(s, buf, ret);
  ---

  Or please do "git revert 64c800f808748522727847b9cdc73412f22dffb9".

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

[Qemu-devel] [Bug 1586756] Re: "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0

[redcap97]

Sorry, this issue only occur in qemu-system-arm (vexpress-a9).
In hw/char/pl011.c, qemu_chr_fe_write function is called directly and EAGAIN 
isn't handled.

http://git.qemu.org/?p=qemu.git;a=blob;f=hw/char/pl011.c;h=210c87b4c2bd000d80c359ca5c05d43c64210677;hb=bfc766d38e1fae5767d43845c15c79ac8fa6d6af#l148

So I use following a patch.


diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index c0fbf8a..6e29db8 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -146,7 +146,7 @@ static void pl011_write(void *opaque, hwaddr offset,
 /* ??? Check if transmitter is enabled.  */
 ch = value;
 if (s->chr)
-qemu_chr_fe_write(s->chr, , 1);
+qemu_chr_fe_write_all(s->chr, , 1);
 s->int_level |= PL011_INT_TX;
 pl011_update(s);
 break;


qemu_chr_fe_write_all function handles EAGAIN.

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

Title:
  "-serial unix:" option of qemu-system-* is broken in qemu 2.6.0

Status in QEMU:
  Incomplete

Bug description:
  I found a bug of "-serial unix:PATH_TO_SOCKET" in qemu 2.6.0 (qemu 2.5.1 
works fine).
  Occasionally, a part of the output of qemu disappears in the bug.

  It looks like following commit is the cause:

  char: ensure all clients are in non-blocking mode (Author: Daniel P. Berrange 
)
  
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=64c800f808748522727847b9cdc73412f22dffb9

  In this commit, UNIX socket is set to non-blocking mode, but 
qemu_chr_fe_write function doesn't handle EAGAIN.
  You should fix code like that:

  ---
  diff --git a/qemu-char.c b/qemu-char.c
  index b597ee1..0361d78 100644
  --- a/qemu-char.c
  +++ b/qemu-char.c
  @@ -270,6 +270,7 @@ static int qemu_chr_fe_write_buffer(CharDriverState *s, 
const uint8_t *buf, int
   int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
   {
   int ret;
  +int offset = 0;
   
   if (s->replay && replay_mode == REPLAY_MODE_PLAY) {
   int offset;
  @@ -280,7 +281,21 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t 
*buf, int len)
   }
   
   qemu_mutex_lock(>chr_write_lock);
  -ret = s->chr_write(s, buf, len);
  +
  +while (offset < len) {
  +retry:
  +ret = s->chr_write(s, buf, len);
  +if (ret < 0 && errno == EAGAIN) {
  +g_usleep(100);
  +goto retry;
  +}
  +
  +if (ret <= 0) {
  +break;
  +}
  +
  +offset += ret;
  +}
   
   if (ret > 0) {
   qemu_chr_fe_write_log(s, buf, ret);
  ---

  Or please do "git revert 64c800f808748522727847b9cdc73412f22dffb9".

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

[Qemu-devel] [PATCH for-2.7] tests: Test blockjob IDs

[Alberto Garcia]

Since 7f0317cfc8da6 we have API to specify the ID of block jobs and we
also guarantee that they are well-formed and unique.

This patch adds tests to check some common scenarios.

Signed-off-by: Alberto Garcia 
---
 tests/Makefile.include |   2 +
 tests/test-blockjob.c  | 147 +
 2 files changed, 149 insertions(+)
 create mode 100644 tests/test-blockjob.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index ebecfa4..14be491 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -52,6 +52,7 @@ gcov-files-test-thread-pool-y = thread-pool.c
 gcov-files-test-hbitmap-y = util/hbitmap.c
 check-unit-y += tests/test-hbitmap$(EXESUF)
 gcov-files-test-hbitmap-y = blockjob.c
+check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
@@ -449,6 +450,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o 
$(test-block-obj-y)
 tests/test-aio$(EXESUF): tests/test-aio.o $(test-block-obj-y)
 tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o $(test-util-obj-y)
 tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
+tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) 
$(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o 
$(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
new file mode 100644
index 000..5b0e934
--- /dev/null
+++ b/tests/test-blockjob.c
@@ -0,0 +1,147 @@
+/*
+ * Blockjob tests
+ *
+ * Copyright Igalia, S.L. 2016
+ *
+ * Authors:
+ *  Alberto Garcia   
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/main-loop.h"
+#include "block/blockjob.h"
+#include "sysemu/block-backend.h"
+
+static const BlockJobDriver test_block_job_driver = {
+.instance_size = sizeof(BlockJob),
+};
+
+static void block_job_cb(void *opaque, int ret)
+{
+}
+
+static BlockJob *do_test_id(BlockBackend *blk, const char *id,
+bool should_succeed)
+{
+BlockJob *job;
+Error *errp = NULL;
+
+job = block_job_create(id, _block_job_driver, blk_bs(blk), 0,
+   block_job_cb, NULL, );
+if (should_succeed) {
+g_assert_null(errp);
+g_assert_nonnull(job);
+if (id) {
+g_assert_cmpstr(job->id, ==, id);
+} else {
+g_assert_cmpstr(job->id, ==, blk_name(blk));
+}
+} else {
+g_assert_nonnull(errp);
+g_assert_null(job);
+error_free(errp);
+}
+
+return job;
+}
+
+/* This creates a BlockBackend (optionally with a name) with a
+ * BlockDriverState inserted. */
+static BlockBackend *create_blk(const char *name)
+{
+BlockBackend *blk = blk_new();
+BlockDriverState *bs = bdrv_new();
+
+blk_insert_bs(blk, bs);
+bdrv_unref(bs);
+
+if (name) {
+Error *errp = NULL;
+monitor_add_blk(blk, name, );
+g_assert_null(errp);
+}
+
+return blk;
+}
+
+/* This destroys the backend */
+static void destroy_blk(BlockBackend *blk)
+{
+if (blk_name(blk)[0] != '\0') {
+monitor_remove_blk(blk);
+}
+
+blk_remove_bs(blk);
+blk_unref(blk);
+}
+
+static void test_job_ids(void)
+{
+BlockBackend *blk[3];
+BlockJob *job[3];
+
+blk[0] = create_blk(NULL);
+blk[1] = create_blk("drive1");
+blk[2] = create_blk("drive2");
+
+/* No job ID provided and the block backend has no name */
+job[0] = do_test_id(blk[0], NULL, false);
+
+/* These are all invalid job IDs */
+job[0] = do_test_id(blk[0], "0id", false);
+job[0] = do_test_id(blk[0], "",false);
+job[0] = do_test_id(blk[0], "   ", false);
+job[0] = do_test_id(blk[0], "123", false);
+job[0] = do_test_id(blk[0], "_id", false);
+job[0] = do_test_id(blk[0], "-id", false);
+job[0] = do_test_id(blk[0], ".id", false);
+job[0] = do_test_id(blk[0], "#id", false);
+
+/* This one is valid */
+job[0] = do_test_id(blk[0], "id0", true);
+
+/* We cannot have two jobs in the same BDS */
+do_test_id(blk[0], "id1", false);
+
+/* Duplicate job IDs are not allowed */
+job[1] = do_test_id(blk[1], "id0", false);
+
+/* But once job[0] finishes we can reuse its ID */
+block_job_unref(job[0]);
+job[1] = do_test_id(blk[1], "id0", true);
+
+/* No job ID specified, defaults to the backend name ('drive1') */
+block_job_unref(job[1]);
+job[1] = do_test_id(blk[1], NULL, true);
+
+/* Duplicate job ID */
+job[2] = do_test_id(blk[2], "drive1", false);
+

Re: [Qemu-devel] [RFC v4 5/6] hw/intc/arm_gicv3_its: Implement support for in-kernel ITS emulation

[Peter Maydell]

On 29 July 2016 at 15:13, Auger Eric  wrote:
> Hi Peter,
> On 29/07/2016 15:35, Peter Maydell wrote:
>> On 6 July 2016 at 10:46, Eric Auger  wrote:
>>> +/**
>>> + *
>>> + * We currently do not use kvm_arm_register_device to provide
>>> + * the kernel with the vITS control frame base address since the
>>> + * KVM_DEV_ARM_VGIC_CTRL_INIT init MUST be called after the
>>> + * KVM_ARM_SET_DEVICE_ADDR and the kvm_arm_register_device
>>> + * infra does not allow this.
>>> + */
>>
>> So the conclusion on the kvm-arm mailing list was that the
>> kernel ought to permit DEVICE_ADDR to be set after INIT,
>> the same way it does for the GIC proper, right?
> Yes this was indeed evoked but eventually what is in next, right now is
> the same implementation where KVM_ARM_SET_DEVICE_ADDR is forbidden once
> init is done:

That's bad and we need to fix it, preferably before it hits master.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] avx2 configure: Disable if static build

[Peter Maydell]

On 19 July 2016 at 20:12, Dr. David Alan Gilbert  wrote:
> * Aaron Lindsay (alind...@codeaurora.org) wrote:
>> This avoids a segfault like the following for at least some 4.8 versions
>> of gcc when configured with --static if avx2 instructions are also
>> enabled:
>>
>>   Program received signal SIGSEGV, Segmentation fault.
>>   buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
>>   333 {
>>   (gdb) bt
>>   #0  buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
>>   #1  0x00939c58 in __libc_start_main ()
>>   #2  0x00419337 in _start ()
>>
>> Signed-off-by: Aaron Lindsay 
>
> It does look like the simplest solution to the problem;
>
> Reviewed-by: Dr. David Alan Gilbert 

Applied to master, thanks.

-- PMM

Re: [Qemu-devel] [RFC v4 5/6] hw/intc/arm_gicv3_its: Implement support for in-kernel ITS emulation

[Auger Eric]

Hi Peter,
On 29/07/2016 15:35, Peter Maydell wrote:
> On 6 July 2016 at 10:46, Eric Auger  wrote:
>> +/**
>> + *
>> + * We currently do not use kvm_arm_register_device to provide
>> + * the kernel with the vITS control frame base address since the
>> + * KVM_DEV_ARM_VGIC_CTRL_INIT init MUST be called after the
>> + * KVM_ARM_SET_DEVICE_ADDR and the kvm_arm_register_device
>> + * infra does not allow this.
>> + */
> 
> So the conclusion on the kvm-arm mailing list was that the
> kernel ought to permit DEVICE_ADDR to be set after INIT,
> the same way it does for the GIC proper, right?
Yes this was indeed evoked but eventually what is in next, right now is
the same implementation where KVM_ARM_SET_DEVICE_ADDR is forbidden once
init is done:

see virt/kvm/arm/vgic/vgic-its.c
its->initialized set by vgic_its_init_its (KVM_DEV_ARM_VGIC_GRP_CTRL)

case KVM_DEV_ARM_VGIC_GRP_ADDR:
../..
if (its->initialized)
return -EBUSY;

Thanks

Eric

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v2 1/1] block/parallels: check new image size

[Stefan Hajnoczi]

On Wed, Jul 27, 2016 at 08:08:20PM +0300, Denis V. Lunev wrote:
> From: Klim Kireev 
> 
> Before this patch incorrect image could be created via qemu-img
> (Example: qemu-img create -f parallels -o size=4096T hack.img),
> incorrect images cannot be used due to overflow in main image structure.
> 
> This patch add check of size in image creation.
> 
> After reading size it compare it with UINT32_MAX * cluster_size.
> 
> Signed-off-by: Klim Kireev 
> Signed-off-by: Denis V. Lunev 
> CC: Stefan Hajnoczi 
> ---
> changes from v1:
> - fixed from: to be virtuozzo.mipt.com to match Signed-off-by
> 
>  block/parallels.c | 5 +
>  1 file changed, 5 insertions(+)

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

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH for-2.7] apic: fix broken migration for kvm-apic

[Igor Mammedov]

commit f6e98444 (apic: Use apic_id as apic's migration instance_id)
breaks migration when in kernel irqchip is used for 2.6 and older
machine types.

It applies compat property only for userspace 'apic' type
instead of applying it to all apic types inherited from
'apic-common' type as it was supposed to do.

Fix it by setting compat property 'legacy-instance-id' for
'apic-common' type which affects inherited types (i.e. not
only 'apic' but also 'kvm-apic' types)

Signed-off-by: Igor Mammedov 
---
 include/hw/i386/pc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index c87c5c1..74c175c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -388,7 +388,7 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 .value = "off",\
 },\
 {\
-.driver   = "apic",\
+.driver   = "apic-common",\
 .property = "legacy-instance-id",\
 .value= "on",\
 },
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH 00/11] Clock framework API.

[Peter Maydell]

On 13 June 2016 at 17:27,   wrote:
> From: KONRAD Frederic 
>
> Hi,
>
> This is a first draft of the clock framework API it contains:
>
>   * The first 5 patches which introduce the framework.
>   * The 6th patch which introduces a fixed-clock model.
>   * The rest which gives an example how to model a PLL from the existing
> zynqmp-crf extracted from the qemu xilinx tree.
>
> No specific behavior is expected yet when the CRF register set is accessed but
> the user can see for example the dp_video_ref and vpll_to_lpd rate changing in
> the monitor with the "info qtree" command when the vpll_ctrl register is
> modified.

Apologies for the 6-week-late review. I think mostly this is
along the right lines but I've given comments where I have
specific issues.

It might also be useful to sketch out how a simple clock
tree would look in the documentation file.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Unbreak FreeBSD build after optionrom update.

[Peter Maydell]

On 26 July 2016 at 22:05, Sean Bruno  wrote:
> Update the build flags appropriately for FreeBSD and add the
> correct LD_EMULATION type for the FreeBSD build case.
>
> Fixes FreeBSD build error:
> ld: unrecognised emulation mode: elf_i386
> Supported emulations: elf_x86_64_fbsd elf_i386_fbsd
> gmake[1]: *** [Makefile:51: linuxboot_dma.img] Error 1
> gmake: *** [Makefile:229: romsubdir-optionrom] Error 2
>
> Signed-off-by: Sean Bruno 
> ---

Applied, thanks.

-- PMM

Re: [Qemu-devel] [RFC PATCH 09/11] zynqmp_crf: add the clock mechanism

[Peter Maydell]

On 13 June 2016 at 17:27,   wrote:
> From: KONRAD Frederic 
>
> This adds the pll to the zynqmp_crf and the dp_video clock output.
>
> Signed-off-by: KONRAD Frederic 
> ---
>  hw/misc/xilinx_zynqmp_crf.c | 440 
> 
>  1 file changed, 440 insertions(+)
>
> diff --git a/hw/misc/xilinx_zynqmp_crf.c b/hw/misc/xilinx_zynqmp_crf.c
> index 4c670a0..2097534 100644
> --- a/hw/misc/xilinx_zynqmp_crf.c
> +++ b/hw/misc/xilinx_zynqmp_crf.c
> @@ -30,6 +30,7 @@
>  #include "hw/register.h"
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
> +#include "qemu/qemu-clock.h"
>
>  #ifndef XILINX_CRF_APB_ERR_DEBUG
>  #define XILINX_CRF_APB_ERR_DEBUG 0
> @@ -281,6 +282,38 @@ typedef struct CRF_APB {
>
>  uint32_t regs[R_MAX];
>  RegisterInfo regs_info[R_MAX];
> +
> +/* input clocks */
> +qemu_clk pss_ref_clk;
> +qemu_clk video_clk;
> +qemu_clk pss_alt_ref_clk;
> +qemu_clk aux_refclk;
> +qemu_clk gt_crx_ref_clk;
> +
> +/* internal clocks */
> +qemu_clk apll_clk;
> +qemu_clk dpll_clk;
> +qemu_clk vpll_clk;
> +
> +/* output clocks */
> +qemu_clk acpu_clk;
> +qemu_clk dbg_trace;
> +qemu_clk dbg_fdp;
> +qemu_clk dp_video_ref;
> +qemu_clk dp_audio_ref;
> +qemu_clk dp_stc_ref;
> +qemu_clk ddr;
> +qemu_clk gpu_ref;
> +qemu_clk sata_ref;
> +qemu_clk pcie_ref;
> +qemu_clk gdma_ref;
> +qemu_clk dpdma_ref;
> +qemu_clk topsw_main;
> +qemu_clk topsw_lsbus;
> +qemu_clk dbg_tstmp;
> +qemu_clk apll_to_lpd;
> +qemu_clk dpll_to_lpd;
> +qemu_clk vpll_to_lpd;
>  } CRF_APB;

This looks a bit weird. Why are the input clocks and the output
clocks the same type? I was expecting that an output clock would
be "owned" by this device (and so a qemu_clk), whereas an input
clock would just be a reference to a clock owned by the device
on the other end of it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device specification

[Stefan Hajnoczi]

On Wed, Jul 27, 2016 at 06:08:23PM +0800, Gonglei wrote:
> +The first driver-read-only field, \field{version} specifies the virtio 
> crypto??s
> +version, which is reserved for back-compatibility in future.It??s currently

Here...

> +Step2: Execute the detail encryption operation:
> +\begin{enumerate}
> +\item The driver fills out the encrypt requests;
> +\item Put the requests into dataq and kick the virtqueue;
> +\item The device executes the encryption operation according the requests?? 
> arguments;

...and here there are invalid characters.

The email was sent with charset UTF-8 but these bytes are not valid
UTF-8.

Please check your text editor and git-send-email(1) configuration so
that you send patches with the correct charset.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [RFC PATCH 08/11] zynqmp_crf: fix against AF_EX32 changes

[Peter Maydell]

On 13 June 2016 at 17:27,   wrote:
> From: KONRAD Frederic 
>
> This seems to be due to a difference between the AF_EX32 define.
>
> Signed-off-by: KONRAD Frederic 
> ---
>  hw/misc/xilinx_zynqmp_crf.c | 354 
> ++--
>  1 file changed, 177 insertions(+), 177 deletions(-)

This should just be squashed into the previous patch.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 05/11] docs: add qemu-clock documentation

[Peter Maydell]

On 13 June 2016 at 17:27,   wrote:
> From: KONRAD Frederic 
>
> This adds the qemu-clock documentation.
>
> Signed-off-by: KONRAD Frederic 
> ---
>  docs/clock.txt | 112 
> +
>  1 file changed, 112 insertions(+)
>  create mode 100644 docs/clock.txt
>
> diff --git a/docs/clock.txt b/docs/clock.txt
> new file mode 100644
> index 000..f4ad4c8
> --- /dev/null
> +++ b/docs/clock.txt
> @@ -0,0 +1,112 @@
> +
> +What is a QEMU_CLOCK
> +
> +
> +A QEMU_CLOCK is a QOM Object developed for the purpose of modeling a clock 
> tree
> +with QEMU.
> +
> +It only simulates the clock by keeping a copy of the current frequency and
> +doesn't model the signal itself such as pin toggle or duty cycle.
> +
> +It allows to model the impact of badly configured PLL, clock source selection
> +or disabled clock on the models.

I think this would be a good place to give a brief bullet-point summary
of the operations that can be done on a clock (which then can be
expanded on in sections below).


> +
> +Bounding the clock together to create a tree

"Binding" ?

> +
> +
> +In order to create a clock tree with QEMU_CLOCK two or more clock must be 
> bound
> +together.

Is the simple case of "only one clock, not bound to any other clock,
with a callback" not a valid clock tree ?

> Let's say there are two clocks clk_a and clk_b:
> +Using qemu_clk_bound(clk_a, clk_b) will bound clk_a and clk_b.

"bind" ? (and in the function name)

> +
> +Binding two qemu-clk together is a unidirectional link which means that 
> changing

"creates a unidirectional link" ?

> +the rate of clk_a will propagate to clk_b and not the opposite. The bound
> +process automatically refresh clk_b rate.

"refreshes"

> +
> +Clock can be bound and unbound during execution for modeling eg: a clock
> +selector.
> +
> +A clock can drive more than one other clock. eg with this code:
> +qemu_clk_bound(clk_a, clk_b);
> +qemu_clk_bound(clk_a, clk_c);
> +
> +A clock rate change one clk_a will propagate to clk_b and clk_c.

If you bind clock A to clock B, then is trying to change the rate
of clock B (a) a bug (assert, or fail the call) (b) OK but
ignored (c) OK but the changed rate doesn't get used until/unless
the clocks are unbound ?

Is it possible to bind two clocks together but with a frequency
difference between them (ie to model the common case where there's
some master clock and then a clock divider that creates various
other clocks based on the rate of that master) ?

> +
> +Implementing a callback on a rate change
> +
> +
> +The function prototype is the following:
> +typedef float (*qemu_clk_rate_change_cb)(void *opaque, float rate);
> +
> +It's main goal is to modify the rate before it's passed to the next clocks in
> +the tree.
> +
> +eg: for a 4x PLL the function will be:
> +float qemu_clk_rate_change_cb(void *opaque, float rate)
> +{
> +return 4.0 * rate;
> +}
> +
> +To set the callback for the clock:
> +void qemu_clk_set_callback(qemu_clk clk, qemu_clk_on_rate_update_cb cb,
> +   void *opaque);
> +can be called.
> +
> +NOTE: It's not recommended that the clock is driven by more than one clock 
> as it

did you mean "callback" here ?

> +would mean that we don't know which clock trigger the callback.
> +
> +The rate update process
> +===
> +
> +The rate update happen in this way:
> +When a model wants to update a clock frequency (eg: based on a register 
> change
> +or something similar) it will call qemu_clk_update_rate(..) on the clock:
> +  * The callback associated to the clock is called with the new rate.
> +  * qemu_clk_update_rate(..) is then called on all bound clock with the
> +value returned by the callback.
> +
> +NOTE: When no callback is attached the clock qemu_clk_update_rate(..) is 
> called
> +with the unmodified rate.
> +
> +Attaching a QEMU_CLOCK to a DeviceState
> +===
> +
> +Attaching a qemu-clk to a DeviceState is required to be able to get the clock
> +outside the model through qemu_clk_get_pin(..).
> +
> +It is also required to be able to print the clock and its rate with info 
> qtree.
> +For example:
> +
> +  type System
> +  dev: xlnx.zynqmp_crf, id ""
> +gpio-out "sysbus-irq" 1
> +gpio-out "RST_A9" 4
> +qemu-clk "dbg_trace" 0.0
> +qemu-clk "vpll_to_lpd" 62500.0
> +qemu-clk "dp_stc_ref" 0.0
> +qemu-clk "dpll_to_lpd" 1250.0
> +qemu-clk "acpu_clk" 0.0
> +qemu-clk "pcie_ref" 0.0
> +qemu-clk "topsw_main" 0.0
> +qemu-clk "topsw_lsbus" 0.0
> +qemu-clk "dp_audio_ref" 0.0
> +qemu-clk "sata_ref" 0.0
> +qemu-clk "dp_video_ref" 71428568.0
> +qemu-clk "vpll_clk" 25.0
> +qemu-clk "apll_to_lpd" 1250.0
> +qemu-clk "dpll_clk" 5000.0
> +

Re: [Qemu-devel] [RFC PATCH 03/11] qemu-clk: allow to bound two clocks together

[Peter Maydell]

On 13 June 2016 at 17:27,   wrote:
> From: KONRAD Frederic 
>
> This introduces the clock binding and the update part.
> When the qemu_clk_rate_update(qemu_clk, int) function is called:
>   * The clock callback is called on the qemu_clk so it can change the rate.
>   * The qemu_clk_rate_update function is called on all the driven clock.
>
> Signed-off-by: KONRAD Frederic 
> ---
>  include/qemu/qemu-clock.h | 65 
> +++
>  qemu-clock.c  | 56 
>  2 files changed, 121 insertions(+)
>
> diff --git a/include/qemu/qemu-clock.h b/include/qemu/qemu-clock.h
> index a2ba105..677de9a 100644
> --- a/include/qemu/qemu-clock.h
> +++ b/include/qemu/qemu-clock.h
> @@ -27,15 +27,29 @@
>  #include "qemu/osdep.h"
>  #include "qom/object.h"
>
> +typedef float (*qemu_clk_on_rate_update_cb)(void *opaque, float rate);
> +
>  #define TYPE_CLOCK "qemu-clk"
>  #define QEMU_CLOCK(obj) OBJECT_CHECK(struct qemu_clk, (obj), TYPE_CLOCK)
>
> +typedef struct ClkList ClkList;
> +
>  typedef struct qemu_clk {
>  /*< private >*/
>  Object parent_obj;
>  char *name;/* name of this clock in the device. */
> +float in_rate; /* rate of the clock which drive this pin. */
> +float out_rate;/* rate of this clock pin. */

I'm rather dubious that we should be using floats here.
Almost nothing in our hardware emulation uses float or double.

> +void *opaque;
> +qemu_clk_on_rate_update_cb cb;
> +QLIST_HEAD(, ClkList) bound;
>  } *qemu_clk;

thanks
-- PMM

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Halil Pasic]

On 07/28/2016 11:03 PM, Eric Blake wrote:
> On 07/28/2016 09:29 AM, Halil Pasic wrote:
> 
>>> You mean va_start, not start_va.  And actually, C11 is clear that errno
>>> is unspecified after library functions (but not macros) that don't
>>> explicitly state otherwise.  Since va_start() is a macro and not a
>>> library function, that means va_start does NOT have carte blanche
>>> permission to modify errno.  For more reading on the topic:
>>
>> I also considered this function/macro thing but in the end I am not
>> aware of anything in C11 what would prohibit va_start to modify errno --
>> correct me if I'm wrong. With that it boils down to 'may' and relying on
>> 'does not' means you are not covered by the standard C11 (but may
>> be covered by something else -- in which case this should be documented
>> in HACKING).
>>
>>>
>>> http://austingroupbugs.net/view.php?id=384
>>>
>>
>> This got rejected, or? Means that there is no willingness to introduce
>> this guarantee at POSIX level?
>>  
> 
> That particular bug report was rejected because the POSIX folks decided
> that the C11 wording was clear enough that va_start() was already
> guaranteed to not mess with errno, so no additionally wording was needed
> in POSIX.
> 

Sadly, I still do not get it. I have re-read the relevant parts of N1570
and even had a conversation with the in house compiler team. The
compiler guy's opinion was also that there is no guarantee provided by
C11. In http://austingroupbugs.net/view.php?id=384 you stated in the
description that the code example provided there is not conforming. Your
last reply I read like you were wrong with that statement. I still do
not understand why were you wrong there. In fact, I could argue that you
were right, but I'm afraid the argument would be somewhat lengthy and
confusing, and I'm already feeling bad about taking so much of your time
with this. Since I'm  admittedly quite inexperienced in this field I
decided to just accept your the conclusion you and the POSIX guys
reached -- without fully understanding it.

Thanks again for your time.

Regards,
Halil




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] bug in usb_bus_release() ?

[Juergen Gross]

On 27/07/16 16:56, Juergen Gross wrote:
> I can reproduce a problem in qemu with Xen just by adding and removing a
> USB bus. The bus is added via usb_bus_new() in hw/usb/xen-usb.c and
> removed later via usb_bus_release().
> 
> Nothing bad happens until I close an active VNC viewer connected to the
> graphical console emulated by the same qemu process. In the log file I
> see "*** Error in `/usr/lib/xen/bin/qemu-system-i386': corrupted double-
> linked list: 0x55b236bd56b0 ***" and the qemu process will hang.
> 
> Looking into the sources I suspected a missing
> 
> object_unparent(OBJECT(>bus));
> 
> after calling usb_bus_release(>bus) to be the culprit, but
> adding this call didn't help (shouldn't this be called from
> usb_bus_release() instead?)
> 
> I suspect something else is missing in qemu for removing a USB bus
> without leaking resources, but I couldn't find anything up to now. Does
> anyone have an idea what could be wrong?

Okay, problem solved. The bug was completely unrelated to
usb_bus_release(). It was just always triggered just after that call.
The problem was in Xen backend handling releasing more memory than
desired. Patch already sent out.


Juergen

Re: [Qemu-devel] [RFC v4 5/6] hw/intc/arm_gicv3_its: Implement support for in-kernel ITS emulation

[Peter Maydell]

On 6 July 2016 at 10:46, Eric Auger  wrote:
> +/**
> + *
> + * We currently do not use kvm_arm_register_device to provide
> + * the kernel with the vITS control frame base address since the
> + * KVM_DEV_ARM_VGIC_CTRL_INIT init MUST be called after the
> + * KVM_ARM_SET_DEVICE_ADDR and the kvm_arm_register_device
> + * infra does not allow this.
> + */

So the conclusion on the kvm-arm mailing list was that the
kernel ought to permit DEVICE_ADDR to be set after INIT,
the same way it does for the GIC proper, right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] optionrom: fix detection of -Wa,-32

[Peter Maydell]

On 20 July 2016 at 20:36, Paolo Bonzini  wrote:
> The cc-option macro runs $(CC) in -S mode (generate assembly) to avoid a
> pointless run of the assembler.  However, this does not work when you want
> to detect support for cc->as option passthrough.  clang ignores -Wa unless
> -c is provided, and exits successfully even if the -Wa,-32 option is not
> supported.
>
> Reported-by: Stefan Hajnoczi 
> Signed-off-by: Paolo Bonzini 
> ---

Applied to master, thanks (since it fixes BSD builds).

-- PMM

Re: [Qemu-devel] target-ppc: SPR_BOOKE_ESR not set on FP exceptions

[alarson]

David Gibson  wrote on 07/29/2016 12:40:15 
AM:

> From: David Gibson 
> To: alar...@ddci.com
> Cc: qemu-devel@nongnu.org, qemu-...@nongnu.org, ag...@suse.de
> Date: 07/29/2016 12:38 AM
> Subject: Re: target-ppc: SPR_BOOKE_ESR not set on FP exceptions
> 
> On Thu, Jul 28, 2016 at 06:32:27PM -0500, alar...@ddci.com wrote:
...
> > I did a quick check of the bits set in the POWERPC_EXCP_PROGRAM case.
> > The classic PPC sets SRR1 bits 11--15 depending on the exception.  In
> > Book E these correspond to bits 43--47,
> 
> Um.. what?  I'm not understanding where this bit shift is coming
> from. 

Sorry, I was looking at an old "classic" 32-bit manual for the SRR1 
exception definition and a 64-bit manual for the BookE.  They are
the same bits.  MSB0 bit numbering bytes again :-)

Re: [Qemu-devel] [PULL 0/2] target-mips queue

[Peter Maydell]

On 29 July 2016 at 10:11, Leon Alrae <leon.al...@imgtec.com> wrote:
> Hi,
>
> Just a couple of bug fixes for rc1.
>
> Thanks,
> Leon
>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: Aurelien Jarno <aurel...@aurel32.net>
>
> The following changes since commit 21a21b853a1bb606358af61e738abfb9aecbd720:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
> staging (2016-07-27 18:18:21 +0100)
>
> are available in the git repository at:
>
>   git://github.com/lalrae/qemu.git tags/mips-20160729
>
> for you to fetch changes up to 701074a6fc7470d0ed54e4a4bcd4d491ad8da22e:
>
>   target-mips: fix EntryHi.EHINV being cleared on TLB exception (2016-07-28 
> 11:24:02 +0100)
>
> 
> MIPS patches 2016-07-29
>
> Changes:
> * bug fixes

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Marc-André Lureau]

Hi

On Thu, Jul 28, 2016 at 11:07 AM, Prerna Saxena  wrote:
> From: Prerna Saxena 
>
> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>
> If negotiated, client applications should send a u64 payload in
> response to any message that contains the "need_reply" bit set
> on the message flags. Setting the payload to "zero" indicates the
> command finished successfully. Likewise, setting it to "non-zero"
> indicates an error.
>
> Currently implemented only for SET_MEM_TABLE.
>
> Signed-off-by: Prerna Saxena 
> ---
>  docs/specs/vhost-user.txt | 26 ++
>  hw/virtio/vhost-user.c| 32 
>  2 files changed, 58 insertions(+)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 777c49c..54b5c8f 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
>   * Flags: 32-bit bit field:
> - Lower 2 bits are the version (currently 0x01)
> - Bit 2 is the reply flag - needs to be sent on each reply from the slave
> +   - Bit 3 is the need_reply flag - see VHOST_USER_PROTOCOL_F_REPLY_ACK for
> + details.
>   * Size - 32-bit size of the payload
>
>
> @@ -126,6 +128,8 @@ the ones that do:
>   * VHOST_GET_VRING_BASE
>   * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
>
> +[ Also see the section on REPLY_ACK protocol extension. ]
> +
>  There are several messages that the master sends with file descriptors passed
>  in the ancillary data:
>
> @@ -254,6 +258,7 @@ Protocol features
>  #define VHOST_USER_PROTOCOL_F_MQ 0
>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD  1
>  #define VHOST_USER_PROTOCOL_F_RARP   2
> +#define VHOST_USER_PROTOCOL_F_REPLY_ACK  3
>
>  Message types
>  -
> @@ -464,3 +469,24 @@ Message types
>is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>The first 6 bytes of the payload contain the mac address of the guest 
> to
>allow the vhost user backend to construct and broadcast the fake RARP.
> +
> +VHOST_USER_PROTOCOL_F_REPLY_ACK:
> +---
> +The original vhost-user specification only demands replies for certain
> +commands. This differs from the vhost protocol implementation where commands
> +are sent over an ioctl() call and block until the client has completed.
> +
> +With this protocol extension negotiated, the sender (QEMU) can set the
> +"need_reply" [Bit 3] flag to any command. This indicates that
> +the client MUST respond with a Payload VhostUserMsg indicating success or
> +failure. The payload should be set to zero on success or non-zero on failure.
> +(Unless the message already has an explicit reply body)

Unless/unless (for consistency, the rest of the document doesn't use
Upper-case inside parentheses)

> +
> +This indicates to QEMU that the requested operation has deterministically
> +been met or not. Today, QEMU is expected to terminate the main vhost-user
> +loop upon receiving such errors. In future, qemu could be taught to be more
> +resilient for selective requests.
> +
> +For the message types that already solicit a reply from the client, the
> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set 
> brings
> +no behaviourial change. (See the 'Communication' section for details.)

See/see

> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 495e09f..86e7ae0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -31,6 +31,7 @@ enum VhostUserProtocolFeature {
>  VHOST_USER_PROTOCOL_F_MQ = 0,
>  VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
>  VHOST_USER_PROTOCOL_F_RARP = 2,
> +VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
>
>  VHOST_USER_PROTOCOL_F_MAX
>  };
> @@ -84,6 +85,7 @@ typedef struct VhostUserMsg {
>
>  #define VHOST_USER_VERSION_MASK (0x3)
>  #define VHOST_USER_REPLY_MASK   (0x1<<2)
> +#define VHOST_USER_NEED_REPLY_MASK   (0x1 << 3)

You could align it, and use the same style as the line above

>  uint32_t flags;
>  uint32_t size; /* the following payload size */
>  union {
> @@ -158,6 +160,25 @@ fail:
>  return -1;
>  }
>
> +static int process_message_reply(struct vhost_dev *dev,
> +VhostUserRequest request)

bad indentation

> +{
> +VhostUserMsg msg;
> +
> +if (vhost_user_read(dev, ) < 0) {
> +return 0;

return -1

> +}
> +
> +if (msg.request != request) {
> +error_report("Received unexpected msg type."
> +"Expected %d received %d",
> +request, msg.request);

bad indentation

> +return -1;
> +}
> +
> +return msg.payload.u64 ? -1 : 0;
> +}
> +
>  static bool vhost_user_one_time_request(VhostUserRequest request)
>  {
>  switch (request) {
> @@ -239,11 +260,18 @@ static int vhost_user_set_mem_table(struct vhost_dev 
> 

Re: [Qemu-devel] [PATCH 28/32] ppc: Avoid double translation for lvx/lvxl/stvx/stvxl

[Richard Henderson]

On 07/29/2016 02:30 PM, Benjamin Herrenschmidt wrote:

 DEF_HELPER_3(lmw, void, env, tl, i32)
-DEF_HELPER_3(stmw, void, env, tl, i32)
+DEF_HELPER_FLAGS_3(stmw, TCG_CALL_NO_WG, void, env, tl, i32)
 DEF_HELPER_4(lsw, void, env, tl, i32, i32)
 DEF_HELPER_5(lswx, void, env, tl, i32, i32, i32)
-DEF_HELPER_4(stsw, void, env, tl, i32, i32)
-DEF_HELPER_3(dcbz, void, env, tl, i32)
-DEF_HELPER_2(icbi, void, env, tl)
+DEF_HELPER_FLAGS_4(stsw, TCG_CALL_NO_WG, void, env, tl, i32, i32)
+DEF_HELPER_FLAGS_3(dcbz, TCG_CALL_NO_WG, void, env, tl, i32)
+DEF_HELPER_FLAGS_2(icbi, TCG_CALL_NO_WG, void, env, tl)
 DEF_HELPER_5(lscbx, tl, env, tl, i32, i32, i32)

 #if defined(TARGET_PPC64)

If my understanding is right, the above is correct, as none of these
instructions will write to the env, though they can read from it and/
or generate faults.

Sadly I haven't observed any performance improvement as a result in
a few micro-benchmarks I cooked up.


Too bad.

But the changes look correct, so you might as well queue them.  ;-)


r~

Re: [Qemu-devel] [PATCH v1 4/8] target-ppc: add vabsdu[b, h, w] instructions

[Richard Henderson]

On 07/29/2016 10:26 AM, David Gibson wrote:

Btw, your mailer seems to have screwed up and sent this as html only.


"Sent from my phone" should have been the disclaimer.  ;-)


r~

Re: [Qemu-devel] [PATCH 28/32] ppc: Avoid double translation for lvx/lvxl/stvx/stvxl

[Richard Henderson]

On 07/29/2016 12:26 PM, Benjamin Herrenschmidt wrote:

I notice that sadly, all of the vector ops are helper with full
clobbers, because I assume, the "avr" is passed as pointer due to the
lack of an int128 type in TCG correct ?


Yes.  Although x86 doesn't declare the vector registers as tcg registers at 
all.  Which is both a blessing and a curse.


My opinion is that aarch64 is the model to aim for -- most operations can 
operate on 64-bit slices at a time, no registers clobbered.  There will always 
be exceptions, of course, but hopefully with rarer insns.



r~

Re: [Qemu-devel] [PATCH 28/32] ppc: Avoid double translation for lvx/lvxl/stvx/stvxl

[Richard Henderson]

On 07/29/2016 12:26 PM, Benjamin Herrenschmidt wrote:

So I can't do something like set TCG_CALL_NO_RWG (nor TCG_CALL_NO_WG)
on something like dcbz because it can take an exception, correct ?


You can't set NO_RWG, but you can say NO_WG.  It "reads" the registers via the 
exception (and it certainly has side effects).  But it doesn't write to the 
registers (along the normal return path; a loophole I've used elsewhere).



r~

[Qemu-devel] [Bug 1599214] Re: virtlogd: qemu 2.6.0 doesn't log boot message

[Daniel Berrange]

Patch at https://lists.gnu.org/archive/html/qemu-
devel/2016-07/msg06708.html

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

Title:
  virtlogd: qemu 2.6.0 doesn't log boot message

Status in QEMU:
  New

Bug description:
  This report is related to the OpenStack Nova bug [1].

  OpenStack tries to utilize the "virtlogd" feature of libvirt which
  gets provided by qemu with [2].

  steps to reproduce:
  1) launch a quest with qemu 2.6.0 which uses virtlogd for the stdout/stderr 
of its char device
  2) check the contents of the backing file of that char device

  expected result:
  The boot messages of the guest are logged in this file

  actual result:
  The file is empty

  notes:
  When I'm connected to that char device and reboot the guest, I see the boot 
messages in the terminal and also in the backing log file.

  References:
  [1] https://bugs.launchpad.net/nova/+bug/1597789
  [2] 
http://git.qemu.org/?p=qemu.git;a=blobdiff;f=qemu-char.c;h=11caa5648de99c9e0ee158f280fbc02ab05915d3;hp=d7be1851e5e9d268aa924a05958da292b048839c;hb=d0d7708ba29cbcc343364a46bff981e0ff88366f;hpb=f1c17521e79df863a5771d96974fab0d07f02be0

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

Re: [Qemu-devel] [PULL 0/6] ppc-for-2.7 queue 20160729

[Peter Maydell]

On 29 July 2016 at 05:53, David Gibson <da...@gibson.dropbear.id.au> wrote:
> The following changes since commit 21a21b853a1bb606358af61e738abfb9aecbd720:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
> staging (2016-07-27 18:18:21 +0100)
>
> are available in the git repository at:
>
>   git://github.com/dgibson/qemu.git tags/ppc-for-2.7-20160729
>
> for you to fetch changes up to 059ce0f00af0124740bcb9991d160f93c92ae174:
>
>   tests: add drive_del-test to ppc/ppc64 (2016-07-29 14:14:15 +1000)
>
> 
> ppc patch queue 2016-07-29
>
> Here are the current pending ppc and spapr related patches for
> qemu-2.7.  Given the freeze status, these are all bugfixes, with two
> exceptions:
>
>   * There's some final rework of the vcpu hotplug model.  Specifically
> we add spapr specific code on the generic basis Igor established
> to make cpu_index stable for pseries-2.7 and later machine types.
>   - This allows us to remove the limitation that cpu cores had to
> be inserted in linear order, and removed in LIFO order.
>   - This is worth merging this late in 2.7 because it will avoid
> considerable future grief with management layers needing to
> discover whether out-of-order hotplug is possible, amongst
> other things.
>   - For now we do add a constraint that the initial cpu cannot be
> unplugged.
>   * We add two extra testcases to make check, for postcopy and
> drive_del on ppc64.
>   - Not strictly bugfixes, but safe, because they don't affect the
> actual code, and increase test coverage.
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 00/41] pc, pci, virtio: cleanups, fixes

[Peter Maydell]

On 29 July 2016 at 04:14, Michael S. Tsirkin  wrote:
> The following changes since commit 2d2e632ad00d11867c6c5625605b1fbc022dd62f:
>
>   Update version for v2.7.0-rc0 release (2016-07-22 15:32:42 +0100)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to f077f889121c89612eaf0f21e94b97aa110a6910:
>
>   mptsas: Fix a migration compatible issue (2016-07-29 06:09:55 +0300)
>
> 
> pc, pci, virtio: cleanups, fixes
>
> a bunch of bugfixes and a couple of cleanups
> making these easier and/or making debugging easier
>
> Signed-off-by: Michael S. Tsirkin 
>
> 

Applied, thanks.

-- PMM

[Qemu-devel] [PATCH] virtio-console: set frontend open permanently for console devs

[Daniel P. Berrange]

The virtio-console.c file handles both serial consoles
and interactive consoles, since they're backed by the
same device model.

Since serial devices are expected to be reliable and
need to notify the guest when the backend is opened
or closed, the virtio-console.c file wires up support
for chardev events. This affects both serial consoles
and interactive consoles, using a network connection
based chardev backend such as 'socket', but not when
using a PTY based backend or plain 'file' backends.

When a device is open, however, the behaviour is
different - if the backend chardev returns EAGAIN or
a short write, the serial console will block and
setup a watch to poll for writability, ensuring no
data is lost.  The interactive consoles meanwhile
will simply discard data.

This means that the interactive consoles have different
blocking behaviour depending on whether the chardev is
open or not. If open, data may be discarded if not
consumed, where as if closed, data will always be queued
pending an open.

This behaviour is unhelpful in general - applications
outputting messages on the guest console should not be
blocked simply because no client is conencted to the host
side.

Consider for example, configuring a x86_64 guest with a
plain UART serial port

  -chardev 
socket,id=charserial1,host=127.0.0.1,port=9001,server,nowait,logfile=console1.log,logappend=on
  -device isa-serial,chardev=charserial1,id=serial1

vs a s390 guest which has to use the virtio-console port

  -chardev 
socket,id=charconsole1,host=127.0.0.1,port=9000,server,nowait,logfile=console2.log,logappend=on
  -device virtconsole,chardev=charconsole1,id=console1

The isa-serial one gets data written to the log regardless
of whether a client is connected, while the virtioconsole
one only gets data written to the log when a client is
connected.

This patch changes the behaviour so that virtconsole
devices work in same way as other traditional console
devices. Specifically, the frontend will now be marked
as permanently open, so data flows regardless of the
backend status.

NB, the behaviour of virtserialport devices is *not*
changed, only virtconsole.

Fixes bug: https://bugs.launchpad.net/qemu/+bug/1599214

Signed-off-by: Daniel P. Berrange 
---
 hw/char/virtio-console.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

NB If there is a concern about backends compatibility with
this change, we could instead add a boolean property to
the virtio-console device 'explicit-open' which controls
whether the virtconsole device has the old behaviour or
the new behaviour and default to old. Personally I think
it is fine to just change behaviour for virtconsole
unconditionally though

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 2e36481..4f0e03d 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -85,8 +85,9 @@ static void set_guest_connected(VirtIOSerialPort *port, int 
guest_connected)
 {
 VirtConsole *vcon = VIRTIO_CONSOLE(port);
 DeviceState *dev = DEVICE(port);
+VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
-if (vcon->chr) {
+if (vcon->chr && !k->is_console) {
 qemu_chr_fe_set_open(vcon->chr, guest_connected);
 }
 
@@ -156,9 +157,25 @@ static void virtconsole_realize(DeviceState *dev, Error 
**errp)
 }
 
 if (vcon->chr) {
-vcon->chr->explicit_fe_open = 1;
-qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-  vcon);
+/*
+ * For consoles we don't block guest data transfer just
+ * because nothing is connected - we'll just let it go
+ * whetherever the chardev wants - /dev/null probably.
+ *
+ * For serial ports we need 100% reliable data transfer
+ * so we use the opened/closed signals from chardev to
+ * trigger open/close of the device
+ */
+if (k->is_console) {
+vcon->chr->explicit_fe_open = 0;
+qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
+  NULL, vcon);
+virtio_serial_open(port);
+} else {
+vcon->chr->explicit_fe_open = 1;
+qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read,
+  chr_event, vcon);
+}
 }
 }
 
-- 
2.5.5

[Qemu-devel] [PATCH 2/2] xen: drain submit queue in xen-usb before removing device

[Juergen Gross]

When unplugging a device in the Xen pvusb backend drain the submit
queue before deallocation of the control structures. Otherwise there
will be bogus memory accesses when I/O contracts are finished.

Correlated to this issue is the handling of cancel requests: a packet
cancelled will still lead to the call of complete, so add a flag
to the request indicating it should be just dropped on complete.

Signed-off-by: Juergen Gross 
---
 hw/usb/xen-usb.c | 93 
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 7992456..6ad666e 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -90,6 +90,8 @@ struct usbback_req {
 void *buffer;
 void *isoc_buffer;
 struct libusb_transfer   *xfer;
+
+bool cancelled;
 };
 
 struct usbback_hotplug {
@@ -301,20 +303,23 @@ static void usbback_do_response(struct usbback_req 
*usbback_req, int32_t status,
 usbback_req->isoc_buffer = NULL;
 }
 
-res = RING_GET_RESPONSE(>urb_ring, usbif->urb_ring.rsp_prod_pvt);
-res->id = usbback_req->req.id;
-res->status = status;
-res->actual_length = actual_length;
-res->error_count = error_count;
-res->start_frame = 0;
-usbif->urb_ring.rsp_prod_pvt++;
-RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(>urb_ring, notify);
+if (usbif->urb_sring) {
+res = RING_GET_RESPONSE(>urb_ring, 
usbif->urb_ring.rsp_prod_pvt);
+res->id = usbback_req->req.id;
+res->status = status;
+res->actual_length = actual_length;
+res->error_count = error_count;
+res->start_frame = 0;
+usbif->urb_ring.rsp_prod_pvt++;
+RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(>urb_ring, notify);
 
-if (notify) {
-xen_be_send_notify(xendev);
+if (notify) {
+xen_be_send_notify(xendev);
+}
 }
 
-usbback_put_req(usbback_req);
+if (!usbback_req->cancelled)
+usbback_put_req(usbback_req);
 }
 
 static void usbback_do_response_ret(struct usbback_req *usbback_req,
@@ -366,15 +371,14 @@ static void usbback_set_address(struct usbback_info 
*usbif,
 }
 }
 
-static bool usbback_cancel_req(struct usbback_req *usbback_req)
+static void usbback_cancel_req(struct usbback_req *usbback_req)
 {
-bool ret = false;
-
 if (usb_packet_is_inflight(_req->packet)) {
 usb_cancel_packet(_req->packet);
-ret = true;
+QTAILQ_REMOVE(_req->stub->submit_q, usbback_req, q);
+usbback_req->cancelled = true;
+usbback_do_response_ret(usbback_req, -EPROTO);
 }
-return ret;
 }
 
 static void usbback_process_unlink_req(struct usbback_req *usbback_req)
@@ -391,7 +395,7 @@ static void usbback_process_unlink_req(struct usbback_req 
*usbback_req)
 devnum = usbif_pipedevice(usbback_req->req.pipe);
 if (unlikely(devnum == 0)) {
 usbback_req->stub = usbif->ports +
-usbif_pipeportnum(usbback_req->req.pipe);
+usbif_pipeportnum(usbback_req->req.pipe) - 1;
 if (unlikely(!usbback_req->stub)) {
 ret = -ENODEV;
 goto fail_response;
@@ -406,9 +410,7 @@ static void usbback_process_unlink_req(struct usbback_req 
*usbback_req)
 
 QTAILQ_FOREACH(unlink_req, _req->stub->submit_q, q) {
 if (unlink_req->req.id == id) {
-if (usbback_cancel_req(unlink_req)) {
-usbback_do_response_ret(unlink_req, -EPROTO);
-}
+usbback_cancel_req(unlink_req);
 break;
 }
 }
@@ -681,6 +683,31 @@ static void usbback_hotplug_enq(struct usbback_info 
*usbif, unsigned port)
 usbback_hotplug_notify(usbif);
 }
 
+static void usbback_portid_drain(struct usbback_info *usbif, unsigned port)
+{
+struct usbback_req *req, *tmp;
+bool sched = false;
+
+QTAILQ_FOREACH_SAFE(req, >ports[port - 1].submit_q, q, tmp) {
+usbback_cancel_req(req);
+sched = true;
+}
+
+if (sched)
+qemu_bh_schedule(usbif->bh);
+}
+
+static void usbback_portid_detach(struct usbback_info *usbif, unsigned port)
+{
+if (!usbif->ports[port - 1].attached)
+return;
+
+usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
+usbif->ports[port - 1].attached = false;
+usbback_portid_drain(usbif, port);
+usbback_hotplug_enq(usbif, port);
+}
+
 static void usbback_portid_remove(struct usbback_info *usbif, unsigned port)
 {
 USBPort *p;
@@ -694,9 +721,7 @@ static void usbback_portid_remove(struct usbback_info 
*usbif, unsigned port)
 
 object_unparent(OBJECT(usbif->ports[port - 1].dev));
 usbif->ports[port - 1].dev = NULL;
-usbif->ports[port - 1].speed = USBIF_SPEED_NONE;
-usbif->ports[port - 1].attached = false;
-usbback_hotplug_enq(usbif, port);
+usbback_portid_detach(usbif, port);
 
 TR_BUS(>xendev, "port %d removed\n", port);
 

[Qemu-devel] [PATCH 1/2] xen: when removing a backend don't remove many of them

[Juergen Gross]

When a Xenstore watch fires indicating a backend has to be removed
don't remove all backends for that domain with the specified device
index, but just the one which has the correct type.

The easiest way to achieve this is to use the already determined
xendev as parameter for xen_be_del_xendev() instead of only the domid
and device index.

This at once removes the open coded QTAILQ_FOREACH_SAVE() in
xen_be_del_xendev() as there is no need to search for the correct
xendev any longer.

Signed-off-by: Juergen Gross 
---
 hw/xen/xen_backend.c | 58 +---
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index bab79b1..3ceb778 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -321,48 +321,28 @@ static struct XenDevice *xen_be_get_xendev(const char 
*type, int dom, int dev,
 /*
  * release xen backend device.
  */
-static struct XenDevice *xen_be_del_xendev(int dom, int dev)
+static void xen_be_del_xendev(struct XenDevice *xendev)
 {
-struct XenDevice *xendev, *xnext;
-
-/*
- * This is pretty much like QTAILQ_FOREACH(xendev, , next) but
- * we save the next pointer in xnext because we might free xendev.
- */
-xnext = xendevs.tqh_first;
-while (xnext) {
-xendev = xnext;
-xnext = xendev->next.tqe_next;
-
-if (xendev->dom != dom) {
-continue;
-}
-if (xendev->dev != dev && dev != -1) {
-continue;
-}
-
-if (xendev->ops->free) {
-xendev->ops->free(xendev);
-}
-
-if (xendev->fe) {
-char token[XEN_BUFSIZE];
-snprintf(token, sizeof(token), "fe:%p", xendev);
-xs_unwatch(xenstore, xendev->fe, token);
-g_free(xendev->fe);
-}
+if (xendev->ops->free) {
+xendev->ops->free(xendev);
+}
 
-if (xendev->evtchndev != NULL) {
-xenevtchn_close(xendev->evtchndev);
-}
-if (xendev->gnttabdev != NULL) {
-xengnttab_close(xendev->gnttabdev);
-}
+if (xendev->fe) {
+char token[XEN_BUFSIZE];
+snprintf(token, sizeof(token), "fe:%p", xendev);
+xs_unwatch(xenstore, xendev->fe, token);
+g_free(xendev->fe);
+}
 
-QTAILQ_REMOVE(, xendev, next);
-g_free(xendev);
+if (xendev->evtchndev != NULL) {
+xenevtchn_close(xendev->evtchndev);
 }
-return NULL;
+if (xendev->gnttabdev != NULL) {
+xengnttab_close(xendev->gnttabdev);
+}
+
+QTAILQ_REMOVE(, xendev, next);
+g_free(xendev);
 }
 
 /*
@@ -682,7 +662,7 @@ static void xenstore_update_be(char *watch, char *type, int 
dom,
 if (xendev != NULL) {
 bepath = xs_read(xenstore, 0, xendev->be, );
 if (bepath == NULL) {
-xen_be_del_xendev(dom, dev);
+xen_be_del_xendev(xendev);
 } else {
 free(bepath);
 xen_be_backend_changed(xendev, path);
-- 
2.6.6

[Qemu-devel] [PATCH 0/2] xen: bug fixes in Xen backend handling

[Juergen Gross]

When testing qemu based pvusb backend two bugs have been discovered:
- detaching of a usb controller leads to memory clobbering in qemu
- detaching of a usb device with active I/O requests could result in
  crash of qemu

Juergen Gross (2):
  xen: when removing a backend don't remove many of them
  xen: drain submit queue in xen-usb before removing device

 hw/usb/xen-usb.c | 93 +---
 hw/xen/xen_backend.c | 58 +++-
 2 files changed, 79 insertions(+), 72 deletions(-)

-- 
2.6.6

[Qemu-devel] [PATCH 3/3] cpus: update comments

[Cao jin]

The returned value of cpu_get_clock() is plused with the offset,
so it is the time elapsed in virtual machine when vm is active.

Cc: Paolo Bonzini 
Cc  Peter Crosthwaite 
Cc: Richard Henderson 
Signed-off-by: Cao jin 
---
 cpus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index d534f78..722f503 100644
--- a/cpus.c
+++ b/cpus.c
@@ -191,7 +191,7 @@ int64_t cpu_icount_to_ns(int64_t icount)
 return icount << icount_time_shift;
 }
 
-/* return the host CPU cycle counter and handle stop/restart */
+/* return the host CPU cycle counter */
 /* Caller must hold the BQL */
 int64_t cpu_get_ticks(void)
 {
@@ -229,7 +229,8 @@ static int64_t cpu_get_clock_locked(void)
 return time;
 }
 
-/* return the host CPU monotonic time */
+/* Return the monotonic time elapsed in VM, i.e.,
+ * the time between vm_start and vm_stop */
 int64_t cpu_get_clock(void)
 {
 int64_t ti;
-- 
2.1.0

[Qemu-devel] [PATCH 0/3] trivial changes of timer & cpus

[Cao jin]

Cc: Paolo Bonzini 
Cc: Peter Maydell 
Cc  Peter Crosthwaite 
Cc: Richard Henderson 

Cao jin (3):
  timer: update comments
  cpus: rename local variable to meaningful one
  cpus: update comments

 cpus.c   | 13 +++--
 include/qemu/timer.h | 19 ++-
 2 files changed, 13 insertions(+), 19 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 2/3] cpus: rename local variable to meaningful one

[Cao jin]

The function actually returns monotonic time value in nanosecond,
the "ticks" is not suitable.

Cc: Paolo Bonzini 
Cc  Peter Crosthwaite 
Cc: Richard Henderson 
Signed-off-by: Cao jin 
---
 cpus.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index d4afd93..d534f78 100644
--- a/cpus.c
+++ b/cpus.c
@@ -219,14 +219,14 @@ int64_t cpu_get_ticks(void)
 
 static int64_t cpu_get_clock_locked(void)
 {
-int64_t ticks;
+int64_t time;
 
-ticks = timers_state.cpu_clock_offset;
+time = timers_state.cpu_clock_offset;
 if (timers_state.cpu_ticks_enabled) {
-ticks += get_clock();
+time += get_clock();
 }
 
-return ticks;
+return time;
 }
 
 /* return the host CPU monotonic time */
-- 
2.1.0

[Qemu-devel] [PATCH 1/3] timer: update comments

[Cao jin]

The comments is outdated. The patch has following changes:
1. tense correction.
2. all clock time value is returned in nanoseconds, so, they are same in
precision.
3. virtual clock doesn't use cpu cycles.

Cc: Paolo Bonzini 
Cc: Peter Maydell 
Signed-off-by: Cao jin 
---
 include/qemu/timer.h | 19 ++-
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 930b5d3..9910d22 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -22,23 +22,20 @@
  * @QEMU_CLOCK_REALTIME: Real time clock
  *
  * The real time clock should be used only for stuff which does not
- * change the virtual machine state, as it is run even if the virtual
- * machine is stopped. The real time clock has a frequency of 1000
- * Hz.
+ * change the virtual machine state, as it runs even if the virtual
+ * machine is stopped.
  *
  * @QEMU_CLOCK_VIRTUAL: virtual clock
  *
- * The virtual clock is only run during the emulation. It is stopped
- * when the virtual machine is stopped. Virtual timers use a high
- * precision clock, usually cpu cycles (use ticks_per_sec).
+ * The virtual clock only runs during the emulation. It stops
+ * when the virtual machine is stopped.
  *
  * @QEMU_CLOCK_HOST: host clock
  *
- * The host clock should be use for device models that emulate accurate
+ * The host clock should be used for device models that emulate accurate
  * real time sources. It will continue to run when the virtual machine
  * is suspended, and it will reflect system time changes the host may
- * undergo (e.g. due to NTP). The host clock has the same precision as
- * the virtual clock.
+ * undergo (e.g. due to NTP).
  *
  * @QEMU_CLOCK_VIRTUAL_RT: realtime clock used for icount warp
  *
@@ -77,10 +74,6 @@ struct QEMUTimer {
 extern QEMUTimerListGroup main_loop_tlg;
 
 /*
- * QEMUClockType
- */
-
-/*
  * qemu_clock_get_ns;
  * @type: the clock type
  *
-- 
2.1.0

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 28 July 2016 at 23:50, John Snow  wrote:
> The following changes since commit 21a21b853a1bb606358af61e738abfb9aecbd720:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
> staging (2016-07-27 18:18:21 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 87ac25fd1fed05a30a93d27dbeb2a4c4b83ec95f:
>
>   ide: fix halted IO segfault at reset (2016-07-28 17:34:19 -0400)
>
> 
>
> 
>
> John Snow (1):
>   ide: fix halted IO segfault at reset
>
>  hw/ide/core.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH 1/1] migration: mmap error check fix

[Dr. David Alan Gilbert]

* Denis V. Lunev (d...@openvz.org) wrote:
> From: Evgeny Yakovlev 
> 
> mmap man page:
> "On success, mmap() returns a pointer to the mapped area. On error, the
> value MAP_FAILED (that is, (void *) -1) is returned, and errno  is  set
> to indicate the cause of the error."
> 
> The check in postcopy_get_tmp_page is definitely wrong and should be
> fixed.

Oops, nice spot!

Reviewed-by: Dr. David Alan Gilbert 

> Signed-off-by: Evgeny Yakovlev 
> Signed-off-by: Denis V. Lunev 
> CC: Juan Quintela 
> CC: Amit Shah 
> ---
>  migration/postcopy-ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index abe8c60..e761f3c 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -604,7 +604,8 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
>  mis->postcopy_tmp_page = mmap(NULL, getpagesize(),
>   PROT_READ | PROT_WRITE, MAP_PRIVATE |
>   MAP_ANONYMOUS, -1, 0);
> -if (!mis->postcopy_tmp_page) {
> +if (mis->postcopy_tmp_page == MAP_FAILED) {
> +mis->postcopy_tmp_page = NULL;
>  error_report("%s: %s", __func__, strerror(errno));
>  return NULL;
>  }
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/1] migration: mmap error check fix

[Amit Shah]

On (Fri) 29 Jul 2016 [12:48:25], Denis V. Lunev wrote:
> From: Evgeny Yakovlev 
> 
> mmap man page:
> "On success, mmap() returns a pointer to the mapped area. On error, the
> value MAP_FAILED (that is, (void *) -1) is returned, and errno  is  set
> to indicate the cause of the error."
> 
> The check in postcopy_get_tmp_page is definitely wrong and should be
> fixed.
> 
> Signed-off-by: Evgeny Yakovlev 
> Signed-off-by: Denis V. Lunev 
> CC: Juan Quintela 
> CC: Amit Shah 

Reviewed-by: Amit Shah 

Amit

Re: [Qemu-devel] [PATCH] virtio-blk: Release s->rq queue at system_reset

[Laszlo Ersek]

Fam,

On 07/29/16 12:22, Fam Zheng wrote:
> At system_reset, there is no point in retrying the queued request,
> because the driver that issued the request won't be around any more.
> 
> Analyzed-by: Laszlo Ersek 
> Reported-by: Laszlo Ersek 
> Signed-off-by: Fam Zheng 
> ---
>  hw/block/virtio-blk.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 475a822..89eca65 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -654,6 +654,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>  {
>  VirtIOBlock *s = VIRTIO_BLK(vdev);
>  AioContext *ctx;
> +VirtIOBlockReq *req;
>  
>  /*
>   * This should cancel pending requests, but can't do nicely until there
> @@ -661,6 +662,11 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>   */
>  ctx = blk_get_aio_context(s->blk);
>  aio_context_acquire(ctx);
> +while (s->rq) {
> +req = s->rq;
> +s->rq = req->next;
> +virtio_blk_free_request(req);
> +}
>  blk_drain(s->blk);
>  
>  if (s->dataplane) {
> 

The comment

/*
 * This should cancel pending requests, but can't do nicely until there
 * are per-device request lists.
 */

dates back to

commit 6e02c38dadfe4cf02b0da6135adfd8d9352b90e1
Author: aliguori 
Date:   Thu Dec 4 19:52:44 2008 +

Add virtio-blk support

Virtio-blk is a paravirtual block device based on VirtIO.  It can be used by
specifying the if=virtio parameter to the -drive parameter.

When using -enable-kvm, it can achieve very good performance compared to 
IDE or
SCSI.

Signed-off-by: Anthony Liguori 



git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@5870 
c046a42c-6fe2-441c-8c8c-71466251a162

I think the comment is obsolete now, the idea that it expresses was actually 
fixed in the following commit, in my opinion:

commit 6e40b3bfc7e82823cf4df5f0bf668f56db41e53a
Author: Alexander Yarygin 
Date:   Wed Jun 17 13:37:20 2015 +0300

virtio-blk: Use blk_drain() to drain IO requests

Each call of the virtio_blk_reset() function calls blk_drain_all(),
which works for all existing BlockDriverStates, while draining only
one is needed.

This patch replaces blk_drain_all() by blk_drain() in
virtio_blk_reset(). virtio_blk_data_plane_stop() should be called
after draining because it restores vblk->complete_request.

Cc: "Michael S. Tsirkin" 
Cc: Christian Borntraeger 
Cc: Cornelia Huck 
Cc: Kevin Wolf 
Cc: Paolo Bonzini 
Cc: Stefan Hajnoczi 
Signed-off-by: Alexander Yarygin 
Message-id: 1434537440-28236-3-git-send-email-yary...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi 

So how about turning this patch into a two part series: the first patch should 
drop the comment, and reference 6e40b3bfc7e82823cf4df5f0bf668f56db41e53a, while 
the second patch should be this patch?

For this patch:

Reviewed-by: Laszlo Ersek 

I definitely recommend that Stefan review this patch too.

Thanks!
Laszlo

[Qemu-devel] VM CPU/Disk unexplained increase in resize time

[Chathura M. Sarathchandra Magurawalage]

Hi,

Does anyone know the reason for, VM resizing time to increase faster if you 
continuously increase CPU or DISK resources by +1 (e.g. 1-2, 2-3, 3-4, 4-5). 
Whereas, when you increase from 1 to any other (e.g. 1-2, 1-2, 1,3, 1-4, 1-5) 
it takes less time in comparison. Can anyone give an explanation for this? I 
have plotted two graphs.

https://www.dropbox.com/s/5e8xrrctu0rcwx3/CPU%20scaling%20%20-%20continuous%20vs%20increasing%20from%201.png?dl=0
https://www.dropbox.com/s/txpkb8k6mpyexv8/CPU%20scaling%20-%20increase%20from%201.png?dl=0

The first graph shows the VM CPU resize time (y axis) vs number of vCPUs (x 
axis) of continuous (blue) and resize from a VM with 1 vCPU (green) 
scenarios.The second graph shows the VM CPU resize time (y axis) vs number of 
vCPUs (x axis), when resized from a VM with 1 vCPU at each step (The green line 
in first graph). The error bars show the standard error of the gathered values 
at each step, as I did resize multiple times to get a mean value. I use 
openstack on x86 with KVM, although I have asked the openstack community I 
could not yet find an answer to this.

Thanks!

[Qemu-devel] [PATCH] virtio-blk: Release s->rq queue at system_reset

[Fam Zheng]

At system_reset, there is no point in retrying the queued request,
because the driver that issued the request won't be around any more.

Analyzed-by: Laszlo Ersek 
Reported-by: Laszlo Ersek 
Signed-off-by: Fam Zheng 
---
 hw/block/virtio-blk.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 475a822..89eca65 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -654,6 +654,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
 {
 VirtIOBlock *s = VIRTIO_BLK(vdev);
 AioContext *ctx;
+VirtIOBlockReq *req;
 
 /*
  * This should cancel pending requests, but can't do nicely until there
@@ -661,6 +662,11 @@ static void virtio_blk_reset(VirtIODevice *vdev)
  */
 ctx = blk_get_aio_context(s->blk);
 aio_context_acquire(ctx);
+while (s->rq) {
+req = s->rq;
+s->rq = req->next;
+virtio_blk_free_request(req);
+}
 blk_drain(s->blk);
 
 if (s->dataplane) {
-- 
2.7.4

[Qemu-devel] [PATCH 1/1] migration: mmap error check fix

[Denis V. Lunev]

From: Evgeny Yakovlev 

mmap man page:
"On success, mmap() returns a pointer to the mapped area. On error, the
value MAP_FAILED (that is, (void *) -1) is returned, and errno  is  set
to indicate the cause of the error."

The check in postcopy_get_tmp_page is definitely wrong and should be
fixed.

Signed-off-by: Evgeny Yakovlev 
Signed-off-by: Denis V. Lunev 
CC: Juan Quintela 
CC: Amit Shah 
---
 migration/postcopy-ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index abe8c60..e761f3c 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -604,7 +604,8 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
 mis->postcopy_tmp_page = mmap(NULL, getpagesize(),
  PROT_READ | PROT_WRITE, MAP_PRIVATE |
  MAP_ANONYMOUS, -1, 0);
-if (!mis->postcopy_tmp_page) {
+if (mis->postcopy_tmp_page == MAP_FAILED) {
+mis->postcopy_tmp_page = NULL;
 error_report("%s: %s", __func__, strerror(errno));
 return NULL;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] util/qemu-sockets: revert Yoda Conditions to normal

[Daniel P. Berrange]

On Fri, Jul 29, 2016 at 10:15:46AM +0800, Cao jin wrote:
> Follow CODING_STYLE
> 
> Cc: Daniel P. Berrange 
> Cc: Gerd Hoffmann 
> Cc: Paolo Bonzini 
> Cc: Eric Blake 
> Signed-off-by: Cao jin 
> ---
>  util/qemu-sockets.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> v2 changelog:
> 1. fix to Eric's comments.

Reviewed-by: Daniel P. Berrange 


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

[Qemu-devel] [PULL 2/2] target-mips: fix EntryHi.EHINV being cleared on TLB exception

[Leon Alrae]

While implementing TLB invalidation feature we forgot to modify
part of code responsible for updating EntryHi during TLB exception.
Consequently EntryHi.EHINV is unexpectedly cleared on the exception.

Signed-off-by: Leon Alrae 
---
 target-mips/helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-mips/helper.c b/target-mips/helper.c
index 9fbca26..c864b15 100644
--- a/target-mips/helper.c
+++ b/target-mips/helper.c
@@ -396,6 +396,7 @@ static void raise_mmu_exception(CPUMIPSState *env, 
target_ulong address,
 env->CP0_Context = (env->CP0_Context & ~0x007f) |
((address >> 9) & 0x0070);
 env->CP0_EntryHi = (env->CP0_EntryHi & env->CP0_EntryHi_ASID_mask) |
+   (env->CP0_EntryHi & (1 << CP0EnHi_EHINV)) |
(address & (TARGET_PAGE_MASK << 1));
 #if defined(TARGET_MIPS64)
 env->CP0_EntryHi &= env->SEGMask;
-- 
2.7.4

[Qemu-devel] [PULL 0/2] target-mips queue

[Leon Alrae]

Hi,

Just a couple of bug fixes for rc1.

Thanks,
Leon

Cc: Peter Maydell <peter.mayd...@linaro.org>
Cc: Aurelien Jarno <aurel...@aurel32.net>

The following changes since commit 21a21b853a1bb606358af61e738abfb9aecbd720:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into 
staging (2016-07-27 18:18:21 +0100)

are available in the git repository at:

  git://github.com/lalrae/qemu.git tags/mips-20160729

for you to fetch changes up to 701074a6fc7470d0ed54e4a4bcd4d491ad8da22e:

  target-mips: fix EntryHi.EHINV being cleared on TLB exception (2016-07-28 
11:24:02 +0100)


MIPS patches 2016-07-29

Changes:
* bug fixes


Leon Alrae (1):
  target-mips: fix EntryHi.EHINV being cleared on TLB exception

Paul Burton (1):
  hw/mips_malta: Fix YAMON API print routine

 hw/mips/mips_malta.c | 2 +-
 target-mips/helper.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

[Qemu-devel] [PULL 1/2] hw/mips_malta: Fix YAMON API print routine

[Leon Alrae]

From: Paul Burton 

The print routine provided as part of the in-built bootloader had a bug
in that it attempted to use a jump instruction as part of a loop, but
the target has its upper bits zeroed leading to control flow
transferring to 0xb814 rather than the intended 0xbfc00814. Fix this
by using a branch instruction instead, which seems more fit for purpose.

A simple way to test this is to build a Linux kernel with EVA enabled &
attempt to boot it in QEMU. It will attempt to print a message
indicating the configuration mismatch but QEMU would previously
incorrectly jump & wind up printing a continuous stream of the letter E.

Signed-off-by: Paul Burton 
Cc: Aurelien Jarno 
Cc: Leon Alrae 
Reviewed-by: Aurelien Jarno 
Reviewed-by: Leon Alrae 
Signed-off-by: Leon Alrae 
---
 hw/mips/mips_malta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 34d41ef..e90857e 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -727,7 +727,7 @@ static void write_bootloader(uint8_t *base, int64_t 
run_addr,
 stl_p(p++, 0x); /* nop */
 stl_p(p++, 0x0ff0021c); /* jal 870 */
 stl_p(p++, 0x); /* nop */
-stl_p(p++, 0x08000205); /* j 814 */
+stl_p(p++, 0x1000fff9); /* b 814 */
 stl_p(p++, 0x); /* nop */
 stl_p(p++, 0x01a9); /* jalr t5 */
 stl_p(p++, 0x01602021); /* move a0,t3 
*/
-- 
2.7.4

Re: [Qemu-devel] [PATCH 28/32] ppc: Avoid double translation for lvx/lvxl/stvx/stvxl

[Benjamin Herrenschmidt]

On Fri, 2016-07-29 at 06:19 +0530, Richard Henderson wrote:
> (1) The helper, since it writes to registers controlled by tcg, must be 
> described to clobber all registers.  Which will noticeably increase memory 
> traffic to ENV.  For instance, you won't be able to hold the guest register 
> holding the address in a host register across the call.

So after fixing my test setup, I did observe indeed a small performance
loss using the helper in qemu-user. It might still win us something in
softmmu due to avoiding extra translations but I will leave that aside
as I mentioned separately.

Now out of curosity, I tried this:

--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -22,12 +22,12 @@ DEF_HELPER_1(check_tlb_flush, void, env)
 #endif
 
 DEF_HELPER_3(lmw, void, env, tl, i32)
-DEF_HELPER_3(stmw, void, env, tl, i32)
+DEF_HELPER_FLAGS_3(stmw, TCG_CALL_NO_WG, void, env, tl, i32)
 DEF_HELPER_4(lsw, void, env, tl, i32, i32)
 DEF_HELPER_5(lswx, void, env, tl, i32, i32, i32)
-DEF_HELPER_4(stsw, void, env, tl, i32, i32)
-DEF_HELPER_3(dcbz, void, env, tl, i32)
-DEF_HELPER_2(icbi, void, env, tl)
+DEF_HELPER_FLAGS_4(stsw, TCG_CALL_NO_WG, void, env, tl, i32, i32)
+DEF_HELPER_FLAGS_3(dcbz, TCG_CALL_NO_WG, void, env, tl, i32)
+DEF_HELPER_FLAGS_2(icbi, TCG_CALL_NO_WG, void, env, tl)
 DEF_HELPER_5(lscbx, tl, env, tl, i32, i32, i32)
 
 #if defined(TARGET_PPC64)

If my understanding is right, the above is correct, as none of these
instructions will write to the env, though they can read from it and/
or generate faults.

Sadly I haven't observed any performance improvement as a result in
a few micro-benchmarks I cooked up.

Cheers,
Ben
 

Re: [Qemu-devel] [PATCH v2 33/37] tests: add qtest_add_data_func_full

[Marc-André Lureau]

Hi

On Fri, Jul 29, 2016 at 2:16 AM, Eric Blake  wrote:
> On 07/28/2016 08:38 AM, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> Allows to specify a destroy function for the test data.
>
> "Allows to" is not idiomatic English. Alternatives that sound better are
> "Allows $who to specify" (most simply, "Allows one to"), or "Allows
> specifying"
>
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  tests/libqtest.c | 15 ++-
>>  tests/libqtest.h |  7 ++-
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index eb00f13..6ec56a6 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void))
>>  g_free(path);
>>  }
>>
>> +void qtest_add_data_func_full(const char *str, void *data,
>> +  void (*fn)(const void *),
>> +  GDestroyNotify data_free_func)
>> +{
>> +#if GLIB_CHECK_VERSION(2, 34, 0)
>> +gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>> +g_test_add_data_func_full(path, data, fn, data_free_func);
>> +g_free(path);
>> +#else
>> +qtest_add_data_func(str, data, fn);
>> +#endif
>
> The commit message doesn't mention that the code is dependent on glib
> versions, nor that you are still leaking the data (data_free_func
> remains uncalled) on older glib.  If it is intentional (under the
> argument that "anyone running on older glib can't care too much about
> memory leaks encountered only by the testsuite, and the leaks don't
> affect main qemu"), then stating that in the commit message would let me
> feel more comfortable giving an R-b.

ok

> Is there anything we can do even in older glib to unconditionally invoke
> the cleanup function in the right places?

Yes, calling the undocumented g_test_add_vtable(), with some casts. Is
that acceptable?

-- 
Marc-André Lureau

[Qemu-devel] [PATCH v3] ppc: Rename #include'd .c files to .inc.c

[Benjamin Herrenschmidt]

Also while at it, group the #include statements in translate.c

Signed-off-by: Benjamin Herrenschmidt 
---

v3. Stupid typo I corrected locally but din't commit...
v2. Rebase on top of ppc-for-2.8
Use git renames

 target-ppc/translate.c | 40 +++---
 .../translate/{dfp-impl.c => dfp-impl.inc.c}   |  0
 target-ppc/translate/{dfp-ops.c => dfp-ops.inc.c}  |  0
 target-ppc/translate/{fp-impl.c => fp-impl.inc.c}  |  0
 target-ppc/translate/{fp-ops.c => fp-ops.inc.c}|  0
 .../translate/{spe-impl.c => spe-impl.inc.c}   |  0
 target-ppc/translate/{spe-ops.c => spe-ops.inc.c}  |  0
 .../translate/{vmx-impl.c => vmx-impl.inc.c}   |  0
 target-ppc/translate/{vmx-ops.c => vmx-ops.inc.c}  |  0
 .../translate/{vsx-impl.c => vsx-impl.inc.c}   |  0
 target-ppc/translate/{vsx-ops.c => vsx-ops.inc.c}  |  0
 11 files changed, 20 insertions(+), 20 deletions(-)
 rename target-ppc/translate/{dfp-impl.c => dfp-impl.inc.c} (100%)
 rename target-ppc/translate/{dfp-ops.c => dfp-ops.inc.c} (100%)
 rename target-ppc/translate/{fp-impl.c => fp-impl.inc.c} (100%)
 rename target-ppc/translate/{fp-ops.c => fp-ops.inc.c} (100%)
 rename target-ppc/translate/{spe-impl.c => spe-impl.inc.c} (100%)
 rename target-ppc/translate/{spe-ops.c => spe-ops.inc.c} (100%)
 rename target-ppc/translate/{vmx-impl.c => vmx-impl.inc.c} (100%)
 rename target-ppc/translate/{vmx-ops.c => vmx-ops.inc.c} (100%)
 rename target-ppc/translate/{vsx-impl.c => vsx-impl.inc.c} (100%)
 rename target-ppc/translate/{vsx-ops.c => vsx-ops.inc.c} (100%)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index fc3d371..d1837f8 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -5315,12 +5315,6 @@ static void gen_rfsvc(DisasContext *ctx)
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
-#include "translate/fp-impl.c"
-
-#include "translate/vmx-impl.c"
-
-#include "translate/vsx-impl.c"
-
 /* svc is not implemented for now */
 
 /* BookE specific instructions */
@@ -6107,10 +6101,6 @@ static void gen_maddhd_maddhdu(DisasContext *ctx)
 }
 #endif /* defined(TARGET_PPC64) */
 
-#include "translate/dfp-impl.c"
-
-#include "translate/spe-impl.c"
-
 static void gen_tbegin(DisasContext *ctx)
 {
 if (unlikely(!ctx->tm_enabled)) {
@@ -6190,6 +6180,16 @@ static inline void gen_##name(DisasContext *ctx) 
  \
 GEN_TM_PRIV_NOOP(treclaim);
 GEN_TM_PRIV_NOOP(trechkpt);
 
+#include "translate/fp-impl.inc.c"
+
+#include "translate/vmx-impl.inc.c"
+
+#include "translate/vsx-impl.inc.c"
+
+#include "translate/dfp-impl.inc.c"
+
+#include "translate/spe-impl.inc.c"
+
 static opcode_t opcodes[] = {
 GEN_HANDLER(invalid, 0x00, 0x00, 0x00, 0x, PPC_NONE),
 GEN_HANDLER(cmp, 0x1F, 0x00, 0x00, 0x0040, PPC_INTEGER),
@@ -6714,16 +6714,6 @@ GEN_MAC_HANDLER(mulhhwu, 0x08, 0x00),
 GEN_MAC_HANDLER(mullhw, 0x08, 0x0D),
 GEN_MAC_HANDLER(mullhwu, 0x08, 0x0C),
 
-#include "translate/fp-ops.c"
-
-#include "translate/vmx-ops.c"
-
-#include "translate/vsx-ops.c"
-
-#include "translate/dfp-ops.c"
-
-#include "translate/spe-ops.c"
-
 GEN_HANDLER2_E(tbegin, "tbegin", 0x1F, 0x0E, 0x14, 0x01DFF800, \
PPC_NONE, PPC2_TM),
 GEN_HANDLER2_E(tend,   "tend",   0x1F, 0x0E, 0x15, 0x01FFF800, \
@@ -6746,6 +6736,16 @@ GEN_HANDLER2_E(treclaim, "treclaim", 0x1F, 0x0E, 0x1D, 
0x03E0F800, \
PPC_NONE, PPC2_TM),
 GEN_HANDLER2_E(trechkpt, "trechkpt", 0x1F, 0x0E, 0x1F, 0x03FFF800, \
PPC_NONE, PPC2_TM),
+
+#include "translate/fp-ops.inc.c"
+
+#include "translate/vmx-ops.inc.c"
+
+#include "translate/vsx-ops.inc.c"
+
+#include "translate/dfp-ops.inc.c"
+
+#include "translate/spe-ops.inc.c"
 };
 
 #include "helper_regs.h"
diff --git a/target-ppc/translate/dfp-impl.c 
b/target-ppc/translate/dfp-impl.inc.c
similarity index 100%
rename from target-ppc/translate/dfp-impl.c
rename to target-ppc/translate/dfp-impl.inc.c
diff --git a/target-ppc/translate/dfp-ops.c b/target-ppc/translate/dfp-ops.inc.c
similarity index 100%
rename from target-ppc/translate/dfp-ops.c
rename to target-ppc/translate/dfp-ops.inc.c
diff --git a/target-ppc/translate/fp-impl.c b/target-ppc/translate/fp-impl.inc.c
similarity index 100%
rename from target-ppc/translate/fp-impl.c
rename to target-ppc/translate/fp-impl.inc.c
diff --git a/target-ppc/translate/fp-ops.c b/target-ppc/translate/fp-ops.inc.c
similarity index 100%
rename from target-ppc/translate/fp-ops.c
rename to target-ppc/translate/fp-ops.inc.c
diff --git a/target-ppc/translate/spe-impl.c 
b/target-ppc/translate/spe-impl.inc.c
similarity index 100%
rename from target-ppc/translate/spe-impl.c
rename to target-ppc/translate/spe-impl.inc.c
diff --git a/target-ppc/translate/spe-ops.c b/target-ppc/translate/spe-ops.inc.c
similarity index 100%
rename from target-ppc/translate/spe-ops.c
rename to target-ppc/translate/spe-ops.inc.c
diff --git a/target-ppc/translate/vmx-impl.c 
b/target-ppc/translate/vmx-impl.inc.c
similarity index 

[Qemu-devel] [PATCH v2] ppc: Rename #include'd .c files to .inc.c

[Benjamin Herrenschmidt]

Also while at it, group the #include statements in translate.c

Signed-off-by: Benjamin Herrenschmidt 
---

v2. Rebase on top of ppc-for-2.8
Use git renames

 target-ppc/translate.c | 40 +++---
 .../translate/{dfp-impl.c => dfp-impl.inc.c}   |  0
 target-ppc/translate/{dfp-ops.c => dfp-ops.inc.c}  |  0
 target-ppc/translate/{fp-impl.c => fp-impl.inc.c}  |  0
 target-ppc/translate/{fp-ops.c => fsp-ops.inc.c}   |  0
 .../translate/{spe-impl.c => spe-impl.inc.c}   |  0
 target-ppc/translate/{spe-ops.c => spe-ops.inc.c}  |  0
 .../translate/{vmx-impl.c => vmx-impl.inc.c}   |  0
 target-ppc/translate/{vmx-ops.c => vmx-ops.inc.c}  |  0
 .../translate/{vsx-impl.c => vsx-impl.inc.c}   |  0
 target-ppc/translate/{vsx-ops.c => vsx-ops.inc.c}  |  0
 11 files changed, 20 insertions(+), 20 deletions(-)
 rename target-ppc/translate/{dfp-impl.c => dfp-impl.inc.c} (100%)
 rename target-ppc/translate/{dfp-ops.c => dfp-ops.inc.c} (100%)
 rename target-ppc/translate/{fp-impl.c => fp-impl.inc.c} (100%)
 rename target-ppc/translate/{fp-ops.c => fsp-ops.inc.c} (100%)
 rename target-ppc/translate/{spe-impl.c => spe-impl.inc.c} (100%)
 rename target-ppc/translate/{spe-ops.c => spe-ops.inc.c} (100%)
 rename target-ppc/translate/{vmx-impl.c => vmx-impl.inc.c} (100%)
 rename target-ppc/translate/{vmx-ops.c => vmx-ops.inc.c} (100%)
 rename target-ppc/translate/{vsx-impl.c => vsx-impl.inc.c} (100%)
 rename target-ppc/translate/{vsx-ops.c => vsx-ops.inc.c} (100%)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index fc3d371..d1837f8 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -5315,12 +5315,6 @@ static void gen_rfsvc(DisasContext *ctx)
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
-#include "translate/fp-impl.c"
-
-#include "translate/vmx-impl.c"
-
-#include "translate/vsx-impl.c"
-
 /* svc is not implemented for now */
 
 /* BookE specific instructions */
@@ -6107,10 +6101,6 @@ static void gen_maddhd_maddhdu(DisasContext *ctx)
 }
 #endif /* defined(TARGET_PPC64) */
 
-#include "translate/dfp-impl.c"
-
-#include "translate/spe-impl.c"
-
 static void gen_tbegin(DisasContext *ctx)
 {
 if (unlikely(!ctx->tm_enabled)) {
@@ -6190,6 +6180,16 @@ static inline void gen_##name(DisasContext *ctx) 
  \
 GEN_TM_PRIV_NOOP(treclaim);
 GEN_TM_PRIV_NOOP(trechkpt);
 
+#include "translate/fp-impl.inc.c"
+
+#include "translate/vmx-impl.inc.c"
+
+#include "translate/vsx-impl.inc.c"
+
+#include "translate/dfp-impl.inc.c"
+
+#include "translate/spe-impl.inc.c"
+
 static opcode_t opcodes[] = {
 GEN_HANDLER(invalid, 0x00, 0x00, 0x00, 0x, PPC_NONE),
 GEN_HANDLER(cmp, 0x1F, 0x00, 0x00, 0x0040, PPC_INTEGER),
@@ -6714,16 +6714,6 @@ GEN_MAC_HANDLER(mulhhwu, 0x08, 0x00),
 GEN_MAC_HANDLER(mullhw, 0x08, 0x0D),
 GEN_MAC_HANDLER(mullhwu, 0x08, 0x0C),
 
-#include "translate/fp-ops.c"
-
-#include "translate/vmx-ops.c"
-
-#include "translate/vsx-ops.c"
-
-#include "translate/dfp-ops.c"
-
-#include "translate/spe-ops.c"
-
 GEN_HANDLER2_E(tbegin, "tbegin", 0x1F, 0x0E, 0x14, 0x01DFF800, \
PPC_NONE, PPC2_TM),
 GEN_HANDLER2_E(tend,   "tend",   0x1F, 0x0E, 0x15, 0x01FFF800, \
@@ -6746,6 +6736,16 @@ GEN_HANDLER2_E(treclaim, "treclaim", 0x1F, 0x0E, 0x1D, 
0x03E0F800, \
PPC_NONE, PPC2_TM),
 GEN_HANDLER2_E(trechkpt, "trechkpt", 0x1F, 0x0E, 0x1F, 0x03FFF800, \
PPC_NONE, PPC2_TM),
+
+#include "translate/fp-ops.inc.c"
+
+#include "translate/vmx-ops.inc.c"
+
+#include "translate/vsx-ops.inc.c"
+
+#include "translate/dfp-ops.inc.c"
+
+#include "translate/spe-ops.inc.c"
 };
 
 #include "helper_regs.h"
diff --git a/target-ppc/translate/dfp-impl.c 
b/target-ppc/translate/dfp-impl.inc.c
similarity index 100%
rename from target-ppc/translate/dfp-impl.c
rename to target-ppc/translate/dfp-impl.inc.c
diff --git a/target-ppc/translate/dfp-ops.c b/target-ppc/translate/dfp-ops.inc.c
similarity index 100%
rename from target-ppc/translate/dfp-ops.c
rename to target-ppc/translate/dfp-ops.inc.c
diff --git a/target-ppc/translate/fp-impl.c b/target-ppc/translate/fp-impl.inc.c
similarity index 100%
rename from target-ppc/translate/fp-impl.c
rename to target-ppc/translate/fp-impl.inc.c
diff --git a/target-ppc/translate/fp-ops.c b/target-ppc/translate/fsp-ops.inc.c
similarity index 100%
rename from target-ppc/translate/fp-ops.c
rename to target-ppc/translate/fsp-ops.inc.c
diff --git a/target-ppc/translate/spe-impl.c 
b/target-ppc/translate/spe-impl.inc.c
similarity index 100%
rename from target-ppc/translate/spe-impl.c
rename to target-ppc/translate/spe-impl.inc.c
diff --git a/target-ppc/translate/spe-ops.c b/target-ppc/translate/spe-ops.inc.c
similarity index 100%
rename from target-ppc/translate/spe-ops.c
rename to target-ppc/translate/spe-ops.inc.c
diff --git a/target-ppc/translate/vmx-impl.c 
b/target-ppc/translate/vmx-impl.inc.c
similarity index 100%
rename from target-ppc/translate/vmx-impl.c
rename 

Re: [Qemu-devel] [PATCH] fw_cfg: Make base type "fw_cfg" abstract

[Markus Armbruster]

Laszlo Ersek  writes:

> On 07/29/16 09:29, Markus Armbruster wrote:
>> Missed when commit 5712db6 split off "fw_cfg_io" and "fw_cfg_mem".
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/nvram/fw_cfg.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 2873030..0ccab2d 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -990,6 +990,7 @@ static void fw_cfg_class_init(ObjectClass *klass, void 
>> *data)
>>  static const TypeInfo fw_cfg_info = {
>>  .name  = TYPE_FW_CFG,
>>  .parent= TYPE_SYS_BUS_DEVICE,
>> +.abstract = true,
>>  .instance_size = sizeof(FWCfgState),
>>  .class_init= fw_cfg_class_init,
>>  };
>> 
>
> Not sure how consistent we try to be about this: should the equal sign
> (in the assignment) line up with the other equal signs?
>
> Looking for prior art, I ran
>
>   git grep -E 'abstract {2,}= true'
>
> and it returned 27 hits.
>
> Functionally the patch is right, of course. And I think the whitespace
> can be adjusted without a repost, if we agree it should be adjusted.

Yes, please.

> Reviewed-by: Laszlo Ersek 
>
> Thanks!
> Laszlo

Thanks!

Re: [Qemu-devel] [PATCH] fw_cfg: Make base type "fw_cfg" abstract

[Laszlo Ersek]

On 07/29/16 09:29, Markus Armbruster wrote:
> Missed when commit 5712db6 split off "fw_cfg_io" and "fw_cfg_mem".
> 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/nvram/fw_cfg.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 2873030..0ccab2d 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -990,6 +990,7 @@ static void fw_cfg_class_init(ObjectClass *klass, void 
> *data)
>  static const TypeInfo fw_cfg_info = {
>  .name  = TYPE_FW_CFG,
>  .parent= TYPE_SYS_BUS_DEVICE,
> +.abstract = true,
>  .instance_size = sizeof(FWCfgState),
>  .class_init= fw_cfg_class_init,
>  };
> 

Not sure how consistent we try to be about this: should the equal sign
(in the assignment) line up with the other equal signs?

Looking for prior art, I ran

  git grep -E 'abstract {2,}= true'

and it returned 27 hits.

Functionally the patch is right, of course. And I think the whitespace
can be adjusted without a repost, if we agree it should be adjusted.

Reviewed-by: Laszlo Ersek 

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Peter Maydell]

On 28 July 2016 at 17:38, Wei Huang  wrote:
> This patch adds a pmu=[on/off] option to enable/disable vpmu support
> in guest vm. There are several reasons to justify this option. First
> vpmu can be problematic for cross-migration between different SoC as
> perf counters is architecture-dependent. It is more flexible to
> have an option to turn it on/off. Secondly it matches the -cpu pmu
> option in libivrt. This patch has been tested on both DT/ACPI modes.


What particular two systems are you trying to migrate between?
In general we don't support migrating between different CPU
types at the moment, so the PMU sholud be the same on both ends.

(If we ever do get to supporting cross-cpu-type migration
then it will probably involve a very long and detailed command
line to specify exactly a whole lot of things like pmu yes/no,
number of hw breakpoints/watchpoints, and everything else that
can differ between implementations.)

That said, I don't have any objection to making the PMU
presence controllable (especially if we have similar
control on x86).

> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -579,8 +579,9 @@ struct ARMCPU {
>  bool powered_off;
>  /* CPU has security extension */
>  bool has_el3;
> -/* CPU has PMU (Performance Monitor Unit) */
> -bool has_pmu;
> +
> +/* CPU has vPMU (Performance Monitor Unit) support */
> +bool enable_pmu;

Why rename the flag? has_foo is what we use for other features,
as you can see in the context of this bit of the patch.

>
>  /* CPU has memory protection unit */
>  bool has_mpu;

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device specification

[Zeng, Xin]

On Friday, July 29, 2016 1:34 PM Michael S. Tsirkin Wrote:
> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, July 29, 2016 1:34 PM
> To: Zeng, Xin
> Cc: Gonglei (Arei); qemu-devel@nongnu.org; virtio-dev@lists.oasis-
> open.org; Ola Liljedahl; Keating, Brian A; Hanweidong (Randy); Luonengjun;
> Huangpeng (Peter); Griffin, John; Ma, Liang J; Stefan Hajnoczi; Cornelia Huck;
> Varun Sethi; Jani Kokkonen; Lingli Deng; Huangweidong (C)
> Subject: Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device
> specification
> 
> On Thu, Jul 28, 2016 at 05:28:33AM +, Zeng, Xin wrote:
> > On Thursday, July 28, 2016 10:51 AM  Gonglei (Arei) Wrote:
> > > > > Changes from v4:
> > > > >  - introduce crypto services into virtio crypto device. The services
> > > > >currently defined are CIPHER, MAC, HASH, AEAD, KDF, ASYM,
> > > > PRIMITIVE.
> > > > >  - define a unified crypto request format that is consisted of
> > > > >general header + service specific request,  Where 'general
> > > > > header' is for
> > > > all
> > > > >crypto request,  'service specific request' is composed of
> > > > >operation parameter + input data + output data in generally.
> > > > >operation parameter is algorithm-specific parameters,
> > > > >input data is the data should be operated ,
> > > > >output data is the "operation result + result buffer".
> > > > >  - redefine the algorithms and structure based on above crypto
> services.
> > > > >  - rearrange the title and subtitle
> > > > >  - Only support CIPHER, MAC, HASH and AEAD crypto services, and Xin
> will
> > > > >focus KDF, ASYM and PRIMITIVE services.
> > > > >  - Some other corresponding fixes.
> > > > >  - Make a formal patch using tex type.
> > > > >
> > > > > Changes from v3:
> > > > >  - Don't use enum is the spec but macros in specific structures.
> > > > > [Michael &
> > > > Stefan]
> > > > >  - Add two complete structures for session creation and closing, so
> that
> > > > >   the spec is clear on how to lay out the request.  [Stefan]
> > > > >  - Definite the crypto operation request with assigned
> > > > > structure, in this
> > > way,
> > > > >   each data request only occupies *one entry* of the Vring
> > > > > descriptor
> > > table,
> > > > >   which *improves* the *throughput* of data transferring.
> > > > >
> > > > > Changes from v2:
> > > > >  - Reserve virtio device ID 20 for crypto device. [Cornelia]
> > > > >  - Drop all feature bits, those capabilities are offered by the
> > > > > device all the
> > > > time.  [Stefan & Cornelia]
> > > > >  - Add a new section 1.4.2 for driver requirements. [Stefan]
> > > > >  - Use definite type definition instead of enum type in some
> structure.
> > > > [Stefan]
> > > > >  - Add virtio_crypto_cipher_alg definition. [Stefan]
> > > > >  - Add a "Device requirements" section as using MUST. [Stefan]
> > > > >  - Some grammar nits fixes and typo fixes. [Stefan & Cornelia]
> > > > >  - Add one VIRTIO_CRYPTO_S_STARTED status for the driver as the
> > > > > flag of
> > > > virtio-crypto device started and can work now.
> > > > >
> > > > > Great thanks for Stefan and Cornelia!
> > > > >
> > > > > Changes from v1:
> > > > >  - Drop the feature bit definition for each algorithm, and using
> > > > > config
> > > space
> > > > instead  [Cornelia]
> > > > >  - Add multiqueue support and add corresponding feature bit
> > > > >  - Update Encryption process and header definition
> > > > >  - Add session operation process and add corresponding header
> > > description
> > > > >  - Other better description in order to fit for virtio spec
> > > > > [Michael]
> > > > >  - Some other trivial fixes.
> > > >
> > > > OK I will let people who understand crypto comment on this.
> > >
> > > Excellently, thanks!
> > >
> > > > Down the road before we release this we'll need to link
> > > > confirmance
> > > clauses
> > > > from confirmance section. Can be a patch on top, no big deal.
> > > >
> > >
> > > Sorry, where is the confirmance section and what's confirmance clauses?
> 
> I meant conformance :) The stuff in conformance.tex
> 
> > >
> > > >
> > > > > ---
> > > > >  content.tex   |   2 +
> > > > >  virtio-crypto.tex | 792
> > > > ++
> > > > >  2 files changed, 794 insertions(+)  create mode 100644
> > > > > virtio-crypto.tex
> > > > >
> > > > > diff --git a/content.tex b/content.tex index 4b45678..ab75f78
> > > > > 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len},
> > > > \field{residual},
> > > > >  \field{status_qualifier}, \field{status}, \field{response} and
> > > > > \field{sense} fields.
> > > > >
> > > > > +\input{virtio-crypto.tex}
> > > > > +
> > > > >  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature
> > > > > Bits}
> > > > >
> > > > >  Currently there are three device-independent feature bits defined:
> > > 

Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel

[Marc-André Lureau]

Hi

On Fri, Jul 29, 2016 at 10:31 AM, Igor Mammedov  wrote:
> On Thu, 28 Jul 2016 17:43:37 +0400
> Marc-André Lureau  wrote:
>
>> Hi
>>
>> On Thu, Jul 28, 2016 at 3:29 PM, Igor Mammedov  wrote:
>> > On Thu, 28 Jul 2016 15:13:57 +0400
>> > marcandre.lur...@redhat.com wrote:
>> >
>> >> From: Marc-André Lureau 
>> >>
>> >> The property should own the allocated and unreferenced pointer. In case
>> >> of error, it should also be freed.
>> > I wonder, what use case triggers above error
>> >
>> >
>>
>> See right below in commit message:
>>
>> /x86_64/qom/pc-i440fx-1.7: qemu-system-x86_64: attempt to add
>> duplicate property 'acpi-pcihp-bsel' to object (type 'PCI')
> Maybe I've should have asked in another way,
> How do you reproduce it?

It's a qtest error (with the error reporting added here):

QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/qom-test  -p
/x86_64/qom/pc-i440fx-1.7

This is also triggered simply by running:
x86_64-softmmu/qemu-system-x86_64 -machine pc-i440fx-1.7

>
>>
>>
>> >>
>> >> RFC, because this patch triggers:
>> >> /x86_64/qom/pc-i440fx-1.7:
>> >> qemu-system-x86_64: attempt to add duplicate property 'acpi-pcihp-bsel' 
>> >> to object (type 'PCI')
>> >>
>> >> Signed-off-by: Marc-André Lureau 
>> >> ---
>> >>  hw/i386/acpi-build.c | 15 +--
>> >>  include/qom/object.h |  4 
>> >>  qom/object.c |  9 +
>> >>  3 files changed, 26 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index 017bb51..2012007 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -425,6 +425,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
>> >> PCMachineState *pcms)
>> >>   table_data->len - madt_start, 1, NULL, NULL);
>> >>  }
>> >>
>> >> +static void bsel_release(Object *obj, const char *name, void *opaque)
>> >> +{
>> >> +g_free(opaque);
>> >> +}
>> >> +
>> >>  /* Assign BSEL property to all buses.  In the future, this can be changed
>> >>   * to only assign to buses that support hotplug.
>> >>   */
>> >> @@ -432,13 +437,19 @@ static void *acpi_set_bsel(PCIBus *bus, void 
>> >> *opaque)
>> >>  {
>> >>  unsigned *bsel_alloc = opaque;
>> >>  unsigned *bus_bsel;
>> >> +Error *err = NULL;
>> >>
>> >>  if (qbus_is_hotpluggable(BUS(bus))) {
>> >>  bus_bsel = g_malloc(sizeof *bus_bsel);
>> >>
>> >>  *bus_bsel = (*bsel_alloc)++;
>> >> -object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
>> >> -   bus_bsel, NULL);
>> >> +object_property_add_uint32_ptr_release(OBJECT(bus),
>> >> +   ACPI_PCIHP_PROP_BSEL,
>> >> +   bus_bsel, bsel_release, 
>> >> );
>> >> +if (err) {
>> >> +g_free(bus_bsel);
>> >> +error_report_err(err);
>> >> +}
>> >>  }
>> >>
>> >>  return bsel_alloc;
>> >> diff --git a/include/qom/object.h b/include/qom/object.h
>> >> index 5ecc2d1..41c1051 100644
>> >> --- a/include/qom/object.h
>> >> +++ b/include/qom/object.h
>> >> @@ -1488,6 +1488,10 @@ void 
>> >> object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
>> >>   */
>> >>  void object_property_add_uint32_ptr(Object *obj, const char *name,
>> >>  const uint32_t *v, Error **errp);
>> >> +void object_property_add_uint32_ptr_release(Object *obj, const char 
>> >> *name,
>> >> +uint32_t *v,
>> >> +ObjectPropertyRelease 
>> >> *release,
>> >> +Error **errp);
>> >>  void object_class_property_add_uint32_ptr(ObjectClass *klass, const char 
>> >> *name,
>> >>const uint32_t *v, Error 
>> >> **errp);
>> >>
>> >> diff --git a/qom/object.c b/qom/object.c
>> >> index 8166b7d..1635f57 100644
>> >> --- a/qom/object.c
>> >> +++ b/qom/object.c
>> >> @@ -2157,6 +2157,15 @@ void object_property_add_uint32_ptr(Object *obj, 
>> >> const char *name,
>> >>  NULL, NULL, (void *)v, errp);
>> >>  }
>> >>
>> >> +void object_property_add_uint32_ptr_release(Object *obj, const char 
>> >> *name,
>> >> +uint32_t *v,
>> >> +ObjectPropertyRelease 
>> >> *release,
>> >> +Error **errp)
>> >> +{
>> >> +object_property_add(obj, name, "uint32", property_get_uint32_ptr,
>> >> +NULL, release, (void *)v, errp);
>> >> +}
>> >> +
>> >>  void object_class_property_add_uint32_ptr(ObjectClass *klass, const char 
>> >> *name,
>> >>const uint32_t 

[Qemu-devel] [PATCH] fw_cfg: Make base type "fw_cfg" abstract

[Markus Armbruster]

Missed when commit 5712db6 split off "fw_cfg_io" and "fw_cfg_mem".

Signed-off-by: Markus Armbruster 
---
 hw/nvram/fw_cfg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 2873030..0ccab2d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -990,6 +990,7 @@ static void fw_cfg_class_init(ObjectClass *klass, void 
*data)
 static const TypeInfo fw_cfg_info = {
 .name  = TYPE_FW_CFG,
 .parent= TYPE_SYS_BUS_DEVICE,
+.abstract = true,
 .instance_size = sizeof(FWCfgState),
 .class_init= fw_cfg_class_init,
 };
-- 
2.5.5