date:20170522

Re: [Qemu-devel] [PATCH v2 3/4] target/ppc: consolidate CPU device-tree id computation in helper

2017-05-22 Thread Greg Kurz

On Tue, 23 May 2017 12:35:54 +1000
David Gibson  wrote:

> On Mon, May 22, 2017 at 10:59:50AM +0200, Greg Kurz wrote:
> > On Mon, 22 May 2017 12:04:13 +1000
> > David Gibson  wrote:
> >   
> > > On Fri, May 19, 2017 at 12:32:20PM +0200, Greg Kurz wrote:  
> > > > For historical reasons, we compute CPU device-tree ids with a 
> > > > non-trivial
> > > > logic. This patch consolidate the logic in a single helper to be used
> > > > in various places where it is currently open-coded.
> > > > 
> > > > It is okay to get rid of DIV_ROUND_UP() because we're sure that the 
> > > > number
> > > > of threads per core in the guest cannot exceed the number of threads per
> > > > core in the host.
> > > 
> > > However, your new logic still gives different answers in some cases.
> > > In particular when max_cpus is not a multiple of smp_threads.  Which
> > > is generally a bad idea, but allowed for older machine types for
> > > compatibility.   e.g. smp_threads=4, max_cpus=6 smt=8
> > > 
> > > Old logic:
> > >DIV_ROUND_UP(6 * 8, 4)
> > >  = ⌈48 / 4⌉ = 12
> > > 
> > > New logic gives: ⌊6 / 4⌋ * 8 + (6 % 4)
> > >= 1 * 8 + 2
> > >  = 10
> > >   
> > 
> > I now realize this two formulas are hardly reconcilable... this
> > probably means that this patch shouldn't try to consolidate the
> > logic in hw/ppc/spapr.c with the one in target/ppc/translate_init.c.  
> 
> Ok.
> 
> > > In any case the DIV_ROUND_UP() isn't to handle the case where guest
> > > threads-per-core is bigger than host threads-per-core, it's (IIRC) for
> > > the case where guest threads-per-core is not a factor of host
> > > threads-per-core.  Again, a bad idea, but I think allowed in some old
> > > cases.
> > >   
> > 
> > FWIW, DIV_ROUND_UP() comes from this commit:
> > 
> > f303f117fec3 spapr: ensure we have at least one XICS server  
> 
> Ah, yes, I see your point.  Hrm.  I thought even then that guest
> threads > host threads was definitely incorrect; but I'm wondering if
> the change was just because the check for guest threads > host threads
> came later during init and we didn't want to crash before we got to
> it.

AFAICR, this was the only motivation... but I hadn't realized the trickiness
of CPU id computations in ppc at the time. Otherwise I would have added
another smp_threads > kvmppc_smt_threads() sanity check instead. :-\

> 
> > but I agree that this was a bad idea...  
> 
> But yeah, looks like we'll be taking a different approach so it's kind
> of moot anyway.
> 
> >   
> > > > 
> > > > Signed-off-by: Greg Kurz 
> > > > ---
> > > >  hw/ppc/spapr.c  |6 ++
> > > >  target/ppc/cpu.h|   17 +
> > > >  target/ppc/translate_init.c |3 +--
> > > >  3 files changed, 20 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 75e298b4c6be..1bb05a9a6b07 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -981,7 +981,6 @@ static void *spapr_build_fdt(sPAPRMachineState 
> > > > *spapr,
> > > >  void *fdt;
> > > >  sPAPRPHBState *phb;
> > > >  char *buf;
> > > > -int smt = kvmppc_smt_threads();
> > > >  
> > > >  fdt = g_malloc0(FDT_MAX_SIZE);
> > > >  _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > > > @@ -1021,7 +1020,7 @@ static void *spapr_build_fdt(sPAPRMachineState 
> > > > *spapr,
> > > >  _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> > > >  
> > > >  /* /interrupt controller */
> > > > -spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, 
> > > > PHANDLE_XICP);
> > > > +spapr_dt_xics(ppc_cpu_dt_id_from_index(max_cpus), fdt, 
> > > > PHANDLE_XICP);
> > > >  
> > > >  ret = spapr_populate_memory(spapr, fdt);
> > > >  if (ret < 0) {
> > > > @@ -1977,7 +1976,6 @@ static void spapr_init_cpus(sPAPRMachineState 
> > > > *spapr)
> > > >  MachineState *machine = MACHINE(spapr);
> > > >  MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > >  char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > > > -int smt = kvmppc_smt_threads();
> > > >  const CPUArchIdList *possible_cpus;
> > > >  int boot_cores_nr = smp_cpus / smp_threads;
> > > >  int i;
> > > > @@ -2014,7 +2012,7 @@ static void spapr_init_cpus(sPAPRMachineState 
> > > > *spapr)
> > > >  sPAPRDRConnector *drc =
> > > >  spapr_dr_connector_new(OBJECT(spapr),
> > > > SPAPR_DR_CONNECTOR_TYPE_CPU,
> > > > -   (core_id / smp_threads) * smt);
> > > > +   
> > > > ppc_cpu_dt_id_from_index(core_id));
> > > >  
> > > >  qemu_register_reset(spapr_drc_reset, drc);
> > > >  }
> > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > > index 401e10e7dad8..47fe6c64698f 100644
> > > > --- a/target/ppc/cpu.h
> > > > +++ b/target/ppc/cpu.h
> > > > @@ -2529,4 +2529,21 @@ int ppc_get_

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] target/ppc: consolidate CPU device-tree id computation in helper

2017-05-22 Thread David Gibson

On Mon, May 22, 2017 at 03:13:25PM +0200, Greg Kurz wrote:
> On Mon, 22 May 2017 10:59:50 +0200
> Greg Kurz  wrote:
> 
> > On Mon, 22 May 2017 12:04:13 +1000
> > David Gibson  wrote:
> > 
> > > On Fri, May 19, 2017 at 12:32:20PM +0200, Greg Kurz wrote:  
> > > > For historical reasons, we compute CPU device-tree ids with a 
> > > > non-trivial
> > > > logic. This patch consolidate the logic in a single helper to be used
> > > > in various places where it is currently open-coded.
> > > > 
> > > > It is okay to get rid of DIV_ROUND_UP() because we're sure that the 
> > > > number
> > > > of threads per core in the guest cannot exceed the number of threads per
> > > > core in the host.
> > > 
> > > However, your new logic still gives different answers in some cases.
> > > In particular when max_cpus is not a multiple of smp_threads.  Which
> > > is generally a bad idea, but allowed for older machine types for
> > > compatibility.   e.g. smp_threads=4, max_cpus=6 smt=8
> > > 
> 
> FWIW, this topology was never supported for pseries >= 2.7 since commit
> 94a94e4c4919 ("spapr: convert boot CPUs into CPU core devices", QEMU 2.7):
> 
> qemu-system-ppc64: max_cpus (6) must be multiple of threads (4)
> 
> The same happens goes with smp_cpus.

