Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Thu) 10 Mar 2016 [07:44:19], Li, Liang Z wrote:
> 
> Hi Amit,
> 
>  Could provide more information on how to use virtio-serial to exchange data? 
>  Thread , Wiki or code are all OK. 
>  I have not find some useful information yet.

See this commit in the Linux sources:

108fc82596e3b66b819df9d28c1ebbc9ab5de14c

that adds a way to send guest trace data over to the host.  I think
that's the most relevant to your use-case.  However, you'll have to
add an in-kernel user of virtio-serial (like the virtio-console code
-- the code that deals with tty and hvc currently).  There's no other
non-tty user right now, and this is the right kind of use-case to add
one for!

For many other (userspace) use-cases, see the qemu-guest-agent in the
qemu sources.

The API is documented in the wiki:

http://www.linux-kvm.org/page/Virtio-serial_API

and the feature pages have some information that may help as well:

https://fedoraproject.org/wiki/Features/VirtioSerial

There are some links in here too:

http://log.amitshah.net/2010/09/communication-between-guests-and-hosts/

Hope this helps.


Amit

Re: [Qemu-devel] [PATCH] usb: fix unbounded stack warning for xhci_dma_write_u32s

[Peter Xu]

On Thu, Mar 10, 2016 at 08:34:13AM +0100, Gerd Hoffmann wrote:
> On Do, 2016-03-10 at 10:11 +0800, Peter Xu wrote:
> > Signed-off-by: Peter Xu 
> > ---
> >  hw/usb/hcd-xhci.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > index 44b6f8c..d15918f 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -698,11 +698,13 @@ static inline void xhci_dma_write_u32s(XHCIState 
> > *xhci, dma_addr_t addr,
> > uint32_t *buf, size_t len)
> >  {
> >  int i;
> > -uint32_t tmp[len / sizeof(uint32_t)];
> > +uint32_t tmp[12];
> 
> Where does the 12 come from?

As mentioned in previous thread, because all the callers of
xhci_dma_write_u32s() are using const size in "len". The maximum
currently is 5 * sizeof(uint32_t) = 20 bytes. Here I choose number
bigger than 5 should work for now. To make it a little bit bigger, I
just chose 12 with no specific reason... Since 8/12/16/... seems all
works for me.

Thanks.
Peter

Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization

[Li, Liang Z]

> > This patch set is the QEMU side implementation.
> >
> > The virtio-balloon is extended so that QEMU can get the free pages
> > information from the guest through virtio.
> >
> > After getting the free pages information (a bitmap), QEMU can use it
> > to filter out the guest's free pages in the ram bulk stage. This make
> > the live migration process much more efficient.
> >
> > This RFC version doesn't take the post-copy and RDMA into
> > consideration, maybe both of them can benefit from this PV solution by
> > with some extra modifications.
> 
> I like the idea, just have to prove (review) and test it a lot to ensure we 
> don't
> end up skipping pages that matter.
> 
> However, there are a couple of points:
> 
> In my opinion, the information that's exchanged between the guest and the
> host should be exchanged over a virtio-serial channel rather than virtio-
> balloon.  First, there's nothing related to the balloon here.
> It just happens to be memory info.  Second, I would never enable balloon in
> a guest that I want to be performance-sensitive.  So even if you add this as
> part of balloon, you'll find no one is using this solution.
> 
> Secondly, I suggest virtio-serial, because it's meant exactly to exchange 
> free-
> flowing information between a host and a guest, and you don't need to
> extend any part of the protocol for it (hence no changes necessary to the
> spec).  You can see how spice, vnc, etc., use virtio-serial to exchange data.
> 
> 
>   Amit

Hi Amit,

 Could provide more information on how to use virtio-serial to exchange data?  
Thread , Wiki or code are all OK. 
 I have not find some useful information yet.

Thanks
Liang

Re: [Qemu-devel] [PATCH 2/2] usb: trivial cleanup for usb_mtp_add_str

[Gerd Hoffmann]

On Do, 2016-03-10 at 07:51 +0100, Markus Armbruster wrote:
> Fam Zheng  writes:
> 
> > On Wed, 03/09 14:10, Peter Xu wrote:
> >> Remove useless var "ret".
> >> 
> >> Signed-off-by: Peter Xu 
> >> ---
> >>  hw/usb/dev-mtp.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >> 
> >> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> >> index cf63fd0..38cc4fc 100644
> >> --- a/hw/usb/dev-mtp.c
> >> +++ b/hw/usb/dev-mtp.c
> >> @@ -719,10 +719,8 @@ static void usb_mtp_add_str(MTPData *data, const char 
> >> *str)
> >>  {
> >>  uint32_t len = strlen(str)+1;
> >>  wchar_t *wstr = g_malloc(sizeof(wchar_t) * len);
> >> -size_t ret;
> >>  
> >> -ret = mbstowcs(wstr, str, len);
> >> -if (ret == -1) {
> >> +if (mbstowcs(wstr, str, len) == -1) {
> >>  usb_mtp_add_wstr(data, L"Oops");
> >>  } else {
> >>  usb_mtp_add_wstr(data, wstr);
> >> -- 
> >> 2.4.3
> >> 
> >> 
> >
> > The old way has no problem, no need to clean up, IMO.
> 
> It's a very small readability improvement, but it's an improvement.  If
> I was the maintainer, I'd take it.

Matter of taste, I find the version with ret more readable (not
surprising given I wrote that code ;)

It's easier to see what the "if (...)" condition is because the line is
shorter.

cheers,
  Gerd

Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 04/16] spapr_iommu: Introduce "enabled" state for TCE table

[Alexey Kardashevskiy]

On 03/03/2016 02:00 PM, David Gibson wrote:

On Tue, Mar 01, 2016 at 08:10:29PM +1100, Alexey Kardashevskiy wrote:

Currently TCE tables are created once at start and their sizes never
change. We are going to change that by introducing a Dynamic DMA windows
support where DMA configuration may change during the guest execution.

This changes spapr_tce_new_table() to create an empty zero-size IOMMU
memory region (IOMMU MR). Only LIOBN is assigned by the time of creation.
It still will be called once at the owner object (VIO or PHB) creation.

This introduces an "enabled" state for TCE table objects with two
helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
- spapr_tce_table_enable() receives TCE table parameters, allocates
a guest view of the TCE table (in the user space or KVM) and
sets the correct size on the IOMMU MR.
- spapr_tce_table_disable() disposes the table and resets the IOMMU MR
size.

This changes the PHB reset handler to do the default DMA initialization
instead of spapr_phb_realize(). This does not make differenct now but
later with more than just one DMA window, we will have to remove them all
and create the default one on a system reset.

No visible change in behaviour is expected except the actual table
will be reallocated every reset. We might optimize this later.

The other way to implement this would be dynamically create/remove
the TCE table QOM objects but this would make migration impossible
as the migration code expects all QOM objects to exist at the receiver
so we have to have TCE table objects created when migration begins.

spapr_tce_table_do_enable() is separated from from spapr_tce_table_enable()
as later it will be called at the sPAPRTCETable post-migration stage when
it already has all the properties set after the migration.

Signed-off-by: Alexey Kardashevskiy 


Reviewed-by: David Gibson 

Although there's one nit that could be improved:



---
  hw/ppc/spapr_iommu.c   | 80 +++---
  hw/ppc/spapr_pci.c | 21 +
  hw/ppc/spapr_vio.c |  9 +++---
  include/hw/ppc/spapr.h | 10 +++
  4 files changed, 80 insertions(+), 40 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 8132f64..e66e128 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -174,15 +174,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
  sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);

  tcet->fd = -1;
-tcet->table = spapr_tce_alloc_table(tcet->liobn,
-tcet->page_shift,
-tcet->nb_table,
->fd,
-tcet->need_vfio);
-
  memory_region_init_iommu(>iommu, OBJECT(dev), _iommu_ops,
- "iommu-spapr",
- (uint64_t)tcet->nb_table << tcet->page_shift);
+ "iommu-spapr", 0);

  QLIST_INSERT_HEAD(_tce_tables, tcet, list);

@@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool 
need_vfio)
  tcet->table = newtable;
  }

-sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
-   uint64_t bus_offset,
-   uint32_t page_shift,
-   uint32_t nb_table,
-   bool need_vfio)
+sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
  {
  sPAPRTCETable *tcet;
-char tmp[64];
+char tmp[32];

  if (spapr_tce_find_by_liobn(liobn)) {
  fprintf(stderr, "Attempted to create TCE table with duplicate"
@@ -239,16 +228,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, 
uint32_t liobn,
  return NULL;
  }

-if (!nb_table) {
-return NULL;
-}
-
  tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
  tcet->liobn = liobn;
-tcet->bus_offset = bus_offset;
-tcet->page_shift = page_shift;
-tcet->nb_table = nb_table;
-tcet->need_vfio = need_vfio;

  snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
  object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
@@ -258,14 +239,65 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, 
uint32_t liobn,
  return tcet;
  }

+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
+{
+if (!tcet->nb_table) {
+return;
+}
+
+tcet->table = spapr_tce_alloc_table(tcet->liobn,
+tcet->page_shift,
+tcet->nb_table,
+>fd,
+tcet->need_vfio);
+
+memory_region_set_size(>iommu,
+   (uint64_t)tcet->nb_table << tcet->page_shift);
+
+tcet->enabled = true;
+}
+
+void spapr_tce_table_enable(sPAPRTCETable 

Re: [Qemu-devel] [PATCH] usb: fix unbounded stack warning for xhci_dma_write_u32s

[Gerd Hoffmann]

On Do, 2016-03-10 at 10:11 +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu 
> ---
>  hw/usb/hcd-xhci.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 44b6f8c..d15918f 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -698,11 +698,13 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, 
> dma_addr_t addr,
> uint32_t *buf, size_t len)
>  {
>  int i;
> -uint32_t tmp[len / sizeof(uint32_t)];
> +uint32_t tmp[12];

Where does the 12 come from?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/3] input: linux evdev support

[Gerd Hoffmann]

  Hi,

> However, instead of adding a new -input-linux option, could you make it
> a QOM object which implements UserCreatable?  Then you can add it with
> something like "-object input-linux,path=/dev/input/input10" (perhaps
> "input-evdev" would be more specific).  This has three advantages:

It's merged meanwhile, but when doing it before 2.6 we can still change
it I think.  Do you have a pointer to some sample code doing this and/or
docs for me?

thanks,
  Gerd

Re: [Qemu-devel] [RFC kernel 0/2]A PV solution for KVM live migration optimization

[Amit Shah]

On (Thu) 10 Mar 2016 [12:31:32], Jitendra Kolhe wrote:
> On 3/8/2016 4:44 PM, Amit Shah wrote:
>  Hi,
>    An interesting solution; I know a few different people have been 
>  looking at
>  how to speed up ballooned VM migration.
> 
> >>>
> >>> Ooh, different solutions for the same purpose, and both based on the 
> >>> balloon.
> >>
> >> We were also tying to address similar problem, without actually needing to 
> >> modify
> >> the guest driver. Please find patch details under mail with subject.
> >> migration: skip sending ram pages released by virtio-balloon driver
> >
> > The scope of this patch series seems to be wider: don't send free
> > pages to a dest at all, vs. don't send pages that are ballooned out.
> 
> Hi,
> 
> Thanks for your response. The scope of this patch series doesn’t seem to take 
> care 
> of ballooned out pages. To balloon out a guest ram page the guest balloon 
> driver does 
> a alloc_page() and then return the guest pfn to Qemu, so ballooned out pages 
> will not 
> be seen as free ram pages by the guest.
> Thus we will still end up scanning (for zero page) for ballooned out pages 
> during 
> migration. It would be ideal if we could have both solutions.

Yes, of course it would be nice to have both solutions.  My response was to the 
line:

> >>> Ooh, different solutions for the same purpose, and both based on the 
> >>> balloon.

which sounded misleading to me for a couple of reasons: 1, as you
describe, pages being considered by this patchset and yours are
different; and 2, as I mentioned in the other mail, this patchset
doesn't really depend on the balloon, and I believe it should not.


Amit

[Qemu-devel] [Bug 1555452] Re: GDB server does not work in Windows

[Yorkwar]

It seems that someone is fixing this at https://www.mail-archive.com
/qemu-devel@nongnu.org/msg358825.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/1555452

Title:
  GDB server does not work in Windows

Status in QEMU:
  New

Bug description:
  I build qemu with msys2, MINGW64. After fix the socket_error() problem, and 
manually specify to use IPv4, GDB server still does not work.  The related qemu 
command is
  "-monitor none -nographic -gdb tcp::1234 -S"
  GDB reports "Timed out"

  There's a message at 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg357981.html.
  I've fixed the socket_error() problem.
  I see that qio_channel_create_socket_watch is called.

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

[Qemu-devel] [Bug 1555452] [NEW] GDB server does not work in Windows

[Yorkwar]

Public bug reported:

I build qemu with msys2, MINGW64. After fix the socket_error() problem, and 
manually specify to use IPv4, GDB server still does not work.  The related qemu 
command is
"-monitor none -nographic -gdb tcp::1234 -S"
GDB reports "Timed out"

There's a message at 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg357981.html.
I've fixed the socket_error() problem.
I see that qio_channel_create_socket_watch is called.

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: gdb windows

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

Title:
  GDB server does not work in Windows

Status in QEMU:
  New

Bug description:
  I build qemu with msys2, MINGW64. After fix the socket_error() problem, and 
manually specify to use IPv4, GDB server still does not work.  The related qemu 
command is
  "-monitor none -nographic -gdb tcp::1234 -S"
  GDB reports "Timed out"

  There's a message at 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg357981.html.
  I've fixed the socket_error() problem.
  I see that qio_channel_create_socket_watch is called.

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

Re: [Qemu-devel] [RFC kernel 0/2]A PV solution for KVM live migration optimization

[Li, Liang Z]

> On 3/8/2016 4:44 PM, Amit Shah wrote:
> > On (Fri) 04 Mar 2016 [15:02:47], Jitendra Kolhe wrote:
> 
>  * Liang Li (liang.z...@intel.com) wrote:
> > The current QEMU live migration implementation mark the all the
> > guest's RAM pages as dirtied in the ram bulk stage, all these
> > pages will be processed and that takes quit a lot of CPU cycles.
> >
> > From guest's point of view, it doesn't care about the content in
> > free pages. We can make use of this fact and skip processing the
> > free pages in the ram bulk stage, it can save a lot CPU cycles and
> > reduce the network traffic significantly while speed up the live
> > migration process obviously.
> >
> > This patch set is the QEMU side implementation.
> >
> > The virtio-balloon is extended so that QEMU can get the free pages
> > information from the guest through virtio.
> >
> > After getting the free pages information (a bitmap), QEMU can use
> > it to filter out the guest's free pages in the ram bulk stage.
> > This make the live migration process much more efficient.
> 
>  Hi,
>    An interesting solution; I know a few different people have been
>  looking at how to speed up ballooned VM migration.
> 
> >>>
> >>> Ooh, different solutions for the same purpose, and both based on the
> balloon.
> >>
> >> We were also tying to address similar problem, without actually
> >> needing to modify the guest driver. Please find patch details under mail
> with subject.
> >> migration: skip sending ram pages released by virtio-balloon driver
> >
> > The scope of this patch series seems to be wider: don't send free
> > pages to a dest at all, vs. don't send pages that are ballooned out.
> >
> > Amit
> 
> Hi,
> 
> Thanks for your response. The scope of this patch series doesn’t seem to
> take care of ballooned out pages. To balloon out a guest ram page the guest
> balloon driver does a alloc_page() and then return the guest pfn to Qemu, so
> ballooned out pages will not be seen as free ram pages by the guest.
> Thus we will still end up scanning (for zero page) for ballooned out pages
> during migration. It would be ideal if we could have both solutions.
> 

Agree,  for users who care about the performance, just skipping the free pages.
For users who have already turned on virtio-balloon,  your solution can take 
effect.

Liang
> Thanks,
> - Jitendra

Re: [Qemu-devel] [RFC kernel 0/2]A PV solution for KVM live migration optimization

[Jitendra Kolhe]

On 3/8/2016 4:44 PM, Amit Shah wrote:
> On (Fri) 04 Mar 2016 [15:02:47], Jitendra Kolhe wrote:

 * Liang Li (liang.z...@intel.com) wrote:
> The current QEMU live migration implementation mark the all the
> guest's RAM pages as dirtied in the ram bulk stage, all these pages
> will be processed and that takes quit a lot of CPU cycles.
>
> From guest's point of view, it doesn't care about the content in free
> pages. We can make use of this fact and skip processing the free pages
> in the ram bulk stage, it can save a lot CPU cycles and reduce the
> network traffic significantly while speed up the live migration
> process obviously.
>
> This patch set is the QEMU side implementation.
>
> The virtio-balloon is extended so that QEMU can get the free pages
> information from the guest through virtio.
>
> After getting the free pages information (a bitmap), QEMU can use it
> to filter out the guest's free pages in the ram bulk stage. This make
> the live migration process much more efficient.

 Hi,
   An interesting solution; I know a few different people have been looking 
 at
 how to speed up ballooned VM migration.

>>>
>>> Ooh, different solutions for the same purpose, and both based on the 
>>> balloon.
>>
>> We were also tying to address similar problem, without actually needing to 
>> modify
>> the guest driver. Please find patch details under mail with subject.
>> migration: skip sending ram pages released by virtio-balloon driver
>
> The scope of this patch series seems to be wider: don't send free
> pages to a dest at all, vs. don't send pages that are ballooned out.
>
>   Amit

Hi,

Thanks for your response. The scope of this patch series doesn’t seem to take 
care 
of ballooned out pages. To balloon out a guest ram page the guest balloon 
driver does 
a alloc_page() and then return the guest pfn to Qemu, so ballooned out pages 
will not 
be seen as free ram pages by the guest.
Thus we will still end up scanning (for zero page) for ballooned out pages 
during 
migration. It would be ideal if we could have both solutions.

Thanks,
- Jitendra

Re: [Qemu-devel] [PATCH v2 05/42] ivshmem-server: Don't overload POSIX shmem and file name

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Wed, Mar 9, 2016 at 9:14 PM, Markus Armbruster  wrote:
>>> @@ -77,7 +77,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int 
>>> argc, char *argv[])
>>> "F"  /* foreground */
>>> "p:" /* pid_file */
>>> "S:" /* unix_socket_path */
>>> -   "m:" /* shm_path */
>>> +   "m:" /* dirname */
>>
>> The existing comments all name the member of args set by the option.
>> There is no member dirname.
>
> I read from help: "-m : where to create shared memory"

Differently logical.  In your interpretation, the comments are of very
little value.  In mine, even less.  That makes yours "superior".

>>> "M:" /* shm_path */
>>> "l:" /* shm_size */
>>> "n:" /* n_vectors */
[...]

Re: [Qemu-devel] [PATCH 2/2] usb: trivial cleanup for usb_mtp_add_str

[Markus Armbruster]

Fam Zheng  writes:

> On Wed, 03/09 14:10, Peter Xu wrote:
>> Remove useless var "ret".
>> 
>> Signed-off-by: Peter Xu 
>> ---
>>  hw/usb/dev-mtp.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index cf63fd0..38cc4fc 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -719,10 +719,8 @@ static void usb_mtp_add_str(MTPData *data, const char 
>> *str)
>>  {
>>  uint32_t len = strlen(str)+1;
>>  wchar_t *wstr = g_malloc(sizeof(wchar_t) * len);
>> -size_t ret;
>>  
>> -ret = mbstowcs(wstr, str, len);
>> -if (ret == -1) {
>> +if (mbstowcs(wstr, str, len) == -1) {
>>  usb_mtp_add_wstr(data, L"Oops");
>>  } else {
>>  usb_mtp_add_wstr(data, wstr);
>> -- 
>> 2.4.3
>> 
>> 
>
> The old way has no problem, no need to clean up, IMO.

It's a very small readability improvement, but it's an improvement.  If
I was the maintainer, I'd take it.

Re: [Qemu-devel] [PATCH v2 33/42] ivshmem: Implement shm=... with a memory backend

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Wed, Mar 9, 2016 at 9:59 PM, Markus Armbruster  wrote:
>> The integrated memory backend has to go.
>>
>> Yes, my desugaring to memory-backend-file assumes Linux and the
>> conventional mount point /dev/shm.
>>
>> For what it's worth, glibc implements shm_open() as a thin wrapper
>> around open() on all targets.  On Linux, it looks for other mountpoints
>> when /dev/shm/ isn't there or unsuitable.  On other targets, it always
>> uses /dev/shm/.  I didn't bother to duplicate glibc's mountpoint search,
>> because distros converged to /dev/shm/ long ago.
>>
>> The proper way to support POSIX shared memory objects on systems where
>> they're not files (and therefore can't be mapped with
>> memory-backend-file) is to create memory-backend-shm.  If such systems
>> exist.
>>
>> I didn't do this now, because one, I'm not aware of a system that needs
>> it, and two, ivshmem is Linux-specific anyway.  ivshmem-plain could be
>> made more portable, and once that's done, memory-backend-shm might
>> become useful.
>
> How is ivshmem (the "plain" part) Linux-specific? Afaik it works on bsd too.

To make ivshmem-plain more portable, either add suitable #ifdefs to
ivshmem.c or split ivshmem-plain off into its own file.  As is, we only
compile ivshmem.c with CONFIG_EVENTFD.

If someone does that work, *and* any of the systems opened up by that
can't do use memory-backend-file, then those systems would profit from
memory-backend-shm.

>> That leaves permissions.  You're right, the patch changes them from 0777
>> to 0644.  I'm inclined to call it a bug fix.  I failed to mention it in
>> my commit message, and I'll fix that.  We may want to mention it in
>> release notes, too.
>
> That's good enough for me.

Okay :)

Re: [Qemu-devel] [PATCH v2 28/42] ivshmem: Propagate errors through ivshmem_recv_setup()

[Markus Armbruster]

Marc-André Lureau  writes:

> On Wed, Mar 9, 2016 at 9:25 PM, Markus Armbruster  wrote:
>> Now let me explain *why* I want the assertion.  The purpose of the
>> previous patch and this one is to ensure that the realized device always
>> has the shared memory mapped.  The loop does that, but it's not 100%
>> obvious, so I want the assertion to document my intent, and to prevent
>> screwups.
>
>
> alright then, a short comment would have helped! thanks

I can add one.

Re: [Qemu-devel] [PATCH v2 09/11] vfio: vote a device to do host bus reset

[Chen Fan]

On 03/10/2016 12:37 AM, Alex Williamson wrote:

On Mon, 7 Mar 2016 11:23:02 +0800
Cao jin  wrote:


From: Chen Fan 

Signed-off-by: Chen Fan 
---
  hw/vfio/pci.c | 57 +
  hw/vfio/pci.h |  1 +
  2 files changed, 58 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 24848c9..8e902d2 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1937,6 +1937,8 @@ static void vfio_check_host_bus_reset(VFIOPCIDevice 
*vdev, Error **errp)
  /* List all affected devices by bus reset */
  devices = >devices[0];
  
+vdev->single_depend_dev = (info->count == 1);

+
  /* Verify that we have all the groups required */
  for (i = 0; i < info->count; i++) {
  PCIHostDeviceAddress host;
@@ -2998,6 +3000,49 @@ static void vfio_exitfn(PCIDevice *pdev)
  vfio_unregister_bars(vdev);
  }
  
+static VFIOPCIDevice *vfio_pci_get_do_reset_device(VFIOPCIDevice *vdev)

+{
+struct vfio_pci_hot_reset_info *info = NULL;
+struct vfio_pci_dependent_device *devices;
+VFIOGroup *group;
+VFIODevice *vbasedev_iter;
+int ret;
+
+ret = vfio_get_hot_reset_info(vdev, );
+if (ret) {
+error_report("vfio: Cannot enable AER for device %s,"
+ " device does not support hot reset.",
+ vdev->vbasedev.name);
+return NULL;

Isn't this path called for all devices, not just those trying to
support AER?


+}
+
+devices = >devices[0];
+
+QLIST_FOREACH(group, _group_list, next) {
+if (group->groupid == devices[0].group_id) {
+break;
+}
+}
+
+if (!group) {
+error_report("vfio: Cannot enable AER for device %s, "
+"depends on group %d which is not owned.",
+vdev->vbasedev.name, devices[0].group_id);
+return NULL;

Same here, we're not in an AER specific code path and we haven't even
tested if the device is trying to support AER.


+}
+
+QLIST_FOREACH(vbasedev_iter, >device_list, next) {
+if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI ||
+!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
+continue;
+}
+
+return container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);

So the reset device is simply the first device in the group list, this
needs to be documented, but DMA isolation (grouping) and bus isolation
are separate things and we can have more than one group per bus...
often an IOMMU group is a single device, so have we really solved
anything here?  If the next device is on the same bus but in a separate
group, we're going to hot reset again, right?  Thanks,
You are right, I recall my original idea was not something what like 
this. :)

I tend to favor MST's ideas which said in V1 series patchset:
"If the assumption that vfio functions are combined
in the same way as on the host, then this is not needed:
just reset when function 0 is reset."

so, seems we could implement this in next version.
1. requiring functions on same host bus must were assigned on the same 
virtual

bus. here we should not have a subordinate bus. because nowadays bridge
do not support pass through. so we can simply reset the vfio device 
with the

smallest devfn in a bus reset as long as one device has aer on the bus.

with this, we don't need to care the groups isolation, because we 
only need the devices
by a bus hot reset, if the devices all on one virtual bus. reset 
anyone is sufficient.


2. in order to identify the reset is from a normal reset or aer recovery 
reset, we could
add a flags in vfio device to mark the aer have occurred in 
err_notifier_handle, if
subsequent reset is coming with the is_bus_rst in parent bus is 
true. we can

directly do the hot reset.

Thanks,
Chen



Alex


+}
+
+return NULL;
+}
+
  static void vfio_pci_reset(DeviceState *dev)
  {
  PCIDevice *pdev = DO_UPCAST(PCIDevice, qdev, dev);
@@ -3005,6 +3050,18 @@ static void vfio_pci_reset(DeviceState *dev)
  
  trace_vfio_pci_reset(vdev->vbasedev.name);
  
+if (pdev->bus->in_hot_reset) {

+VFIOPCIDevice *tmp;
+
+tmp = vfio_pci_get_do_reset_device(vdev);
+if (tmp) {
+if (tmp == vdev) {
+vfio_pci_hot_reset(vdev, vdev->single_depend_dev);
+}
+return;
+}
+}
+
  vfio_pci_pre_reset(vdev);
  
  if (vdev->resetfn && !vdev->resetfn(vdev)) {

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index aff46c2..32bd31f 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -143,6 +143,7 @@ typedef struct VFIOPCIDevice {
  bool no_kvm_intx;
  bool no_kvm_msi;
  bool no_kvm_msix;
+bool single_depend_dev;
  } VFIOPCIDevice;
  
  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);



.

Re: [Qemu-devel] [PATCH v3 5/5] replay: introduce block devices record/replay

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kw...@redhat.com]
> Am 01.03.2016 um 12:08 hat Pavel Dovgalyuk geschrieben:
> > This patch introduces block driver that implement recording
> > and replaying of block devices' operations.
> > All block completion operations are added to the queue.
> > Queue is flushed at checkpoints and information about processed requests
> > is recorded to the log. In replay phase the queue is matched with
> > events read from the log. Therefore block devices requests are processed
> > deterministically.
> >
> > Signed-off-by: Pavel Dovgalyuk 
> 
> I like this new version much better than the old one. :-)

Thanks for reviewing!

