Re: [Qemu-devel] [PATCH v4 0/7] Block Throttle Group Support

[Alberto Garcia]

On Fri, Mar 27, 2015 at 09:11:19AM +0800, Fam Zheng wrote:

> > Hmm... this actually breaks the new test because now BDS are
> > required to have an AioContext attached when they're added to a
> > group, which doesn't happen in qtest:
> > 
> > bdrv = bdrv_new();
> > throttle_group_register_bs(bdrv, "bar");
> > 
> > I'm unsure of how to proceed with this, I don't see a clear way to
> > attach the AioContext here (unless I do bdrv->aio_context = ctx).
> > Suggestions are welcome.
> 
> Use bdrv_attach_aio_context?

That doesn't work in that case, bdrv_attach_aio_context() does nothing
if bdrv->drv is null.

The test works fine if I set bdrv->aio_context directly as I said
above, but is it ok to do that?

Berto

[Qemu-devel] Use of QERR_ macros and error classes (was: [RFC PATCH COLO v2 06/13] NBD client: implement block driver interfaces for block replication)

[Markus Armbruster]

Fam Zheng  writes:

> On Thu, 03/26 15:32, Wen Congyang wrote:
>> On 03/26/2015 03:21 PM, Fam Zheng wrote:
>> > On Wed, 03/25 17:36, Wen Congyang wrote:
>> >> Signed-off-by: Wen Congyang 
>> >> Signed-off-by: zhanghailiang 
>> >> Signed-off-by: Gonglei 
>> >> ---
>> >>  block/nbd.c | 49 +
>> >>  1 file changed, 49 insertions(+)
>> >>
>> >> diff --git a/block/nbd.c b/block/nbd.c
>> >> index 3faf865..753b322 100644
>> >> --- a/block/nbd.c
>> >> +++ b/block/nbd.c
>> >> @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState 
>> >> *bs)
>> >>  bs->full_open_options = opts;
>> >>  }
>> >>  
>> >> +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode,
>> >> +  Error **errp)
>> >> +{
>> >> +BDRVNBDState *s = bs->opaque;
>> >> +
>> >> +/*
>> >> + * TODO: support COLO_SECONDARY_MODE if we allow secondary
>> >> + * QEMU becoming primary QEMU.
>> >> + */
>> >> +if (mode != COLO_MODE_PRIMARY) {
>> >> +error_set(errp, QERR_INVALID_PARAMETER, "mode");
>> > 
>> > Please use error_setg. (Please grep the whole series :)
>> 
>> Why? QERR_INVALID_PARAMETER includes ERROR_CLASS_GENERIC_ERROR.
>
> Because error classes are deprecated. See also commit 5b347c5410.

Yes, new code should use ERROR_CLASS_GENERIC_ERROR, preferably through
error_setg() or similar.

The QERR_ macros in qerror.h expand to *two* expressions CLS, FMT, for
use as function arguments.  Dirty.  Was done that way when error classes
were introduced to reduce churn (commit 8546505..0f32cf6).

Besides being dirty, they hide the error class.  Bad, because we really
want unusual things like a funky error class stand out right where the
error is created.  If they don't, we're prone to accidental uses of
funky error classes creeping in.

So far, we've eliminated all QERR_ macros using a funky error class but
one: QERR_DEVICE_NOT_FOUND.  That one needs to go, too.  I've got
patches in my queue for 2.4.

In fact, all of qerror.h needs to go.  I've got patches for 2.4 to clean
it out except for a bunch of QERR_.  Replacing these isn't hard, just
prone to conflicts, so I'm saving it for a little later.

For now, adding uses of QERR_ macros other than QERR_DEVICE_NOT_FOUND is
still okay, it just creates a little extra work for me.  But avoiding
them makes their ultimate removal a bit easier, and is encouraged.

Re: [Qemu-devel] [PATCH v5 16/28] qapi: Better error messages for duplicated expressions

[Markus Armbruster]

One more...

Eric Blake  writes:

[...]
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 90eb3bc..5d0dc91 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
[...]
> @@ -560,12 +585,22 @@ def type_name(name):
>  return c_list_type(name[0])
>  return name
>
> -enum_types = []
> -struct_types = []
> -union_types = []
> +def add_name(name, info, meta, implicit = False):
> +global all_names
> +if name in all_names:
> +raise QAPIExprError(info,
> +"%s '%s' is already defined"
> +%(all_names[name], name))

We say "struct 'Foo'", and expect the user to know that 'struct' means
'complex type'.  It'll do, it's just a development tool.

I'm not really happy with 'complex type', though.  Isn't a union type
complex, too?  Anyway, we can clean up our confused terminology later;
this series is long enough.

> +if not implicit and name[-4:] == 'Kind':
> +raise QAPIExprError(info,
> +"%s '%s' should not end in 'Kind'"
> +%(meta, name))
> +all_names[name] = meta
>
> -def add_struct(definition):
> +def add_struct(definition, info):
>  global struct_types
> +name = definition['type']
> +add_name(name, info, 'struct')
>  struct_types.append(definition)
>
>  def find_struct(name):
[...]

[Qemu-devel] Need some helps

[Qiang Guan]

Hi,

I am working on the old version QEMU which is still using dyngen with my
own constraints. I want to know how I can get the guest instruction to
intermediate code mapping. Are there any docs?

For example:

For this guest instruction
add [ebp + 0x08], edx

It will be split into multiple micro-ops:
movl A0 ebp
addl A0 0x0c
addl A0 ss.base
movl T1 edx
ldl_raw T0 A0
addl T0 T1
stl_raw T0 A0
update2_cc

My question is how I can  get the guest instruction to micron-ops mapping.
If I have a "cmpl" guest instruction, how can I find the mapping micro-ops
in intermediate code?


Another question is that when I run 32bits Guest VM, I can only allocate up
to 2 GB not 4 GB. Are there any special reasons for this design?

Thanks,

- Qiang

Re: [Qemu-devel] [PATCH v5 20/28] qapi: More rigourous checking of types

[Markus Armbruster]

Eric Blake  writes:

> Now that we know every expression is valid with regards to
> its keys, we can add further tests that those keys refer to
> valid types.  With this patch, all uses of a type (the 'data':
> of command, type, union, alternate, and event; the 'returns':
> of command; the 'base': of type and union) must resolve to an
> appropriate subset of metatypes  declared by the current qapi
> parse; this includes recursing into each member of a data
> dictionary.  Dealing with '**' and nested anonymous structs
> will be done in later patches.
>
> Update the testsuite to match improved output.
>
> Signed-off-by: Eric Blake 
> ---
[...]
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6ed6a34..c42683b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -276,6 +276,63 @@ def discriminator_find_enum_define(expr):
>
>  return find_enum(discriminator_type)
>
> +def check_type(expr_info, source, data, allow_array = False,
> +   allowed_metas = [], allow_dict = True):

I'd allow_dict = False here, because I like defaulting to false.  Matter
of taste.

I'd name the third parameter def rather than data.  Matter of taste
again.

> +global all_names
> +
> +if data is None:
> +return
> +
> +if data == '**':
> +return
> +
> +# Check if array type for data is okay
> +if isinstance(data, list):
> +if not allow_array:
> +raise QAPIExprError(expr_info,
> +"%s cannot be an array" % source)
> +if len(data) != 1 or not isinstance(data[0], str):
> +raise QAPIExprError(expr_info,
> +"%s: array type must contain single type 
> name"
> +% source)
> +data = data[0]
> +
> +# Check if type name for data is okay
> +if isinstance(data, str):
> +if not data in all_names:
> +raise QAPIExprError(expr_info,
> +"%s uses unknown type '%s'"
> +% (source, data))
> +if not all_names[data] in allowed_metas:
> +raise QAPIExprError(expr_info,
> +"%s cannot use %s type '%s'"
> +% (source, all_names[data], data))
> +return
> +
> +# data is a dictionary, check that each member is okay
> +if not isinstance(data, OrderedDict):
> +raise QAPIExprError(expr_info,
> +"%s should be a dictionary" % source)
> +if not allow_dict:
> +raise QAPIExprError(expr_info,
> +"%s should be a type name" % source)
> +for (key, value) in data.items():
> +check_type(expr_info, "Member '%s' of %s" % (key, source), value,

This can produce messages like

Member 'inner' of Member 'outer' of ...

I figure the problem will go away when you ditch nested structs.  Not
worth worrying about it then.

> +   allow_array=True,
> +   allowed_metas=['built-in', 'union', 'alternate', 'struct',
> +  'enum'],
> +   allow_dict=True)
> +
> +def check_command(expr, expr_info):
> +name = expr['command']
> +check_type(expr_info, "'data' for command '%s'" % name,
> +   expr.get('data'),
> +   allowed_metas=['union', 'struct'])
> +check_type(expr_info, "'returns' for command '%s'" % name,
> +   expr.get('returns'), allow_array=True,
> +   allowed_metas=['built-in', 'union', 'alternate', 'struct',
> +  'enum'])
> +
>  def check_event(expr, expr_info):
>  global events
>  name = expr['event']
> @@ -287,7 +344,8 @@ def check_event(expr, expr_info):
>  raise QAPIExprError(expr_info, "Event name '%s' should be upper case"
>  % name)
>  events.append(name)
> -
> +check_type(expr_info, "'data' for event '%s'" % name,
> +   expr.get('data'), allowed_metas=['union', 'struct'])
>  if params:
>  for argname, argentry, optional, structured in parse_args(params):
>  if structured:
> @@ -348,6 +406,13 @@ def check_union(expr, expr_info):
>
>  # Check every branch
>  for (key, value) in members.items():
> +# Each value must name a known type
> +check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
> +   value, allow_array=True,
> +   allowed_metas=['built-in', 'union', 'alternate', 'struct',
> +  'enum'],
> +   allow_dict=False)
> +
>  # If the discriminator names an enum type, then all members
>  # of 'data' must also be members of the enum type.
>  if enum_define:
> @@ -383,15 +448,12 @@ def check_alternate(expr, expr_info):
>  values[c_key] = key
>
>  # Ensure alternates have no type conflicts.
> -if isin

Re: [Qemu-devel] [PATCH v4 0/7] Block Throttle Group Support

[Fam Zheng]

On Fri, 03/27 08:23, Alberto Garcia wrote:
> On Fri, Mar 27, 2015 at 09:11:19AM +0800, Fam Zheng wrote:
> 
> > > Hmm... this actually breaks the new test because now BDS are
> > > required to have an AioContext attached when they're added to a
> > > group, which doesn't happen in qtest:
> > > 
> > > bdrv = bdrv_new();
> > > throttle_group_register_bs(bdrv, "bar");
> > > 
> > > I'm unsure of how to proceed with this, I don't see a clear way to
> > > attach the AioContext here (unless I do bdrv->aio_context = ctx).
> > > Suggestions are welcome.
> > 
> > Use bdrv_attach_aio_context?
> 
> That doesn't work in that case, bdrv_attach_aio_context() does nothing
> if bdrv->drv is null.
> 
> The test works fine if I set bdrv->aio_context directly as I said
> above, but is it ok to do that?

I think it should be OK as a test.

Or assert(qtest_enabled()) in throttle_group_register_bs and assign the main
loop AioContext, which is basically the same.

Fam

Re: [Qemu-devel] [PATCH v5 21/28] qapi: Require valid names

[Markus Armbruster]

Eric Blake  writes:

> Previous commits demonstrated that the generator overlooked various
> bad naming situations:
> - types, commands, and events need a valid name
> - union and alternate branches cannot be marked optional
>
> The set of valid names includes [a-zA-Z0-9._-] (where '.' is
> useful only in downstream extensions).
>
> Signed-off-by: Eric Blake 
> ---
>  scripts/qapi.py| 57 
> --
>  tests/qapi-schema/bad-ident.err|  1 +
>  tests/qapi-schema/bad-ident.exit   |  2 +-
>  tests/qapi-schema/bad-ident.json   |  2 +-
>  tests/qapi-schema/bad-ident.out|  3 --
>  tests/qapi-schema/flat-union-bad-discriminator.err |  2 +-
>  .../flat-union-optional-discriminator.err  |  1 +
>  .../flat-union-optional-discriminator.exit |  2 +-
>  .../flat-union-optional-discriminator.json |  2 +-
>  .../flat-union-optional-discriminator.out  |  5 --
>  tests/qapi-schema/union-optional-branch.err|  1 +
>  tests/qapi-schema/union-optional-branch.exit   |  2 +-
>  tests/qapi-schema/union-optional-branch.json   |  2 +-
>  tests/qapi-schema/union-optional-branch.out|  3 --
>  14 files changed, 53 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c42683b..ed5385a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -15,6 +15,7 @@ import re
>  from ordereddict import OrderedDict
>  import os
>  import sys
> +import string
>
>  builtin_types = {
>  'str':  'QTYPE_QSTRING',
> @@ -276,8 +277,27 @@ def discriminator_find_enum_define(expr):
>
>  return find_enum(discriminator_type)
>
> +valid_characters = set(string.ascii_letters + string.digits + '.' + '-' + 
> '_')

strings.ascii_letters depends on the locale...

> +def check_name(expr_info, source, name, allow_optional = False):
> +membername = name
> +
> +if not isinstance(name, str):
> +raise QAPIExprError(expr_info,
> +"%s requires a string name" % source)
> +if name == '**':
> +return

Doesn't this permit '**' anywhere, not just as pseudo-type in command
arguments and results?

> +if name.startswith('*'):
> +membername = name[1:]
> +if not allow_optional:
> +raise QAPIExprError(expr_info,
> +"%s does not allow optional name '%s'"
> +% (source, name))
> +if not set(membername) <= valid_characters:

... so this check would break if we called locale.setlocale() in this
program.  While I don't think we need to worry about it, I think you
could just as well use something like

valid_name = re.compile(r"^[A-Za-z0-9-._]+$")

if not valid_name.match(membername):

> +raise QAPIExprError(expr_info,
> +"%s uses invalid name '%s'" % (source, name))
> +
>  def check_type(expr_info, source, data, allow_array = False,
> -   allowed_metas = [], allow_dict = True):
> +   allowed_metas = [], allow_dict = True, allow_optional = 
> False):
>  global all_names
>
>  if data is None:
> @@ -317,21 +337,23 @@ def check_type(expr_info, source, data, allow_array = 
> False,
>  raise QAPIExprError(expr_info,
>  "%s should be a type name" % source)
>  for (key, value) in data.items():
> +check_name(expr_info, "Member of %s" % source, key,
> +   allow_optional=allow_optional)
>  check_type(expr_info, "Member '%s' of %s" % (key, source), value,
> allow_array=True,
> allowed_metas=['built-in', 'union', 'alternate', 'struct',
>'enum'],
> -   allow_dict=True)
> +   allow_dict=True, allow_optional=True)
>
>  def check_command(expr, expr_info):
>  name = expr['command']
>  check_type(expr_info, "'data' for command '%s'" % name,
> expr.get('data'),
> -   allowed_metas=['union', 'struct'])
> +   allowed_metas=['union', 'struct'], allow_optional=True)
>  check_type(expr_info, "'returns' for command '%s'" % name,
> expr.get('returns'), allow_array=True,
> allowed_metas=['built-in', 'union', 'alternate', 'struct',
> -  'enum'])
> +  'enum'], allow_optional=True)
>
>  def check_event(expr, expr_info):
>  global events
> @@ -345,7 +367,8 @@ def check_event(expr, expr_info):
>  % name)
>  events.append(name)
>  check_type(expr_info, "'data' for event '%s'" % name,
> -   expr.get('data'), allowed_metas=['union', 'struct'])
> +   expr.get('data'), allowed_metas=['union', 'struct'],
> +   allow_optional=True)
>  if params:
>  for argname, argentry, optio

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Stefan Hajnoczi]

On Thu, Mar 26, 2015 at 11:29:43AM +0100, Juan Quintela wrote:
> Wen Congyang  wrote:
> > On 03/25/2015 05:50 PM, Juan Quintela wrote:
> >> zhanghailiang  wrote:
> >>> Hi all,
> >>>
> >>> We found that, sometimes, the content of VM's memory is
> >>> inconsistent between Source side and Destination side
> >>> when we check it just after finishing migration but before VM continue to 
> >>> Run.
> >>>
> >>> We use a patch like bellow to find this issue, you can find it from affix,
> >>> and Steps to reprduce:
> >>>
> >>> (1) Compile QEMU:
> >>>  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && make
> >>>
> >>> (2) Command and output:
> >>> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
> >>> qemu64,-kvmclock -netdev tap,id=hn0-device
> >>> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
> >>> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
> >>> -device
> >>> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
> >>> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
> >>> -monitor stdio
> >> 
> >> Could you try to reproduce:
> >> - without vhost
> >> - without virtio-net
> >> - cache=unsafe is going to give you trouble, but trouble should only
> >>   happen after migration of pages have finished.
> >
> > If I use ide disk, it doesn't happen.
> > Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
> > it is because I migrate the guest when it is booting. The virtio net
> > device is not used in this case.
> 
> Kevin, Stefan, Michael, any great idea?

You must use -drive cache=none if you want to use live migration.  It
should not directly affect memory during migration though.

> >>> We have done further test and found that some pages has been
> >>> dirtied but its corresponding migration_bitmap is not set.
> >>> We can't figure out which modules of QEMU has missed setting bitmap
> >>> when dirty page of VM,
> >>> it is very difficult for us to trace all the actions of dirtying VM's 
> >>> pages.
> >> 
> >> This seems to point to a bug in one of the devices.

I think you'll need to track down which pages are different.  If you are
lucky, their contents will reveal what the page is used for.

Stefan


pgpAf0g_2nzeA.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.3 1/4] virtio-ccw: fix range check for SET_VQ

[Christian Borntraeger]

Am 26.03.2015 um 16:35 schrieb Cornelia Huck:
> VIRTIO_PCI_QUEUE_MAX is already too big; a malicious guest would be
> able to trigger a write beyond the VirtQueue structure.
> 
> Cc: qemu-sta...@nongnu.org
> Reviewed-by: David Hildenbrand 
> Signed-off-by: Cornelia Huck 
Acked-by: Christian Borntraeger 

> ---
>  hw/s390x/virtio-ccw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 130535c..ceb6a45 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -266,7 +266,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t 
> addr, uint32_t align,
>  {
>  VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
> 
> -if (index > VIRTIO_PCI_QUEUE_MAX) {
> +if (index >= VIRTIO_PCI_QUEUE_MAX) {
>  return -EINVAL;
>  }
> 

Re: [Qemu-devel] [PATCH for-2.3 2/4] virtio-ccw: range check in READ_VQ_CONF

[Christian Borntraeger]

Am 26.03.2015 um 16:36 schrieb Cornelia Huck:
> Processing for READ_VQ_CONF needs to check whether the requested queue
> value is actually in the supported range and post a channel program
> check if not.
> 
> Cc: qemu-sta...@nongnu.org
> Reviewed-by: David Hildenbrand 
> Signed-off-by: Cornelia Huck 
Acked-by: Christian Borntraeger 

> ---
>  hw/s390x/virtio-ccw.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index ceb6a45..d32ecaf 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -549,6 +549,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>  ret = -EFAULT;
>  } else {
>  vq_config.index = lduw_be_phys(&address_space_memory, ccw.cda);
> +if (vq_config.index >= VIRTIO_PCI_QUEUE_MAX) {
> +ret = -EINVAL;
> +break;
> +}
>  vq_config.num_max = virtio_queue_get_num(vdev,
>   vq_config.index);
>  stw_be_phys(&address_space_memory,
> 

[Qemu-devel] block-commit & dropping privs

[Michael Tokarev]

Hello.

I tried to experiment with block-commit command, which propagates
changes accumulated in an overlay (qcow2) block image file back to
the base image file.

And immediately faced a problem.  All my VMs are run chrooted into
an empty dir and with low-priv user (using -runsa and -chroot options,
initially started as root).  Ofcourse this low-priv qemu process
can't open the base image anymore, because it doesn't have the
necessary permissions and because the base file is inaccessible
within the chroot.

So I wonder if we can avoid reopening the base img by always opening
it read-write (using a command-line option), does it make sense?

Or maybe there's some other possible solution to this, for example,
passing in a filedescriptor for the new base img over a unix socket?

Thanks,

/mjt

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Wen Congyang]

On 03/27/2015 04:56 PM, Stefan Hajnoczi wrote:
> On Thu, Mar 26, 2015 at 11:29:43AM +0100, Juan Quintela wrote:
>> Wen Congyang  wrote:
>>> On 03/25/2015 05:50 PM, Juan Quintela wrote:
 zhanghailiang  wrote:
> Hi all,
>
> We found that, sometimes, the content of VM's memory is
> inconsistent between Source side and Destination side
> when we check it just after finishing migration but before VM continue to 
> Run.
>
> We use a patch like bellow to find this issue, you can find it from affix,
> and Steps to reprduce:
>
> (1) Compile QEMU:
>  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && make
>
> (2) Command and output:
> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
> qemu64,-kvmclock -netdev tap,id=hn0-device
> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
> -device
> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
> -monitor stdio

 Could you try to reproduce:
 - without vhost
 - without virtio-net
 - cache=unsafe is going to give you trouble, but trouble should only
   happen after migration of pages have finished.
>>>
>>> If I use ide disk, it doesn't happen.
>>> Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
>>> it is because I migrate the guest when it is booting. The virtio net
>>> device is not used in this case.
>>
>> Kevin, Stefan, Michael, any great idea?
> 
> You must use -drive cache=none if you want to use live migration.  It
> should not directly affect memory during migration though.

Otherwise, what will happen? If the user doesn't use cache=none, and
tries to use live migration, qemu doesn't output any message or trigger
an event to notify the user.

Thanks
Wen Congyang

> 
> We have done further test and found that some pages has been
> dirtied but its corresponding migration_bitmap is not set.
> We can't figure out which modules of QEMU has missed setting bitmap
> when dirty page of VM,
> it is very difficult for us to trace all the actions of dirtying VM's 
> pages.

 This seems to point to a bug in one of the devices.
> 
> I think you'll need to track down which pages are different.  If you are
> lucky, their contents will reveal what the page is used for.
> 
> Stefan
> 

Re: [Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary

[Markus Armbruster]

Eric Blake  writes:

> ...or an array of dictionaries.  Although we have to cater to
> existing commands, returning a non-dictionary means the command
> is not extensible (no new name/value pairs can be added if more
> information must be returned in parallel).  By making the
> whitelist explicit, any new command that falls foul of this
> practice will have to be self-documenting, which will encourage
> developers to either justify the action or rework the design to
> use a dictionary after all.
>
> Signed-off-by: Eric Blake 
> ---
>  scripts/qapi.py  | 30 --
>  tests/qapi-schema/returns-alternate.err  |  1 +
>  tests/qapi-schema/returns-alternate.exit |  2 +-
>  tests/qapi-schema/returns-alternate.json |  2 +-
>  tests/qapi-schema/returns-alternate.out  |  4 
>  tests/qapi-schema/returns-int.json   |  3 ++-
>  tests/qapi-schema/returns-int.out|  2 +-
>  tests/qapi-schema/returns-whitelist.err  |  1 +
>  tests/qapi-schema/returns-whitelist.exit |  2 +-
>  tests/qapi-schema/returns-whitelist.json |  2 +-
>  tests/qapi-schema/returns-whitelist.out  |  7 ---
>  11 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index ed5385a..9421431 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -33,6 +33,30 @@ builtin_types = {
>  'size': 'QTYPE_QINT',
>  }
>
> +# Whitelist of commands allowed to return a non-dictionary
> +returns_whitelist = [
> +# From QMP:
> +'human-monitor-command',
> +'query-migrate-cache-size',
> +'query-tpm-models',
> +'query-tpm-types',
> +'ringbuf-read',
> +
> +# From QGA:
> +'guest-file-open',
> +'guest-fsfreeze-freeze',
> +'guest-fsfreeze-freeze-list',
> +'guest-fsfreeze-status',
> +'guest-fsfreeze-thaw',
> +'guest-get-time',
> +'guest-set-vcpus',
> +'guest-sync',
> +'guest-sync-delimited',
> +
> +# From qapi-schema-test:
> +'user_def_cmd3',
> +]
> +

Since there's just one whitelist, all schemata share it, and that means
it's too permissive for each of them.  Sloppy, but good enough.

If the sloppiness bothers us, here are two alternatives:

* Program takes the whitelist as argument, say

scripts/qapi-commands.py --legacy-returns qmp-legacy-returns ...

* Leave enforcing to C

  If a command 'frobnicate' returns a non-dictionary, generate something
  like

  #ifndef FROBNICATE_LEGACY_RETURN_OK
  #error Command 'frobnicate' should return a dictionary
  #endif

  Then manually define the macros necessary to keep the current use
  working in a suitable header.

>  enum_types = []
>  struct_types = []
>  union_types = []
> @@ -350,10 +374,12 @@ def check_command(expr, expr_info):
>  check_type(expr_info, "'data' for command '%s'" % name,
> expr.get('data'),
> allowed_metas=['union', 'struct'], allow_optional=True)
> +returns_meta = ['union', 'struct']
> +if name in returns_whitelist:
> +returns_meta += ['built-in', 'alternate', 'enum']
>  check_type(expr_info, "'returns' for command '%s'" % name,
> expr.get('returns'), allow_array=True,
> -   allowed_metas=['built-in', 'union', 'alternate', 'struct',
> -  'enum'], allow_optional=True)
> +   allowed_metas=returns_meta, allow_optional=True)
>
>  def check_event(expr, expr_info):
>  global events
[...]

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH 14/22] exec: only check relevant bitmaps for cleanliness

[Paolo Bonzini]

On 27/03/2015 07:10, Fam Zheng wrote:
>>  static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t 
>> start,
>> -ram_addr_t 
>> length)
>> +ram_addr_t 
>> length,
>> +uint8_t mask)
>>  {
>> -bool vga = cpu_physical_memory_get_clean(start, length, 
>> DIRTY_MEMORY_VGA);
>> -bool code = cpu_physical_memory_get_clean(start, length, 
>> DIRTY_MEMORY_CODE);
>> -bool migration =
>> -cpu_physical_memory_get_clean(start, length, 
>> DIRTY_MEMORY_MIGRATION);
>> -return vga || code || migration;
>> +bool clean = false;
>> +if (mask & (1 << DIRTY_MEMORY_VGA)) {
>> +clean = cpu_physical_memory_get_clean(start, length, 
>> DIRTY_MEMORY_VGA);
>> +}
>> +if (!clean && (mask & (1 << DIRTY_MEMORY_CODE))) {
>> +clean = cpu_physical_memory_get_clean(start, length, 
>> DIRTY_MEMORY_CODE);
>> +}
>> +if (!clean && (mask & (1 << DIRTY_MEMORY_MIGRATION))) {
>> +clean = cpu_physical_memory_get_clean(start, length, 
>> DIRTY_MEMORY_MIGRATION);
>> +}
>> +return clean;
>>  }
> 
> Out of curiosity, is it valid that a mask bit is cleared but the corresponding
> dirty bit is set?

Yes, for example if migration is cancelled you'll have some bits set in
the DIRTY_MEMORY_MIGRATION bitmap, but DIRTY_MEMORY_MIGRATION itself
will not be enabled.

Paolo

>>  
>>  static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
>> -- 
>> 2.3.3
>>
>>
>>

Re: [Qemu-devel] [PATCH 17/22] bitmap: add atomic test and clear

[Paolo Bonzini]