Yes, pre-2.7 is what I meant by older machine types.

> If we care for compat with pre-2.7 machine types (ie, no support for CPU
> hotplug),

We do.. RHEL 7.3 is still 2.6 based, for one thing.

> this topology isn't valid anymore since QEMU 2.9, with these
> commits:
> 
> 0c86d0fd92aa ("pseries: Always use core objects for CPU construction") which
> causes the following error if we only set max_cpus:
> 
> qemu-system-ppc64: This machine version does not support CPU hotplug

That patch has explicit provision for allowing the last core to have a
not-full complement of threads.

> 8149e2992f78 ("pseries: Enforce homogeneous threads-per-core") which
> causes the following error if we set smp_cpus (or smp_cpus == max_cpus):
> 
> qemu-system-ppc64: invalid nr-threads 2, must be 4

This one does indeed do what you say - but that's a bug :(.  It means
migration from older versions may be broken.

> So in the end, we already enforce max_cpus and smp_cpus to be multiple
> of smp_threads for all machine types. In this case...
> 
> > > Old logic:
> > >DIV_ROUND_UP(6 * 8, 4)
> > >  = ⌈48 / 4⌉ = 12
> > > 
> > > New logic gives: ⌊6 / 4⌋ * 8 + (6 % 4)
> > >= 1 * 8 + 2
> > >  = 10
> > >   
> > 
> > I now realize this two formulas are hardly reconcilable... this
> > probably means that this patch shouldn't try to consolidate the
> > logic in hw/ppc/spapr.c with the one in target/ppc/translate_init.c.
> 
> ... both formulas are equivalent, unless I'm missing something. Or
> do we really want to re-allow the funky topologies for older
> machines ?

Want to?  Not really.  Have to for compatibility?  Yes, absolutely.

I've just sent a patch to address this.

-- 
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

[Qemu-devel] [PATCH] pseries: Restore support for total vcpus not a multiple of threads-per-core for old machine types

2017-05-22 Thread David Gibson

As of pseries-2.7 and later, we require the total number of guest vcpus to
be a multiple of the threads-per-core.  pseries-2.6 and earlier machine
types, however, are supposed to allow this for the sake of migration from
old qemu versions which allowed this.

Unfortunately, 8149e29 "pseries: Enforce homogeneous threads-per-core"
broke this by not considering the old machine type case.  This fixes it by
only applying the check when the machine type supports hotpluggable cpus.
By not-entirely-coincidence, that corresponds to the same time when we
started enforcing total threads being a multiple of threads-per-core.

Fixes: 8149e2992f7811355cc34721b79d69d1a3a667dd

Signed-off-by: David Gibson 
---
 hw/ppc/spapr.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c92d269..bcb0e18 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2863,7 +2863,13 @@ static void spapr_core_pre_plug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 goto out;
 }
 
-if (cc->nr_threads != smp_threads) {
+/*
+ * In general we should have homogeneous threads-per-core, but old
+ * (pre hotplug support) machine types allow the last core to have
+ * reduced threads as a compatibility hack for when we allowed
+ * total vcpus not a multiple of threads-per-core.
+ */
+if (mc->has_hotpluggable_cpus && (cc->nr_threads != smp_threads)) {
 error_setg(errp, "invalid nr-threads %d, must be %d",
cc->nr_threads, smp_threads);
 return;
-- 
2.9.4

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [PATCH v2 00/16] Vhost-pci for inter-VM communication

2017-05-22 Thread Jason Wang

On 2017年05月23日 13:47, Wei Wang wrote:

On 05/23/2017 10:08 AM, Jason Wang wrote:

On 2017年05月22日 19:46, Wang, Wei W wrote:

On Monday, May 22, 2017 10:28 AM, Jason Wang wrote:

On 2017年05月19日 23:33, Stefan Hajnoczi wrote:

On Fri, May 19, 2017 at 11:10:33AM +0800, Jason Wang wrote:

On 2017年05月18日 11:03, Wei Wang wrote:

On 05/17/2017 02:22 PM, Jason Wang wrote:

On 2017年05月17日 14:16, Jason Wang wrote:

On 2017年05月16日 15:12, Wei Wang wrote:

Hi:

Care to post the driver codes too?

OK. It may take some time to clean up the driver code before
post
it out. You can first have a check of the draft at the repo
here:

https://github.com/wei-w-wang/vhost-pci-driver

Best,
Wei

Interesting, looks like there's one copy on tx side. We used to
have zerocopy support for tun for VM2VM traffic. Could you please
try to compare it with your vhost-pci-net by:

We can analyze from the whole data path - from VM1's network stack
to send packets -> VM2's network stack to receive packets. The
number of copies are actually the same for both.
That's why I'm asking you to compare the performance. The only
reason

for vhost-pci is performance. You should prove it.

There is another reason for vhost-pci besides maximum performance:

vhost-pci makes it possible for end-users to run networking or
storage

appliances in compute clouds. Cloud providers do not allow end-users
to run custom vhost-user processes on the host so you need vhost-pci.

Stefan
Then it has non NFV use cases and the question goes back to the
performance
comparing between vhost-pci and zerocopy vhost_net. If it does not
perform

better, it was less interesting at least in this case.

Probably I can share what we got about vhost-pci and vhost-user:
https://github.com/wei-w-wang/vhost-pci-discussion/blob/master/vhost_pci_vs_vhost_user.pdf

Right now, I don’t have the environment to add the vhost_net test.

Thanks, the number looks good. But I have some questions:

- Is the number measured through your vhost-pci kernel driver code?

Yes, the kernel driver code.

Interesting, in the above link, "l2fwd" was used in vhost-pci testing. I
want to know more about the test configuration: If l2fwd is the one that
dpdk had, want to know how can you make it work for kernel driver.
(Maybe packet socket I think?) If not, want to know how do you configure
it (e.g through bridge or act_mirred or others). And in OVS dpdk, is
dpdk l2fwd + pmd used in the testing?

- Have you tested packet size other than 64B?

Not yet.

Better to test more since the time spent on 64B copy should be very fast.

- Is zerocopy supported in OVS-dpdk? If yes, is it enabled in your test?

zerocopy is not used in the test, but I don't think zerocopy can increase
the throughput to 2x.

I agree, but we need prove this with numbers.

Thanks

On the other side, we haven't put effort to optimize
the draft kernel driver yet.

Best,
Wei

Re: [Qemu-devel] [virtio-dev] Re: [PATCH RFC] virtio-net: enable configurable tx queue size

2017-05-22 Thread Jason Wang




On 2017年05月23日 13:15, Wei Wang wrote:

On 05/23/2017 10:04 AM, Jason Wang wrote:



On 2017年05月22日 19:52, Wei Wang wrote:

On 05/20/2017 04:42 AM, Michael S. Tsirkin wrote:

On Fri, May 19, 2017 at 10:32:19AM +0800, Wei Wang wrote:

This patch enables the virtio-net tx queue size to be configurable
between 256 (the default queue size) and 1024 by the user. The queue
size specified by the user should be power of 2.

Setting the tx queue size to be 1024 requires the guest driver to
support the VIRTIO_NET_F_MAX_CHAIN_SIZE feature.

This should be a generic ring feature, not one specific to virtio net.

OK. How about making two more changes below:

1) make the default tx queue size = 1024 (instead of 256).


As has been pointed out, you need compat the default value too in 
this case.


The driver gets the size info from the device, then would it cause any
compatibility issue if we change the default ring size to 1024 in the
vhost case? In other words, is there any software (i.e. any virtio-net 
driver)

functions based on the assumption of 256 queue size?


I don't know. But is it safe e.g we migrate from 1024 to an older qemu 
with 256 as its queue size?




For live migration, the queue size that is being used will also be 
transferred

to the destination.




We can reduce the size (to 256) if the MAX_CHAIN_SIZE feature
is not supported by the guest.
In this way, people who apply the QEMU patch can directly use the
largest queue size(1024) without adding the booting command line.

2) The vhost backend does not use writev, so I think when the vhost
backed is used, using 1024 queue size should not depend on the
MAX_CHAIN_SIZE feature.


But do we need to consider even larger queue size now?


Need Michael's feedback on this. Meanwhile, I'll get the next version of
code ready and check if larger queue size would cause any corner case.


The problem is, do we really need a new config filed for this? Or just 
introduce a flag which means "I support up to 1024 sgs" is sufficient?






Btw, I think it's better to draft a spec patch.



I think it should be easier to draft the spec patch when the code is
almost done.


Maybe both.

Thanks




Best,
Wei

Re: [Qemu-devel] [PATCH risu] ppc64: Fix patterns for rotate doubleword instructions

2017-05-22 Thread Nikunj A Dadhania

G 3  writes:

> On May 22, 2017, at 4:32 AM, qemu-devel-requ...@nongnu.org wrote:
>
> Hello I have also done some work risu. My patches add ppc32 support.  
> Well my patches were made to work with Mac OS X but they are required  
> to work with Linux. Do you think you could help port these patches to  
> Linux?

Ziviani did the ppc64 work, lets see if he can spare some time.

The patches haven't come right on the mailing list, its tedious to pull
them. Please resend them with git send-mail.

>
> ppc.risu:
> https://patchwork.kernel.org/patch/9697489/
>
> risu_ppc.c:
> https://patchwork.kernel.org/patch/9697491/
>
> risu_reginfo_ppc.c:
> https://patchwork.kernel.org/patch/9697493/
>
> risu_reginfo_ppc.h:
> https://patchwork.kernel.org/patch/9697495/
>
> risugen_ppc.pm:
> https://patchwork.kernel.org/patch/9697497/
>
> Add ppc support to configure:
> https://patchwork.kernel.org/patch/9697499/
>
> Add verbose option:
> https://patchwork.kernel.org/patch/9697501/
>
> Add end of test message:
> https://patchwork.kernel.org/patch/9697503/
>
> Add more descriptive comment for mismatch or end of test condition:
> https://patchwork.kernel.org/patch/9697505/

Regards
Nikunj

Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [PATCH v2 00/16] Vhost-pci for inter-VM communication

2017-05-22 Thread Wei Wang

On 05/23/2017 10:08 AM, Jason Wang wrote:

On 2017年05月22日 19:46, Wang, Wei W wrote:

On Monday, May 22, 2017 10:28 AM, Jason Wang wrote:

On 2017年05月19日 23:33, Stefan Hajnoczi wrote:

On Fri, May 19, 2017 at 11:10:33AM +0800, Jason Wang wrote:

On 2017年05月18日 11:03, Wei Wang wrote:

On 05/17/2017 02:22 PM, Jason Wang wrote:

On 2017年05月17日 14:16, Jason Wang wrote:

On 2017年05月16日 15:12, Wei Wang wrote:

Hi:

Care to post the driver codes too?

OK. It may take some time to clean up the driver code before post
it out. You can first have a check of the draft at the repo here:
https://github.com/wei-w-wang/vhost-pci-driver

Best,
Wei

Interesting, looks like there's one copy on tx side. We used to
have zerocopy support for tun for VM2VM traffic. Could you please
try to compare it with your vhost-pci-net by:

We can analyze from the whole data path - from VM1's network stack
to send packets -> VM2's network stack to receive packets. The
number of copies are actually the same for both.

That's why I'm asking you to compare the performance. The only reason
for vhost-pci is performance. You should prove it.

There is another reason for vhost-pci besides maximum performance:

vhost-pci makes it possible for end-users to run networking or storage
appliances in compute clouds. Cloud providers do not allow end-users
to run custom vhost-user processes on the host so you need vhost-pci.

Stefan
Then it has non NFV use cases and the question goes back to the
performance
comparing between vhost-pci and zerocopy vhost_net. If it does not
perform

better, it was less interesting at least in this case.

Probably I can share what we got about vhost-pci and vhost-user:
https://github.com/wei-w-wang/vhost-pci-discussion/blob/master/vhost_pci_vs_vhost_user.pdf

Right now, I don’t have the environment to add the vhost_net test.

Thanks, the number looks good. But I have some questions:

- Is the number measured through your vhost-pci kernel driver code?

Yes, the kernel driver code.

- Have you tested packet size other than 64B?

Not yet.

- Is zerocopy supported in OVS-dpdk? If yes, is it enabled in your test?

zerocopy is not used in the test, but I don't think zerocopy can increase
the throughput to 2x. On the other side, we haven't put effort to optimize
the draft kernel driver yet.

Best,
Wei

Re: [Qemu-devel] [PATCH] ivshmem-server: Detect and use if there is required -lrt linking

2017-05-22 Thread Markus Armbruster

Kamil Rytarowski  writes:

> On 22.05.2017 08:28, Markus Armbruster wrote:
>> Kamil Rytarowski  writes:
>> 
>>> Hello,
>>>
>>> Excuse me for delay, I missed this mail.
>> 
>> No problem.
>> 
>>> Please see in-line.
>>>
>>> On 17.05.2017 09:28, Markus Armbruster wrote:
 Kamil Rytarowski  writes:

> ivshmem-server makes use of the POSIX shared memory object interfaces.
> This library is provided on NetBSD in -lrt (POSIX Real-time Library).
> Add ./configure check if there is needed -lrt linking for shm_open()
> and if so use it. Introduce new configure generated variable LIBS_SHMLIB.
>
> This fixes build issue on NetBSD.
>
> Signed-off-by: Kamil Rytarowski 
> ---
>  Makefile  |  1 +
>  configure | 20 
>  2 files changed, 21 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 31d41a7eae..3248cb53d7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -473,6 +473,7 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) 
> $(COMMON_LDADDS)
>   $(call LINK, $^)
>  ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
>   $(call LINK, $^)
> +ivshmem-server$(EXESUF): LIBS += $(LIBS_SHMLIB)
>  
>  module_block.h: $(SRC_PATH)/scripts/modules/module_block.py 
> config-host.mak
>   $(call quiet-command,$(PYTHON) $< $@ \
> diff --git a/configure b/configure
> index 7c020c076b..50c3aee746 100755
> --- a/configure
> +++ b/configure
> @@ -179,6 +179,7 @@ audio_pt_int=""
>  audio_win_int=""
>  cc_i386=i386-pc-linux-gnu-gcc
>  libs_qga=""
> +libs_shmlib=""
>  debug_info="yes"
>  stack_protector=""
>  
> @@ -4133,6 +4134,24 @@ elif compile_prog "" "$pthread_lib -lrt" ; then
>libs_qga="$libs_qga -lrt"
>  fi
>  
> +##
> +# Do we need librt for shm_open()
> +cat > $TMPC < +#include 
> +#include 
> +#include 
> +#include 
> +int main(void) {
> +  return shm_open(NULL, O_RDWR, 0644);
> +}
> +EOF
> +
> +if compile_prog "" "" ; then
> +  :
> +elif compile_prog "" "-lrt" ; then
> +  libs_shmlib="$libs_shmlib -lrt"
> +fi
> +
>  if test "$darwin" != "yes" -a "$mingw32" != "yes" -a "$solaris" != yes 
> -a \
>  "$aix" != "yes" -a "$haiku" != "yes" ; then
>  libs_softmmu="-lutil $libs_softmmu"
> @@ -5949,6 +5968,7 @@ echo "EXESUF=$EXESUF" >> $config_host_mak
>  echo "DSOSUF=$DSOSUF" >> $config_host_mak
>  echo "LDFLAGS_SHARED=$LDFLAGS_SHARED" >> $config_host_mak
>  echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
> +echo "LIBS_SHMLIB+=$libs_shmlib" >> $config_host_mak
>  echo "TASN1_LIBS=$tasn1_libs" >> $config_host_mak
>  echo "TASN1_CFLAGS=$tasn1_cflags" >> $config_host_mak
>  echo "POD2MAN=$POD2MAN" >> $config_host_mak

 We already have a test for -lrt.
>>>
>>> Correct.
>>>
  It looks for timer_create() and
 clock_gettime().
>>>
>>>
>>> timer_create(2) and clock_settime(2) are in libc on NetBSD.
>>>
  If we need -lrt,
>>>
>>> We need it just for shm_open(3).
>>>
 we add it to LIBS and to LIBS_QGA.
 The latter because we don't use LIBS for qemu-ga:

 qemu-ga$(EXESUF): LIBS = $(LIBS_QGA)

 This patch adds a second test for -lrt, to look for shm_open().  It adds
 -lrt to new variable LIBS_SHMLIB, which gets used only for
 ivshmem-server:

 ivshmem-server$(EXESUF): LIBS += $(LIBS_SHMLIB)

 Note that ivshmem-server already uses LIBS.

 Shouldn't we instead widen the existing test for -lrt to cover
 shm_open()?

>>>
>>> I will prepare patch in the requested way. I don't have preference.
>>>
 Can you confirm that the existing test does not add -lrt to LIBS on
 NetBSD?

>>>
>>> config-host.mak:LIBS+=-lm -L/usr/pkg/lib -lgthread-2.0 -pthread
>>> -Wl,-R/usr/pkg/lib -lglib-2.0 -lintl   -lz
>>> config-host.mak:LIBS_QGA+=-lm -L/usr/pkg/lib -lgthread-2.0 -pthread
>>> -Wl,-R/usr/pkg/lib -lglib-2.0 -lintl
>>>
 tests/ivshmem-test.c also calls shm_open().  Does it work on NetBSD?

>>>
>>> Currently it's disabled, as it requires eventfd() (Linux API).
>> 
>> You're right.
>> 
>> So is the ivshmem device.  Hmm.  Why would anyone want ivshmem-server on
>> NetBSD?  Its purpose is connecting ivshmem-doorbell devices.  Could we
>> simply add the obvious CONFIG_IVSHMEM dependence to
>> contrib/ivshmem-*/Makefile.objs?
>> 
>
> In general I would like to get feature parity on NetBSD, there is no
> reason to blacklist this feature for !Linux hosts. However short term

Feature parity is a worthy goal, but compiling ivshmem-server without
ivshmem-doorbell doesn't gets you a feature, it gets you a binary you
can't use for its intended purpose :)

> there are higher priorities, to fix build of pristine sources
> (ivshmem-server is blocking here, not that I'm right now working on i

Re: [Qemu-devel] [virtio-dev] Re: [PATCH RFC] virtio-net: enable configurable tx queue size

2017-05-22 Thread Wei Wang


On 05/23/2017 10:04 AM, Jason Wang wrote:



On 2017年05月22日 19:52, Wei Wang wrote:

On 05/20/2017 04:42 AM, Michael S. Tsirkin wrote:

On Fri, May 19, 2017 at 10:32:19AM +0800, Wei Wang wrote:

This patch enables the virtio-net tx queue size to be configurable
between 256 (the default queue size) and 1024 by the user. The queue
size specified by the user should be power of 2.

Setting the tx queue size to be 1024 requires the guest driver to
support the VIRTIO_NET_F_MAX_CHAIN_SIZE feature.

This should be a generic ring feature, not one specific to virtio net.

OK. How about making two more changes below:

1) make the default tx queue size = 1024 (instead of 256).


As has been pointed out, you need compat the default value too in this 
case.


The driver gets the size info from the device, then would it cause any
compatibility issue if we change the default ring size to 1024 in the
vhost case? In other words, is there any software (i.e. any virtio-net 
driver)

functions based on the assumption of 256 queue size?

For live migration, the queue size that is being used will also be 
transferred

to the destination.




We can reduce the size (to 256) if the MAX_CHAIN_SIZE feature
is not supported by the guest.
In this way, people who apply the QEMU patch can directly use the
largest queue size(1024) without adding the booting command line.

2) The vhost backend does not use writev, so I think when the vhost
backed is used, using 1024 queue size should not depend on the
MAX_CHAIN_SIZE feature.


But do we need to consider even larger queue size now?


Need Michael's feedback on this. Meanwhile, I'll get the next version of
code ready and check if larger queue size would cause any corner case.



Btw, I think it's better to draft a spec patch.



I think it should be easier to draft the spec patch when the code is
almost done.


Best,
Wei

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v2 4/4] spapr: Fix migration of Radix guests

2017-05-22 Thread Bharata B Rao

On Mon, May 22, 2017 at 04:30:50PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2017-05-19 at 11:10 +0530, Bharata B Rao wrote:
> > Fix migration of radix guests by ensuring that we issue
> > KVM_PPC_CONFIGURE_V3_MMU for radix case post migration.
> > 
> > Reported-by: Nageswara R Sastry 
> > Signed-off-by: Bharata B Rao 
> > ---
> >  hw/ppc/spapr.c | 12 
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index daf335c..8f20f14 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1400,6 +1400,18 @@ static int spapr_post_load(void *opaque, int
> > version_id)
> >  err = spapr_rtc_import_offset(&spapr->rtc, spapr-
> > >rtc_offset);
> >  }
> 
> This will break migration for tcg radix guests.
> 
> Given that there is essentially nothing special we need to do on
> incoming tcg migration, I suggest we make it:
> 
> if (spapr->patb_entry && kvm_enabled()) {
> [snip]
> }
> 
> >  
> > +if (spapr->patb_entry) {
> > +PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > +if (kvmppc_has_cap_mmu_radix() && kvm_enabled()) {
> 
> Why not make this a bit more generic? Something like:
> 
> err = kvmppc_configure_v3_mmu(cpu, !!(spapr->patb_entry & PATBE1_GR),
> !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE), spapr->patb_entry);
> if (err) {
> error_report("Process table config unsupported by the host");
> return -EINVAL;
> }
> 
> return err;
> 
> While I don't think it's possible currently, there is nothing
> inherently incorrect about having a non-zero process table entry for a
> hash guest. And this saves a future update.

How about having this logic in spapr_post_load() ?

if (spapr->patb_entry) {
/* Can be Hash(in future?) or Radix guest (current) */
PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
bool radix = !!(spapr->patb_entry & PATBE1_GR);
bool gtse = !!(cpu->env.spr[SPR_LPCR] & LPCR_GTSE);

if (radix) {
/* Radix guest, configure MMU */
/* kvmppc_configure_v3_mmu() is NOP for TCG */
err = kvmppc_configure_v3_mmu(cpu, radix, gtse, spapr->patb_entry);
if (err) {
error_report("Process table config unsupported by the host");
return -EINVAL;
}
} else {
/* Can be Hash guest (in future ?), nothing to do */
}
} else {
/* Hash guest (current), nothing to do */
}

Regards,
Bharata.

Re: [Qemu-devel] [PATCH] migration: keep bytes_xfer_prev init'd to zero

2017-05-22 Thread Peter Xu

On Fri, May 19, 2017 at 10:59:02PM +0100, Felipe Franciosi wrote:
> The first time migration_bitmap_sync() is called, bytes_xfer_prev is set
> to ram_state.bytes_transferred which is, at this point, zero. The next
> time migration_bitmap_sync() is called, an iteration has happened and
> bytes_xfer_prev is set to 'x' bytes. Most likely, more than one second
> has passed, so the auto converge logic will be triggered and
> bytes_xfer_now will also be set to 'x' bytes.
> 
> This condition is currently masked by dirty_rate_high_cnt, which will
> wait for a few iterations before throttling. It would otherwise always
> assume zero bytes have been copied and therefore throttle the guest
> (possibly) prematurely.
> 
> Given bytes_xfer_prev is only used by the auto convergence logic, it
> makes sense to only set its value after a check has been made against
> bytes_xfer_now.
> 
> Signed-off-by: Felipe Franciosi 
> ~
> ---
>  migration/ram.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f59fdd4..793af39 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -670,10 +670,6 @@ static void migration_bitmap_sync(RAMState *rs)
>  
>  rs->bitmap_sync_count++;
>  
> -if (!rs->bytes_xfer_prev) {
> -rs->bytes_xfer_prev = ram_bytes_transferred();
> -}
> -
>  if (!rs->time_last_bitmap_sync) {
>  rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>  }
> -- 
> 1.9.5
> 
> 

I feel like this patch wants to correctly initialize bytes_xfer_prev,
however I still see problem. E.g., when user specify auto-convergence
during migration, and in the first iteration we'll always have a very
small bytes_xfer_prev (with this patch, it'll be zero) with a very big
bytes_xfer_now (which is the ram_bytes_transferred() value). If so, do
you think squash below change together with current one would be more
accurate?

diff --git a/migration/ram.c b/migration/ram.c
index f59fdd4..e01a218 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -703,7 +703,8 @@ static void migration_bitmap_sync(RAMState *rs)
throttling */
 bytes_xfer_now = ram_bytes_transferred();
 
-if (rs->dirty_pages_rate &&
+/* Skip first iteration when bytes_xfer_prev not inited */
+if (rs->bytes_xfer_prev && rs->dirty_pages_rate &&
(rs->num_dirty_pages_period * TARGET_PAGE_SIZE >
(bytes_xfer_now - rs->bytes_xfer_prev) / 2) &&
(rs->dirty_rate_high_cnt++ >= 2)) {

Even I am not sure whether we can move bytes_xfer_prev out of RAMState
since it's only used by migration_bitmap_sync().

Thanks,

-- 
Peter Xu

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

2017-05-22 Thread David Gibson

On Mon, May 22, 2017 at 03:40:39PM -0300, Daniel Henrique Barboza wrote:
> From: Jianjun Duan 
> 
> In racing situations between hotplug events and migration operation,
> a rtas hotplug event could have not yet be delivered to the source
> guest when migration is started. In this case the pending_events of
> spapr state need be transmitted to the target so that the hotplug
> event can be finished on the target.
> 
> All the different fields of the events are encoded as defined by
> PAPR. We can migrate them as a binary stream inside VBUFFER without
> any concerns about data padding or endianess.
> 
> pending_events is put in a subsection in the spapr state VMSD to make
> sure migration across different versions is not broken.
> 
> Signed-off-by: Jianjun Duan 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  hw/ppc/spapr.c | 32 
>  hw/ppc/spapr_events.c  | 19 +++
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 75e298b..558f951 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1453,6 +1453,37 @@ static bool version_before_3(void *opaque, int 
> version_id)
>  return version_id < 3;
>  }
>  
> +static bool spapr_pending_events_needed(void *opaque)
> +{
> +sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +return !QTAILQ_EMPTY(&spapr->pending_events);
> +}
> +
> +static const VMStateDescription vmstate_spapr_event_entry = {
> +.name = "spapr_event_log_entry",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_INT32(log_type, sPAPREventLogEntry),
> +VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> +VMSTATE_VBUFFER_ALLOC_UINT32(data, sPAPREventLogEntry, 0,
> + NULL, data_size),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> +static const VMStateDescription vmstate_spapr_pending_events = {
> +.name = "spapr_pending_events",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = spapr_pending_events_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
> + vmstate_spapr_event_entry, sPAPREventLogEntry, 
> next),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
>  static bool spapr_ov5_cas_needed(void *opaque)
>  {
>  sPAPRMachineState *spapr = opaque;
> @@ -1551,6 +1582,7 @@ static const VMStateDescription vmstate_spapr = {
>  .subsections = (const VMStateDescription*[]) {
>  &vmstate_spapr_ov5_cas,
>  &vmstate_spapr_patb_entry,
> +&vmstate_spapr_pending_events,
>  NULL
>  }
>  };
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 73e2a18..6c42041 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -346,10 +346,29 @@ static void rtas_event_log_queue(int log_type, void 
> *data)
>  {
>  sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
> +struct epow_log_full *new_epow = NULL;
> +struct hp_log_full *new_hp = NULL;
> +uint32_t ext_length = 0;
>  
>  g_assert(data);
>  entry->log_type = log_type;
>  entry->data = data;
> +
> +switch (log_type) {
> +case RTAS_LOG_TYPE_EPOW:
> +new_epow = (struct epow_log_full *)data;
> +ext_length = be32_to_cpu(new_epow->hdr.extended_length);
> +entry->data_size = ext_length + sizeof(new_epow->hdr);
> +break;
> +case RTAS_LOG_TYPE_HOTPLUG:
> +new_hp = (struct hp_log_full *)data;
> +ext_length = be32_to_cpu(new_hp->hdr.extended_length);
> +entry->data_size = ext_length + sizeof(new_hp->hdr);
> +break;
> +default:
> +g_assert(false);
> +}
> +

You're still overcomplicating this.  Both epow_log_full and
hp_log_full start with an rtas_error_log header, which is what
contains the extended_length field.  You can just case data directly
to struct rtas_error_log *, and derive the data size from there.

And.. come to think of it, you don't need the log_type in the vmsd
either, since it can be derived from the summary field in the same
header.

And.. going even further, we could alter the existing code so that
instead of embedding the rtas_error_log header in the allocated
buffer, we could inline it into the sPAPREventLogEntry structure, with
just the extended data in the variable sized buffer.  Then we wouldn't
need a new data_size field, the extended_size field from the
rtas_error_log substructure would already be exactly what we need to
size the buffer.


>  QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index be2b3b8..b2fcd62 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -598,8 +598,9 @@ struct sP

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

2017-05-22 Thread Philippe Mathieu-Daudé


Hi Hervé,

On 05/22/2017 06:12 PM, Hervé Poussineau wrote:

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


It also says "Typically this is some indication of what system formatted
the volume."

From https://technet.microsoft.com/en-us/library/cc976796.aspx:

"Following the jump instruction is the 8-byte OEM ID, a string of 
characters that identifies the name and version number of the operating 
system that formatted the volume. To preserve compatibility with MS-DOS, 
Windows 2000 records "MSDOS5.0" in this field on FAT16 and FAT32 disks. 
On NTFS disks, Windows 2000 records "NTFS."
You may also see the OEM ID "MSWIN4.0" on disks formatted by Windows 95 
and "MSWIN4.1" on disks formatted by Windows 95 OSR2 and Windows 98. 
Windows 2000 does not use the OEM ID field in the boot sector except for 
verifying NTFS volumes."


I'm curious which one is the most up-to-date, the specification v1.03 or 
a Windows 2000. Do you have an idea if there is more MSDOS/W2K vs W95/98 
users? Hopefully W95 is a Long time no see to me.


You think having "QEMU" OEM does more harm than good?



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

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



Regards,

Phil.

[Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling

2017-05-22 Thread Mao Zhongyi

Memory allocation functions like world_alloc, desc_ring_alloc etc,
they are all wrappers around g_malloc, g_new etc. But g_malloc and 
similar functions doesn't return null. Because they ignore the fact 
that g_malloc() of 0 bytes returns null. So error checks for these
allocation failure are superfluous. Now, remove them entirely.

Signed-off-by: Mao Zhongyi 
---
 hw/net/rocker/rocker.c| 46 ---
 hw/net/rocker/rocker_desc.c   | 10 --
 hw/net/rocker/rocker_fp.c |  4 
 hw/net/rocker/rocker_of_dpa.c | 20 ---
 hw/net/rocker/rocker_world.c  | 12 +--
 5 files changed, 5 insertions(+), 87 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 6e70fdd..d01ba9d 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -239,10 +239,6 @@ static int tx_consume(Rocker *r, DescInfo *info)
 }
 iov[iovcnt].iov_len = frag_len;
 iov[iovcnt].iov_base = g_malloc(frag_len);
-if (!iov[iovcnt].iov_base) {
-err = -ROCKER_ENOMEM;
-goto err_no_mem;
-}
 
 if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base,
  iov[iovcnt].iov_len)) {
@@ -262,7 +258,6 @@ static int tx_consume(Rocker *r, DescInfo *info)
 
 err_too_many_frags:
 err_bad_io:
-err_no_mem:
 err_bad_attr:
 for (i = 0; i < ROCKER_TX_FRAGS_MAX; i++) {
 g_free(iov[i].iov_base);
@@ -674,10 +669,6 @@ int rx_produce(World *world, uint32_t pport,
  */
 
 data = g_malloc(data_size);
-if (!data) {
-err = -ROCKER_ENOMEM;
-goto out;
-}
 iov_to_buf(iov, iovcnt, 0, data, data_size);
 pci_dma_write(dev, frag_addr, data, data_size);
 g_free(data);
@@ -722,11 +713,6 @@ static void rocker_test_dma_ctrl(Rocker *r, uint32_t val)
 
 buf = g_malloc(r->test_dma_size);
 
-if (!buf) {
-DPRINTF("test dma buffer alloc failed");
-return;
-}
-
 switch (val) {
 case ROCKER_TEST_DMA_CTRL_CLEAR:
 memset(buf, 0, r->test_dma_size);
@@ -1313,13 +1299,6 @@ static int pci_rocker_init(PCIDevice *dev)
 
 r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
 
-for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
-if (!r->worlds[i]) {
-err = -ENOMEM;
-goto err_world_alloc;
-}
-}
-
 if (!r->world_name) {
 r->world_name = 
g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
 }
@@ -1396,9 +1375,6 @@ static int pci_rocker_init(PCIDevice *dev)
 }
 
 r->rings = g_new(DescRing *, rocker_pci_ring_count(r));
-if (!r->rings) {
-goto err_rings_alloc;
-}
 
 /* Rings are ordered like this:
  * - command ring
@@ -1410,14 +1386,9 @@ static int pci_rocker_init(PCIDevice *dev)
  * .
  */
 
-err = -ENOMEM;
 for (i = 0; i < rocker_pci_ring_count(r); i++) {
 DescRing *ring = desc_ring_alloc(r, i);
 
-if (!ring) {
-goto err_ring_alloc;
-}
-
 if (i == ROCKER_RING_CMD) {
 desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
 } else if (i == ROCKER_RING_EVENT) {
@@ -1437,10 +1408,6 @@ static int pci_rocker_init(PCIDevice *dev)
 fp_port_alloc(r, r->name, &r->fp_start_macaddr,
   i, &r->fp_ports_peers[i]);
 
-if (!port) {
-goto err_port_alloc;
-}
-
 r->fp_port[i] = port;
 fp_port_set_world(port, r->world_dflt);
 }
@@ -1449,25 +1416,12 @@ static int pci_rocker_init(PCIDevice *dev)
 
 return 0;
 
-err_port_alloc:
-for (--i; i >= 0; i--) {
-FpPort *port = r->fp_port[i];
-fp_port_free(port);
-}
-i = rocker_pci_ring_count(r);
-err_ring_alloc:
-for (--i; i >= 0; i--) {
-desc_ring_free(r->rings[i]);
-}
-g_free(r->rings);
-err_rings_alloc:
 err_duplicate:
 rocker_msix_uninit(r);
 err_msix_init:
 object_unparent(OBJECT(&r->msix_bar));
 object_unparent(OBJECT(&r->mmio));
 err_world_type_by_name:
-err_world_alloc:
 for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
 if (r->worlds[i]) {
 world_free(r->worlds[i]);
diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c
index ac02797..d0df89a 100644
--- a/hw/net/rocker/rocker_desc.c
+++ b/hw/net/rocker/rocker_desc.c
@@ -65,10 +65,6 @@ char *desc_get_buf(DescInfo *info, bool read_only)
 info->buf_size = size;
 }
 
-if (!info->buf) {
-return NULL;
-}
-
 if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, size)) {
 return NULL;
 }
@@ -144,9 +140,6 @@ bool desc_ring_set_size(DescRing *ring, uint32_t size)
 ring->head = ring->tail = 0;
 
 ring->info = g_renew(DescInfo, ring->info, size);
-if (!ring->info) {
-return false;
-}
 
 memset(ring->info, 0, size * sizeof(DescInfo));
 
@@ -347,9 +340,6 @@ DescRing *desc_ring_alloc(Rocker *r, i

[Qemu-devel] [PATCH v5 4/4] net/rocker: Fix the unusual macro name

2017-05-22 Thread Mao Zhongyi

Suggested-by: Markus Armbruster 
Signed-off-by: Mao Zhongyi 
---
 hw/net/rocker/rocker.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 2764529..f8a32f7 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -69,10 +69,10 @@ struct rocker {
 QLIST_ENTRY(rocker) next;
 };
 
-#define ROCKER "rocker"
+#define TYPE_ROCKER "rocker"
 
-#define to_rocker(obj) \
-OBJECT_CHECK(Rocker, (obj), ROCKER)
+#define ROCKER(obj) \
+OBJECT_CHECK(Rocker, (obj), TYPE_ROCKER)
 
 static QLIST_HEAD(, rocker) rockers;
 
@@ -1287,7 +1287,7 @@ static World *rocker_world_type_by_name(Rocker *r, const 
char *name)
 
 static void pci_rocker_realize(PCIDevice *dev, Error **errp)
 {
-Rocker *r = to_rocker(dev);
+Rocker *r = ROCKER(dev);
 const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
 const MACAddr dflt = { .a = { 0x52, 0x54, 0x00, 0x12, 0x35, 0x01 } };
 static int sw_index;
@@ -1333,7 +1333,7 @@ static void pci_rocker_realize(PCIDevice *dev, Error 
**errp)
 /* validate switch properties */
 
 if (!r->name) {
-r->name = g_strdup(ROCKER);
+r->name = g_strdup(TYPE_ROCKER);
 }
 
 if (rocker_find(r->name)) {
@@ -1429,7 +1429,7 @@ err_world_type_by_name:
 
 static void pci_rocker_uninit(PCIDevice *dev)
 {
-Rocker *r = to_rocker(dev);
+Rocker *r = ROCKER(dev);
 int i;
 
 QLIST_REMOVE(r, next);
@@ -1462,7 +1462,7 @@ static void pci_rocker_uninit(PCIDevice *dev)
 
 static void rocker_reset(DeviceState *dev)
 {
-Rocker *r = to_rocker(dev);
+Rocker *r = ROCKER(dev);
 int i;
 
 for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
@@ -1500,7 +1500,7 @@ static Property rocker_properties[] = {
 };
 
 static const VMStateDescription rocker_vmsd = {
-.name = ROCKER,
+.name = TYPE_ROCKER,
 .unmigratable = 1,
 };
 
@@ -1523,7 +1523,7 @@ static void rocker_class_init(ObjectClass *klass, void 
*data)
 }
 
 static const TypeInfo rocker_info = {
-.name  = ROCKER,
+.name  = TYPE_ROCKER,
 .parent= TYPE_PCI_DEVICE,
 .instance_size = sizeof(Rocker),
 .class_init= rocker_class_init,
-- 
2.9.3

[Qemu-devel] [PATCH v5 2/4] net/rocker: Plug memory leak in pci_rocker_init()

2017-05-22 Thread Mao Zhongyi

pci_rocker_init() leaks a World when the name more than 9 chars,
then return a negative value directly, doesn't make a correct
cleanup. So add a new goto label to fix it.

Signed-off-by: Mao Zhongyi 
Reviewed-by: Markus Armbruster 
---
 hw/net/rocker/rocker.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index d01ba9d..6d44f37 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1357,7 +1357,8 @@ static int pci_rocker_init(PCIDevice *dev)
 fprintf(stderr,
 "rocker: name too long; please shorten to at most %d chars\n",
 MAX_ROCKER_NAME_LEN);
-return -EINVAL;
+err = -EINVAL;
+goto err_name_too_long;
 }
 
 if (memcmp(&r->fp_start_macaddr, &zero, sizeof(zero)) == 0) {
@@ -1416,6 +1417,7 @@ static int pci_rocker_init(PCIDevice *dev)
 
 return 0;
 
+err_name_too_long:
 err_duplicate:
 rocker_msix_uninit(r);
 err_msix_init:
-- 
2.9.3

[Qemu-devel] [PATCH v5 3/4] net/rocker: Convert to realize()

2017-05-22 Thread Mao Zhongyi

The rocker device still implements the old PCIDeviceClass .init()
instead of the new .realize(). All devices need to be converted to
.realize().

.init() reports errors with fprintf() and return 0 on success, negative
number on failure. Meanwhile, when -device rocker fails, it first report
a specific error, then a generic one, like this:

$ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
rocker: name too long; please shorten to at most 9 chars
qemu-system-x86_64: -device rocker,name=qemu-rocker: Device initialization 
failed

Now, convert it to .realize() that passes errors to its callers via its
errp argument. Also avoid the superfluous second error message. After
the patch, effect like this:

$ x86_64-softmmu/qemu-system-x86_64 -device rocker,name=qemu-rocker
qemu-system-x86_64: -device rocker,name=qemu-rocker: name too long; please 
shorten to at most 9 chars

Signed-off-by: Mao Zhongyi 
Reviewed-by: Markus Armbruster 
---
 hw/net/rocker/rocker.c | 27 +++
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 6d44f37..2764529 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1238,20 +1238,18 @@ rollback:
 return err;
 }
 
-static int rocker_msix_init(Rocker *r)
+static int rocker_msix_init(Rocker *r, Error **errp)
 {
 PCIDevice *dev = PCI_DEVICE(r);
 int err;
-Error *local_err = NULL;
 
 err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
 &r->msix_bar,
 ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
 &r->msix_bar,
 ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
-0, &local_err);
+0, errp);
 if (err) {
-error_report_err(local_err);
 return err;
 }
 
@@ -1287,7 +1285,7 @@ static World *rocker_world_type_by_name(Rocker *r, const 
char *name)
 return NULL;
 }
 
-static int pci_rocker_init(PCIDevice *dev)
+static void pci_rocker_realize(PCIDevice *dev, Error **errp)
 {
 Rocker *r = to_rocker(dev);
 const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
@@ -1305,10 +1303,9 @@ static int pci_rocker_init(PCIDevice *dev)
 
 r->world_dflt = rocker_world_type_by_name(r, r->world_name);
 if (!r->world_dflt) {
-fprintf(stderr,
-"rocker: requested world \"%s\" does not exist\n",
+error_setg(errp,
+"invalid argument requested world %s does not exist",
 r->world_name);
-err = -EINVAL;
 goto err_world_type_by_name;
 }
 
@@ -1328,7 +1325,7 @@ static int pci_rocker_init(PCIDevice *dev)
 
 /* MSI-X init */
 
-err = rocker_msix_init(r);
+err = rocker_msix_init(r, errp);
 if (err) {
 goto err_msix_init;
 }
@@ -1340,7 +1337,7 @@ static int pci_rocker_init(PCIDevice *dev)
 }
 
 if (rocker_find(r->name)) {
-err = -EEXIST;
+error_setg(errp, "%s already exists", r->name);
 goto err_duplicate;
 }
 
@@ -1354,10 +1351,9 @@ static int pci_rocker_init(PCIDevice *dev)
 #define ROCKER_IFNAMSIZ 16
 #define MAX_ROCKER_NAME_LEN  (ROCKER_IFNAMSIZ - 1 - 3 - 3)
 if (strlen(r->name) > MAX_ROCKER_NAME_LEN) {
-fprintf(stderr,
-"rocker: name too long; please shorten to at most %d chars\n",
+error_setg(errp,
+"name too long; please shorten to at most %d chars",
 MAX_ROCKER_NAME_LEN);
-err = -EINVAL;
 goto err_name_too_long;
 }
 
@@ -1415,7 +1411,7 @@ static int pci_rocker_init(PCIDevice *dev)
 
 QLIST_INSERT_HEAD(&rockers, r, next);
 
-return 0;
+return;
 
 err_name_too_long:
 err_duplicate:
@@ -1429,7 +1425,6 @@ err_world_type_by_name:
 world_free(r->worlds[i]);
 }
 }
-return err;
 }
 
 static void pci_rocker_uninit(PCIDevice *dev)
@@ -1514,7 +1509,7 @@ static void rocker_class_init(ObjectClass *klass, void 
*data)
 DeviceClass *dc = DEVICE_CLASS(klass);
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-k->init = pci_rocker_init;
+k->realize = pci_rocker_realize;
 k->exit = pci_rocker_uninit;
 k->vendor_id = PCI_VENDOR_ID_REDHAT;
 k->device_id = PCI_DEVICE_ID_REDHAT_ROCKER;
-- 
2.9.3

[Qemu-devel] [PATCH v5 0/4] Convert to realize and fix error handling

2017-05-22 Thread Mao Zhongyi

v5:
* Patch 1 removed the dead error handling that was previously missing.
* Patch 2 and 3 has not changed.
* Patch 4 is a new patch to fix the unusual macro name.

v4:
* Patch 1 is following Markus's suggestion that remove the dead error
  handling.
* Patch 2 is separate from patch 1 to plug the memory leak in the v3.
* Patch 3 is based on the patch 1 in the v3. Meanwhile, dorp the superfluous
  prefix "rocker:" and adjust the commit message.

v3:
* Following Jason's suggstion that add suitable error message to 
  each error site.
* Modified the commit message to make it easier to read.

v2:
* Following Philippe's suggestion that shorten the patch subject 
  "hw/net/rocker/rocker" to "net/rocker". 
* Use a consistent log format to report error message.
* Add a specific goto label "err_name_too_long" to make a correct
  cleanup.

Mao Zhongyi (4):
  net/rocker: Remove the dead error handling
  net/rocker: Plug memory leak in pci_rocker_init()
  net/rocker: Convert to realize()
  net/rocker: Fix the unusual macro name

 hw/net/rocker/rocker.c| 93 ++-
 hw/net/rocker/rocker_desc.c   | 10 -
 hw/net/rocker/rocker_fp.c |  4 --
 hw/net/rocker/rocker_of_dpa.c | 20 --
 hw/net/rocker/rocker_world.c  | 12 +++---
 5 files changed, 27 insertions(+), 112 deletions(-)

-- 
2.9.3

1 2 3 >

1 - 100 of 256 matches

Mail list logo