> > +static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
> > +  Error **errp)
> > +{
> > +Error *local_err = NULL;
> > +int ret;
> > +
> > +/* Open the image file */
> > +bs->file = bdrv_open_child(NULL, options, "image",
> > +   bs, _file, false, _err);
> > +if (local_err) {
> > +ret = -EINVAL;
> > +error_propagate(errp, local_err);
> > +goto fail;
> > +}
> > +
> > +ret = 0;
> > +fail:
> > +if (ret < 0) {
> > +bdrv_unref_child(bs, bs->file);
> 
> This is unnecessary because in error cases, bdrv_open_child() returns
> NULL. On the other hand, bdrv_unref_child() accepts a NULL child and
> just doesn't do anything then, so it's harmless.

Right, but this may be useful in case of future changes (if something else
will be initialized here).

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[Bharata B Rao]

On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:
> On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:
> > On Tue, 8 Mar 2016 20:04:12 +0530
> > Bharata B Rao  wrote:
> > 
> > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > > > Signed-off-by: Igor Mammedov 
> > > > ---
> > > > replaced link set check removed in previous patch
> > > > ---
> > > >  hw/ppc/spapr.c | 26 ++
> > > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 6890a44..db33c29 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState 
> > > > *dev, CPUState *cs,
> > > >  return fdt;
> > > >  }
> > > > 
> > > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > +  DeviceState *dev, Error 
> > > > **errp)
> > > > +{
> > > > +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > > +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > > +
> > > > +if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > +int core = object_property_get_int(OBJECT(dev), 
> > > > CPU_CORE_ID_PROP,
> > > > +   _abort);
> > > > +
> > > > +if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > > +error_setg(errp, "CPU hotplug not supported for this 
> > > > machine");
> > > > +return;
> > > > +}
> > > > +if (spapr->cores[core]) {
> > > > +error_setg(errp, "core %d is already present", core);
> > > > +return;
> > > > +}  
> > > 
> > > Wondering why can't we do the above check from core's realizefn and fail
> > > the core hotplug from realizefn ?
> > that's rather simple, in ideal QOM world child shouldn't
> > poke into parents internal if it could be helped.
> > So hook provides responsibility separation where
> > board/or something else(HotplugHandler) can do a necessary
> > wiring of a component which is being hotplugged, without
> > forcing hotplugged device being aware about it.
> 
> Oh.. yes.  Sorry, somehow I got confused and thought you were
> suggesting a 'pre_realize()' method on the *object* rather than a
> pre_plug hotplughandler hook.
> 
> > That's what HotplugHandler->plug callback is doing for
> > post realize and HotplugHandler->pre_plug will do similar
> > thing but allowing board to execute preliminary tasks
> > (like check/set properties, amend its internal state)
> > before object is realized.
> 
> > That will make realize() cleaner as it won't have to hack
> > into data it shouldn't and would prevent us calling unrealize()
> > if we were to check it later at HotplugHandler->plug time.
> > (i.e. realize() won't even have a chance to introduce side
> > effects that should be undone with unlealize())
> 
> Hmm.. how big a deal is it to roll back from the existing plug()
> handler?

Since plug() handler is post-realize, rolling back involves
deleting the threads of the core we created and finally deleting the core
itself. We aleady do this kind of roll back when core hotplug is attemptedi
on machine type version that don't support hotplug.

For the present case of rejecting the hotplug for duplicate core_ids,
are you in fact hinting that instead of failing the hotplug in pre_plug()
lets realize then and then roll back from plug() ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v4 2/8] pc: move igd support code to igd.c

[Tian, Kevin]

> From: Gerd Hoffmann [mailto:kra...@redhat.com]
> Sent: Wednesday, March 09, 2016 11:08 PM
> 
>   Hi,
> 
> > +/* Here we just expose minimal host bridge offset subset. */
> > +static const IGDHostInfo igd_host_bridge_infos[] = {
> > +{0x08, 2},  /* revision id */
> > +{0x2c, 2},  /* sybsystem vendor id */
> > +{0x2e, 2},  /* sybsystem id */
> 
> Can anyone clarify where this comes from?

Add Allen who is the original author.

> 
> Setting the subsystem id without also setting the pci id looks wrong,
> given that each pci id has its own subsystem id namespace.
> 
> Testing (with alex vfio patches) shows that dropping this seems to have
> no bad effects.  Things are still working fine of we only set these ...

It's possible that original list may become more than what current
driver requires...

> 
> > +{0x50, 2},  /* SNB: processor graphics control register */
> > +{0x52, 2},  /* processor graphics control register */
> > +{0xa4, 4},  /* SNB: graphics base of stolen memory */
> > +{0xa8, 4},  /* SNB: base of GTT stolen memory */
> 
> ... gfx registers in host bridge pci config space.
> 

Yes.

Thanks
Kevin

Re: [Qemu-devel] chardev: vhost-user: reconnect issue when QEMU as server

[Yuanhan Liu]

On Wed, Mar 09, 2016 at 04:04:45PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2016 at 2:29 PM, Yuanhan Liu  
> wrote:
> > I could spend more time to dig it. But before that, I want to know
> > how you guys think about it?  Does that proposal makes sense to you?
> > Or, any better ideas?
> 
> See also this thread, which I have recently rebased:
> https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg02172.html

Brillient! Hopefully I can get some time to have a detailed look at
the code and give some tries next week.

BTW, one question: what's the current status of it? Why the last version
is half year ago?

--yliu

> 
> I am trying to get the following series reviewed/merged before
> discussion the rest
> https://lists.nongnu.org/archive/html/qemu-devel/2016-02/msg05419.html
> 
> Help welcome
> 
> -- 
> Marc-André Lureau

Re: [Qemu-devel] [iGVT-g] [PATCH v2] vfio/igd: handle q35 machine type

[Tian, Kevin]

> From: Gerd Hoffmann
> Sent: Wednesday, March 09, 2016 11:01 PM
> 
>   Hi,
> 
> [ context for igvt list: how do we deal with the ich9 lpc emulated by
>   q35 machine type best, when pci-assigning a igd? ]
> 
> > > It seems
> > > quite limiting of igd assignment on q35 VMs if we're going to remove
> > > collateral device modification.  Should we only claim "legacy" igd
> > > support on 440FX and support only universal passthrough on q35?  Thanks,
> >
> > I'll go test this a bit more.  Possibly we can drop the whole lpc
> > tweaking.  At least when looking at the linux driver code it doesn't
> > seem to be very important.
> 
> Ok, we can't, it's actually checked in quite a few places.  The checks
> are hidden in a macro though which I initially didn't spot.  The bios is
> unhappy too if the lpc @ 1f.0 goes away.
> 
> But just copying over the pci ids doesn't work either, it breaks
> firmware q35 initialization.  Which in turn breaks acpi (partly), so
> some things like poweroff stop working.  And I have some irq routing
> errors in the kernel log too.
> 
> So, q35 works (without lpc tweaks) with 4.5-rc6+ guest kernel (should be
> 4.4.5+ soon) and bios rom disabled.  That is at least a start.
> 
> To get the bios and older kernels working we have to fiddle with the
> lpc.  Copying from the host looks bad to me, we have to maintain tons of
> pci ids in the firmware then.  At least the intel driver only needs to
> know which pch generation is used, so we might be able to get away with
> one pch per igd generation, and hopefully can stop doing that long term,
> when intel untangles igd and pch in hardware.
> 

Yes, in the future we can expect sort of default PCH generation based on
PCIID, when no lpc is exposed.

Thanks
Kevin

Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 02/16] spapr_pci: Move DMA window enablement to a helper

[Alexey Kardashevskiy]

On 03/03/2016 12:40 PM, David Gibson wrote:

On Tue, Mar 01, 2016 at 08:10:27PM +1100, Alexey Kardashevskiy wrote:

We are going to have multiple DMA windows soon so let's start preparing.

This adds a new helper to create a DMA window and makes use of it in
sPAPRPHBState::realize().

Signed-off-by: Alexey Kardashevskiy 
---
  hw/ppc/spapr_pci.c | 40 +++-
  1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 3d1145e..248f20a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, 
PCIDevice *pdev)
  return buf;
  }

+static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb,
+   uint32_t liobn, uint32_t page_shift,
+   uint64_t window_addr,
+   uint64_t window_size)
+{
+sPAPRTCETable *tcet;
+uint32_t nb_table = window_size >> page_shift;
+
+if (!nb_table) {
+return -1;
+}


The caller shouldn't do this, so this probably makes more sense as an
assert() than an error return.



@dma_win_size is a PHB property so the cli can set it to zero - where is it 
supposed to fail? When DMA won't work? This will be not really obvious for 
the user. Check dma_win_size==0 where we check mem_win_addr/...?





+
+tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr,
+   page_shift, nb_table, false);
+if (!tcet) {
+return -1;
+}


Since you're adding error reporting, you might as well make it via the
error API instead of a return code.  That way if we wasnt to add more
detailed error API reporting to spapr_tce_new_table() in future,
there's less to change.


Well, the table allocation is the only real thing which may fail there and 
spapr_phb_realize() does not pass Error to the callers so 
spapr_phb_dma_window_enable() would be the first one to propagate an error 
and it just seems a bit over engineered. Should I still do that?




+
+memory_region_add_subregion(>iommu_root, tcet->bus_offset,
+spapr_tce_get_iommu(tcet));
+return 0;
+}
+
  /* Macros to operate with address in OF binding to PCI */
  #define b_x(x, p, l)(((x) & ((1<<(l))-1)) << (p))
  #define b_n(x)  b_x((x), 31, 1) /* 0 if relocatable */
@@ -1228,8 +1251,6 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
  int i;
  PCIBus *bus;
  uint64_t msi_window_size = 4096;
-sPAPRTCETable *tcet;
-uint32_t nb_table;

  if (sphb->index != (uint32_t)-1) {
  hwaddr windows_base;
@@ -1381,18 +1402,11 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
  }
  }

-nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT;
-tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
-   0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
-if (!tcet) {
-error_setg(errp, "Unable to create TCE table for %s",
-   sphb->dtbusname);
-return;
-}
-
  /* Register default 32bit DMA window */
-memory_region_add_subregion(>iommu_root, sphb->dma_win_addr,
-spapr_tce_get_iommu(tcet));
+if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, 
SPAPR_TCE_PAGE_SHIFT,
+sphb->dma_win_addr, sphb->dma_win_size)) {
+error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname);
+}

  sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, 
g_free);
  }





--
Alexey

Re: [Qemu-devel] [PATCH 2/2] i386: Interrupt remapping support for VT-d

[Peter Xu]

On Wed, Mar 09, 2016 at 12:58:17AM +0530, Rita Sinha wrote:
> From: Jan Kiszka 
> 
> Still a bit hacky, unconditionally enabled (must become opt-in, not
> available with in-kernel irqchip), not reporting faults properly - but
> it works! And revealed a Linux bug [1]

If the patch is to be merged finally, shall we better add a
parameter to disable this feature for people do not need this?
Also, shall we make sure:

- make sure patches' in-reply-to are correct (so that it's in a
  series, as mentioned by Eric before)
- remove useless lines like "/* printf(...) */"
- add one-line subject for each patch (possibly)?
- ...

[...]

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 566e3d8..f7adc8e 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -431,6 +431,17 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, 
> void *opaque, int devfn)
>  assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX);
>  
>  vtd_as = vtd_find_add_as(s, bus, devfn);
> +
> +memory_region_init_iommu(_as->iommu, OBJECT(s),
> + >iommu_ops, "intel_iommu", UINT64_MAX);
> +address_space_init(_as->as,
> + _as->iommu, "intel_iommu");
> +memory_region_init_io(_as->int_remap_region, OBJECT(s),
> + _int_remap_ops, vtd_as,
> + "intel_int_remap", UINT64_MAX);
> +address_space_init(_as->int_remap_as,
> + _as->int_remap_region,
> + "intel_int_remap");

One more thing... vtd_as->{as|iommu} should have been inited in
vtd_find_add_as() already.

Peter

Re: [Qemu-devel] [PATCH v2 4/5] spapr: check if cpu core is already present

[David Gibson]

On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 20:04:12 +0530
> Bharata B Rao  wrote:
> 
> > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov 
> > > ---
> > > replaced link set check removed in previous patch
> > > ---
> > >  hw/ppc/spapr.c | 26 ++
> > >  1 file changed, 22 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 6890a44..db33c29 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState 
> > > *dev, CPUState *cs,
> > >  return fdt;
> > >  }
> > > 
> > > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > +  DeviceState *dev, Error **errp)
> > > +{
> > > +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > > +sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > > +
> > > +if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > +int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > > +   _abort);
> > > +
> > > +if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > > +error_setg(errp, "CPU hotplug not supported for this 
> > > machine");
> > > +return;
> > > +}
> > > +if (spapr->cores[core]) {
> > > +error_setg(errp, "core %d is already present", core);
> > > +return;
> > > +}  
> > 
> > Wondering why can't we do the above check from core's realizefn and fail
> > the core hotplug from realizefn ?
> that's rather simple, in ideal QOM world child shouldn't
> poke into parents internal if it could be helped.
> So hook provides responsibility separation where
> board/or something else(HotplugHandler) can do a necessary
> wiring of a component which is being hotplugged, without
> forcing hotplugged device being aware about it.

Oh.. yes.  Sorry, somehow I got confused and thought you were
suggesting a 'pre_realize()' method on the *object* rather than a
pre_plug hotplughandler hook.

> That's what HotplugHandler->plug callback is doing for
> post realize and HotplugHandler->pre_plug will do similar
> thing but allowing board to execute preliminary tasks
> (like check/set properties, amend its internal state)
> before object is realized.

> That will make realize() cleaner as it won't have to hack
> into data it shouldn't and would prevent us calling unrealize()
> if we were to check it later at HotplugHandler->plug time.
> (i.e. realize() won't even have a chance to introduce side
> effects that should be undone with unlealize())

Hmm.. how big a deal is it to roll back from the existing plug()
handler?

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/2] i386: Prepare for interrupt remapping

[Peter Xu]

Hi, Jan/Rita,

Have not gone deeper... Got several comments and questions inline.

On Wed, Mar 09, 2016 at 12:58:41AM +0530, Rita Sinha wrote:

[...]

> +static AddressSpace *get_dma_address_space(void)
> +{
> +return _MACHINE(qdev_get_machine())->dma_address_space;
> +}
> +
>  /* Given the reg addr of both the message data and address, generate an
>   * interrupt via MSI.
>   */
> @@ -282,7 +288,7 @@ static void vtd_generate_interrupt(IntelIOMMUState *s, 
> hwaddr mesg_addr_reg,
>  data = vtd_get_long_raw(s, mesg_data_reg);
>  
>  VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data);
> -address_space_stl_le(_space_memory, addr, data,
> +address_space_stl_le(get_dma_address_space(), addr, data,
>   MEMTXATTRS_UNSPECIFIED, NULL);
>  }

Would this work? AFAIU, IOMMU generated fault interrupts does not
need any translation at all.

One more question about the design itself: I see that one new AS is
created for DMA address space named dma_address_space. Could you
help explain why we need this? I am still naive on QEMU memory, what
I feel is that, current memory framework can work nicely without
this extra address space, using existing address translation
mechanisms, like the implementation in the following patch:

https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04393.html

With the new address space, we will need more loops when doing
memory address translation for IR (in address_space_translate()). 