On 27/03/2015 07:37, Fam Zheng wrote:
> On Thu, 03/26 18:38, Paolo Bonzini wrote:
>> From: Stefan Hajnoczi 
>>
>> The new bitmap_test_and_clear_atomic() function clears a range and
>> returns whether or not the bits were set.
>>
>> Signed-off-by: Stefan Hajnoczi 
>> Message-Id: <1417519399-3166-3-git-send-email-stefa...@redhat.com>
>> [Test before xchg; then a full barrier is needed at the end just like
>>  in the previous patch.  The barrier can be avoided if we did at least
>>  one xchg.  - Paolo]
>>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  include/qemu/bitmap.h |  2 ++
>>  util/bitmap.c | 43 +++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
>> index 3e0a4f3..86dd9cd 100644
>> --- a/include/qemu/bitmap.h
>> +++ b/include/qemu/bitmap.h
>> @@ -41,6 +41,7 @@
>>   * bitmap_set(dst, pos, nbits)  Set specified bit area
>>   * bitmap_set_atomic(dst, pos, nbits)   Set specified bit area with atomic 
>> ops
>>   * bitmap_clear(dst, pos, nbits)Clear specified bit area
>> + * bitmap_test_and_clear_atomic(dst, pos, nbits)Test and clear area
>>   * bitmap_find_next_zero_area(buf, len, pos, n, mask)   Find bit free 
>> area
>>   */
>>  
>> @@ -229,6 +230,7 @@ static inline int bitmap_intersects(const unsigned long 
>> *src1,
>>  void bitmap_set(unsigned long *map, long i, long len);
>>  void bitmap_set_atomic(unsigned long *map, long i, long len);
>>  void bitmap_clear(unsigned long *map, long start, long nr);
>> +bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr);
>>  unsigned long bitmap_find_next_zero_area(unsigned long *map,
>>   unsigned long size,
>>   unsigned long start,
>> diff --git a/util/bitmap.c b/util/bitmap.c
>> index e4957da..570758a 100644
>> --- a/util/bitmap.c
>> +++ b/util/bitmap.c
>> @@ -232,6 +232,49 @@ void bitmap_clear(unsigned long *map, long start, long 
>> nr)
>>  }
>>  }
>>  
>> +bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
>> +{
>> +unsigned long *p = map + BIT_WORD(start);
>> +const long size = start + nr;
>> +int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
>> +unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
>> +unsigned long dirty = 0;
>> +unsigned long old_bits;
>> +
>> +/* First word */
>> +if (nr - bits_to_clear > 0) {
>> +old_bits = atomic_fetch_and(p, ~mask_to_clear);
>> +dirty |= old_bits & mask_to_clear;
>> +nr -= bits_to_clear;
>> +bits_to_clear = BITS_PER_LONG;
>> +mask_to_clear = ~0UL;
>> +p++;
>> +}
>> +
>> +/* Full words */
>> +while (nr - bits_to_clear >= 0) {
>> +if (*p) {
>> +old_bits = atomic_xchg(p, 0);
>> +dirty |= old_bits;
>> +}
>> +nr -= bits_to_clear;
>> +p++;
>> +}
>> +
>> +/* Last word */
>> +if (nr) {
>> +mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
>> +old_bits = atomic_fetch_and(p, ~mask_to_clear);
>> +dirty |= old_bits & mask_to_clear;
>> +} else {
>> +if (!dirty) {
>> +smp_mb();
> 
> Is this for the "*p" in the while loop?

Yes, this is why it's within "if (!dirty)".  If any bit was set, an
atomic_xchg was done and the memory barrier is not needed.

> If so, and if the while loop is not
> executed (bits contained in the first word, and clean), isn't this barrier
> superfluous then?

Yes, I can add an "if (*p)" in the first if too.

Paolo

[Qemu-devel] [PATCH v5 1/7] docs: update documentation for memory hot unplug

[Zhu Guihua]

Add specification about how to use memory hot unplug, and add
a flow diagram to explain memory hot unplug process.

Signed-off-by: Zhu Guihua 
---
 docs/memory-hotplug.txt | 24 
 docs/specs/acpi_mem_hotplug.txt | 16 +++-
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/docs/memory-hotplug.txt b/docs/memory-hotplug.txt
index f70571d..0de7f1b 100644
--- a/docs/memory-hotplug.txt
+++ b/docs/memory-hotplug.txt
@@ -1,12 +1,10 @@
 QEMU memory hotplug
 ===
 
-This document explains how to use the memory hotplug feature in QEMU,
+This part explains how to use the memory hotplug feature in QEMU,
 which is present since v2.1.0.
 
-Please, note that memory hotunplug is not supported yet. This means
-that you're able to add memory, but you're not able to remove it.
-Also, proper guest support is required for memory hotplug to work.
+And, proper guest support is required for memory hotplug to work.
 
 Basic RAM hotplug
 -
@@ -74,3 +72,21 @@ comes from regular RAM, 1GB is a 1GB hugepage page and 256MB 
is from
-device pc-dimm,id=dimm1,memdev=mem1 \
-object memory-backend-file,id=mem2,size=256M,mem-path=/mnt/hugepages-2MB \
-device pc-dimm,id=dimm2,memdev=mem2
+
+
+QEMU memory hot unplug
+==
+This part explains how to use the memory hot unplug feature in QEMU.
+
+In order to be able to hot unplug pc-dimm device, QEMU has to be told the id
+of pc-dimm device. The id was assigned when you hot plugged memory.
+
+One monitor command is used to hot unplug memory:
+
+ - "device_del": deletes a front-end pc-dimm device
+
+For example, assuming that the pc-dimm device with id "dimm1" was hotplugged
+into the guest, the following command deletes the pc-dimm device from the
+guest.
+
+  (qemu) device_del dimm1
diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index 1290994..a88b286 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -2,7 +2,7 @@ QEMU<->ACPI BIOS memory hotplug interface
 --
 
 ACPI BIOS GPE.3 handler is dedicated for notifying OS about memory hot-add
-events.
+and hot-remove events.
 
 Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
 ---
@@ -42,3 +42,17 @@ Selecting memory device slot beyond present range has no 
effect on platform:
  ignored
- read accesses to memory hot-plug registers not documented above return
  all bits set to 1.
+
+Memory hot remove process diagram:
+--
+Memory hot unplug is rather complicated multi-stage process, it is asynchronous
+procedures. The follwoing diagram describes memory hot unplug process.
+
+ QEMU | Guest OS
+---
+ unplug request cb -> |
+  |  OSPM processes ejection request
+  |  <- OSPM clears device remove event
+clear remove event -> |
+  |  <- OSPM issues _EJ0 and initiates device eject
+  unplug cb (eject device)|
-- 
1.9.3

[Qemu-devel] [PATCH v5 0/7] QEMU memory hot unplug support

[Zhu Guihua]

Memory hot unplug are both asynchronous procedures.
When the unplug operation happens, unplug request cb is called first.
And when guest OS finished handling unplug, unplug cb will be called
to do the real removal of device.

v5:
 -reorganize the patchset
 -add documentation to understand patch easily
 -add MEMORY_SLOT_EJECT for initiating device eject
 -add support to send qmp event to notify mgmt about memory unplug error

v4:
 -reorganize the patchset
 -drop the new API acpi_send_gpe_event()
 -update ssdt-mem

v3:
 -commit message changes
 -reorganize the patchset, squash and separate some patches
 -update specs about acpi_mem_hotplug
 -first cleanup external state, then un-map and un-register memory device

v2:
 -do a generic for acpi to send gpe event
 -unparent object by PC_MACHINE
 -update description in acpi_mem_hotplug.txt
 -combine the last two patches in the last version
 -cleanup external state in acpi_memory_unplug_cb

Tang Chen (3):
  acpi, mem-hotplug: add acpi_memory_slot_status() to get MemStatus
  acpi, mem-hotplug: add unplug request cb for memory device
  acpi, mem-hotplug: add unplug cb for memory device

Zhu Guihua (4):
  docs: update documentation for memory hot unplug
  acpi: extend aml_field() to support UpdateRule
  acpi: add hardware implementation for memory hot unplug
  qmp-event: add event notification for memory hot unplug error

 docs/memory-hotplug.txt   | 24 --
 docs/qmp/qmp-events.txt   | 17 +++
 docs/specs/acpi_mem_hotplug.txt   | 32 --
 hw/acpi/aml-build.c   |  4 +-
 hw/acpi/ich9.c| 19 ++--
 hw/acpi/memory_hotplug.c  | 93 ---
 hw/acpi/piix4.c   | 17 +--
 hw/core/qdev.c|  2 +-
 hw/i386/acpi-build.c  | 25 ---
 hw/i386/acpi-dsdt-mem-hotplug.dsl | 13 +-
 hw/i386/pc.c  | 62 --
 include/hw/acpi/aml-build.h   | 10 -
 include/hw/acpi/memory_hotplug.h  | 12 +
 include/hw/acpi/pc-hotplug.h  |  3 ++
 include/hw/qdev-core.h|  1 +
 monitor.c |  1 +
 qapi/event.json   | 14 ++
 trace-events  |  2 +
 18 files changed, 315 insertions(+), 36 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v5 7/7] qmp-event: add event notification for memory hot unplug error

[Zhu Guihua]

When memory hot unplug fails, this patch adds support to send
QMP event to notify mgmt about this failure.

Signed-off-by: Zhu Guihua 
---
 docs/qmp/qmp-events.txt  | 17 +
 hw/acpi/memory_hotplug.c |  8 +++-
 monitor.c|  1 +
 qapi/event.json  | 14 ++
 4 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index d759d19..7a05705 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -226,6 +226,23 @@ Example:
 { "event": "GUEST_PANICKED",
  "data": { "action": "pause" } }
 
+MEM_HOT_UNPLUG_ERROR
+
+Emitted when memory hot unplug occurs error.
+
+Data:
+
+- "device": device name (json-string)
+- "msg": Informative message (e.g., reason for the error) (json-string)
+
+Example:
+
+{ "event": "MEM_HOT_UNPLUG_ERROR"
+  "data": { "device": "dimm1",
+"msg": "acpi: device unplug for not supported device"
+  },
+  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
 NIC_RX_FILTER_CHANGED
 -
 
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 2a1b866..f1cef71 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -94,6 +94,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
 ACPIOSTInfo *info;
 DeviceState *dev = NULL;
 HotplugHandler *hotplug_ctrl = NULL;
+Error *local_err = NULL;
 
 if (!mem_st->dev_count) {
 return;
@@ -148,7 +149,12 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
 dev = DEVICE(mdev->dimm);
 hotplug_ctrl = qdev_get_hotplug_handler(dev);
 /* call pc-dimm unplug cb */
-hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
+hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
+if (local_err) {
+qapi_event_send_mem_unplug_error(dev->id,
+ error_get_pretty(local_err),
+ &error_abort);
+}
 }
 break;
 default:
diff --git a/monitor.c b/monitor.c
index 68873ec..d52ab87 100644
--- a/monitor.c
+++ b/monitor.c
@@ -587,6 +587,7 @@ static void monitor_qapi_event_init(void)
 monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
 monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
 monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
+monitor_qapi_event_throttle(QAPI_EVENT_MEM_UNPLUG_ERROR, 1000);
 
 qmp_event_set_func_emit(monitor_qapi_event_queue);
 }
diff --git a/qapi/event.json b/qapi/event.json
index c51dc49..862df67 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -330,3 +330,17 @@
 ##
 { 'event': 'VSERPORT_CHANGE',
   'data': { 'id': 'str', 'open': 'bool' } }
+
+##
+# @MEM_UNPLUG_ERROR
+#
+# Emitted when memory hot unplug occurs error.
+#
+# @device: device name
+#
+# @msg: Informative message
+#
+# Since: 2.3
+##
+{ 'event': 'MEM_UNPLUG_ERROR',
+  'data': { 'device': 'str', 'msg': 'str' } }
-- 
1.9.3

[Qemu-devel] [PATCH v5 2/7] acpi, mem-hotplug: add acpi_memory_slot_status() to get MemStatus

[Zhu Guihua]

From: Tang Chen 

Add a new API named acpi_memory_slot_status() to obtain a single memory
slot status. Doing this is because this procedure will be used by other
functions in the next coming patches.

Signed-off-by: Tang Chen 
Signed-off-by: Zhu Guihua 
---
 hw/acpi/memory_hotplug.c | 34 --
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index c6580da..6af9303 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -163,29 +163,51 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object 
*owner,
 memory_region_add_subregion(as, ACPI_MEMORY_HOTPLUG_BASE, &state->io);
 }
 
-void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
- DeviceState *dev, Error **errp)
+/**
+ * acpi_memory_slot_status:
+ * @mem_st: memory hotplug state
+ * @dev: device
+ * @errp: set in case of an error
+ *
+ * Obtain a single memory slot status.
+ *
+ * This function will be called by memory unplug request cb and unplug cb.
+ */
+static MemStatus *
+acpi_memory_slot_status(MemHotplugState *mem_st,
+DeviceState *dev, Error **errp)
 {
-MemStatus *mdev;
 Error *local_err = NULL;
 int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
&local_err);
 
 if (local_err) {
 error_propagate(errp, local_err);
-return;
+return NULL;
 }
 
 if (slot >= mem_st->dev_count) {
 char *dev_path = object_get_canonical_path(OBJECT(dev));
-error_setg(errp, "acpi_memory_plug_cb: "
+error_setg(errp, "acpi_memory_slot_status: "
"device [%s] returned invalid memory slot[%d]",
 dev_path, slot);
 g_free(dev_path);
+return NULL;
+}
+
+return &mem_st->devs[slot];
+}
+
+void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
+ DeviceState *dev, Error **errp)
+{
+MemStatus *mdev;
+
+mdev = acpi_memory_slot_status(mem_st, dev, errp);
+if (!mdev) {
 return;
 }
 
-mdev = &mem_st->devs[slot];
 mdev->dimm = dev;
 mdev->is_enabled = true;
 mdev->is_inserting = true;
-- 
1.9.3

[Qemu-devel] [PATCH v5 4/7] acpi, mem-hotplug: add unplug cb for memory device

[Zhu Guihua]

From: Tang Chen 

This patch adds unplug cb for memory device. It resets memory status
"is_enabled" in acpi_memory_unplug_cb(), removes the corresponding
memory region, unregisters vmstate, and unparents the object.

Signed-off-by: Tang Chen 
Signed-off-by: Zhu Guihua 
---
 hw/acpi/ich9.c   |  9 +++--
 hw/acpi/memory_hotplug.c | 14 ++
 hw/acpi/piix4.c  | 11 +--
 hw/i386/pc.c | 34 --
 include/hw/acpi/memory_hotplug.h |  2 ++
 5 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index b85eed4..84e5bb8 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -413,8 +413,13 @@ void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, 
DeviceState *dev,
 void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
   Error **errp)
 {
-error_setg(errp, "acpi: device unplug for not supported device"
-   " type: %s", object_get_typename(OBJECT(dev)));
+if (pm->acpi_memory_hotplug.is_enabled &&
+object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+acpi_memory_unplug_cb(&pm->acpi_memory_hotplug, dev, errp);
+} else {
+error_setg(errp, "acpi: device unplug for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
+}
 }
 
 void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 42fe668..07e281f 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -237,6 +237,20 @@ void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq 
irq,
 acpi_update_sci(ar, irq);
 }
 
+void acpi_memory_unplug_cb(MemHotplugState *mem_st,
+   DeviceState *dev, Error **errp)
+{
+MemStatus *mdev;
+
+mdev = acpi_memory_slot_status(mem_st, dev, errp);
+if (!mdev) {
+return;
+}
+
+mdev->is_enabled = false;
+mdev->dimm = NULL;
+}
+
 static const VMStateDescription vmstate_memhp_sts = {
 .name = "memory hotplug device state",
 .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f716e91..1b28481 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -377,8 +377,15 @@ static void piix4_device_unplug_request_cb(HotplugHandler 
*hotplug_dev,
 static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
 {
-error_setg(errp, "acpi: device unplug for not supported device"
-   " type: %s", object_get_typename(OBJECT(dev)));
+PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+
+if (s->acpi_memory_hotplug.is_enabled &&
+object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+acpi_memory_unplug_cb(&s->acpi_memory_hotplug, dev, errp);
+} else {
+error_setg(errp, "acpi: device unplug for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
+}
 }
 
 static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9b0859c..769eb25 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1697,6 +1697,32 @@ out:
 error_propagate(errp, local_err);
 }
 
+static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
+   DeviceState *dev, Error **errp)
+{
+PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+PCDIMMDevice *dimm = PC_DIMM(dev);
+PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+MemoryRegion *mr = ddc->get_memory_region(dimm);
+HotplugHandlerClass *hhc;
+Error *local_err = NULL;
+
+hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
+hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+
+if (local_err) {
+goto out;
+}
+
+memory_region_del_subregion(&pcms->hotplug_memory, mr);
+vmstate_unregister_ram(mr, dev);
+
+object_unparent(OBJECT(dev));
+
+ out:
+error_propagate(errp, local_err);
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
 DeviceState *dev, Error **errp)
 {
@@ -1750,8 +1776,12 @@ static void 
pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
 DeviceState *dev, Error **errp)
 {
-error_setg(errp, "acpi: device unplug for not supported device"
-   " type: %s", object_get_typename(OBJECT(dev)));
+if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+pc_dimm_unplug(hotplug_dev, dev, errp);
+} else {
+error_setg(errp, "acpi: device unplug for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
+}
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index fbc2d65..8f9eb2d 100644
--- a/inc

[Qemu-devel] [PATCH v5 3/7] acpi, mem-hotplug: add unplug request cb for memory device

[Zhu Guihua]

From: Tang Chen 

This patch adds unplug request cb for memory device, and adds the
is_removing boolean field to MemStatus. This field is used to indicate
whether the memory slot is being removed. This field is set to true in
acpi_memory_unplug_request_cb().

Signed-off-by: Tang Chen 
Signed-off-by: Zhu Guihua 
---
 hw/acpi/ich9.c   | 10 --
 hw/acpi/memory_hotplug.c | 19 +++
 hw/acpi/piix4.c  |  6 +-
 hw/i386/pc.c | 28 ++--
 include/hw/acpi/memory_hotplug.h | 10 ++
 5 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 5352e19..b85eed4 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -400,8 +400,14 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState 
*dev, Error **errp)
 void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
   Error **errp)
 {
-error_setg(errp, "acpi: device unplug request for not supported device"
-   " type: %s", object_get_typename(OBJECT(dev)));
+if (pm->acpi_memory_hotplug.is_enabled &&
+object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq,
+  &pm->acpi_memory_hotplug, dev, errp);
+} else {
+error_setg(errp, "acpi: device unplug request for not supported device"
+   " type: %s", object_get_typename(OBJECT(dev)));
+}
 }
 
 void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 6af9303..42fe668 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -75,6 +75,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr 
addr,
 case 0x14: /* pack and return is_* fields */
 val |= mdev->is_enabled   ? 1 : 0;
 val |= mdev->is_inserting ? 2 : 0;
+val |= mdev->is_removing  ? 4 : 0;
 trace_mhp_acpi_read_flags(mem_st->selector, val);
 break;
 default:
@@ -218,6 +219,24 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, 
MemHotplugState *mem_st,
 return;
 }
 
+void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq,
+   MemHotplugState *mem_st,
+   DeviceState *dev, Error **errp)
+{
+MemStatus *mdev;
+
+mdev = acpi_memory_slot_status(mem_st, dev, errp);
+if (!mdev) {
+return;
+}
+
+mdev->is_removing = true;
+
+/* Do ACPI magic */
+ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
+acpi_update_sci(ar, irq);
+}
+
 static const VMStateDescription vmstate_memhp_sts = {
 .name = "memory hotplug device state",
 .version_id = 1,
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index d1f1179..f716e91 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -361,7 +361,11 @@ static void piix4_device_unplug_request_cb(HotplugHandler 
*hotplug_dev,
 {
 PIIX4PMState *s = PIIX4_PM(hotplug_dev);
 
-if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+if (s->acpi_memory_hotplug.is_enabled &&
+object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+acpi_memory_unplug_request_cb(&s->ar, s->irq, &s->acpi_memory_hotplug,
+  dev, errp);
+} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
 acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
 errp);
 } else {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a8e6be1..9b0859c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1677,6 +1677,26 @@ out:
 error_propagate(errp, local_err);
 }
 
+static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
+   DeviceState *dev, Error **errp)
+{
+HotplugHandlerClass *hhc;
+Error *local_err = NULL;
+PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+
+if (!pcms->acpi_dev) {
+error_setg(&local_err,
+   "memory hotplug is not enabled: missing acpi device");
+goto out;
+}
+
+hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
+hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+
+out:
+error_propagate(errp, local_err);
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
 DeviceState *dev, Error **errp)
 {
@@ -1719,8 +1739,12 @@ static void pc_machine_device_plug_cb(HotplugHandler 
*hotplug_dev,
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 DeviceState *dev, Error **errp)
 {
-error_setg(errp, "acpi: device unplug request for not supported device"
-   " type: %s", object_get_typename(OBJECT(dev)));
+if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+pc_dimm_unplug_request(hotplug_d

[Qemu-devel] [PATCH v5 6/7] acpi: add hardware implementation for memory hot unplug

[Zhu Guihua]

- implements QEMU hardware part of memory hot unplug protocol
  described at "docs/spec/acpi_mem_hotplug.txt"
- handles memory remove notification event
- handles device eject notification

Signed-off-by: Zhu Guihua 
---
 docs/specs/acpi_mem_hotplug.txt   | 16 +---
 hw/acpi/memory_hotplug.c  | 20 +++-
 hw/core/qdev.c|  2 +-
 hw/i386/acpi-build.c  | 12 
 hw/i386/acpi-dsdt-mem-hotplug.dsl | 13 -
 include/hw/acpi/pc-hotplug.h  |  3 +++
 include/hw/qdev-core.h|  1 +
 trace-events  |  2 ++
 8 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
index a88b286..96f9abe 100644
--- a/docs/specs/acpi_mem_hotplug.txt
+++ b/docs/specs/acpi_mem_hotplug.txt
@@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
access):
   1: Device insert event, used to distinguish device for which
  no device check event to OSPM was issued.
  It's valid only when bit 1 is set.
-  2-7: reserved and should be ignored by OSPM
+  2: Device remove event, used to distinguish device for which
+ no device check event to OSPM was issued.
+  3-7: reserved and should be ignored by OSPM
   [0x15-0x17] reserved
 
   write access:
@@ -31,11 +33,19 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
access):
   [0xc-0x13] reserved, writes into it are ignored
   [0x14] Memory device control fields
   bits:
-  0: reserved, OSPM must clear it before writing to register
+  0: reserved, OSPM must clear it before writing to register.
+ Due to BUG in versions prior 2.4 that field isn't
+ cleared when other fields are written.Keep it reserved
+ and don't try to reuse it.
   1: if set to 1 clears device insert event, set by OSPM
  after it has emitted device check event for the
  selected memory device
-  2-7: reserved, OSPM must clear them before writing to register
+  2: if set to 1 clears device remove event, set by OSPM
+ after it has emitted device check event for the
+ selected memory device
+  3: if set to 1 initiates device eject, set by OSPM when it
+ triggers memory device removal and calls _EJ0 method
+  4-7: reserved, OSPM must clear them before writing to register
 
 Selecting memory device slot beyond present range has no effect on platform:
- write accesses to memory hot-plug registers not documented above are
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 07e281f..2a1b866 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -2,6 +2,7 @@
 #include "hw/acpi/pc-hotplug.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/boards.h"
+#include "hw/qdev-core.h"
 #include "trace.h"
 #include "qapi-event.h"
 
@@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
 MemHotplugState *mem_st = opaque;
 MemStatus *mdev;
 ACPIOSTInfo *info;
+DeviceState *dev = NULL;
+HotplugHandler *hotplug_ctrl = NULL;
 
 if (!mem_st->dev_count) {
 return;
@@ -128,13 +131,28 @@ static void acpi_memory_hotplug_write(void *opaque, 
hwaddr addr, uint64_t data,
 qapi_event_send_acpi_device_ost(info, &error_abort);
 qapi_free_ACPIOSTInfo(info);
 break;
-case 0x14:
+case 0x14: /* set is_* fields  */
 mdev = &mem_st->devs[mem_st->selector];
 if (data & 2) { /* clear insert event */
 mdev->is_inserting  = false;
 trace_mhp_acpi_clear_insert_evt(mem_st->selector);
+} else if (data & 4) {
+mdev->is_removing = false;
+trace_mhp_acpi_clear_remove_evt(mem_st->selector);
+} else if (data & 8) {
+if (!mdev->is_enabled) {
+trace_mhp_acpi_ejecting_invalid_slot(mem_st->selector);
+break;
+}
+
+dev = DEVICE(mdev->dimm);
+hotplug_ctrl = qdev_get_hotplug_handler(dev);
+/* call pc-dimm unplug cb */
+hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
 }
 break;
+default:
+break;
 }
 
 }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6e6a65d..b0f0f84 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -273,7 +273,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int 
alias_id,
 dev->alias_required_for_version = required_for_version;
 }
 
-static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
+HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
 HotplugHandler *hotplug_ctrl = NULL;
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b767cbe

[Qemu-devel] [PATCH v5 5/7] acpi: extend aml_field() to support UpdateRule

[Zhu Guihua]

The flags field is declared with default update rule 'Preserve',
this patch is to extend aml_field() to support UpdateRule so that
we can specify different value to UpdateRule for different field.

Signed-off-by: Zhu Guihua 
---
 hw/acpi/aml-build.c |  4 +++-
 hw/i386/acpi-build.c| 13 -
 include/hw/acpi/aml-build.h | 10 --
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d7945f6..f926c9a 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -635,9 +635,11 @@ Aml *aml_reserved_field(unsigned length)
 }
 
 /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefField */
-Aml *aml_field(const char *name, AmlFieldFlags flags)
+Aml *aml_field(const char *name, AmlAccessType type, AmlUpdateRule rule)
 {
 Aml *var = aml_bundle(0x81 /* FieldOp */, AML_EXT_PACKAGE);
+uint8_t flags = rule << 5 | type;
+
 build_append_namestring(var->buf, "%s", name);
 build_append_byte(var->buf, flags);
 return var;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d0a5c85..b767cbe 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -798,7 +798,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 
 aml_append(dev, aml_operation_region("PEOR", aml_system_io,
   misc->pvpanic_port, 1));
-field = aml_field("PEOR", aml_byte_acc);
+field = aml_field("PEOR", aml_byte_acc, aml_preserve);
 aml_append(field, aml_named_field("PEPT", 8));
 aml_append(dev, field);
 
@@ -835,7 +835,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 /* declare CPU hotplug MMIO region and PRS field to access it */
 aml_append(sb_scope, aml_operation_region(
 "PRST", aml_system_io, pm->cpu_hp_io_base, pm->cpu_hp_io_len));
-field = aml_field("PRST", aml_byte_acc);
+field = aml_field("PRST", aml_byte_acc, aml_preserve);
 aml_append(field, aml_named_field("PRS", 256));
 aml_append(sb_scope, field);
 
@@ -909,7 +909,8 @@ build_ssdt(GArray *table_data, GArray *linker,
 pm->mem_hp_io_base, pm->mem_hp_io_len)
 );
 
-field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc);
+field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc,
+aml_preserve);
 aml_append(field, /* read only */
 aml_named_field(stringify(MEMORY_SLOT_ADDR_LOW), 32));
 aml_append(field, /* read only */
@@ -922,7 +923,8 @@ build_ssdt(GArray *table_data, GArray *linker,
 aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32));
 aml_append(scope, field);
 
-field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc);
+field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc,
+  aml_write_as_zeros);
 aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */));
 aml_append(field, /* 1 if enabled, read only */
 aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1));
@@ -931,7 +933,8 @@ build_ssdt(GArray *table_data, GArray *linker,
 aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1));
 aml_append(scope, field);
 
-field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc);
+field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc,
+  aml_preserve);
 aml_append(field, /* DIMM selector, write only */
 aml_named_field(stringify(MEMORY_SLOT_SLECTOR), 32));
 aml_append(field, /* _OST event code, write only */
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 17d3beb..5aa5e7a 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -35,7 +35,13 @@ typedef enum {
 aml_dword_acc = 3,
 aml_qword_acc = 4,
 aml_buffer_acc = 5,
-} AmlFieldFlags;
+} AmlAccessType;
+
+typedef enum {
+aml_preserve = 0,
+aml_write_as_ones = 1,
+aml_write_as_zeros = 2,
+} AmlUpdateRule;
 
 typedef enum {
 aml_system_memory = 0x00,
@@ -185,7 +191,7 @@ Aml *aml_if(Aml *predicate);
 Aml *aml_package(uint8_t num_elements);
 Aml *aml_buffer(void);
 Aml *aml_resource_template(void);
-Aml *aml_field(const char *name, AmlFieldFlags flags);
+Aml *aml_field(const char *name, AmlAccessType type, AmlUpdateRule rule);
 Aml *aml_varpackage(uint32_t num_elements);
 
 #endif
-- 
1.9.3

Re: [Qemu-devel] [PATCH v5 23/28] qapi: More rigorous checking for type safety bypass

[Markus Armbruster]

Eric Blake  writes:

> Now that we have a way to validate every type, we can also be
> stricter about enforcing that callers that want to bypass
> type safety in generated code.  Prior to this patch, it didn't
> matter what value was associated with the key 'gen', but it
> looked odd that 'gen':'yes' could result in bypassing the
> generated code.  These changes also enforce the changes made
> earlier in the series for documentation and consolidation of
> using '**' as the wildcard type, as well as 'gen':false as the
> canonical spelling for requesting type bypass.
>
> Signed-off-by: Eric Blake 
> ---
>  scripts/qapi.py| 23 ++-
>  tests/qapi-schema/type-bypass-bad-gen.err  |  1 +
>  tests/qapi-schema/type-bypass-bad-gen.exit |  2 +-
>  tests/qapi-schema/type-bypass-bad-gen.json |  2 +-
>  tests/qapi-schema/type-bypass-bad-gen.out  |  3 ---
>  tests/qapi-schema/type-bypass-no-gen.err   |  1 +
>  tests/qapi-schema/type-bypass-no-gen.exit  |  2 +-
>  tests/qapi-schema/type-bypass-no-gen.json  |  2 +-
>  tests/qapi-schema/type-bypass-no-gen.out   |  3 ---
>  9 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 9421431..800e8e4 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -321,13 +321,14 @@ def check_name(expr_info, source, name, allow_optional 
> = False):
>  "%s uses invalid name '%s'" % (source, name))
>
>  def check_type(expr_info, source, data, allow_array = False,
> -   allowed_metas = [], allow_dict = True, allow_optional = 
> False):
> +   allowed_metas = [], allow_dict = True, allow_optional = False,
> +   allow_star = False):
>  global all_names
>
>  if data is None:
>  return
>
> -if data == '**':
> +if allow_star and data == '**':
>  return
>
>  # Check if array type for data is okay

Aha!  Takes us back to my question on PATCH 21.

State so far: check_name() accepts '**', but some of its callers reject
it.  Whether it's rejected everywhere we don't want it isn't obvious,
even if we add rejects that are currently missing.

Perhaps we should try to do this the other way round: require extra code
in places where we want to accept it.

I can accept this as is, because it's an improvement, and we can plug
remaining holes on top.

> @@ -343,6 +344,10 @@ def check_type(expr_info, source, data, allow_array = 
> False,
>
>  # Check if type name for data is okay
>  if isinstance(data, str):
> +if data == '**':
> +raise QAPIExprError(expr_info,
> +"%s uses '**' but did not request 
> 'gen':false"
> +% source)
>  if not data in all_names:
>  raise QAPIExprError(expr_info,
>  "%s uses unknown type '%s'"
> @@ -367,19 +372,23 @@ def check_type(expr_info, source, data, allow_array = 
> False,
> allow_array=True,
> allowed_metas=['built-in', 'union', 'alternate', 'struct',
>'enum'],
> -   allow_dict=True, allow_optional=True)
> +   allow_dict=True, allow_optional=True, 
> allow_star=allow_star)
>
>  def check_command(expr, expr_info):
>  name = expr['command']
> +allow_star = expr.has_key('gen')
> +
>  check_type(expr_info, "'data' for command '%s'" % name,
> expr.get('data'),
> -   allowed_metas=['union', 'struct'], allow_optional=True)
> +   allowed_metas=['union', 'struct'], allow_optional=True,
> +   allow_star=allow_star)
>  returns_meta = ['union', 'struct']
>  if name in returns_whitelist:
>  returns_meta += ['built-in', 'alternate', 'enum']
>  check_type(expr_info, "'returns' for command '%s'" % name,
> expr.get('returns'), allow_array=True,
> -   allowed_metas=returns_meta, allow_optional=True)
> +   allowed_metas=returns_meta, allow_optional=True,
> +   allow_star=allow_star)
>
>  def check_event(expr, expr_info):
>  global events
> @@ -574,6 +583,10 @@ def check_keys(expr_elem, meta, required, optional=[]):
>  raise QAPIExprError(info,
>  "%s '%s' has unknown key '%s'"
>  % (meta, name, key))
> +if (key == 'gen' or key == 'success-response') and value != False:
> +raise QAPIExprError(info,
> +"'%s' of %s '%s' should only use false value"
> +% (key, meta, name))
>  for key in required:
>  if not expr.has_key(key):
>  raise QAPIExprError(info,
> diff --git a/tests/qapi-schema/type-bypass-bad-gen.err 
> b/tests/qapi-schema/type-bypass-bad-gen.err
> index e69de29..a83c3c6 100644
> --- a/tests/qapi-schema/type-bypass-bad-gen.err

[Qemu-devel] [PATCH v2] spapr: populate ibm,loc-code

[Nikunj A Dadhania]

Each hardware instance has a platform unique location code.  The OF
device tree that describes a part of a hardware entity must include
the “ibm,loc-code” property with a value that represents the location
code for that hardware entity.

Introduce an hcall to populate ibm,loc-code.
1) PCI passthru devices need to identify with its own ibm,loc-code
   available on the host.
2) Emulated devices encode as following: qemu_:.

Signed-off-by: Nikunj A Dadhania 
---

Changelog
v1:
* Dropped is_vfio patch and using TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE 
  to recognise vfio devices
* Removed wrapper for hcall
* Added sPAPRPHBClass::get_loc_code

 hw/ppc/spapr_hcall.c|  1 +
 hw/ppc/spapr_pci.c  | 38 +
 hw/ppc/spapr_pci_vfio.c | 51 +
 include/hw/pci-host/spapr.h |  1 +
 include/hw/ppc/spapr.h  |  8 ++-
 5 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 4f76f1c..b394681 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1010,6 +1010,7 @@ static void hypercall_register_types(void)
 
 /* ibm,client-architecture-support support */
 spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
+spapr_register_hypercall(KVMPPC_H_GET_LOC_CODE, phb_get_loc_code);
 }
 
 type_init(hypercall_register_types)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 05f4fac..c2ee476 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -248,6 +248,44 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, 
bool msix,
 }
 }
 
+target_ulong phb_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+  target_ulong opcode, target_ulong *args)
+{
+sPAPRPHBState *sphb = NULL;
+sPAPRPHBClass *spc = NULL;
+char *buf = NULL, path[PATH_MAX];
+PCIDevice *pdev;
+target_ulong buid = args[0];
+target_ulong config_addr = args[1];
+target_ulong loc_code = args[2];
+target_ulong size = args[3];
+
+sphb = find_phb(spapr, buid);
+pdev = find_dev(spapr, buid, config_addr);
+
+if (!sphb || !pdev) {
+return H_PARAMETER;
+}
+
+spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+if (spc->get_loc_code && spc->get_loc_code(sphb, pdev, &buf)) {
+cpu_physical_memory_write(loc_code, buf, strlen(buf));
+g_free(buf);
+return H_SUCCESS;
+}
+
+/*
+ * For non-vfio devices and failures make up the location code out
+ * of the name, slot and function.
+ *
+ *   qemu_:.
+ */
+snprintf(path, sizeof(path), "qemu_%s:%02d.%1d", pdev->name,
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+cpu_physical_memory_write(loc_code, path, size);
+return H_SUCCESS;
+}
+
 static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 uint32_t token, uint32_t nargs,
 target_ulong args, uint32_t nret,
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 99a1be5..bfdfa67 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -171,6 +171,56 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, 
int option)
 return RTAS_OUT_SUCCESS;
 }
 
