Re: [Qemu-devel] virtio-net regression [was: syslinux vs. OVMF]
On 04/10/15 16:36, Laszlo Ersek wrote: > Anyway I think we can rule out any qemu regression at this point. > It's a bug in some other component that the different memory map (due > to the larger, 0x2 allocation) exposes. It was an iPXE issue: http://thread.gmane.org/gmane.network.ipxe.devel/3918 Thanks & sorry about the noise. Laszlo
Re: [Qemu-devel] [PATCH 01/12 v9] linux-user: tilegx: Firstly add architecture related features
Firstly, thank you very much for your patient work for all related patches. And I shall try to send patch v10 within this month, and let linux-user run "Hello world" completely (finish emulate a demo executable binary successfully). The related reply is below: On 4/10/15 05:21, Peter Maydell wrote: > On 27 March 2015 at 10:48, Chen Gang wrote: >> 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. */ > > This is true but a slightly odd place to say so. > OK, thanks, I shall remove it. >> +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 */ > > Why aren't we using an array, the way the kernel does? > OK, thanks. I shall do it like pt_reg have done. >> +tilegx_reg_t tp; /* thread-local data */ >> +tilegx_reg_t sp; /* stack pointer */ >> +tilegx_reg_t lr; /* lr pointer */ > > This is missing a bunch of stuff from the kernel uapi > pt_regs type, which is bad because this struct is part > of the user-facing ABI (it gets used in signal handling). > OK, thanks. And I guess, sigcontext is a little betther than pt_regs. >> diff --git a/linux-user/tilegx/termbits.h b/linux-user/tilegx/termbits.h >> new file mode 100644 >> index 000..c11ce3e >> --- /dev/null >> +++ b/linux-user/tilegx/termbits.h > >> +#define TARGET_TIOCNOTTY0x5422 >> +#define TARGET_TIOCSETD 0x5423 >> +#define TARGET_TIOCGETD 0x5424 >> +#define TARGET_TCSBRKP 0x5425 >> +#define TARGET_TIOCSBRK 0x5427 >> +#define TARGET_TIOCCBRK 0x5428 >> +#define TARGET_TIOCGSID 0x5429 >> +#define TARGET_TCGETS2 _IOR('T', 0x2A, struct termios2) > > You probably mean TARGET_IOR/TARGET_IOW here and below. > > OK, thanks. I shall use TARGET_IOR/TARGET_IOW instead of _IOR/_IOW. >> +#define TARGET_TCSETS2 _IOW('T', 0x2B, struct termios2) >> +#define TARGET_TCSETSW2 _IOW('T', 0x2C, struct termios2) >> +#define TARGET_TCSETSF2 _IOW('T', 0x2D, struct termios2) >> +#define TARGET_TIOCGRS485 0x542E >> +#define TARGET_TIOCSRS485 0x542F >> +#define TARGET_TIOCGPTN _IOR('T', 0x30, unsigned int) >> +#define TARGET_TIOCSPTLCK _IOW('T', 0x31, int) >> +#define TARGET_TIOCGDEV _IOR('T', 0x32, unsigned int) >> +#define TARGET
[Qemu-devel] [PATCH v4] target-i386: Register QOM properties for feature flags
This uses the feature name arrays to register QOM properties for feature flags. This simply adds properties that can be configured using -global, but doesn't change x86_cpu_parse_featurestr() to use them yet. Signed-off-by: Eduardo Habkost --- Changes v1 -> v2: * Use "cpuid-" prefix instead of "feat-" * Register release function for property * Convert '_' to '-' on feature name before registering property * Add dev->realized check to property setter Changes v2 -> v3: * Register alias properties for feature name aliases * Patch is based on x86 tree, at: https://github.com/ehabkost/qemu.git x86 Changes v3 -> v4: * Remove "cpuid-" prefix altogether --- target-i386/cpu.c | 115 ++ 1 file changed, 115 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e657f10..1d97656 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2848,12 +2848,120 @@ out: } } +typedef struct FeatureProperty { +FeatureWord word; +uint32_t mask; +} FeatureProperty; + +static void x86_cpu_get_feature_prop(Object *obj, + struct Visitor *v, + void *opaque, + const char *name, + Error **errp) +{ +X86CPU *cpu = X86_CPU(obj); +CPUX86State *env = &cpu->env; +FeatureProperty *fp = opaque; +bool value = (env->features[fp->word] & fp->mask) == fp->mask; +visit_type_bool(v, &value, name, errp); +} + +static void x86_cpu_set_feature_prop(Object *obj, + struct Visitor *v, + void *opaque, + const char *name, + Error **errp) +{ +X86CPU *cpu = X86_CPU(obj); +DeviceState *dev = DEVICE(obj); +CPUX86State *env = &cpu->env; +FeatureProperty *fp = opaque; +bool value; + +if (dev->realized) { +qdev_prop_set_after_realize(dev, name, errp); +return; +} + +visit_type_bool(v, &value, name, errp); +if (value) { +env->features[fp->word] |= fp->mask; +} else { +env->features[fp->word] &= ~fp->mask; +} +} + +static void x86_cpu_release_feature_prop(Object *obj, const char *name, + void *opaque) +{ +FeatureProperty *prop = opaque; +g_free(prop); +} + +/* Register a boolean feature-bits property. + * If mask has multiple bits, all must be set for the property to return true. + * The same property name can be registered multiple times to make it affect + * multiple bits in the same FeatureWord. + */ +static void x86_cpu_register_feature_prop(X86CPU *cpu, + const char *prop_name, + FeatureWord w, + uint32_t mask) +{ +FeatureProperty *fp; +ObjectProperty *op; +op = object_property_find(OBJECT(cpu), prop_name, NULL); +if (op) { +fp = op->opaque; +assert(fp->word == w); +fp->mask |= mask; +} else { +fp = g_new0(FeatureProperty, 1); +fp->word = w; +fp->mask = mask; +object_property_add(OBJECT(cpu), prop_name, "bool", +x86_cpu_get_feature_prop, +x86_cpu_set_feature_prop, +x86_cpu_release_feature_prop, fp, &error_abort); +} +} + +static void x86_cpu_register_feature_bit_props(X86CPU *cpu, + FeatureWord w, + int bit) +{ +Object *obj = OBJECT(cpu); +int i; +char **names; +FeatureWordInfo *fi = &feature_word_info[w]; + +if (!fi->feat_names) { +return; +} +if (!fi->feat_names[bit]) { +return; +} + +names = g_strsplit(fi->feat_names[bit], "|", 0); + +feat2prop(names[0]); +x86_cpu_register_feature_prop(cpu, names[0], w, (1UL << bit)); + +for (i = 1; names[i]; i++) { +feat2prop(names[i]); +object_property_add_alias(obj, names[i], obj, names[0], &error_abort); +} + +g_strfreev(names); +} + static void x86_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); X86CPU *cpu = X86_CPU(obj); X86CPUClass *xcc = X86_CPU_GET_CLASS(obj); CPUX86State *env = &cpu->env; +FeatureWord w; static int inited; cs->env_ptr = env; @@ -2894,6 +3002,13 @@ static void x86_cpu_initfn(Object *obj) cpu->apic_id = -1; #endif +for (w = 0; w < FEATURE_WORDS; w++) { +int bit; +for (bit = 0; bit < 32; bit++) { +x86_cpu_register_feature_bit_props(cpu, w, bit); +} +} + x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort); /* init various static tables used in TCG mode */ -- 2.1.0
[Qemu-devel] [PATCH v6 38/36] qapi: Check for member name conflicts with a base class
Our type inheritance for both 'struct' and for flat 'union' merges key/value pairs from the base class with those from the type in question. Although the C code currently boxes things so that there is a distinction between which member is referred to, the QMP wire format does not allow passing a key more than once in a single object. Besides, if we ever change the generated C code to not be quite so boxy, we'd want to avoid duplicate member names there, too. Fix a testsuite entry added in an earlier patch, as well as adding a couple more tests to ensure we have appropriate coverage. Signed-off-by: Eric Blake --- v6: new patch --- scripts/qapi.py| 21 - tests/Makefile | 3 ++- tests/qapi-schema/flat-union-branch-clash.err | 1 + tests/qapi-schema/flat-union-branch-clash.exit | 2 +- tests/qapi-schema/flat-union-branch-clash.json | 2 +- tests/qapi-schema/flat-union-branch-clash.out | 9 - tests/qapi-schema/struct-base-clash-deep.err | 1 + tests/qapi-schema/struct-base-clash-deep.exit | 1 + tests/qapi-schema/struct-base-clash-deep.json | 9 + tests/qapi-schema/struct-base-clash-deep.out | 0 tests/qapi-schema/struct-base-clash.err| 1 + tests/qapi-schema/struct-base-clash.exit | 1 + tests/qapi-schema/struct-base-clash.json | 6 ++ tests/qapi-schema/struct-base-clash.out| 0 14 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 tests/qapi-schema/struct-base-clash-deep.err create mode 100644 tests/qapi-schema/struct-base-clash-deep.exit create mode 100644 tests/qapi-schema/struct-base-clash-deep.json create mode 100644 tests/qapi-schema/struct-base-clash-deep.out create mode 100644 tests/qapi-schema/struct-base-clash.err create mode 100644 tests/qapi-schema/struct-base-clash.exit create mode 100644 tests/qapi-schema/struct-base-clash.json create mode 100644 tests/qapi-schema/struct-base-clash.out diff --git a/scripts/qapi.py b/scripts/qapi.py index 853f9a3..281e762 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -429,6 +429,18 @@ def check_type(expr_info, source, value, allow_array = False, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) +def check_member_clash(expr_info, base_name, data, source = ""): +base = find_struct(base_name) +assert base +base_members = base['data'] +for key in data.keys(): +if key in base_members: +raise QAPIExprError(expr_info, +"Member name '%s'%s clashes with base '%s'" +%(key, source, base_name)) +if base.get('base'): +check_member_clash(expr_info, base['base'], data, source) + def check_command(expr, expr_info): name = expr['command'] allow_star = expr.has_key('gen') @@ -518,9 +530,14 @@ def check_union(expr, expr_info): check_name(expr_info, "Member of union '%s'" % name, key) # Each value must name a known type; futhermore, in flat unions, -# branches must be a struct +# branches must be a struct with no overlapping member names check_type(expr_info, "Member '%s' of union '%s'" % (key, name), value, allow_array=True, allow_metas=allow_metas) +if base: +branch_struct = find_struct(value) +assert branch_struct +check_member_clash(expr_info, expr['base'], branch_struct['data'], + " of branch '%s'" %key) # If the discriminator names an enum type, then all members # of 'data' must also be members of the enum type. @@ -597,6 +614,8 @@ def check_struct(expr, expr_info): allow_dict=True, allow_optional=True) check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'), allow_metas=['struct']) +if expr.get('base'): +check_member_clash(expr_info, expr['base'], expr['data']) def check_exprs(schema): for expr_elem in schema.exprs: diff --git a/tests/Makefile b/tests/Makefile index 0cd114f..eb5655e 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -242,7 +242,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ include-simple.json include-relpath.json include-format-err.json \ include-non-file.json include-no-file.json include-before-err.json \ include-nested-err.json include-self-cycle.json include-cycle.json \ - include-repetition.json event-nest-struct.json event-case.json) + include-repetition.json event-nest-struct.json event-case.json \ + struct-base-clash.json struct-base-clash-deep.json ) GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \ tests/test-qmp-commands.h tests/test-qapi-event.h diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-cl
[Qemu-devel] [PATCH v6 37/36] qapi: Support (subset of) \u escapes in strings
The handling of \ inside QAPI strings was less than ideal, and really only worked JSON's \/, \\, \", and our extension of \' (an obvious extension, when you realize we use '' instead of "" for strings). For other things, like '\n', it resulted in a literal 'n' instead of a newline. Of course, at the moment, we really have no use for escaped characters, as QAPI has to map to C identifiers, and we currently support ASCII only for that. But down the road, we may add support for default values for string parameters to a command or struct; if that happens, it would be nice to correctly support all JSON escape sequences, such as \n or \u. This gets us closer, by supporting Unicode escapes in the ASCII range. Since JSON does not require \OCTAL or \xXX escapes, I did not add it here, but it would be an easy addition if we desired it. Signed-off-by: Eric Blake --- v6: new patch --- scripts/qapi.py | 33 +++- tests/Makefile | 1 + tests/qapi-schema/escape-too-big.err | 1 + tests/qapi-schema/escape-too-big.exit| 1 + tests/qapi-schema/escape-too-big.json| 3 +++ tests/qapi-schema/escape-too-big.out | 0 tests/qapi-schema/escape-too-short.err | 1 + tests/qapi-schema/escape-too-short.exit | 1 + tests/qapi-schema/escape-too-short.json | 3 +++ tests/qapi-schema/escape-too-short.out | 0 tests/qapi-schema/ident-with-escape.err | 1 - tests/qapi-schema/ident-with-escape.exit | 2 +- tests/qapi-schema/ident-with-escape.json | 2 +- tests/qapi-schema/ident-with-escape.out | 3 +++ tests/qapi-schema/unicode-str.err| 1 + tests/qapi-schema/unicode-str.exit | 1 + tests/qapi-schema/unicode-str.json | 2 ++ tests/qapi-schema/unicode-str.out| 0 18 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 tests/qapi-schema/escape-too-big.err create mode 100644 tests/qapi-schema/escape-too-big.exit create mode 100644 tests/qapi-schema/escape-too-big.json create mode 100644 tests/qapi-schema/escape-too-big.out create mode 100644 tests/qapi-schema/escape-too-short.err create mode 100644 tests/qapi-schema/escape-too-short.exit create mode 100644 tests/qapi-schema/escape-too-short.json create mode 100644 tests/qapi-schema/escape-too-short.out create mode 100644 tests/qapi-schema/unicode-str.err create mode 100644 tests/qapi-schema/unicode-str.exit create mode 100644 tests/qapi-schema/unicode-str.json create mode 100644 tests/qapi-schema/unicode-str.out diff --git a/scripts/qapi.py b/scripts/qapi.py index 60ed34a..853f9a3 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -173,7 +173,38 @@ class QAPISchema: raise QAPISchemaError(self, 'Missing terminating "\'"') if esc: -string += ch +if ch == 'b': +string += '\b' +elif ch == 'f': +string += '\f' +elif ch == 'n': +string += '\n' +elif ch == 'r': +string += '\r' +elif ch == 't': +string += '\t' +elif ch == 'u': +value = 0 +for x in range(0, 4): +ch = self.src[self.cursor] +self.cursor += 1 +if ch not in "0123456789abcdefABCDEF": +raise QAPISchemaError(self, + '\\u escape needs 4 ' + 'hex digits') +value = (value << 4) + int(ch, 16) +# If Python 2 and 3 didn't disagree so much on +# how to handle Unicode, then we could allow +# Unicode string defaults. But most of QAPI is +# ASCII-only, so we aren't losing much for now. +if value > 0x7f: +raise QAPISchemaError(self, + 'For now, \\u escape ' + 'only supports values ' + 'up to \\u007f') +string += chr(value) +else: +string += ch esc = False elif ch == "\\": esc = True diff --git a/tests/Makefile b/tests/Makefile index f37cd01..0cd114f 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -212,6 +212,7 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
Re: [Qemu-devel] [PATCH 02/12 v9] linux-user: tilegx: Add target features support within qemu
On 4/10/15 05:31, Peter Maydell wrote: > On 27 March 2015 at 10:49, Chen Gang wrote: [...] >> +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; > > This is slightly confusing, because the kernel code we're matching here > doesn't call this register RE, it just uses 0: > childregs->regs[0] = 0; /* return value is zero */ > TILEGX_R_RE is just 0, I define a macro instead the hardcode number in cpu.h with the comment for it. [...] >> + >> +/* 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; > > Where does this come from? It doesn't match the kernel's > generic-headers struct layout. > Oh, sorry, originally, I guess, I only copied it from microblaze, did not check kernel. I shall use generic-headers which tilegx will use (the result will like alpha has done): typedef struct target_sigaltstack { abi_ulong ss_sp; int32_t ss_flags; int32_t dummy; abi_ulong ss_size; } target_stack_t; [...] >> + >> +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; >> +}; > > Again, doesn't seem to match kernel? > For me, it matches kernel. mode is abi_uint (__kernel_mode_t is 32-bit). Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 03/12 v9] linux-user: Support tilegx architecture in syscall
On 4/10/15 05:38, Peter Maydell wrote: > On 27 March 2015 at 10:50, Chen Gang wrote: [...] >> >> +#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; >> +}; > > We already have the generic stat struct for TARGET_OPENRISC, > so you should just add TILEGX to the #if for that. (Pretty > sure you want the stat64 struct too, or at least that it > won't hurt.) > OK, thanks, it sounds a good idea to me. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 04/12 v9] linux-user: Support tilegx architecture in linux-user
On 4/10/15 05:44, Peter Maydell wrote: > On 27 March 2015 at 10:52, Chen Gang wrote: [...] >> + >> +#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; > > This is wrong (see later). > >> +regs->sp = infop->start_stack; >> + >> +} >> + [...] >> >> +#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); > > Calling exit() with negative values is never right (exit codes > are always positive), and in any case this is the wrong way to > handle a "can't happen" case in code. If we can never get here > then you want > g_assert_not_reached(); > OK, thanks. [...] >> +#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 */ > > This is why you should have declared target_pt_regs > with an array, because then you can use a loop to > do this initialization. > OK, thanks. >> +env->regs[53] = regs->tp; /* TILEGX_R_TP */ >> +env->regs[54] = regs->sp; /* TILEGX_R_SP */ >> +env->regs[55] = regs->lr; /* TILEGX_R_LR */ >> +env->pc = regs->lr; > > Er, what? You should set the env->pc from the entry > in target_pt_regs that corresponds to the PC, not the LR. > (Which in turn means you need to set that field, not LR, > in init_thread().) > OK, thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 05/12 v9] linux-user/syscall.c: conditionalize syscalls which are not defined in tilegx
On 4/10/15 05:46, Peter Maydell wrote: > On 27 March 2015 at 10:53, Chen Gang wrote: >> For tilegx, several syscall macros are not supported, so switch them to >> avoid building break. >> >> Signed-off-by: Chen Gang > >> +#ifdef TARGET_NR_getpgrp >> case TARGET_NR_getpgrp: >> ret = get_errno(getpgrp()); >> break; >> case TARGET_NR_setsid: >> ret = get_errno(setsid()); >> break; >> +#endif > > This looks wrong -- your ifdef for NR_getpgrp is covering > the setsid syscall too. > OK, thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
[Qemu-devel] [PATCH] qapi: Drop dead genlist parameter
Defaulting a parameter to True, then having all callers omit or pass an explicit True for that parameter, is pointless. Looks like it has been dead since introduction in commit 06d64c6. Signed-off-by: Eric Blake --- scripts/qapi-visit.py | 36 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index f5d4355..6156162 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -369,34 +369,31 @@ out: return ret -def generate_declaration(name, members, genlist=True, builtin_type=False): +def generate_declaration(name, members, builtin_type=False): ret = "" if not builtin_type: ret += mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp); ''', -name=name) - -if genlist: -ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); -''', - name=name) - -return ret - -def generate_enum_declaration(name, members, genlist=True): -ret = "" -if genlist: -ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); -''', name=name) +ret += mcgen(''' +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); +''', + name=name) + +return ret + +def generate_enum_declaration(name, members): +ret = mcgen(''' +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); +''', +name=name) + return ret -def generate_decl_enum(name, members, genlist=True): +def generate_decl_enum(name, members): return mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp); @@ -510,8 +507,7 @@ exprs = parse_schema(input_file) # for built-in types in our header files and simply guard them fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL")) for typename in builtin_types.keys(): -fdecl.write(generate_declaration(typename, None, genlist=True, - builtin_type=True)) +fdecl.write(generate_declaration(typename, None, builtin_type=True)) fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL")) # ...this doesn't work for cases where we link in multiple objects that -- 2.1.0
Re: [Qemu-devel] [PATCH 06/12 v9] target-tilegx: Add cpu basic features for linux-user
On 4/10/15 05:55, Peter Maydell wrote: > On 27 March 2015 at 10:54, Chen Gang wrote: >> 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 ++ > > You don't really need a separate cpu-qom.h and cpu.h -- that's > just the way we've ended up with for the older targets which > got converted to QOM for legacy reasons. See target-moxie/ > for an example which combines the two headers. > > OK, thanks. >> +static const VMStateDescription vmstate_tilegx_cpu = { >> +.name = "cpu", >> +.unmigratable = 1, >> +}; > > I'd prefer to see a correct VMState from the start -- it's > not very difficult. Migration/snapshotting is much easier > to enforce at the point where we let code in to the tree > than if we let in non-migratable devices and CPUs and then > have to fix them up later... > > OK, thanks. I shall try. [...] >> + >> +#include "exec/cpu-defs.h" >> +#include "fpu/softfloat.h" > > What's the softfloat include for? > OK, thanks. I shall remove it. [...] >> + >> +/* TILE-Gx memory attributes */ >> +#define TARGET_PAGE_BITS 16 /* TILE-Gx uses 64KB page size */ >> +#define MMAP_SHIFT TARGET_PAGE_BITS >> +#define TARGET_PHYS_ADDR_SPACE_BITS 42 /* TILE-Gx is 42 bit physical >> address */ >> +#define TARGET_VIRT_ADDR_SPACE_BITS 64 /* TILE-Gx has 64 bit virtual >> address */ > > nitpick: "has [...] addresses" is the correct grammar in both these comments. > OK, thanks. >> +#define MMU_USER_IDX0 /* independent from both qemu and architecture */ > > Not sure what you mean with this comment? > OK, thanks. I shall remove the comment. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH] qapi: Drop dead genlist parameter
On 04/10/2015 02:59 PM, Eric Blake wrote: > Defaulting a parameter to True, then having all callers omit > or pass an explicit True for that parameter, is pointless. > Looks like it has been dead since introduction in commit 06d64c6. > > Signed-off-by: Eric Blake > --- > scripts/qapi-visit.py | 36 > 1 file changed, 16 insertions(+), 20 deletions(-) > @@ -510,8 +507,7 @@ exprs = parse_schema(input_file) > # for built-in types in our header files and simply guard them > fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL")) > for typename in builtin_types.keys(): Sorry; this doesn't apply to current master. I'll send a v2 that doesn't depend on my pending qapi nested struct series. > -fdecl.write(generate_declaration(typename, None, genlist=True, > - builtin_type=True)) > +fdecl.write(generate_declaration(typename, None, builtin_type=True)) > fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL")) > > # ...this doesn't work for cases where we link in multiple objects that > -- 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 08/12 v9] target-tilegx: Add opcode basic implementation for tilegx
On 4/10/15 06:03, Peter Maydell wrote: > On 27 March 2015 at 10:56, Chen Gang wrote: >> It is from Tilera Corporation, and copied from Linux kernel "arch/tile/ >> include/uapi/arch/opcode_tilegx.h". >> >> Signed-off-by: Chen Gang > > It's good to have this import as a single "this is the upstream > kernel header with no changes at all" patch, but you then > need a following patch that fixes it up so it will work with QEMU. > Specifically, all the uses of "__inline" need to be changed to > "inline". > OK, thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
[Qemu-devel] [PATCH v2] qapi: Drop dead genlist parameter
Defaulting a parameter to True, then having all callers omit or pass an explicit True for that parameter, is pointless. Looks like it has been dead since introduction in commit 06d64c6, more than 4 years ago. Signed-off-by: Eric Blake --- v2: tweak commit message, don't depend on pending series scripts/qapi-visit.py | 38 +- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 8f845a2..1be4d67 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -2,7 +2,7 @@ # QAPI visitor generator # # Copyright IBM, Corp. 2011 -# Copyright (C) 2014 Red Hat, Inc. +# Copyright (C) 2014-2015 Red Hat, Inc. # # Authors: # Anthony Liguori @@ -401,34 +401,31 @@ out: return ret -def generate_declaration(name, members, genlist=True, builtin_type=False): +def generate_declaration(name, members, builtin_type=False): ret = "" if not builtin_type: ret += mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp); ''', -name=name) - -if genlist: -ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); -''', - name=name) - -return ret - -def generate_enum_declaration(name, members, genlist=True): -ret = "" -if genlist: -ret += mcgen(''' -void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); -''', name=name) +ret += mcgen(''' +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); +''', + name=name) + +return ret + +def generate_enum_declaration(name, members): +ret = mcgen(''' +void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp); +''', +name=name) + return ret -def generate_decl_enum(name, members, genlist=True): +def generate_decl_enum(name, members): return mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp); @@ -542,8 +539,7 @@ exprs = parse_schema(input_file) # for built-in types in our header files and simply guard them fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL")) for typename in builtin_types: -fdecl.write(generate_declaration(typename, None, genlist=True, - builtin_type=True)) +fdecl.write(generate_declaration(typename, None, builtin_type=True)) fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL")) # ...this doesn't work for cases where we link in multiple objects that -- 2.1.0
Re: [Qemu-devel] [PATCH 09/12 v9] target-tilegx: Finish processing bundle and preparing decoding pipes
On 4/10/15 06:08, Peter Maydell wrote: > On 27 March 2015 at 10:57, Chen Gang wrote: >> Finish processing tilegx bundle, and reach to related pipes. > >> +qemu_log_mask(LOG_UNIMP, >> + "UNIMP y0, opcode %d, bundle [" FMT64X "]\n", >> + opcode, (uint64_t)bundle); > > I feel like we should change the typedef of tilegx_bundle_bits to > uint64_t, then you could avoid all these casts. > OK, thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 10/12 v9] target-tilegx: Add TILE-Gx building files
On 4/10/15 06:10, Peter Maydell wrote: > On 27 March 2015 at 11:00, Chen Gang wrote: [...] >>;; >> + tilegx) >> +TARGET_ARCH=tilegx >> + ;; > > TARGET_ARCH already defaults to the target name, so you don't > need to set it again unless it's different. You can just have > an empty case here, like unicore32 does immediately below: > >>unicore32) >>;; >>xtensa|xtensaeb) > OK, thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 12/12 v9] target-tilegx: Generate tcg instructions to execute to 1st system call
On 4/10/15 06:19, Peter Maydell wrote: > On 27 March 2015 at 11:07, Chen Gang wrote: [...] >> >> +static void gen_fnop(void) >> +{ >> +qemu_log_mask(CPU_LOG_TB_IN_ASM, "(f)nop\n"); > > I really don't much like mixing a fake disassembler in > with the translator. If you want a disassembler, write one > and put it in disas/... > For me, it is necessary (for I am not quite sure whether my disassemble must be correct). It is only for tracing. >> +} >> + >> +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)); >> +} > > This is hugely repetitive. Write a common function that takes a > TCG_COND_* as a parameter. > OK, thanks. >> + >> +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); > > This cast is pointless, because the 'imm' argument to > gen_addimm() already has type int64_t. > OK, thanks. >> +} >> + >> +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); > > Another pointless cast. > OK, thanks. [...] >> + >> +/* >> + * The related functional description for bfextu in isa document: >> + * >> + * uint64_t mask = 0; >> + * mask = (-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1); >> + * uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart) >> + *| (rf[SrcA] << (64 - BFStart)); >> + * rf[Dest] = rot_src & mask; >> + */ >> +static void gen_bfextu(struct DisasContext *dc, uint8_t rdst, uint8_t rsrc, >> + int8_t start, int8_t end) >> +{ >> +uint64_t mask = (-1ULL) ^ ((-1ULL << ((end - start) & 63)) << 1); >> +TCGv tmp = dest_gr(dc, rdst); > > Are the start and end immediates here limited such that we're > guaranteed not to hit any of C's undefined behaviour for out > of range shifts, and that we don't hit TCG's undefined-value > behaviour on bad rotates? > For me, it is correct, it is only the copy of the document, which has already considered about any cases (include C's undefined behaviour). >> static void decode_addi_opcode_y0(struct DisasContext *dc, >>tilegx_bundle_bits bundle) >> { >> +uint8_t rsrc = (uint8_t)get_SrcA_Y0(bundle); >> +uint8_t rdst = (uint8_t)get_Dest_Y0(bundle); >> +int8_t imm8 = (int8_t)get_Imm8_Y0(bundle); >
Re: [Qemu-devel] make test fails on Mac OS X
On 10 April 2015 at 18:56, Programmingkid wrote: > The 'make test' command fails to build on Mac OS X. This is because of a > missing header file. Here is the error: > > include/glib-compat.h:19:18: fatal error: glib.h: No such file or directory > #include "make test" builds a bunch of unmaintained TCG tests. Our actual working test suite is "make check". -- PMM
Re: [Qemu-devel] [PATCH 01/12 v9] linux-user: tilegx: Firstly add architecture related features
On 10 April 2015 at 21:01, Chen Gang wrote: > > Firstly, thank you very much for your patient work for all related > patches. > > And I shall try to send patch v10 within this month, and let linux-user > run "Hello world" completely (finish emulate a demo executable binary > successfully). > > The related reply is below: > > On 4/10/15 05:21, Peter Maydell wrote: >>> +tilegx_reg_t tp; /* thread-local data */ >>> +tilegx_reg_t sp; /* stack pointer */ >>> +tilegx_reg_t lr; /* lr pointer */ >> >> This is missing a bunch of stuff from the kernel uapi >> pt_regs type, which is bad because this struct is part >> of the user-facing ABI (it gets used in signal handling). >> > > OK, thanks. And I guess, sigcontext is a little betther than pt_regs. sigcontext is a different struct. The target_pt_regs struct should match the kernel's pt_regs struct, and the target_sigcontext should match the kernel's sigcontext struct. -- PMM
Re: [Qemu-devel] [PATCH 02/12 v9] linux-user: tilegx: Add target features support within qemu
On 10 April 2015 at 21:41, Chen Gang wrote: > On 4/10/15 05:31, Peter Maydell wrote: >> On 27 March 2015 at 10:49, Chen Gang wrote: >>> +typedef struct target_sigaltstack { >>> +abi_ulong ss_sp; >>> +abi_ulong ss_size; >>> +abi_long ss_flags; >>> +} target_stack_t; >> >> Where does this come from? It doesn't match the kernel's >> generic-headers struct layout. >> > > Oh, sorry, originally, I guess, I only copied it from microblaze, did > not check kernel. These structures are all user-guest-facing ABI, so they must match the kernel's structures for your target architecture. > I shall use generic-headers which tilegx will use (the result will like > alpha has done): > > typedef struct target_sigaltstack { > abi_ulong ss_sp; > int32_t ss_flags; > int32_t dummy; > abi_ulong ss_size; > } target_stack_t; This doesn't match the kernel either. http://lxr.free-electrons.com/source/include/uapi/asm-generic/signal.h#L111 You have a pointer, an int and a size_t, so you want abi_ulong ss_sp; abi_int ss_flags; abi_ulong ss_size; like aarch64. > > [...] >>> + >>> +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; >>> +}; >> >> Again, doesn't seem to match kernel? >> > > For me, it matches kernel. mode is abi_uint (__kernel_mode_t is 32-bit). I'm looking at http://lxr.free-electrons.com/source/include/uapi/linux/ipc.h#L9 which doesn't have that padding and unused fields at the end. However the ipc structs are pretty confusing so maybe that's the wrong one -- which one are you looking at? -- PMM
Re: [Qemu-devel] [PATCH 12/12 v9] target-tilegx: Generate tcg instructions to execute to 1st system call
On 10 April 2015 at 22:28, Chen Gang wrote: > On 4/10/15 06:19, Peter Maydell wrote: >> On 27 March 2015 at 11:07, Chen Gang wrote: >>> +/* >>> + * The related functional description for bfextu in isa document: >>> + * >>> + * uint64_t mask = 0; >>> + * mask = (-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1); >>> + * uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart) >>> + *| (rf[SrcA] << (64 - BFStart)); >>> + * rf[Dest] = rot_src & mask; >>> + */ >>> +static void gen_bfextu(struct DisasContext *dc, uint8_t rdst, uint8_t rsrc, >>> + int8_t start, int8_t end) >>> +{ >>> +uint64_t mask = (-1ULL) ^ ((-1ULL << ((end - start) & 63)) << 1); >>> +TCGv tmp = dest_gr(dc, rdst); >> >> Are the start and end immediates here limited such that we're >> guaranteed not to hit any of C's undefined behaviour for out >> of range shifts, and that we don't hit TCG's undefined-value >> behaviour on bad rotates? >> > > For me, it is correct, it is only the copy of the document, which has > already considered about any cases (include C's undefined behaviour). Even if the ISA document implicitly (or explicitly) permits some values of start and end to be undefined behaviour of the CPU, you must not either have C code in translate.c that does undefined behaviour, or emit generated code that is undefined behaviour by TCG's rules (undefined values may be OK). You need to make sure QEMU can't crash or misbehave if the guest passes us badly encoded or meaningless instructions. You need to work through the possibilities and convince yourselves (and us) that everything is correctly handled. It's not enough to just say "it's a copy of the C code from the ISA so it must be OK". thanks -- PMM
Re: [Qemu-devel] [PATCH 02/12 v9] linux-user: tilegx: Add target features support within qemu
Am 10.04.2015 um 23:51 schrieb Peter Maydell: > On 10 April 2015 at 21:41, Chen Gang wrote: >> On 4/10/15 05:31, Peter Maydell wrote: >>> On 27 March 2015 at 10:49, Chen Gang wrote: +typedef struct target_sigaltstack { +abi_ulong ss_sp; +abi_ulong ss_size; +abi_long ss_flags; +} target_stack_t; >>> >>> Where does this come from? It doesn't match the kernel's >>> generic-headers struct layout. >>> >> >> Oh, sorry, originally, I guess, I only copied it from microblaze, did >> not check kernel. > > These structures are all user-guest-facing ABI, so they must > match the kernel's structures for your target architecture. > >> I shall use generic-headers which tilegx will use (the result will like >> alpha has done): >> >> typedef struct target_sigaltstack { >> abi_ulong ss_sp; >> int32_t ss_flags; >> int32_t dummy; >> abi_ulong ss_size; >> } target_stack_t; > > This doesn't match the kernel either. > > http://lxr.free-electrons.com/source/include/uapi/asm-generic/signal.h#L111 > > You have a pointer, an int and a size_t, so you want > abi_ulong ss_sp; > abi_int ss_flags; > abi_ulong ss_size; > > like aarch64. I know linux-user is a mess. But that does not sound appealing. If this is from a generic header, can't we put that in a shared header too and #include it from aarch64 and tilegx without duplicating it? Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH RFC 03/19] qapi: Rename _generate_enum_string() to camel_to_upper()
On 04/02/2015 11:28 AM, Markus Armbruster wrote: > Signed-off-by: Markus Armbruster > --- > scripts/qapi.py | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 02/12 v9] linux-user: tilegx: Add target features support within qemu
On 10 April 2015 at 22:59, Andreas Färber wrote: > I know linux-user is a mess. But that does not sound appealing. If this > is from a generic header, can't we put that in a shared header too and > #include it from aarch64 and tilegx without duplicating it? I certainly wouldn't object if somebody wanted to try to factor out some of our mess of ifdefs to produce something more like "structs defined in linux-user/$arch/$foo.h, which might just include linux-user/generic/$foo.h". I'm just not insisting on it as a blocker for a new target (would be a bit hypocritical given we didn't do it for aarch64). This is just one of the many cleanups I'd do in linux-user if I had 3 to 6 months to spend on refactoring and fixing it :-) -- PMM
Re: [Qemu-devel] seccomp breakage on arm
On Friday, April 10, 2015 05:40:40 PM Andreas Färber wrote: > My main concern - that you apparently misunderstood - was whether this > is a QEMU or a libseccomp issue. If it's on libseccomp's side then it's > less urgent for QEMU and any new configure checks are just candy IMO. My opinion is that the 32-bit ARM build issue you described is a libseccomp bug (see the bug fix I sent a few hours ago); QEMU's usage of libseccomp is perfectly reasonable. That said, the libseccomp bug clearly affected QEMU in a bad way. Making matters worse is that the problem wasn't caught until late in the QEMU release process, nobody likes surprises like this. We've corrected the issue in libseccomp, and it looks like the QEMU folks are reverting ARM/seccomp support so I think we've resolved this for the immediate future. Long term I think the libseccomp project can look at adding some additional automated tests so these issues will be caught at build/packaging time, further, I think the QEMU project can restore ARM/seccomp support in the next release and look at its own testing procedures to understand why this issue wasn't caught earlier as well. -- paul moore security @ redhat
Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection
On 04/02/2015 11:29 AM, Markus Armbruster wrote: > Caution, rough edges. > > qapi/introspect.json defines the introspection schema. It should do > for uses other than QMP. That is, QGA should also be able to reuse it for introspection. > FIXME it's almost entirely devoid of comments. Yeah, but it's only RFC quality, and if we alter design you don't have to worry about lengthy comments to keep in sync :) Of course, the final version ought to have good comments. > > The introspection schema does not reflect all the rules and > restrictions that apply to QAPI schemata. A valid QAPI schema has an > introspection value conforming to the introspection schema, but the > converse is not true. I can live with that. Introspection is output-only (we aren't creating new runtime commands by using introspection descriptions as input, so it doesn't matter if the introspection schema permits more than qapi will ever generate). > > Introspection lowers away a number of schema details: > > * The builtin types are declared with their JSON type. > > TODO should we map all the integer types to just int? Or even have introspection output two things: type ('int' for all JSON integers, regardless of range) and range ([0,255] for 'uint8'). I could live with that (ideally, knowing the range will help libvirt avoid passing in too-large-a-value to a parameter it introspected; but right now the idea is that most of libvirt's introspection will be "does something exist" not "what range does it support", and that libvirt will already have hard-coded knowledge of "if it exists, it is a uint8" without having to ask introspection for the type libvirt will supply). > > * Implicit type definitions are made explicit, and given > auto-generated names. These names start with ':' so they don't > clash with the user's names. Hopefully that works. Although having to parse for ':' is an argument that we are packing multiple pieces of information in one JSON name/value, which sometimes means we should have instead supplied two different name/values for the two different pieces of information. Oh well, I'll see more when I get into the schema part. Maybe we could also tweak the generator to put implicit names in the leading '_' namespace (we already outlaw use of leading '_' in all but downstream names, and require downstream names to use double '__', so we could easily identify '_generated' and '___generated' as internal vs. 'regular' and '__downstream' as user-supplied; it would free up current places where the qapi user cannot supply certain names. But not for this series). > > Example: a simple union implicitly defines an enumeration type for > its discriminator. > > * All type references are by name. Fine if the type is named; but what happens if the type is anonymous inline (as in many commands?) Also, in my nested struct cleanups, I found several places where it would be nice to patch the generator to support more anonymous inline types, such as in: { 'enum': 'MyEnum', 'data': [ 'one', 'two' ] } { 'union': 'Foo', 'base': { 'switch': 'MyEnum', 'name': 'str' }, 'discriminator': 'switch', data': { 'one': { 'value': 'int' }, 'two': { 'default': 'int' } } } It would even work for our long-hand of optional arguments: { 'command': 'foo', 'data': { 'bar': { 'type': { 'name': 'str', 'value': 'int' }, 'optional': true } } } I guess we can always go with anonymous inline types resulting in an implicit named type creation, and refer to that name, if cramming an anonymous type into the schema is too hard. > > * Base types are flattened. That's fine - we are introspecting what can be sent on the wire, and don't care what types were used in the qapi to arrive at that point. > > * The top type (named '**') can hold any value. > > It can currently occur only in commands, but the introspection > schema doesn't reflect that. > > * The struct and union types are generalized into an object type. I think you mentioned ideas on that in reviewing my v5 thread; they sounded reasonable at the time, so we'll see when I actually review the schema. > > * Commands take a single argument and return a single result. > > Dictionary argument/result or list result is an implicit type > definition. > > Missing argument/result is an implicit definition of the empty > object. > > The argument is always of object type, but the introspection schema > doesn't reflect that. > > The 'gen': false directive is omitted as implementation detail. Yep, that's fine. QAPI drives internal details that don't affect the wire format, and it's fine that introspection doesn't have to expose them. > > * Events carry a single data value. > > Implicit type definition as above. > > The value is of object type, but the introspection schema doesn't > reflect that. > > * Types not used by commands or events are omitted. > > Indirect use counts as use. > > * Optional members have a default, which
Re: [Qemu-devel] [PATCH 01/12 v9] linux-user: tilegx: Firstly add architecture related features
On 4/11/15 05:38, Peter Maydell wrote: > On 10 April 2015 at 21:01, Chen Gang wrote: >> >> Firstly, thank you very much for your patient work for all related >> patches. >> >> And I shall try to send patch v10 within this month, and let linux-user >> run "Hello world" completely (finish emulate a demo executable binary >> successfully). >> >> The related reply is below: >> >> On 4/10/15 05:21, Peter Maydell wrote: +tilegx_reg_t tp; /* thread-local data */ +tilegx_reg_t sp; /* stack pointer */ +tilegx_reg_t lr; /* lr pointer */ >>> >>> This is missing a bunch of stuff from the kernel uapi >>> pt_regs type, which is bad because this struct is part >>> of the user-facing ABI (it gets used in signal handling). >>> >> >> OK, thanks. And I guess, sigcontext is a little betther than pt_regs. > > sigcontext is a different struct. The target_pt_regs struct > should match the kernel's pt_regs struct, and the target_sigcontext > should match the kernel's sigcontext struct. > OK, Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 3/7] ipxe: add local patches
Am 10.04.2015 um 16:17 schrieb Gerd Hoffmann: > There are two ipxe patches needed to make efi pxe boots work. > They didn't made it upstream yet, and I don't want to wait any > longer with updating qemu. So add them here, with some logic > to apply them before building ipxe. > > /me still hopes I can revert that patch some day. > > Signed-off-by: Gerd Hoffmann > --- > roms/Makefile | 12 +- > ...rove-compliance-with-the-EFI_SIMPLE_NETWO.patch | 160 > + > ...0002-efi-make-load-file-protocol-optional.patch | 102 + > 3 files changed, 271 insertions(+), 3 deletions(-) > create mode 100644 > roms/ipxe-patches/0001-efi_snp-improve-compliance-with-the-EFI_SIMPLE_NETWO.patch > create mode 100644 > roms/ipxe-patches/0002-efi-make-load-file-protocol-optional.patch > > diff --git a/roms/Makefile b/roms/Makefile > index 461cb49..ab4532c 100644 > --- a/roms/Makefile > +++ b/roms/Makefile > @@ -115,12 +115,12 @@ efi-rom-%: build-pxe-roms build-efi-roms > -ec ipxe/src/bin-x86_64-efi/$(VID)$(DID).efidrv \ > -o ../pc-bios/efi-$*.rom > > -build-pxe-roms: ipxe/src/config/local/general.h > +build-pxe-roms: ipxe/qemu-patches ipxe/src/config/local/general.h > $(MAKE) -C ipxe/src GITVERSION="" \ > CROSS_COMPILE=$(x86_64_cross_prefix) \ > $(patsubst %,bin/%.rom,$(pxerom_targets)) > > -build-efi-roms: build-pxe-roms ipxe/src/config/local/general.h > +build-efi-roms: ipxe/qemu-patches build-pxe-roms > ipxe/src/config/local/general.h > $(MAKE) -C ipxe/src GITVERSION="" \ > CROSS_COMPILE=$(x86_64_cross_prefix) \ > $(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \ > @@ -129,6 +129,12 @@ build-efi-roms: build-pxe-roms > ipxe/src/config/local/general.h > ipxe/src/config/local/%: config.ipxe.% > cp $< $@ > > +ipxe/qemu-patches: Looks like this is only ever removed by clean? Should depend on the patch files, in case they change with an update. But why are you adding patch files in the first place? Can't we just push the commits to a branch on git.qemu-project.org and update the submodule config accordingly? Regards, Andreas > + for patch in ipxe-patches/*; do \ > + echo "# applying $$patch"; \ > + cat $$patch | (cd ipxe; patch -p1); \ > + done > + touch $@ > > slof: > $(MAKE) -C SLOF CROSS=$(powerpc64_cross_prefix) qemu > @@ -148,6 +154,6 @@ clean: > $(MAKE) -C sgabios clean > rm -f sgabios/.depend > $(MAKE) -C ipxe/src veryclean > - (cd ipxe; rm -f src/config/local/*.h) > + (cd ipxe; git reset --hard; rm -f qemu-patches src/config/local/*.h) > $(MAKE) -C SLOF clean > rm -rf u-boot/build.e500 [snip] -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH 12/12 v9] target-tilegx: Generate tcg instructions to execute to 1st system call
On 4/11/15 05:56, Peter Maydell wrote: > On 10 April 2015 at 22:28, Chen Gang wrote: >> On 4/10/15 06:19, Peter Maydell wrote: >>> On 27 March 2015 at 11:07, Chen Gang wrote: +/* + * The related functional description for bfextu in isa document: + * + * uint64_t mask = 0; + * mask = (-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1); + * uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart) + *| (rf[SrcA] << (64 - BFStart)); + * rf[Dest] = rot_src & mask; + */ +static void gen_bfextu(struct DisasContext *dc, uint8_t rdst, uint8_t rsrc, + int8_t start, int8_t end) +{ +uint64_t mask = (-1ULL) ^ ((-1ULL << ((end - start) & 63)) << 1); +TCGv tmp = dest_gr(dc, rdst); >>> >>> Are the start and end immediates here limited such that we're >>> guaranteed not to hit any of C's undefined behaviour for out >>> of range shifts, and that we don't hit TCG's undefined-value >>> behaviour on bad rotates? >>> >> >> For me, it is correct, it is only the copy of the document, which has >> already considered about any cases (include C's undefined behaviour). > > Even if the ISA document implicitly (or explicitly) permits > some values of start and end to be undefined behaviour of the > CPU, you must not either have C code in translate.c that does > undefined behaviour, or emit generated code that is undefined > behaviour by TCG's rules (undefined values may be OK). You > need to make sure QEMU can't crash or misbehave if the guest > passes us badly encoded or meaningless instructions. > > You need to work through the possibilities and convince > yourselves (and us) that everything is correctly handled. > It's not enough to just say "it's a copy of the C code from > the ISA so it must be OK". > Oh, maybe we misunderstood the "C's undefined behaviour". in our case: - if end < start, it is OK (it is also one of important cases). - start and end are within 0 - 63 (they have 6 bits), but they still need "& 63" for end < start case. (I guess, it will be better to use uint8_t instead of my original int8_t for start and end). The related introduction is: bfextu Bit field Extract Unsigned Syntax bfextu Dest, SrcA, BFStart, BFEnd Example bfextu r5, r6, 5, 7 Description Extract and right justify the specified bit field of the destination operand. The bit field is specified by the BFStart and BFEnd immediate operands, which contain the bit fields starting and ending bit positions. If the start position is less than or equal to the end position, then the field contains bits from start bit position up to and including the ending bit position. If the start position is greater than the end position, then the field contains the bits start bit position up to the WORD_SIZE bit position, and from the zero bit position up to the end bit position. Functional Description uint64_t mask = 0; mask = ((-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1)); uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart) | (rf[SrcA] << (64 - BFStart)); rf[Dest] = rot_src & mask; Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
Re: [Qemu-devel] [PATCH 02/12 v9] linux-user: tilegx: Add target features support within qemu
On 4/11/15 05:51, Peter Maydell wrote: > On 10 April 2015 at 21:41, Chen Gang wrote: >> On 4/10/15 05:31, Peter Maydell wrote: >>> On 27 March 2015 at 10:49, Chen Gang wrote: +typedef struct target_sigaltstack { +abi_ulong ss_sp; +abi_ulong ss_size; +abi_long ss_flags; +} target_stack_t; >>> >>> Where does this come from? It doesn't match the kernel's >>> generic-headers struct layout. >>> >> >> Oh, sorry, originally, I guess, I only copied it from microblaze, did >> not check kernel. > > These structures are all user-guest-facing ABI, so they must > match the kernel's structures for your target architecture. > >> I shall use generic-headers which tilegx will use (the result will like >> alpha has done): >> >> typedef struct target_sigaltstack { >> abi_ulong ss_sp; >> int32_t ss_flags; >> int32_t dummy; >> abi_ulong ss_size; >> } target_stack_t; > > This doesn't match the kernel either. > > http://lxr.free-electrons.com/source/include/uapi/asm-generic/signal.h#L111 > > You have a pointer, an int and a size_t, so you want > abi_ulong ss_sp; > abi_int ss_flags; > abi_ulong ss_size; > > like aarch64. > For me, for tilegx which is always 64-bit, add 'dummy' is more clearer (but need to use abi_int instead of original int32_t). And does it pragma packed ()? As far as I know, it doesn't. >> >> [...] + +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; +}; >>> >>> Again, doesn't seem to match kernel? >>> >> >> For me, it matches kernel. mode is abi_uint (__kernel_mode_t is 32-bit). > > I'm looking at > http://lxr.free-electrons.com/source/include/uapi/linux/ipc.h#L9 > which doesn't have that padding and unused fields at the end. > However the ipc structs are pretty confusing so maybe that's > the wrong one -- which one are you looking at? > I check the linux-next tree next-20150401 in include/uapi/linux/ipc.h, the __kernel_mode_t is unsigned int for tile (and also most of 64-bit targets in qemu, mode is 32-bit), please check again. it really has no "__pad2" and "__unused?", but after check all the other targets within qemu, they all have "__pad2" and "__unused?". May qemu itself need them? I am not quite sure, but for me, appending them is OK. Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
[Qemu-devel] Failing iotests in v2.3.0-rc2 / master
Hi, 001 seems to hang for -qcow (or is not reasonably "quick": >5 min). 033 is failing for -vhdx. (Note that `make check-block` only tests -qcow2, so didn't uncover either of them.) Given a failing test, am I seeing correctly that there is no command line option to skip this one failing test? -x seems to be for groups only. Regards, Andreas $ ./check -v -T -qcow -g quick [...] 001 6s ...[05:12:39] $ ./check -v -T -vhdx -g quick [...] 033 1s ...[04:06:09] [04:06:11] - output mismatch (see 033.out.bad) --- /home/andreas/QEMU/qemu/tests/qemu-iotests/033.out 2015-04-08 20:19:11.947290578 +0200 +++ 033.out.bad 2015-04-11 04:06:11.939300375 +0200 @@ -2,52 +2,46 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 == 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) +write failed: Operation not supported +write failed: Operation not supported +write failed: Operation not supported == verifying patterns (1) == +Pattern verification failed at offset 512, 512 bytes 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) +Pattern verification failed at offset 132096, 512 bytes 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) +write failed: Operation not supported +write failed: Operation not supported == 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) +write failed: Operation not supported +write failed: Operation not supported +write failed: Operation not supported == verifying patterns (1) == +Pattern verification failed at offset 512, 512 bytes 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) +Pattern verification failed at offset 132096, 512 bytes 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) +write failed: Operation not supported +write failed: Operation not supported == verifying patterns (2) == read 131072/131072 bytes at offset 1024 [...] Not run: 017 018 019 020 024 025 027 028 029 031 034 035 036 037 038 039 042 045 046 047 048 050 053 054 058 059 060 062 063 065 066 067 068 069 071 073 074 075 077 078 081 082 084 086 087 088 089 090 092 094 095 098 101 102 103 107 108 110 111 113 114 116 123 128 130 Failures: 033 Failed 1 of 19 tests -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)