>  
> @@ -496,7 +502,7 @@ static int vtd_get_root_entry(IntelIOMMUState *s, uint8_t 
> index,
>  dma_addr_t addr;
>  
>  addr = s->root + index * sizeof(*re);
> -if (dma_memory_read(_space_memory, addr, re, sizeof(*re))) {
> +if (dma_memory_read(get_dma_address_space(), addr, re, sizeof(*re))) {

For memory reads from IOMMU, I suppose we do not need translation as
well? I think this should work though, IMHO is because you did not
implement read() op for int_remap_as.  So, this read will fall
through to system address space finally, just like what happened
before this change.

>  VTD_DPRINTF(GENERAL, "error: fail to access root-entry at 0x%"PRIx64
>  " + %"PRIu8, s->root, index);
>  re->val = 0;
> @@ -521,7 +527,7 @@ static int vtd_get_context_entry_from_root(VTDRootEntry 
> *root, uint8_t index,
>  return -VTD_FR_ROOT_ENTRY_P;
>  }
>  addr = (root->val & VTD_ROOT_ENTRY_CTP) + index * sizeof(*ce);
> -if (dma_memory_read(_space_memory, addr, ce, sizeof(*ce))) {
> +if (dma_memory_read(get_dma_address_space(), addr, ce, sizeof(*ce))) {

Same as above. Will skip all similiar ones.

[...]

>  static void kvm_apic_reset(APICCommonState *s)
> @@ -182,8 +186,10 @@ static void kvm_apic_realize(DeviceState *dev, Error 
> **errp)
>  {
>  APICCommonState *s = APIC_COMMON(dev);
>  
> -memory_region_init_io(>io_memory, NULL, _apic_io_ops, s, 
> "kvm-apic-msi",
> -  APIC_SPACE_SIZE);
> +memory_region_init(>io_memory, NULL, "kvm-apic", APIC_SPACE_SIZE);
> +
> +memory_region_init_io(>msi_region, NULL, _msi_region_ops, NULL,
> +  "kvm-msi", MSI_REGION_SIZE);

I do not quite understand why we need to have two MRs. Could you
help explain too?

Thanks.
Peter

Re: [Qemu-devel] [PULL 0/8] VFIO updates 2016-03-09

[Peter Maydell]

On 10 March 2016 at 12:03, Peter Maydell  wrote:
> On 10 March 2016 at 02:53, Alex Williamson  wrote:
>> The following changes since commit 4ba364b47275fe428723442987b57b260b215dba:
>>
>>   Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into 
>> staging (2016-03-09 05:14:55 +)
>>
>> are available in the git repository at:
>>
>>
>>   git://github.com/awilliam/qemu-vfio.git tags/vfio-update-20160309.0
>>
>> for you to fetch changes up to 799f7b98c501e5aec0a539119613787cd5705136:
>>
>>   MAINTAINERS: Add entry for the include/hw/vfio/ folder (2016-03-09 
>> 11:19:14 -0700)
>>
>> 
>> VFIO updates 2016-03-09
>>
>>  - Allow devices to be specified via sysfs path (Alex Williamson)
>>  - vfio region helpers and generalization for future device specific regions
>>(Alex Williamson)
>>  - Automatic ROM device ID and checksum fixup (Alex Williamson)
>>  - Split VGA setup to allow enabling VGA from quirks (Alex Williamson)
>>  - Remove fixed string limit for ROM MemoryRegion name (Neo Jia)
>>  - MAINTAINERS update (Thomas Huth)
>
> Fails to build on 32-bit I'm afraid:
> ./trace/generated-tracers.h: In function 'trace_vfio_msix_fixup':
> ./trace/generated-tracers.h:16113:23: error: format '%lx' expects
> argument of type 'long unsigned int', but argument 8 has type 'off_t'
> [-Werror=format=]
>, name, bar, offset, size);
>^
> ./trace/generated-tracers.h:16113:23: error: format '%lx' expects
> argument of type 'long unsigned int', but argument 9 has type 'size_t'
> [-Werror=format=]
>   CCblock.o

Similarly, OSX warns about the off_t argument:
../trace/generated-tracers.h:16113:36: warning: format specifies type
'unsigned long' but the argument has type 'off_t'
  (aka 'long long') [-Wformat]
  , name, bar, offset, size);
   ^~
1 warning generated.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v1 05/10] cpu: Abstract CPU core type

[David Gibson]

On Wed, Mar 09, 2016 at 11:32:28AM +0100, Igor Mammedov wrote:
> On Wed, 9 Mar 2016 13:55:51 +1100
> David Gibson  wrote:
> 
> > On Tue, Mar 08, 2016 at 10:11:17AM +0100, Igor Mammedov wrote:
> > > On Tue, 8 Mar 2016 14:57:10 +1100
> > > David Gibson  wrote:
> > >   
> > > > On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:  
> > > > > On Mon, 7 Mar 2016 14:01:55 +0530
> > > > > Bharata B Rao  wrote:
> > > > > 
> > > > > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:
> > > > > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:
> > > > > > >   
> > > > > > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > > > > > Bharata B Rao  wrote:
> > > > > > > >   
> > > > > > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov 
> > > > > > > > > wrote:  
> > > > > > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > > > > > Bharata B Rao  wrote:
> > > > > > > > > > 
> > > > > > > > > > > Add an abstract CPU core type that could be used by 
> > > > > > > > > > > machines that want
> > > > > > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Bharata B Rao 
> > > > > > > > > > > ---
> > > > > > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > > > > > >  hw/cpu/core.c | 44 
> > > > > > > > > > > 
> > > > > > > > > > >  include/hw/cpu/core.h | 30 ++
> > > > > > > > > > >  3 files changed, 75 insertions(+)
> > > > > > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > > > > > index 0954a18..942a4bb 100644
> > > > > > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > > > > > +obj-y += core.o
> > > > > > > > > > >  
> > > > > > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000..d8caf37
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/hw/cpu/core.c
> > > > > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > > > > +/*
> > > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > > + *
> > > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao 
> > > > > > > > > > > 
> > > > > > > > > > > + *
> > > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, 
> > > > > > > > > > > version 2 or later.
> > > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > > + */
> > > > > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > > > > +
> > > > > > > > > > > +static char *core_prop_get_slot(Object *obj, Error 
> > > > > > > > > > > **errp)
> > > > > > > > > > > +{
> > > > > > > > > > > +CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > > +
> > > > > > > > > > > +return g_strdup(core->slot);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static void core_prop_set_slot(Object *obj, const char 
> > > > > > > > > > > *val, Error **errp)
> > > > > > > > > > > +{
> > > > > > > > > > > +CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > > +
> > > > > > > > > > > +core->slot = g_strdup(val);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > > > > > +{
> > > > > > > > > > > +object_property_add_str(obj, "slot", 
> > > > > > > > > > > core_prop_get_slot, core_prop_set_slot,
> > > > > > > > > > > +NULL);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > > > > > +.name = TYPE_CPU_CORE,
> > > > > > > > > > > +.parent = TYPE_DEVICE,
> > > > > > > > > > > +.abstract = true,
> > > > > > > > > > > +.instance_size = sizeof(CPUCore),
> > > > > > > > > > > +.instance_init = cpu_core_instance_init,
> > > > > > > > > > > +};
> > > > > > > > > > > +
> > > > > > > > > > > +static void cpu_core_register_types(void)
> > > > > > > > > > > +{
> > > > > > > > > > > +type_register_static(_core_type_info);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +type_init(cpu_core_register_types)
> > > > > > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > > > > > new file mode 100644
> > > > > > 

Re: [Qemu-devel] [Qemu-ppc] Migrating decrementer

[David Gibson]

On Mon, Feb 29, 2016 at 08:21:39PM +, Mark Cave-Ayland wrote:
> On 29/02/16 03:57, David Gibson wrote:
> 
> > On Fri, Feb 26, 2016 at 12:29:51PM +, Mark Cave-Ayland wrote:
> >> On 26/02/16 04:35, David Gibson wrote:
> >>
>  Sign. And let me try that again, this time after caffeine:
> 
>  cpu_start/resume():
>  cpu->tb_env->tb_offset =
>  muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>   cpu->tb_env->tb_freq, NANOSECONDS_PER_SECOND) +
>  cpu->tb_env->tb_offset -
>  cpu_get_host_ticks();
> 
>  This should translate to: at CPU start, calculate the difference between
>  the current guest virtual timebase and the host timebase, storing the
>  difference in cpu->tb_env->tb_offset.
> >>>
> >>> Ummm... I think that's right.  Except that you need to make sure you
> >>> calculate the tb_offset just once, and set the same value to all guest
> >>> CPUs.  Otherwise the guest TBs may be slightly out of sync with each
> >>> other, which is bad (the host should have already ensure that all host
> >>> TBs are in sync with each other).
> >>
> >> Nods. The reason I really like this solution is because it correctly
> >> handles both paused/live machines and simplifies the migration logic
> >> significantly. In fact, with this solution the only information you
> >> would need in vmstate_ppc_timebase for migration would be the current
> >> tb_offset so the receiving host can calculate the guest timebase from
> >> the virtual clock accordingly.
> > 
> >>> We really should make helper routines that each Power machine type can
> >>> use for this.  Unfortunately we can't put it directly into the common
> >>> ppc cpu migration code because of the requirement to keep the TBs
> >>> synced across the machine.
> >>
> >> Effectively I believe there are 2 cases here: TCG and KVM. For TCG all
> >> of this is a no-op since tb/decr are already derived from the virtual
> >> clock + tb_offset already so that really just leaves KVM.
> >>
> >> >From what you're saying we would need 2 hooks for KVM here: one on
> >> machine start/resume which updates tb_offset for all vCPUs and one for
> >> vCPU resume which updates just that one particular vCPU.
> >>
> >> Just curious as to your comment regarding helper routines for each Power
> >> machine type - is there any reason not to enable this globally for all
> >> Power machine types? If tb_offset isn't supported by the guest then the
> >> problem with not being able to handle a jump in timebase post-migration
> >> still remains exactly the same.
> > 
> > Well, I can't see a place to put it globally.  We can't put it in the
> > general vCPU stuff, because that would migrate each CPU's timebase
> > independently, but we want to migrate as a system wide operation, to
> > ensure the TBs are all synchronized in the destination guest.
> > 
> > To do the platform wide stuff, it pretty much has to be in the machine
> > type.
> 
> (goes and looks)
> 
> It strikes me that a good solution here would be to introduce a new
> PPCMachineClass from which all of the PPC machines could derive instead
> of each different machine being a direct subclass of MachineClass (this
> is not dissimilar as to the existing PCMachineClass) and move the
> timebase and decrementer information into it. With this then all of the
> PPC machine types can pick up the changes automatically.

Um.. maybe, yes.  There might be some gotches in attempting that
(particularly maintaining backwards compat for migration), but it
could be worth a shot.

> >> The other question of course relates to the reason this thread was
> >> started which is will this approach still support migrating the
> >> decrementer? My feeling is that this would still work in that we would
> >> encode the number of ticks before the decrementer reaches its interrupt
> >> value into vmstate_ppc_timebase, whether high or low. For TCG it is easy
> >> to ensure that the decrementer will still fire in n ticks time
> >> post-migration (which solves my particular use case), but I don't have a
> >> feeling as to whether this is possible, or indeed desirable for KVM.
> > 
> > Yes, for TCG it should be fairly straightforward.  The DECR should be
> > calculated from the timebase.  We do need to check it on incoming
> > migration though, and check when we need to refire the decrementer
> > interrupt.
> 
> So just to confirm that while reads from the timebase are not privileged
> (and so cannot be intercepted between host and guest), we still have
> individual control of the per-guest decrementer interrupts?

I'm not entirely sure I understand the question, but I think the
answer is yes.

> > For KVM we'll need to load an appropriate value into the real
> > decrementer.  We probably want to migrate a difference between the TB
> > and the decrementer.  What could get hairy here is that there are a
> > number of different variants between ppc models on how exactly the
> > decrementer 

Re: [Qemu-devel] [PULL 0/8] VFIO updates 2016-03-09

[Peter Maydell]

On 10 March 2016 at 02:53, Alex Williamson  wrote:
> The following changes since commit 4ba364b47275fe428723442987b57b260b215dba:
>
>   Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into 
> staging (2016-03-09 05:14:55 +)
>
> are available in the git repository at:
>
>
>   git://github.com/awilliam/qemu-vfio.git tags/vfio-update-20160309.0
>
> for you to fetch changes up to 799f7b98c501e5aec0a539119613787cd5705136:
>
>   MAINTAINERS: Add entry for the include/hw/vfio/ folder (2016-03-09 11:19:14 
> -0700)
>
> 
> VFIO updates 2016-03-09
>
>  - Allow devices to be specified via sysfs path (Alex Williamson)
>  - vfio region helpers and generalization for future device specific regions
>(Alex Williamson)
>  - Automatic ROM device ID and checksum fixup (Alex Williamson)
>  - Split VGA setup to allow enabling VGA from quirks (Alex Williamson)
>  - Remove fixed string limit for ROM MemoryRegion name (Neo Jia)
>  - MAINTAINERS update (Thomas Huth)

Fails to build on 32-bit I'm afraid:
./trace/generated-tracers.h: In function 'trace_vfio_msix_fixup':
./trace/generated-tracers.h:16113:23: error: format '%lx' expects
argument of type 'long unsigned int', but argument 8 has type 'off_t'
[-Werror=format=]
   , name, bar, offset, size);
   ^
./trace/generated-tracers.h:16113:23: error: format '%lx' expects
argument of type 'long unsigned int', but argument 9 has type 'size_t'
[-Werror=format=]
  CCblock.o


thanks
-- PMM

Re: [Qemu-devel] [PULL 0/5] ui: add linux evdev support, vnc and console fixes

[Peter Maydell]

On 9 March 2016 at 16:04, Gerd Hoffmann <kra...@redhat.com> wrote:
>   Hi,
>
> Here is the ui patch queue, bringing support for linux input devices and
> two fixes for the console and vnc.
>
> please pull,
>   Gerd
>
> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' 
> into staging (2016-03-06 11:53:27 +)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-ui-20160309-1
>
> for you to fetch changes up to 58aa7d8e443c7f79710a4f5757966f6c511f2242:
>
>   ui/console: add escape sequence \e[5, 6n (2016-03-09 09:35:56 +0100)
>
> 
> add linux evdev support, vnc and console fixes.
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 00/14] Net patches

[Li Zhijian]

On 03/10/2016 10:28 AM, Jason Wang wrote:



On 03/08/2016 05:54 PM, Peter Maydell wrote:

On 8 March 2016 at 16:06, Zhang Chen  wrote:

I found the reason for this problem is that
unix_connect() have not connect to sock_path before iov_send().
It need time to establish connection. so can we fix it with usleep()
like this:

 recv_sock = unix_connect(sock_path, NULL);
 g_assert_cmpint(recv_sock, !=, -1);
+usleep(1000);

 ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
sizeof(send_buf));
 g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
 close(send_sock[0]);

 ret = qemu_recv(recv_sock, , sizeof(len), 0);

I would prefer it if we could find a way to fix this race
reliably rather than just inserting a delay and hoping it
is sufficient. Otherwise the test is likely to be unreliable
if run on a heavily loaded or slow machine.

thanks
-- PMM



+1

To make sure the connected socket to be proceeded before iov_send(), you
could use some like  qmp("{ 'execute' : 'query-status'}") before
iov_send(). With this we are guaranteed that connected is setting to
true before iov_send().




it seem works, but i don't know.
Is this because that both qemu accepting the connection and qmp command are 
working under *iothread*,
so that when the qemu command returns, we can guaranteed the connection is 
accepted ?

Thanks
Li Zhijian

Re: [Qemu-devel] [RFC PATCH v2 3/3] VFIO: Type1 IOMMU mapping support for vGPU

[Jike Song]

On 03/08/2016 08:31 AM, Neo Jia wrote:
> On Mon, Mar 07, 2016 at 02:07:15PM +0800, Jike Song wrote:
>> Hi Neo,
>>
>> On Fri, Mar 4, 2016 at 3:00 PM, Neo Jia  wrote:
>>> On Wed, Mar 02, 2016 at 04:38:34PM +0800, Jike Song wrote:
 On 02/24/2016 12:24 AM, Kirti Wankhede wrote:
> +   vgpu_dma->size = map->size;
> +
> +   vgpu_link_dma(vgpu_iommu, vgpu_dma);

 Hi Kirti & Neo,

 seems that no one actually setup mappings for IOMMU here?

>>>
>>> Hi Jike,
>>>
>>> Yes.
>>>
>>> The actual mapping should be done by the host kernel driver after calling 
>>> the
>>> translation/pinning API vgpu_dma_do_translate.
>>
>> Thanks for the reply. I mis-deleted the mail in my intel account, so
>> reply with private mail account, sorry for that.
>>
>>
>> In vgpu_dma_do_translate():
>>
>> for (i = 0; i < count; i++) {
>>{snip}
>>dma_addr_t iova = gfn_buffer[i] << PAGE_SHIFT;
>>vgpu_dma = vgpu_find_dma(vgpu_iommu, iova, 0 /*  size */);
>>
>> remote_vaddr = vgpu_dma->vaddr + iova - vgpu_dma->iova;
>> if (get_user_pages_unlocked(NULL, mm, remote_vaddr, 1, 1, 0, page) == 1) 
>> {
>> pfn = page_to_pfn(page[0]);
>> }
>> gfn_buffer[i] = pfn;
>> }
>>
>> If I understand correctly, the purpose of above code, is given an
>> array of gfns, try to pin & return associated pfns. There is still no
>> IOMMU mappings here.  
> 
> Yes.
> 

Thanks for the conformation.

>> Is it supposed to be the caller who should set
>> up IOMMU by DMA api such as dma_map_page(), after calling
>> vgpu_dma_do_translate()?
>>
> 
> Don't think you need to call dma_map_page here. Once you have the pfn 
> available
> to your GPU kernel driver, you can just go ahead to setup the mapping as you
> normally do such as calling pci_map_sg and its friends.
> 

Technically it's definitely OK to call DMA API from the caller rather than here,
however personally I think it is a bit counter-intuitive: IOMMU page tables
should be constructed within the VFIO IOMMU driver.


> Thanks,
> Neo

--
Thanks,
Jike

Re: [Qemu-devel] [PATCH 1/1] MAINTAINERS: Fix typo, block/stream.h -> block/stream.c

[Fam Zheng]

On Wed, 03/09 21:54, Jeff Cody wrote:
> There is no block/stream.h, the intended filename is block/stream.c
> instead.
> 
> Signed-off-by: Jeff Cody 
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc0aa54..e04e9e9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1008,7 +1008,7 @@ F: blockjob.c
>  F: include/block/blockjob.h
>  F: block/backup.c
>  F: block/commit.c
> -F: block/stream.h
> +F: block/stream.c
>  F: block/mirror.c
>  T: git git://github.com/codyprime/qemu-kvm-jtc.git block
>  
> -- 
> 1.9.3
> 
> 

Reviewed-by: Fam Zheng 

[Qemu-devel] [PATCH 1/1] MAINTAINERS: Fix typo, block/stream.h -> block/stream.c

[Jeff Cody]

There is no block/stream.h, the intended filename is block/stream.c
instead.

Signed-off-by: Jeff Cody 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index dc0aa54..e04e9e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1008,7 +1008,7 @@ F: blockjob.c
 F: include/block/blockjob.h
 F: block/backup.c
 F: block/commit.c
-F: block/stream.h
+F: block/stream.c
 F: block/mirror.c
 T: git git://github.com/codyprime/qemu-kvm-jtc.git block
 
-- 
1.9.3

[Qemu-devel] [PATCH v12 3/3] qmp: add monitor command to add/remove a child

[Changlong Xie]

From: Wen Congyang 

The new QMP command name is x-blockdev-change. It's just for adding/removing
quorum's child now, and doesn't support all kinds of children, all kinds of
operations, nor all block drivers. So it is experimental now.

Signed-off-by: Wen Congyang 
Signed-off-by: zhanghailiang 
Signed-off-by: Gonglei 
Signed-off-by: Changlong Xie 
Reviewed-by: Max Reitz 
---
 blockdev.c   | 55 
 qapi/block-core.json | 32 ++
 qmp-commands.hx  | 54 +++
 3 files changed, 141 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 0f20c65..435631e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4017,6 +4017,61 @@ out:
 aio_context_release(aio_context);
 }
 
+static BdrvChild *bdrv_find_child(BlockDriverState *parent_bs,
+  const char *child_name)
+{
+BdrvChild *child;
+
+QLIST_FOREACH(child, _bs->children, next) {
+if (strcmp(child->name, child_name) == 0) {
+return child;
+}
+}
+
+return NULL;
+}
+
+void qmp_x_blockdev_change(const char *parent, bool has_child,
+   const char *child, bool has_node,
+   const char *node, Error **errp)
+{
+BlockDriverState *parent_bs, *new_bs = NULL;
+BdrvChild *p_child;
+
+parent_bs = bdrv_lookup_bs(parent, parent, errp);
+if (!parent_bs) {
+return;
+}
+
+if (has_child == has_node) {
+if (has_child) {
+error_setg(errp, "The parameters child and node are in conflict");
+} else {
+error_setg(errp, "Either child or node must be specified");
+}
+return;
+}
+
+if (has_child) {
+p_child = bdrv_find_child(parent_bs, child);
+if (!p_child) {
+error_setg(errp, "Node '%s' does not have child '%s'",
+   parent, child);
+return;
+}
+bdrv_del_child(parent_bs, p_child, errp);
+}
+
+if (has_node) {
+new_bs = bdrv_find_node(node);
+if (!new_bs) {
+error_setg(errp, "Node '%s' not found", node);
+return;
+}
+bdrv_add_child(parent_bs, new_bs, errp);
+}
+}
+
 BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 {
 BlockJobInfoList *head = NULL, **p_next = 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9bf1b22..bc3fd0b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2548,3 +2548,35 @@
 ##
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+##
+# @x-blockdev-change
+#
+# Dynamically reconfigure the block driver state graph. It can be used
+# to add, remove, insert or replace a graph node. Currently only the
+# Quorum driver implements this feature to add or remove its child. This
+# is useful to fix a broken quorum child.
+#
+# If @node is specified, it will be inserted under @parent. @child
+# may not be specified in this case. If both @parent and @child are
+# specified but @node is not, @child will be detached from @parent.
+#
+# @parent: the id or name of the parent node.
+#
+# @child: #optional the name of a child under the given parent node.
+#
+# @node: #optional the name of the node that will be added.
+#
+# Note: this command is experimental, and its API is not stable. It
+# does not support all kinds of operations, all kinds of children, nor
+# all block drivers.
+#
+# Warning: The data in a new quorum child MUST be consistent with that of
+# the rest of the array.
+#
+# Since: 2.6
+##
+{ 'command': 'x-blockdev-change',
+  'data' : { 'parent': 'str',
+ '*child': 'str',
+ '*node': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b629673..2a55135 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4398,6 +4398,60 @@ Example:
 EQMP
 
 {
+.name   = "x-blockdev-change",
+.args_type  = "parent:B,child:B?,node:B?",
+.mhandler.cmd_new = qmp_marshal_x_blockdev_change,
+},
+
+SQMP
+x-blockdev-change
+-
+
+Dynamically reconfigure the block driver state graph. It can be used
+to add, remove, insert or replace a graph node. Currently only the
+Quorum driver implements this feature to add or remove its child. This
+is useful to fix a broken quorum child.
+
+If @node is specified, it will be inserted under @parent. @child
+may not be specified in this case. If both @parent and @child are
+specified but @node is not, @child will be detached from @parent.
+
+Arguments:
+- "parent": the id or name of the parent node (json-string)
+- "child": the name of a child under the given parent node (json-string, 
optional)
+- "node": the name of the node that will be added (json-string, 

[Qemu-devel] [PATCH v12 1/3] Add new block driver interface to add/delete a BDS's child

[Changlong Xie]

From: Wen Congyang 

In some cases, we want to take a quorum child offline, and take
another child online.

Signed-off-by: Wen Congyang 
Signed-off-by: zhanghailiang 
Signed-off-by: Gonglei 
Signed-off-by: Changlong Xie 
Reviewed-by: Max Reitz 
---
 block.c   | 49 +++
 include/block/block.h |  4 
 include/block/block_int.h |  5 +
 3 files changed, 58 insertions(+)

diff --git a/block.c b/block.c
index ba24b8e..d48f441 100644
--- a/block.c
+++ b/block.c
@@ -4395,3 +4395,52 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 QDECREF(json);
 }
 }
+
+/*
+ * Hot add/remove a BDS's child. So the user can take a child offline when
+ * it is broken and take a new child online
+ */
+void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+Error **errp)
+{
+
+if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) {
+error_setg(errp, "The node %s does not support adding a child",
+   bdrv_get_device_or_node_name(parent_bs));
+return;
+}
+
+if (!QLIST_EMPTY(_bs->parents)) {
+error_setg(errp, "The node %s already has a parent",
+   child_bs->node_name);
+return;
+}
+
+parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp);
+}
+
+void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error 
**errp)
+{
+BdrvChild *tmp;
+
+if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) {
+error_setg(errp, "The node %s does not support removing a child",
+   bdrv_get_device_or_node_name(parent_bs));
+return;
+}
+
+QLIST_FOREACH(tmp, _bs->children, next) {
+if (tmp == child) {
+break;
+}
+}
+
+if (!tmp) {
+error_setg(errp, "The node %s does not have child named %s",
+   bdrv_get_device_or_node_name(parent_bs),
+   bdrv_get_device_or_node_name(child->bs));
+return;
+}
+
+parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 1c4f4d8..7378e74 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -582,4 +582,8 @@ void bdrv_drained_begin(BlockDriverState *bs);
  */
 void bdrv_drained_end(BlockDriverState *bs);
 
+void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
+Error **errp);
+void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9ef823a..704efe5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -305,6 +305,11 @@ struct BlockDriver {
  */
 void (*bdrv_drain)(BlockDriverState *bs);
 
+void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
+   Error **errp);
+void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
+   Error **errp);
+
 QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.9.3

[Qemu-devel] [PATCH v12 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Changlong Xie]

From: Wen Congyang 

Signed-off-by: Wen Congyang 
Signed-off-by: zhanghailiang 
Signed-off-by: Gonglei 
Signed-off-by: Changlong Xie 
Reviewed-by: Max Reitz 
---
 block.c   |   8 ++--
 block/quorum.c| 121 +-
 include/block/block.h |   4 ++
 3 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index d48f441..66d21af 100644
--- a/block.c
+++ b/block.c
@@ -1194,10 +1194,10 @@ static int bdrv_fill_options(QDict **options, const 
char *filename,
 return 0;
 }
 
-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-BlockDriverState *child_bs,
-const char *child_name,
-const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+ BlockDriverState *child_bs,
+ const char *child_name,
+ const BdrvChildRole *child_role)
 {
 BdrvChild *child = g_new(BdrvChild, 1);
 *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..97f030b 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -24,6 +24,7 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi-event.h"
 #include "crypto/hash.h"
+#include "qemu/bitmap.h"
 
 #define HASH_LENGTH 32
 
@@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
 bool rewrite_corrupted;/* true if the driver must rewrite-on-read corrupted
 * block if Quorum is reached.
 */
+unsigned long *index_bitmap;
+int bsize;
 
 QuorumReadPattern read_pattern;
 } BDRVQuorumState;
@@ -877,9 +880,9 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 ret = -EINVAL;
 goto exit;
 }
-if (s->num_children < 2) {
+if (s->num_children < 1) {
 error_setg(_err,
-   "Number of provided children must be greater than 1");
+   "Number of provided children must be 1 or more");
 ret = -EINVAL;
 goto exit;
 }
@@ -928,6 +931,7 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 /* allocate the children array */
 s->children = g_new0(BdrvChild *, s->num_children);
 opened = g_new0(bool, s->num_children);
+s->index_bitmap = bitmap_new(s->num_children);
 
 for (i = 0; i < s->num_children; i++) {
 char indexstr[32];
@@ -943,6 +947,8 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 opened[i] = true;
 }
+bitmap_set(s->index_bitmap, 0, s->num_children);
+s->bsize = s->num_children;
 
 g_free(opened);
 goto exit;
@@ -999,6 +1005,114 @@ static void quorum_attach_aio_context(BlockDriverState 
*bs,
 }
 }
 
+static int get_new_child_index(BDRVQuorumState *s)
+{
+int index;
+
+index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
+if (index < s->bsize) {
+return index;
+}
+
+s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
+ s->bsize + 1);
+return s->bsize++;
+}
+
+static void remove_child_index(BDRVQuorumState *s, int index)
+{
+int last_index, old_bsize;
+size_t new_len;
+
+assert(index < s->bsize);
+
+clear_bit(index, s->index_bitmap);
+if (index < s->bsize - 1) {
+/* The last bit is always set */
+return;
+}
+
+/* Clear last bit */
+old_bsize = s->bsize;
+last_index = find_last_bit(s->index_bitmap, s->bsize);
+assert(last_index < old_bsize);
+s->bsize = last_index + 1;
+
+if (BITS_TO_LONGS(old_bsize) == BITS_TO_LONGS(s->bsize)) {
+return;
+}
+
+new_len = BITS_TO_LONGS(s->bsize) * sizeof(unsigned long);
+s->index_bitmap = g_realloc(s->index_bitmap, new_len);
+}
+
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+ Error **errp)
+{
+BDRVQuorumState *s = bs->opaque;
+BdrvChild *child;
+char indexstr[32];
+int index, ret;
+
+index = get_new_child_index(s);
+ret = snprintf(indexstr, 32, "children.%d", index);
+if (ret < 0 || ret >= 32) {
+error_setg(errp, "cannot generate child name");
+return;
+}
+
+bdrv_drain(bs);
+
+assert(s->num_children <= INT_MAX / sizeof(BdrvChild *));
+if (s->num_children == INT_MAX / sizeof(BdrvChild *)) {
+error_setg(errp, "Too many children");
+return;
+}
+s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
+
+bdrv_ref(child_bs);
+child = bdrv_attach_child(bs, child_bs, indexstr, _format);
+s->children[s->num_children++] = child;
+set_bit(index, s->index_bitmap);
+}
+

[Qemu-devel] [PATCH v12 0/3] qapi: child add/delete support

[Changlong Xie]

ChangLog:
v11~v12:
1. Address comments from Max
p1. Add R-B
p2. Add R-B, remove unnecessary "endptr" "value"
p3. Add R-B

v10~v11:
1. Rebase to the newest codes
2. Address comment from Max
Don't use contractions in error messages,
p1: Remove R-Bs, and use "BdrvChild *child" in bdrv_del_child
p2: Fix error logic in get_new_child_index/remove_child_index, and prefect
child->name parsing
p3: Make bdrv_find_child return BdrvChild *, and add missing explanation

v9~v10:
1. Rebase to the newest codes
2. Address comments from Berto and Max, update the documents in block-core.json 
   and qmp-commands.hx 
3. Remove redundant codes in quorum_add_child() and quorum_del_child()
v8:
1. Rebase to the newest codes
2. Address the comments from Eric Blake
v7:
1. Remove the qmp command x-blockdev-change's parameter operation according
   to Kevin's comments.
2. Remove the hmp command.
v6:
1. Use a single qmp command x-blockdev-change to replace x-blockdev-child-add
   and x-blockdev-child-delete
v5:
1. Address Eric Blake's comments
v4:
1. drop nbd driver's implementation. We can use human-monitor-command
   to do it.
2. Rename the command name.
v3:
1. Don't open BDS in bdrv_add_child(). Use the existing BDS which is
   created by the QMP command blockdev-add.
2. The driver NBD can support filename, path, host:port now.
v2:
1. Use bdrv_get_device_or_node_name() instead of new function
   bdrv_get_id_or_node_name()
2. Update the error message
3. Update the documents in block-core.json

Wen Congyang (3):
  Add new block driver interface to add/delete a BDS's child
  quorum: implement bdrv_add_child() and bdrv_del_child()
  qmp: add monitor command to add/remove a child

 block.c   |  57 --
 block/quorum.c| 121 +-
 blockdev.c|  55 +
 include/block/block.h |   8 +++
 include/block/block_int.h |   5 ++
 qapi/block-core.json  |  32 
 qmp-commands.hx   |  54 +
 7 files changed, 326 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] usb: trivial cleanup for usb_mtp_add_str

[Peter Xu]

Remove useless var "ret".

Signed-off-by: Peter Xu 
---
 hw/usb/dev-mtp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 62fb7cd..3c11148 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -719,10 +719,8 @@ static void usb_mtp_add_str(MTPData *data, const char *str)
 {
 uint32_t len = strlen(str)+1;
 wchar_t *wstr = g_new(wchar_t, len);
-size_t ret;
 
-ret = mbstowcs(wstr, str, len);
-if (ret == -1) {
+if (mbstowcs(wstr, str, len) == -1) {
 usb_mtp_add_wstr(data, L"Oops");
 } else {
 usb_mtp_add_wstr(data, wstr);
-- 
2.4.3

[Qemu-devel] [PATCH v2 1/2] usb: fix unbound stack usage for usb_mtp_add_str

[Peter Xu]

Use heap instead of stack.

Signed-off-by: Peter Xu 
---
 hw/usb/dev-mtp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 7391783..62fb7cd 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -718,7 +718,7 @@ static void usb_mtp_add_wstr(MTPData *data, const wchar_t 
*str)
 static void usb_mtp_add_str(MTPData *data, const char *str)
 {
 uint32_t len = strlen(str)+1;
-wchar_t wstr[len];
+wchar_t *wstr = g_new(wchar_t, len);
 size_t ret;
 
 ret = mbstowcs(wstr, str, len);
@@ -727,6 +727,8 @@ static void usb_mtp_add_str(MTPData *data, const char *str)
 } else {
 usb_mtp_add_wstr(data, wstr);
 }
+
+g_free(wstr);
 }
 
 static void usb_mtp_add_time(MTPData *data, time_t time)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Jeff Cody]

On Wed, Mar 02, 2016 at 11:24:42AM -0500, Jeff Cody wrote:
> The function qemu_strtoul() reads 'unsigned long' sized data,
> which is larger than uint32_t on 64-bit machines.
> 
> Even though the snap_id field in the header is 32-bits, we must
> accomodate the full size in qemu_strtoul().
> 
> This patch also adds more meaningful error handling to the
> qemu_strtoul() call, and subsequent results.
> 
> Reported-by: Paolo Bonzini 
> Signed-off-by: Jeff Cody 
> ---
>  block/sheepdog.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 8739acc..87f0027 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>const char *name,
>Error **errp)
>  {
> -uint32_t snap_id = 0;
> +unsigned long snap_id = 0;
>  char snap_tag[SD_MAX_VDI_TAG_LEN];
>  Error *local_err = NULL;
>  int fd, ret;
> @@ -2565,12 +2565,15 @@ static int sd_snapshot_delete(BlockDriverState *bs,
>  memset(buf, 0, sizeof(buf));
>  memset(snap_tag, 0, sizeof(snap_tag));
>  pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
> -if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)_id)) {
> -return -1;
> +ret = qemu_strtoul(snapshot_id, NULL, 10, _id);
> +if (ret || snap_id > UINT32_MAX) {
> +error_setg(errp, "Invalid snapshot ID: %s",
> + snapshot_id ? snapshot_id : "");
> +return -EINVAL;
>  }
>  
>  if (snap_id) {
> -hdr.snapid = snap_id;
> +hdr.snapid = (uint32_t) snap_id;
>  } else {
>  pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
>  pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
> -- 
> 1.9.3
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

[Qemu-devel] [PATCH v2 0/2] usb: trivial fixes

[Peter Xu]

Patch 1 fixes one unbounded stack warning case. Sending patch 2
again for convenience, please just drop it if we do not need it.

Thanks.

Peter Xu (2):
  usb: fix unbound stack usage for usb_mtp_add_str
  usb: trivial cleanup for usb_mtp_add_str

 hw/usb/dev-mtp.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.4.3

Re: [Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

On 03/08/2016 05:54 PM, Peter Maydell wrote:
> On 8 March 2016 at 16:06, Zhang Chen  wrote:
>> I found the reason for this problem is that
>> unix_connect() have not connect to sock_path before iov_send().
>> It need time to establish connection. so can we fix it with usleep()
>> like this:
>>
>> recv_sock = unix_connect(sock_path, NULL);
>> g_assert_cmpint(recv_sock, !=, -1);
>> +usleep(1000);
>>
>> ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>> sizeof(send_buf));
>> g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>> close(send_sock[0]);
>>
>> ret = qemu_recv(recv_sock, , sizeof(len), 0);
> I would prefer it if we could find a way to fix this race
> reliably rather than just inserting a delay and hoping it
> is sufficient. Otherwise the test is likely to be unreliable
> if run on a heavily loaded or slow machine.
>
> thanks
> -- PMM
>

+1

To make sure the connected socket to be proceeded before iov_send(), you
could use some like  qmp("{ 'execute' : 'query-status'}") before
iov_send(). With this we are guaranteed that connected is setting to
true before iov_send().

Re: [Qemu-devel] [PATCH 1/2] usb: fix unbound stack usage for usb_mtp_add_str

[Peter Xu]

On Thu, Mar 10, 2016 at 09:54:41AM +0800, Fam Zheng wrote:
> On Wed, 03/09 14:10, Peter Xu wrote:
> > +wchar_t *wstr = g_malloc(sizeof(wchar_t) * len);
> 
> I think it is better to use g_new() in this case.

Okay. Will re-send this one.

Thanks.
Peter

[Qemu-devel] [PATCH] usb: fix unbounded stack warning for xhci_dma_write_u32s

[Peter Xu]

Signed-off-by: Peter Xu 
---
 hw/usb/hcd-xhci.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 44b6f8c..d15918f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -698,11 +698,13 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, 
dma_addr_t addr,
uint32_t *buf, size_t len)
 {
 int i;
-uint32_t tmp[len / sizeof(uint32_t)];
+uint32_t tmp[12];
+uint32_t n = len / sizeof(uint32_t);
 
 assert((len % sizeof(uint32_t)) == 0);
+assert(n <= ARRAY_SIZE(tmp));
 
-for (i = 0; i < (len / sizeof(uint32_t)); i++) {
+for (i = 0; i < n; i++) {
 tmp[i] = cpu_to_le32(buf[i]);
 }
 pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

On Wed, Mar 09, 2016 at 01:59:03PM +0100, Paolo Bonzini wrote:
> If you look at users, they only write about 20 bytes at most.  My
> suggestion is to use your patch, and replace
> 
> assert(__BUF_SIZE >= n);
> 
> with
> 
> assert(n < ARRAY_SIZE(tmp));
> 
> Then you don't need the #define.

Okay. Will fix and post another one.

Thanks.
Peter

Re: [Qemu-devel] [PATCH v2 06/11] pci: add a is_valid_func callback to check device if complete

[Chen Fan]

On 03/10/2016 01:14 AM, Michael S. Tsirkin wrote:

On Wed, Mar 09, 2016 at 09:50:31AM -0700, Alex Williamson wrote:

On Wed, 9 Mar 2016 18:22:24 +0200
"Michael S. Tsirkin"  wrote:


On Mon, Mar 07, 2016 at 11:22:59AM +0800, Cao jin wrote:

From: Chen Fan 

Signed-off-by: Chen Fan 

So if you create a mess, you discover it when
you later add function 0.
Why not call this when function is added?
O(N^2) on # of functions, but that # is up to 256 so maybe
that is not too bad.

Because the configuration isn't valid until the slot is closed.  Take a
dual port NIC example again, after we add the first function, the
configuration is invalid because we have an AER indicated device that
can't do a bus reset because the second function, which may be in a
separate IOMMU group, hasn't been added yet.  Therefore we can only
check the configuration when the slot is complete.  Thanks,

Alex

I see. The name mislead me.  So what you want to do is validate a
multi-function device, not the bus.


---
  hw/pci/pci.c | 39 +++
  include/hw/pci/pci.h |  1 +
  2 files changed, 40 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d940f79..72650c5 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1836,6 +1836,31 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, 
uint8_t devfn)
  return bus->devices[devfn];
  }
  
+static void pci_bus_check_device(PCIDevice *pdev, Error **errp)

Is not it true that what you are really after is validating
functions of the given device?
Pls rename this pci_check_valid_functions or something
like this, and change it to only scan functions of the device,
not all devices on the bus.

thanks, will do that.

Chen




+{
+PCIBus *bus = pdev->bus;
+PCIDeviceClass *pc;
+int i;
+Error *local_err = NULL;
+
+for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
+if (!bus->devices[i]) {
+continue;
+}
+
+pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
+if (!pc->is_valid_func) {
+continue;
+}
+
+pc->is_valid_func(bus->devices[i], _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+}
+}
+
  static void pci_qdev_realize(DeviceState *qdev, Error **errp)
  {
  PCIDevice *pci_dev = (PCIDevice *)qdev;
@@ -1878,6 +1903,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
**errp)
  pci_qdev_unrealize(DEVICE(pci_dev), NULL);
  return;
  }