+static bool spapr_phb_vfio_get_devspec(PCIDevice *pdev, char **value)
+{
+char *host;
+char path[PATH_MAX];
+struct stat st;
+
+host = object_property_get_str(OBJECT(pdev), "host", NULL);
+if (!host) {
+return false;
+}
+
+snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
+g_free(host);
+if (stat(path, &st) < 0) {
+return false;
+}
+
+return g_file_get_contents(path, value, NULL, NULL);
+}
+
+static int spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev,
+char **loc_code)
+{
+sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
+char path[PATH_MAX], *buf = NULL;
+struct stat st;
+
+/* Non VFIO devices */
+if (!svphb) {
+return false;
+}
+
+/* We have a vfio host bridge lets get the path. */
+if (!spapr_phb_vfio_get_devspec(pdev, &buf)) {
+return false;
+}
+
+snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+g_free(buf);
+if (stat(path, &st) < 0) {
+return false;
+}
+
+/* A valid file, now read the loc-code */
+if (g_file_get_contents(path, loc_code, NULL, NULL)) {
+return true;
+}
+return false;
+}
+
 static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
 {
 sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
@@ -199,6 +249,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, 
void *data)
 spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
 spc->eeh_reset = spapr_phb_vfio_eeh_reset;
 spc->eeh_configure = spapr_phb_vfio_eeh_configure;
+spc->get_loc_code =

Re: [Qemu-devel] [PATCH v5 24/28] qapi: Merge UserDefTwo and UserDefNested in tests

[Markus Armbruster]

Eric Blake  writes:

> In the testsuite, UserDefTwo and UserDefNested were identical
> types other than the member names.  Reduce code duplication by
> having just one type, and choose names that also favor reuse.
> This will also make it easier for a later patch to get rid of
> inline nested types in QAPI; it means that the type is now boxed
> instead of unboxed in C code, but has no difference to the QMP
> wire protocol.

I can't see a change from boxed to unboxed in this patch.  Does the
remark apply to the elimination of nested types?  If yes, it's premature
here.

> When touching code to add new allocations, also
> convert existing allocations to consistently prefer typesafe
> g_new0 over g_malloc0.

I can't see any change from g_malloc0() to g_new0().  Is this stale?

>
> Ensure that 'make check-qapi-schema check-unit' still passes.
>
> Signed-off-by: Eric Blake 

Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell]

On Fri, 2015-03-27 at 09:29 +0800, Chen, Tiejun wrote:
> On 2015/3/26 18:06, Ian Campbell wrote:
> > On Thu, 2015-03-26 at 08:53 +0800, Chen, Tiejun wrote:
> >>> Hrm, OK. I suppose we can live with autodetect and igd both meaning igd
> >>> and whoever adds a new type will have to remember to add a check for
> >>> qemu-trad then.
> >>>
> >>
> >> When we really have to introduce a new type, this means we probably need
> >> to change something inside qemu codes. So a new type should just go into
> >> that table to support qemu upstream since now we shouldn't refactor
> >> anything in qemu-xen-traditional, right?
> >
> > We'd want to error out on attempts to use qemu-xen-trad with non-IGD
> > passthru.
> >
> 
> On qemu-xen-traditional side, we always recognize this as BOOLEAN,
> 
>  if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { 
> 
>  flexarray_append(dm_args, "-gfx_passthru"); 
> 
>  }
> 
> Additionally, this is also clarified explicitly in manpage, and 
> especially we don't change this behavior now, so I'm just wondering why 
> we should do this :)

If someone does gfx_passthru = "foobar" and device_model_version =
"qemu-xen-traditional" in their xl config then it would be rather mean
of us to silently enable IGD passthru for them instead. When this occurs
libxl should notice and fail.

IGD is currently the only option, so this code would be needed when
someone adds "foobar" support.

Ian.

Re: [Qemu-devel] [PATCH] tcg: optimise memory layout of TCGTemp

[Alex Bennée]

Emilio G. Cota  writes:

> This brings down the size of the struct from 56 to 32 bytes on 64-bit,
> and to 16 bytes on 32-bit.

Have you been able to measure any performance improvement with these new
structures? In theory, if aligned with cache lines, performance should
improve but real numbers would be nice.

>
> The appended adds macros to prevent us from mistakenly overflowing
> the bitfields when more elements are added to the corresponding
> enums/macros.

I can see the defines but I can't see any checks. Should we be able to
do a compile time check if TCG_TYPE_COUNT doesn't fit into
TCG_TYPE_NR_BITS?

>
> Note that reg/mem_reg need only 6 bits (for ia64) but for performance
> is probably better to align them to a byte address.
>
> Given that TCGTemp is used in large arrays this leads to a few KBs
> of savings. However, unpacking the bits takes additional code, so
> the net effect depends on the target (host is x86_64):
>
> Before:
> $ find . -name 'tcg.o' | xargs size
>textdata bss dec hex filename
>   41131   29800  88   71019   1156b ./aarch64-softmmu/tcg/tcg.o
>   37969   29416  96   67481   10799 ./x86_64-linux-user/tcg/tcg.o
>   39354   28816  96   68266   10aaa ./arm-linux-user/tcg/tcg.o
>   40802   29096  88   69986   11162 ./arm-softmmu/tcg/tcg.o
>   39417   29672  88   69177   10e39 ./x86_64-softmmu/tcg/tcg.o
>
> After:
> $ find . -name 'tcg.o' | xargs size
>textdata bss dec hex filename
>   41187   29800  88   71075   115a3 ./aarch64-softmmu/tcg/tcg.o
>   3   29416  96   67289   106d9 ./x86_64-linux-user/tcg/tcg.o
>   39162   28816  96   68074   109ea ./arm-linux-user/tcg/tcg.o
>   40858   29096  88   70042   1119a ./arm-softmmu/tcg/tcg.o
>   39473   29672  88   69233   10e71 ./x86_64-softmmu/tcg/tcg.o
>
> Suggested-by: Stefan Weil 
> Suggested-by: Richard Henderson 
> Signed-off-by: Emilio G. Cota 
> ---
>  tcg/tcg.h | 22 +-
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index add7f75..71ae7b2 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -193,7 +193,7 @@ typedef struct TCGPool {
>  typedef enum TCGType {
>  TCG_TYPE_I32,
>  TCG_TYPE_I64,
> -TCG_TYPE_COUNT, /* number of different types */
> +TCG_TYPE_COUNT, /* number of different types, see TCG_TYPE_NR_BITS */
>  
>  /* An alias for the size of the host register.  */
>  #if TCG_TARGET_REG_BITS == 32
> @@ -217,6 +217,9 @@ typedef enum TCGType {
>  #endif
>  } TCGType;
>  
> +/* used for bitfield packing to save space */
> +#define TCG_TYPE_NR_BITS 1
> +
>  /* Constants for qemu_ld and qemu_st for the Memory Operation field.  */
>  typedef enum TCGMemOp {
>  MO_8 = 0,
> @@ -421,16 +424,14 @@ static inline TCGCond tcg_high_cond(TCGCond c)
>  #define TEMP_VAL_REG   1
>  #define TEMP_VAL_MEM   2
>  #define TEMP_VAL_CONST 3
> +#define TEMP_VAL_NR_BITS 2

A similar compile time check could be added here.

>  
> -/* XXX: optimize memory layout */
>  typedef struct TCGTemp {
> -TCGType base_type;
> -TCGType type;
> -int val_type;
> -int reg;
> -tcg_target_long val;
> -int mem_reg;
> -intptr_t mem_offset;
> +unsigned int reg:8;
> +unsigned int mem_reg:8;
> +unsigned int val_type:TEMP_VAL_NR_BITS;
> +unsigned int base_type:TCG_TYPE_NR_BITS;
> +unsigned int type:TCG_TYPE_NR_BITS;
>  unsigned int fixed_reg:1;
>  unsigned int mem_coherent:1;
>  unsigned int mem_allocated:1;
> @@ -438,6 +439,9 @@ typedef struct TCGTemp {
>basic blocks. Otherwise, it is not
>preserved across basic blocks. */
>  unsigned int temp_allocated:1; /* never used for code gen */
> +
> +tcg_target_long val;
> +intptr_t mem_offset;
>  const char *name;
>  } TCGTemp;

-- 
Alex Bennée

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Stefan Hajnoczi]

On Fri, Mar 27, 2015 at 9:14 AM, Wen Congyang  wrote:
> On 03/27/2015 04:56 PM, Stefan Hajnoczi wrote:
>> On Thu, Mar 26, 2015 at 11:29:43AM +0100, Juan Quintela wrote:
>>> Wen Congyang  wrote:
 On 03/25/2015 05:50 PM, Juan Quintela wrote:
> zhanghailiang  wrote:
>> Hi all,
>>
>> We found that, sometimes, the content of VM's memory is
>> inconsistent between Source side and Destination side
>> when we check it just after finishing migration but before VM continue 
>> to Run.
>>
>> We use a patch like bellow to find this issue, you can find it from 
>> affix,
>> and Steps to reprduce:
>>
>> (1) Compile QEMU:
>>  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && 
>> make
>>
>> (2) Command and output:
>> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
>> qemu64,-kvmclock -netdev tap,id=hn0-device
>> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
>> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
>> -device
>> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
>> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
>> -monitor stdio
>
> Could you try to reproduce:
> - without vhost
> - without virtio-net
> - cache=unsafe is going to give you trouble, but trouble should only
>   happen after migration of pages have finished.

 If I use ide disk, it doesn't happen.
 Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
 it is because I migrate the guest when it is booting. The virtio net
 device is not used in this case.
>>>
>>> Kevin, Stefan, Michael, any great idea?
>>
>> You must use -drive cache=none if you want to use live migration.  It
>> should not directly affect memory during migration though.
>
> Otherwise, what will happen? If the user doesn't use cache=none, and
> tries to use live migration, qemu doesn't output any message or trigger
> an event to notify the user.

There is a risk that the destination host sees an inconsistent view of
the image file because the source was still accessing it towards the
end of migration.

Stefan

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Wen Congyang]

On 03/27/2015 05:57 PM, Stefan Hajnoczi wrote:
> On Fri, Mar 27, 2015 at 9:14 AM, Wen Congyang  wrote:
>> On 03/27/2015 04:56 PM, Stefan Hajnoczi wrote:
>>> On Thu, Mar 26, 2015 at 11:29:43AM +0100, Juan Quintela wrote:
 Wen Congyang  wrote:
> On 03/25/2015 05:50 PM, Juan Quintela wrote:
>> zhanghailiang  wrote:
>>> Hi all,
>>>
>>> We found that, sometimes, the content of VM's memory is
>>> inconsistent between Source side and Destination side
>>> when we check it just after finishing migration but before VM continue 
>>> to Run.
>>>
>>> We use a patch like bellow to find this issue, you can find it from 
>>> affix,
>>> and Steps to reprduce:
>>>
>>> (1) Compile QEMU:
>>>  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && 
>>> make
>>>
>>> (2) Command and output:
>>> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
>>> qemu64,-kvmclock -netdev tap,id=hn0-device
>>> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
>>> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
>>> -device
>>> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
>>> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
>>> -monitor stdio
>>
>> Could you try to reproduce:
>> - without vhost
>> - without virtio-net
>> - cache=unsafe is going to give you trouble, but trouble should only
>>   happen after migration of pages have finished.
>
> If I use ide disk, it doesn't happen.
> Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
> it is because I migrate the guest when it is booting. The virtio net
> device is not used in this case.

 Kevin, Stefan, Michael, any great idea?
>>>
>>> You must use -drive cache=none if you want to use live migration.  It
>>> should not directly affect memory during migration though.
>>
>> Otherwise, what will happen? If the user doesn't use cache=none, and
>> tries to use live migration, qemu doesn't output any message or trigger
>> an event to notify the user.
> 
> There is a risk that the destination host sees an inconsistent view of
> the image file because the source was still accessing it towards the
> end of migration.

Does the flag BDRV_O_NOFLUSH cause it?

Thanks
Wen Congyang

> 
> Stefan
> .
> 

Re: [Qemu-devel] [RFC 00/10] MultiThread TCG.

[Alex Bennée]

fred.kon...@greensocs.com writes:

> From: KONRAD Frederic 
>
> Hi everybody,
>
> This is the start of our work on TCG multithread.

It's been a while since the first RFC are we likely to see a v2 of the
patch series any time soon?

>
> We send it for comment to be sure we are taking the right direction.
> We already discussed the first patch but we keep it for simplicity.
>
> We choice to keep a common tbs array for all VCPU but protect it with the
> tb_lock from TBContext. Then for each PageDesc we have a tb list per VCPU.
>
> Known issues:
>   * Some random deadlock.
>   * qemu_pause_cond is broken we can't quit QEMU.
>   * tb_flush is broken we must make sure no VCPU are executing code.
>
> Jan Kiszka (1):
>   Drop global lock during TCG code execution
>
> KONRAD Frederic (9):
>   target-arm: protect cpu_exclusive_*.
>   use a different translation block list for each cpu.
>   replace spinlock by QemuMutex.
>   remove unused spinlock.
>   extract TBContext from TCGContext.
>   protect TBContext with tb_lock.
>   tcg: remove tcg_halt_cond global variable.
>   cpu: remove exit_request global.
>   tcg: switch on multithread.
>
>  cpu-exec.c|  47 +++
>  cpus.c| 122 +++
>  cputlb.c  |   5 ++
>  exec.c|  25 ++
>  include/exec/exec-all.h   |   4 +-
>  include/exec/spinlock.h   |  49 ---
>  include/qom/cpu.h |   1 +
>  linux-user/main.c |   6 +-
>  scripts/checkpatch.pl |   9 +-
>  softmmu_template.h|   6 ++
>  target-arm/cpu.c  |  14 
>  target-arm/cpu.h  |   3 +
>  target-arm/helper.h   |   3 +
>  target-arm/op_helper.c|  10 +++
>  target-arm/translate.c|   6 ++
>  target-i386/mem_helper.c  |  16 +++-
>  target-i386/misc_helper.c |  27 +-
>  tcg/i386/tcg-target.c |   8 ++
>  tcg/tcg.h |   3 +-
>  translate-all.c   | 208 
> +++---
>  vl.c  |   6 ++
>  21 files changed, 335 insertions(+), 243 deletions(-)
>  delete mode 100644 include/exec/spinlock.h

-- 
Alex Bennée

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Stefan Hajnoczi]

On Fri, Mar 27, 2015 at 10:05 AM, Wen Congyang  wrote:
> On 03/27/2015 05:57 PM, Stefan Hajnoczi wrote:
>> On Fri, Mar 27, 2015 at 9:14 AM, Wen Congyang  wrote:
>>> On 03/27/2015 04:56 PM, Stefan Hajnoczi wrote:
 On Thu, Mar 26, 2015 at 11:29:43AM +0100, Juan Quintela wrote:
> Wen Congyang  wrote:
>> On 03/25/2015 05:50 PM, Juan Quintela wrote:
>>> zhanghailiang  wrote:
 Hi all,

 We found that, sometimes, the content of VM's memory is
 inconsistent between Source side and Destination side
 when we check it just after finishing migration but before VM continue 
 to Run.

 We use a patch like bellow to find this issue, you can find it from 
 affix,
 and Steps to reprduce:

 (1) Compile QEMU:
  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && 
 make

 (2) Command and output:
 SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
 qemu64,-kvmclock -netdev tap,id=hn0-device
 virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
 file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
 -device
 virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
 -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
 -monitor stdio
>>>
>>> Could you try to reproduce:
>>> - without vhost
>>> - without virtio-net
>>> - cache=unsafe is going to give you trouble, but trouble should only
>>>   happen after migration of pages have finished.
>>
>> If I use ide disk, it doesn't happen.
>> Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
>> it is because I migrate the guest when it is booting. The virtio net
>> device is not used in this case.
>
> Kevin, Stefan, Michael, any great idea?

 You must use -drive cache=none if you want to use live migration.  It
 should not directly affect memory during migration though.
>>>
>>> Otherwise, what will happen? If the user doesn't use cache=none, and
>>> tries to use live migration, qemu doesn't output any message or trigger
>>> an event to notify the user.
>>
>> There is a risk that the destination host sees an inconsistent view of
>> the image file because the source was still accessing it towards the
>> end of migration.
>
> Does the flag BDRV_O_NOFLUSH cause it?

Partly, but the problem is worse than that.  BDRV_O_NOFLUSH means that
the host never issues fdatasync(2) and you have no guarantee that the
data reaches the physical disk safely.

The migration problem is related to the host kernel page cache.
cache=none bypasses the page cache with O_DIRECT so the destination
host doesn't see outdated cached data.

Stefan

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[zhanghailiang]

On 2015/3/26 11:52, Li Zhijian wrote:

On 03/26/2015 11:12 AM, Wen Congyang wrote:

On 03/25/2015 05:50 PM, Juan Quintela wrote:

zhanghailiang  wrote:

Hi all,

We found that, sometimes, the content of VM's memory is inconsistent between 
Source side and Destination side
when we check it just after finishing migration but before VM continue to Run.

We use a patch like bellow to find this issue, you can find it from affix,
and Steps to reprduce:

(1) Compile QEMU:
  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && make

(2) Command and output:
SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu qemu64,-kvmclock 
-netdev tap,id=hn0-device virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive 
file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
 -device 
virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 -vnc 
:7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet -monitor stdio

Could you try to reproduce:
- without vhost
- without virtio-net
- cache=unsafe is going to give you trouble, but trouble should only
   happen after migration of pages have finished.

If I use ide disk, it doesn't happen.
Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
it is because I migrate the guest when it is booting. The virtio net
device is not used in this case.

Er～～
it reproduces in my ide disk
there is no any virtio device, my command line like below

x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu qemu64,-kvmclock -net none
-boot c -drive file=/home/lizj/ubuntu.raw -vnc :7 -m 2048 -smp 2 -machine
usb=off -no-user-config -nodefaults -monitor stdio -vga std

it seems easily to reproduce this issue by following steps in _ubuntu_ guest
1.  in source side, choose memtest in grub
2. do live migration
3. exit memtest(type Esc in when memory testing)
4. wait migration complete



Yes，it is a thorny problem. It is indeed easy to reproduce, just as
your steps in the above.

This is my test result: (I also test accel=tcg, it can be reproduced also.)
Source side:
# x86_64-softmmu/qemu-system-x86_64 -machine pc-i440fx-2.3,accel=kvm,usb=off 
-no-user-config -nodefaults  -cpu qemu64,-kvmclock -boot c -drive 
file=/mnt/sdb/pure_IMG/ubuntu/ubuntu_14.04_server_64_2U_raw -device 
cirrus-vga,id=video0,vgamem_mb=8 -vnc :7 -m 2048 -smp 2 -monitor stdio
(qemu) ACPI_BUILD: init ACPI tables
ACPI_BUILD: init ACPI tables
migrate tcp:9.61.1.8:3004
ACPI_BUILD: init ACPI tables
before cpu_synchronize_all_states
5a8f72d66732cac80d6a0d5713654c0e
md_host : before saving ram complete
5a8f72d66732cac80d6a0d5713654c0e
md_host : after saving ram complete
5a8f72d66732cac80d6a0d5713654c0e
(qemu)

Destination side:
# x86_64-softmmu/qemu-system-x86_64 -machine pc-i440fx-2.3,accel=kvm,usb=off 
-no-user-config -nodefaults  -cpu qemu64,-kvmclock -boot c -drive 
file=/mnt/sdb/pure_IMG/ubuntu/ubuntu_14.04_server_64_2U_raw -device 
cirrus-vga,id=video0,vgamem_mb=8 -vnc :7 -m 2048 -smp 2 -monitor stdio 
-incoming tcp:0:3004
(qemu) QEMU_VM_SECTION_END, after loading ram
d7cb0d8a4bdd1557fb0e78baee50c986
md_host : after loading all vmstate
d7cb0d8a4bdd1557fb0e78baee50c986
md_host : after cpu_synchronize_all_post_init
d7cb0d8a4bdd1557fb0e78baee50c986


Thanks,
zhang




What kind of load were you having when reproducing this issue?
Just to confirm, you have been able to reproduce this without COLO
patches, right?


(qemu) migrate tcp:192.168.3.8:3004
before saving ram complete
ff703f6889ab8701e4e040872d079a28
md_host : after saving ram complete
ff703f6889ab8701e4e040872d079a28

DST: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu qemu64,-kvmclock 
-netdev tap,id=hn0,vhost=on -device virtio-net-pci,id=net-pci0,netdev=hn0 -boot 
c -drive 
file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
 -device 
virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 -vnc 
:7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet -monitor stdio 
-incoming tcp:0:3004
(qemu) QEMU_VM_SECTION_END, after loading ram
230e1e68ece9cd4e769630e1bcb5ddfb
md_host : after loading all vmstate
230e1e68ece9cd4e769630e1bcb5ddfb
md_host : after cpu_synchronize_all_post_init
230e1e68ece9cd4e769630e1bcb5ddfb

This happens occasionally, and it is more easy to reproduce when issue 
migration command during VM's startup time.

OK, a couple of things.  Memory don't have to be exactly identical.
Virtio devices in particular do funny things on "post-load".  There
aren't warantees for that as far as I know, we should end with an
equivalent device state in memory.


We have done further test and found that some pages has been dirtied but its 
corresponding migration_bitmap is not set.
We can't figure out which modules of QEMU has missed setting bitmap when dirty 
page of VM,
it is very difficult for us to trace all the actions of dirtying VM's pages.

This seems to point to a bug in one of the devices.


Actually, the first time we found t

Re: [Qemu-devel] E5-2620v2 - emulation stop error

[Andrey Korolyov]

On Fri, Mar 27, 2015 at 12:03 AM, Bandan Das  wrote:
> Radim Krčmář  writes:
>
>> 2015-03-26 21:24+0300, Andrey Korolyov:
>>> On Thu, Mar 26, 2015 at 8:40 PM, Radim Krčmář  wrote:
>>> > 2015-03-26 20:08+0300, Andrey Korolyov:
>>> >> KVM internal error. Suberror: 2
>>> >> extra data[0]: 80ef
>>> >> extra data[1]: 8b0d
>>> >
>>> > Btw. does this part ever change?
>>> >
>>> > I see that first report had:
>>> >
>>> >   KVM internal error. Suberror: 2
>>> >   extra data[0]: 80d1
>>> >   extra data[1]: 8b0d
>>> >
>>> > Was that a Windows guest by any chance?
>>>
>>> Yes, exactly, different extra data output was from a Windows VMs.
>>
>> Windows uses vector 0xd1 for timer interrupts.
>
>> I second Bandan -- checking that it reproduces on other machine would be
>> great for sanity :)  (Although a bug in our APICv is far more likely.)
>
> If it's APICv related, a run without apicv enabled could give more hints.
>
> Your "devices not getting reset" hypothesis makes the most sense to me,
> maybe the timer vector in the error message is just one part of
> the whole story. Another misbehaving interrupt from the dark comes in at the
> same time and leads to a double fault.

Default trace (APICv enabled, first reboot introduced the issue):
http://xdel.ru/downloads/kvm-e5v2-issue/hanged-reboot-apic-on.dat.gz

Trace without APICv (three reboots, just to make sure to hit the
problematic condition of supposed DF, as it still have not one hundred
percent reproducibility):
http://xdel.ru/downloads/kvm-e5v2-issue/apic-off.dat.gz

It would be great of course to reproduce this somewhere else,
otherwise all this thread may end in fixing a bug which exists only at
my particular platform. Right now I have no hardware except a lot of
well-known (in terms of existing issues) Supermicro boards of one
model.

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Dr. David Alan Gilbert]

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
> On 2015/3/26 11:52, Li Zhijian wrote:
> >On 03/26/2015 11:12 AM, Wen Congyang wrote:
> >>On 03/25/2015 05:50 PM, Juan Quintela wrote:
> >>>zhanghailiang  wrote:
> Hi all,
> 
> We found that, sometimes, the content of VM's memory is inconsistent 
> between Source side and Destination side
> when we check it just after finishing migration but before VM continue to 
> Run.
> 
> We use a patch like bellow to find this issue, you can find it from affix,
> and Steps to reprduce:
> 
> (1) Compile QEMU:
>   ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && 
>  make
> 
> (2) Command and output:
> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu 
> qemu64,-kvmclock -netdev tap,id=hn0-device 
> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive 
> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
>  -device 
> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
>  -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet 
> -monitor stdio
> >>>Could you try to reproduce:
> >>>- without vhost
> >>>- without virtio-net
> >>>- cache=unsafe is going to give you trouble, but trouble should only
> >>>   happen after migration of pages have finished.
> >>If I use ide disk, it doesn't happen.
> >>Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
> >>it is because I migrate the guest when it is booting. The virtio net
> >>device is not used in this case.
> >Er??
> >it reproduces in my ide disk
> >there is no any virtio device, my command line like below
> >
> >x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu qemu64,-kvmclock -net none
> >-boot c -drive file=/home/lizj/ubuntu.raw -vnc :7 -m 2048 -smp 2 -machine
> >usb=off -no-user-config -nodefaults -monitor stdio -vga std
> >
> >it seems easily to reproduce this issue by following steps in _ubuntu_ guest
> >1.  in source side, choose memtest in grub
> >2. do live migration
> >3. exit memtest(type Esc in when memory testing)
> >4. wait migration complete
> >
> 
> Yes???it is a thorny problem. It is indeed easy to reproduce, just as
> your steps in the above.
> 
> This is my test result: (I also test accel=tcg, it can be reproduced also.)
> Source side:
> # x86_64-softmmu/qemu-system-x86_64 -machine pc-i440fx-2.3,accel=kvm,usb=off 
> -no-user-config -nodefaults  -cpu qemu64,-kvmclock -boot c -drive 
> file=/mnt/sdb/pure_IMG/ubuntu/ubuntu_14.04_server_64_2U_raw -device 
> cirrus-vga,id=video0,vgamem_mb=8 -vnc :7 -m 2048 -smp 2 -monitor stdio
> (qemu) ACPI_BUILD: init ACPI tables
> ACPI_BUILD: init ACPI tables
> migrate tcp:9.61.1.8:3004
> ACPI_BUILD: init ACPI tables
> before cpu_synchronize_all_states
> 5a8f72d66732cac80d6a0d5713654c0e
> md_host : before saving ram complete
> 5a8f72d66732cac80d6a0d5713654c0e
> md_host : after saving ram complete
> 5a8f72d66732cac80d6a0d5713654c0e
> (qemu)
>
> Destination side:
> # x86_64-softmmu/qemu-system-x86_64 -machine pc-i440fx-2.3,accel=kvm,usb=off 
> -no-user-config -nodefaults  -cpu qemu64,-kvmclock -boot c -drive 
> file=/mnt/sdb/pure_IMG/ubuntu/ubuntu_14.04_server_64_2U_raw -device 
> cirrus-vga,id=video0,vgamem_mb=8 -vnc :7 -m 2048 -smp 2 -monitor stdio 
> -incoming tcp:0:3004
> (qemu) QEMU_VM_SECTION_END, after loading ram
> d7cb0d8a4bdd1557fb0e78baee50c986
> md_host : after loading all vmstate
> d7cb0d8a4bdd1557fb0e78baee50c986
> md_host : after cpu_synchronize_all_post_init
> d7cb0d8a4bdd1557fb0e78baee50c986

Hmm, that's not good.  I suggest you md5 each of the RAMBlock's individually;
to see if it's main RAM that's different or something more subtle like
video RAM.

But then maybe it's easier just to dump the whole of RAM to file
and byte compare it (hexdump the two dumps and diff ?)

Dave

> 
> 
> Thanks,
> zhang
> 
> >>
> >>>What kind of load were you having when reproducing this issue?
> >>>Just to confirm, you have been able to reproduce this without COLO
> >>>patches, right?
> >>>
> (qemu) migrate tcp:192.168.3.8:3004
> before saving ram complete
> ff703f6889ab8701e4e040872d079a28
> md_host : after saving ram complete
> ff703f6889ab8701e4e040872d079a28
> 
> DST: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu 
> qemu64,-kvmclock -netdev tap,id=hn0,vhost=on -device 
> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive 
> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
>  -device 
> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
>  -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet 
> -monitor stdio -incoming tcp:0:3004
> (qemu) QEMU_VM_SECTION_END, after loading ram
> 230e1e68ece9cd4e769630e1bcb5ddfb
> md_host : after loading all vmstate
> 230e1e68ece9cd4e769630e1bcb5ddfb
> md_host : after cpu_sy

[Qemu-devel] [PULL for-2.3 0/4] Block patches

[Stefan Hajnoczi]

The following changes since commit 4ad9e2b36e1e00fe5b96c3448ecd673e11c4d6d8:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-gtk-20150326-1' into 
staging (2015-03-26 18:35:09 +)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to da2cf4e80355e42fbaeb8bcfa2b07f5bceddd323:

  block: Document blockdev-add's immaturity (2015-03-27 10:01:12 +)





Fam Zheng (2):
  block: Fix unaligned zero write
  qemu-iotests: Test unaligned 4k zero write

Markus Armbruster (1):
  block: Document blockdev-add's immaturity

Stefan Weil (1):
  nvme: Fix unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)

 block.c| 45 ++--
 hw/block/nvme.c|  2 +-
 qapi/block-core.json   |  4 
 qmp-commands.hx|  4 
 tests/qemu-iotests/033 | 47 +-
 tests/qemu-iotests/033.out | 26 +
 6 files changed, 104 insertions(+), 24 deletions(-)

-- 
2.1.0

[Qemu-devel] [PULL for-2.3 2/4] block: Fix unaligned zero write

[Stefan Hajnoczi]

From: Fam Zheng 

If the zero write is not aligned, bdrv_co_do_pwritev will segfault
because of accessing to the NULL qiov passed in by bdrv_co_write_zeroes.
Fix this by allocating a local qiov in bdrv_co_do_pwritev if the request
is not aligned. (In this case the padding iovs are necessary anyway, so
it doesn't hurt.)

Also add a check at the end of bdrv_co_do_pwritev to clear the zero flag
if padding is involved.

Signed-off-by: Fam Zheng 
Message-id: 1427160230-4489-2-git-send-email-f...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block.c | 45 +++--
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 0fe97de..f2f8ae7 100644
--- a/block.c
+++ b/block.c
@@ -3118,6 +3118,19 @@ out:
 return ret;
 }
 
+static inline uint64_t bdrv_get_align(BlockDriverState *bs)
+{
+/* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
+return MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+}
+
+static inline bool bdrv_req_is_aligned(BlockDriverState *bs,
+   int64_t offset, size_t bytes)
+{
+int64_t align = bdrv_get_align(bs);
+return !(offset & (align - 1) || (bytes & (align - 1)));
+}
+
 /*
  * Handle a read request in coroutine context
  */
@@ -3128,8 +3141,7 @@ static int coroutine_fn 
bdrv_co_do_preadv(BlockDriverState *bs,
 BlockDriver *drv = bs->drv;
 BdrvTrackedRequest req;
 
-/* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
-uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+uint64_t align = bdrv_get_align(bs);
 uint8_t *head_buf = NULL;
 uint8_t *tail_buf = NULL;
 QEMUIOVector local_qiov;
@@ -3371,8 +3383,7 @@ static int coroutine_fn 
bdrv_co_do_pwritev(BlockDriverState *bs,
 BdrvRequestFlags flags)
 {
 BdrvTrackedRequest req;
-/* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
-uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
+uint64_t align = bdrv_get_align(bs);
 uint8_t *head_buf = NULL;
 uint8_t *tail_buf = NULL;
 QEMUIOVector local_qiov;
@@ -3471,6 +3482,10 @@ static int coroutine_fn 
bdrv_co_do_pwritev(BlockDriverState *bs,
 bytes = ROUND_UP(bytes, align);
 }
 
+if (use_local_qiov) {
+/* Local buffer may have non-zero data. */
+flags &= ~BDRV_REQ_ZERO_WRITE;
+}
 ret = bdrv_aligned_pwritev(bs, &req, offset, bytes,
use_local_qiov ? &local_qiov : qiov,
flags);
@@ -3511,14 +3526,32 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState 
*bs,
   int64_t sector_num, int nb_sectors,
   BdrvRequestFlags flags)
 {
+int ret;
+
 trace_bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
 
 if (!(bs->open_flags & BDRV_O_UNMAP)) {
 flags &= ~BDRV_REQ_MAY_UNMAP;
 }
+if (bdrv_req_is_aligned(bs, sector_num << BDRV_SECTOR_BITS,
+nb_sectors << BDRV_SECTOR_BITS)) {
+ret = bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
+BDRV_REQ_ZERO_WRITE | flags);
+} else {
+uint8_t *buf;
+QEMUIOVector local_qiov;
+size_t bytes = nb_sectors << BDRV_SECTOR_BITS;
 
-return bdrv_co_do_writev(bs, sector_num, nb_sectors, NULL,
- BDRV_REQ_ZERO_WRITE | flags);
+buf = qemu_memalign(bdrv_opt_mem_align(bs), bytes);
+memset(buf, 0, bytes);
+qemu_iovec_init(&local_qiov, 1);
+qemu_iovec_add(&local_qiov, buf, bytes);
+
+ret = bdrv_co_do_writev(bs, sector_num, nb_sectors, &local_qiov,
+BDRV_REQ_ZERO_WRITE | flags);
+qemu_vfree(buf);
+}
+return ret;
 }
 
 /**
-- 
2.1.0

[Qemu-devel] [PULL for-2.3 1/4] nvme: Fix unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)

[Stefan Hajnoczi]

From: Stefan Weil 

The shift operation on nlb gives a 32 bit result if no type cast is
applied. This bug was reported by Coverity.

Signed-off-by: Stefan Weil 
Message-id: 1426348844-8793-1-git-send-email...@weilnetz.de
Signed-off-by: Stefan Hajnoczi 
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 0f3dfb9..1e07166 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -222,7 +222,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
NvmeCmd *cmd,
 
 uint8_t lba_index  = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
 uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
-uint64_t data_size = nlb << data_shift;
+uint64_t data_size = (uint64_t)nlb << data_shift;
 uint64_t aio_slba  = slba << (data_shift - BDRV_SECTOR_BITS);
 int is_write = rw->opcode == NVME_CMD_WRITE ? 1 : 0;
 
-- 
2.1.0

[Qemu-devel] [PULL for-2.3 4/4] block: Document blockdev-add's immaturity

[Stefan Hajnoczi]

From: Markus Armbruster 

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Message-id: 1426858337-21423-1-git-send-email-arm...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 qapi/block-core.json | 4 
 qmp-commands.hx  | 4 
 2 files changed, 8 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f525b04..7873084 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1721,6 +1721,10 @@
 #
 # Creates a new block device.
 #
+# This command is still a work in progress.  It doesn't support all
+# block drivers, it lacks a matching blockdev-del, and more.  Stay
+# away from it unless you want to help with its development.
+#
 # @options: block device options for the new device
 #
 # Since: 1.7
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7f68760..3a42ad0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3636,6 +3636,10 @@ blockdev-add
 
 Add a block device.
 
+This command is still a work in progress.  It doesn't support all
+block drivers, it lacks a matching blockdev-del, and more.  Stay away
+from it unless you want to help with its development.
+
 Arguments:
 
 - "options": block driver options
-- 
2.1.0

[Qemu-devel] [PULL for-2.3 3/4] qemu-iotests: Test unaligned 4k zero write

[Stefan Hajnoczi]

From: Fam Zheng 

Signed-off-by: Fam Zheng 
Message-id: 1427160230-4489-3-git-send-email-f...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 tests/qemu-iotests/033 | 47 +-
 tests/qemu-iotests/033.out | 26 +
 2 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index ea3351c..4008f10 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -46,26 +46,39 @@ _supported_os Linux
 size=128M
 _make_test_img $size
 
-echo
-echo "== preparing image =="
-$QEMU_IO -c "write -P 0xa 0x200 0x400" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "write -P 0xa 0x2 0x600" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "write -z 0x400 0x2" "$TEST_IMG" | _filter_qemu_io
+do_test()
+{
+   local align=$1
+   local iocmd=$2
+   local img=$3
+   {
+   echo "open -o driver=$IMGFMT,file.align=$align blkdebug::$img"
+   echo $iocmd
+   } | $QEMU_IO
+}
 
-echo
-echo "== verifying patterns (1) =="
-$QEMU_IO -c "read -P 0xa 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0x0 0x400 0x2" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -P 0xa 0x20400 0x200" "$TEST_IMG" | _filter_qemu_io
+for align in 512 4k; do
+   echo
+   echo "== preparing image =="
+   do_test $align "write -P 0xa 0x200 0x400" "$TEST_IMG" | _filter_qemu_io
+   do_test $align "write -P 0xa 0x2 0x600" "$TEST_IMG" | 
_filter_qemu_io
+   do_test $align "write -z 0x400 0x2" "$TEST_IMG" | _filter_qemu_io
 
-echo
-echo "== rewriting zeroes =="
-$QEMU_IO -c "write -P 0xb 0x1 0x1" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "write -z 0x1 0x1" "$TEST_IMG" | _filter_qemu_io
+   echo
+   echo "== verifying patterns (1) =="
+   do_test $align "read -P 0xa 0x200 0x200" "$TEST_IMG" | _filter_qemu_io
+   do_test $align "read -P 0x0 0x400 0x2" "$TEST_IMG" | _filter_qemu_io
+   do_test $align "read -P 0xa 0x20400 0x200" "$TEST_IMG" | _filter_qemu_io
 
-echo
-echo "== verifying patterns (2) =="
-$QEMU_IO -c "read -P 0x0 0x400 0x2" "$TEST_IMG" | _filter_qemu_io
+   echo
+   echo "== rewriting zeroes =="
+   do_test $align "write -P 0xb 0x1 0x1" "$TEST_IMG" | 
_filter_qemu_io
+   do_test $align "write -z 0x1 0x1" "$TEST_IMG" | _filter_qemu_io
+
+   echo
+   echo "== verifying patterns (2) =="
+   do_test $align "read -P 0x0 0x400 0x2" "$TEST_IMG" | _filter_qemu_io
+done
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/033.out b/tests/qemu-iotests/033.out
index 41475ee..305949f 100644
--- a/tests/qemu-iotests/033.out
+++ b/tests/qemu-iotests/033.out
@@ -26,4 +26,30 @@ wrote 65536/65536 bytes at offset 65536
 == verifying patterns (2) ==
 read 131072/131072 bytes at offset 1024
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== preparing image ==
+wrote 1024/1024 bytes at offset 512
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1536/1536 bytes at offset 131072
+1.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (1) ==
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 132096
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== rewriting zeroes ==
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying patterns (2) ==
+read 131072/131072 bytes at offset 1024
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.1.0

Re: [Qemu-devel] [PATCH v5 25/28] qapi: Drop tests for inline nested structs

[Markus Armbruster]

Eric Blake  writes:

> A future patch will be using a 'name':{dictionary} entry in the
> QAPI schema to specify a default value for an optional argument;
> but existing use of inline nested structs conflicts with that goal.

I'm okay with this rationale as is, just want to mention the more
general motivation again: we want to make the schema language
extensible.

A definition in the schema associates a name with a set of properties in
a certain lexical environment.

Example 1: { 'type': 'Foo', 'data': { MEMBERS... } }
at the top level associates global name 'Foo' with properties (meta-type
struct) and MEMBERS...

Example 2: 'mumble': TYPE
within the MEMBERS... above associates 'mumble' with properties (type
TYPE) and (optional false) within type Foo.

The syntax in example 1 is extensible: when we need another property, we
can add another member, just like we added 'base'.

The syntax in example 2 isn't extensible that way, because the right
hand side can only be a type.

Property optional is encoded in the name: "'*mumble': 'int'" associates
'mumble' with (type int) and (optional true).  Convenient, but won't
scale.

What we want to do is change the core syntax to

NAME: { 'type': TYPE }

with syntactic sugar

NAME: TYPE-->  NAME:  { 'type': TYPE, 'optional': false }
*ONAME: TYPE  -->  ONAME: { 'type': TYPE, 'optional': true }

where *ONAME is a string starting with '*', and ONAME its tail.

Now we can extend by adding another member.

Naturally, the schema will mostly be written in the sugared form.

Adding optional defaults is an instance of this general "extend the
schema language" pattern.

Feel free to crib from the above if you want more of the motivation in
the commit log,

> This patch fixes the testsuite to avoid inline nested types, by
> breaking the nesting into explicit types; it means that the type
> is now boxed instead of unboxed in C code, but makes no difference
> on the wire.

Yes, this patch actually adds boxing.

The QAPI code generator boxes too much for my taste, but this series is
not the place to mess with that.

>   When touching code to add new allocations, also
> convert existing allocations to consistently prefer typesafe
> g_new0 over g_malloc0.

Again, I can't see new uses of g_new0().

> Signed-off-by: Eric Blake 
> ---
>  tests/qapi-schema/qapi-schema-test.json | 12 ++--
>  tests/qapi-schema/qapi-schema-test.out  |  8 +++--
>  tests/test-qmp-commands.c   | 17 ++-
>  tests/test-qmp-input-visitor.c  | 14 +
>  tests/test-qmp-output-visitor.c | 44 +++
>  tests/test-visitor-serialization.c  | 53 
> ++---
>  6 files changed, 87 insertions(+), 61 deletions(-)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index fb6a350..7aeb490 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -14,11 +14,17 @@
>'base': 'UserDefZero',
>'data': { 'string': 'str', '*enum1': 'EnumOne' } }
>
> +{ 'type': 'UserDefTwoDictDict',
> +  'data': { 'userdef': 'UserDefOne', 'string': 'str' } }
> +
> +{ 'type': 'UserDefTwoDict',
> +  'data': { 'string1': 'str',
> +'dict2': 'UserDefTwoDictDict',
> +'*dict3': 'UserDefTwoDictDict' } }
> +
>  { 'type': 'UserDefTwo',
>'data': { 'string0': 'str',
> -'dict1': { 'string1': 'str',
> -   'dict2': { 'userdef': 'UserDefOne', 'string': 'str' },
> -   '*dict3': { 'userdef': 'UserDefOne', 'string': 'str' 
> } } } }
> +'dict1': 'UserDefTwoDict' } }
>
>  # for testing unions
>  { 'type': 'UserDefA',
[...]
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 9189cd2..dc199d3 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -33,12 +33,15 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
>
>  ret = g_malloc0(sizeof(UserDefTwo));
>  ret->string0 = strdup("blah1");
> -ret->dict1.string1 = strdup("blah2");
> -ret->dict1.dict2.userdef = ud1c;
> -ret->dict1.dict2.string = strdup("blah3");
> -ret->dict1.has_dict3 = true;
> -ret->dict1.dict3.userdef = ud1d;
> -ret->dict1.dict3.string = strdup("blah4");
> +ret->dict1 = g_malloc0(sizeof(UserDefTwoDict));

Hmm, you could g_new0(UserDefTwoDict) here.  More of the same below, and
in the previous patch.

> +ret->dict1->string1 = strdup("blah2");
> +ret->dict1->dict2 = g_malloc0(sizeof(UserDefTwoDictDict));
> +ret->dict1->dict2->userdef = ud1c;
> +ret->dict1->dict2->string = strdup("blah3");
> +ret->dict1->dict3 = g_malloc0(sizeof(UserDefTwoDictDict));
> +ret->dict1->has_dict3 = true;
> +ret->dict1->dict3->userdef = ud1d;
> +ret->dict1->dict3->string = strdup("blah4");
>
>  return ret;
>  }
[...]

With either the commit message corrected not to claim use of g_new0(),
or the patch

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Juan Quintela]

Wen Congyang  wrote:
> On 03/27/2015 04:56 PM, Stefan Hajnoczi wrote:
>> On Thu, Mar 26, 2015 at 11:29:43AM +0100, Juan Quintela wrote:
>>> Wen Congyang  wrote:
 On 03/25/2015 05:50 PM, Juan Quintela wrote:
> zhanghailiang  wrote:
>> Hi all,
>>
>> We found that, sometimes, the content of VM's memory is
>> inconsistent between Source side and Destination side
>> when we check it just after finishing migration but before VM
>> continue to Run.
>>
>> We use a patch like bellow to find this issue, you can find it from 
>> affix,
>> and Steps to reprduce:
>>
>> (1) Compile QEMU:
>>  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && 
>> make
>>
>> (2) Command and output:
>> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
>> qemu64,-kvmclock -netdev tap,id=hn0-device
>> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
>> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
>> -device
>> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
>> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
>> -monitor stdio
>
> Could you try to reproduce:
> - without vhost
> - without virtio-net
> - cache=unsafe is going to give you trouble, but trouble should only
>   happen after migration of pages have finished.

 If I use ide disk, it doesn't happen.
 Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
 it is because I migrate the guest when it is booting. The virtio net
 device is not used in this case.
>>>
>>> Kevin, Stefan, Michael, any great idea?
>> 
>> You must use -drive cache=none if you want to use live migration.  It
>> should not directly affect memory during migration though.
>
> Otherwise, what will happen? If the user doesn't use cache=none, and
> tries to use live migration, qemu doesn't output any message or trigger
> an event to notify the user.

Problem here is what is your shared storage.  Some clustered filesystem
got this right and can run without cache=none.  But neither NFS, iscsi
or FC (FC is by my understanding, not sure) can.  And that are the more
used ones.  So, qemu don't now what FS/storage type the user has, so it
can make no real errors.

Later, Juan.


>
> Thanks
> Wen Congyang
>
>> 
>> We have done further test and found that some pages has been
>> dirtied but its corresponding migration_bitmap is not set.
>> We can't figure out which modules of QEMU has missed setting bitmap
>> when dirty page of VM,
>> it is very difficult for us to trace all the actions of dirtying VM's 
>> pages.
>
> This seems to point to a bug in one of the devices.
>> 
>> I think you'll need to track down which pages are different.  If you are
>> lucky, their contents will reveal what the page is used for.
>> 
>> Stefan
>> 

Re: [Qemu-devel] [PATCH v5 26/28] qapi: Drop inline nested type in query-version

[Markus Armbruster]

Eric Blake  writes:

> A future patch will be using a 'name':{dictionary} entry in the
> QAPI schema to specify a default value for an optional argument;
> but existing use of inline nested structs conflicts with that goal.
> This patch fixes one of only two commands relying on nested
> types, by breaking the nesting into an explicit type; it means
> that the type is now boxed instead of unboxed in C code, but the
> QMP wire format is unaffected by this change.
>
> Prefer the safer g_new0() while making the conversion.
>
> Signed-off-by: Eric Blake 

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Juan Quintela]

Wen Congyang  wrote:
> On 03/27/2015 05:57 PM, Stefan Hajnoczi wrote:
 You must use -drive cache=none if you want to use live migration.  It
 should not directly affect memory during migration though.
>>>
>>> Otherwise, what will happen? If the user doesn't use cache=none, and
>>> tries to use live migration, qemu doesn't output any message or trigger
>>> an event to notify the user.
>> 
>> There is a risk that the destination host sees an inconsistent view of
>> the image file because the source was still accessing it towards the
>> end of migration.
>
> Does the flag BDRV_O_NOFLUSH cause it?

The biggest isue is reads.

Target: reads 1st block of disk
it enters page cache of target
Source: reads and writes lots of blocks

Migration happens.

Target: rereads 1st block of disk, and gets stalled contents.

No amount of fdatasync(), syncs() whatever on source will fix this problem.


Later, Juan.

Re: [Qemu-devel] [RFC 00/10] MultiThread TCG.

[Frederic Konrad]

Hi,

Yes a v2 will come soon.
Actually I'm trying to unbreak GDB stub and a strange segfault with smp > 2.

Fred

On 27/03/2015 11:08, Alex Bennée wrote:

fred.kon...@greensocs.com writes:


From: KONRAD Frederic 

Hi everybody,

This is the start of our work on TCG multithread.

It's been a while since the first RFC are we likely to see a v2 of the
patch series any time soon?


We send it for comment to be sure we are taking the right direction.
We already discussed the first patch but we keep it for simplicity.

We choice to keep a common tbs array for all VCPU but protect it with the
tb_lock from TBContext. Then for each PageDesc we have a tb list per VCPU.

Known issues:
   * Some random deadlock.
   * qemu_pause_cond is broken we can't quit QEMU.
   * tb_flush is broken we must make sure no VCPU are executing code.

Jan Kiszka (1):
   Drop global lock during TCG code execution

KONRAD Frederic (9):
   target-arm: protect cpu_exclusive_*.
   use a different translation block list for each cpu.
   replace spinlock by QemuMutex.
   remove unused spinlock.
   extract TBContext from TCGContext.
   protect TBContext with tb_lock.
   tcg: remove tcg_halt_cond global variable.
   cpu: remove exit_request global.
   tcg: switch on multithread.

  cpu-exec.c|  47 +++
  cpus.c| 122 +++
  cputlb.c  |   5 ++
  exec.c|  25 ++
  include/exec/exec-all.h   |   4 +-
  include/exec/spinlock.h   |  49 ---
  include/qom/cpu.h |   1 +
  linux-user/main.c |   6 +-
  scripts/checkpatch.pl |   9 +-
  softmmu_template.h|   6 ++
  target-arm/cpu.c  |  14 
  target-arm/cpu.h  |   3 +
  target-arm/helper.h   |   3 +
  target-arm/op_helper.c|  10 +++
  target-arm/translate.c|   6 ++
  target-i386/mem_helper.c  |  16 +++-
  target-i386/misc_helper.c |  27 +-
  tcg/i386/tcg-target.c |   8 ++
  tcg/tcg.h |   3 +-
  translate-all.c   | 208 +++---
  vl.c  |   6 ++
  21 files changed, 335 insertions(+), 243 deletions(-)
  delete mode 100644 include/exec/spinlock.h

Re: [Qemu-devel] [PATCH v5 27/28] qapi: Drop inline nested types in query-pci

[Markus Armbruster]

Eric Blake  writes:

> A future patch will be using a 'name':{dictionary} entry in the
> QAPI schema to specify a default value for an optional argument;
> but existing use of inline nested structs conflicts with that goal.
> This patch fixes one of only two commands relying on nested
> types, by breaking the nesting into an explicit type; it means
> that the type is now boxed instead of unboxed in C code, but the
> QMP wire format is unaffected by this change.
>
> Prefer the safer g_new0() while making the conversion, and reduce
> some long lines.
>
> Signed-off-by: Eric Blake 

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH v5 28/28] qapi: Drop support for inline nested types

[Markus Armbruster]

Eric Blake  writes:

> A future patch will be using a 'name':{dictionary} entry in the
> QAPI schema to specify a default value for an optional argument;
> but existing use of inline nested structs conflicts with that goal.
> Now that all commands have been changed to avoid inline nested
> structs, nuke support for them, and turn it into a hard error.
> Update the testsuite to reflect tighter parsing rules.
>
> Signed-off-by: Eric Blake 
> ---
>  scripts/qapi-commands.py |  8 +++---
>  scripts/qapi-event.py|  4 +--
>  scripts/qapi-types.py|  9 ++-
>  scripts/qapi-visit.py| 37 
> 
>  scripts/qapi.py  | 18 +-
>  tests/qapi-schema/event-nest-struct.err  |  2 +-
>  tests/qapi-schema/nested-struct-data.err |  1 +
>  tests/qapi-schema/nested-struct-data.exit|  2 +-
>  tests/qapi-schema/nested-struct-data.json|  2 +-
>  tests/qapi-schema/nested-struct-data.out |  3 ---
>  tests/qapi-schema/nested-struct-returns.err  |  1 +
>  tests/qapi-schema/nested-struct-returns.exit |  2 +-
>  tests/qapi-schema/nested-struct-returns.json |  2 +-
>  tests/qapi-schema/nested-struct-returns.out  |  3 ---
>  14 files changed, 26 insertions(+), 68 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 053ba85..db81044 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -28,7 +28,7 @@ def type_visitor(name):
>
>  def generate_command_decl(name, args, ret_type):
>  arglist=""
> -for argname, argtype, optional, structured in parse_args(args):
> +for argname, argtype, optional in parse_args(args):
>  argtype = c_type(argtype, is_param=True)
>  if optional:
>  arglist += "bool has_%s, " % c_var(argname)
> @@ -53,7 +53,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
>  retval=""
>  if ret_type:
>  retval = "retval = "
> -for argname, argtype, optional, structured in parse_args(args):
> +for argname, argtype, optional in parse_args(args):
>  if optional:
>  arglist += "has_%s, " % c_var(argname)
>  arglist += "%s, " % (c_var(argname))
> @@ -96,7 +96,7 @@ Visitor *v;
>  def gen_visitor_input_vars_decl(args):
>  ret = ""
>  push_indent()
> -for argname, argtype, optional, structured in parse_args(args):
> +for argname, argtype, optional in parse_args(args):
>  if optional:
>  ret += mcgen('''
>  bool has_%(argname)s = false;
> @@ -139,7 +139,7 @@ v = qapi_dealloc_get_visitor(md);
>  v = qmp_input_get_visitor(mi);
>  ''')
>
> -for argname, argtype, optional, structured in parse_args(args):
> +for argname, argtype, optional in parse_args(args):
>  if optional:
>  ret += mcgen('''
>  visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 601e307..47dc041 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -21,7 +21,7 @@ def _generate_event_api_name(event_name, params):
>  l = len(api_name)
>
>  if params:
> -for argname, argentry, optional, structured in parse_args(params):
> +for argname, argentry, optional in parse_args(params):
>  if optional:
>  api_name += "bool has_%s,\n" % c_var(argname)
>  api_name += "".ljust(l)
> @@ -93,7 +93,7 @@ def generate_event_implement(api_name, event_name, params):
>  """,
>  event_name = event_name)
>
> -for argname, argentry, optional, structured in parse_args(params):
> +for argname, argentry, optional in parse_args(params):
>  if optional:
>  ret += mcgen("""
>  if (has_%(var)s) {
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 9c8d68c..4789528 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -63,18 +63,13 @@ typedef struct %(name)sList
>  def generate_struct_fields(members):
>  ret = ''
>
> -for argname, argentry, optional, structured in parse_args(members):
> +for argname, argentry, optional in parse_args(members):
>  if optional:
>  ret += mcgen('''
>  bool has_%(c_name)s;
>  ''',
>   c_name=c_var(argname))
> -if structured:
> -push_indent()
> -ret += generate_struct({ "field": argname, "data": argentry})
> -pop_indent()
> -else:
> -ret += mcgen('''
> +ret += mcgen('''
>  %(c_type)s %(c_name)s;
>  ''',
>   c_type=c_type(argentry), c_name=c_var(argname))
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 3e11089..10b08c6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -51,27 +51,6 @@ def generate_visit_struct_fields(name, field_prefix, 
> fn_pr

Re: [Qemu-devel] [v6 08/14] migration: Add the core code of multi-thread compression

[Juan Quintela]

Liang Li  wrote:
> Implement the core logic of the multiple thread compression. At this
> point, multiple thread compression can't co-work with xbzrle yet.
>
> Signed-off-by: Liang Li 
> Signed-off-by: Yang Zhang 


Coming back to here, as we have the full code.

> ---
>  arch_init.c | 184 
> +---
>  1 file changed, 177 insertions(+), 7 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 48cae22..9f63c0f 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -355,12 +355,33 @@ static DecompressParam *decomp_param;
>  static QemuThread *decompress_threads;
>  static uint8_t *compressed_data_buf;
>  
> +static int do_compress_ram_page(CompressParam *param);
> +
>  static void *do_data_compress(void *opaque)
>  {
> -while (!quit_comp_thread) {
> +CompressParam *param = opaque;
>  
> -/* To be done */
Change this line to

> +while (!quit_comp_thread) {

  while(true) {

> +qemu_mutex_lock(¶m->mutex);
> +/* Re-check the quit_comp_thread in case of
> + * terminate_compression_threads is called just before
> + * qemu_mutex_lock(¶m->mutex) and after
> + * while(!quit_comp_thread), re-check it here can make
> + * sure the compression thread terminate as expected.
> + */
Change this

> +while (!param->start && !quit_comp_thread) {

to

while (!param->start && !parm->quit) {

> +qemu_cond_wait(¶m->cond, ¶m->mutex);
> +}

And this

> +if (!quit_comp_thread) {

to

  if (!param->quit) {
> +do_compress_ram_page(param);
> +}

Take care here of exiting correctly of the loop.
Notice that the only case where we are not going to take the look is the
last iteration, so I think the optimization don't gives us nothing (in
this place), no?

> +param->start = false;
> +qemu_mutex_unlock(¶m->mutex);
>  
> +qemu_mutex_lock(comp_done_lock);
> +param->done = true;
> +qemu_cond_signal(comp_done_cond);
> +qemu_mutex_unlock(comp_done_lock);
>  }


>  
>  return NULL;
> @@ -368,9 +389,15 @@ static void *do_data_compress(void *opaque)
>  
>  static inline void terminate_compression_threads(void)
>  {
> -quit_comp_thread = true;
> +int idx, thread_count;
>  
> -/* To be done */
> +thread_count = migrate_compress_threads();
> +quit_comp_thread = true;


> +for (idx = 0; idx < thread_count; idx++) {
> +qemu_mutex_lock(&comp_param[idx].mutex);
Add this
   comp_param[idx].quit = true;


And for now on, quit_comp_thread is only used on migration_thread, so it
should be safe to use, no?

flush_compresed_data() is only ever called from the migration_thread, so
no lock there needed either.

> +qemu_cond_signal(&comp_param[idx].cond);
> +qemu_mutex_unlock(&comp_param[idx].mutex);
> +}
>  }
>  
>  void migrate_compress_threads_join(void)
> @@ -420,6 +447,7 @@ void migrate_compress_threads_create(void)
>   * it's ops to empty.
>   */
>  comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> +comp_param[i].done = true;
>  qemu_mutex_init(&comp_param[i].mutex);
>  qemu_cond_init(&comp_param[i].cond);
>  qemu_thread_create(compress_threads + i, "compress",
> @@ -829,6 +857,97 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, 
> ram_addr_t offset,
>  return pages;
>  }
>  
> +static int do_compress_ram_page(CompressParam *param)
> +{
> +int bytes_sent, blen;
> +uint8_t *p;
> +RAMBlock *block = param->block;
> +ram_addr_t offset = param->offset;
> +
> +p = memory_region_get_ram_ptr(block->mr) + (offset & TARGET_PAGE_MASK);
> +
> +bytes_sent = save_page_header(param->file, block, offset |
> +  RAM_SAVE_FLAG_COMPRESS_PAGE);
> +blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
> + migrate_compress_level());
> +bytes_sent += blen;
> +atomic_inc(&acct_info.norm_pages);
> +
> +return bytes_sent;
> +}
> +
> +static inline void start_compression(CompressParam *param)
> +{
> +param->done = false;
> +qemu_mutex_lock(¶m->mutex);
> +param->start = true;
> +qemu_cond_signal(¶m->cond);
> +qemu_mutex_unlock(¶m->mutex);
> +}
> +
> +
> +static uint64_t bytes_transferred;
> +
> +static void flush_compressed_data(QEMUFile *f)
> +{
> +int idx, len, thread_count;
> +
> +if (!migrate_use_compression()) {
> +return;
> +}
> +thread_count = migrate_compress_threads();
> +for (idx = 0; idx < thread_count; idx++) {
> +if (!comp_param[idx].done) {
> +qemu_mutex_lock(comp_done_lock);
> +while (!comp_param[idx].done && !quit_comp_thread) {
> +qemu_cond_wait(comp_done_cond, comp_done_lock);
> +}
> +qemu_mutex_unlock(comp_done_lock);
> +}
> +if (!quit_com

[Qemu-devel] [PATCH 00/12 v9] tilegx: Firstly add tilegx feature for linux-user

[Chen Gang]

After load elf64 binary, qemu tilegx can finish executing the first
system call (uname) successfully in  _dl_discover_osversion(), and
return to __libc_start_main().

Chen Gang (12):
  linux-user: tilegx: Firstly add architecture related features
  linux-user: tilegx: Add target features support within qemu
  linux-user: Support tilegx architecture in syscall
  linux-user: Support tilegx architecture in linux-user
  linux-user/syscall.c: conditionalize syscalls which are not defined in
tilegx
  target-tilegx: Add cpu basic features for linux-user
  target-tilegx: Add helper features for linux-user
  target-tilegx: Add opcode basic implementation for tilegx
  target-tilegx: Finish processing bundle and preparing decoding pipes
  target-tilegx: Add TILE-Gx building files
  target-tilegx: Decoding pipes to support finish running 1st system
call
  target-tilegx: Generate tcg instructions to execute to 1st system call

 configure |3 +
 default-configs/tilegx-linux-user.mak |1 +
 include/elf.h |2 +
 linux-user/elfload.c  |   23 +
 linux-user/main.c |   86 ++
 linux-user/syscall.c  |   50 +-
 linux-user/syscall_defs.h |   38 +-
 linux-user/tilegx/syscall.h   |   80 ++
 linux-user/tilegx/syscall_nr.h|  278 ++
 linux-user/tilegx/target_cpu.h|   35 +
 linux-user/tilegx/target_signal.h |   28 +
 linux-user/tilegx/target_structs.h|   48 +
 linux-user/tilegx/termbits.h  |  285 ++
 target-tilegx/Makefile.objs   |1 +
 target-tilegx/cpu-qom.h   |   73 ++
 target-tilegx/cpu.c   |  149 +++
 target-tilegx/cpu.h   |   94 ++
 target-tilegx/helper.c|   31 +
 target-tilegx/helper.h|1 +
 target-tilegx/opcode_tilegx.h | 1406 ++
 target-tilegx/translate.c | 1762 +
 21 files changed, 4469 insertions(+), 5 deletions(-)
 create mode 100644 default-configs/tilegx-linux-user.mak
 create mode 100644 linux-user/tilegx/syscall.h
 create mode 100644 linux-user/tilegx/syscall_nr.h
 create mode 100644 linux-user/tilegx/target_cpu.h
 create mode 100644 linux-user/tilegx/target_signal.h
 create mode 100644 linux-user/tilegx/target_structs.h
 create mode 100644 linux-user/tilegx/termbits.h
 create mode 100644 target-tilegx/Makefile.objs
 create mode 100644 target-tilegx/cpu-qom.h
 create mode 100644 target-tilegx/cpu.c
 create mode 100644 target-tilegx/cpu.h
 create mode 100644 target-tilegx/helper.c
 create mode 100644 target-tilegx/helper.h
 create mode 100644 target-tilegx/opcode_tilegx.h
 create mode 100644 target-tilegx/translate.c

-- 
1.9.3

Re: [Qemu-devel] [PATCH v5 17/45] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages.

[Dr. David Alan Gilbert]

* David Gibson (da...@gibson.dropbear.id.au) wrote:
> On Thu, Mar 26, 2015 at 04:33:28PM +, Dr. David Alan Gilbert wrote:
> > (Only replying to some of the items in this mail - the others I'll get
> > to another time).
> > 
> > * David Gibson (da...@gibson.dropbear.id.au) wrote:
> > > On Wed, Feb 25, 2015 at 04:51:40PM +, Dr. David Alan Gilbert (git) 
> > > wrote:
> > > > From: "Dr. David Alan Gilbert" 
> > > > 
> > > > The state of the postcopy process is managed via a series of messages;
> > > >* Add wrappers and handlers for sending/receiving these messages
> > > >* Add state variable that track the current state of postcopy
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert 
> > > > ---
> > > >  include/migration/migration.h |  15 ++
> > > >  include/sysemu/sysemu.h   |  23 +++
> > > >  migration/migration.c |  13 ++
> > > >  savevm.c  | 325 
> > > > ++
> > > >  trace-events  |  11 ++
> > > >  5 files changed, 387 insertions(+)
> > > > 
> > > > diff --git a/include/migration/migration.h 
> > > > b/include/migration/migration.h
> > > > index f94af5b..81cd1f2 100644
> > > > --- a/include/migration/migration.h
> > > > +++ b/include/migration/migration.h
> > > > @@ -52,6 +52,14 @@ typedef struct MigrationState MigrationState;
> > > >  
> > > >  typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head;
> > > >  
> > > > +typedef enum {
> > > > +POSTCOPY_INCOMING_NONE = 0,  /* Initial state - no postcopy */
> > > > +POSTCOPY_INCOMING_ADVISE,
> > > > +POSTCOPY_INCOMING_LISTENING,
> > > > +POSTCOPY_INCOMING_RUNNING,
> > > > +POSTCOPY_INCOMING_END
> > > > +} PostcopyState;
> > > > +
> > > >  /* State for the incoming migration */
> > > >  struct MigrationIncomingState {
> > > >  QEMUFile *file;
> > > > @@ -59,6 +67,8 @@ struct MigrationIncomingState {
> > > >  /* See savevm.c */
> > > >  LoadStateEntry_Head loadvm_handlers;
> > > >  
> > > > +PostcopyState postcopy_state;
> > > > +
> > > >  QEMUFile *return_path;
> > > >  QemuMutex  rp_mutex;/* We send replies from multiple 
> > > > threads */
> > > >  };
> > > > @@ -219,4 +229,9 @@ size_t ram_control_save_page(QEMUFile *f, 
> > > > ram_addr_t block_offset,
> > > >   ram_addr_t offset, size_t size,
> > > >   int *bytes_sent);
> > > >  
> > > > +PostcopyState postcopy_state_get(MigrationIncomingState *mis);
> > > > +
> > > > +/* Set the state and return the old state */
> > > > +PostcopyState postcopy_state_set(MigrationIncomingState *mis,
> > > > + PostcopyState new_state);
> > > >  #endif
> > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > > > index 8da879f..d6a6d51 100644
> > > > --- a/include/sysemu/sysemu.h
> > > > +++ b/include/sysemu/sysemu.h
> > > > @@ -87,6 +87,18 @@ enum qemu_vm_cmd {
> > > >  MIG_CMD_INVALID = 0,   /* Must be 0 */
> > > >  MIG_CMD_OPEN_RETURN_PATH,  /* Tell the dest to open the Return 
> > > > path */
> > > >  MIG_CMD_PING,  /* Request a PONG on the RP */
> > > > +
> > > > +MIG_CMD_POSTCOPY_ADVISE = 20,  /* Prior to any page transfers, just
> > > > +  warn we might want to do PC */
> > > > +MIG_CMD_POSTCOPY_LISTEN,   /* Start listening for incoming
> > > > +  pages as it's running. */
> > > > +MIG_CMD_POSTCOPY_RUN,  /* Start execution */
> > > > +MIG_CMD_POSTCOPY_END,  /* Postcopy is finished. */
> > > > +
> > > > +MIG_CMD_POSTCOPY_RAM_DISCARD,  /* A list of pages to discard that
> > > > +  were previously sent during
> > > > +  precopy but are dirty. */
> > > > +
> > > >  };
> > > >  
> > > >  bool qemu_savevm_state_blocked(Error **errp);
> > > > @@ -101,6 +113,17 @@ void qemu_savevm_command_send(QEMUFile *f, enum 
> > > > qemu_vm_cmd command,
> > > >uint16_t len, uint8_t *data);
> > > >  void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
> > > >  void qemu_savevm_send_open_return_path(QEMUFile *f);
> > > > +void qemu_savevm_send_postcopy_advise(QEMUFile *f);
> > > > +void qemu_savevm_send_postcopy_listen(QEMUFile *f);
> > > > +void qemu_savevm_send_postcopy_run(QEMUFile *f);
> > > > +void qemu_savevm_send_postcopy_end(QEMUFile *f, uint8_t status);
> > > > +
> > > > +void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char 
> > > > *name,
> > > > +   uint16_t len, uint8_t 
> > > > offset,
> > > > +   uint64_t *addrlist,
> > > > +   uint32_t *masklist);
> > > > +
> > > > +
> > > >  int qemu_loadvm_state(QEMUFile *f);
> > > >  
> > > >  /* SLIRP */
> > > > diff --git a/migration/migration.c b/migr

[Qemu-devel] [PATCH 01/12 v9] linux-user: tilegx: Firstly add architecture related features

[Chen Gang]

They are based on Linux kernel tilegx architecture for 64 bit binary,
also based on tilegx ABI reference document.

Signed-off-by: Chen Gang 
---
 linux-user/tilegx/syscall.h|  80 
 linux-user/tilegx/syscall_nr.h | 278 
 linux-user/tilegx/termbits.h   | 285 +
 3 files changed, 643 insertions(+)
 create mode 100644 linux-user/tilegx/syscall.h
 create mode 100644 linux-user/tilegx/syscall_nr.h
 create mode 100644 linux-user/tilegx/termbits.h

diff --git a/linux-user/tilegx/syscall.h b/linux-user/tilegx/syscall.h
new file mode 100644
index 000..561e158
--- /dev/null
+++ b/linux-user/tilegx/syscall.h
@@ -0,0 +1,80 @@
+#ifndef TILEGX_SYSCALLS_H
+#define TILEGX_SYSCALLS_H
+
+#define UNAME_MACHINE "tilegx"
+#define UNAME_MINIMUM_RELEASE "3.19"
+
+/* We use tilegx to keep things similar to the kernel sources.  */
+typedef uint64_t tilegx_reg_t;
+
+struct target_pt_regs {
+
+/* Can be as parameters */
+tilegx_reg_t r0;  /* Also for return value, both function and system call 
*/
+tilegx_reg_t r1;
+tilegx_reg_t r2;
+tilegx_reg_t r3;
+tilegx_reg_t r4;
+tilegx_reg_t r5;
+tilegx_reg_t r6;
+tilegx_reg_t r7;
+tilegx_reg_t r8;
+tilegx_reg_t r9;
+
+/* Normal using, caller saved */
+tilegx_reg_t r10;  /* Also for system call */
+tilegx_reg_t r11;
+tilegx_reg_t r12;
+tilegx_reg_t r13;
+tilegx_reg_t r14;
+tilegx_reg_t r15;
+tilegx_reg_t r16;
+tilegx_reg_t r17;
+tilegx_reg_t r18;
+tilegx_reg_t r19;
+tilegx_reg_t r20;
+tilegx_reg_t r21;
+tilegx_reg_t r22;
+tilegx_reg_t r23;
+tilegx_reg_t r24;
+tilegx_reg_t r25;
+tilegx_reg_t r26;
+tilegx_reg_t r27;
+tilegx_reg_t r28;
+tilegx_reg_t r29;
+
+/* Normal using, callee saved */
+tilegx_reg_t r30;
+tilegx_reg_t r31;
+tilegx_reg_t r32;
+tilegx_reg_t r33;
+tilegx_reg_t r34;
+tilegx_reg_t r35;
+tilegx_reg_t r36;
+tilegx_reg_t r37;
+tilegx_reg_t r38;
+tilegx_reg_t r39;
+tilegx_reg_t r40;
+tilegx_reg_t r41;
+tilegx_reg_t r42;
+tilegx_reg_t r43;
+tilegx_reg_t r44;
+tilegx_reg_t r45;
+tilegx_reg_t r46;
+tilegx_reg_t r47;
+tilegx_reg_t r48;
+tilegx_reg_t r49;
+tilegx_reg_t r50;
+tilegx_reg_t r51;
+
+/* Control using */
+tilegx_reg_t r52;/* optional frame pointer */
+tilegx_reg_t tp; /* thread-local data */
+tilegx_reg_t sp; /* stack pointer */
+tilegx_reg_t lr; /* lr pointer */
+};
+
+#define TARGET_MLOCKALL_MCL_CURRENT 1
+#define TARGET_MLOCKALL_MCL_FUTURE  2
+
+#endif
diff --git a/linux-user/tilegx/syscall_nr.h b/linux-user/tilegx/syscall_nr.h
new file mode 100644
index 000..8121154
--- /dev/null
+++ b/linux-user/tilegx/syscall_nr.h
@@ -0,0 +1,278 @@
+#ifndef TILEGX_SYSCALL_NR
+#define TILEGX_SYSCALL_NR
+
+/*
+ * Copy from linux kernel asm-generic/unistd.h, which tilegx uses.
+ */
+#define TARGET_NR_io_setup  0
+#define TARGET_NR_io_destroy1
+#define TARGET_NR_io_submit 2
+#define TARGET_NR_io_cancel 3
+#define TARGET_NR_io_getevents  4
+#define TARGET_NR_setxattr  5
+#define TARGET_NR_lsetxattr 6
+#define TARGET_NR_fsetxattr 7
+#define TARGET_NR_getxattr  8
+#define TARGET_NR_lgetxattr 9
+#define TARGET_NR_fgetxattr 10
+#define TARGET_NR_listxattr 11
+#define TARGET_NR_llistxattr12
+#define TARGET_NR_flistxattr13
+#define TARGET_NR_removexattr   14
+#define TARGET_NR_lremovexattr  15
+#define TARGET_NR_fremovexattr  16
+#define TARGET_NR_getcwd17
+#define TARGET_NR_lookup_dcookie18
+#define TARGET_NR_eventfd2  19
+#define TARGET_NR_epoll_create1 20
+#define TARGET_NR_epoll_ctl 21
+#define TARGET_NR_epoll_pwait   22
+#define TARGET_NR_dup   23
+#define TARGET_NR_dup3  24
+#define TARGET_NR_fcntl 25
+#define TARGET_NR_inotify_init1 26
+#define TARGET_NR_inotify_add_watch 27
+#define TARGET_NR_inotify_rm_watch  28
+#define TARGET_NR_ioctl 29
+#define TARGET_NR_ioprio_set30
+#define TARGET_NR_ioprio_get31
+#define TARGET_NR_flock 32
+#define TARGET_NR_mknodat   33
+#define TARGET_NR_mkdirat   34
+#define TARGET_NR_unlinkat  35
+#define TARGET_NR_symlinkat 36
+#define TARGET_NR_linkat37
+#define TARGET_NR_renameat   

[Qemu-devel] [PATCH 03/12 v9] linux-user: Support tilegx architecture in syscall

[Chen Gang]

Add tilegx architecture in "syscall_defs.h", all related features (ioctrl,
and stat) are based on Linux kernel tilegx 64-bit implementation.

Signed-off-by: Chen Gang 
---
 linux-user/syscall_defs.h | 38 ++
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index edd5f3c..023f4b5 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -64,8 +64,9 @@
 #endif
 
 #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_SH4) \
-|| defined(TARGET_M68K) || defined(TARGET_CRIS) || 
defined(TARGET_UNICORE32) \
-|| defined(TARGET_S390X) || defined(TARGET_OPENRISC)
+|| defined(TARGET_M68K) || defined(TARGET_CRIS) \
+|| defined(TARGET_UNICORE32) || defined(TARGET_S390X) \
+|| defined(TARGET_OPENRISC) || defined(TARGET_TILEGX)
 
 #define TARGET_IOC_SIZEBITS14
 #define TARGET_IOC_DIRBITS 2
@@ -365,7 +366,8 @@ int do_sigaction(int sig, const struct target_sigaction 
*act,
 || defined(TARGET_PPC) || defined(TARGET_MIPS) || defined(TARGET_SH4) \
 || defined(TARGET_M68K) || defined(TARGET_ALPHA) || defined(TARGET_CRIS) \
 || defined(TARGET_MICROBLAZE) || defined(TARGET_UNICORE32) \
-|| defined(TARGET_S390X) || defined(TARGET_OPENRISC)
+|| defined(TARGET_S390X) || defined(TARGET_OPENRISC) \
+|| defined(TARGET_TILEGX)
 
 #if defined(TARGET_SPARC)
 #define TARGET_SA_NOCLDSTOP8u
@@ -1922,6 +1924,32 @@ struct target_stat64 {
 unsigned int __unused5;
 };
 
+#elif defined(TARGET_TILEGX)
+
+/* Copy from Linux kernel "uapi/asm-generic/stat.h" */
+struct target_stat {
+abi_ulong st_dev;   /* Device.  */
+abi_ulong st_ino;   /* File serial number.  */
+unsigned int st_mode;   /* File mode.  */
+unsigned int st_nlink;  /* Link count.  */
+unsigned int st_uid;/* User ID of the file's owner.  */
+unsigned int st_gid;/* Group ID of the file's group. */
+abi_ulong st_rdev;  /* Device number, if device.  */
+abi_ulong __pad1;
+abi_long  st_size;  /* Size of file, in bytes.  */
+int st_blksize; /* Optimal block size for I/O.  */
+int __pad2;
+abi_long st_blocks; /* Number 512-byte blocks allocated. */
+abi_long target_st_atime;   /* Time of last access.  */
+abi_ulong target_st_atime_nsec;
+abi_long target_st_mtime;   /* Time of last modification.  */
+abi_ulong target_st_mtime_nsec;
+abi_long target_st_ctime;   /* Time of last status change.  */
+abi_ulong target_st_ctime_nsec;
+unsigned int __unused4;
+unsigned int __unused5;
+};
+
 #else
 #error unsupported CPU
 #endif
@@ -2264,7 +2292,9 @@ struct target_flock {
 struct target_flock64 {
short  l_type;
short  l_whence;
-#if defined(TARGET_PPC) || defined(TARGET_X86_64) || defined(TARGET_MIPS) || 
defined(TARGET_SPARC) || defined(TARGET_HPPA) || defined (TARGET_MICROBLAZE)
+#if defined(TARGET_PPC) || defined(TARGET_X86_64) || defined(TARGET_MIPS) \
+|| defined(TARGET_SPARC) || defined(TARGET_HPPA) \
+|| defined(TARGET_MICROBLAZE) || defined(TARGET_TILEGX)
 int __pad;
 #endif
unsigned long long l_start;
-- 
1.9.3

[Qemu-devel] [PATCH 02/12 v9] linux-user: tilegx: Add target features support within qemu

[Chen Gang]

They are for target features within qemu which independent from outside.

Signed-off-by: Chen Gang 
---
 linux-user/tilegx/target_cpu.h | 35 +++
 linux-user/tilegx/target_signal.h  | 28 ++
 linux-user/tilegx/target_structs.h | 48 ++
 3 files changed, 111 insertions(+)
 create mode 100644 linux-user/tilegx/target_cpu.h
 create mode 100644 linux-user/tilegx/target_signal.h
 create mode 100644 linux-user/tilegx/target_structs.h

diff --git a/linux-user/tilegx/target_cpu.h b/linux-user/tilegx/target_cpu.h
new file mode 100644
index 000..c96e81d
--- /dev/null
+++ b/linux-user/tilegx/target_cpu.h
@@ -0,0 +1,35 @@
+/*
+ * TILE-Gx specific CPU ABI and functions for linux-user
+ *
+ * Copyright (c) 2015 Chen Gang
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+#ifndef TARGET_CPU_H
+#define TARGET_CPU_H
+
+static inline void cpu_clone_regs(CPUTLGState *env, target_ulong newsp)
+{
+if (newsp) {
+env->regs[TILEGX_R_SP] = newsp;
+}
+env->regs[TILEGX_R_RE] = 0;
+}
+
+static inline void cpu_set_tls(CPUTLGState *env, target_ulong newtls)
+{
+env->regs[TILEGX_R_TP] = newtls;
+}
+
+#endif
diff --git a/linux-user/tilegx/target_signal.h 
b/linux-user/tilegx/target_signal.h
new file mode 100644
index 000..fbab216
--- /dev/null
+++ b/linux-user/tilegx/target_signal.h
@@ -0,0 +1,28 @@
+#ifndef TARGET_SIGNAL_H
+#define TARGET_SIGNAL_H
+
+#include "cpu.h"
+
+/* this struct defines a stack used during syscall handling */
+
+typedef struct target_sigaltstack {
+abi_ulong ss_sp;
+abi_ulong ss_size;
+abi_long ss_flags;
+} target_stack_t;
+
+/*
+ * sigaltstack controls
+ */
+#define TARGET_SS_ONSTACK 1
+#define TARGET_SS_DISABLE 2
+
+#define TARGET_MINSIGSTKSZ2048
+#define TARGET_SIGSTKSZ   8192
+
+static inline abi_ulong get_sp_from_cpustate(CPUTLGState *state)
+{
+return state->regs[TILEGX_R_SP];
+}
+
+#endif /* TARGET_SIGNAL_H */
diff --git a/linux-user/tilegx/target_structs.h 
b/linux-user/tilegx/target_structs.h
new file mode 100644
index 000..13a1505
--- /dev/null
+++ b/linux-user/tilegx/target_structs.h
@@ -0,0 +1,48 @@
+/*
+ * TILE-Gx specific structures for linux-user
+ *
+ * Copyright (c) 2015 Chen Gang
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ */
+#ifndef TARGET_STRUCTS_H
+#define TARGET_STRUCTS_H
+
+struct target_ipc_perm {
+abi_int __key;  /* Key.  */
+abi_uint uid;   /* Owner's user ID.  */
+abi_uint gid;   /* Owner's group ID.  */
+abi_uint cuid;  /* Creator's user ID.  */
+abi_uint cgid;  /* Creator's group ID.  */
+abi_uint mode;/* Read/write permission.  */
+abi_ushort __seq;   /* Sequence number.  */
+abi_ushort __pad2;
+abi_ulong __unused1;
+abi_ulong __unused2;
+};
+
+struct target_shmid_ds {
+struct target_ipc_perm shm_perm;/* operation permission struct */
+abi_long shm_segsz; /* size of segment in bytes */
+abi_ulong shm_atime;/* time of last shmat() */
+abi_ulong shm_dtime;/* time of last shmdt() */
+abi_ulong shm_ctime;/* time of last change by shmctl() */
+abi_int shm_cpid;   /* pid of creator */
+abi_int shm_lpid;   /* pid of last shmop */
+abi_ulong shm_nattch;   /* number of current attaches */
+abi_ulong __unused4;
+abi_ulong __unused5;
+};
+
+#endif
-- 
1.9.3

Re: [Qemu-devel] [Migration Bug? ] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

[Juan Quintela]

zhanghailiang  wrote:
> On 2015/3/26 11:52, Li Zhijian wrote:
>> On 03/26/2015 11:12 AM, Wen Congyang wrote:
>>> On 03/25/2015 05:50 PM, Juan Quintela wrote:
 zhanghailiang  wrote:
> Hi all,
>
> We found that, sometimes, the content of VM's memory is
> inconsistent between Source side and Destination side
> when we check it just after finishing migration but before VM continue to 
> Run.
>
> We use a patch like bellow to find this issue, you can find it from affix,
> and Steps to reprduce:
>
> (1) Compile QEMU:
>   ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && 
> make
>
> (2) Command and output:
> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
> qemu64,-kvmclock -netdev tap,id=hn0-device
> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
> -device
> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
> -monitor stdio
 Could you try to reproduce:
 - without vhost
 - without virtio-net
 - cache=unsafe is going to give you trouble, but trouble should only
happen after migration of pages have finished.
>>> If I use ide disk, it doesn't happen.
>>> Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
>>> it is because I migrate the guest when it is booting. The virtio net
>>> device is not used in this case.
>> Er～～
>> it reproduces in my ide disk
>> there is no any virtio device, my command line like below
>>
>> x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu qemu64,-kvmclock -net none
>> -boot c -drive file=/home/lizj/ubuntu.raw -vnc :7 -m 2048 -smp 2 -machine
>> usb=off -no-user-config -nodefaults -monitor stdio -vga std
>>
>> it seems easily to reproduce this issue by following steps in _ubuntu_ guest
>> 1.  in source side, choose memtest in grub
>> 2. do live migration
>> 3. exit memtest(type Esc in when memory testing)
>> 4. wait migration complete
>>
>
> Yes，it is a thorny problem. It is indeed easy to reproduce, just as
> your steps in the above.

Thanks for the test case.  I will try to give a try on Monday.  Now that
we have a test case, it should be able to instrument things.  As the
problem is on memtest, it can't be the disk, clearly :p

Later, Juan.


>
> This is my test result: (I also test accel=tcg, it can be reproduced also.)
> Source side:
> # x86_64-softmmu/qemu-system-x86_64 -machine
> pc-i440fx-2.3,accel=kvm,usb=off -no-user-config -nodefaults -cpu
> qemu64,-kvmclock -boot c -drive
> file=/mnt/sdb/pure_IMG/ubuntu/ubuntu_14.04_server_64_2U_raw -device
> cirrus-vga,id=video0,vgamem_mb=8 -vnc :7 -m 2048 -smp 2 -monitor stdio
> (qemu) ACPI_BUILD: init ACPI tables
> ACPI_BUILD: init ACPI tables
> migrate tcp:9.61.1.8:3004
> ACPI_BUILD: init ACPI tables
> before cpu_synchronize_all_states
> 5a8f72d66732cac80d6a0d5713654c0e
> md_host : before saving ram complete
> 5a8f72d66732cac80d6a0d5713654c0e
> md_host : after saving ram complete
> 5a8f72d66732cac80d6a0d5713654c0e
> (qemu)
>
> Destination side:
> # x86_64-softmmu/qemu-system-x86_64 -machine
> pc-i440fx-2.3,accel=kvm,usb=off -no-user-config -nodefaults -cpu
> qemu64,-kvmclock -boot c -drive
> file=/mnt/sdb/pure_IMG/ubuntu/ubuntu_14.04_server_64_2U_raw -device
> cirrus-vga,id=video0,vgamem_mb=8 -vnc :7 -m 2048 -smp 2 -monitor stdio
> -incoming tcp:0:3004
> (qemu) QEMU_VM_SECTION_END, after loading ram
> d7cb0d8a4bdd1557fb0e78baee50c986
> md_host : after loading all vmstate
> d7cb0d8a4bdd1557fb0e78baee50c986
> md_host : after cpu_synchronize_all_post_init
> d7cb0d8a4bdd1557fb0e78baee50c986
>
>
> Thanks,
> zhang
>
>>>
 What kind of load were you having when reproducing this issue?
 Just to confirm, you have been able to reproduce this without COLO
 patches, right?

> (qemu) migrate tcp:192.168.3.8:3004
> before saving ram complete
> ff703f6889ab8701e4e040872d079a28
> md_host : after saving ram complete
> ff703f6889ab8701e4e040872d079a28
>
> DST: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
> qemu64,-kvmclock -netdev tap,id=hn0,vhost=on -device
> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
> -device
> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
> -monitor stdio -incoming tcp:0:3004
> (qemu) QEMU_VM_SECTION_END, after loading ram
> 230e1e68ece9cd4e769630e1bcb5ddfb
> md_host : after loading all vmstate
> 230e1e68ece9cd4e769630e1bcb5ddfb
> md_host : after cpu_synchronize_all_post_init
> 230e1e68ece9cd4e769630e1bcb5ddfb
>
> This happens occasionally, and it is more easy to reproduce when
> issu

[Qemu-devel] [PATCH 05/12 v9] linux-user/syscall.c: conditionalize syscalls which are not defined in tilegx

[Chen Gang]

For tilegx, several syscall macros are not supported, so switch them to
avoid building break.

Signed-off-by: Chen Gang 
---
 linux-user/syscall.c | 50 +-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5720195..d1a00ad 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -213,7 +213,7 @@ static int gettid(void) {
 return -ENOSYS;
 }
 #endif
-#ifdef __NR_getdents
+#if defined(TARGET_NR_getdents) && defined(__NR_getdents)
 _syscall3(int, sys_getdents, uint, fd, struct linux_dirent *, dirp, uint, 
count);
 #endif
 #if !defined(__NR_getdents) || \
@@ -5580,6 +5580,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 ret = get_errno(write(arg1, p, arg3));
 unlock_user(p, arg2, 0);
 break;
+#ifdef TARGET_NR_open
 case TARGET_NR_open:
 if (!(p = lock_user_string(arg1)))
 goto efault;
@@ -5588,6 +5589,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
   arg3));
 unlock_user(p, arg1, 0);
 break;
+#endif
 case TARGET_NR_openat:
 if (!(p = lock_user_string(arg2)))
 goto efault;
@@ -5602,9 +5604,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 case TARGET_NR_brk:
 ret = do_brk(arg1);
 break;
+#ifdef TARGET_NR_fork
 case TARGET_NR_fork:
 ret = get_errno(do_fork(cpu_env, SIGCHLD, 0, 0, 0, 0));
 break;
+#endif
 #ifdef TARGET_NR_waitpid
 case TARGET_NR_waitpid:
 {
@@ -5639,6 +5643,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 unlock_user(p, arg1, 0);
 break;
 #endif
+#ifdef TARGET_NR_link
 case TARGET_NR_link:
 {
 void * p2;
@@ -5652,6 +5657,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 unlock_user(p, arg1, 0);
 }
 break;
+#endif
 #if defined(TARGET_NR_linkat)
 case TARGET_NR_linkat:
 {
@@ -5669,12 +5675,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 }
 break;
 #endif
+#ifdef TARGET_NR_unlink
 case TARGET_NR_unlink:
 if (!(p = lock_user_string(arg1)))
 goto efault;
 ret = get_errno(unlink(p));
 unlock_user(p, arg1, 0);
 break;
+#endif
 #if defined(TARGET_NR_unlinkat)
 case TARGET_NR_unlinkat:
 if (!(p = lock_user_string(arg2)))
@@ -5791,12 +5799,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 }
 break;
 #endif
+#ifdef TARGET_NR_mknod
 case TARGET_NR_mknod:
 if (!(p = lock_user_string(arg1)))
 goto efault;
 ret = get_errno(mknod(p, arg2, arg3));
 unlock_user(p, arg1, 0);
 break;
+#endif
 #if defined(TARGET_NR_mknodat)
 case TARGET_NR_mknodat:
 if (!(p = lock_user_string(arg2)))
@@ -5805,12 +5815,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 unlock_user(p, arg2, 0);
 break;
 #endif
+#ifdef TARGET_NR_chmod
 case TARGET_NR_chmod:
 if (!(p = lock_user_string(arg1)))
 goto efault;
 ret = get_errno(chmod(p, arg2));
 unlock_user(p, arg1, 0);
 break;
+#endif
 #ifdef TARGET_NR_break
 case TARGET_NR_break:
 goto unimplemented;
@@ -5945,6 +5957,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 }
 break;
 #endif
+#ifdef TARGET_NR_utimes
 case TARGET_NR_utimes:
 {
 struct timeval *tvp, tv[2];
@@ -5963,6 +5976,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 unlock_user(p, arg1, 0);
 }
 break;
+#endif
 #if defined(TARGET_NR_futimesat)
 case TARGET_NR_futimesat:
 {
@@ -5991,12 +6005,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 case TARGET_NR_gtty:
 goto unimplemented;
 #endif
+#ifdef TARGET_NR_access
 case TARGET_NR_access:
 if (!(p = lock_user_string(arg1)))
 goto efault;
 ret = get_errno(access(path(p), arg2));
 unlock_user(p, arg1, 0);
 break;
+#endif
 #if defined(TARGET_NR_faccessat) && defined(__NR_faccessat)
 case TARGET_NR_faccessat:
 if (!(p = lock_user_string(arg2)))
@@ -6021,6 +6037,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 case TARGET_NR_kill:
 ret = get_errno(kill(arg1, target_to_host_signal(arg2)));
 break;
+#ifdef TARGET_NR_rename
 case TARGET_NR_rename:
 {
 void *p2;
@@ -6034,6 +6051,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 unlock_user(p, arg1, 0);
 }
 break;
+#endif
 #if defined(TARGET_NR_renameat)
 case TARGET_NR_renameat:
 {
@@ -6049,12 +6067,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
 }
 break;
 #endif
+#ifdef TARG

[Qemu-devel] [PATCH 07/12 v9] target-tilegx: Add helper features for linux-user

[Chen Gang]

For supporting linux-user system call, tilegx need support exception
helper features for it.

Signed-off-by: Chen Gang 
---
 target-tilegx/helper.c | 31 +++
 target-tilegx/helper.h |  1 +
 2 files changed, 32 insertions(+)
 create mode 100644 target-tilegx/helper.c
 create mode 100644 target-tilegx/helper.h

diff --git a/target-tilegx/helper.c b/target-tilegx/helper.c
new file mode 100644
index 000..ffac2b9
--- /dev/null
+++ b/target-tilegx/helper.c
@@ -0,0 +1,31 @@
+/*
+ * QEMU TILE-Gx helpers
+ *
+ *  Copyright (c) 2015 Chen Gang
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+#include "cpu.h"
+#include "qemu-common.h"
+#include "exec/helper-proto.h"
+
+void helper_exception(CPUTLGState *env, uint32_t excp)
+{
+CPUState *cs = CPU(tilegx_env_get_cpu(env));
+
+cs->exception_index = excp;
+cpu_loop_exit(cs);
+}
diff --git a/target-tilegx/helper.h b/target-tilegx/helper.h
new file mode 100644
index 000..36d1cd9
--- /dev/null
+++ b/target-tilegx/helper.h
@@ -0,0 +1 @@
+DEF_HELPER_2(exception, noreturn, env, i32)
-- 
1.9.3

[Qemu-devel] [PATCH 08/12 v9] target-tilegx: Add opcode basic implementation for tilegx

[Chen Gang]

It is from Tilera Corporation, and copied from Linux kernel "arch/tile/
include/uapi/arch/opcode_tilegx.h".

Signed-off-by: Chen Gang 
---
 target-tilegx/opcode_tilegx.h | 1406 +
 1 file changed, 1406 insertions(+)
 create mode 100644 target-tilegx/opcode_tilegx.h

diff --git a/target-tilegx/opcode_tilegx.h b/target-tilegx/opcode_tilegx.h
new file mode 100644
index 000..d76ff2d
--- /dev/null
+++ b/target-tilegx/opcode_tilegx.h
@@ -0,0 +1,1406 @@
+/* TILE-Gx opcode information.
+ *
+ * Copyright 2011 Tilera Corporation. All Rights Reserved.
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful, but
+ *   WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ *   NON INFRINGEMENT.  See the GNU General Public License for
+ *   more details.
+ *
+ *
+ *
+ *
+ *
+ */
+
+#ifndef __ARCH_OPCODE_H__
+#define __ARCH_OPCODE_H__
+
+#ifndef __ASSEMBLER__
+
+typedef unsigned long long tilegx_bundle_bits;
+
+/* These are the bits that determine if a bundle is in the X encoding. */
+#define TILEGX_BUNDLE_MODE_MASK ((tilegx_bundle_bits)3 << 62)
+
+enum
+{
+  /* Maximum number of instructions in a bundle (2 for X, 3 for Y). */
+  TILEGX_MAX_INSTRUCTIONS_PER_BUNDLE = 3,
+
+  /* How many different pipeline encodings are there? X0, X1, Y0, Y1, Y2. */
+  TILEGX_NUM_PIPELINE_ENCODINGS = 5,
+
+  /* Log base 2 of TILEGX_BUNDLE_SIZE_IN_BYTES. */
+  TILEGX_LOG2_BUNDLE_SIZE_IN_BYTES = 3,
+
+  /* Instructions take this many bytes. */
+  TILEGX_BUNDLE_SIZE_IN_BYTES = 1 << TILEGX_LOG2_BUNDLE_SIZE_IN_BYTES,
+
+  /* Log base 2 of TILEGX_BUNDLE_ALIGNMENT_IN_BYTES. */
+  TILEGX_LOG2_BUNDLE_ALIGNMENT_IN_BYTES = 3,
+
+  /* Bundles should be aligned modulo this number of bytes. */
+  TILEGX_BUNDLE_ALIGNMENT_IN_BYTES =
+(1 << TILEGX_LOG2_BUNDLE_ALIGNMENT_IN_BYTES),
+
+  /* Number of registers (some are magic, such as network I/O). */
+  TILEGX_NUM_REGISTERS = 64,
+};
+
+/* Make a few "tile_" variables to simplify common code between
+   architectures.  */
+
+typedef tilegx_bundle_bits tile_bundle_bits;
+#define TILE_BUNDLE_SIZE_IN_BYTES TILEGX_BUNDLE_SIZE_IN_BYTES
+#define TILE_BUNDLE_ALIGNMENT_IN_BYTES TILEGX_BUNDLE_ALIGNMENT_IN_BYTES
+#define TILE_LOG2_BUNDLE_ALIGNMENT_IN_BYTES \
+  TILEGX_LOG2_BUNDLE_ALIGNMENT_IN_BYTES
+#define TILE_BPT_BUNDLE TILEGX_BPT_BUNDLE
+
+/* 64-bit pattern for a { bpt ; nop } bundle. */
+#define TILEGX_BPT_BUNDLE 0x286a44ae51485000ULL
+
+static __inline unsigned int
+get_BFEnd_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 12)) & 0x3f);
+}
+
+static __inline unsigned int
+get_BFOpcodeExtension_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 24)) & 0xf);
+}
+
+static __inline unsigned int
+get_BFStart_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 18)) & 0x3f);
+}
+
+static __inline unsigned int
+get_BrOff_X1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 31)) & 0x003f) |
+ (((unsigned int)(n >> 37)) & 0x0001ffc0);
+}
+
+static __inline unsigned int
+get_BrType_X1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 54)) & 0x1f);
+}
+
+static __inline unsigned int
+get_Dest_Imm8_X1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 31)) & 0x003f) |
+ (((unsigned int)(n >> 43)) & 0x00c0);
+}
+
+static __inline unsigned int
+get_Dest_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 0)) & 0x3f);
+}
+
+static __inline unsigned int
+get_Dest_X1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 31)) & 0x3f);
+}
+
+static __inline unsigned int
+get_Dest_Y0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 0)) & 0x3f);
+}
+
+static __inline unsigned int
+get_Dest_Y1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 31)) & 0x3f);
+}
+
+static __inline unsigned int
+get_Imm16_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 12)) & 0x);
+}
+
+static __inline unsigned int
+get_Imm16_X1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 43)) & 0x);
+}
+
+static __inline unsigned int
+get_Imm8OpcodeExtension_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 20)) & 0xff);
+}
+
+static __inline unsigned int
+get_Imm8OpcodeExtension_X1(tilegx_bundle_bits n)
+{
+  return (((unsigned int)(n >> 51)) & 0xff);
+}
+
+static __inline unsigned int
+get_Imm8_X0(tilegx_bundle_bits num)
+{
+  const unsigned int n = (unsigned int)num;
+  return (((n >> 12)) & 0xff);
+}
+
+static __inline unsigned int
+get_Imm8_X1(tilegx_bundle_b

[Qemu-devel] [PATCH 09/12 v9] target-tilegx: Finish processing bundle and preparing decoding pipes

[Chen Gang]

Finish processing tilegx bundle, and reach to related pipes.

Signed-off-by: Chen Gang 
---
 target-tilegx/translate.c | 515 ++
 1 file changed, 515 insertions(+)
 create mode 100644 target-tilegx/translate.c

diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
new file mode 100644
index 000..0ac7e11
--- /dev/null
+++ b/target-tilegx/translate.c
@@ -0,0 +1,515 @@
+/*
+ * QEMU TILE-Gx CPU
+ *
+ *  Copyright (c) 2015 Chen Gang
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+#include "cpu.h"
+#include "qemu/log.h"
+#include "disas/disas.h"
+#include "tcg-op.h"
+#include "exec/cpu_ldst.h"
+#include "opcode_tilegx.h"
+
+#define FMT64X  "%016" PRIx64
+
+#define TILEGX_OPCODE_MAX_X0164  /* include 164 */
+#define TILEGX_OPCODE_MAX_X1107  /* include 107 */
+#define TILEGX_OPCODE_MAX_Y0 15  /* include 15 */
+#define TILEGX_OPCODE_MAX_Y1 15  /* include 15 */
+#define TILEGX_OPCODE_MAX_Y2  3  /* include 3 */
+
+static TCGv_ptr cpu_env;
+static TCGv cpu_pc;
+static TCGv cpu_regs[TILEGX_R_COUNT];
+
+static const char * const reg_names[] = {
+ "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
+ "r8",  "r9", "r10", "r11", "r12", "r13", "r14", "r15",
+"r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
+"r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
+"r32", "r33", "r34", "r35", "r36", "r37", "r38", "r39",
+"r40", "r41", "r42", "r43", "r44", "r45", "r46", "r47",
+"r48", "r49", "r50", "r51",  "bp",  "tp",  "sp",  "lr"
+};
+
+/* It is for temporary registers */
+typedef struct DisasContextTemp {
+uint8_t idx;   /* index */
+TCGv val;  /* value */
+} DisasContextTemp;
+
+/* This is the state at translation time.  */
+typedef struct DisasContext {
+uint64_t pc;   /* Current pc */
+uint64_t exception;/* Current exception */
+
+TCGv zero; /* For zero register */
+
+DisasContextTemp *tmp_regcur;  /* Current temporary registers */
+DisasContextTemp tmp_regs[TILEGX_MAX_INSTRUCTIONS_PER_BUNDLE];
+   /* All temporary registers */
+struct {
+TCGCond cond;  /* Branch condition */
+TCGv dest; /* pc jump destination, if will jump */
+TCGv val1; /* Firt value for condition comparing */
+TCGv val2; /* Second value for condition comparing */
+} jmp; /* Jump object, only once in each TB block 
*/
+} DisasContext;
+
+#include "exec/gen-icount.h"
+
+static void gen_exception(DisasContext *dc, int num)
+{
+TCGv_i32 tmp = tcg_const_i32(num);
+
+gen_helper_exception(cpu_env, tmp);
+tcg_temp_free_i32(tmp);
+}
+
+static void decode_addi_opcode_y0(struct DisasContext *dc,
+  tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_rrr_1_opcode_y0(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_rrr_5_opcode_y0(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_addi_opcode_y1(struct DisasContext *dc,
+  tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_rrr_1_opcode_y1(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_rrr_5_opcode_y1(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_ldst0_opcode_y2(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_ldst1_opcode_y2(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_ldst2_opcode_y2(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_ldst3_opcode_y2(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+static void decode_addli_opcode_x0(struct DisasContext *dc,
+   tilegx_bundle_bits bundle)
+{
+}
+
+stat

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Peter Maydell]

On 16 March 2015 at 17:20, Peter Maydell  wrote:
> Define an API so that devices can register MemoryRegionOps whose read
> and write callback functions are passed an arbitrary pointer to some
> transaction attributes and can return a success-or-failure status code.
> This will allow us to model devices which:
>  * behave differently for ARM Secure/NonSecure memory accesses
>  * behave differently for privileged/unprivileged accesses
>  * may return a transaction failure (causing a guest exception)
>for erroneous accesses

> The success/failure response indication is currently ignored; it is
> provided so we can implement it later without having to change the
> callback function API yet again in future.

> +/* New-style MMIO accessors can indicate that the transaction failed.
> + * This is currently ignored, but provided in the API to allow us to add
> + * support later without changing the MemoryRegionOps functions yet again.
> + */
> +typedef enum {
> +MemTx_OK = 0,
> +MemTx_DecodeError = 1, /* "nothing at that address" */
> +MemTx_SlaveError = 2,  /* "device unhappy with request" (eg 
> misalignment) */
> +} MemTxResult;

So I was looking at how this would actually get plumbed through
the memory subsystem code, and there are some awkwardnesses
with this simple enum approach. In particular, functions like
address_space_rw want to combine the error returns from
several io_mem_read/write calls into a single response to
return to the caller. With an enum we'd need some pretty
ugly code to prioritise particular failure types, or to
do something arbitrary like "return first failure code".
Alternatively we could:
(a) make MemTxResult a uint32_t, where all-bits zero indicates
"OK" and any bit set indicates some kind of error, eg
bit 0 set for "device returned an error", and bit 1 for
"decode error", and higher bits available for other kinds
of extra info about errors in future. Then address_space_rw
just ORs together all the bits in all the return codes it
receives.
(b) give up and say "just use a bool"

Opinions?

-- PMM

[Qemu-devel] [PATCH 10/12 v9] target-tilegx: Add TILE-Gx building files

[Chen Gang]

Add related configuration, make files for tilegx. Now, qemu tilegx can
pass building.

Signed-off-by: Chen Gang 
---
 configure | 3 +++
 default-configs/tilegx-linux-user.mak | 1 +
 target-tilegx/Makefile.objs   | 1 +
 3 files changed, 5 insertions(+)
 create mode 100644 default-configs/tilegx-linux-user.mak
 create mode 100644 target-tilegx/Makefile.objs

diff --git a/configure b/configure
index 589798e..a8704ec 100755
--- a/configure
+++ b/configure
@@ -5215,6 +5215,9 @@ case "$target_name" in
   s390x)
 gdb_xml_files="s390x-core64.xml s390-acr.xml s390-fpr.xml"
   ;;
+  tilegx)
+TARGET_ARCH=tilegx
+  ;;
   unicore32)
   ;;
   xtensa|xtensaeb)
diff --git a/default-configs/tilegx-linux-user.mak 
b/default-configs/tilegx-linux-user.mak
new file mode 100644
index 000..3e47493
--- /dev/null
+++ b/default-configs/tilegx-linux-user.mak
@@ -0,0 +1 @@
+# Default configuration for tilegx-linux-user
diff --git a/target-tilegx/Makefile.objs b/target-tilegx/Makefile.objs
new file mode 100644
index 000..8b3dc76
--- /dev/null
+++ b/target-tilegx/Makefile.objs
@@ -0,0 +1 @@
+obj-y += cpu.o translate.o helper.o
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/5] qapi: Clean up documentation of alternate mappings

[Markus Armbruster]

Eric Blake  writes:

> QObject is an internal coding concept and requires the reader to
> reverse engineer the mapping; it is nicer to be explicit and call
> out specific JSON types.
>
> Signed-off-by: Eric Blake 
> ---
>  docs/qapi-code-gen.txt | 24 ++--
>  1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 1f7d0ca..7fb0db7 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -331,17 +331,21 @@ Resulting in this JSON object:
>
>  Usage: { 'alternate: 'str', 'data': 'dict' }
>
> -An alternate type is one that allows a choice between two or more
> -QObject data types (string, integer, number, or dictionary, but not
> -array) on the wire.  The definition is similar to a simple union type,
> -where each branch of the dictionary names a type, and where an
> -implicit C enum NameKind is created for the alternate Name.  But
> +An alternate type is one that allows a choice between two or more JSON
> +data types on the wire.  The definition is similar to a simple union
> +type, where each branch of the dictionary names a QAPI type, and where
> +an implicit C enum NameKind is created for the alternate Name.  But
>  unlike a union, the discriminator string is never passed on the wire
> -for QMP, instead appearing only in the generated C code.  The type on
> -the wire serves an implicit discriminator, which in turn means that an
> -alternate can express a choice between a string and a single complex
> -type (passed as a dictionary), but cannot distinguish between two
> -different complex types.  For example:
> +for QMP, instead appearing only in the generated C code.  Rather, the
> +first byte of the value on the wire serves an implicit discriminator:

I think "first byte of" is implementation detail, and should be left
out.

> +if the branch is typed as the 'bool' built-in, it accepts true and
> +false; if it is typed as any of the various numeric built-ins, it
> +accepts a JSON number; if it is typed as a 'str' built-in or named
> +enum types it accepts a JSON string, and if it is typed as a named
> +struct or union type it accepts a JSON object.  Thus, an alternate can
> +express a choice between a string and a single complex type (passed as
> +a dictionary), but cannot distinguish between two different complex
> +types or two different numeric built-in types.  For example:
>
>   { 'alternate': 'BlockRef',
> 'data': { 'definition': 'BlockdevOptions',

[Qemu-devel] [PATCH 11/12 v9] target-tilegx: Decoding pipes to support finish running 1st system call

[Chen Gang]

For the instructions which need tcg generation, the decoding functions
return directly, or they will direct to the exception.

Signed-off-by: Chen Gang 
---
 target-tilegx/translate.c | 704 ++
 1 file changed, 704 insertions(+)

diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
index 0ac7e11..dada275 100644
--- a/target-tilegx/translate.c
+++ b/target-tilegx/translate.c
@@ -89,11 +89,56 @@ static void decode_addi_opcode_y0(struct DisasContext *dc,
 static void decode_rrr_1_opcode_y0(struct DisasContext *dc,
tilegx_bundle_bits bundle)
 {
+switch (get_RRROpcodeExtension_Y0(bundle)) {
+case UNARY_RRR_1_OPCODE_Y0:
+switch (get_UnaryOpcodeExtension_Y0(bundle)) {
+case NOP_UNARY_OPCODE_Y0:
+case  FNOP_UNARY_OPCODE_Y0:
+if (!get_SrcA_Y0(bundle) && !get_Dest_Y0(bundle)) {
+return;
+}
+break;
+case CNTLZ_UNARY_OPCODE_Y0:
+case CNTTZ_UNARY_OPCODE_Y0:
+case FSINGLE_PACK1_UNARY_OPCODE_Y0:
+case PCNT_UNARY_OPCODE_Y0:
+case REVBITS_UNARY_OPCODE_Y0:
+case REVBYTES_UNARY_OPCODE_Y0:
+case TBLIDXB0_UNARY_OPCODE_Y0:
+case TBLIDXB1_UNARY_OPCODE_Y0:
+case TBLIDXB2_UNARY_OPCODE_Y0:
+case TBLIDXB3_UNARY_OPCODE_Y0:
+default:
+break;
+}
+break;
+case SHL1ADD_RRR_1_OPCODE_Y0:
+case SHL2ADD_RRR_1_OPCODE_Y0:
+case SHL3ADD_RRR_1_OPCODE_Y0:
+default:
+break;
+}
+
+qemu_log_mask(LOG_UNIMP, "UNIMP rrr_1_opcode_y0, [" FMT64X "]\n",
+  (uint64_t)bundle);
+dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
 }
 
 static void decode_rrr_5_opcode_y0(struct DisasContext *dc,
tilegx_bundle_bits bundle)
 {
+switch (get_RRROpcodeExtension_Y0(bundle)) {
+case OR_RRR_5_OPCODE_Y0:
+return;
+case AND_RRR_5_OPCODE_Y0:
+case NOR_RRR_5_OPCODE_Y0:
+case XOR_RRR_5_OPCODE_Y0:
+default:
+break;
+}
+qemu_log_mask(LOG_UNIMP, "UNIMP rrr_5_opcode_y0, [" FMT64X "]\n",
+  (uint64_t)bundle);
+dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
 }
 
 static void decode_addi_opcode_y1(struct DisasContext *dc,
@@ -104,31 +149,116 @@ static void decode_addi_opcode_y1(struct DisasContext 
*dc,
 static void decode_rrr_1_opcode_y1(struct DisasContext *dc,
tilegx_bundle_bits bundle)
 {
+switch (get_RRROpcodeExtension_Y1(bundle)) {
+case UNARY_RRR_1_OPCODE_Y1:
+switch (get_UnaryOpcodeExtension_Y1(bundle)) {
+case NOP_UNARY_OPCODE_Y1:
+case FNOP_UNARY_OPCODE_Y1:
+if (!get_SrcA_Y1(bundle) && !get_Dest_Y1(bundle)) {
+return;
+}
+break;
+case ILL_UNARY_OPCODE_Y1:
+case JALRP_UNARY_OPCODE_Y1:
+case JALR_UNARY_OPCODE_Y1:
+case JRP_UNARY_OPCODE_Y1:
+case JR_UNARY_OPCODE_Y1:
+case LNK_UNARY_OPCODE_Y1:
+default:
+break;
+}
+break;
+case SHL1ADD_RRR_1_OPCODE_Y1:
+case SHL2ADD_RRR_1_OPCODE_Y1:
+case SHL3ADD_RRR_1_OPCODE_Y1:
+default:
+break;
+}
+qemu_log_mask(LOG_UNIMP, "UNIMP rrr_1_opcode_y1, [" FMT64X "]\n",
+  (uint64_t)bundle);
+dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
 }
 
 static void decode_rrr_5_opcode_y1(struct DisasContext *dc,
tilegx_bundle_bits bundle)
 {
+switch (get_RRROpcodeExtension_Y1(bundle)) {
+case OR_RRR_5_OPCODE_Y1:
+return;
+case AND_RRR_5_OPCODE_Y1:
+case NOR_RRR_5_OPCODE_Y1:
+case XOR_RRR_5_OPCODE_Y1:
+default:
+break;
+}
+qemu_log_mask(LOG_UNIMP, "UNIMP rrr_5_opcode_y1, [" FMT64X "]\n",
+  (uint64_t)bundle);
+dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
 }
 
 static void decode_ldst0_opcode_y2(struct DisasContext *dc,
tilegx_bundle_bits bundle)
 {
+switch (get_Mode(bundle)) {
+case MODE_OPCODE_YA2:
+return;
+case MODE_OPCODE_YB2:
+case MODE_OPCODE_YC2:
+default:
+break;
+}
+qemu_log_mask(LOG_UNIMP, "UNIMP ldst0_opcode_y2, [" FMT64X "]\n",
+  (uint64_t)bundle);
+dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
 }
 
 static void decode_ldst1_opcode_y2(struct DisasContext *dc,
tilegx_bundle_bits bundle)
 {
+switch (get_Mode(bundle)) {
+case MODE_OPCODE_YB2:
+return;
+case MODE_OPCODE_YA2:
+case MODE_OPCODE_YC2:
+default:
+break;
+}
+qemu_log_mask(LOG_UNIMP, "UNIMP ldst1_opcode_y2, [" FMT64X "]\n",
+  (uint64_t)bundle);
+dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
 }
 
 static void decode_ldst2_opcode_y2(struct DisasContext *dc,
   

Re: [Qemu-devel] [PATCH 0/5] qapi doc cleanups

[Markus Armbruster]

Eric Blake  writes:

> Markus had some nice review comments on my earlier patch:
> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05310.html
>
> Depends on the full series "[PATCH v5 00/28] drop qapi nested structs",
> although if I have to respin that, I'll probably just squash the
> approved portions of this series directly into those patches rather
> than respinning this series.  Broken into several patches, to try and
> logically group the changes, and so that it is easier to pick and
> choose whether to take all of the changes or drop some as overkill.

With the small touch up to 2/5 I asked for:

Reviewed-by: Markus Armbruster 

[Qemu-devel] [PATCH 12/12 v9] target-tilegx: Generate tcg instructions to execute to 1st system call

[Chen Gang]

Generate related tcg instructions, and qemu tilegx can run to 1st system
call (uname) successfully in  _dl_discover_osversion(), and return to
__libc_start_main().

Signed-off-by: Chen Gang 
---
 target-tilegx/translate.c | 543 ++
 1 file changed, 543 insertions(+)

diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
index dada275..8274d8b 100644
--- a/target-tilegx/translate.c
+++ b/target-tilegx/translate.c
@@ -73,6 +73,32 @@ typedef struct DisasContext {
 
 #include "exec/gen-icount.h"
 
+static TCGv load_zero(DisasContext *dc)
+{
+if (TCGV_IS_UNUSED_I64(dc->zero)) {
+dc->zero = tcg_const_i64(0);
+}
+return dc->zero;
+}
+
+static TCGv load_gr(DisasContext *dc, uint8_t reg)
+{
+if (likely(reg < TILEGX_R_COUNT)) {
+return cpu_regs[reg];
+} else if (reg != TILEGX_R_ZERO) {
+dc->exception = TILEGX_EXCP_REG_UNSUPPORTED;
+}
+return load_zero(dc);
+}
+
+static TCGv dest_gr(DisasContext *dc, uint8_t rdst)
+{
+DisasContextTemp *tmp = dc->tmp_regcur;
+tmp->idx = rdst;
+tmp->val = tcg_temp_new_i64();
+return tmp->val;
+}
+
 static void gen_exception(DisasContext *dc, int num)
 {
 TCGv_i32 tmp = tcg_const_i32(num);
@@ -81,9 +107,402 @@ static void gen_exception(DisasContext *dc, int num)
 tcg_temp_free_i32(tmp);
 }
 
+static void gen_fnop(void)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "(f)nop\n");
+}
+
+static void gen_cmpltui(struct DisasContext *dc,
+uint8_t rdst, uint8_t rsrc, int8_t imm8)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpltui r%d, r%d, %d\n",
+  rdst, rsrc, imm8);
+tcg_gen_setcondi_i64(TCG_COND_LTU, dest_gr(dc, rdst), load_gr(dc, rsrc),
+(uint64_t)imm8);
+}
+
+static void gen_cmpeqi(struct DisasContext *dc,
+   uint8_t rdst, uint8_t rsrc, int8_t imm8)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpeqi r%d, r%d, %d\n", rdst, rsrc, 
imm8);
+tcg_gen_setcondi_i64(TCG_COND_EQ, dest_gr(dc, rdst), load_gr(dc, rsrc),
+(uint64_t)imm8);
+}
+
+static void gen_cmpne(struct DisasContext *dc,
+  uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpne r%d, r%d, r%d\n",
+  rdst, rsrc, rsrcb);
+tcg_gen_setcond_i64(TCG_COND_NE, dest_gr(dc, rdst), load_gr(dc, rsrc),
+load_gr(dc, rsrcb));
+}
+
+static void gen_cmoveqz(struct DisasContext *dc,
+uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmoveqz r%d, r%d, r%d\n",
+  rdst, rsrc, rsrcb);
+tcg_gen_movcond_i64(TCG_COND_EQ, dest_gr(dc, rdst), load_gr(dc, rsrc),
+load_zero(dc), load_gr(dc, rsrcb), load_gr(dc, rdst));
+}
+
+static void gen_cmovnez(struct DisasContext *dc,
+uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmovnez r%d, r%d, r%d\n",
+  rdst, rsrc, rsrcb);
+tcg_gen_movcond_i64(TCG_COND_NE, dest_gr(dc, rdst), load_gr(dc, rsrc),
+load_zero(dc), load_gr(dc, rsrcb), load_gr(dc, rdst));
+}
+
+static void gen_add(struct DisasContext *dc,
+uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "add r%d, r%d, r%d\n",
+  rdst, rsrc, rsrcb);
+tcg_gen_add_i64(dest_gr(dc, rdst), load_gr(dc, rsrc), load_gr(dc, rsrcb));
+}
+
+static void gen_addimm(struct DisasContext *dc,
+   uint8_t rdst, uint8_t rsrc, int64_t imm)
+{
+tcg_gen_addi_i64(dest_gr(dc, rdst), load_gr(dc, rsrc), imm);
+}
+
+static void gen_addi(struct DisasContext *dc,
+ uint8_t rdst, uint8_t rsrc, int8_t imm8)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "addi r%d, r%d, %d\n", rdst, rsrc, imm8);
+gen_addimm(dc, rdst, rsrc, (int64_t)imm8);
+}
+
+static void gen_addli(struct DisasContext *dc,
+  uint8_t rdst, uint8_t rsrc, int16_t im16)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "addli r%d, r%d, %d\n", rdst, rsrc, im16);
+gen_addimm(dc, rdst, rsrc, (int64_t)im16);
+}
+
+static void gen_addx(struct DisasContext *dc,
+ uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
+{
+TCGv vdst = dest_gr(dc, rdst);
+
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "addx r%d, r%d, r%d\n", rdst, rsrc, 
rsrcb);
+tcg_gen_add_i64(vdst, load_gr(dc, rsrc), load_gr(dc, rsrcb));
+tcg_gen_ext32s_i64(vdst, vdst);
+}
+
+static void gen_addximm(struct DisasContext *dc,
+uint8_t rdst, uint8_t rsrc, int32_t imm)
+{
+TCGv vdst = dest_gr(dc, rdst);
+
+tcg_gen_addi_i64(vdst, load_gr(dc, rsrc), imm);
+tcg_gen_ext32s_i64(vdst, vdst);
+}
+
+static void gen_addxi(struct DisasContext *dc,
+ uint8_t rdst, uint8_t rsrc, int8_t imm8)
+{
+qemu_log_mask(CPU_LOG_TB_IN_ASM, "addxi r%d, r%d, %d\n", rdst, 

[Qemu-devel] [PATCH 06/12 v9] target-tilegx: Add cpu basic features for linux-user

[Chen Gang]

It implements minimized cpu features for linux-user.

Signed-off-by: Chen Gang 
---
 target-tilegx/cpu-qom.h |  73 
 target-tilegx/cpu.c | 149 
 target-tilegx/cpu.h |  94 ++
 3 files changed, 316 insertions(+)
 create mode 100644 target-tilegx/cpu-qom.h
 create mode 100644 target-tilegx/cpu.c
 create mode 100644 target-tilegx/cpu.h

diff --git a/target-tilegx/cpu-qom.h b/target-tilegx/cpu-qom.h
new file mode 100644
index 000..5615c3b
--- /dev/null
+++ b/target-tilegx/cpu-qom.h
@@ -0,0 +1,73 @@
+/*
+ * QEMU TILE-Gx CPU
+ *
+ * Copyright (c) 2015 Chen Gang
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+#ifndef QEMU_TILEGX_CPU_QOM_H
+#define QEMU_TILEGX_CPU_QOM_H
+
+#include "qom/cpu.h"
+
+#define TYPE_TILEGX_CPU "tilegx-cpu"
+
+#define TILEGX_CPU_CLASS(klass) \
+OBJECT_CLASS_CHECK(TileGXCPUClass, (klass), TYPE_TILEGX_CPU)
+#define TILEGX_CPU(obj) \
+OBJECT_CHECK(TileGXCPU, (obj), TYPE_TILEGX_CPU)
+#define TILEGX_CPU_GET_CLASS(obj) \
+OBJECT_GET_CLASS(TileGXCPUClass, (obj), TYPE_TILEGX_CPU)
+
+/**
+ * TileGXCPUClass:
+ * @parent_realize: The parent class' realize handler.
+ * @parent_reset: The parent class' reset handler.
+ *
+ * A Tile-Gx CPU model.
+ */
+typedef struct TileGXCPUClass {
+/*< private >*/
+CPUClass parent_class;
+/*< public >*/
+
+DeviceRealize parent_realize;
+void (*parent_reset)(CPUState *cpu);
+} TileGXCPUClass;
+
+/**
+ * TileGXCPU:
+ * @env: #CPUTLGState
+ *
+ * A Tile-GX CPU.
+ */
+typedef struct TileGXCPU {
+/*< private >*/
+CPUState parent_obj;
+/*< public >*/
+
+CPUTLGState env;
+} TileGXCPU;
+
+static inline TileGXCPU *tilegx_env_get_cpu(CPUTLGState *env)
+{
+return container_of(env, TileGXCPU, env);
+}
+
+#define ENV_GET_CPU(e) CPU(tilegx_env_get_cpu(e))
+
+#define ENV_OFFSET offsetof(TileGXCPU, env)
+
+#endif
diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
new file mode 100644
index 000..8255fdc
--- /dev/null
+++ b/target-tilegx/cpu.c
@@ -0,0 +1,149 @@
+/*
+ * QEMU TILE-Gx CPU
+ *
+ *  Copyright (c) 2015 Chen Gang
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+#include "cpu.h"
+#include "qemu-common.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+TileGXCPU *cpu_tilegx_init(const char *cpu_model)
+{
+TileGXCPU *cpu;
+
+cpu = TILEGX_CPU(object_new(TYPE_TILEGX_CPU));
+
+object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
+
+return cpu;
+}
+
+static void tilegx_cpu_set_pc(CPUState *cs, vaddr value)
+{
+TileGXCPU *cpu = TILEGX_CPU(cs);
+
+cpu->env.pc = value;
+}
+
+static bool tilegx_cpu_has_work(CPUState *cs)
+{
+return true;
+}
+
+static void tilegx_cpu_reset(CPUState *s)
+{
+TileGXCPU *cpu = TILEGX_CPU(s);
+TileGXCPUClass *tcc = TILEGX_CPU_GET_CLASS(cpu);
+CPUTLGState *env = &cpu->env;
+
+tcc->parent_reset(s);
+
+memset(env, 0, sizeof(CPUTLGState));
+tlb_flush(s, 1);
+}
+
+static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+CPUState *cs = CPU(dev);
+TileGXCPUClass *tcc = TILEGX_CPU_GET_CLASS(dev);
+
+cpu_reset(cs);
+qemu_init_vcpu(cs);
+
+tcc->parent_realize(dev, errp);
+}
+
+static void tilegx_cpu_initfn(Object *obj)
+{
+CPUState *cs = CPU(obj);
+TileGXCPU *cpu = TILEGX_CPU(obj);
+CPUTLGState *env = &cpu->env;
+static bool tcg_initialized;
+
+cs->env_ptr = env;
+cpu_exec_init(env);
+
+if (tcg_enabled() && !tcg_initialized) {
+tcg_initialized = true;
+tilegx_tcg_init();
+}
+}
+
+static const VMStateDescription vmstate_tilegx_cpu = {
+.name = "c

[Qemu-devel] [PATCH 04/12 v9] linux-user: Support tilegx architecture in linux-user

[Chen Gang]

Add main working flow feature, system call processing feature, and elf64
tilegx binary loading feature, based on Linux kernel tilegx 64-bit
implementation.

Signed-off-by: Chen Gang 
---
 include/elf.h|  2 ++
 linux-user/elfload.c | 23 ++
 linux-user/main.c| 86 
 3 files changed, 111 insertions(+)

diff --git a/include/elf.h b/include/elf.h
index 3e75f05..154144e 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -133,6 +133,8 @@ typedef int64_t  Elf64_Sxword;
 
 #define EM_AARCH64  183
 
+#define EM_TILEGX   191 /* TILE-Gx */
+
 /* This is the info that is needed to parse the dynamic section of the file */
 #define DT_NULL0
 #define DT_NEEDED  1
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 399c021..2571cb8 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1189,6 +1189,29 @@ static inline void init_thread(struct target_pt_regs 
*regs, struct image_info *i
 
 #endif /* TARGET_S390X */
 
+#ifdef TARGET_TILEGX
+
+/* 42 bits real used address, a half for user mode */
+#define ELF_START_MMAP (0x00200ULL)
+
+#define elf_check_arch(x) ((x) == EM_TILEGX)
+
+#define ELF_CLASS   ELFCLASS64
+#define ELF_DATAELFDATA2LSB
+#define ELF_ARCHEM_TILEGX
+
+static inline void init_thread(struct target_pt_regs *regs,
+   struct image_info *infop)
+{
+regs->lr = infop->entry;
+regs->sp = infop->start_stack;
+
+}
+
+#define ELF_EXEC_PAGESIZE65536 /* TILE-Gx page size is 64KB */
+
+#endif /* TARGET_TILEGX */
+
 #ifndef ELF_PLATFORM
 #define ELF_PLATFORM (NULL)
 #endif
diff --git a/linux-user/main.c b/linux-user/main.c
index 6e446de..ecfc80b 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3418,6 +3418,32 @@ void cpu_loop(CPUS390XState *env)
 
 #endif /* TARGET_S390X */
 
+#ifdef TARGET_TILEGX
+void cpu_loop(CPUTLGState *env)
+{
+CPUState *cs = CPU(tilegx_env_get_cpu(env));
+int trapnr;
+
+while (1) {
+cpu_exec_start(cs);
+trapnr = cpu_tilegx_exec(env);
+cpu_exec_end(cs);
+switch (trapnr) {
+case TILEGX_EXCP_SYSCALL:
+env->regs[TILEGX_R_RE] = do_syscall(env, env->regs[TILEGX_R_NR],
+env->regs[0], env->regs[1],
+env->regs[2], env->regs[3],
+env->regs[4], env->regs[5],
+env->regs[6], env->regs[7]);
+break;
+default:
+exit(-1);
+}
+process_pending_signals(env);
+}
+}
+#endif
+
 THREAD CPUState *thread_cpu;
 
 void task_settid(TaskState *ts)
@@ -4392,6 +4418,66 @@ int main(int argc, char **argv, char **envp)
 env->psw.mask = regs->psw.mask;
 env->psw.addr = regs->psw.addr;
 }
+#elif defined(TARGET_TILEGX)
+{
+env->regs[0] = regs->r0;
+env->regs[1] = regs->r1;
+env->regs[2] = regs->r2;
+env->regs[3] = regs->r3;
+env->regs[4] = regs->r4;
+env->regs[5] = regs->r5;
+env->regs[6] = regs->r6;
+env->regs[7] = regs->r7;
+env->regs[8] = regs->r8;
+env->regs[9] = regs->r9;
+env->regs[10] = regs->r10;
+env->regs[11] = regs->r11;
+env->regs[12] = regs->r12;
+env->regs[13] = regs->r13;
+env->regs[14] = regs->r14;
+env->regs[15] = regs->r15;
+env->regs[16] = regs->r16;
+env->regs[17] = regs->r17;
+env->regs[18] = regs->r18;
+env->regs[19] = regs->r19;
+env->regs[20] = regs->r20;
+env->regs[21] = regs->r21;
+env->regs[22] = regs->r22;
+env->regs[23] = regs->r23;
+env->regs[24] = regs->r24;
+env->regs[25] = regs->r25;
+env->regs[26] = regs->r26;
+env->regs[27] = regs->r27;
+env->regs[28] = regs->r28;
+env->regs[29] = regs->r29;
+env->regs[30] = regs->r30;
+env->regs[31] = regs->r31;
+env->regs[32] = regs->r32;
+env->regs[33] = regs->r33;
+env->regs[34] = regs->r34;
+env->regs[35] = regs->r35;
+env->regs[36] = regs->r36;
+env->regs[37] = regs->r37;
+env->regs[38] = regs->r38;
+env->regs[39] = regs->r39;
+env->regs[40] = regs->r40;
+env->regs[41] = regs->r41;
+env->regs[42] = regs->r42;
+env->regs[43] = regs->r43;
+env->regs[44] = regs->r44;
+env->regs[45] = regs->r45;
+env->regs[46] = regs->r46;
+env->regs[47] = regs->r47;
+env->regs[48] = regs->r48;
+env->regs[49] = regs->r49;
+env->regs[50] = regs->r50;
+env->regs[51] = regs->r51;
+env->regs[52] = regs->r52; /* TILEGX_R_BP */
+env->regs[53] = regs->tp;  /* TILEGX_R_TP */
+env->regs[54] = regs->sp;  /* TILEGX_R_SP */
+env->regs[5

Re: [Qemu-devel] E5-2620v2 - emulation stop error

[Andrey Korolyov]

On Thu, Mar 26, 2015 at 11:40 PM, Radim Krčmář  wrote:
> 2015-03-26 21:24+0300, Andrey Korolyov:
>> On Thu, Mar 26, 2015 at 8:40 PM, Radim Krčmář  wrote:
>> > 2015-03-26 20:08+0300, Andrey Korolyov:
>> >> KVM internal error. Suberror: 2
>> >> extra data[0]: 80ef
>> >> extra data[1]: 8b0d
>> >
>> > Btw. does this part ever change?
>> >
>> > I see that first report had:
>> >
>> >   KVM internal error. Suberror: 2
>> >   extra data[0]: 80d1
>> >   extra data[1]: 8b0d
>> >
>> > Was that a Windows guest by any chance?
>>
>> Yes, exactly, different extra data output was from a Windows VMs.
>
> Windows uses vector 0xd1 for timer interrupts.
>
> I second Bandan -- checking that it reproduces on other machine would be
> great for sanity :)  (Although a bug in our APICv is far more likely.)

Trace with new bits:

KVM internal error. Suberror: 2
extra data[0]: 80ef
extra data[1]: 8b0d
extra data[2]: 77b
EAX= EBX= ECX= EDX=
ESI= EDI= EBP= ESP=6d24
EIP=d331 EFL=00010202 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =   9300
CS =f000 000f  9b00
SS =   9300
DS =   9300
FS =   9300
GS =   9300
LDT=   8200
TR =   8b00
GDT= 000f6cb0 0037
IDT=  03ff
CR0=0010 CR2= CR3= CR4=
DR0= DR1= DR2=
DR3=
DR6=0ff0 DR7=0400
EFER=
Code=66 c3 cd 02 cb cd 10 cb cd 13 cb cd 15 cb cd 16 cb cd 18 cb 
19 cb cd 1c cb cd 4a cb fa fc 66 ba 47 d3 0f 00 e9 ad fe f3 90 f0 0f
ba 2d d4 fe fb 3f

[Qemu-devel] [PULL for-2.3 0/1] Net patches

[Stefan Hajnoczi]

The following changes since commit 4ad9e2b36e1e00fe5b96c3448ecd673e11c4d6d8:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-gtk-20150326-1' into 
staging (2015-03-26 18:35:09 +)

are available in the git repository at:

  git://github.com/stefanha/qemu.git net-pull-request

for you to fetch changes up to 4e60a250d395ef0d04eb8b6489cc5f7615a8909b:

  hw/net/e1000: fix integer endianness (2015-03-27 10:23:50 +)





Shannon Zhao (1):
  hw/net/e1000: fix integer endianness

 hw/net/e1000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.1.0

[Qemu-devel] [PULL for-2.3 1/1] hw/net/e1000: fix integer endianness

[Stefan Hajnoczi]

From: Shannon Zhao 

It's detected by coverity.In is_vlan_packet s->mac_reg[VET] is
unsigned int but is dereferenced as a narrower unsigned short.
This may lead to unexpected results depending on machine
endianness.

Signed-off-by: Shannon Zhao 
Signed-off-by: Shannon Zhao 
Message-id: 1426224119-8352-1-git-send-email-zhaoshengl...@huawei.com
Signed-off-by: Stefan Hajnoczi 
---
 hw/net/e1000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 3405cb9..091d61a 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -578,7 +578,7 @@ static inline int
 is_vlan_packet(E1000State *s, const uint8_t *buf)
 {
 return (be16_to_cpup((uint16_t *)(buf + 12)) ==
-le16_to_cpup((uint16_t *)(s->mac_reg + VET)));
+le16_to_cpu(s->mac_reg[VET]));
 }
 
 static inline int
@@ -711,7 +711,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
 (tp->cptse || txd_lower & E1000_TXD_CMD_EOP)) {
 tp->vlan_needed = 1;
 stw_be_p(tp->vlan_header,
-  le16_to_cpup((uint16_t *)(s->mac_reg + VET)));
+  le16_to_cpu(s->mac_reg[VET]));
 stw_be_p(tp->vlan_header + 2,
   le16_to_cpu(dp->upper.fields.special));
 }
-- 
2.1.0

[Qemu-devel] [RESEND PULL for-2.3 1/1] hw/net/e1000: fix integer endianness

[Stefan Hajnoczi]

From: Shannon Zhao 

It's detected by coverity.In is_vlan_packet s->mac_reg[VET] is
unsigned int but is dereferenced as a narrower unsigned short.
This may lead to unexpected results depending on machine
endianness.

Signed-off-by: Shannon Zhao 
Signed-off-by: Shannon Zhao 
Message-id: 1426224119-8352-1-git-send-email-zhaoshengl...@huawei.com
Signed-off-by: Stefan Hajnoczi 
---
 hw/net/e1000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 3405cb9..091d61a 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -578,7 +578,7 @@ static inline int
 is_vlan_packet(E1000State *s, const uint8_t *buf)
 {
 return (be16_to_cpup((uint16_t *)(buf + 12)) ==
-le16_to_cpup((uint16_t *)(s->mac_reg + VET)));
+le16_to_cpu(s->mac_reg[VET]));
 }
 
 static inline int
@@ -711,7 +711,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
 (tp->cptse || txd_lower & E1000_TXD_CMD_EOP)) {
 tp->vlan_needed = 1;
 stw_be_p(tp->vlan_header,
-  le16_to_cpup((uint16_t *)(s->mac_reg + VET)));
+  le16_to_cpu(s->mac_reg[VET]));
 stw_be_p(tp->vlan_header + 2,
   le16_to_cpu(dp->upper.fields.special));
 }
-- 
2.1.0

[Qemu-devel] [RESEND PULL for-2.3 0/1] Net patches

[Stefan Hajnoczi]

The following changes since commit 4ad9e2b36e1e00fe5b96c3448ecd673e11c4d6d8:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-gtk-20150326-1' into 
staging (2015-03-26 18:35:09 +)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/net-pull-request

for you to fetch changes up to 4e60a250d395ef0d04eb8b6489cc5f7615a8909b:

  hw/net/e1000: fix integer endianness (2015-03-27 10:23:50 +)





Shannon Zhao (1):
  hw/net/e1000: fix integer endianness

 hw/net/e1000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.1.0

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 10:58:01AM +, Peter Maydell wrote:
> On 16 March 2015 at 17:20, Peter Maydell  wrote:
> > Define an API so that devices can register MemoryRegionOps whose read
> > and write callback functions are passed an arbitrary pointer to some
> > transaction attributes and can return a success-or-failure status code.
> > This will allow us to model devices which:
> >  * behave differently for ARM Secure/NonSecure memory accesses
> >  * behave differently for privileged/unprivileged accesses
> >  * may return a transaction failure (causing a guest exception)
> >for erroneous accesses
> 
> > The success/failure response indication is currently ignored; it is
> > provided so we can implement it later without having to change the
> > callback function API yet again in future.
> 
> > +/* New-style MMIO accessors can indicate that the transaction failed.
> > + * This is currently ignored, but provided in the API to allow us to add
> > + * support later without changing the MemoryRegionOps functions yet again.
> > + */
> > +typedef enum {
> > +MemTx_OK = 0,
> > +MemTx_DecodeError = 1, /* "nothing at that address" */
> > +MemTx_SlaveError = 2,  /* "device unhappy with request" (eg 
> > misalignment) */
> > +} MemTxResult;
> 
> So I was looking at how this would actually get plumbed through
> the memory subsystem code, and there are some awkwardnesses
> with this simple enum approach. In particular, functions like
> address_space_rw want to combine the error returns from
> several io_mem_read/write calls into a single response to
> return to the caller. With an enum we'd need some pretty
> ugly code to prioritise particular failure types, or to
> do something arbitrary like "return first failure code".
> Alternatively we could:
> (a) make MemTxResult a uint32_t, where all-bits zero indicates
> "OK" and any bit set indicates some kind of error, eg
> bit 0 set for "device returned an error", and bit 1 for
> "decode error", and higher bits available for other kinds
> of extra info about errors in future. Then address_space_rw
> just ORs together all the bits in all the return codes it
> receives.
> (b) give up and say "just use a bool"
> 
> Opinions?

Hi Peter,

Is this related to masters relying on the memory frameworks magic
handling of unaliged accesses?

I guess that masters that really care about accurate the error
handling would need to issue transactions without relying on
the intermediate "magic" that splits unaligned accesses...

Anyway, I think your option a sounds the most flexible...

Cheers,
Edgar

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Paolo Bonzini]

On 27/03/2015 13:02, Edgar E. Iglesias wrote:
> On Fri, Mar 27, 2015 at 10:58:01AM +, Peter Maydell wrote:
>> On 16 March 2015 at 17:20, Peter Maydell  wrote:
>>> Define an API so that devices can register MemoryRegionOps whose read
>>> and write callback functions are passed an arbitrary pointer to some
>>> transaction attributes and can return a success-or-failure status code.
>>> This will allow us to model devices which:
>>>  * behave differently for ARM Secure/NonSecure memory accesses
>>>  * behave differently for privileged/unprivileged accesses
>>>  * may return a transaction failure (causing a guest exception)
>>>for erroneous accesses
>>
>>> The success/failure response indication is currently ignored; it is
>>> provided so we can implement it later without having to change the
>>> callback function API yet again in future.
>>
>>> +/* New-style MMIO accessors can indicate that the transaction failed.
>>> + * This is currently ignored, but provided in the API to allow us to add
>>> + * support later without changing the MemoryRegionOps functions yet again.
>>> + */
>>> +typedef enum {
>>> +MemTx_OK = 0,
>>> +MemTx_DecodeError = 1, /* "nothing at that address" */
>>> +MemTx_SlaveError = 2,  /* "device unhappy with request" (eg 
>>> misalignment) */
>>> +} MemTxResult;
>>
>> So I was looking at how this would actually get plumbed through
>> the memory subsystem code, and there are some awkwardnesses
>> with this simple enum approach. In particular, functions like
>> address_space_rw want to combine the error returns from
>> several io_mem_read/write calls into a single response to
>> return to the caller. With an enum we'd need some pretty
>> ugly code to prioritise particular failure types, or to
>> do something arbitrary like "return first failure code".
>> Alternatively we could:
>> (a) make MemTxResult a uint32_t, where all-bits zero indicates
>> "OK" and any bit set indicates some kind of error, eg
>> bit 0 set for "device returned an error", and bit 1 for
>> "decode error", and higher bits available for other kinds
>> of extra info about errors in future. Then address_space_rw
>> just ORs together all the bits in all the return codes it
>> receives.
>> (b) give up and say "just use a bool"
>>
>> Opinions?
> 
> Hi Peter,
> 
> Is this related to masters relying on the memory frameworks magic
> handling of unaliged accesses?

Not necessarily, you can get the same just by doing a large write that
spans multiple MemoryRegions.  See the loop in address_space_rw.

> Anyway, I think your option a sounds the most flexible...

ACK

Paolo

> Cheers,
> Edgar
> 

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Peter Maydell]

On 27 March 2015 at 12:02, Edgar E. Iglesias  wrote:
> On Fri, Mar 27, 2015 at 10:58:01AM +, Peter Maydell wrote:
>> So I was looking at how this would actually get plumbed through
>> the memory subsystem code, and there are some awkwardnesses
>> with this simple enum approach. In particular, functions like
>> address_space_rw want to combine the error returns from
>> several io_mem_read/write calls into a single response to
>> return to the caller. With an enum we'd need some pretty
>> ugly code to prioritise particular failure types, or to
>> do something arbitrary like "return first failure code".
>> Alternatively we could:
>> (a) make MemTxResult a uint32_t, where all-bits zero indicates
>> "OK" and any bit set indicates some kind of error, eg
>> bit 0 set for "device returned an error", and bit 1 for
>> "decode error", and higher bits available for other kinds
>> of extra info about errors in future. Then address_space_rw
>> just ORs together all the bits in all the return codes it
>> receives.
>> (b) give up and say "just use a bool"

> Is this related to masters relying on the memory frameworks magic
> handling of unaliged accesses?

Well, that, and masters that just want to say "write
this entire buffer" or otherwise access at larger
than the destination's access size.

> I guess that masters that really care about accurate the error
> handling would need to issue transactions without relying on
> the intermediate "magic" that splits unaligned accesses...

Yes, I think this is probably true. (I suspect we don't
actually care at that level of detail.)

> Anyway, I think your option a sounds the most flexible...

Yes, it's the best thing I can think of currently.

-- PMM

Re: [Qemu-devel] [PULL for-2.3 0/4] Block patches

[Peter Maydell]

On 27 March 2015 at 10:20, Stefan Hajnoczi  wrote:
> The following changes since commit 4ad9e2b36e1e00fe5b96c3448ecd673e11c4d6d8:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-gtk-20150326-1' into 
> staging (2015-03-26 18:35:09 +)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to da2cf4e80355e42fbaeb8bcfa2b07f5bceddd323:
>
>   block: Document blockdev-add's immaturity (2015-03-27 10:01:12 +)
>
> 

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v5 06/28] qapi: Add some union tests

[Markus Armbruster]

Eric Blake  writes:

> On 03/26/2015 07:18 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Demonstrate that the qapi generator doesn't deal well with unions
>>> that aren't up to par. Later patches will update the expected
>>> reseults as the generator is made stricter.
>>>
>>> Of particular note, we currently allow 'base' without 'discriminator'
>>> as a way to create a simple union with a base class.  However, none
>>> of the existing QMP or QGA interfaces use it (it is exercised only
>>> in the testsuite).
>> 
>> qapi-code-gen.txt documents 'base' only for flat unions.  We should
>> either document its use in simple unions, or outlaw it.
>
> I went with outlaw later in the series, and the rest of my commit
> message was an attempt to explain my reasoning in that choice.  But I
> can certainly try to improve the wording, if we need a respin.

If you respin, I suggest to add that it's undocumented.

>> 
>>> Meanwhile, it would be nice to allow
>>> 'discriminator':'EnumType' without 'base' for creating a simple union
>>> that is type-safe rather than open-coded to a generated enum (right
>>> now, we are only type-safe when using a flat union, but that uses
>>> a different syntax of 'discriminator':'member-name' which requires
>>> a base class containing a 'member-name' enum field).
>> 
>> I'm afraid I don't get you.  Can you give examples?
>
> Using this common code with the appropriate union for each example:
> { 'command':'foo', 'data':UNION }
>
> Right now, we have flat unions which are required to be type-safe (all
> branches MUST map back to the enum type of the discriminator, enforced
> by the generator, so that if the enum later adds a member, the union
> must also be updated to match):
>
> [1]
> { 'union':'Safe', 'base':'Base', 'discriminator':'switch',
>   'data':{ 'one':'str', 'two':'int' }}
>
> {"execute":"foo", "arguments":{'switch':'one', 'data':'hello'}}

As far as I can tell, the generator rejects flat union 'data' members
that aren't discriminator values.  It doesn't require the union to cover
all discriminator members.  Makes sense to me, because the union may not
have additional data for some discriminator values.

Your claim "so that if the enum later adds a member, the union must also
be updated to match" appears to be wrong.

> and simple unions which cannot be typesafe (the branches of the union
> are open-coded - even if they correlate to an existing enum, there is
> nothing enforcing that extensions to the enum be reflected into the union):
>
> [2]
> { 'union':'SimpleButOpenCoded',
>   'data':{ 'one': 'str', 'two':'int' }}
>
> {"execute":"foo", "arguments":{'type':'one', 'data':'hello'}}

Ah, now I get what you mean!  We have a user-defined enum and the simple
union's implicit enum, and no way to connect the two.  In C, we get two
distinct enum types.

Perhaps you can clarify the commit message a bit.  Or maybe just drop
the part that confused me.  Okay since the relevant FIXME doesn't pass
judgement:

# FIXME: either allow base in non-flat unions, or diagnose missing discriminator

> I'm hoping to add as a followup series a variant of simple unions that
> is type-safe:
>
> [3]
> { 'union':'SimpleAndSafe', 'discriminator':'MyEnum',
>   'data':{ 'one':'str', 'two':'int' }}
>
> {"execute":"foo", "arguments":{'type':'one', 'data':'hello'}}
>
> But the existing, unused-except-in-testsuite, notion of a simple union
> with a base class looks like:
>
> [4]
> { 'type':'Shared', 'data':{'common':'int'}}
> { 'union':'SimpleWithBase', 'base':'Shared',
>   'data':{ 'one':'str', 'two':'int' }}
>
> {"execute":"foo", "arguments":{'common':1, 'type':'one', 'data':'hello'}}
>
> If we were to allow the addition of 'discriminator':'EnumType' to a
> simple union [3], but then add that discriminator to an existing case of
> a simple union with base [4], it would look like:
>
> { 'type':'Shared', 'data':{'common':'int'}}
> { 'union':'SimpleWithBaseAndDiscriminator', 'base':'Shared',
>   'discriminator':'MyEnum',
>   'data':{ 'one':'str', 'two':'int' }}
>
> Yuck.  That is indistinguishable from flat unions [1], except by whether
> discriminator names an enum type or a member of the base class.

Too subtle.  The difference should be syntactic, so it's immediately
visible.

>>>   If both 'base'
>>> and 'discriminator' are optional, then converting a simple union
>>> with base class to a type-safe simple union with an enum discriminator
>>> would not be possible.  So my plan is to get rid of 'base' without
>>> 'discriminator' later in the series;
>> 
>> Aha: you're going to outlaw 'base' in simple unions.  Yes, please.
>
> Okay, you came to my desired conclusion; it's just that my wording in
> the middle didn't help.
>
>> 
>>>  this will be no real loss, as any
>>> union that needs additional common fields can always use a flat
>>> union.
>> 
>> The mathematical concept behind unions is the

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 01:10:07PM +0100, Paolo Bonzini wrote:
> 
> 
> On 27/03/2015 13:02, Edgar E. Iglesias wrote:
> > On Fri, Mar 27, 2015 at 10:58:01AM +, Peter Maydell wrote:
> >> On 16 March 2015 at 17:20, Peter Maydell  wrote:
> >>> Define an API so that devices can register MemoryRegionOps whose read
> >>> and write callback functions are passed an arbitrary pointer to some
> >>> transaction attributes and can return a success-or-failure status code.
> >>> This will allow us to model devices which:
> >>>  * behave differently for ARM Secure/NonSecure memory accesses
> >>>  * behave differently for privileged/unprivileged accesses
> >>>  * may return a transaction failure (causing a guest exception)
> >>>for erroneous accesses
> >>
> >>> The success/failure response indication is currently ignored; it is
> >>> provided so we can implement it later without having to change the
> >>> callback function API yet again in future.
> >>
> >>> +/* New-style MMIO accessors can indicate that the transaction failed.
> >>> + * This is currently ignored, but provided in the API to allow us to add
> >>> + * support later without changing the MemoryRegionOps functions yet 
> >>> again.
> >>> + */
> >>> +typedef enum {
> >>> +MemTx_OK = 0,
> >>> +MemTx_DecodeError = 1, /* "nothing at that address" */
> >>> +MemTx_SlaveError = 2,  /* "device unhappy with request" (eg 
> >>> misalignment) */
> >>> +} MemTxResult;
> >>
> >> So I was looking at how this would actually get plumbed through
> >> the memory subsystem code, and there are some awkwardnesses
> >> with this simple enum approach. In particular, functions like
> >> address_space_rw want to combine the error returns from
> >> several io_mem_read/write calls into a single response to
> >> return to the caller. With an enum we'd need some pretty
> >> ugly code to prioritise particular failure types, or to
> >> do something arbitrary like "return first failure code".
> >> Alternatively we could:
> >> (a) make MemTxResult a uint32_t, where all-bits zero indicates
> >> "OK" and any bit set indicates some kind of error, eg
> >> bit 0 set for "device returned an error", and bit 1 for
> >> "decode error", and higher bits available for other kinds
> >> of extra info about errors in future. Then address_space_rw
> >> just ORs together all the bits in all the return codes it
> >> receives.
> >> (b) give up and say "just use a bool"
> >>
> >> Opinions?
> > 
> > Hi Peter,
> > 
> > Is this related to masters relying on the memory frameworks magic
> > handling of unaliged accesses?
> 
> Not necessarily, you can get the same just by doing a large write that
> spans multiple MemoryRegions.  See the loop in address_space_rw.

Right, this is another case of "magic" memory handling that allows masters
to issue unnatural transactions and rely on the memory framework to
split things up.
In these cases aren't the masters trading accuracy (including error
handling accuracy) for performance or model simplicity?

It could maybe be useful to have a flag so masters can say one of the
following (could be encoded in the memattrs):
1. Stop at first error and return.
2. Keep going after errors and give me the OR result of all errors.

For 1 to be useful, I think it would have to be combined with some
kind of return info that can point out where in the magic splitting
of large or unaligned transactions that things went wrong.

Probably overkill at the moment...

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v5 10/28] qapi: Segregate anonymous unions into alternates in generator

[Markus Armbruster]

Eric Blake  writes:

> On 03/26/2015 08:47 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Special-casing 'discriminator == {}' for handling anonymous unions
>>> is getting awkward; since this particular type is not always a
>>> dictionary on the wire, it is easier to treat it as a completely
>>> different class of type, "alternate", so that if a type is listed
>>> in the union_types array, we know it is not an anonymous union.
>>>
>>> This patch just further segregates union handling, to make sure that
>>> anonymous unions are not stored in union_types, and splitting up
>>> check_union() into separate functions.  A future patch will change
>>> the qapi grammar, and having the segregation already in place will
>>> make it easier to deal with the distinct meta-type.
>>>
>>> Signed-off-by: Eric Blake 
>>> ---
>
>>> @@ -535,7 +546,8 @@ def find_struct(name):
>>>
>>>  def add_union(definition):
>>>  global union_types
>>> -union_types.append(definition)
>>> +if definition.get('discriminator') != {}:
>>> +union_types.append(definition)
>>>
>>>  def find_union(name):
>>>  global union_types
>> 
>> This is the only unobvious hunk.
>> 
>> union_types is used only through find_union.  The hunk makes
>> find_union(N) return None when N names an anonymous union.
>> 
>> find_union() is used in two places:
>> 
>> * find_alternate_member_qtype()
>> 
>>   Patched further up.  It really wants only non-anonymous unions, and
>>   this change to find_union() renders its check for anonymous unions
>>   superfluous.  Good.
>> 
>> * generate_visit_alternate()
>> 
>>   Asserts that each member's type is either a built-in type, a complex
>>   type, a union type, or an enum type.
>> 
>>   The change relaxes the assertion not to trigger on anonymous union
>>   types.  Why is that okay?
>
> No, the change tightens the assertion so that it will now fail on an
> anonymous union nested as a branch of another anonymous union (where
> before it could silently pass the assertion), because the anonymous
> union is no longer found by find_union().  And this is okay because the
> earlier change to find_alternate_member_qtype means that we don't allow
> nested anonymous unions, so making the assertion fail if an anonymous
> union gets through anyway is correct.

Thanks for unconfusing me.

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH v5 13/28] qapi: Add some expr tests

[Markus Armbruster]

Eric Blake  writes:

> On 03/26/2015 09:55 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Demonstrate that the qapi generator doesn't deal well with
>>> expressions that aren't up to par. Later patches will improve
>>> the expected results as the generator is made stricter.  Only
>>> one of the added tests actually behaves sanely at rejecting
>>> obvious problems.
>>>
>> 
>> qapi-code-gen.txt documents the naming conventions:
>> 
>> Types, commands, and events share a common namespace.  Therefore,
>> generally speaking, type definitions should always use CamelCase for
>> user-defined type names, while built-in types are lowercase. Type
>> definitions should not end in 'Kind', as this namespace is used for
>> creating implicit C enums for visiting union types.  Command names,
>> and field names within a type, should be all lower case with words
>> separated by a hyphen.  However, some existing older commands and
>> complex types use underscore; when extending such expressions,
>> consistency is preferred over blindly avoiding underscore.  Event
>> names should be ALL_CAPS with words separated by underscore.  The
>> special string '**' appears for some commands that manually perform
>> their own type checking rather than relying on the type-safe code
>> produced by the qapi code generators.
>> 
>> We should either enforce the conventions consistently, or not at all.
>> 
>> Enforcing them makes certain kinds of name clashes in generated C
>> impossible.  If we don't enforce them, we should catch the clashes.
>> 
>> Since I haven't read to the end of your series, I have to ask: do you
>> intend to enforce them?
>
> I added tests to enforce it for event names, but did not enforce things
> for command names or complex type members.  I guess that can be added on
> top, if desired.
>
> So, did this patch get R-b?

I'd rather not enforce naming conventions just for events.

If we want to enforce them, let's do it consistently, and in a separate
series that includes this patch.  Okay?

Re: [Qemu-devel] [PATCH v5 18/28] qapi: Unify type bypass and add tests

[Markus Armbruster]

Eric Blake  writes:

> On 03/26/2015 11:38 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> For a few QMP commands, we are forced to pass an arbitrary type
>>> without tracking it properly in QAPI.  Among the existing clients,
>>> this unnamed type was spelled 'dict', 'visitor', and '**'; this
>>> patch standardizes on '**'.
>>>
>>> Meanwhile, for both 'gen' and 'success-response' keys, we have been
>>> ignoring the value, although the schema consistently used "'no'".
>>> But now that we can support a literal "false" in the schema, we
>>> might as well use that rather than ignoring the value or
>>> special-casing a random string.
>>>
>>> There is no difference to the generated code.  As these features
>>> were previously undocumented before this series, add some tests
>>> and documentation on what we'd like to guarantee, although it will
>>> take later patches to clean up test results and actually enforce
>>> the use of a bool parameter.
>> 
>> You don't actually add documentation in this patch.
>
> Hmm, more evidence that I waffled about per-commit doc fixes, vs.
> lumping it all in patch 1, and I obviously failed to scrub the commit
> messages after changing my mind.

Fortunately, commit messages don't need testing, so this is easy enough
to fix in a respin :)

>> Aside: 'gen': false is required when '**' is used anywhere in the
>> command.  If it was permitted only then, it would be redundant.  I think
>> we happily accept 'gen': false without '**' so far, although we don't
>> use it.  That's okay.
>
> Also, even though the code accepts 'gen':false, it rejects 'gen':true
> ('gen' is only a one-way switch away from the default).  Also something
> I didn't think worth worrying about.

Agree.

>>> Signed-off-by: Eric Blake 
>> 
>> Reviewed-by: Markus Armbruster 
>> 
>> 

Re: [Qemu-devel] [PATCH v5 00/28] drop qapi nested structs

[Markus Armbruster]

Eric Blake  writes:

> After several months of sitting on this, I finally made progress
> on it.  Let's get it in 2.4, and I promise to kick out a v6
> (if needed) with much less delay.
>
> We want to eventually allow qapi defaults, by making:
>  'data':{'*flag':'bool'}
> shorthand for:
>  'data':{'flag':{'type':'bool', 'optional':true}}
> so that the default can be specified:
>  'data':{'flag':{'type':'bool', 'optional':true, 'default':true}}
>
> This series does not quite get us there, but it DOES do a number
> of other things.  It gets rid of the three uses of nested inline
> structs, changes anonymous unions to use a specific 'alternate'
> metatype rather than abusing 'union', and fixes lots of other
> parser bugs found while designing the testsuite.  The testsuite
> changes make the bulk of this series, with a repeating pattern
> of writing tests that expose weaknesses in the old parser, then
> beefing up the generator to catch the problem during the initial
> parse rather than choking with an obscure python message or even
> causing a C compilation failure.
>
> v4 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg04022.html
>
> Changes since then: lots of refactoring; 8 new commits; lots of
> testsuite additions; address Markus' review comments where I
> could.  However, so much has changed, and so much time elapsed,
> that it is probably easier to just review this series fresh than
> to try and compare it to earlier versions.

Okay, I'm through.  I like it well, and I think it's really close.
There are a few questions / ideas you haven't replied to already in
PATCH 21ff.  With those conversations wrapped up, I may well agree to
taking the series as is, but I think I'd prefer a *conservative* respin
addressing stuff that doesn't need new testing, like commit messages and
comments, and *selected* (by you) improvments that do need new testing.

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Paolo Bonzini]

On 27/03/2015 13:32, Edgar E. Iglesias wrote:
>>> Is this related to masters relying on the memory frameworks magic
>>> handling of unaliged accesses?
>>
>> Not necessarily, you can get the same just by doing a large write that
>> spans multiple MemoryRegions.  See the loop in address_space_rw.
> 
> Right, this is another case of "magic" memory handling that allows masters
> to issue unnatural transactions and rely on the memory framework to
> split things up.
> In these cases aren't the masters trading accuracy (including error
> handling accuracy) for performance or model simplicity?

Yes.  There are no "natural" transactions beyond 32 or 64-bit accesses.

> It could maybe be useful to have a flag so masters can say one of the
> following (could be encoded in the memattrs):
> 1. Stop at first error and return.
> 2. Keep going after errors and give me the OR result of all errors.

It could just be a length pointer in the same vein as
address_space_map's.  If NULL, keep going.  If not NULL, stop and return.

Paolo

> 
> For 1 to be useful, I think it would have to be combined with some
> kind of return info that can point out where in the magic splitting
> of large or unaligned transactions that things went wrong.
> 
> Probably overkill at the moment...
> 
> Cheers,
> Edgar
> 

Re: [Qemu-devel] [RESEND PULL for-2.3 0/1] Net patches

[Peter Maydell]

On 27 March 2015 at 11:59, Stefan Hajnoczi  wrote:
> The following changes since commit 4ad9e2b36e1e00fe5b96c3448ecd673e11c4d6d8:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-gtk-20150326-1' into 
> staging (2015-03-26 18:35:09 +)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/net-pull-request
>
> for you to fetch changes up to 4e60a250d395ef0d04eb8b6489cc5f7615a8909b:
>
>   hw/net/e1000: fix integer endianness (2015-03-27 10:23:50 +)
>

Applied, thanks.

-- PMM

[Qemu-devel] [Bug 1392504] Re: libvirt not relabeling devices on USB Passthrough

[Kelvin Middleton]

...actually are you talking kernel or the full 15.04 pre-release?

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

Title:
  libvirt not relabeling devices on USB Passthrough

Status in QEMU:
  New
Status in libvirt package in Ubuntu:
  Triaged

Bug description:
  After upgrading from Ubuntu 14.04 to Ubuntu 14.10 USB passthrough in
  QEMU (version is now 2.1.0 - Debian2.1+dfsg-4ubuntu6.1) is not working
  any more. Already tried to remove and attach the USB devices. I use 1
  USB2 HDD  +  1 USB3 HDD to a virtual linux machine; 1 USB2 HDD to a
  virual FNAS machine and a USB 2camera to virtual Win7 machine. All
  these devices are not working any more.

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

[Qemu-devel] [Bug 1392504] Re: libvirt not relabeling devices on USB Passthrough

[Kelvin Middleton]

I'm running an ACS patched linux mainline 3.18 as well as Ubuntu 14.10
stock.  Haven't tested the Ubuntu 15.04 stock but will grab the debs and
confirm.

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

Title:
  libvirt not relabeling devices on USB Passthrough

Status in QEMU:
  New
Status in libvirt package in Ubuntu:
  Triaged

Bug description:
  After upgrading from Ubuntu 14.04 to Ubuntu 14.10 USB passthrough in
  QEMU (version is now 2.1.0 - Debian2.1+dfsg-4ubuntu6.1) is not working
  any more. Already tried to remove and attach the USB devices. I use 1
  USB2 HDD  +  1 USB3 HDD to a virtual linux machine; 1 USB2 HDD to a
  virual FNAS machine and a USB 2camera to virtual Win7 machine. All
  these devices are not working any more.

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

Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 02:16:53PM +0100, Paolo Bonzini wrote:
> 
> 
> On 27/03/2015 13:32, Edgar E. Iglesias wrote:
> >>> Is this related to masters relying on the memory frameworks magic
> >>> handling of unaliged accesses?
> >>
> >> Not necessarily, you can get the same just by doing a large write that
> >> spans multiple MemoryRegions.  See the loop in address_space_rw.
> > 
> > Right, this is another case of "magic" memory handling that allows masters
> > to issue unnatural transactions and rely on the memory framework to
> > split things up.
> > In these cases aren't the masters trading accuracy (including error
> > handling accuracy) for performance or model simplicity?
> 
> Yes.  There are no "natural" transactions beyond 32 or 64-bit accesses.
> 
> > It could maybe be useful to have a flag so masters can say one of the
> > following (could be encoded in the memattrs):
> > 1. Stop at first error and return.
> > 2. Keep going after errors and give me the OR result of all errors.
> 
> It could just be a length pointer in the same vein as
> address_space_map's.  If NULL, keep going.  If not NULL, stop and return.
> 

Good point, thanks.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH for-2.3 3/4] s390x: do not include ram_addr.h

[Thomas Huth]

Am Thu, 26 Mar 2015 16:36:01 +0100
schrieb Cornelia Huck :

> From: Paolo Bonzini 
> 
> ram_addr.h is an internal interface and it is not needed anyway by
> hw/s390x/ipl.c.
> 
> Cc: Christian Borntraeger 
> Signed-off-by: Paolo Bonzini 
> Message-Id: <1427295389-5054-1-git-send-email-pbonz...@redhat.com>
> Signed-off-by: Christian Borntraeger 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/s390x/ipl.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 54d0835..5c86613 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -14,7 +14,6 @@
>  #include "sysemu/sysemu.h"
>  #include "cpu.h"
>  #include "elf.h"
> -#include "exec/ram_addr.h"
>  #include "hw/loader.h"
>  #include "hw/sysbus.h"
>  #include "hw/s390x/virtio-ccw.h"

Uh, IIRC I temporarily played around with qemu_get_ram_ptr() when I
recently improved the BIOS loading in this file - and later forgot to
remove that include file again - sorry! So of course, it should not be
here. Thanks for cleaning it up!

Reviewed-by: Thomas Huth 

[Qemu-devel] unhandled signal 11

[Catalin Vasile]

How can I debug errors like this:
qemu-system-ppc[2273]: unhandled signal 11 at 0008 nip 1007ed08 lr
103404e0 code 30001
?
I know it is a ppc binary, but I am asking a general question.
>From a first impression it looks a little like Linux Kernel Oops messages.
How can I actually find out the line in my code that is generating this thing?

Re: [Qemu-devel] [PATCH] fix assertion in "info memory-devices" if memdev isn't accessible

[Igor Mammedov]

On Thu, 26 Mar 2015 21:33:19 +0100
Paolo Bonzini  wrote:

> 
> 
> On 26/03/2015 17:42, Igor Mammedov wrote:
> > +mdevid = object_property_get_str(OBJECT(dimm->hostmem), "id",
> > + &error_abort);
> > +mdevpath = g_strdup_printf("/objects/%s", mdevid);
> > +g_free(mdevid);
> > +mdev = object_resolve_path_type(mdevpath, TYPE_MEMORY_BACKEND,
> > +NULL);
> 
> You can add a backend with id xyz, remove it, and add another with the
> same id, right?
> 
> I think the can_be_deleted solution is better and not more invasive,
> even for 2.3.
yep, it's better.

> 
> Paolo

Re: [Qemu-devel] [PATCH] fix assertion in "info memory-devices" if memdev isn't accessible

[Igor Mammedov]

On Thu, 26 Mar 2015 16:26:17 -0400
Luiz Capitulino  wrote:

> On Thu, 26 Mar 2015 15:59:25 -0400
> Luiz Capitulino  wrote:
> 
> > On Thu, 26 Mar 2015 16:42:54 +
> > Igor Mammedov  wrote:
> > 
> > > showing a memory device whose memdev is removed leads to an assert:
> > > 
> > > (qemu) object_add memory-backend-ram,id=ram0,size=128M
> > > (qemu) device_add pc-dimm,id=d0,memdev=ram0
> > > (qemu) object_del ram0
> > > (qemu) info memory-devices
> > > **
> > > ERROR:qom/object.c:1274:object_get_canonical_path_component:\
> > > assertion failed: (obj->parent != NULL)
> > > Aborted
> > > 
> > > so check if memdev is present before calling
> > > object_get_canonical_path() and in case it's not assign to
> > > "memdev:" field "removed" value.
> > > 
> > > Signed-off-by: Igor Mammedov 
> > 
> > Igor, do you want to go in through my tree?
> 
> What about the patch, btw? :)
> 
> > 
> > > ---
> > > alternative more generic solution:
> > >http://patchwork.ozlabs.org/patch/453343/
> > > ---
> > >  backends/hostmem.c   | 23 +++
> > >  hw/mem/pc-dimm.c | 15 ++-
> > >  include/sysemu/hostmem.h |  1 +
> > >  qmp.c|  1 +
> > >  4 files changed, 39 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > index 99e8f99..aa88691 100644
> > > --- a/backends/hostmem.c
> > > +++ b/backends/hostmem.c
> > > @@ -227,6 +227,26 @@ static void host_memory_backend_set_prealloc(Object 
> > > *obj, bool value,
> > >  }
> > >  }
> > >  
> > > +static void host_memory_backend_set_id(Object *obj, const char *id,
> > > +   Error **errp)
> > > +{
> > > +HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > +
> > > +if (backend->id) {
> > > +error_set(errp, QERR_INVALID_PARAMETER_VALUE, "id",
> > > +  "can't change already set value");
> > > +return;
> > > +}
> > > +backend->id = g_strdup(id);
> > > +}
> > > +
> > > +static char *host_memory_backend_get_id(Object *obj, Error **errp)
> > > +{
> > > +HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > +
> > > +return g_strdup(backend->id);
> > > +}
> > > +
> > >  static void host_memory_backend_init(Object *obj)
> > >  {
> > >  HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > > @@ -237,6 +257,9 @@ static void host_memory_backend_init(Object *obj)
> > >"dump-guest-core", true);
> > >  backend->prealloc = mem_prealloc;
> > >  
> > > +object_property_add_str(obj, "id",
> > > +host_memory_backend_get_id,
> > > +host_memory_backend_set_id, NULL);
> > >  object_property_add_bool(obj, "merge",
> > >  host_memory_backend_get_merge,
> > >  host_memory_backend_set_merge, NULL);
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index 39f0c97..482a7a0 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -69,6 +69,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > >  DeviceState *dev = DEVICE(obj);
> > >  
> > >  if (dev->realized) {
> > > +char *mdevid, *mdevpath;
> > > +Object *mdev;
> > >  MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
> > >  MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
> > >  PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
> > > @@ -86,7 +88,18 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > >  di->node = dimm->node;
> > >  di->size = object_property_get_int(OBJECT(dimm), 
> > > PC_DIMM_SIZE_PROP,
> > > NULL);
> > > -di->memdev = 
> > > object_get_canonical_path(OBJECT(dimm->hostmem));
> > > +mdevid = object_property_get_str(OBJECT(dimm->hostmem), "id",
> > > + &error_abort);
> > > +mdevpath = g_strdup_printf("/objects/%s", mdevid);
> > > +g_free(mdevid);
> > > +mdev = object_resolve_path_type(mdevpath, 
> > > TYPE_MEMORY_BACKEND,
> > > +NULL);
> > > +if (mdev == OBJECT(dimm->hostmem)) {
> > > +di->memdev = 
> > > object_get_canonical_path(OBJECT(dimm->hostmem));
> > > +} else {
> > > +di->memdev = g_strdup("removed");
> > > +}
> > > +g_free(mdevpath);
> > >  
> > >  info->dimm = di;
> > >  elem->value = info;
> > > diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> > > index 1ce4394..63918aa 100644
> > > --- a/include/sysemu/hostmem.h
> > > +++ b/include/sysemu/hostmem.h
> > > @@ -53,6 +53,7 @@ struct HostMemoryBackend {
> > >  Object parent;
> > >  
> > >  /* protected */
> > > +

Re: [Qemu-devel] unhandled signal 11

[Peter Maydell]

On 27 March 2015 at 14:09, Catalin Vasile  wrote:
> How can I debug errors like this:
> qemu-system-ppc[2273]: unhandled signal 11 at 0008 nip 1007ed08 lr
> 103404e0 code 30001
> ?
> I know it is a ppc binary, but I am asking a general question.
> From a first impression it looks a little like Linux Kernel Oops messages.
> How can I actually find out the line in my code that is generating this thing?

This is just the kernel telling you that qemu-system-ppc crashed.
Run the program under a debugger and you'll be able to get
more useful information about what is happening.

-- PMM

Re: [Qemu-devel] [PATCH] fix assertion in "info memory-devices" if memdev isn't accessible

[Igor Mammedov]

On Thu, 26 Mar 2015 21:33:19 +0100
Paolo Bonzini  wrote:

> 
> 
> On 26/03/2015 17:42, Igor Mammedov wrote:
> > +mdevid = object_property_get_str(OBJECT(dimm->hostmem), "id",
> > + &error_abort);
> > +mdevpath = g_strdup_printf("/objects/%s", mdevid);
> > +g_free(mdevid);
> > +mdev = object_resolve_path_type(mdevpath, TYPE_MEMORY_BACKEND,
> > +NULL);
> 
> You can add a backend with id xyz, remove it, and add another with the
> same id, right?
right, but in the end it compares pointers to hostmem so it safe.
but can_be_deleted() is more generic hence more preferred.

> 
> I think the can_be_deleted solution is better and not more invasive,
> even for 2.3.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v4 1/2] qom: Add can_be_deleted callback to UserCreatableClass

[Igor Mammedov]

On Fri, 27 Mar 2015 13:36:11 +0800
Lin Ma  wrote:

> If backends implement the can_be_deleted and it returns false,
> Then the qmp_object_del won't delete the given backends.
> 
> Signed-off-by: Lin Ma 
> ---
>  include/qom/object_interfaces.h | 14 ++
>  qmp.c   |  5 +
>  qom/object_interfaces.c | 15 +++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index b792283..012b653 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -25,6 +25,8 @@ typedef struct UserCreatable {
>   * UserCreatableClass:
>   * @parent_class: the base class
>   * @complete: callback to be called after @obj's properties are set.
> + * @can_be_deleted: callback to be called before an object is removed
> + * to check if @obj can be removed safely.
>   *
>   * Interface is designed to work with -object/object-add/object_add
>   * commands.
> @@ -47,6 +49,7 @@ typedef struct UserCreatableClass {
>  
>  /*  */
>  void (*complete)(UserCreatable *uc, Error **errp);
> +bool (*can_be_deleted)(UserCreatable *uc, Error **errp);
>  } UserCreatableClass;
>  
>  /**
> @@ -59,4 +62,15 @@ typedef struct UserCreatableClass {
>   * nothing.
>   */
>  void user_creatable_complete(Object *obj, Error **errp);
> +
> +/**
> + * user_creatable_can_be_deleted:
> + * @obj: the object whose can_be_deleted() method is called if defined
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Wrapper to call can_be_deleted() method if one of types it's inherited
> + * from implements USER_CREATABLE interface, otherwise the call does
> + * nothing.
> + */
> +bool user_creatable_can_be_deleted(Object *obj, Error **errp);
>  #endif
> diff --git a/qmp.c b/qmp.c
> index c479e77..dc61da1 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -711,6 +711,11 @@ void qmp_object_del(const char *id, Error **errp)
>  error_setg(errp, "object id not found");
>  return;
>  }
> +
> +if (!user_creatable_can_be_deleted(obj, errp)) {
> +error_setg(errp, "%s is in used, can not be deleted", id);
^^^ is in use || is used
> +return;
> +}
>  object_unparent(obj);
>  }
>  
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 6360818..25ae6ca 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -18,6 +18,21 @@ void user_creatable_complete(Object *obj, Error **errp)
>  }
>  }
>  
> +bool user_creatable_can_be_deleted(Object *obj, Error **errp)
> +{
> +
> +UserCreatableClass *ucc;
> +UserCreatable *uc =
> +(UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
direc cast is wron, pls use type specific caster with QOM objects
in this case you can even drop dynamic cast since /objects holds
only TYPE_USER_CREATABLE objects and just do
  UserCreatable *uc = USER_CREATABLE(obj)
you don't check for null pointer anyway

> +
> +ucc = USER_CREATABLE_GET_CLASS(uc);
> +if (ucc->can_be_deleted) {
> +return ucc->can_be_deleted(uc, errp);
> +} else {
> +return true;
> +}
> +}
> +
>  static void register_types(void)
>  {
>  static const TypeInfo uc_interface_info = {

Re: [Qemu-devel] [PATCH v4 2/2] hostmem: Prevent removing an in-use memory backend object

[Igor Mammedov]

On Fri, 27 Mar 2015 13:36:12 +0800
Lin Ma  wrote:

> showing a memory device whose memdev is removed leads an assert:
> 
> (qemu) object_add memory-backend-ram,id=ram0,size=128M
> (qemu) device_add pc-dimm,id=d0,memdev=ram0
> (qemu) object_del ram0
> (qemu) info memory-devices
> **
> ERROR:qom/object.c:1274:object_get_canonical_path_component:\
> assertion failed: (obj->parent != NULL)
> Aborted
> 
> The patch prevents removing an in-use mem backend and error out.
> 
> Signed-off-by: Lin Ma 
> ---
>  backends/hostmem.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 99e8f99..dbf94a9 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -335,12 +335,26 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>  }
>  }
>  
> +static bool
> +host_memory_backend_can_be_deleted(UserCreatable *uc, Error **errp)
> +{
> +MemoryRegion *mr;
add separating line here

> +mr = host_memory_backend_get_memory(MEMORY_BACKEND(uc), errp);
> +if (memory_region_is_mapped(mr)) {
> +return false;
> +} else {
> +return true;
> +}
> +}
> +
>  static void
>  host_memory_backend_class_init(ObjectClass *oc, void *data)
>  {
>  UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>  
>  ucc->complete = host_memory_backend_memory_complete;
> +
unnecessary line, pls drop.

> +ucc->can_be_deleted = host_memory_backend_can_be_deleted;
>  }
>  
>  static const TypeInfo host_memory_backend_info = {

Re: [Qemu-devel] [PATCH v4 0/2] Add generic can_be_deleted to UserCreatableClass.

[Igor Mammedov]

On Fri, 27 Mar 2015 13:36:10 +0800
Lin Ma  wrote:

> The patchset adds a generic can_be_deleted callback to UserCreatableClass.
> It prevents removing a usercreatable object if the callback returns false.
> 
> Backends could implement the callback if it shoudn't be removed while it's
> in use.
> 
> Thank Peter Crosthwaite, Paolo Bonzini, Andreas Färber and Igor Mammedov for
> helping review.
perhaps it's better to take this into 2.3


> 
> 
> ChangeLog:
> V4:
> Add function user_creatable_can_be_deleted to wrapper can_be_deleted method.
> Remove some unnecessary code.
> 
> V3:
> Move the callback to the correct place: UserCreatableClass.
> Move the backend callback implementation to hostmem.c.
> 
> V2:
> Make it generic, add the can_be_deleted callback to TypeInfo and TypeImpl.
> Implement the callback in hostmem-file.c and hostmem-ram.c.
> 
> V1:
> Initial version, hard coded in object_del to prevent removing an in-use
> host memory backend.
> 
> 
> Lin Ma (2):
>   qom: Add can_be_deleted callback to UserCreatableClass
>   hostmem: Prevent removing an in-use memory backend object
> 
>  backends/hostmem.c  | 14 ++
>  include/qom/object_interfaces.h | 14 ++
>  qmp.c   |  5 +
>  qom/object_interfaces.c | 15 +++
>  4 files changed, 48 insertions(+)
> 

Re: [Qemu-devel] block-commit & dropping privs

[Eric Blake]

On 03/27/2015 03:07 AM, Michael Tokarev wrote:
> Hello.
> 
> I tried to experiment with block-commit command, which propagates
> changes accumulated in an overlay (qcow2) block image file back to
> the base image file.
> 
> And immediately faced a problem.  All my VMs are run chrooted into
> an empty dir and with low-priv user (using -runsa and -chroot options,
> initially started as root).  Ofcourse this low-priv qemu process
> can't open the base image anymore, because it doesn't have the
> necessary permissions and because the base file is inaccessible
> within the chroot.
> 
> So I wonder if we can avoid reopening the base img by always opening
> it read-write (using a command-line option), does it make sense?

It is already possible to open a file read-write on the command line, by
using -add-fd twice to put both a read-only and a read-write fd handle
to the same file in a single set N, then using -drive options to specify
/dev/fdset/N rather than the file name.  By that argument, I'm not sure
if adding any other command line options is necessary.

> 
> Or maybe there's some other possible solution to this, for example,
> passing in a filedescriptor for the new base img over a unix socket?

Hmm, we do allow QMP to pass in fds over Unix sockets already, but what
we don't have yet is the ability to associate one of those just-passed
fds to an existing open BDS (right now, you can only create a new fdset
as tied to a new BDS, but block-commit can only target an open BDS;
there is no way to pivot which BDS base is associated with another BDS).
 So maybe there is still room for improvement, especially since having a
QMP solution is nicer than requiring foresight to pass in an fdset from
the command line.

-- 
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 v5 7/7] qmp-event: add event notification for memory hot unplug error

[Eric Blake]

On 03/27/2015 03:20 AM, Zhu Guihua wrote:
> When memory hot unplug fails, this patch adds support to send
> QMP event to notify mgmt about this failure.
> 
> Signed-off-by: Zhu Guihua 
> ---
>  docs/qmp/qmp-events.txt  | 17 +
>  hw/acpi/memory_hotplug.c |  8 +++-
>  monitor.c|  1 +
>  qapi/event.json  | 14 ++
>  4 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index d759d19..7a05705 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -226,6 +226,23 @@ Example:
>  { "event": "GUEST_PANICKED",
>   "data": { "action": "pause" } }
>  
> +MEM_HOT_UNPLUG_ERROR
> +
> +Emitted when memory hot unplug occurs error.

s/occurs error/error occurs/

> +
> +Data:
> +
> +- "device": device name (json-string)
> +- "msg": Informative message (e.g., reason for the error) (json-string)
> +
> +Example:
> +
> +{ "event": "MEM_HOT_UNPLUG_ERROR"
> +  "data": { "device": "dimm1",
> +"msg": "acpi: device unplug for not supported device"

s/not supported/unsupported/

> +  },
> +  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +
>  NIC_RX_FILTER_CHANGED
>  -
>  
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 2a1b866..f1cef71 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -94,6 +94,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
> addr, uint64_t data,
>  ACPIOSTInfo *info;
>  DeviceState *dev = NULL;
>  HotplugHandler *hotplug_ctrl = NULL;
> +Error *local_err = NULL;
>  
>  if (!mem_st->dev_count) {
>  return;
> @@ -148,7 +149,12 @@ static void acpi_memory_hotplug_write(void *opaque, 
> hwaddr addr, uint64_t data,
>  dev = DEVICE(mdev->dimm);
>  hotplug_ctrl = qdev_get_hotplug_handler(dev);
>  /* call pc-dimm unplug cb */
> -hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> +hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> +if (local_err) {
> +qapi_event_send_mem_unplug_error(dev->id,
> + error_get_pretty(local_err),
> + &error_abort);
> +}
>  }

Does this leak local_err?

> +++ b/qapi/event.json
> @@ -330,3 +330,17 @@
>  ##
>  { 'event': 'VSERPORT_CHANGE',
>'data': { 'id': 'str', 'open': 'bool' } }
> +
> +##
> +# @MEM_UNPLUG_ERROR
> +#
> +# Emitted when memory hot unplug occurs error.

s/occurs error/error occurs/

> +#
> +# @device: device name
> +#
> +# @msg: Informative message
> +#
> +# Since: 2.3

You're awfully late for 2.3; this may need to be 2.4.

> +##
> +{ 'event': 'MEM_UNPLUG_ERROR',
> +  'data': { 'device': 'str', 'msg': 'str' } }
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1437367] [NEW] Qemu guest fails to write files with raw disk (like \\.\PhysicalDrive1) on Windows host.

[hiroaki]

Public bug reported:

Qemu guest fails to write files with specifing raw disk like \\.\PhysicalDrive1
full command line is below.
qemu-sysytem-i386.exe -kernel bzImage -drive file=rootfs.ext2,index=0,if=scsi 
-append root=/dev/sda -drive file=\\.\PhysicalDrive1,index=1,if=scsi

I found the reason is below aio_worker returns -EIO when flush
operation.

https://github.com/qemu/qemu/blob/master/block/raw-win32.c#L95

static int aio_worker(void *arg)
...
case QEMU_AIO_FLUSH:
if (!FlushFileBuffers(aiocb->hfile)) {
return -EIO;
}

FlushFileBuffers always fails with GetLastError() == ERROR_INVALID_FUNCTION
I think this function doesn't support raw device.
For flushing, you might have to issue scsi/ata command or use another way.
Trying to just ignoring this error, writing function seems to be fine for me.

Thanks
hiroaki

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  Qemu guest fails to write files with raw disk (like
  \\.\PhysicalDrive1) on Windows host.

Status in QEMU:
  New

Bug description:
  Qemu guest fails to write files with specifing raw disk like 
\\.\PhysicalDrive1
  full command line is below.
  qemu-sysytem-i386.exe -kernel bzImage -drive file=rootfs.ext2,index=0,if=scsi 
-append root=/dev/sda -drive file=\\.\PhysicalDrive1,index=1,if=scsi

  I found the reason is below aio_worker returns -EIO when flush
  operation.

  https://github.com/qemu/qemu/blob/master/block/raw-win32.c#L95

  static int aio_worker(void *arg)
  ...
  case QEMU_AIO_FLUSH:
  if (!FlushFileBuffers(aiocb->hfile)) {
  return -EIO;
  }

  FlushFileBuffers always fails with GetLastError() == ERROR_INVALID_FUNCTION
  I think this function doesn't support raw device.
  For flushing, you might have to issue scsi/ata command or use another way.
  Trying to just ignoring this error, writing function seems to be fine for me.

  Thanks
  hiroaki

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

Re: [Qemu-devel] [PATCH] tcg: optimise memory layout of TCGTemp

[Richard Henderson]

On 03/25/2015 12:50 PM, Emilio G. Cota wrote:
> This brings down the size of the struct from 56 to 32 bytes on 64-bit,
> and to 16 bytes on 32-bit.
> 
> The appended adds macros to prevent us from mistakenly overflowing
> the bitfields when more elements are added to the corresponding
> enums/macros.
> 
> Note that reg/mem_reg need only 6 bits (for ia64) but for performance
> is probably better to align them to a byte address.
> 
> Given that TCGTemp is used in large arrays this leads to a few KBs
> of savings. However, unpacking the bits takes additional code, so
> the net effect depends on the target (host is x86_64):
> 
> Before:
> $ find . -name 'tcg.o' | xargs size
>textdata bss dec hex filename
>   41131   29800  88   71019   1156b ./aarch64-softmmu/tcg/tcg.o
>   37969   29416  96   67481   10799 ./x86_64-linux-user/tcg/tcg.o
>   39354   28816  96   68266   10aaa ./arm-linux-user/tcg/tcg.o
>   40802   29096  88   69986   11162 ./arm-softmmu/tcg/tcg.o
>   39417   29672  88   69177   10e39 ./x86_64-softmmu/tcg/tcg.o
> 
> After:
> $ find . -name 'tcg.o' | xargs size
>textdata bss dec hex filename
>   41187   29800  88   71075   115a3 ./aarch64-softmmu/tcg/tcg.o
>   3   29416  96   67289   106d9 ./x86_64-linux-user/tcg/tcg.o
>   39162   28816  96   68074   109ea ./arm-linux-user/tcg/tcg.o
>   40858   29096  88   70042   1119a ./arm-softmmu/tcg/tcg.o
>   39473   29672  88   69233   10e71 ./x86_64-softmmu/tcg/tcg.o
> 
> Suggested-by: Stefan Weil 
> Suggested-by: Richard Henderson 
> Signed-off-by: Emilio G. Cota 
> ---
>  tcg/tcg.h | 22 +-
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index add7f75..71ae7b2 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -193,7 +193,7 @@ typedef struct TCGPool {
>  typedef enum TCGType {
>  TCG_TYPE_I32,
>  TCG_TYPE_I64,
> -TCG_TYPE_COUNT, /* number of different types */
> +TCG_TYPE_COUNT, /* number of different types, see TCG_TYPE_NR_BITS */
>  
>  /* An alias for the size of the host register.  */
>  #if TCG_TARGET_REG_BITS == 32
> @@ -217,6 +217,9 @@ typedef enum TCGType {
>  #endif
>  } TCGType;
>  
> +/* used for bitfield packing to save space */
> +#define TCG_TYPE_NR_BITS 1

I'd rather you moved TCG_TYPE_COUNT out of the enum and into a define.  Perhaps
even as (1 << TCG_TYPE_NR_BITS).

> @@ -421,16 +424,14 @@ static inline TCGCond tcg_high_cond(TCGCond c)
>  #define TEMP_VAL_REG   1
>  #define TEMP_VAL_MEM   2
>  #define TEMP_VAL_CONST 3
> +#define TEMP_VAL_NR_BITS 2

And make this an enumeration.

>  typedef struct TCGTemp {
> -TCGType base_type;
> -TCGType type;
> -int val_type;
> -int reg;
> -tcg_target_long val;
> -int mem_reg;
> -intptr_t mem_offset;
> +unsigned int reg:8;
> +unsigned int mem_reg:8;
> +unsigned int val_type:TEMP_VAL_NR_BITS;
> +unsigned int base_type:TCG_TYPE_NR_BITS;
> +unsigned int type:TCG_TYPE_NR_BITS;

And do *not* change these from the enumeration to an unsigned int.

I know why you did this -- to keep the compiler from warning that the TCGType
enum didn't fit in the bitfield, because of TCG_TYPE_COUNT being an enumerator,
rather than an unrelated number.  Except that's exactly the warning we want to
keep, on the off-chance that someone modifies the enums without modifying the
_NR_BITS defines.


r~

[Qemu-devel] [PATCH] target-tricore: fix CACHEA/I_POSTINC/PREINC using data register..

[Bastian Koppelmann]

..for address calculation instead address registers.

Signed-off-by: Bastian Koppelmann 
---
 target-tricore/translate.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-tricore/translate.c b/target-tricore/translate.c
index bbcfee9..54a48cd 100644
--- a/target-tricore/translate.c
+++ b/target-tricore/translate.c
@@ -4509,14 +4509,14 @@ static void 
decode_bo_addrmode_post_pre_base(CPUTriCoreState *env,
 case OPC2_32_BO_CACHEA_I_POSTINC:
 /* instruction to access the cache, but we still need to handle
the addressing mode */
-tcg_gen_addi_tl(cpu_gpr_d[r2], cpu_gpr_d[r2], off10);
+tcg_gen_addi_tl(cpu_gpr_a[r2], cpu_gpr_a[r2], off10);
 break;
 case OPC2_32_BO_CACHEA_WI_PREINC:
 case OPC2_32_BO_CACHEA_W_PREINC:
 case OPC2_32_BO_CACHEA_I_PREINC:
 /* instruction to access the cache, but we still need to handle
the addressing mode */
-tcg_gen_addi_tl(cpu_gpr_d[r2], cpu_gpr_d[r2], off10);
+tcg_gen_addi_tl(cpu_gpr_a[r2], cpu_gpr_a[r2], off10);
 break;
 case OPC2_32_BO_CACHEI_WI_SHORTOFF:
 case OPC2_32_BO_CACHEI_W_SHORTOFF:
@@ -4526,13 +4526,13 @@ static void 
decode_bo_addrmode_post_pre_base(CPUTriCoreState *env,
 case OPC2_32_BO_CACHEI_W_POSTINC:
 case OPC2_32_BO_CACHEI_WI_POSTINC:
 if (tricore_feature(env, TRICORE_FEATURE_131)) {
-tcg_gen_addi_tl(cpu_gpr_d[r2], cpu_gpr_d[r2], off10);
+tcg_gen_addi_tl(cpu_gpr_a[r2], cpu_gpr_a[r2], off10);
 } /* TODO: else raise illegal opcode trap */
 break;
 case OPC2_32_BO_CACHEI_W_PREINC:
 case OPC2_32_BO_CACHEI_WI_PREINC:
 if (tricore_feature(env, TRICORE_FEATURE_131)) {
-tcg_gen_addi_tl(cpu_gpr_d[r2], cpu_gpr_d[r2], off10);
+tcg_gen_addi_tl(cpu_gpr_a[r2], cpu_gpr_a[r2], off10);
 } /* TODO: else raise illegal opcode trap */
 break;
 case OPC2_32_BO_ST_A_SHORTOFF:
-- 
2.3.4