+
+/*
+ *  If the function is func 0, indicate the closure of the slot.
+ *  then we get the chance to check all functions on same device
+ *  if valid.
+ */
+if (pci_get_function_0(pci_dev) == pci_dev) {
+pci_bus_check_device(pci_dev, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+pci_qdev_unrealize(DEVICE(pci_dev), NULL);
+return;
+}
+}
  }
  
  static void pci_default_realize(PCIDevice *dev, Error **errp)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index dedf277..4e56256 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
  
  void (*realize)(PCIDevice *dev, Error **errp);

  int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
+void (*is_valid_func)(PCIDevice *dev, Error **errp);
  PCIUnregisterFunc *exit;
  PCIConfigReadFunc *config_read;
  PCIConfigWriteFunc *config_write;
--
1.9.3

   


.

Re: [Qemu-devel] [PATCH v11 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Changlong Xie]

On 03/10/2016 02:11 AM, Max Reitz wrote:

On 09.03.2016 04:51, Changlong Xie wrote:

From: Wen Congyang 

Signed-off-by: Wen Congyang 
Signed-off-by: zhanghailiang 
Signed-off-by: Gonglei 
Signed-off-by: Changlong Xie 
---
  block.c   |   8 ++--
  block/quorum.c| 123 +-
  include/block/block.h |   4 ++
  3 files changed, 129 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index d48f441..66d21af 100644
--- a/block.c
+++ b/block.c
@@ -1194,10 +1194,10 @@ static int bdrv_fill_options(QDict **options, const 
char *filename,
  return 0;
  }

-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-BlockDriverState *child_bs,
-const char *child_name,
-const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+ BlockDriverState *child_bs,
+ const char *child_name,
+ const BdrvChildRole *child_role)
  {
  BdrvChild *child = g_new(BdrvChild, 1);
  *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..469e4a3 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -24,6 +24,7 @@
  #include "qapi/qmp/qstring.h"
  #include "qapi-event.h"
  #include "crypto/hash.h"
+#include "qemu/bitmap.h"

  #define HASH_LENGTH 32

@@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
  bool rewrite_corrupted;/* true if the driver must rewrite-on-read 
corrupted
  * block if Quorum is reached.
  */
+unsigned long *index_bitmap;
+int bsize;

  QuorumReadPattern read_pattern;
  } BDRVQuorumState;
@@ -877,9 +880,9 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
  ret = -EINVAL;
  goto exit;
  }
-if (s->num_children < 2) {
+if (s->num_children < 1) {
  error_setg(_err,
-   "Number of provided children must be greater than 1");
+   "Number of provided children must be 1 or more");
  ret = -EINVAL;
  goto exit;
  }
@@ -928,6 +931,7 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
  /* allocate the children array */
  s->children = g_new0(BdrvChild *, s->num_children);
  opened = g_new0(bool, s->num_children);
+s->index_bitmap = bitmap_new(s->num_children);

  for (i = 0; i < s->num_children; i++) {
  char indexstr[32];
@@ -943,6 +947,8 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,

  opened[i] = true;
  }
+bitmap_set(s->index_bitmap, 0, s->num_children);
+s->bsize = s->num_children;

  g_free(opened);
  goto exit;
@@ -999,6 +1005,116 @@ static void quorum_attach_aio_context(BlockDriverState 
*bs,
  }
  }

+static int get_new_child_index(BDRVQuorumState *s)
+{
+int index;
+
+index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
+if (index < s->bsize) {
+return index;
+}
+
+s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
+ s->bsize + 1);


If s->bsize == INT_MAX, then this will overflow to INT_MIN (probably).
This negative value will then be converted to a smaller negative value
by BITS_TO_LONGS() * sizeof(long) in bitmap_zero_extend(), and this
negative value will then be implicitly casted to a size_t value for the
g_realloc() call. On both 32 and 64 bit systems, allocating this will
probably fail due to insufficient memory which will then crash qemu.



It's a problem.


One way to prevent this: Prevent the overflow in this function by
failing if s->bsize == INT_MAX before bitmap_zero_extend() is called.

Another way: Do not limit the number of children in quorum_add_child()
(and additionally in quorum_open()) to INT_MAX, but to something more
sane like 256 or 1024 or 65536 if you want to go really high (I can't
imagine anyone using more than 32 children). That way, s->bsize can
never grow to be INT_MAX in the first place.

In any case, qemu will probably crash long before this overflows because
trying to create 2G BDSs will definitely break something. This is why
I'd prefer the second approach (limiting the number of children to a


Me too


sane amount), and this is also why I don't actually care about this
overflow here:

In my opinion you don't need to change anything here. A follow-up patch
can take care of limiting the number of quorum children to a sane amount.


Okay




+return s->bsize++;
+}
+
+static void remove_child_index(BDRVQuorumState *s, int index)
+{
+int last_index, old_bsize;
+size_t new_len;
+
+assert(index < s->bsize);
+
+clear_bit(index, 

Re: [Qemu-devel] [PATCH 2/2] usb: trivial cleanup for usb_mtp_add_str

[Fam Zheng]

On Wed, 03/09 14:10, Peter Xu wrote:
> Remove useless var "ret".
> 
> Signed-off-by: Peter Xu 
> ---
>  hw/usb/dev-mtp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index cf63fd0..38cc4fc 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -719,10 +719,8 @@ static void usb_mtp_add_str(MTPData *data, const char 
> *str)
>  {
>  uint32_t len = strlen(str)+1;
>  wchar_t *wstr = g_malloc(sizeof(wchar_t) * len);
> -size_t ret;
>  
> -ret = mbstowcs(wstr, str, len);
> -if (ret == -1) {
> +if (mbstowcs(wstr, str, len) == -1) {
>  usb_mtp_add_wstr(data, L"Oops");
>  } else {
>  usb_mtp_add_wstr(data, wstr);
> -- 
> 2.4.3
> 
> 

The old way has no problem, no need to clean up, IMO.

Fam

Re: [Qemu-devel] Memory on stellaris board

[Peter Maydell]

On 10 March 2016 at 07:23, Aurelio Remonda
 wrote:
>
> El 9 mar. 2016 8:25 PM, "Peter Maydell"  escribió:
>> You should reject unworkable ram sizes rather than silently
>> changing what the user asked for; this matches our preferred
>> approach where the user asks for more RAM than the board
>> can support.
>
> With unworkable you mean RAM values over dc0 máximum? I make sure that it
> does not exceed 0x before i replace the dc0 value so if it goes over 16M
> the program report the problem (via error_report()) and exit.

Also if the user asks for a non-round-number ram size:
don't round ram_size up or down, just fail if the user asked
for something that's not representable in a dc0 value.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] usb: fix unbound stack usage for usb_mtp_add_str

[Fam Zheng]

On Wed, 03/09 14:10, Peter Xu wrote:
> Use heap instead of stack.
> 
> Signed-off-by: Peter Xu 
> ---
>  hw/usb/dev-mtp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 7391783..cf63fd0 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -718,7 +718,7 @@ static void usb_mtp_add_wstr(MTPData *data, const wchar_t 
> *str)
>  static void usb_mtp_add_str(MTPData *data, const char *str)
>  {
>  uint32_t len = strlen(str)+1;
> -wchar_t wstr[len];
> +wchar_t *wstr = g_malloc(sizeof(wchar_t) * len);

I think it is better to use g_new() in this case.

Fam

>  size_t ret;
>  
>  ret = mbstowcs(wstr, str, len);
> @@ -727,6 +727,8 @@ static void usb_mtp_add_str(MTPData *data, const char 
> *str)
>  } else {
>  usb_mtp_add_wstr(data, wstr);
>  }
> +
> +g_free(wstr);
>  }
>  
>  static void usb_mtp_add_time(MTPData *data, time_t time)
> -- 
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH v2 3/3] vmdk: Switch to heap arrays for vmdk_parent_open

[Fam Zheng]

On Wed, 03/09 10:50, Kevin Wolf wrote:
> Am 09.03.2016 um 01:43 hat Fam Zheng geschrieben:
> > On Tue, 03/08 16:24, Fam Zheng wrote:
> > > Signed-off-by: Fam Zheng 
> > > ---
> > >  block/vmdk.c | 17 +++--
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/block/vmdk.c b/block/vmdk.c
> > > index c68f456..03be7f0 100644
> > > --- a/block/vmdk.c
> > > +++ b/block/vmdk.c
> > > @@ -343,15 +343,16 @@ static int vmdk_reopen_prepare(BDRVReopenState 
> > > *state,
> > >  static int vmdk_parent_open(BlockDriverState *bs)
> > >  {
> > >  char *p_name;
> > > -char desc[DESC_SIZE + 1];
> > > +char *desc;
> > >  BDRVVmdkState *s = bs->opaque;
> > >  int ret;
> > >  
> > > -desc[DESC_SIZE] = '\0';
> > > +desc = g_malloc0(DESC_SIZE + 1);
> > >  ret = bdrv_pread(bs->file->bs, s->desc_offset, desc, DESC_SIZE);
> > >  if (ret < 0) {
> > > -return ret;
> > > +goto out;
> > >  }
> > > +ret = 0;
> > 
> > Kevin, were you referring to this "ret = 0" in the cover letter? If so, it 
> > is
> > not useless, because ret was set to DESC_SIZE by bdrv_pread. :)
> 
> Nope, I meant the initialisation in patch 1.

You're right.

Fam

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Fam Zheng]

On Wed, 03/09 17:17, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 15:29, Christian Borntraeger wrote:
> > FWIW, it seems that this patch triggers this error, the 
> > "tracked_request_begin"
> > that I reported yesterday and / or some early read issues from the 
> > bootloader
> > in a random fashion.
> > Using 2906cddfecff21af20eedab43288b485a679f9ac^ seems to work all the time,
> > moving around vblk->dataplane_started = true also triggers all 3 types
> > of bugs
> 
> In all likelihood, the bug is that virtio_blk_handle_output is being
> called in two threads.
> 
> It's not clear to me how that's possible, though.

The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
suspicious:

   main thread  iothread

virtio_blk_handle_output()
 virtio_blk_data_plane_start()
  vblk->dataplane_started = true;
  blk_set_aio_context()
   bdrv_set_aio_context()
bdrv_drain()
 aio_poll()
  
   virtio_blk_handle_output()
/* s->dataplane_started is true */
!!!   ->virtio_blk_handle_request()
 event_notifier_set(ioeventfd)
aio_poll()
 virtio_blk_handle_request()

Christian, could you try the followed patch? The aio_poll above is replaced
with a "limited aio_poll" that doesn't disptach ioeventfd.

(Note: perhaps moving "vblk->dataplane_started = true;" after
blk_set_aio_context() also *works around* this.)

---

diff --git a/block.c b/block.c
index ba24b8e..e37e8f7 100644
--- a/block.c
+++ b/block.c
@@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
 
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
-bdrv_drain(bs); /* ensure there are no in-flight requests */
+/* ensure there are no in-flight requests */
+bdrv_drained_begin(bs);
+bdrv_drained_end(bs);
 
 bdrv_detach_aio_context(bs);

Re: [Qemu-devel] [PATCH 1/2] block/qapi: make two printf() formats literal

[Peter Xu]

On Wed, Mar 09, 2016 at 03:14:03PM -0700, Eric Blake wrote:
> > +func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
> > + composite ? '\n' : ' ');
> 
> [The nerd in me wants to point out that you could avoid the ternary by
> writing '"\n "[composite]', but that's too ugly to use outside of IOCCC
> submissions, and I wouldn't be surprised if it (rightfully) triggers
> clang warnings]

Do you mean something like:

int i = 0;
printf("%c", '"\n "[i]');

Is this a grammar btw?

Peter

Re: [Qemu-devel] [RFC qemu 0/4] A PV solution for live migration optimization

[Li, Liang Z]

> > > > > Yes, we really can teach qemu to skip these pages and it's not hard.
> > > > > The problem is the poor performance, this PV solution
> > > >
> > > > Balloon is always PV. And do not call patches solutions please.
> > > >
> > > > > is aimed to make it more
> > > > > efficient and reduce the performance impact on guest.
> > > >
> > > > We need to get a bit beyond this.  You are making multiple
> > > > changes, it seems to make sense to split it all up, and analyse
> > > > each change separately.
> > >
> > > Couldn't agree more.
> > >
> > > There are three stages in this optimization:
> > >
> > > 1) choosing which pages to skip
> > >
> > > 2) communicating them from guest to host
> > >
> > > 3) skip transferring uninteresting pages to the remote side on
> > > migration
> > >
> > > For (3) there seems to be a low-hanging fruit to amend
> > > migration/ram.c:iz_zero_range() to consult /proc/self/pagemap.  This
> > > would work for guest RAM that hasn't been touched yet or which has
> > > been ballooned out.
> > >
> > > For (1) I've been trying to make a point that skipping clean pages
> > > is much more likely to result in noticable benefit than free pages only.
> > >
> >
> > I am considering to drop the pagecache before getting the free pages.
> >
> > > As for (2), we do seem to have a problem with the existing balloon:
> > > according to your measurements it's very slow; besides, I guess it
> > > plays badly
> >
> > I didn't say communicating is slow. Even this is very slow, my
> > solution use bitmap instead of PFNs, there is fewer data traffic, so it's
> faster than the existing balloon which use PFNs.
> 
> By how much?
> 

Haven't measured yet. 
To identify a page, 1 bit is needed if using bitmap, 4 Bytes(32bit) is needed 
if using PFN, 

For a guest with 8GB RAM,  the corresponding free page bitmap size is 256KB.
And the corresponding total PFNs size is 8192KB. Assuming the inflating size
is 7GB, the total PFNs size is 7168KB.

Maybe this is not the point.

Liang

> > > with transparent huge pages (as both the guest and the host work
> > > with one 4k page at a time).  This is a problem for other use cases
> > > of balloon (e.g. as a facility for resource management); tackling
> > > that appears a more natural application for optimization efforts.
> > >
> > > Thanks,
> > > Roman.

Re: [Qemu-devel] [PATCH] qdict: fix unbounded stack for qdict_array_entries

[Peter Xu]

Sorry to forgot CCing Eric/Markus/Kevin.

This patch title is not correct, which should be:

"Fix unbounded stack warning for qdict_array_entries"

Do I need to re-send with the same content?

I'm using g_strdup_printf() here, considering it's most convenient,
safe, and as long as it's called rarely only when quorum device
opens.

Thanks.
Peter

On Wed, Mar 09, 2016 at 02:03:38PM +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu 
> ---
>  qobject/qdict.c | 15 ++-
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 9833bd0..9188a87 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -704,19 +704,16 @@ int qdict_array_entries(QDict *src, const char 
> *subqdict)
>  for (i = 0; i < INT_MAX; i++) {
>  QObject *subqobj;
>  int subqdict_entries;
> -size_t slen = 32 + subqdict_len;
> -char indexstr[slen], prefix[slen];
> -size_t snprintf_ret;
> +char *prefix = g_strdup_printf("%s%u.", subqdict, i);
>  
> -snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i);
> -assert(snprintf_ret < slen);
> +subqdict_entries = qdict_count_prefixed_entries(src, prefix);
>  
> -subqobj = qdict_get(src, indexstr);
> +/* Remove ending "." */
> +prefix[strlen(prefix) - 1] = 0x00;
> +subqobj = qdict_get(src, prefix);
>  
> -snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i);
> -assert(snprintf_ret < slen);
> +g_free(prefix);
>  
> -subqdict_entries = qdict_count_prefixed_entries(src, prefix);
>  if (subqdict_entries < 0) {
>  return subqdict_entries;
>  }
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries

[Peter Xu]

On Wed, Mar 09, 2016 at 10:03:51PM +0100, Markus Armbruster wrote:
> Kevin Wolf  writes:
> > I think it's unnecessary, but fine with me. I'm just trying to say that
> > making it a fixed 128 byte array on the stack certainly doesn't improve
> > anything.
> 
> It trades a few bytes of stack for a fixed stack frame.  A fixed stack
> frame is a bit more efficient (not that it matters here), and lets us
> scratch the function permanently from the list of stack fram size false
> positives.  I think that's a reasonable trade.

Yes, that's what I want to do. I did fix nothing, but tried to avoid
the warning. Sorry that I made it a wrong title (also in the
following splitted patch). I should say:

"Fix unbounded stack warning for qdict_array_entries"

Rather than:

"Fix unbounded stack for qdict_array_entries"

Thanks.
Peter

Re: [Qemu-devel] [PATCH] exec;fix early return from ram_block_add

[Fam Zheng]

On Wed, 03/09 18:16, Paolo Bonzini wrote:

In the subject:

s/;/: /

> After reporting an error, ram_block_add was going on with the registration
> of the RAMBlock.  The visible effect is that it unlocked the ramlist
> mutex twice.
> 
> Fixes: 528f46af6ecd1e300db18684969104d4067b867b

Yes, this was my mistake in that patch. Thanks for the fix!

Reviewed-by: Fam Zheng 

Fam

> Cc: Fam Zheng 
> Signed-off-by: Paolo Bonzini 
> ---
>  exec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index d972737..11a5fa1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1588,6 +1588,7 @@ static void ram_block_add(RAMBlock *new_block, Error 
> **errp)
>  if (err) {
>  error_propagate(errp, err);
>  qemu_mutex_unlock_ramlist();
> +return;
>  }
>  } else {
>  new_block->host = phys_mem_alloc(new_block->max_length,
> @@ -1597,6 +1598,7 @@ static void ram_block_add(RAMBlock *new_block, Error 
> **errp)
>   "cannot set up guest memory '%s'",
>   memory_region_name(new_block->mr));
>  qemu_mutex_unlock_ramlist();
> +return;
>  }
>  memory_try_enable_merging(new_block->host, 
> new_block->max_length);
>  }
> -- 
> 2.5.0
> 

[Qemu-devel] [PATCH v5 13/14] qapi: Allow anonymous base for flat union

[Eric Blake]

Rather than requiring all flat unions to explicitly create
a separate base struct, we can allow the qapi schema to specify
the common members via an inline dictionary. This is similar to
how commands can specify an inline anonymous type for its 'data'.
We already have several struct types that only exist to serve as
a single flat union's base; the next commit will clean them up
(in particular, the doc change to the BlockdevOptions example in
this patch will be reflected to QMP in the next).

Now that anonymous bases are legal, we need to rework the
flat-union-bad-base negative test (as previously written, it
forms what is now valid QAPI; tweak it to now provide coverage
of a new error message path), and add a positive test in
qapi-schema-test to use an anonymous base (making the integer
argument optional, for even more coverage).

Note that this patch only allows anonymous bases for flat unions;
simple unions are already enough syntactic sugar that we do not
want to burden them further.  Meanwhile, while it would be easy
to also allow an anonymous base for structs, that would be quite
redundant, as the members can be put right into the struct
instead.

Signed-off-by: Eric Blake 

---
v5: rebase to earlier changes, improve commit message, split off
unrelated doc tweaks
v4: rebase to use implicit type, improve commit message, allow optional
members in anonymous type
[no v3]
v2: rebase onto s/fields/members/ change
v1: rebase and rework to use gen_visit_fields_call(), test new error
Previously posted as part of qapi cleanup subset F:
v6: avoid redundant error check in gen_visit_union(), rebase to
earlier gen_err_check() improvements
---
 scripts/qapi.py| 12 ++--
 scripts/qapi-types.py  | 10 ++
 docs/qapi-code-gen.txt | 26 +-
 tests/qapi-schema/flat-union-bad-base.err  |  2 +-
 tests/qapi-schema/flat-union-bad-base.json |  5 ++---
 tests/qapi-schema/qapi-schema-test.json|  6 +-
 tests/qapi-schema/qapi-schema-test.out | 10 +-
 7 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index d91af94..a38ef52 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -327,6 +327,8 @@ class QAPISchemaParser(object):


 def find_base_members(base):
+if isinstance(base, dict):
+return base
 base_struct_define = find_struct(base)
 if not base_struct_define:
 return None
@@ -561,9 +563,10 @@ def check_union(expr, expr_info):

 # Else, it's a flat union.
 else:
-# The object must have a string member 'base'.
+# The object must have a string or dictionary 'base'.
 check_type(expr_info, "'base' for union '%s'" % name,
-   base, allow_metas=['struct'])
+   base, allow_dict=True, allow_optional=True,
+   allow_metas=['struct'])
 if not base:
 raise QAPIExprError(expr_info,
 "Flat union '%s' must have a base"
@@ -1039,6 +1042,8 @@ class QAPISchemaMember(object):
 owner = owner[6:]
 if owner.endswith('-arg'):
 return '(parameter of %s)' % owner[:-4]
+elif owner.endswith('-base'):
+return '(base of %s)' % owner[:-5]
 else:
 assert owner.endswith('-wrapper')
 # Unreachable and not implemented
@@ -1325,6 +1330,9 @@ class QAPISchema(object):
 base = expr.get('base')
 tag_name = expr.get('discriminator')
 tag_member = None
+if isinstance(base, dict):
+base = (self._make_implicit_object_type(
+name, info, 'base', self._make_members(base, info)))
 if tag_name:
 variants = [self._make_variant(key, value)
 for (key, value) in data.iteritems()]
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 185b33e..cafedc4 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -72,12 +72,14 @@ struct %(c_name)s {
  c_name=c_name(name))

 if base:
-ret += mcgen('''
+if not base.is_implicit():
+ret += mcgen('''
 /* Members inherited from %(c_name)s: */
 ''',
- c_name=base.c_name())
+ c_name=base.c_name())
 ret += gen_struct_members(base.members)
-ret += mcgen('''
+if not base.is_implicit():
+ret += mcgen('''
 /* Own members: */
 ''')
 ret += gen_struct_members(members)
@@ -223,7 +225,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 def visit_object_type(self, name, info, base, members, variants):
 self._fwdecl += gen_fwd_object_or_array(name)
 self.decl += gen_object(name, base, members, variants)
-if base:
+if base and not base.is_implicit():
 self.decl += gen_upcast(name, base)

[Qemu-devel] [PATCH v5 05/14] qapi: Emit implicit structs in generated C

[Eric Blake]

We already have several places that want to visit all the members
of an implicit object within a larger context (simple union variant,
event with anonymous data, command with anonymous arguments struct);
and will be adding another one soon (the ability to declare an
anonymous base for a flat union).  Having a C struct declared for
these implicit types, along with a visit_type_FOO_members() helper
function, will make for fewer special cases in our generator.

We do not, however, need qapi_free_FOO() or visit_type_FOO()
functions for implicit types, because they should not be used
directly outside of the generated code.  This is done by adding a
conditional in visit_object_type() for both qapi-types.py and
qapi-visit.py based on the object name.  The comparison of
"name.startswith('q_')" is a bit hacky (it's basically duplicating
what .is_implicit() already uses), but beats changing the signature
of the visit_object_type() callback to pass a new 'implicit' flag.
The hack should be temporary: we are considering adding a future
patch that consolidates the narrow visit_object_type() and
visit_object_type_flat() [with different pieces already broken out]
into a broader visit_object_type() [where the visitor can query the
type object directly].

Also, now that we WANT to output C code for implicits, we have to
change the condition in the visit_needed() filter.  We have to
special-case 'q_empty' in that filter as something that does not
get output: because it is a built-in type, it would be emitted in
multiple files (the main qapi-types.h and in qga-qapi-types.h)
causing compilation failure due to redefinition.  But since it
has no members, it's easier to just avoid an attempt to visit
that particular type.

The patch relies on the changed naming of implicit types in the
previous patch.  It is a bit unfortunate that the generated struct
names and visit_type_FOO_members() don't match normal naming
conventions, but it's not too bad, since they will only be used in
generated code.

The generated code grows substantially in size: the implicit
'-wrapper' types must be emitted in qapi-types.h before any union
can include an unboxed member of that type.  Arguably, the '-args'
types could be emitted in a private header for just qapi-visit.c
and qmp-marshal.c, rather than polluting qapi-types.h; but adding
complexity to the generator to split the output location according
to role doesn't seem worth the maintenance costs.  Another idea
for a later patch would be reworking error.h to not need to
include all of qapi-types.h.

Signed-off-by: Eric Blake 

---
v5: rebase onto different naming scheme, document future improvements
v4: new patch
---
 scripts/qapi.py   |  2 --
 scripts/qapi-types.py | 14 --
 scripts/qapi-visit.py | 20 ++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f6701f5..96fb216 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1000,7 +1000,6 @@ class QAPISchemaObjectType(QAPISchemaType):
 return self.name.startswith('q_')

 def c_name(self):
-assert not self.is_implicit()
 return QAPISchemaType.c_name(self)

 def c_type(self):
@@ -1008,7 +1007,6 @@ class QAPISchemaObjectType(QAPISchemaType):
 return c_name(self.name) + pointer_suffix

 def c_unboxed_type(self):
-assert not self.is_implicit()
 return c_name(self.name)

 def json_type(self):
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f194bea..107eabe 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -61,8 +61,7 @@ def gen_object(name, base, members, variants):
 ret = ''
 if variants:
 for v in variants.variants:
-if (isinstance(v.type, QAPISchemaObjectType) and
-not v.type.is_implicit()):
+if isinstance(v.type, QAPISchemaObjectType):
 ret += gen_object(v.type.name, v.type.base,
   v.type.local_members, v.type.variants)

@@ -197,9 +196,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self._btin = None

 def visit_needed(self, entity):
-# Visit everything except implicit objects
-return not (entity.is_implicit() and
-isinstance(entity, QAPISchemaObjectType))
+# Visit everything except the special empty builtin
+return entity.name != 'q_empty'

 def _gen_type_cleanup(self, name):
 self.decl += gen_type_cleanup_decl(name)
@@ -233,7 +231,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self.decl += gen_object(name, base, members, variants)
 if base:
 self.decl += gen_upcast(name, base)
-self._gen_type_cleanup(name)
+# FIXME Worth changing the visitor signature, so we could
+# directly use rather than repeat type.is_implicit()?
+if not name.startswith('q_'):
+# implicit types won't be 

[Qemu-devel] [PATCH v5 11/14] qapi: Don't special-case simple union wrappers

[Eric Blake]

Simple unions were carrying a special case that hid their 'data'
QMP member from the resulting C struct, via the hack method
QAPISchemaObjectTypeVariant.simple_union_type().  But by using
the work we started by unboxing flat union and alternate
branches, coupled with the ability to visit the members of an
implicit type, we can now expose the simple union's implicit
type in qapi-types.h:

| struct q_obj_ImageInfoSpecificQCow2_wrapper {
| ImageInfoSpecificQCow2 *data;
| };
|
| struct q_obj_ImageInfoSpecificVmdk_wrapper {
| ImageInfoSpecificVmdk *data;
| };
...
| struct ImageInfoSpecific {
| ImageInfoSpecificKind type;
| union { /* union tag is @type */
| void *data;
|-ImageInfoSpecificQCow2 *qcow2;
|-ImageInfoSpecificVmdk *vmdk;
|+q_obj_ImageInfoSpecificQCow2_wrapper qcow2;
|+q_obj_ImageInfoSpecificVmdk_wrapper vmdk;
| } u;
| };

Doing this removes asymmetry between QAPI's QMP side and its
C side (both sides now expose 'data'), and means that the
treatment of a simple union as sugar for a flat union is now
equivalent in both languages (previously the two approaches used
a different layer of dereferencing, where the simple union could
be converted to a flat union with equivalent C layout but
different {} on the wire, or to an equivalent QMP wire form
but with different C representation).  Using the implicit type
also lets us get rid of the simple_union_type() hack.

Of course, now all clients of simple unions have to adjust from
using su->u.member to using su->u.member.data; while this touches
a number of files in the tree, some earlier cleanup patches
helped minimize the change to the initialization of a temporary
variable rather than every single member access.  The generated
qapi-visit.c code is also affected by the layout change:

|@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member
| }
| switch (obj->type) {
| case IMAGE_INFO_SPECIFIC_KIND_QCOW2:
|-visit_type_ImageInfoSpecificQCow2(v, "data", >u.qcow2, );
|+visit_type_q_obj_ImageInfoSpecificQCow2_wrapper_members(v, 
>u.qcow2, );
| break;
| case IMAGE_INFO_SPECIFIC_KIND_VMDK:
|-visit_type_ImageInfoSpecificVmdk(v, "data", >u.vmdk, );
|+visit_type_q_obj_ImageInfoSpecificVmdk_wrapper_members(v, 
>u.vmdk, );
| break;
| default:
| abort();

Signed-off-by: Eric Blake 

---
v5: rebase to q_obj naming change, hoist assertion earlier in series
v4: improve commit message, rebase onto exposed implicit types
[no v3]
v2: rebase onto s/fields/members/ change, changes in master; pick
up missing net/ files
---
 scripts/qapi.py | 10 -
 scripts/qapi-types.py   |  8 +---
 scripts/qapi-visit.py   | 22 ++-
 backends/baum.c |  2 +-
 backends/msmouse.c  |  2 +-
 block/nbd.c |  6 +--
 block/qcow2.c   |  6 +--
 block/vmdk.c|  8 ++--
 blockdev.c  | 24 ++--
 hmp.c   |  8 ++--
 hw/char/escc.c  |  2 +-
 hw/input/hid.c  |  8 ++--
 hw/input/ps2.c  |  6 +--
 hw/input/virtio-input-hid.c |  8 ++--
 hw/mem/pc-dimm.c|  2 +-
 net/dump.c  |  2 +-
 net/hub.c   |  2 +-
 net/l2tpv3.c|  2 +-
 net/net.c   |  4 +-
 net/netmap.c|  2 +-
 net/slirp.c |  2 +-
 net/socket.c|  2 +-
 net/tap-win32.c |  2 +-
 net/tap.c   |  4 +-
 net/vde.c   |  2 +-
 net/vhost-user.c|  2 +-
 numa.c  |  4 +-
 qemu-char.c | 82 +
 qemu-nbd.c  |  6 +--
 replay/replay-input.c   | 44 +++---
 spice-qemu-char.c   | 14 ---
 tests/test-io-channel-socket.c  | 40 ++--
 tests/test-qmp-commands.c   |  2 +-
 tests/test-qmp-input-visitor.c  | 25 +++--
 tests/test-qmp-output-visitor.c | 24 ++--
 tpm.c   |  2 +-
 ui/console.c|  4 +-
 ui/input-keymap.c   | 10 ++---
 ui/input-legacy.c   |  8 ++--
 ui/input.c  | 34 -
 ui/vnc-auth-sasl.c  |  3 +-
 ui/vnc.c| 29 ---
 util/qemu-sockets.c | 35 +-
 43 files changed, 246 insertions(+), 268 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 08d63bf..d91af94 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1115,16 +1115,6 @@ class 
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 def __init__(self, name, typ):
 QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

-# This 

[Qemu-devel] [PATCH v5 10/14] qapi: Drop unused c_null()

[Eric Blake]

Now that we are always bulk-initializing a QAPI C struct to 0
(whether by g_malloc0() or by 'Type qapi = {0};'), we no longer
have any clients of c_null() in the generator for per-element
initialization.  This patch is easy enough to revert if we find
a use in the future, but in the present, get rid of the dead code.

Signed-off-by: Eric Blake 

---
v5: new patch
---
 scripts/qapi.py | 46 +-
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3b50e4d..08d63bf 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -836,9 +836,6 @@ class QAPISchemaType(QAPISchemaEntity):
 def c_unboxed_type(self):
 return self.c_type()

-def c_null(self):
-return 'NULL'
-
 def json_type(self):
 pass

@@ -854,14 +851,13 @@ class QAPISchemaType(QAPISchemaEntity):


 class QAPISchemaBuiltinType(QAPISchemaType):
-def __init__(self, name, json_type, c_type, c_null):
+def __init__(self, name, json_type, c_type):
 QAPISchemaType.__init__(self, name, None)
 assert not c_type or isinstance(c_type, str)
 assert json_type in ('string', 'number', 'int', 'boolean', 'null',
  'value')
 self._json_type_name = json_type
 self._c_type_name = c_type
-self._c_null_val = c_null

 def c_name(self):
 return self.name
@@ -874,9 +870,6 @@ class QAPISchemaBuiltinType(QAPISchemaType):
 return 'const ' + self._c_type_name
 return self._c_type_name

-def c_null(self):
-return self._c_null_val
-
 def json_type(self):
 return self._json_type_name

@@ -909,10 +902,6 @@ class QAPISchemaEnumType(QAPISchemaType):
 def member_names(self):
 return [v.name for v in self.values]

-def c_null(self):
-return c_enum_const(self.name, (self.member_names() + ['_MAX'])[0],
-self.prefix)
-
 def json_type(self):
 return 'string'

@@ -1240,9 +1229,8 @@ class QAPISchema(object):
 def lookup_type(self, name):
 return self.lookup_entity(name, QAPISchemaType)

-def _def_builtin_type(self, name, json_type, c_type, c_null):
-self._def_entity(QAPISchemaBuiltinType(name, json_type,
-   c_type, c_null))
+def _def_builtin_type(self, name, json_type, c_type):
+self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
 # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
 # qapi-types.h from a single .c, all arrays of builtins must be
 # declared in the first file whether or not they are used.  Nicer
@@ -1251,20 +1239,20 @@ class QAPISchema(object):
 self._make_array_type(name, None)

 def _def_predefineds(self):
-for t in [('str','string',  'char' + pointer_suffix, 'NULL'),
-  ('number', 'number',  'double',   '0'),
-  ('int','int', 'int64_t',  '0'),
-  ('int8',   'int', 'int8_t',   '0'),
-  ('int16',  'int', 'int16_t',  '0'),
-  ('int32',  'int', 'int32_t',  '0'),
-  ('int64',  'int', 'int64_t',  '0'),
-  ('uint8',  'int', 'uint8_t',  '0'),
-  ('uint16', 'int', 'uint16_t', '0'),
-  ('uint32', 'int', 'uint32_t', '0'),
-  ('uint64', 'int', 'uint64_t', '0'),
-  ('size',   'int', 'uint64_t', '0'),
-  ('bool',   'boolean', 'bool', 'false'),
-  ('any','value',   'QObject' + pointer_suffix, 'NULL')]:
+for t in [('str','string',  'char' + pointer_suffix),
+  ('number', 'number',  'double'),
+  ('int','int', 'int64_t'),
+  ('int8',   'int', 'int8_t'),
+  ('int16',  'int', 'int16_t'),
+  ('int32',  'int', 'int32_t'),
+  ('int64',  'int', 'int64_t'),
+  ('uint8',  'int', 'uint8_t'),
+  ('uint16', 'int', 'uint16_t'),
+  ('uint32', 'int', 'uint32_t'),
+  ('uint64', 'int', 'uint64_t'),
+  ('size',   'int', 'uint64_t'),
+  ('bool',   'boolean', 'bool'),
+  ('any','value',   'QObject' + pointer_suffix)]:
 self._def_builtin_type(*t)
 self.the_empty_object_type = QAPISchemaObjectType('q_empty', None,
   None, [], None)
-- 
2.5.0

[Qemu-devel] [PATCH v5 12/14] qapi: Make BlockdevOptions doc example closer to reality

[Eric Blake]

Although we don't want to repeat the entire BlockdevOptions
QMP command in the example, it helps if we aren't needlessly
diverging (the initial example was written before we had
committed the actual QMP interface).  Use names that match what
is found in qapi/block-core.json, such as '*read-only' rather
than 'readonly', or 'BlockdevRef' rather than 'BlockRef'.

For the simple union example, invent BlockdevOptionsSimple so
that later text is unambiguous which of the two union forms is
meant (telling the user to refer back to two 'BlockdevOptions'
wasn't nice, and QMP has only the flat union form).

Also, mention that the discriminator of a flat union is
non-optional.

Signed-off-by: Eric Blake 

---
v5: split out from patch 8/10, sync more naming
---
 docs/qapi-code-gen.txt | 74 +-
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c648f76..12af1b8 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -297,22 +297,22 @@ be empty.
 A simple union type defines a mapping from automatic discriminator
 values to data types like in this example:

- { 'struct': 'FileOptions', 'data': { 'filename': 'str' } }
- { 'struct': 'Qcow2Options',
-   'data': { 'backing-file': 'str', 'lazy-refcounts': 'bool' } }
+ { 'struct': 'BlockdevOptionsFile', 'data': { 'filename': 'str' } }
+ { 'struct': 'BlockdevOptionsQcow2',
+   'data': { 'backing': 'str', '*lazy-refcounts': 'bool' } }

- { 'union': 'BlockdevOptions',
-   'data': { 'file': 'FileOptions',
- 'qcow2': 'Qcow2Options' } }
+ { 'union': 'BlockdevOptionsSimple',
+   'data': { 'file': 'BlockdevOptionsFile',
+ 'qcow2': 'BlockdevOptionsQcow2' } }

 In the Client JSON Protocol, a simple union is represented by a
 dictionary that contains the 'type' member as a discriminator, and a
 'data' member that is of the specified data type corresponding to the
 discriminator value, as in these examples:

- { "type": "file", "data" : { "filename": "/some/place/my-image" } }
- { "type": "qcow2", "data" : { "backing-file": "/some/place/my-image",
-   "lazy-refcounts": true } }
+ { "type": "file", "data": { "filename": "/some/place/my-image" } }
+ { "type": "qcow2", "data": { "backing": "/some/place/my-image",
+  "lazy-refcounts": true } }

 The generated C code uses a struct containing a union. Additionally,
 an implicit C enum 'NameKind' is created, corresponding to the union
@@ -325,29 +325,29 @@ avoids nesting on the wire.  All branches of the union 
must be
 complex types, and the top-level members of the union dictionary on
 the wire will be combination of members from both the base type and the
 appropriate branch type (when merging two dictionaries, there must be
-no keys in common).  The 'discriminator' member must be the name of an
-enum-typed member of the base struct.
+no keys in common).  The 'discriminator' member must be the name of a
+non-optional enum-typed member of the base struct.

 The following example enhances the above simple union example by
-adding a common member 'readonly', renaming the discriminator to
-something more applicable, and reducing the number of {} required on
-the wire:
+adding an optional common member 'read-only', renaming the
+discriminator to something more applicable than the simple union's
+default of 'type', and reducing the number of {} required on the wire:

  { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
- { 'struct': 'BlockdevCommonOptions',
-   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
+ { 'struct': 'BlockdevOptionsBase',
+   'data': { 'driver': 'BlockdevDriver', '*read-only': 'bool' } }
  { 'union': 'BlockdevOptions',
-   'base': 'BlockdevCommonOptions',
+   'base': 'BlockdevOptionsBase',
'discriminator': 'driver',
-   'data': { 'file': 'FileOptions',
- 'qcow2': 'Qcow2Options' } }
+   'data': { 'file': 'BlockdevOptionsFile',
+ 'qcow2': 'BlockdevOptionsQcow2' } }

 Resulting in these JSON objects:

- { "driver": "file", "readonly": true,
+ { "driver": "file", "read-only": true,
"filename": "/some/place/my-image" }
- { "driver": "qcow2", "readonly": false,
-   "backing-file": "/some/place/my-image", "lazy-refcounts": true }
+ { "driver": "qcow2", "read-only": false,
+   "backing": "/some/place/my-image", "lazy-refcounts": true }

 Notice that in a flat union, the discriminator name is controlled by
 the user, but because it must map to a base member with enum type, the
@@ -382,7 +382,7 @@ data types (string, integer, number, or object, but 
currently not
 array) on the wire.  The definition is similar to a simple union type,
 where each branch of the union names a QAPI type.  For example:

- { 'alternate': 'BlockRef',
+ { 'alternate': 'BlockdevRef',
'data': { 'definition': 'BlockdevOptions',
  'reference': 'str' } }

@@ -403,7 +403,7 @@ 

Re: [Qemu-devel] [PULL 3/8] vfio: Generalize region support

[Eric Auger]

Hi Alex,
On 03/09/2016 08:53 PM, Alex Williamson wrote:
> Both platform and PCI vfio drivers create a "slow", I/O memory region
> with one or more mmap memory regions overlayed when supported by the
> device. Generalize this to a set of common helpers in the core that
> pulls the region info from vfio, fills the region data, configures
> slow mapping, and adds helpers for comleting the mmap, enable/disable,
> and teardown.  This can be immediately used by the PCI MSI-X code,
> which needs to mmap around the MSI-X vector table.
> 
> This also changes VFIORegion.mem to be dynamically allocated because
> otherwise we don't know how the caller has allocated VFIORegion and
> therefore don't know whether to unreference it to destroy the
> MemoryRegion or not.
> 
> Signed-off-by: Alex Williamson 
> ---
>  hw/arm/sysbus-fdt.c   |4 -
>  hw/vfio/common.c  |  172 
> ++---
>  hw/vfio/pci-quirks.c  |   24 +++---
>  hw/vfio/pci.c |  168 +---
>  hw/vfio/platform.c|   72 +++--
>  include/hw/vfio/vfio-common.h |   23 -
>  trace-events  |   10 ++
>  7 files changed, 283 insertions(+), 190 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 04afeae..49bd212 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -240,7 +240,7 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice 
> *sbdev, void *opaque)
>  mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
>  reg_attr[2 * i] = cpu_to_be32(mmio_base);
>  reg_attr[2 * i + 1] = cpu_to_be32(
> -memory_region_size(>regions[i]->mem));
> +memory_region_size(vdev->regions[i]->mem));
>  }
>  qemu_fdt_setprop(fdt, nodename, "reg", reg_attr,
>   vbasedev->num_regions * 2 * sizeof(uint32_t));
> @@ -374,7 +374,7 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, 
> void *opaque)
>  mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
>  reg_attr[2 * i] = cpu_to_be32(mmio_base);
>  reg_attr[2 * i + 1] = cpu_to_be32(
> -memory_region_size(>regions[i]->mem));
> +memory_region_size(vdev->regions[i]->mem));
>  }
>  qemu_fdt_setprop(guest_fdt, nodename, "reg", reg_attr,
>   vbasedev->num_regions * 2 * sizeof(uint32_t));
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index e20fc4f..96ccb79 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -493,46 +493,162 @@ static void vfio_listener_release(VFIOContainer 
> *container)
>  memory_listener_unregister(>listener);
>  }
>  
> -int vfio_mmap_region(Object *obj, VFIORegion *region,
> - MemoryRegion *mem, MemoryRegion *submem,
> - void **map, size_t size, off_t offset,
> - const char *name)
> +int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
> +  int index, const char *name)
>  {
> -int ret = 0;
> -VFIODevice *vbasedev = region->vbasedev;
> +struct vfio_region_info *info;
> +int ret;
> +
> +ret = vfio_get_region_info(vbasedev, index, );
> +if (ret) {
> +return ret;
> +}
> +
> +region->vbasedev = vbasedev;
> +region->flags = info->flags;
> +region->size = info->size;
> +region->fd_offset = info->offset;
> +region->nr = index;
>  
> -if (!vbasedev->no_mmap && size && region->flags &
> -VFIO_REGION_INFO_FLAG_MMAP) {
> -int prot = 0;
> +if (region->size) {
> +region->mem = g_new0(MemoryRegion, 1);
> +memory_region_init_io(region->mem, obj, _region_ops,
> +  region, name, region->size);
>  
> -if (region->flags & VFIO_REGION_INFO_FLAG_READ) {
> -prot |= PROT_READ;
> +if (!vbasedev->no_mmap &&
> +region->flags & VFIO_REGION_INFO_FLAG_MMAP &&
> +!(region->size & ~qemu_real_host_page_mask)) {
> +
> +region->nr_mmaps = 1;
> +region->mmaps = g_new0(VFIOMmap, region->nr_mmaps);
> +
> +region->mmaps[0].offset = 0;
> +region->mmaps[0].size = region->size;
>  }
> +}
> +
> +g_free(info);
> +
> +trace_vfio_region_setup(vbasedev->name, index, name,
> +region->flags, region->fd_offset, region->size);
> +return 0;
> +}
>  
> -if (region->flags & VFIO_REGION_INFO_FLAG_WRITE) {
> -prot |= PROT_WRITE;
> +int vfio_region_mmap(VFIORegion *region)
> +{
> +int i, prot = 0;
> +char *name;
> +
> +if (!region->mem) {
> +return 0;
> +}
> +
> +prot |= region->flags & VFIO_REGION_INFO_FLAG_READ ? PROT_READ : 0;
> +prot |= region->flags & 

[Qemu-devel] [PATCH v5 14/14] qapi: Use anonymous bases in QMP flat unions

[Eric Blake]

Now that the generator supports it, we might as well use an
anonymous base rather than breaking out a single-use Base
structure, for all three of our current QMP flat unions.

Oddly enough, this change does not affect the resulting
introspection output (because we already inline the members of
a base type into an object, and had no independent use of the
base type reachable from a command).

The case_whitelist now has to list the name of an implicit
type; which is not too bad (consider it a feature if it makes
it harder for developers to make the whitelist grow :)

Signed-off-by: Eric Blake 

---
v5: rebase to implicit type name change
v4: retitle, merge, add BlockdevOptions
[no v3]
v2: no change
v1: no change
Previously posted as part of qapi cleanup subset F:
v6: new patch
---
 scripts/qapi.py  |  2 +-
 qapi-schema.json | 20 ---
 qapi/block-core.json | 98 
 qapi/introspect.json | 12 +--
 4 files changed, 53 insertions(+), 79 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index a38ef52..b13ae47 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -63,12 +63,12 @@ returns_whitelist = [
 case_whitelist = [
 # From QMP:
 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
-'CpuInfoBase',  # CPU, visible through query-cpu
 'CpuInfoMIPS',  # PC, visible through query-cpu
 'CpuInfoTricore',   # PC, visible through query-cpu
 'QapiErrorClass',   # all members, visible through errors
 'UuidInfo', # UUID, visible through query-uuid
 'X86CPURegister32', # all members, visible indirectly through qom-get
+'q_obj_CpuInfo-base',   # CPU, visible through query-cpu
 ]

 enum_types = []
diff --git a/qapi-schema.json b/qapi-schema.json
index 362c9d8..bd03bcc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -753,9 +753,9 @@
   'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }

 ##
-# @CpuInfoBase:
+# @CpuInfo:
 #
-# Common information about a virtual CPU
+# Information about a virtual CPU
 #
 # @CPU: the index of the virtual CPU
 #
@@ -776,18 +776,10 @@
 # Notes: @halted is a transient state that changes frequently.  By the time the
 #data is sent to the client, the guest may no longer be halted.
 ##
-{ 'struct': 'CpuInfoBase',
-  'data': {'CPU': 'int', 'current': 'bool', 'halted': 'bool',
-   'qom_path': 'str', 'thread_id': 'int', 'arch': 'CpuInfoArch' } }
-
-##
-# @CpuInfo:
-#
-# Information about a virtual CPU
-#
-# Since: 0.14.0
-##
-{ 'union': 'CpuInfo', 'base': 'CpuInfoBase', 'discriminator': 'arch',
+{ 'union': 'CpuInfo',
+  'base': {'CPU': 'int', 'current': 'bool', 'halted': 'bool',
+   'qom_path': 'str', 'thread_id': 'int', 'arch': 'CpuInfoArch' },
+  'discriminator': 'arch',
   'data': { 'x86': 'CpuInfoX86',
 'sparc': 'CpuInfoSPARC',
 'ppc': 'CpuInfoPPC',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9bf1b22..b1cf77d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1644,57 +1644,6 @@
 'vmdk', 'vpc', 'vvfat' ] }

 ##
-# @BlockdevOptionsBase
-#
-# Options that are available for all block devices, independent of the block
-# driver.
-#
-# @driver:block driver name
-# @id:#optional id by which the new block device can be referred 
to.
-# This option is only allowed on the top level of blockdev-add.
-# A BlockBackend will be created by blockdev-add if and only if
-# this option is given.
-# @node-name: #optional the name of a block driver state node (Since 2.0).
-# This option is required on the top level of blockdev-add if
-# the @id option is not given there.
-# @discard:   #optional discard-related options (default: ignore)
-# @cache: #optional cache-related options
-# @aio:   #optional AIO backend (default: threads)
-# @rerror:#optional how to handle read errors on the device
-# (default: report)
-# @werror:#optional how to handle write errors on the device
-# (default: enospc)
-# @read-only: #optional whether the block device should be read-only
-# (default: false)
-# @stats-account-invalid: #optional whether to include invalid
-# operations when computing last access statistics
-# (default: true) (Since 2.5)
-# @stats-account-failed: #optional whether to include failed
-# operations when computing latency and last
-# access statistics (default: true) (Since 2.5)
-# @stats-intervals: #optional list of intervals for collecting I/O
-#   statistics, in seconds (default: none) (Since 2.5)
-# @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
-# (default: off)
-#
-# Since: 1.7

[Qemu-devel] [PATCH v5 08/14] qapi-commands: Inline single-use helpers of gen_marshal()

[Eric Blake]

Originally, gen_marshal_input_visit() (or gen_visitor_input_block()
before commit f1538019) was factored out to make it easy to do two
passes of a visit to each member of a (possibly-implicit) object,
without duplicating lots of code.  But after recent changes, those
visits now occupy a single line of emitted code, and the helper
method has become a series of conditionals both before and after
the one important line, making it rather awkward to see at a glance
what gets emitted on the first (parsing) or second (deallocation)
pass.  It's a lot easier to read the generator code if we just
inline both uses directly into gen_marshal(), without all the
conditionals.

Once we've done that, it's easy to notice that gen_marshal_vars()
is used only once, and inlining it too lets us consolidate some
mcgen() calls that used to be split across helpers.

gen_call() remains a single-use helper function, but it has
enough indentation and complexity that inlining it would hamper
legibility.

No change to generated output.  The fact that the diffstat shows
a net reduction in lines is an argument in favor of this cleanup.

Signed-off-by: Eric Blake 

---
v5: no change
v4: new patch
---
 scripts/qapi-commands.py | 106 +--
 1 file changed, 39 insertions(+), 67 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 5ffc381..710e853 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -55,68 +55,6 @@ def gen_call(name, arg_type, ret_type):
 return ret


-def gen_marshal_vars(arg_type, ret_type):
-ret = mcgen('''
-Error *err = NULL;
-''')
-
-if ret_type:
-ret += mcgen('''
-%(c_type)s retval;
-''',
- c_type=ret_type.c_type())
-
-if arg_type and arg_type.members:
-ret += mcgen('''
-QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
-QapiDeallocVisitor *qdv;
-Visitor *v;
-%(c_name)s qapi = {0};
-
-''',
- c_name=arg_type.c_name())
-else:
-ret += mcgen('''
-
-(void)args;
-''')
-
-return ret
-
-
-def gen_marshal_input_visit(arg_type, dealloc=False):
-ret = ''
-
-if not arg_type or not arg_type.members:
-return ret
-
-if dealloc:
-ret += mcgen('''
-qmp_input_visitor_cleanup(qiv);
-qdv = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(qdv);
-''')
-errp = 'NULL'
-else:
-ret += mcgen('''
-v = qmp_input_get_visitor(qiv);
-''')
-errp = ''
-
-ret += mcgen('''
-visit_type_%(c_name)s_members(v, , %(errp)s);
-''',
- c_name=arg_type.c_name(), errp=errp)
-
-if dealloc:
-ret += mcgen('''
-qapi_dealloc_visitor_cleanup(qdv);
-''')
-else:
-ret += gen_err_check()
-return ret
-
-
 def gen_marshal_output(ret_type):
 return mcgen('''

@@ -165,15 +103,40 @@ def gen_marshal(name, arg_type, ret_type):

 %(proto)s
 {
+Error *err = NULL;
 ''',
 proto=gen_marshal_proto(name))

-ret += gen_marshal_vars(arg_type, ret_type)
-ret += gen_marshal_input_visit(arg_type)
+if ret_type:
+ret += mcgen('''
+%(c_type)s retval;
+''',
+ c_type=ret_type.c_type())
+
+if arg_type and arg_type.members:
+ret += mcgen('''
+QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+QapiDeallocVisitor *qdv;
+Visitor *v;
+%(c_name)s qapi = {0};
+
+v = qmp_input_get_visitor(qiv);
+visit_type_%(c_name)s_members(v, , );
+if (err) {
+goto out;
+}
+''',
+ c_name=arg_type.c_name())
+
+else:
+ret += mcgen('''
+
+(void)args;
+''')
+
 ret += gen_call(name, arg_type, ret_type)

-# 'goto out' produced by gen_marshal_input_visit->gen_visit_members()
-# for each arg_type member, and by gen_call() for ret_type
+# 'goto out' produced above for arg_type, and by gen_call() for ret_type
 if (arg_type and arg_type.members) or ret_type:
 ret += mcgen('''

@@ -182,7 +145,16 @@ out:
 ret += mcgen('''
 error_propagate(errp, err);
 ''')
-ret += gen_marshal_input_visit(arg_type, dealloc=True)
+if arg_type and arg_type.members:
+ret += mcgen('''
+qmp_input_visitor_cleanup(qiv);
+qdv = qapi_dealloc_visitor_new();
+v = qapi_dealloc_get_visitor(qdv);
+visit_type_%(c_name)s_members(v, , NULL);
+qapi_dealloc_visitor_cleanup(qdv);
+''',
+ c_name=arg_type.c_name())
+
 ret += mcgen('''
 }
 ''')
-- 
2.5.0

[Qemu-devel] [PATCH v5 00/14] easier unboxed visits/qapi implicit types

[Eric Blake]

Addresses Markus' comments on v4; the series was pretty close, and
most of the changes here are a new naming scheme ('q_obj' rather
than ':obj') and additional cleanups (for example, c_null() is
unused).  I dropped patch 10, but the new patches make the series
a bit longer.

Built directly on master qemu.git.

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git qapi-implicit-v5

and will soon be part of my branch at:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

v4: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01205.html

001/14:[0001] [FC] 'qapi: Assert in places where variants are not handled'
002/14:[] [--] 'qapi: Fix command with named empty argument type'
003/14:[] [--] 'qapi: Make c_type() more OO-like'
004/14:[down] 'qapi: Adjust names of implicit types'
005/14:[0019] [FC] 'qapi: Emit implicit structs in generated C'
006/14:[down] 'qapi-event: Slightly shrink generated code'
007/14:[0055] [FC] 'qapi: Utilize implicit struct visits'
008/14:[] [--] 'qapi-commands: Inline single-use helpers of gen_marshal()'
009/14:[down] 'qapi: Inline gen_visit_members() into lone caller'
010/14:[down] 'qapi: Drop unused c_null()'
011/14:[0001] [FC] 'qapi: Don't special-case simple union wrappers'
012/14:[down] 'qapi: Make BlockdevOptions doc example closer to reality'
013/14:[0026] [FC] 'qapi: Allow anonymous base for flat union'
014/14:[0002] [FC] 'qapi: Use anonymous bases in QMP flat unions'

Eric Blake (14):
  qapi: Assert in places where variants are not handled
  qapi: Fix command with named empty argument type
  qapi: Make c_type() more OO-like
  qapi: Adjust names of implicit types
  qapi: Emit implicit structs in generated C
  qapi-event: Slightly shrink generated code
  qapi: Utilize implicit struct visits
  qapi-commands: Inline single-use helpers of gen_marshal()
  qapi: Inline gen_visit_members() into lone caller
  qapi: Drop unused c_null()
  qapi: Don't special-case simple union wrappers
  qapi: Make BlockdevOptions doc example closer to reality
  qapi: Allow anonymous base for flat union
  qapi: Use anonymous bases in QMP flat unions

 scripts/qapi.py| 171 -
 scripts/qapi-commands.py   | 117 
 scripts/qapi-event.py  |  58 +++---
 scripts/qapi-types.py  |  29 ++---
 scripts/qapi-visit.py  |  65 +--
 backends/baum.c|   2 +-
 backends/msmouse.c |   2 +-
 block/nbd.c|   6 +-
 block/qcow2.c  |   6 +-
 block/vmdk.c   |   8 +-
 blockdev.c |  24 ++--
 hmp.c  |   8 +-
 hw/char/escc.c |   2 +-
 hw/input/hid.c |   8 +-
 hw/input/ps2.c |   6 +-
 hw/input/virtio-input-hid.c|   8 +-
 hw/mem/pc-dimm.c   |   2 +-
 net/dump.c |   2 +-
 net/hub.c  |   2 +-
 net/l2tpv3.c   |   2 +-
 net/net.c  |   4 +-
 net/netmap.c   |   2 +-
 net/slirp.c|   2 +-
 net/socket.c   |   2 +-
 net/tap-win32.c|   2 +-
 net/tap.c  |   4 +-
 net/vde.c  |   2 +-
 net/vhost-user.c   |   2 +-
 numa.c |   4 +-
 qemu-char.c|  82 +++---
 qemu-nbd.c |   6 +-
 replay/replay-input.c  |  44 
 spice-qemu-char.c  |  14 ++-
 tests/test-io-channel-socket.c |  40 +++
 tests/test-qmp-commands.c  |   7 +-
 tests/test-qmp-input-visitor.c |  25 +++--
 tests/test-qmp-output-visitor.c|  24 ++--
 tpm.c  |   2 +-
 ui/console.c   |   4 +-
 ui/input-keymap.c  |  10 +-
 ui/input-legacy.c  |   8 +-
 ui/input.c |  34 +++---
 ui/vnc-auth-sasl.c |   3 +-
 ui/vnc.c   |  29 ++---
 util/qemu-sockets.c|  35 +++---
 docs/qapi-code-gen.txt |  98 -
 qapi-schema.json   |  20 +---
 qapi/block-core.json   |  98 -
 qapi/introspect.json   |  12 +-
 tests/qapi-schema/comments.out |   2 +-
 tests/qapi-schema/empty.out|   2 +-
 

[Qemu-devel] [PATCH v5 04/14] qapi: Adjust names of implicit types

[Eric Blake]

The original choice of ':obj-' as the prefix for implicit types
made it obvious that we weren't going to clash with any user-defined
names.  But now we want to create structs for implicit types.  We
could transliterate ':' to '_', except that C99 says that a leading
underscore and lower-case letter should be used only for file scope
identifiers, while we would be exposing it in qapi-types.h.  So it's
time to change our naming convention; we can instead use the 'q_'
prefix that we reserved for ourselves back in commit 9fb081e0.  As
long as we don't declare 'empty' or 'obj' ticklish, it shouldn't
clash with c_name() prepending 'q_' to the user's ticklish names.

Signed-off-by: Eric Blake 

---
v5: new patch
---
 scripts/qapi.py  |  18 ++--
 docs/qapi-code-gen.txt   |  14 +--
 tests/qapi-schema/comments.out   |   2 +-
 tests/qapi-schema/empty.out  |   2 +-
 tests/qapi-schema/event-case.out |   2 +-
 tests/qapi-schema/ident-with-escape.out  |   8 +-
 tests/qapi-schema/include-relpath.out|   2 +-
 tests/qapi-schema/include-repetition.out |   2 +-
 tests/qapi-schema/include-simple.out |   2 +-
 tests/qapi-schema/indented-expr.out  |   2 +-
 tests/qapi-schema/qapi-schema-test.out   | 154 +++
 11 files changed, 105 insertions(+), 103 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b7fbdae..f6701f5 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -391,7 +391,8 @@ def check_name(expr_info, source, name, 
allow_optional=False,
 # code always prefixes it with the enum name
 if enum_member and membername[0].isdigit():
 membername = 'D' + membername
-# Reserve the entire 'q_' namespace for c_name()
+# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
+# and 'q_obj_*' implicit type names.
 if not valid_name.match(membername) or \
c_name(membername, False).startswith('q_'):
 raise QAPIExprError(expr_info,
@@ -994,8 +995,9 @@ class QAPISchemaObjectType(QAPISchemaType):
 m.check_clash(info, seen)

 def is_implicit(self):
-# See QAPISchema._make_implicit_object_type()
-return self.name[0] == ':'
+# See QAPISchema._make_implicit_object_type(), as well as
+# _def_predefineds()
+return self.name.startswith('q_')

 def c_name(self):
 assert not self.is_implicit()
@@ -1044,10 +1046,10 @@ class QAPISchemaMember(object):

 def _pretty_owner(self):
 owner = self.owner
-if owner.startswith(':obj-'):
+if owner.startswith('q_obj_'):
 # See QAPISchema._make_implicit_object_type() - reverse the
 # mapping there to create a nice human-readable description
-owner = owner[5:]
+owner = owner[6:]
 if owner.endswith('-arg'):
 return '(parameter of %s)' % owner[:-4]
 else:
@@ -1266,8 +1268,8 @@ class QAPISchema(object):
   ('bool',   'boolean', 'bool', 'false'),
   ('any','value',   'QObject' + pointer_suffix, 'NULL')]:
 self._def_builtin_type(*t)
-self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
-  [], None)
+self.the_empty_object_type = QAPISchemaObjectType('q_empty', None,
+  None, [], None)
 self._def_entity(self.the_empty_object_type)
 qtype_values = self._make_enum_members(['none', 'qnull', 'qint',
 'qstring', 'qdict', 'qlist',
@@ -1295,7 +1297,7 @@ class QAPISchema(object):
 if not members:
 return None
 # See also QAPISchemaObjectTypeMember._pretty_owner()
-name = ':obj-%s-%s' % (name, role)
+name = 'q_obj_%s-%s' % (name, role)
 if not self.lookup_entity(name, QAPISchemaObjectType):
 self._def_entity(QAPISchemaObjectType(name, info, None,
   members, None))
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index e0b2ef1..c648f76 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -575,9 +575,9 @@ names an object type without members.
 Example: the SchemaInfo for command query-qmp-schema

 { "name": "query-qmp-schema", "meta-type": "command",
-  "arg-type": ":empty", "ret-type": "SchemaInfoList" }
+  "arg-type": "q_empty", "ret-type": "SchemaInfoList" }

-Type ":empty" is an object type without members, and type
+Type "q_empty" is an automatic object type without members, and type
 "SchemaInfoList" is the array of SchemaInfo type.

 The SchemaInfo for an event has meta-type "event", and variant member
@@ -594,9 +594,9 @@ QAPI schema implicitly defines an object type.
 Example: the SchemaInfo for EVENT_C from section Events

[Qemu-devel] [PATCH v5 09/14] qapi: Inline gen_visit_members() into lone caller

[Eric Blake]

Commit 82ca8e46 noticed that we had multiple implementations of
visiting every member of a struct, and consolidated it into
gen_visit_fields() (now gen_visit_members()) with enough
parameters to cater to slight differences between the clients.
But recent exposure of implicit types has meant that we are now
down to a single use of that method, so we can clean up the
unused conditionals and just inline it into the remaining
caller: gen_visit_object_members().

Likewise, gen_err_check() no longer needs optional parameters,
as the lone use of non-defaults was via gen_visit_members().

No change to generated code.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 

---
v5: new patch
---
 scripts/qapi.py   | 46 ++
 scripts/qapi-visit.py | 23 ---
 2 files changed, 22 insertions(+), 47 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 96fb216..3b50e4d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1657,54 +1657,12 @@ def gen_params(arg_type, extra):
 return ret


-def gen_err_check(label='out', skiperr=False):
-if skiperr:
-return ''
+def gen_err_check():
 return mcgen('''
 if (err) {
-goto %(label)s;
-}
-''',
- label=label)
-
-
-def gen_visit_members(members, prefix='', need_cast=False, skiperr=False,
-  label='out'):
-ret = ''
-if skiperr:
-errparg = 'NULL'
-else:
-errparg = ''
-
-for memb in members:
-if memb.optional:
-ret += mcgen('''
-if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
-''',
- prefix=prefix, c_name=c_name(memb.name),
- name=memb.name)
-push_indent()
-
-# Ugly: sometimes we need to cast away const
-if need_cast and memb.type.name == 'str':
-cast = '(char **)'
-else:
-cast = ''
-
-ret += mcgen('''
-visit_type_%(c_type)s(v, "%(name)s", %(cast)s&%(prefix)s%(c_name)s, 
%(errp)s);
-''',
- c_type=memb.type.c_name(), prefix=prefix, cast=cast,
- c_name=c_name(memb.name), name=memb.name,
- errp=errparg)
-ret += gen_err_check(skiperr=skiperr, label=label)
-
-if memb.optional:
-pop_indent()
-ret += mcgen('''
+goto out;
 }
 ''')
-return ret


 #
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c486aaa..b425620 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -51,7 +51,24 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
*obj, Error **errp)
  c_type=base.c_name())
 ret += gen_err_check()

-ret += gen_visit_members(members, prefix='obj->')
+for memb in members:
+if memb.optional:
+ret += mcgen('''
+if (visit_optional(v, "%(name)s", >has_%(c_name)s)) {
+''',
+ name=memb.name, c_name=c_name(memb.name))
+push_indent()
+ret += mcgen('''
+visit_type_%(c_type)s(v, "%(name)s", >%(c_name)s, );
+''',
+ c_type=memb.type.c_name(), name=memb.name,
+ c_name=c_name(memb.name))
+ret += gen_err_check()
+if memb.optional:
+pop_indent()
+ret += mcgen('''
+}
+''')

 if variants:
 ret += mcgen('''
@@ -90,8 +107,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s 
*obj, Error **errp)
 }
 ''')

-# 'goto out' produced for base, by gen_visit_members() for each member,
-# and if variants were present
+# 'goto out' produced for base, for each member, and if variants were
+# present
 if base or members or variants:
 ret += mcgen('''

-- 
2.5.0

[Qemu-devel] [PATCH v5 02/14] qapi: Fix command with named empty argument type

[Eric Blake]

The generator special-cased

 { 'command':'foo', 'data': {} }

to avoid emitting a visitor variable, but failed to see that

 { 'struct':'NamedEmptyType, 'data': {} }
 { 'command':'foo', 'data':'NamedEmptyType' }

needs the same treatment.  There, the generator happily generates a
visitor to get no arguments, and a visitor to destroy no arguments;
and the compiler isn't happy with that, as demonstrated by the updated
qapi-schema-test.json:

  tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
  tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used 
[-Werror=unused-but-set-variable]
   Visitor *v;
^

No change to generated code except for the testsuite addition.

Signed-off-by: Eric Blake 

---
v5: no change
v4: move earlier in series, rebase without is_empty, drop R-b
[no v3]
v2: no change
v1: enhance commit message
Previously posted as part of qapi cleanup subset E:
v9: no change
v8: no change
v7: no change
v6: new patch
---
 scripts/qapi-commands.py| 4 ++--
 tests/test-qmp-commands.c   | 5 +
 tests/qapi-schema/qapi-schema-test.json | 2 ++
 tests/qapi-schema/qapi-schema-test.out  | 2 ++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index edcbd10..3784f33 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -66,7 +66,7 @@ def gen_marshal_vars(arg_type, ret_type):
 ''',
  c_type=ret_type.c_type())

-if arg_type:
+if arg_type and arg_type.members:
 ret += mcgen('''
 QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
 QapiDeallocVisitor *qdv;
@@ -98,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type):
 def gen_marshal_input_visit(arg_type, dealloc=False):
 ret = ''

-if not arg_type:
+if not arg_type or not arg_type.members:
 return ret

 if dealloc:
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index d6171f2..650ba46 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -13,6 +13,11 @@ void qmp_user_def_cmd(Error **errp)
 {
 }

+Empty2 *qmp_user_def_cmd0(Error **errp)
+{
+return g_new0(Empty2, 1);
+}
+
 void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
 {
 }
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 728659e..e722748 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -18,6 +18,8 @@
 { 'struct': 'Empty1', 'data': { } }
 { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }

+{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
+
 # for testing override of default naming heuristic
 { 'enum': 'QEnumTwo',
   'prefix': 'QENUM_TWO',
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index f5e2a73..f531961 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -203,6 +203,8 @@ command guest-sync :obj-guest-sync-arg -> any
gen=True success_response=True
 command user_def_cmd None -> None
gen=True success_response=True
+command user_def_cmd0 Empty2 -> Empty2
+   gen=True success_response=True
 command user_def_cmd1 :obj-user_def_cmd1-arg -> None
gen=True success_response=True
 command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
-- 
2.5.0

[Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits

[Eric Blake]

Rather than generate inline per-member visits, take advantage
of the 'visit_type_FOO_members()' function for both event and
command marshalling.  This is possible now that implicit
structs can be visited like any other.

Generated code shrinks accordingly; events initialize a struct
based on parameters, through a new gen_param_var() helper, like:

|@@ -338,6 +250,9 @@ void qapi_event_send_block_job_error(con
| QMPEventFuncEmit emit = qmp_event_get_func_emit();
| QmpOutputVisitor *qov;
| Visitor *v;
|+q_obj_BLOCK_JOB_ERROR_arg param = {
|+(char *)device, operation, action
|+};
|
| if (!emit) {
| return;
@@ -351,19 +266,7 @@ void qapi_event_send_block_job_error(con
| if (err) {
| goto out;
| }
|-visit_type_str(v, "device", (char **), );
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_IoOperationType(v, "operation", , );
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_BlockErrorAction(v, "action", , );
|-if (err) {
|-goto out_obj;
|-}
|-out_obj:
|+visit_type_q_obj_BLOCK_JOB_ERROR_arg_members(v, , );
| visit_end_struct(v, err ? NULL : );

Notice that the initialization of 'param' has to cast away const
(just as the old gen_visit_members() had to do): we can't change
the signature of the user function (which uses 'const char *'), but
have to assign it to a non-const QAPI object (which requires
'char *').

Likewise, command marshalling generates call arguments from a
stack-allocated struct, rather than a list of local variables:

|@@ -57,26 +57,15 @@ void qmp_marshal_add_fd(QDict *args, QOb
| QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
| QapiDeallocVisitor *qdv;
| Visitor *v;
|-bool has_fdset_id = false;
|-int64_t fdset_id = 0;
|-bool has_opaque = false;
|-char *opaque = NULL;
|-
|-v = qmp_input_get_visitor(qiv);
|-if (visit_optional(v, "fdset-id", _fdset_id)) {
|-visit_type_int(v, "fdset-id", _id, );
|-if (err) {
|-goto out;
|-}
|-}
|-if (visit_optional(v, "opaque", _opaque)) {
|-visit_type_str(v, "opaque", , );
|-if (err) {
|-goto out;
|-}
|+q_obj_add_fd_arg qapi = {0};
|+
|+v = qmp_input_get_visitor(qiv);
|+visit_type_q_obj_add_fd_arg_members(v, , );
|+if (err) {
|+goto out;
| }
|
|-retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, );
|+retval = qmp_add_fd(qapi.has_fdset_id, qapi.fdset_id, qapi.has_opaque, 
qapi.opaque, );
| if (err) {
| goto out;
| }
|@@ -88,12 +77,7 @@ out:
| qmp_input_visitor_cleanup(qiv);
| qdv = qapi_dealloc_visitor_new();
| v = qapi_dealloc_get_visitor(qdv);
|-if (visit_optional(v, "fdset-id", _fdset_id)) {
|-visit_type_int(v, "fdset-id", _id, NULL);
|-}
|-if (visit_optional(v, "opaque", _opaque)) {
|-visit_type_str(v, "opaque", , NULL);
|-}
|+visit_type_q_obj_add_fd_arg_members(v, , NULL);
| qapi_dealloc_visitor_cleanup(qdv);
| }

For the marshaller, it has the nice side effect of eliminating a
chance of collision between argument QMP names and local variables.

This patch also paves the way for some followup simplifications
in the generator, in subsequent patches.

Signed-off-by: Eric Blake 

---
v5: move qapi.py:gen_struct_init() to qapi-event.py:gen_param_var(),
improve commit message
v4: new patch
---
 scripts/qapi-commands.py | 28 
 scripts/qapi-event.py| 40 +++-
 2 files changed, 43 insertions(+), 25 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 3784f33..5ffc381 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -33,8 +33,8 @@ def gen_call(name, arg_type, ret_type):
 assert not arg_type.variants
 for memb in arg_type.members:
 if memb.optional:
-argstr += 'has_%s, ' % c_name(memb.name)
-argstr += '%s, ' % c_name(memb.name)
+argstr += 'qapi.has_%s, ' % c_name(memb.name)
+argstr += 'qapi.%s, ' % c_name(memb.name)

 lhs = ''
 if ret_type:
@@ -71,21 +71,10 @@ def gen_marshal_vars(arg_type, ret_type):
 QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
 QapiDeallocVisitor *qdv;
 Visitor *v;
-''')
+%(c_name)s qapi = {0};

-for memb in arg_type.members:
-if memb.optional:
-ret += mcgen('''
-bool has_%(c_name)s = false;
 ''',
- c_name=c_name(memb.name))
-ret += mcgen('''
-%(c_type)s %(c_name)s = %(c_null)s;
-''',
- c_name=c_name(memb.name),
- c_type=memb.type.c_type(),
- c_null=memb.type.c_null())
-ret += '\n'
+ c_name=arg_type.c_name())
 else:
 ret += 

[Qemu-devel] [PATCH v5 01/14] qapi: Assert in places where variants are not handled

[Eric Blake]

We are getting closer to the point where we could use one union
as the base or variant type within another union type (as long
as there are no collisions between any possible combination of
member names allowed across all discriminator choices).  But
until we get to that point, it is worth asserting that variants
are not present in places where we are not prepared to handle
them: when exploding a type into a parameter list, we do not
expect variants.  The qapi.py code is already checking this,
via the older check_type() method; but someday we hope to get
rid of that and move checking into QAPISchema*.check().  The
two asserts added here make sure any refactoring still catches
problems, and makes it locally obvious why we can iterate over
only type.members without worrying about type.variants.

Signed-off-by: Eric Blake 

---
v5: drop redundant hunk, improve commit message
v4: new patch, split out from .is_empty() patch
---
 scripts/qapi-commands.py | 3 ++-
 scripts/qapi-event.py| 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f44e01f..edcbd10 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -2,7 +2,7 @@
 # QAPI command marshaller generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (C) 2014-2015 Red Hat, Inc.
+# Copyright (C) 2014-2016 Red Hat, Inc.
 #
 # Authors:
 #  Anthony Liguori 
@@ -30,6 +30,7 @@ def gen_call(name, arg_type, ret_type):

 argstr = ''
 if arg_type:
+assert not arg_type.variants
 for memb in arg_type.members:
 if memb.optional:
 argstr += 'has_%s, ' % c_name(memb.name)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index fb579dd..c03cb78 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -59,6 +59,7 @@ def gen_event_send(name, arg_type):
  name=name)

 if arg_type and arg_type.members:
+assert not arg_type.variants
 ret += mcgen('''
 qov = qmp_output_visitor_new();
 v = qmp_output_get_visitor(qov);
-- 
2.5.0

[Qemu-devel] [PATCH v5 03/14] qapi: Make c_type() more OO-like

[Eric Blake]

QAPISchemaType.c_type() is a bit awkward: it takes two optional
boolean flags is_param and is_unboxed, and they should never both
be True.

Add a new method for each of the flags, and drop the flags from
c_type().

Most callers pass no flags; they remain unchanged.

One caller passes is_param=True; call the new .c_param_type()
instead.

One caller passes is_unboxed=True, except for simple union types.
This is actually an ugly special case that will go away soon, so
until then, we now have to call either .c_type() or the new
.c_unboxed_type().  Tolerable in the interim.

It requires slightly more Python, but is arguably easier to read.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 

---
v5: improve commit message
v4: hoist earlier in series, rework simple union hack in qapi-types,
improve docs
[no v3]
v2: no change
---
 scripts/qapi.py   | 39 ++-
 scripts/qapi-types.py |  7 +--
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6b2aa6e..b7fbdae 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -822,8 +822,18 @@ class QAPISchemaVisitor(object):


 class QAPISchemaType(QAPISchemaEntity):
-def c_type(self, is_param=False, is_unboxed=False):
-return c_name(self.name) + pointer_suffix
+# Return the C type for common use.
+# For the types we commonly box, this is a pointer type.
+def c_type(self):
+pass
+
+# Return the C type to be used in a parameter list.
+def c_param_type(self):
+return self.c_type()
+
+# Return the C type to be used where we suppress boxing.
+def c_unboxed_type(self):
+return self.c_type()

 def c_null(self):
 return 'NULL'
@@ -855,8 +865,11 @@ class QAPISchemaBuiltinType(QAPISchemaType):
 def c_name(self):
 return self.name

-def c_type(self, is_param=False, is_unboxed=False):
-if is_param and self.name == 'str':
+def c_type(self):
+return self._c_type_name
+
+def c_param_type(self):
+if self.name == 'str':
 return 'const ' + self._c_type_name
 return self._c_type_name

@@ -889,7 +902,7 @@ class QAPISchemaEnumType(QAPISchemaType):
 # See QAPISchema._make_implicit_enum_type()
 return self.name.endswith('Kind')

-def c_type(self, is_param=False, is_unboxed=False):
+def c_type(self):
 return c_name(self.name)

 def member_names(self):
@@ -921,6 +934,9 @@ class QAPISchemaArrayType(QAPISchemaType):
 def is_implicit(self):
 return True

+def c_type(self):
+return c_name(self.name) + pointer_suffix
+
 def json_type(self):
 return 'array'

@@ -985,12 +1001,14 @@ class QAPISchemaObjectType(QAPISchemaType):
 assert not self.is_implicit()
 return QAPISchemaType.c_name(self)

-def c_type(self, is_param=False, is_unboxed=False):
+def c_type(self):
 assert not self.is_implicit()
-if is_unboxed:
-return c_name(self.name)
 return c_name(self.name) + pointer_suffix

+def c_unboxed_type(self):
+assert not self.is_implicit()
+return c_name(self.name)
+
 def json_type(self):
 return 'object'

@@ -1139,6 +1157,9 @@ class QAPISchemaAlternateType(QAPISchemaType):
 for v in self.variants.variants:
 v.check_clash(self.info, seen)

+def c_type(self):
+return c_name(self.name) + pointer_suffix
+
 def json_type(self):
 return 'value'

@@ -1630,7 +1651,7 @@ def gen_params(arg_type, extra):
 sep = ', '
 if memb.optional:
 ret += 'bool has_%s, ' % c_name(memb.name)
-ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
+ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name))
 if extra:
 ret += sep + extra
 return ret
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 0306a88..f194bea 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -124,11 +124,14 @@ def gen_variants(variants):
 for var in variants.variants:
 # Ugly special case for simple union TODO get rid of it
 simple_union_type = var.simple_union_type()
-typ = simple_union_type or var.type
+if simple_union_type:
+typ = simple_union_type.c_type()
+else:
+typ = var.type.c_unboxed_type()
 ret += mcgen('''
 %(c_type)s %(c_name)s;
 ''',
- c_type=typ.c_type(is_unboxed=not simple_union_type),
+ c_type=typ,
  c_name=c_name(var.name))

 ret += mcgen('''
-- 
2.5.0

[Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Eric Blake]

Slightly rearrange the code in gen_event_send() for less generated
output, by initializing 'emit' sooner, deferring an assertion
to qdict_put_obj, and dropping a now-unused 'obj' local variable.

While at it, document a FIXME related to the potential for a
compiler naming collision - if the user ever creates a QAPI event
whose name matches 'errp' or one of our local variables (like
'emit'), we'll have to revisit how we generate functions to
avoid the problem.

|@@ -25,16 +25,13 @@ void qapi_event_send_acpi_device_ost(ACP
| {
| QDict *qmp;
| Error *err = NULL;
|-QMPEventFuncEmit emit;
|+QMPEventFuncEmit emit = qmp_event_get_func_emit();
| QmpOutputVisitor *qov;
| Visitor *v;
|-QObject *obj;
|
|-emit = qmp_event_get_func_emit();
| if (!emit) {
| return;
| }
|-
| qmp = qmp_event_build_dict("ACPI_DEVICE_OST");
|
| qov = qmp_output_visitor_new();
|@@ -53,11 +50,7 @@ out_obj:
| if (err) {
| goto out;
| }
|-
|-obj = qmp_output_get_qobject(qov);
|-g_assert(obj);
|-
|-qdict_put_obj(qmp, "data", obj);
|+qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
| emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, );
|
| out:

Signed-off-by: Eric Blake 

---
v5: new patch
---
 scripts/qapi-event.py | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index c03cb78..02c9556 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -29,13 +29,19 @@ def gen_event_send_decl(name, arg_type):


 def gen_event_send(name, arg_type):
+# FIXME: Our declaration of local variables (and of 'errp' in the
+# parameter list) can collide with exploded members of the event's
+# data type passed in as parameters.  If this collision ever hits in
+# practice, we can rename our local variables with a leading _ prefix,
+# or split the code into a wrapper function that creates a boxed
+# 'param' object then calls another to do the real work.
 ret = mcgen('''

 %(proto)s
 {
 QDict *qmp;
 Error *err = NULL;
-QMPEventFuncEmit emit;
+QMPEventFuncEmit emit = qmp_event_get_func_emit();
 ''',
 proto=gen_event_send_proto(name, arg_type))

@@ -43,16 +49,13 @@ def gen_event_send(name, arg_type):
 ret += mcgen('''
 QmpOutputVisitor *qov;
 Visitor *v;
-QObject *obj;
-
 ''')

 ret += mcgen('''
-emit = qmp_event_get_func_emit();
+
 if (!emit) {
 return;
 }
-
 qmp = qmp_event_build_dict("%(name)s");

 ''',
@@ -76,11 +79,7 @@ out_obj:
 if (err) {
 goto out;
 }
-
-obj = qmp_output_get_qobject(qov);
-g_assert(obj);
-
-qdict_put_obj(qmp, "data", obj);
+qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
 ''')

 ret += mcgen('''
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 33/42] ivshmem: Implement shm=... with a memory backend

[Marc-André Lureau]

Hi

On Wed, Mar 9, 2016 at 9:59 PM, Markus Armbruster  wrote:
> I didn't do this now, because one, I'm not aware of a system that needs
> it, and two, ivshmem is Linux-specific anyway.  ivshmem-plain could be
> made more portable, and once that's done, memory-backend-shm might
> become useful.

How is ivshmem (the "plain" part) Linux-specific? Afaik it works on bsd too.

> That leaves permissions.  You're right, the patch changes them from 0777
> to 0644.  I'm inclined to call it a bug fix.  I failed to mention it in
> my commit message, and I'll fix that.  We may want to mention it in
> release notes, too.

That's good enough for me.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 05/42] ivshmem-server: Don't overload POSIX shmem and file name

[Marc-André Lureau]

Hi

On Wed, Mar 9, 2016 at 9:14 PM, Markus Armbruster  wrote:
>> @@ -77,7 +77,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int 
>> argc, char *argv[])
>> "F"  /* foreground */
>> "p:" /* pid_file */
>> "S:" /* unix_socket_path */
>> -   "m:" /* shm_path */
>> +   "m:" /* dirname */
>
> The existing comments all name the member of args set by the option.
> There is no member dirname.

I read from help: "-m : where to create shared memory"

>
>> "M:" /* shm_path */
>> "l:" /* shm_size */
>> "n:" /* n_vectors */
>> @@ -106,13 +106,15 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int 
>> argc, char *argv[])
>>  break;
>>
>>  case 'M': /* shm_path */
>> -args->shm_path = optarg;
>> -args->use_shm_open = true;
>> -break;
>> +case 'm': /* dirname */
>> +if (args->shm_path) {
>> +fprintf(stderr, "Please specify either -m or -M.\n");
>> +ivshmem_server_help(argv[0]);
>> +exit(1);
>> +}
>>
>> -case 'm': /* shm_path */
>>  args->shm_path = optarg;
>> -args->use_shm_open = false;
>> +args->use_shm_open = c == 'M';
>
> I think I'll steal this idea :)

feel free

>
>>  break;
>>
>>  case 'l': /* shm_size */
>> @@ -207,7 +209,7 @@ main(int argc, char *argv[])
>>  .foreground = IVSHMEM_SERVER_DEFAULT_FOREGROUND,
>>  .pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
>>  .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
>> -.shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
>> +.shm_path = NULL,
>>  .use_shm_open = true,
>>  .shm_size = IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
>>  .n_vectors = IVSHMEM_SERVER_DEFAULT_N_VECTORS,
>> @@ -237,8 +239,9 @@ main(int argc, char *argv[])
>>
>>  /* init the ivshms structure */
>>  if (ivshmem_server_init(, args.unix_socket_path,
>> -args.shm_path, args.use_shm_open,
>> -args.shm_size, args.n_vectors, args.verbose) < 
>> 0) {
>> +args.shm_path ?: 
>> IVSHMEM_SERVER_DEFAULT_SHM_PATH,
>> +args.use_shm_open, args.shm_size, 
>> args.n_vectors,
>> +args.verbose) < 0) {
>>  fprintf(stderr, "cannot init server\n");
>>  goto err;
>>  }

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 28/42] ivshmem: Propagate errors through ivshmem_recv_setup()

[Marc-André Lureau]

On Wed, Mar 9, 2016 at 9:25 PM, Markus Armbruster  wrote:
> Now let me explain *why* I want the assertion.  The purpose of the
> previous patch and this one is to ensure that the realized device always
> has the shared memory mapped.  The loop does that, but it's not 100%
> obvious, so I want the assertion to document my intent, and to prevent
> screwups.


alright then, a short comment would have helped! thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] Memory on stellaris board

[Aurelio Remonda]

El 9 mar. 2016 8:25 PM, "Peter Maydell"  escribió:
>
> On 10 March 2016 at 03:56, Aurelio Remonda
>  wrote:
> >
> >
> > On Mon, Mar 7, 2016 at 8:37 PM, Peter Maydell 
> > wrote:
> >>
> >> On 8 March 2016 at 02:58, Aurelio Remonda
> >>  wrote:
> >> > Hello, sorry for taking so long, I am working on this again.
> >> > About your last response, I was looking at the struct
> >> > stellaris_board_info
> >> > ,which contains
> >> > dc0, and this entire struct is const. If we need to calculate dc0
based
> >> > on
> >> > the provided RAM size
> >> > or default value at least the dc0 field should not be const.
> >>
> >> Yes, certainly.
> >>
> >> You might also need to look at the magic hex numbers in
> >> the stellaris_board[] array -- one is labelled /* dc0 */
> >> so might be related.
> >
> > Thanks Peter!
> > I have a question about the set_memory_options function, when the m
flag is
> > present, this function takes the size value (or the default_ram_size)
and
> > perform
> > an QEMU_ALIGN_UP. Let's say you want the board default ram_size, the
default
> > dc0 value should be 0xff00 (65280) when the align is made this value
becomes
> > 0x00 (65535). This align will make the dc0 value change, so I have
to
> > make some
> > operations on ram_size so dc0 will be as exact as it should.
> >
> > Something like this:
> > ram_size |= ((ram_size-1)>>8);
> > board->dc0 |= (ram_size & 0x)<<16;
> > On stellaris_init function
> >
> > Then the sram_size and the flash_size are exposed like always. I change
the
> > magic hex numbers
> > in the stellaris_board[] array from 0x00ff007f to 0x007f as the
> > flash_size
> > will not change even with the flag. The default_ram_size is also
changed for
> > this board to
> > be 64K on lm3s6965evb_class_init func:
> >mc->default_ram_size = 0xff00;
> >
> > What do you think of this approach? Thank you!
>
> You should reject unworkable ram sizes rather than silently
> changing what the user asked for; this matches our preferred
> approach where the user asks for more RAM than the board
> can support.

With unworkable you mean RAM values over dc0 máximum? I make sure that it
does not exceed 0x before i replace the dc0 value so if it goes over
16M the program report the problem (via error_report()) and exit.

Re: [Qemu-devel] [RFC PATCH v1 05/10] cpu: Abstract CPU core type

[David Gibson]

On Wed, Mar 09, 2016 at 11:40:53AM +0100, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 15:26:27 +1100
> David Gibson  wrote:
> > On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote:
> > > On Mon, 7 Mar 2016 14:36:55 +1100
> > > David Gibson  wrote:
[snip]
> > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > all derived cores would inherit it.
> > > > > > > 
> > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > make every created cpu-core object to have threads set
> > > > > > > at instance_init() time (device_init).
> > > > > > > 
> > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > >   device_add spapr-core,core=x
> > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > but if user wishes he/she still could override global by 
> > > > > > > explicitly
> > > > > > > providing thread property at device_add time:
> > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > 
> > > > > > > wrt this series it would mean, instead of creating threads in 
> > > > > > > property
> > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > but since realize is allowed to fail it should be fine do so. 
> > > > > > >  
> > > > > > 
> > > > > > Ok that would suit us as there are two properties on which thread 
> > > > > > creation
> > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can 
> > > > > > be
> > > > > > created at core realize time, then we don't have to resort to the 
> > > > > > ugliness
> > > > > > of creating the threads from either of the property setters. I 
> > > > > > always
> > > > > > assumed that we shouldn't be creating objects from realize, but if 
> > > > > > that
> > > > > > is fine, it is good.
> > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > to create internal objects there, as far as proper cleanups are done
> > > > > for failure path.
> > > >   
> > > [...]  
> > > > I'm not clear from the above if you're also intending to move at least
> > > > the adding of the threads as child properties is supposed to go into
> > > > the base type,  
> > > I'm not sure that I've got question, could you please rephrase?  
> > 
> > So, it seems like we're agreed that moving the nr_threads property to
> > the base type is a good idea.
> > 
> > My question is, do we also move the object_property_add_child() calls
> > for each thread to the base type (possibly via a helper function or
> > method the base type provides to derived types)?
> I can't think of a reason to do so,
> why can't subtype-core.realize() do it?

It can, but I'm always suspicious of boilerplate stuff that every
subtype *has* to do in order to work properly.

> What would one gain creating callbacks and calling them from base class?

Enforcing - or at least making as easy as possible - consistency in
the child object naming.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v4 05/10] qapi: Utilize implicit struct visits

[Eric Blake]

On 03/08/2016 09:11 AM, Eric Blake wrote:

>>>
>>> -ret += gen_visit_members(arg_type.members, skiperr=dealloc)
>>
>> Unless I'm mistaken, this is the only use of skiperr.  Follow up with a
>> patch to drop the parameter and simplify?
> 
> Oh, nice.  I noticed some cleanups in patch 6/10, but missed this one as
> a reasonable improvement.
> 
> In fact, gen_visit_members() is now only used in qapi-visits.py, so
> maybe I can move it back there (it used to live there before commit
> 82ca8e469 moved it for sharing with the two files simplified here).

In fact, this also nukes the only use of c_null().  I'll have a couple
of cleanup patches in v5.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC] [tcg] Idea on refactoring target code generation loop (gen_intermediate_code)

[Peter Maydell]

On 10 March 2016 at 05:29, Lluís Vilanova  wrote:
> Richard Henderson writes:
>> Alternately... can we broach the subject of C++?  Honestly, it
>> seems we work too hard sometimes to re-implement templates and
>> classes in C.
>
> Whooo, I'd really *love* to switch to C++ just for templates and
> classes... But last time this was discussed, the idea wasn't met
> with much joy :)

I would be more interested in a proposal to move parts of QEMU
to Rust, or just about anything else except C++...

thanks
-- PMM

Re: [Qemu-devel] Memory on stellaris board

[Peter Maydell]

On 10 March 2016 at 03:56, Aurelio Remonda
 wrote:
>
>
> On Mon, Mar 7, 2016 at 8:37 PM, Peter Maydell 
> wrote:
>>
>> On 8 March 2016 at 02:58, Aurelio Remonda
>>  wrote:
>> > Hello, sorry for taking so long, I am working on this again.
>> > About your last response, I was looking at the struct
>> > stellaris_board_info
>> > ,which contains
>> > dc0, and this entire struct is const. If we need to calculate dc0 based
>> > on
>> > the provided RAM size
>> > or default value at least the dc0 field should not be const.
>>
>> Yes, certainly.
>>
>> You might also need to look at the magic hex numbers in
>> the stellaris_board[] array -- one is labelled /* dc0 */
>> so might be related.
>
> Thanks Peter!
> I have a question about the set_memory_options function, when the m flag is
> present, this function takes the size value (or the default_ram_size) and
> perform
> an QEMU_ALIGN_UP. Let's say you want the board default ram_size, the default
> dc0 value should be 0xff00 (65280) when the align is made this value becomes
> 0x00 (65535). This align will make the dc0 value change, so I have to
> make some
> operations on ram_size so dc0 will be as exact as it should.
>
> Something like this:
> ram_size |= ((ram_size-1)>>8);
> board->dc0 |= (ram_size & 0x)<<16;
> On stellaris_init function
>
> Then the sram_size and the flash_size are exposed like always. I change the
> magic hex numbers
> in the stellaris_board[] array from 0x00ff007f to 0x007f as the
> flash_size
> will not change even with the flag. The default_ram_size is also changed for
> this board to
> be 64K on lm3s6965evb_class_init func:
>mc->default_ram_size = 0xff00;
>
> What do you think of this approach? Thank you!

You should reject unworkable ram sizes rather than silently
changing what the user asked for; this matches our preferred
approach where the user asks for more RAM than the board
can support.

thanks
-- PMM

Re: [Qemu-devel] [RFC] [tcg] Idea on refactoring target code generation loop (gen_intermediate_code)

[Lluís Vilanova]

Richard Henderson writes:

> On 03/09/2016 01:16 PM, Lluís Vilanova wrote:
>> Richard Henderson writes:
>> 
>>> On 03/09/2016 09:38 AM, Lluís Vilanova wrote:
 Hi,
 
 NOTE: I won't be throwing patches anytime soon, I just want to know if 
 there's
 interest in this for the future.
 
 While adding events for tracing guest instructions, I've found that the
 per-target "gen_intermediate_code()" function is very similar but not 
 exactly
 the same for each of the targets. This makes architecture-agnostic features
 harder to maintain across targets, specially when it comes to their 
 relative
 order.
 
 So, would it be worth it if I generalized part of that code into an
 architecture-agnostic function that calls into target-specific hooks 
 wherever it
 needs extending? There are many ways to do it that we can discuss later.
>> 
>>> It's worth talking about, since I do believe it would make long-term 
>>> maintenance
>>> across the targets easier.
>> 
>>> These "target-specific hooks" probably ought not be "hooks" in the
>>> traditional sense of attaching them to CPUState.  I'd be more comfortable 
>>> with a
>>> refactoring that used include files -- maybe .h or maybe .inc.c.  If we do 
>>> the
>>> normal sort of hook, then we've got to either expose DisasContext in places 
>>> we
>>> shouldn't, or dynamically allocate it.  Neither seems particularly 
>>> appealing.
>> 
>> Great. I pondered about using QOM vs "template code", and I'm leaning towards
>> the latter:
>> 
>> * translate.c:
>> 
>> struct DisasContextArch {
>> DisasContext common;
>> };
>> 
>> // implement "hooks"
>> 
>> void gen_intermediate_code(CPUArchState *env, TranslationBlock *tb)
>> {
>> DisasContextArch dc;
>> gen_intermediate_code_template(get_cpu(env), , tb);

> Pointer down into "common" here...

>> }
>> 
>> * translate-template.h:
>> 
>> struct DisasContext { /* ... */ };
>> 
>> void gen_intermediate_code_template(CPUState *cpu, DisasContext *dc,
>> TranslationBlock *tb)
>> {
>> // init dc
>> // arch-specific init dc hook
>> 
>> // gen_icount, etc.
>> while (true) {
>> // generic processing code with calls to hooks

> ... means you have to upcast to DisasContextArch here, or in the hooks 
> themselves.

Yup, that was the intention. Hooks should then use "container_of()" to get their
part.


>> }
>> }
>> 
>> While initially simpler, the "template code" still feels a little dirty to
>> me. With QOM, you could implement a DisasContextClass that provides the
>> per-target hook pointers, which can be globally allocated once per target and
>> pointed to by the DisasContext allocated in stack.
>> 
>> I'm not sure about what you mean by exposing DisasContext in places it 
>> shouldn't
>> be, though.

> You either split DisasContextArch / DisasContext in "odd" ways, and then have 
> to
> cast back and forth, or you have to expose the whole of DisasContextArch. It's
> the latter that I considered inappropriate.

Well, moving whatever is used in the generic code into DisasContext doesn't
sound too odd as a first solution.


> Alternately... can we broach the subject of C++?  Honestly, it seems we work 
> too
> hard sometimes to re-implement templates and classes in C.

Whooo, I'd really *love* to switch to C++ just for templates and classes... But
last time this was discussed, the idea wasn't met with much joy :)


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v2] block/gluster: add support for SEEK_DATA/SEEK_HOLE

[Jeff Cody]

On Wed, Mar 09, 2016 at 07:12:41PM +0100, Niels de Vos wrote:
> On Wed, Mar 09, 2016 at 10:46:02AM -0500, Jeff Cody wrote:
> > On Wed, Mar 09, 2016 at 01:30:14PM +0100, Niels de Vos wrote:
> > > GlusterFS 3.8 contains support for SEEK_DATA and SEEK_HOLE. This makes
> > > it possible to detect sparse areas in files.
> > > 
> > > Signed-off-by: Niels de Vos 
> > > 
> > > ---
> > > Tested by compiling and running "qemu-img map gluster://..." with a
> > > build of the current master branch of glusterfs. Using a Fedora cloud
> > > image (in raw format) shows many SEEK procudure calls going back and
> > > forth over the network. The output of "qemu map" matches the output when
> > > run against the image on the local filesystem.
> > > 
> > > v2 based on feedback from Jeff Cody:
> > > - Replace compile time detection by runtime detection
> > > - Update return pointer (new argument) for .bdrv_co_get_block_status
> > > ---
> > >  block/gluster.c | 186 
> > > 
> > >  1 file changed, 186 insertions(+)
> > > 
> > > diff --git a/block/gluster.c b/block/gluster.c
> > > index 65077a0..b01ab52 100644
> > > --- a/block/gluster.c
> > > +++ b/block/gluster.c
> > > @@ -23,6 +23,7 @@ typedef struct GlusterAIOCB {
> > >  typedef struct BDRVGlusterState {
> > >  struct glfs *glfs;
> > >  struct glfs_fd *fd;
> > > +bool supports_seek_data;
> > >  } BDRVGlusterState;
> > >  
> > >  typedef struct GlusterConf {
> > > @@ -286,6 +287,33 @@ static void qemu_gluster_parse_flags(int bdrv_flags, 
> > > int *open_flags)
> > >  }
> > >  }
> > >  
> > > +/*
> > > + * Do SEEK_DATA/HOLE to detect if it is functional. In order to be 
> > > usable, it
> > > + * should return different values for the start of data and the start of 
> > > a
> > > + * hole. There are three different cases to handle:
> > > + *
> > > + *  - the same position is returned for data/hole (indicates broken 
> > > gfapi)
> > > + *  - an error is returned:
> > > + * - ENXIO only gets returned if there is valid support on 
> > > client+server
> > > + * - EINVAL is returned when gfapi or the server does not support it
> > > + */
> > > +static bool qemu_gluster_test_seek(struct glfs_fd *fd)
> > > +{
> > > +off_t start_data, start_hole;
> > > +bool supports_seek_data = false;
> > > +
> > > +start_data = glfs_lseek(fd, 0, SEEK_DATA);
> > > +if (start_data != -1) {
> > 
> > I recommend just checking if the returned value is >= 0.
> 
> Ok, I can change that.
> 
> > > +start_hole = glfs_lseek(fd, 0, SEEK_HOLE);
> > > +if (start_hole != -1)
> > 
> > Minor formatting nit: per QEMU coding standard, all conditional
> > statements require brackets.
> 
> Ah, sure, will do.
> 
> > > +supports_seek_data = !(start_data == start_hole);
> > > +} else if (errno == ENXIO) {
> > 
> > This errno check for ENXIO won't catch the case if an ENXIO error
> > occurs in the SEEK_HOLE call.
> 
> I'm not sure if I'm following. lseek(SEEK_DATA) returns -1 and sets
> errno to ENXIO when the position in the filedescriptor is EOF. In this
> test, we check from position=0, so when ENXIO is returned, we know the
> file is empty and the return value+errno has gone through the whole
> Gluster stack.
> 
> EINVAL would be an other error that is expected, in case either (a
> current) gfapi or the server do not support SEEK_DATA.
> 
> I do not think there is a need to check for ENXIO on lseek(SEEK_HOLE).


Hmm, I think you are right - I can't think of any scenario that
SEEK_HOLE should return ENXIO that SEEK_DATA would not.

As matter of fact, if we can rely on ENXIO always indicating if
SEEK_DATA is supported by gluster, then we can make the whole
detection process much simpler, like this:

static bool qemu_gluster_test_seek(struct glfs_fd *fd)
{
off_t ret, eof;
eof = glfs_lseek(fd, 0, SEEK_END);
if (eof < 0) {
/* this shouldn't occur */
return false;
}
/* this should always fail with ENXIO if SEEK_DATA is supported */
ret = glfs_lseek(fd, eof, SEEK_DATA);
return (ret < 0) && (errno == ENXIO);
}


> 
> Do you have a preference on how I should change it?
> 
> > > +supports_seek_data = true;
> > > +}
> > > +
> > > +return supports_seek_data;
> > > +}
> > > +
> > >  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> > >   int bdrv_flags, Error **errp)
> > >  {
> > > @@ -320,6 +348,8 @@ static int qemu_gluster_open(BlockDriverState *bs,  
> > > QDict *options,
> > >  ret = -errno;
> > >  }
> > >  
> > > +s->supports_seek_data = qemu_gluster_test_seek(s->fd);
> > > +
> > >  out:
> > >  qemu_opts_del(opts);
> > >  qemu_gluster_gconf_free(gconf);
> > > @@ -677,6 +707,158 @@ static int 
> > > qemu_gluster_has_zero_init(BlockDriverState *bs)
> > >  return 0;
> > >  }
> > >  
> > > +/*
> > > + * Find allocation range in @bs around offset @start.
> 

Re: [Qemu-devel] [PATCH 2/2] block/qapi: fix unbounded stack for dump_qdict

[Eric Blake]

On 03/08/2016 10:56 PM, Peter Xu wrote:
> Using heap instead of stack for better safety.
> 
> Signed-off-by: Peter Xu 
> ---
>  block/qapi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index c4c2115..b798e35 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -636,9 +636,8 @@ static void dump_qdict(fprintf_function func_fprintf, 
> void *f, int indentation,
>  for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
>  QType type = qobject_type(entry->value);
>  bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> -char key[strlen(entry->key) + 1];
> +char *key = g_malloc(strlen(entry->key) + 1);
>  int i;
> -
>  /* replace dashes with spaces in key (variable) names */
>  for (i = 0; entry->key[i]; i++) {
>  key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
> @@ -650,6 +649,7 @@ static void dump_qdict(fprintf_function func_fprintf, 
> void *f, int indentation,
>  if (!composite) {
>  func_fprintf(f, "\n");
>  }
> +g_free(key);
>  }
>  }
>  
> 

-- 
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 1/2] block/qapi: make two printf() formats literal

[Eric Blake]

On 03/08/2016 10:56 PM, Peter Xu wrote:
> Fix two places to use literal printf format when possible.
> 
> Signed-off-by: Peter Xu 
> ---
>  block/qapi.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index db2d3fb..c4c2115 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -619,9 +619,8 @@ static void dump_qlist(fprintf_function func_fprintf, 
> void *f, int indentation,
>  for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>  QType type = qobject_type(entry->value);
>  bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> -const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
> -
> -func_fprintf(f, format, indentation * 4, "", i);
> +func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
> + composite ? '\n' : ' ');

[The nerd in me wants to point out that you could avoid the ternary by
writing '"\n "[composite]', but that's too ugly to use outside of IOCCC
submissions, and I wouldn't be surprised if it (rightfully) triggers
clang warnings]

-- 
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.6 04/10] s390x/cpu: Tolerate max_cpus

[Thomas Huth]

On 09.03.2016 18:28, Cornelia Huck wrote:
> From: Matthew Rosato 
> 
> Once hotplug is enabled, interrupts may come in for CPUs
> with an address > smp_cpus.  Allocate for this and allow
> search routines to look beyond smp_cpus.
> 
> Signed-off-by: Matthew Rosato 
> Message-Id: <1457112875-5209-5-git-send-email-mjros...@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/s390x/s390-virtio.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index c501a48..57c3c88 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
...
> @@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
>  machine->cpu_model = "host";
>  }
>  
> -ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> +cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);

While you're at it, it might be better to use g_new0 here instead
(see e.g. https://patchwork.ozlabs.org/patch/517377/ for a description
why this is better).

 Thomas

Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Paolo Bonzini]

On 09/03/2016 20:59, Eric Blake wrote:
> On 03/09/2016 10:47 AM, Paolo Bonzini wrote:
>> > On 09/03/2016 18:28, Daniel P. Berrange wrote:
>>> >> From: Paolo Bonzini 
>> > 
>> > Reviewing my own patch looks weird. :)
>> > 
>>> >> On Win32 we cannot directly poll on socket handles. Instead we
>>> >> create a Win32 event object and associate the socket handle with
>>> >> the event. When the event signals readyness we then have to
>>> >> use select to determine which events are ready. Creating Win32
>>> >> events is moderately heavyweight, so we don't want todo it
>>> >> every time we create a GSource, so this associates a single
>>> >> event with a QIOChannel.
>>> >>
>>> >> Signed-off-by: Daniel P. Berrange 
>>> >> ---
> Especially when it lacks your S-o-b :)

I'm innocent! :)

https://github.com/bonzini/qemu/commit/win32-qio-watch^

Paolo



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 0/3] input: linux evdev support

[Paolo Bonzini]

On 04/03/2016 11:25, Gerd Hoffmann wrote:
>   Hi,
> 
> This patch series adds support for reading input events directly from
> linux input devices instead of getting them from the UI (gtk/spice/...).
> This is useful if you pci passthrough your vga, because you don't need
> some otherwise dummy UI just to feed input into your guest.  Chech the
> patch #1 commit message for all the details.
> 
> It's been a while I posted the patches last time.  Undusted them.
> Rebased to master.  Adapted to some QAPI changes.  Squashed in some
> bugfixes accumulated over time.  Applied some testing using my new
> intel test box.

This is nice! I have used virtio-input-host to do some pseudo-multiseat,
but it was only for Linux guests.

However, instead of adding a new -input-linux option, could you make it
a QOM object which implements UserCreatable?  Then you can add it with
something like "-object input-linux,path=/dev/input/input10" (perhaps
"input-evdev" would be more specific).  This has three advantages:

1) you get hotplug for free;

2) you don't add yet another option to vl.c (btw patch 2 and 3 are not
updating the docs);

3) it's easier to add more backends, though the only ones that come to
mind are rather silly (e.g. input-msmouse could take a chardev and parse
the serial mouse protocol).

If you cannot use QOM, even just using "-inputdev
[backend=]linux,path=/dev/input/input10" would provide (3), but QOM
seems superior at the cost of a little more boilerplate in ui/input-linux.c.

Thanks,

Paolo

Re: [Qemu-devel] [Qemu-ppc] [PATCH 72/77] ppc: A couple more dummy POWER8 Book4 regs

[Thomas Huth]

On 09.03.2016 21:04, Cédric Le Goater wrote:
> On 03/02/2016 09:30 PM, Thomas Huth wrote:
>> On 11.11.2015 01:28, Benjamin Herrenschmidt wrote:
>>> WORT and PID this time
>>>
...
>> AFAICT all patches where you define new SPRs with spr_register_kvm[_hv]
>> are also important independently of the rest of your patch series -
>> otherwise these registers are currently lost during migration since they
>> are not sync'ed with the KVM part in the kernel right now.
>>
>> So if you've got some spare time, could you maybe extract all those
>> patches that define new SPRs with spr_register_kvm[_hv] and send them as
>> a separate patch series? That could help to fix future migration issues,
>> and also would decrease the size of your really huge "Add native POWER8
>> platform" patch series a little bit!
> 
> I have been maintaining a port of Ben's patchset on the latest qemu for other 
> parts which should come after pnv is merged so I have a framework to test 
> such 
> sub-patchsets. I also have time to work on them but clearly not the expertise
> in all areas !

That would be great if you could take care of this!

> What would be nice is to identify the most obvious ones, non controversial
> that could be merged after a few iterations. I have a vague idea, the ones 
> Reviewed-by David obviously being good candidates, the definition of new SPRs 
> (even the dummy ones ?).

I really like to see the KVM SPRs patches first - since they are fixing
potential problems with migration of the _current_ KVM machines already!
And being bug fixes, maybe these patches could even be included for QEMU
2.6 already? (i.e. before the hard freeze at the end of March)

So my wish-list for a first small patch series looks like this:

5b287e66c7513209  ppc: Add macros to register hypervisor mode SPRs
34f1af75e75e7ba0  ppc: Add dummy CIABR SPR
48adf38e9cab4663  ppc: A couple more dummy POWER8 Book4 regs
730a9b4dc9414818  ppc: Add KVM numbers to some P8 SPRs

There are a couple of other patches touching the SPRs initialization,
but they are not important with regards to migration... so not sure
whether it makes sense to include them now already...

 Thomas

Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries

[Markus Armbruster]

Kevin Wolf  writes:

> Am 09.03.2016 um 14:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
>> >> On 03/08/2016 07:57 PM, Peter Xu wrote:
>> >> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
>> >> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>> >> >>> Same arguments as for PATCH 2, except here an argument on the maximum
>> >> >>> length of subqdict would probably be easier.
>> >> >>
>> >> >> Yes, these are constant string literals in all callers, including the
>> >> >> one non-test case in quorum.
>> >> >>
>> >> >> Let's simply assert a reasonable maximum for subqdict_length. The
>> >> >> minimum we need to allow with the existing callers is 9, and I expect
>> >> >> we'll never get keys longer than 16 characters.
>> >> > 
>> >> > Hi, Kevin, Markus,
>> >> > 
>> >> > The patch should be trying to do as mentioned above. To make it
>> >> > clearer, how about the following one:
>> >> > 
>> >> > diff --git a/qobject/qdict.c b/qobject/qdict.c
>> >> > index 9833bd0..dde99e0 100644
>> >> > --- a/qobject/qdict.c
>> >> > +++ b/qobject/qdict.c
>> >> > @@ -704,17 +704,16 @@ int qdict_array_entries(QDict *src, const char 
>> >> > *subqdict)
>> >> >  for (i = 0; i < INT_MAX; i++) {
>> >> >  QObject *subqobj;
>> >> >  int subqdict_entries;
>> >> > -size_t slen = 32 + subqdict_len;
>> >> > -char indexstr[slen], prefix[slen];
>> >> > +char indexstr[128], prefix[128];
>> >> >  size_t snprintf_ret;
>> >> > 
>> >> > -snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i);
>> >> > -assert(snprintf_ret < slen);
>> >> > +snprintf_ret = snprintf(indexstr, ARRAY_SIZE(indexstr), 
>> >> > "%s%u", subqdict, i);
>> >> > +assert(snprintf_ret < ARRAY_SIZE(indexstr));
>> >> 
>> >> sizeof(indexstr) works, and is a bit nicer than ARRAY_SIZE() when
>> >> dealing with char.
>> >> 
>> >> But I'm worried that this can trigger an abort() by someone hammering on
>> >> the command line.  Just because we don't expect any QMP command to
>> >> validate with a key name longer than 128 doesn't mean that we don't have
>> >> to deal with a command line with a garbage key name that long.  What's
>> >> wrong with just using g_strdup_printf() and heap-allocating the result,
>> >> avoiding snprintf() and fixed lengths altogether?
>> >
>> > I can only repeat myself, we're not dealing with user data here, but
>> > with constant literal strings. Put an assert(subqdict_len < 32); at the
>> > beginning of the function and be done with it. Any violation of it is
>> > not unexpected user input, but a caller bug.
>> 
>> The fact that the keys are literals is a non-local, not-quite-obvious
>> argument.
>> 
>> It's non-local, because in qdict.c we don't know what its users store in
>> their qdicts.
>> 
>> It's not quite obvious for the only user so far, quorum_open().  New
>> uses outside the block layer are possible, but seem unlikely; the block
>> layer is the most demanding user of QDicts.
>> 
>> The block layer's use of qdicts and QemuOpts is "interesting" enough for
>> me to call it not quite obvious.  In particular, QemuOpts can either
>> accept a fixed set of keys, or arbitrary keys.  In the latter case,
>> unknown keys should be rejected by the consumer of the QemuOpts.
>> Whether that happens before they can reach qdict_array_entries() is not
>> obvious to me.
>
> And it doesn't matter here because the variable part that determines the
> array size isn't the length of a key in the QDict, but the key name
> prefix we're looking for, i.e. the name of the QAPI array we want to
> parse. Unless you plan to introduce computed field names in QAPI, I
> can't imagine a reason why this could ever be something else than a
> constant string.

I think I see now.

>> We can rely on non-local or subtle arguments when it's worthwhile, but
>> I'm not sure it's worthwhile here.  I'd use g_strdup_printf() and call
>> it a day.
>
> I think it's unnecessary, but fine with me. I'm just trying to say that
> making it a fixed 128 byte array on the stack certainly doesn't improve
> anything.

It trades a few bytes of stack for a fixed stack frame.  A fixed stack
frame is a bit more efficient (not that it matters here), and lets us
scratch the function permanently from the list of stack fram size false
positives.  I think that's a reasonable trade.

[...]

Re: [Qemu-devel] [PATCH v2 33/42] ivshmem: Implement shm=... with a memory backend

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster  wrote:
>> ivshmem has its very own code to create and map shared memory.
>> Replace that with an implicitly created memory backend.  Reduces the
>> number of ways we create BAR 2 from three to two.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/misc/ivshmem.c | 79 
>> ---
>>  1 file changed, 23 insertions(+), 56 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 1c25621..747f9c3 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -26,6 +26,7 @@
>>  #include "migration/migration.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/event_notifier.h"
>> +#include "qom/object_interfaces.h"
>>  #include "sysemu/char.h"
>>  #include "sysemu/hostmem.h"
>>  #include "qapi/visitor.h"
>> @@ -369,31 +370,6 @@ static int check_shm_size(IVShmemState *s, int fd, 
>> Error **errp)
>>  }
>>  }
>>
>> -/* create the shared memory BAR when we are not using the server, so we can
>> - * create the BAR and map the memory immediately */
>> -static int create_shared_memory_BAR(IVShmemState *s, int fd, uint8_t attr,
>> -Error **errp)
>> -{
>> -void * ptr;
>> -
>> -ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>> -if (ptr == MAP_FAILED) {
>> -error_setg_errno(errp, errno, "Failed to mmap shared memory");
>> -return -1;
>> -}
>> -
>> -memory_region_init_ram_ptr(>ivshmem, OBJECT(s), "ivshmem.bar2",
>> -   s->ivshmem_size, ptr);
>> -qemu_set_ram_fd(s->ivshmem.ram_addr, fd);
>> -vmstate_register_ram(>ivshmem, DEVICE(s));
>> -memory_region_add_subregion(>bar, 0, >ivshmem);
>> -
>> -/* region for shared memory */
>> -pci_register_bar(PCI_DEVICE(s), 2, attr, >bar);
>> -
>> -return 0;
>> -}
>> -
>>  static void ivshmem_add_eventfd(IVShmemState *s, int posn, int i)
>>  {
>>  memory_region_add_eventfd(>ivshmem_mmio,
>> @@ -833,6 +809,23 @@ static void ivshmem_write_config(PCIDevice *pdev, 
>> uint32_t address,
>>  }
>>  }
>>
>> +static void desugar_shm(IVShmemState *s)
>> +{
>> +Object *obj;
>> +char *path;
>> +
>> +obj = object_new("memory-backend-file");
>> +path = g_strdup_printf("/dev/shm/%s", s->shmobj);
>> +object_property_set_str(obj, path, "mem-path", _abort);
>> +g_free(path);
>> +object_property_set_int(obj, s->ivshmem_size, "size", _abort);
>> +object_property_set_bool(obj, true, "share", _abort);
>> +object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
>> +  _abort);
>> +user_creatable_complete(obj, _abort);
>> +s->hostmem = MEMORY_BACKEND(obj);
>> +}
>> +
>>  static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>>  {
>>  IVShmemState *s = IVSHMEM(dev);
>> @@ -911,6 +904,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
>> **errp)
>>  attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>>  }
>>
>> +if (s->shmobj) {
>> +desugar_shm(s);
>> +}
>> +
>>  if (s->hostmem != NULL) {
>>  MemoryRegion *mr;
>>
>> @@ -921,7 +918,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
>> **errp)
>>  vmstate_register_ram(mr, DEVICE(s));
>>  memory_region_add_subregion(>bar, 0, mr);
>>  pci_register_bar(PCI_DEVICE(s), 2, attr, >bar);
>> -} else if (s->server_chr != NULL) {
>> +} else {
>>  IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
>>  s->server_chr->filename);
>>
>> @@ -948,36 +945,6 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
>> **errp)
>>  error_setg(errp, "failed to initialize interrupts");
>>  return;
>>  }
>> -} else {
>> -/* just map the file immediately, we're not using a server */
>> -int fd;
>> -
>> -IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
>> -
>> -/* try opening with O_EXCL and if it succeeds zero the memory
>> - * by truncating to 0 */
>> -if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
>> -S_IRWXU|S_IRWXG|S_IRWXO)) > 0) {
>
> Nice patch, but it's worth pointing out that qemu file_ram_alloc()
> creates file with open O_CREAT. Here you rely on the fact that
> /dev/shm is present and file inside maps to POSIX shared memory object
> names. That's a lot more restrictive than using shm_open().
> Furthermore, the permissions are not the same. The current code uses
> 777 for some reasons, while qemu file_ram_alloc() is 644. I am not
> convinced the cleanup is worth these braking changes, especially the
> restriction to Linux only.

The integrated memory backend has to go.

Yes, my desugaring to memory-backend-file assumes Linux and the
conventional mount 

Re: [Qemu-devel] Memory on stellaris board

[Aurelio Remonda]

On Mon, Mar 7, 2016 at 8:37 PM, Peter Maydell 
wrote:
>
> On 8 March 2016 at 02:58, Aurelio Remonda
>  wrote:
> > Hello, sorry for taking so long, I am working on this again.
> > About your last response, I was looking at the struct
stellaris_board_info
> > ,which contains
> > dc0, and this entire struct is const. If we need to calculate dc0 based
on
> > the provided RAM size
> > or default value at least the dc0 field should not be const.
>
> Yes, certainly.
>
> You might also need to look at the magic hex numbers in
> the stellaris_board[] array -- one is labelled /* dc0 */
> so might be related.

Thanks Peter!
I have a question about the set_memory_options function, when the m flag is
present, this function takes the size value (or the default_ram_size) and
perform
an QEMU_ALIGN_UP. Let's say you want the board default ram_size, the default
dc0 value should be 0xff00 (65280) when the align is made this value becomes
0x00 (65535). This align will make the dc0 value change, so I have to
make some
operations on ram_size so dc0 will be as exact as it should.

Something like this:
ram_size |= ((ram_size-1)>>8);
board->dc0 |= (ram_size & 0x)<<16;
On stellaris_init function

Then the sram_size and the flash_size are exposed like always. I change the
magic hex numbers
in the stellaris_board[] array from 0x00ff007f to 0x007f as the
flash_size
will not change even with the flag. The default_ram_size is also changed
for this board to
be 64K on lm3s6965evb_class_init func:
   mc->default_ram_size = 0xff00;

What do you think of this approach? Thank you!


-- 

Aurelio Remonda

Taller Technologies Argentina

Software Engineer

San Lorenzo 47, 3rd Floor, Office 5
Córdoba, Argentina
Phone: +54-351-4217888 / 4218211

Re: [Qemu-devel] [PATCH v2 28/42] ivshmem: Propagate errors through ivshmem_recv_setup()

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster  wrote:
>> This kills off the funny state described in the previous commit.
>>
>> Simplify ivshmem_io_read() accordingly, and update documentation.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  docs/specs/ivshmem-spec.txt |  20 +++
>>  hw/misc/ivshmem.c   | 123 
>> +++-
>>  qemu-doc.texi   |   9 +---
>>  3 files changed, 89 insertions(+), 63 deletions(-)
>>
>> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
>> index 0cd63ad..4c33973 100644
>> --- a/docs/specs/ivshmem-spec.txt
>> +++ b/docs/specs/ivshmem-spec.txt
>> @@ -62,11 +62,11 @@ There are two ways to use this device:
>>likely want to write a kernel driver to handle interrupts.  Requires
>>the device to be configured for interrupts, obviously.
>>
>> -If the device is configured for interrupts, BAR2 is initially invalid.
>> -It becomes safely accessible only after the ivshmem server provided
>> -the shared memory.  Guest software should wait for the IVPosition
>> -register (described below) to become non-negative before accessing
>> -BAR2.
>> +Before QEMU 2.6.0, BAR2 can initially be invalid if the device is
>> +configured for interrupts.  It becomes safely accessible only after
>> +the ivshmem server provided the shared memory.  Guest software should
>> +wait for the IVPosition register (described below) to become
>> +non-negative before accessing BAR2.
>>
>>  The device is not capable to tell guest software whether it is
>>  configured for interrupts.
>> @@ -82,7 +82,7 @@ BAR 0 contains the following registers:
>>  4 4   read/write0   Interrupt Status
>>  bit 0: peer interrupt
>>  bit 1..31: reserved
>> -8 4   read-only   0 or -1   IVPosition
>> +8 4   read-only   0 or ID   IVPosition
>> 12 4   write-only  N/A   Doorbell
>>  bit 0..15: vector
>>  bit 16..31: peer ID
>> @@ -100,12 +100,14 @@ when an interrupt request from a peer is received.  
>> Reading the
>>  register clears it.
>>
>>  IVPosition Register: if the device is not configured for interrupts,
>> -this is zero.  Else, it's -1 for a short while after reset, then
>> -changes to the device's ID (between 0 and 65535).
>> +this is zero.  Else, it is the device's ID (between 0 and 65535).
>> +
>> +Before QEMU 2.6.0, the register may read -1 for a short while after
>> +reset.
>>
>>  There is no good way for software to find out whether the device is
>>  configured for interrupts.  A positive IVPosition means interrupts,
>> -but zero could be either.  The initial -1 cannot be reliably observed.
>> +but zero could be either.
>>
>>  Doorbell Register: writing this register requests to interrupt a peer.
>>  The written value's high 16 bits are the ID of the peer to interrupt,
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 24da19e..c3327dc 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -234,12 +234,7 @@ static uint64_t ivshmem_io_read(void *opaque, hwaddr 
>> addr,
>>  break;
>>
>>  case IVPOSITION:
>> -/* return my VM ID if the memory is mapped */
>> -if (memory_region_is_mapped(>ivshmem)) {
>> -ret = s->vm_id;
>> -} else {
>> -ret = -1;
>> -}
>> +ret = s->vm_id;
>>  break;
>>
>>  default:
>> @@ -511,7 +506,8 @@ static bool fifo_update_and_get_i64(IVShmemState *s,
>>  return false;
>>  }
>>
>> -static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector)
>> +static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
>> + Error **errp)
>>  {
>>  PCIDevice *pdev = PCI_DEVICE(s);
>>  MSIMessage msg = msix_get_message(pdev, vector);
>> @@ -522,22 +518,21 @@ static int ivshmem_add_kvm_msi_virq(IVShmemState *s, 
>> int vector)
>>
>>  ret = kvm_irqchip_add_msi_route(kvm_state, msg, pdev);
>>  if (ret < 0) {
>> -error_report("ivshmem: kvm_irqchip_add_msi_route failed");
>> -return -1;
>> +error_setg(errp, "kvm_irqchip_add_msi_route failed");
>> +return;
>>  }
>>
>>  s->msi_vectors[vector].virq = ret;
>>  s->msi_vectors[vector].pdev = pdev;
>> -
>> -return 0;
>>  }
>>
>> -static void setup_interrupt(IVShmemState *s, int vector)
>> +static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
>>  {
>>  EventNotifier *n = >peers[s->vm_id].eventfds[vector];
>>  bool with_irqfd = kvm_msi_via_irqfd_enabled() &&
>>  ivshmem_has_feature(s, IVSHMEM_MSI);
>>  PCIDevice *pdev = PCI_DEVICE(s);
>> +Error *err = NULL;
>>
>>  

Re: [Qemu-devel] [PATCH v2 22/42] ivshmem: Leave INTx alone when using MSI-X

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster  wrote:
>> The ivshmem device can either use MSI-X or legacy INTx for interrupts.
>>
>> With MSI-X enabled, peer interrupt events trigger an MSI as they
>> should.  But software can still raise INTx via interrupt status and
>> mask register in BAR 0.  This is explicitly prohibited by PCI Local
>> Bus Specification Revision 3.0, section 6.8.3.3:
>>
>> While enabled for MSI or MSI-X operation, a function is prohibited
>> from using its INTx# pin (if implemented) to request service (MSI,
>> MSI-X, and INTx# are mutually exclusive).
>>
>> Fix the device model to leave INTx alone when using MSI-X.
>>
>> Document that we claim to use INTx in config space even when we don't.
>> Unlike other devices, ivshmem does *not* use INTx when configured for
>> MSI-X and MSI-X isn't enabled by software.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/misc/ivshmem.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index cfea151..fc37feb 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -126,6 +126,11 @@ static void ivshmem_update_irq(IVShmemState *s)
>>  PCIDevice *d = PCI_DEVICE(s);
>>  uint32_t isr = s->intrstatus & s->intrmask;
>>
>> +/* No INTx with msi=off, whether the guest enabled MSI-X or not */
>> +if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> +return;
>
> So you probably mean msi=on

You're right.

> with that
> Reviewed-by: Marc-André Lureau 

Thanks!

[...]

Re: [Qemu-devel] [PATCH 2/2] postcopy: Remove the x-

[Dr. David Alan Gilbert]

* Eric Blake (ebl...@redhat.com) wrote:
> On 03/09/2016 12:50 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> > 
> > Postcopy seems to have survived a cycle with only a few fixes,
> > and Jiri has the current libvirt wired up and working
> > ( https://www.redhat.com/archives/libvir-list/2016-March/msg00080.html )
> > so lets remove the experimental tag.
> 
> s/lets/let's/ (in this case, you are contracting "let us")

Thanks.

> Does the balance of the universe stay the same by moving the apostrophe
> from 1/2 into this one?  :)

Damn, you've figured out my scheme.

> 
> > 
> > Signed-off-by: Dr. David Alan Gilbert 
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -540,7 +540,7 @@
> >  # @auto-converge: If enabled, QEMU will automatically throttle down the 
> > guest
> >  #  to speed up convergence of RAM migration. (since 1.6)
> >  #
> > -# @x-postcopy-ram: Start executing on the migration target before all of 
> > RAM has
> > +# @postcopy-ram: Start executing on the migration target before all of RAM 
> > has
> >  #  been migrated, pulling the remaining pages along as needed. 
> > NOTE: If
> >  #  the migration fails during postcopy the VM will fail.  (since 
> > 2.5)
> 
> s/2.5/2.6/ - we want to advertise when the non-experimental name existed
> (compare to commit 6575ccdd dropping the x- from input-send-event).
> 
> With that tweak,
> Reviewed-by: Eric Blake 
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


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

Re: [Qemu-devel] [PATCH v2 21/42] ivshmem: Clean up MSI-X conditions

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster  wrote:
>> There are three predicates related to MSI-X:
>>
>> * ivshmem_has_feature(s, IVSHMEM_MSI) is true unless the non-MSI-X
>>   variant of the device is selected with msi=off.
>>
>> * msix_present() is true when the device has the PCI capability MSI-X.
>>   It's initially false, and becomes true during successful realize of
>>   the MSI-X variant of the device.  Thus, it's the same as
>>   ivshmem_has_feature(s, IVSHMEM_MSI) for realized devices.
>>
>> * msix_enabled() is true when msix_present() is true and guest software
>>   has enabled MSI-X.
>>
>> Code that differs between the non-MSI-X and the MSI-X variant of the
>> device needs to be guarded by ivshmem_has_feature(s, IVSHMEM_MSI) or
>> by msix_present(), except the latter works only for realized devices.
>>
>> Code that depends on whether MSI-X is in use needs to be guarded with
>> msix_enabled().
>>
>> Code review led me to two minor messes:
>>
>> * ivshmem_vector_notify() calls msix_notify() even when
>>   !msix_enabled(), unlike most other MSI-X-capable devices.  As far as
>>   I can tell, msix_notify() does nothing when !msix_enabled().  Add
>>   the guard anyway.
>>
>> * Most callers of ivshmem_use_msix() guard it with
>>   ivshmem_has_feature(s, IVSHMEM_MSI).  Not necessary, because
>>   ivshmem_use_msix() does nothing when !msix_present().  That's
>>   ivshmem's only use of msix_present(), though.  Rename
>>   ivshmem_use_msix() to ivshmem_vector_use(), replace msix_present()
>>   by ivshmem_has_feature() there, and drop the redundant guards.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/misc/ivshmem.c | 22 +-
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 7191914..cfea151 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -274,7 +274,9 @@ static void ivshmem_vector_notify(void *opaque)
>>
>>  IVSHMEM_DPRINTF("interrupt on vector %p %d\n", pdev, vector);
>>  if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> -msix_notify(pdev, vector);
>> +if (msix_enabled(pdev)) {
>> +msix_notify(pdev, vector);
>> +}
>>  } else {
>>  ivshmem_IntrStatus_write(s, 1);
>>  }
>> @@ -712,13 +714,12 @@ static void ivshmem_check_version(void *opaque, const 
>> uint8_t * buf, int size)
>>  /* Select the MSI-X vectors used by device.
>>   * ivshmem maps events to vectors statically, so
>>   * we just enable all vectors on init and after reset. */
>> -static void ivshmem_use_msix(IVShmemState * s)
>> +static void ivshmem_vector_use(IVShmemState *s)
>>  {
>>  PCIDevice *d = PCI_DEVICE(s);
>>  int i;
>>
>> -IVSHMEM_DPRINTF("%s, msix present: %d\n", __func__, msix_present(d));
>> -if (!msix_present(d)) {
>> +if (!ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>  return;
>>  }
>>
>> @@ -733,7 +734,7 @@ static void ivshmem_reset(DeviceState *d)
>>
>>  s->intrstatus = 0;
>>  s->intrmask = 0;
>> -ivshmem_use_msix(s);
>> +ivshmem_vector_use(s);
>>  }
>>
>>  static int ivshmem_setup_interrupts(IVShmemState *s)
>> @@ -747,9 +748,9 @@ static int ivshmem_setup_interrupts(IVShmemState *s)
>>  }
>>
>>  IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors);
>> -ivshmem_use_msix(s);
>>  }
>>
>> +ivshmem_vector_use(s);
>>  return 0;
>>  }
>>
>> @@ -1034,12 +1035,7 @@ static int ivshmem_pre_load(void *opaque)
>>
>>  static int ivshmem_post_load(void *opaque, int version_id)
>>  {
>> -IVShmemState *s = opaque;
>> -
>> -if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> -ivshmem_use_msix(s);
>> -}
>> -
>> +ivshmem_vector_use(opaque);
>>  return 0;
>>  }
>>
>> @@ -1067,11 +1063,11 @@ static int ivshmem_load_old(QEMUFile *f, void 
>> *opaque, int version_id)
>>
>>  if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>  msix_load(pdev, f);
>> -ivshmem_use_msix(s);
>>  } else {
>>  s->intrstatus = qemu_get_be32(f);
>>  s->intrmask = qemu_get_be32(f);
>>  }
>> +ivshmem_vector_use(s);
>>
>
> Sorry I didn't reply to your previous mail (which was slightly
> confusing due to wrong naming), yes I think calling it
> ivshmem_msix_vectors_use() inside the msix block is better (#2 from
> your reply).

I'll give it a try and see how it comes out.

> Other than that
>
> Reviewed-by: Marc-André Lureau 

Thanks!

Re: [Qemu-devel] [PATCH v2 05/42] ivshmem-server: Don't overload POSIX shmem and file name

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster  wrote:
>> Option -m NAME is interpreted as directory name if we can statfs() it
>> and its on hugetlbfs.  Else it's interpreted as POSIX shared memory
>> object name.  This is nuts.
>>
>> Always interpret -m as directory.  Create new -M for POSIX shared
>> memory.  Last of -m or -M wins.
>>
>> Signed-off-by: Markus Armbruster 
>> ---
>
> I don't see why the last should win is a good idea, see attached patch

Last one wins is pretty common behavior.  In fact, it's what this
program does for every single option with an argument.  I didn't feel
like making -m and -M special.

> for a possible solution, also changing a few comments. Feel free to
> squash it in this patch or include it in your series.

I got a few comments inline.

> Reviewed-by: Marc-André Lureau 

Thanks!

[...]
> From e8112678496fd873ceaa34b3169e516130075ed4 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= 
> Date: Tue, 8 Mar 2016 20:31:09 +0100
> Subject: [PATCH] ivshmem-server: expect either -m or -M
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Signed-off-by: Marc-André Lureau 
> ---
>  contrib/ivshmem-server/main.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
> index 2795db5..368fc67 100644
> --- a/contrib/ivshmem-server/main.c
> +++ b/contrib/ivshmem-server/main.c
> @@ -77,7 +77,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int 
> argc, char *argv[])
> "F"  /* foreground */
> "p:" /* pid_file */
> "S:" /* unix_socket_path */
> -   "m:" /* shm_path */
> +   "m:" /* dirname */

The existing comments all name the member of args set by the option.
There is no member dirname.

> "M:" /* shm_path */
> "l:" /* shm_size */
> "n:" /* n_vectors */
> @@ -106,13 +106,15 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int 
> argc, char *argv[])
>  break;
>  
>  case 'M': /* shm_path */
> -args->shm_path = optarg;
> -args->use_shm_open = true;
> -break;
> +case 'm': /* dirname */
> +if (args->shm_path) {
> +fprintf(stderr, "Please specify either -m or -M.\n");
> +ivshmem_server_help(argv[0]);
> +exit(1);
> +}
>  
> -case 'm': /* shm_path */
>  args->shm_path = optarg;
> -args->use_shm_open = false;
> +args->use_shm_open = c == 'M';

I think I'll steal this idea :)

>  break;
>  
>  case 'l': /* shm_size */
> @@ -207,7 +209,7 @@ main(int argc, char *argv[])
>  .foreground = IVSHMEM_SERVER_DEFAULT_FOREGROUND,
>  .pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
>  .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
> -.shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
> +.shm_path = NULL,
>  .use_shm_open = true,
>  .shm_size = IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
>  .n_vectors = IVSHMEM_SERVER_DEFAULT_N_VECTORS,
> @@ -237,8 +239,9 @@ main(int argc, char *argv[])
>  
>  /* init the ivshms structure */
>  if (ivshmem_server_init(, args.unix_socket_path,
> -args.shm_path, args.use_shm_open,
> -args.shm_size, args.n_vectors, args.verbose) < 
> 0) {
> +args.shm_path ?: IVSHMEM_SERVER_DEFAULT_SHM_PATH,
> +args.use_shm_open, args.shm_size, args.n_vectors,
> +args.verbose) < 0) {
>  fprintf(stderr, "cannot init server\n");
>  goto err;
>  }

Re: [Qemu-devel] [PATCH 2/2] postcopy: Remove the x-

[Eric Blake]

On 03/09/2016 12:50 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Postcopy seems to have survived a cycle with only a few fixes,
> and Jiri has the current libvirt wired up and working
> ( https://www.redhat.com/archives/libvir-list/2016-March/msg00080.html )
> so lets remove the experimental tag.

s/lets/let's/ (in this case, you are contracting "let us")

Does the balance of the universe stay the same by moving the apostrophe
from 1/2 into this one?  :)

> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---

> +++ b/qapi-schema.json
> @@ -540,7 +540,7 @@
>  # @auto-converge: If enabled, QEMU will automatically throttle down the guest
>  #  to speed up convergence of RAM migration. (since 1.6)
>  #
> -# @x-postcopy-ram: Start executing on the migration target before all of RAM 
> has
> +# @postcopy-ram: Start executing on the migration target before all of RAM 
> has
>  #  been migrated, pulling the remaining pages along as needed. NOTE: 
> If
>  #  the migration fails during postcopy the VM will fail.  (since 2.5)

s/2.5/2.6/ - we want to advertise when the non-experimental name existed
(compare to commit 6575ccdd dropping the x- from input-send-event).

With that tweak,
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature