[Xen-devel] [xen-unstable-smoke test] 113992: regressions - trouble: blocked/fail
flight 113992 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/113992/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 113972 build-armhf 6 xen-buildfail REGR. vs. 113972 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked n/a version targeted for testing: xen 0ccac4aa461176a056997c34dbf1ef2eeb78303e baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113972 2017-10-03 21:02:43 Z0 days Testing same since 113979 2017-10-04 00:10:13 Z0 days5 attempts People who touched revisions under test: Bhupinder Thakur Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Wei Liu jobs: build-amd64 fail build-armhf fail build-amd64-libvirt blocked test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked test-amd64-amd64-libvirt blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 424 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 19/27] x86: assembly, make some functions local
On 10/02/2017, 02:48 PM, Ard Biesheuvel wrote:
> On 2 October 2017 at 10:12, Jiri Slaby wrote:
>> There is a couple of assembly functions, which are invoked only locally
>> in the file they are defined. In C, we mark them "static". In assembly,
>> annotate them using SYM_{FUNC,CODE}_START_LOCAL (and switch their
>> ENDPROC to SYM_{FUNC,CODE}_END too). Whether FUNC or CODE depends on
>> ENDPROC/END for a particular function (C or non-C).
>>
>
> I wasn't cc'ed on the cover letter, so I am missing the rationale of
> replacing ENTRY/ENDPROC with other macros.
There was no cover letter. I am attaching what is in PATCH 1/27 instead:
Introduce new C macros for annotations of functions and data in
assembly. There is a long-standing mess in macros like ENTRY, END,
ENDPROC and similar. They are used in different manners and sometimes
incorrectly.
So introduce macros with clear use to annotate assembly as follows:
a) Support macros for the ones below
SYM_T_FUNC -- type used by assembler to mark functions
SYM_T_OBJECT -- type used by assembler to mark data
SYM_T_NONE -- type used by assembler to mark entries of unknown type
They are defined as STT_FUNC, STT_OBJECT, and STT_NOTYPE
respectively. According to the gas manual, this is the most portable
way. I am not sure about other assemblers, so we can switch this back
to %function and %object if this turns into a problem. Architectures
can also override them by something like ", @function" if they need.
SYM_A_ALIGN, SYM_A_NONE -- align the symbol?
SYM_V_GLOBAL, SYM_V_WEAK, SYM_V_LOCAL -- visibility of symbols
b) Mostly internal annotations, used by the ones below
SYM_ENTRY -- use only if you have to (for non-paired symbols)
SYM_START -- use only if you have to (for paired symbols)
SYM_END -- use only if you have to (for paired symbols)
c) Annotations for code
SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for
one function
SYM_FUNC_START_ALIAS -- use where there are two global names for one
function
SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed function
SYM_FUNC_START -- use for global functions
SYM_FUNC_START_NOALIGN -- use for global functions, w/o alignment
SYM_FUNC_START_LOCAL -- use for local functions
SYM_FUNC_START_LOCAL_NOALIGN -- use for local functions, w/o
alignment
SYM_FUNC_START_WEAK -- use for weak functions
SYM_FUNC_START_WEAK_NOALIGN -- use for weak functions, w/o alignment
SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START,
SYM_FUNC_START_WEAK, ...
SYM_FUNC_INNER_LABEL -- only for labels in the middle of functions
SYM_FUNC_INNER_LABEL_NOALIGN -- only for labels in the middle of
functions, w/o alignment
For functions with special (non-C) calling conventions:
SYM_CODE_START -- use for non-C (special) functions
SYM_CODE_START_NOALIGN -- use for non-C (special) functions, w/o
alignment
SYM_CODE_START_LOCAL -- use for local non-C (special) functions
SYM_CODE_START_LOCAL_NOALIGN -- use for local non-C (special)
functions, w/o alignment
SYM_CODE_END -- the end of SYM_CODE_START_LOCAL or SYM_CODE_START
SYM_CODE_INNER_LABEL -- only for labels in the middle of code
SYM_CODE_INNER_LABEL_NOALIGN -- only for labels in the middle of code
d) For data
SYM_DATA_START -- global data symbol
SYM_DATA_END -- the end of the SYM_DATA_START symbol
SYM_DATA_END_LABEL -- the labeled end of SYM_DATA_START symbol
SYM_DATA_SIMPLE -- start+end wrapper around simple global data
SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data
==
The macros allow to pair starts and ends of functions and mark functions
correctly in the output ELF objects.
All users of the old macros in x86 are converted to use these in further
patches.
thanks,
--
js
suse labs
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 19/27] x86: assembly, make some functions local
Hello Jiri,
On 4 October 2017 at 08:22, Jiri Slaby wrote:
> On 10/02/2017, 02:48 PM, Ard Biesheuvel wrote:
>> On 2 October 2017 at 10:12, Jiri Slaby wrote:
>>> There is a couple of assembly functions, which are invoked only locally
>>> in the file they are defined. In C, we mark them "static". In assembly,
>>> annotate them using SYM_{FUNC,CODE}_START_LOCAL (and switch their
>>> ENDPROC to SYM_{FUNC,CODE}_END too). Whether FUNC or CODE depends on
>>> ENDPROC/END for a particular function (C or non-C).
>>>
>>
>> I wasn't cc'ed on the cover letter, so I am missing the rationale of
>> replacing ENTRY/ENDPROC with other macros.
>
> There was no cover letter. I am attaching what is in PATCH 1/27 instead:
> Introduce new C macros for annotations of functions and data in
> assembly. There is a long-standing mess in macros like ENTRY, END,
> ENDPROC and similar. They are used in different manners and sometimes
> incorrectly.
>
I must say, I don't share this sentiment.
In arm64, we use ENTRY/ENDPROC for functions with external linkage,
and the bare symbol name/ENDPROC for functions with local linkage. I
guess we could add ENDOBJECT if we wanted to, but we never really felt
the need.
> So introduce macros with clear use to annotate assembly as follows:
>
> a) Support macros for the ones below
>SYM_T_FUNC -- type used by assembler to mark functions
>SYM_T_OBJECT -- type used by assembler to mark data
>SYM_T_NONE -- type used by assembler to mark entries of unknown type
>
Is is necessary to mark an entry as having no type? What is the
default type for an unmarked entry?
>They are defined as STT_FUNC, STT_OBJECT, and STT_NOTYPE
>respectively. According to the gas manual, this is the most portable
>way. I am not sure about other assemblers, so we can switch this back
>to %function and %object if this turns into a problem. Architectures
>can also override them by something like ", @function" if they need.
>
>SYM_A_ALIGN, SYM_A_NONE -- align the symbol?
>SYM_V_GLOBAL, SYM_V_WEAK, SYM_V_LOCAL -- visibility of symbols
>
Linkage != visibility
> b) Mostly internal annotations, used by the ones below
>SYM_ENTRY -- use only if you have to (for non-paired symbols)
>SYM_START -- use only if you have to (for paired symbols)
>SYM_END -- use only if you have to (for paired symbols)
>
> c) Annotations for code
>SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for
> one function
>SYM_FUNC_START_ALIAS -- use where there are two global names for one
> function
>SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed function
>
>SYM_FUNC_START -- use for global functions
>SYM_FUNC_START_NOALIGN -- use for global functions, w/o alignment
>SYM_FUNC_START_LOCAL -- use for local functions
>SYM_FUNC_START_LOCAL_NOALIGN -- use for local functions, w/o
> alignment
>SYM_FUNC_START_WEAK -- use for weak functions
>SYM_FUNC_START_WEAK_NOALIGN -- use for weak functions, w/o alignment
>SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START,
> SYM_FUNC_START_WEAK, ...
>
>SYM_FUNC_INNER_LABEL -- only for labels in the middle of functions
>SYM_FUNC_INNER_LABEL_NOALIGN -- only for labels in the middle of
> functions, w/o alignment
>
>For functions with special (non-C) calling conventions:
>SYM_CODE_START -- use for non-C (special) functions
>SYM_CODE_START_NOALIGN -- use for non-C (special) functions, w/o
> alignment
>SYM_CODE_START_LOCAL -- use for local non-C (special) functions
>SYM_CODE_START_LOCAL_NOALIGN -- use for local non-C (special)
> functions, w/o alignment
>SYM_CODE_END -- the end of SYM_CODE_START_LOCAL or SYM_CODE_START
>
>SYM_CODE_INNER_LABEL -- only for labels in the middle of code
>SYM_CODE_INNER_LABEL_NOALIGN -- only for labels in the middle of code
>
> d) For data
>SYM_DATA_START -- global data symbol
>SYM_DATA_END -- the end of the SYM_DATA_START symbol
>SYM_DATA_END_LABEL -- the labeled end of SYM_DATA_START symbol
>SYM_DATA_SIMPLE -- start+end wrapper around simple global data
>SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data
>
I am sorry but I think this is terrible. Do we really need 20+ new
macros to wrap every single assembler directive involved in defining
symbols and setting their attributes?
Is this issue you are solving widely perceived as a problem?
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 02/13] fuzz/x86_emulate: Actually use cpu_regs input
>>> On 25.09.17 at 16:26, wrote: > Commit c07574b reorganized the way fuzzing was done, explicitly > creating a structure that the input data would be copied into. > > Unfortunately, the cpu register state used by the emulator is on the > stack; it's cleared, but data is never copied into it. > > If we're explicitly setting an entirely new cpu_regs struct for each > new input anyway, there's no need to have two copies around anymore; > just point to the one in the data structure. > > Signed-off-by: George Dunlap > Reviewed-by: Wei Liu Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 04/13] fuzz/x86_emulate: Improve failure descriptions in x86_emulate harness
>>> On 25.09.17 at 16:26, wrote:
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -52,6 +52,14 @@ struct fuzz_state
> struct x86_emulate_ops ops;
> };
>
> +char *x86emul_return_string[] = {
With "static const char* const"
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 03/13] fuzz/x86_emulate: Clear errors after each iteration
>>> On 25.09.17 at 16:26, wrote: > Once feof() returns true for a stream, it will continue to return true > for that stream until clearerr() is called (or the stream is closed > and re-opened). > > In llvm-clang-fast-mode, the same file descriptor is used for each > iteration of the loop, meaning that the "Input too large" check was > broken -- feof() would return true even if the fread() hadn't hit the > end of the file. The result is that AFL generates testcases of > arbitrary size. > > Fix this by clearing the error after each iteration. > > Signed-off-by: George Dunlap Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 05/13] fuzz/x86_emulate: Implement input_read() and input_avail()
>>> On 25.09.17 at 16:26, wrote:
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -52,6 +52,22 @@ struct fuzz_state
> struct x86_emulate_ops ops;
> };
>
> +static inline bool input_available(struct fuzz_state *s, size_t size)
s can be pointer to const
Also how about shortening the function name to what the title says?
> +{
> +return s->data_index + size < s->data_num;
Shouldn't this be <= ?
> +}
> +
> +static inline bool input_read(struct fuzz_state *s, void *dst, size_t size)
> +{
> +if ( !input_available(s, size) )
> +return 0;
false
> +
> +memcpy(dst, &s->corpus->data[s->data_index], size);
> +s->data_index += size;
> +
> +return 1;
true
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 06/13] fuzz/x86_emulate: Rename the file containing the wrapper code
>>> On 25.09.17 at 16:26, wrote: > --- a/tools/fuzz/x86_instruction_emulator/Makefile > +++ b/tools/fuzz/x86_instruction_emulator/Makefile > @@ -18,22 +18,22 @@ asm: > > asm/%: asm ; > > -x86_emulate.c x86_emulate.h: %: > +x86_emulate_user.c x86_emulate_user.h: %: How about avoiding the names getting even longer? E.g. using x86-emulate.[ch] or x86emul-user.[ch] instead? > @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all > > .PHONY: distclean > distclean: clean > - rm -f x86_emulate x86_emulate.c x86_emulate.h asm > + rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm If you want to stick to the longer names, would you mind taking the opportunity to make this just x86_emulate* ? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 07/13] fuzz/x86_emulate: Add 'afl-cov' target
>>> On 25.09.17 at 16:26, wrote: > --- a/tools/fuzz/README.afl > +++ b/tools/fuzz/README.afl > @@ -41,3 +41,17 @@ Use the x86 instruction emulator fuzzer as an example. > $ $AFLPATH/afl-fuzz -t 1000 -i testcase_dir -o findings_dir -- > ./afl-harness > > Please see AFL documentation for more information. > + > +# GENERATING COVERAGE INFORMATION > + > +To use afl-cov or gcov, you need a separate binary instrumented to > +generate coverage data. To do this, use the target `afl-cov`: > + > +$ make afl-cov #produces afl-harness-cov > + > +NOTE: Please also note that the coverage instrumentation hard-codes > +the absolute path for the instrumentation read and write files in the > +binary; so coverage data will always show up in the build directory no > +matter where you run the binary from. > + > +Please see afl-cov and/or gcov documentation for more information. > \ No newline at end of file Would you please add the missing newline? > --- a/tools/fuzz/x86_instruction_emulator/Makefile > +++ b/tools/fuzz/x86_instruction_emulator/Makefile > @@ -23,19 +23,34 @@ x86_emulate_user.c x86_emulate_user.h: %: > > CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I. > > +GCOV_FLAGS=--coverage := ? > x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h > x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h) > > -x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c > $(x86_emulate.h) > +X86_EMULATE_INPUTS = x86_emulate_user.c x86_emulate/x86_emulate.c > $(x86_emulate.h) > +x86_emulate_user.o: $(X86_EMULATE_INPUTS) > + > +x86_emulate_user-cov.o: $(X86_EMULATE_INPUTS) > + $(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ x86_emulate_user.c > > fuzz-emul.o: $(x86_emulate.h) > > +fuzz-emul-cov.o: fuzz-emul.c $(x86_emulate.h) > + $(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ fuzz-emul.c > + > +afl-harness-cov.o: afl-harness.c > + $(CC) -c $(CFLAGS) $(GCOV_FLAGS) $^ -o $@ Rather than effectively repeating this command three time, I think someone else had already suggested to use a pattern rule instead. > @@ -46,7 +61,7 @@ distclean: clean > > .PHONY: clean > clean: > - rm -f *.a *.o .*.d afl-harness > + rm -f *.a *.o .*.d afl-harness afl-harness-cov *.gcda *.gcno *.gcov Perhaps simply *.gc* to cover for possible future generated file types? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 08/13] fuzz/x86_emulate: Take multiple test files for inputs
>>> On 25.09.17 at 16:26, wrote:
> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
> @@ -16,6 +16,8 @@ int main(int argc, char **argv)
> {
> size_t size;
> FILE *fp = NULL;
> +int count = 0;
> +int max;
Generally speaking these should be unsigned int, but I see how this
collides with the types of the variables max is being calculated from.
In any event both could go on a single line.
> @@ -66,11 +70,14 @@ int main(int argc, char **argv)
> __AFL_INIT();
>
> while ( __AFL_LOOP(1000) )
> +#else
> +for( count = 0; count < max; count++ )
Initially I've thought the initializer on count was pointless further
up because of the re-initialization here. Of course that's needed
because of the #if/#else this sits in. Hence I wonder whether omitting
the assignment here wouldn't be appropriate - it wouldn't really be
wromng for a compiler to warn about this redundancy.
Either way
Acked-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 09/13] fuzz/x86_emulate: Move all state into fuzz_state
>>> On 25.09.17 at 16:26, wrote:
> @@ -39,6 +33,12 @@ struct fuzz_corpus
> */
> struct fuzz_state
> {
> +unsigned long options;
> +unsigned long cr[5];
> +uint64_t msr[MSR_INDEX_MAX];
> +struct segment_register segments[SEG_NUM];
> +struct cpu_user_regs regs;
> +
> /* Fuzzer's input data. */
> struct fuzz_corpus *corpus;
>
> @@ -51,6 +51,8 @@ struct fuzz_state
> /* Emulation ops, some of which are disabled based on corpus->options. */
> struct x86_emulate_ops ops;
> };
> +#define DATA_OFFSET offsetof(struct fuzz_state, corpus)
> +
>
Personally I think this would better be placed right between the two
respective fields in the structure, for it to at the same time serve as
a clear indication that it needs either changing when a field would be
inserted there, or the insertion be done elsewhere. Also please don't
add another blank line here.
> @@ -760,12 +761,11 @@ static void disable_hooks(struct x86_emulate_ctxt *ctxt)
> static void sanitize_input(struct x86_emulate_ctxt *ctxt)
> {
> struct fuzz_state *s = ctxt->data;
> -struct fuzz_corpus *c = s->corpus;
> -struct cpu_user_regs *regs = &c->regs;
> -unsigned long bitmap = c->options;
> +struct cpu_user_regs *regs = ctxt->regs;
I think this would more obviously look like the equivalent of the old
code when being &s->regs, but the net effect is the same afaict, so it
doesn't really matter.
In any event (with at least the extra blank line removed)
Acked-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 10/13] fuzz/x86_emulate: Make input more compact
>>> On 25.09.17 at 16:26, wrote:
> @@ -22,13 +25,17 @@ int main(int argc, char **argv)
> setbuf(stdin, NULL);
> setbuf(stdout, NULL);
>
> +opt_compact = true;
How about giving the variable an initializer instead?
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -53,6 +53,15 @@ struct fuzz_state
> };
> #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>
> +bool opt_compact;
> +
> +unsigned int fuzz_minimal_input_size(void)
> +{
> +if ( opt_compact )
> +return sizeof(unsigned long) + 1;
What is this value choice based on / derived from? Oh, judging from
code further down it may be one more than the size of the options
field, in which case it should be sizeof(...->options) here.
> +else
I'd prefer if an else like this one was dropped.
> @@ -647,9 +656,81 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
> {
> struct fuzz_state *s = ctxt->data;
>
> -/* Fuzz all of the state in one go */
> -if (!input_read(s, s, DATA_OFFSET))
> -exit(-1);
> +if ( !opt_compact )
> +{
> +/* Fuzz all of the state in one go */
> +if (!input_read(s, s, DATA_OFFSET))
Missing blanks.
> +exit(-1);
> +return;
> +}
> +
> +/* Modify only select bits of state */
> +
> +/* Always read 'options' */
> +if ( !input_read(s, &s->options, sizeof(s->options)) )
> +return;
> +
> +while(1) {
Style. And for compatibility (read: no warnings) with as wide a range
of compilers as possible, generally for ( ; ; ) is better to use.
> +uint16_t offset;
> +
> +/* Read 16 bits to decide what bit of state to modify */
> +if ( !input_read(s, &offset, sizeof(offset)) )
> +return;
Doesn't this suggest minimal input size wants to be one higher than
what you currently enforce? And isn't the use of uint16_t here in
conflict with the description talking about reading a byte every time?
> +/*
> + * Then decide if it's "pointing to" different bits of the
> + * state
> + */
> +
> +/* cr[]? */
> +if ( offset < 5 )
ARRAY_SIZE()
> +{
> +if ( !input_read(s, s->cr + offset, sizeof(*s->cr)) )
> +return;
> +printf("Setting CR %d to %lx\n", offset, s->cr[offset]);
> +continue;
> +}
> +
> +offset -= 5;
Same here then.
> +/* msr[]? */
> +if ( offset < MSR_INDEX_MAX )
Even here (and below) use of ARRAY_SIZE() may be better.
> +{
> +if ( !input_read(s, s->msr + offset, sizeof(*s->msr)) )
> +return;
> +printf("Setting MSR i%d (%x) to %lx\n", offset,
> + msr_index[offset], s->msr[offset]);
> +continue;
> +}
> +
> +offset -= MSR_INDEX_MAX;
> +
> +/* segments[]? */
> +if ( offset < SEG_NUM )
> +{
> +if ( !input_read(s, s->segments + offset, sizeof(*s->segments)) )
> +return;
> +printf("Setting Segment %d\n", offset);
> +continue;
> +
> +}
> +
> +offset -= SEG_NUM;
> +
> +/* regs? */
> +if ( offset < sizeof(struct cpu_user_regs)
> + && offset + sizeof(uint64_t) <= sizeof(struct cpu_user_regs) )
> +{
> +if ( !input_read(s, ((char *)ctxt->regs) + offset,
> sizeof(uint64_t)) )
> +return;
> +printf("Setting cpu_user_regs offset %x\n", offset);
> +continue;
> +}
> +
> +/* None of the above -- take that as "start emulating" */
> +
> +return;
> +}
Having come here I wonder whether the use of "byte" in the description
is right, and you mean "uint8_t offset" above, as you're far from
consuming the entire 256 value range.
Additionally, was the order of elements here chosen for any specific
reason? It would seem to me that elements having a more significant
effect on emulation may be worth filling first, and I'm not convinced
the "all CRs, all MSRs, all SREGs, all GPRs" order matches that.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 11/13] fuzz/x86_emulate: Add --rerun option to try to track down instability
>>> On 25.09.17 at 16:26, wrote:
> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
> @@ -14,6 +14,7 @@ extern unsigned int fuzz_minimal_input_size(void);
> static uint8_t input[INPUT_SIZE];
>
> extern bool opt_compact;
> +extern bool opt_rerun;
Seeing a second such variable appear, I think it would really be better
to introduce a local header, included by both producer and consumer.
> @@ -886,21 +896,138 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
> return 0;
> }
>
> -int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
> +void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p,
> size_t size)
static (also for other new helper functions)?
> {
> -struct fuzz_state state = {
> -.ops = all_fuzzer_ops,
> -};
> -struct x86_emulate_ctxt ctxt = {
> -.data = &state,
> -.regs = &state.regs,
> -.addr_size = 8 * sizeof(void *),
> -.sp_size = 8 * sizeof(void *),
> -};
> +memset(state, 0, sizeof(*state));
> +state->corpus = (struct fuzz_corpus *)data_p;
Please don't cast away constness here. Perhaps best to make the parameter
const void *, allowing for the cast to be dropped altogether.
> +int runtest(struct fuzz_state *state) {
> int rc;
>
> -/* Reset all global state variables */
> -memset(&input, 0, sizeof(input));
> +struct x86_emulate_ctxt *ctxt = &state->ctxt;
> +
> +state->ops = all_fuzzer_ops;
> +
> +ctxt->data = state;
> +ctxt->regs = &state->regs;
> +ctxt->addr_size = ctxt->sp_size = 8 * sizeof(void *);
Is this actually necessary? I don't see a way for set_sizes() to be
bypassed.
> +void compare_states(struct fuzz_state state[2])
> +{
> +// First zero any "internal" pointers
> +state[0].corpus = state[1].corpus = NULL;
> +state[0].ctxt.data = state[1].ctxt.data = NULL;
> +state[0].ctxt.regs = state[1].ctxt.regs = NULL;
> +
> +
No double blank lines please.
> +if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
> +{
> +int i;
unsigned int (and then %u in the format strings below)
> +printf("State mismatch\n");
> +
> +for ( i=0; i<5; i++)
Blanks missing and please use ARRAY_SIZE() again (also further down).
> +if ( state[0].cr[i] != state[1].cr[i] )
> +printf("cr[%d]: %lx != %lx\n",
> + i, state[0].cr[i], state[1].cr[i]);
> +
> +for ( i=0; i +if ( state[0].msr[i] != state[1].msr[i] )
> +printf("msr[%d]: %lx != %lx\n",
> + i, state[0].msr[i], state[1].msr[i]);
> +
> +for ( i=0; i +if ( memcmp(&state[0].segments[i], &state[1].segments[i],
> +sizeof(state[0].segments[0])) )
> +printf("segments[%d] differ!\n", i);
The actual values would likely be helpful to be printed here, just like
you do for all other state elements.
> +if ( state[0].data_num != state[1].data_num )
> +printf("data_num: %lx != %lx\n", state[0].data_num,
> + state[1].data_num);
> +if ( state[0].data_index != state[1].data_index )
> +printf("data_index: %lx != %lx\n", state[0].data_index,
> + state[1].data_index);
> +
> +if ( memcmp(&state[0].regs, &state[1].regs, sizeof(state[0].regs)) )
> +{
> +printf("registers differ!\n");
> +/* Print If Not Equal */
> +#define PINE(elem)\
> +if ( state[0].elem != state[1].elem ) \
> +printf(#elem " differ: %lx != %lx\n", \
> + (unsigned long)state[0].elem, \
> + (unsigned long)state[1].elem)
> +PINE(regs.r15);
> +PINE(regs.r14);
> +PINE(regs.r13);
> +PINE(regs.r12);
> +PINE(regs.rbp);
> +PINE(regs.rbx);
> +PINE(regs.r10);
> +PINE(regs.r11);
> +PINE(regs.r9);
> +PINE(regs.r8);
> +PINE(regs.rax);
> +PINE(regs.rcx);
> +PINE(regs.rdx);
> +PINE(regs.rsi);
> +PINE(regs.rdi);
> +
> +for ( i = offsetof(struct cpu_user_regs, error_code) /
> sizeof(unsigned);
> + i < sizeof(state[1].regs)/sizeof(unsigned); i++ )
> +{
> +printf("[%04lu] %08x %08x\n",
I think this wants to be %04zu (or perhaps better %4zu or %04zx). Same
for ctxt printing further down.
> @@ -908,7 +1035,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t
> size)
> return 1;
> }
>
> -if ( size > sizeof(input) )
> +if ( size > sizeof(struct fuzz_corpus) )
What's the difference between the two variants?
> @@ -916,25 +1043,24 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p,
> size_t size)
>
> memcpy(&input, data_p, si
Re: [Xen-devel] [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state
>>> On 25.09.17 at 16:26, wrote:
> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
> };
> #undef SET
>
> +static void _set_fpu_state(char *fxsave, bool store)
> +{
> +if ( cpu_has_fxsr )
> +{
> +static union __attribute__((__aligned__(16))) {
> +char x[464];
The final part of the save area isn't being written, yes, but is it
really worth saving the few bytes of stack space here, rather than
having the expected 512 as array dimension?
> +struct {
> +uint32_t other[6];
> +uint32_t mxcsr;
> +uint32_t mxcsr_mask;
> +/* ... */
> +};
> +} *fxs;
> +
> +fxs = (typeof(fxs)) fxsave;
> +
> +if ( store ) {
Style.
> +char null[512] __attribute__((aligned(16))) = { 0 };
No need for the 0 and a blank line between declaration and statements
please.
> +asm volatile(" fxrstor %0; "::"m"(*null));
> +asm volatile(" fxrstor %0; "::"m"(*fxsave));
Style again - these want to follow the
asm volatile ( "..." :: "m" (...) )
form. No need for the ; following the instructions.
> +}
> +
> +asm volatile( "fxsave %0" : "=m" (*fxs) );
This is pretty confusing, the more with the different variable names
used which point to the same piece of memory. You basically store back
into the area you've read from. Is the caller expecting the memory area
to change? Is this being done other than for convenience to not have
another instance of scratch space on the stack? Some comment on the
intentions may be helpful here.
The function's parameter name being "store" adds to the confusion,
since what it controls is actually what we call "load" on x86 (or
"restore" following the insn mnemonics).
And then - what about YMM register state? Other more exotic registers
(like the BND* ones) may indeed not be that relevant to fuzz yet.
> @@ -737,6 +780,17 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
> printf("Setting cpu_user_regs offset %x\n", offset);
> continue;
> }
> +offset -= sizeof(struct cpu_user_regs);
> +
> +/* Fuzz fxsave state */
> +if ( offset < 128 )
> +{
> +if ( !input_read(s, s->fxsave + (offset * 4), 4) )
> +return;
> +printf("Setting fxsave offset %x\n", offset * 4);
What's this 32-bit granularity derived from?
> @@ -883,6 +937,9 @@ static void sanitize_state(struct x86_emulate_ctxt *ctxt)
> s->segments[x86_seg_cs].db = 0;
> s->segments[x86_seg_ss].db = 0;
> }
> +
> +/* Setting this value seems to cause crashes in fxrstor */
> +*((unsigned int *)(s->fxsave) + 6) = 0;
That's the MXCSR field - instead of storing zero you want to mask with
mxcsr_mask. To avoid the ugly literal 6 (and to make clear what it is
that needs adjustment here) it may also be worthwhile widening the
scope of the type declared in _set_fpu_state() and use it for struct
fuzz_state's fxsave field.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 13/13] fuzz/x86_emulate: Add an option to limit the number of instructions executed
>>> On 25.09.17 at 16:26, wrote:
> AFL considers a testcase to be a useful addition not only if there are
> tuples exercised by that testcase which were not exercised otherwise,
> but also if the *number* of times an individual tuple is exercised
> changes significantly; in particular, if the number of the highes bit
> changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c).
Perhaps I simply don't know about AFL (yet) to understand how "highest
bit" matters here, or even whose highest bits there's talk of.
> Unfortunately, one simple way to increase these stats it to execute
> the same (or similar) instructions multiple times.
But the change here doesn't look at instruction similarity at all.
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -960,10 +960,13 @@ void setup_fuzz_state(struct fuzz_state *state, const
> uint8_t *data_p, size_t si
> state->data_num = size;
> }
>
> +int opt_instruction_limit = 0;
unsigned int (and formally no need for an initializer)
> int runtest(struct fuzz_state *state) {
> int rc;
>
> struct x86_emulate_ctxt *ctxt = &state->ctxt;
> +int icount = 0;
unsigned int
> @@ -988,7 +991,9 @@ int runtest(struct fuzz_state *state) {
>
> rc = x86_emulate(ctxt, &state->ops);
> printf("Emulation result: %d\n", rc);
> -} while ( rc == X86EMUL_OKAY );
> +} while ( rc == X86EMUL_OKAY &&
> + (!opt_instruction_limit ||
> + (++icount < opt_instruction_limit)) );
Hmm, if the initalizer of opt_instruction_limit was UINT_MAX, I think
this wouldn't severely impact results (running 4 billion emulations is
simply going to take too long) and this expression could be a simple
comparison.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space
>>> On 19.09.17 at 17:29, wrote:
> +static int vpci_portio_read(const struct hvm_io_handler *handler,
> +uint64_t addr, uint32_t size, uint64_t *data)
> +{
> +struct domain *d = current->domain;
> +unsigned int reg;
> +pci_sbdf_t sbdf;
> +uint32_t cf8;
> +
> +*data = ~(uint64_t)0;
> +
> +if ( addr == 0xcf8 )
> +{
> +ASSERT(size == 4);
> +*data = d->arch.hvm_domain.pci_cf8;
> +return X86EMUL_OKAY;
> +}
> +
> +cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
> +if ( !CF8_ENABLED(cf8) )
> +return X86EMUL_OKAY;
Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
forwarded to qemu if it's not a config space one. Same in the write path
then.
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -124,6 +124,12 @@ SECTIONS
> __param_start = .;
> *(.data.param)
> __param_end = .;
> +
> +#if defined(CONFIG_HAS_PCI) && defined(CONFIG_LATE_HWDOM)
> + __start_vpci_array = .;
> + *(.data.vpci)
> + __end_vpci_array = .;
> +#endif
>} :text
>
> #if defined(BUILD_ID)
> @@ -213,6 +219,12 @@ SECTIONS
> *(.init_array)
> *(SORT(.init_array.*))
> __ctors_end = .;
> +
> +#if defined(CONFIG_HAS_PCI) && !defined(CONFIG_LATE_HWDOM)
> + __start_vpci_array = .;
> + *(.data.vpci)
> + __end_vpci_array = .;
> +#endif
>} :text
Suitable alignment needs to be enforced in both cases, or else we risk
someone adding something immediately ahead of one of your insertions,
making __start_vpci_array no longer point to the first entry.
> @@ -1052,9 +1053,10 @@ static void __hwdom_init setup_one_hwdom_device(const
> struct setup_hwdom *ctxt,
> struct pci_dev *pdev)
> {
> u8 devfn = pdev->devfn;
> +int err;
>
> do {
> -int err = ctxt->handler(devfn, pdev);
> +err = ctxt->handler(devfn, pdev);
>
> if ( err )
Please also remove the now stray blank line.
> +int vpci_add_register(const struct pci_dev *pdev, vpci_read_t *read_handler,
> + vpci_write_t *write_handler, unsigned int offset,
> + unsigned int size, void *data)
> +{
> +struct list_head *prev;
> +struct vpci_register *r;
> +
> +/* Some sanity checks. */
> +if ( (size != 1 && size != 2 && size != 4) ||
> + offset >= PCI_CFG_SPACE_EXP_SIZE || (offset & (size - 1)) ||
> + (!read_handler && !write_handler) )
> +return -EINVAL;
> +
> +r = xmalloc(struct vpci_register);
> +if ( !r )
> +return -ENOMEM;
> +
> +r->read = read_handler ?: vpci_ignored_read;
> +r->write = write_handler ?: vpci_ignored_write;
> +r->size = size;
> +r->offset = offset;
> +r->private = data;
> +
> +spin_lock(&pdev->vpci->lock);
> +
> +/* The list of handlers must be kept sorted at all times. */
> +list_for_each ( prev, &pdev->vpci->handlers )
> +{
> +const struct vpci_register *this =
> +list_entry(prev, const struct vpci_register, node);
> +int cmp = vpci_register_cmp(r, this);
> +
> +if ( cmp < 0 )
> +break;
> +if ( cmp == 0 )
> +{
> +spin_unlock(&pdev->vpci->lock);
> +xfree(r);
> +return -EEXIST;
> +}
> +}
> +
> +list_add_tail(&r->node, prev);
> +spin_unlock(&pdev->vpci->lock);
> +
> +return 0;
> +}
Looking at this and its remove counterpart it is not (no longer?) clear
why they both take a struct pci_dev * as parameter - struct vpci * would
fully suffice, and would eliminate the question on whether functions
like these should have the respective parameters const-qualified.
> +/*
> + * Merge new data into a partial result.
> + *
> + * Copy the value found in 'new' from [0, size) left shifted by
> + * 'offset' into 'data'.
> + */
> +static uint32_t merge_result(uint32_t data, uint32_t new, unsigned int size,
> + unsigned int offset)
> +{
> +uint32_t mask = 0x >> (32 - 8 * size);
> +
> +return (data & ~(mask << (offset * 8))) | ((new & mask) << (offset * 8));
> +}
If a function like this one has a relatively long comment, I think that
comment should clarify that both size and offset are byte-granular.
Especially for offset (used for shifting) bit otherwise would seem more
natural to me.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 03/11] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas
>>> On 19.09.17 at 17:29, wrote:
> +static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
> + unsigned int len, unsigned long *data)
> +{
> +struct domain *d = v->domain;
> +const struct hvm_mmcfg *mmcfg;
> +unsigned int reg;
> +pci_sbdf_t sbdf;
> +
> +*data = ~0ul;
> +
> +read_lock(&d->arch.hvm_domain.mmcfg_lock);
> +mmcfg = vpci_mmcfg_find(d, addr);
> +if ( !mmcfg )
> +{
> +read_unlock(&d->arch.hvm_domain.mmcfg_lock);
> +return X86EMUL_OKAY;
> +}
With the lock dropped between accept() and read() (or write() below),
is it really appropriate to return OKAY here? The access again should
be forwarded to qemu, I would think.
> +int __hwdom_init register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
> + unsigned int start_bus,
> + unsigned int end_bus,
> + unsigned int seg)
> +{
> +struct hvm_mmcfg *mmcfg;
> +
> +ASSERT(is_hardware_domain(d));
> +
> +write_lock(&d->arch.hvm_domain.mmcfg_lock);
> +if ( vpci_mmcfg_find(d, addr) )
> +{
> +write_unlock(&d->arch.hvm_domain.mmcfg_lock);
> +return -EEXIST;
> +}
You check here for an exact match in starting address. Is it really
to reject just this special case, rather than doing a proper overlap
check?
> +mmcfg = xmalloc(struct hvm_mmcfg);
Whenever possible without too much trouble allocations should be done
with no lock held.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 04/11] x86/physdev: enable PHYSDEVOP_pci_mmcfg_reserved for PVH Dom0
>>> On 19.09.17 at 17:29, wrote:
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -559,6 +559,17 @@ ret_t do_physdev_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>
> ret = pci_mmcfg_reserved(info.address, info.segment,
> info.start_bus, info.end_bus, info.flags);
> +if ( !ret && has_vpci(currd) )
> +{
> +/*
> + * For HVM (PVH) domains try to add the newly found MMCFG to the
> + * domain.
> + */
> +ret = register_vpci_mmcfg_handler(currd, info.address,
> + info.start_bus, info.end_bus,
> + info.segment);
> +}
> +
> break;
> }
I think it is wrong to report back -EEXIST here for an exact match
region which we have on record already.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 05/11] pci: split code to size BARs from pci_add_device
>>> On 19.09.17 at 17:29, wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -603,6 +603,56 @@ static int iommu_add_device(struct pci_dev *pdev);
> static int iommu_enable_device(struct pci_dev *pdev);
> static int iommu_remove_device(struct pci_dev *pdev);
>
> +int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos, bool last,
> + uint64_t *paddr, uint64_t *psize, unsigned int flags)
> +{
> +uint32_t hi = 0, bar = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
> + sbdf.func, pos);
> +uint64_t addr, size;
> +bool vf = flags & PCI_BAR_VF;
Honestly I'm not convinced of the utility of this variable; same for the
"rom" one in the next patch.
> +ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
> +pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, ~0);
> +if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +{
> +if ( last )
> +{
> +printk(XENLOG_WARNING
> + "%sdevice %04x:%02x:%02x.%u with 64-bit %sBAR in last
> slot\n",
> + vf ? "SR-IOV " : "", sbdf.seg, sbdf.bus, sbdf.dev,
> sbdf.func,
> + vf ? "vf " : "");
> +return -EINVAL;
> +}
> +hi = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos +
> 4);
> +pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4,
> ~0);
> +}
> +size = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos) &
> + PCI_BASE_ADDRESS_MEM_MASK;
> +if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +{
> +size |= (uint64_t)pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
> + sbdf.func, pos + 4) << 32;
> +pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4,
> hi);
> +}
> +else if ( size )
> +size |= (uint64_t)~0 << 32;
> +pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, bar);
> +size = -size;
> +addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
> +
> +if ( paddr )
> +*paddr = addr;
You need addr only inside the if() - no need for the local variable,
and no need to calculate it unconditionally.
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -189,6 +189,10 @@ const char *parse_pci(const char *, unsigned int *seg,
> unsigned int *bus,
> const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
>unsigned int *dev, unsigned int *func, bool
> *def_seg);
>
> +#define _PCI_BAR_VF 0
> +#define PCI_BAR_VF (1u << _PCI_BAR_VF)
Do you really need both? I know we have quite a few cases where flags
are being defined this way, but that's usually when bit operations
(test_bit() and alike) are intended on the flags fields.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 07/11] xen: introduce rangeset_consume_ranges
>>> On 19.09.17 at 17:29, wrote:
> This function allows to iterate over a rangeset while removing the
> processed regions.
>
> It will be used by the following patches in order to store memory
> regions in rangesets, and remove them while iterating.
This really only repeats what the first paragraph already says. Instead
you want to state why this is actually needed (to be able to split
processing aiui).
> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -298,6 +298,34 @@ int rangeset_report_ranges(
> return rc;
> }
>
> +int rangeset_consume_ranges(
> +struct rangeset *r,
> +int (*cb)(unsigned long s, unsigned long e, void *, unsigned long *c),
> +void *ctxt)
> +{
> +int rc = 0;
> +
> +write_lock(&r->lock);
> +while ( !rangeset_is_empty(r) )
> +{
> +unsigned long consumed = 0;
> +struct range *x = first_range(r);
> +
> +rc = cb(x->s, x->e, ctxt, &consumed);
> +
> +ASSERT(consumed <= x->e - x->s + 1);
> +x->s += consumed;
> +if ( x->s > x->e )
> +destroy_range(r, x);
> +
> +if ( rc )
> +break;
> +}
> +write_unlock(&r->lock);
> +
> +return rc;
> +}
Leaving the rangeset populated in case of error (other than -ERESTART)
looks to be potentially problematic/unexpected. Please at least add a
comment in the header stating this. Perhaps negative vs positive rc
from the callback could be used to direct intended behavior.
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -67,6 +67,10 @@ bool_t __must_check rangeset_overlaps_range(
> int rangeset_report_ranges(
> struct rangeset *r, unsigned long s, unsigned long e,
> int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
> +int rangeset_consume_ranges(
> +struct rangeset *r,
> +int (*cb)(unsigned long s, unsigned long e, void *, unsigned long *c),
> +void *ctxt);
Indentation.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 06/11] pci: add support to size ROM BARs to pci_size_mem_bar
>>> On 19.09.17 at 17:29, wrote: > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -610,11 +610,17 @@ int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos, > bool last, > sbdf.func, pos); > uint64_t addr, size; > bool vf = flags & PCI_BAR_VF; > - > -ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY); > +bool rom = flags & PCI_BAR_ROM; Ideally with this local variable and ... > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -191,6 +191,8 @@ const char *parse_pci_seg(const char *, unsigned int > *seg, unsigned int *bus, > > #define _PCI_BAR_VF 0 > #define PCI_BAR_VF (1u << _PCI_BAR_VF) > +#define _PCI_BAR_ROM1 > +#define PCI_BAR_ROM (1u << _PCI_BAR_ROM) ... the first of these two dropped Reviewed-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 08/11] vpci/bars: add handlers to map the BARs
>>> On 19.09.17 at 17:29, wrote:
> @@ -48,6 +49,9 @@ bool_t hvm_io_pending(struct vcpu *v)
> struct domain *d = v->domain;
> struct hvm_ioreq_server *s;
>
> +if ( has_vpci(v->domain) && vpci_check_pending(v) )
has_vpci(d)
> + return 1;
Indentation.
> +static int vpci_map_range(unsigned long s, unsigned long e, void *data,
> + unsigned long *c)
> +{
> +const struct map_data *map = data;
> +int rc;
> +
> +for ( ; ; )
> +{
> +unsigned long size = e - s + 1;
> +
> +rc = (map->map ? map_mmio_regions : unmap_mmio_regions)
> + (map->d, _gfn(s), size, _mfn(s));
> +if ( rc == 0 )
> +{
> +*c += size;
> +break;
> +}
> +if ( rc < 0 )
> +{
> +printk(XENLOG_G_WARNING
> + "Failed to identity %smap [%" PRI_gfn ", %" PRI_gfn ")
> for d%d: %d\n",
> + map ? "" : "un", s, e, map->d->domain_id, rc);
> +break;
> +}
ASSERT(rc < size) ?
> +bool vpci_check_pending(struct vcpu *v)
"check" in the function name generally suggests (to me at least) that
the parameter ought to be const. Perhaps vpci_process_pending()?
> +{
> +if ( v->vpci.mem )
> +{
> +int rc = vpci_map_memory(v->domain, v->vpci.mem, v->vpci.map);
> +
> +if ( rc == -ERESTART )
> +return true;
There's no real need for the local variable if all other return values
are simply discarded here. However, ...
> +rangeset_destroy(v->vpci.mem);
> +v->vpci.mem = NULL;
> +}
> +
> +return false;
> +}
... I'm not convinced this is a good error handling model. I don't
recall how previous versions dealt with this, but iirc we agreed to
generally make all such Dom0 handling best effort (here: don't skip the
remaining ranges if mapping of one failed). An exception may want/need
to be -ENOMEM.
> +static int vpci_check_bar_overlap(const struct pci_dev *pdev,
> + const struct vpci_bar *rom,
> + struct rangeset *mem)
> +{
> +const struct pci_dev *cmp;
> +
> +/* Check for overlaps with other device's BARs. */
> +list_for_each_entry(cmp, &pdev->domain->arch.pdev_list, domain_list)
> +{
> +unsigned int i;
> +
> +if ( rom == NULL && pdev == cmp )
> +continue;
This check looks rather unmotivated (or even bogus) without a comment.
The other special casing of ROM BARs further down also isn't all that
obvious (and right now I can't even convince myself it's correct).
> +static void vpci_modify_bars(const struct pci_dev *pdev, bool map)
> +{
> +struct vpci_header *header = &pdev->vpci->header;
> +struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> +unsigned int i;
> +int rc;
> +
> +if ( !mem )
> +return;
> +
> +/*
> + * Create a rangeset that represents the current device BARs memory
> region
> + * and compare it against all the currently active BAR memory regions. If
> + * an overlap is found, subtract it from the region to be
> + * mapped/unmapped.
> + *
> + * NB: the rangeset uses inclusive frame numbers.
> + */
> +
> +/* First fill the rangeset with all the BARs of this device. */
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +{
> +const struct vpci_bar *bar = &header->bars[i];
> +
> +if ( !MAPPABLE_BAR(bar) ||
> + (bar->type == VPCI_BAR_ROM && !bar->rom_enabled) )
> +continue;
> +
> +rc = rangeset_add_range(mem, PFN_DOWN(bar->addr),
> +PFN_DOWN(bar->addr + bar->size - 1));
> +if ( rc )
> +{
> +rangeset_destroy(mem);
> +return;
I'm afraid -ENOMEM here (which sadly is possible, as we don't maintain
any reserves) would produce a very hard to diagnose misbehavior. I think
you want to log a message here.
> +}
> +}
> +
> +/* Check for overlaps with other device's BARs. */
> +rc = vpci_check_bar_overlap(pdev, NULL, mem);
Why is this not symmetrical with vpci_modify_rom() (which also checks
overlaps inside the current device)?
> +if ( rc )
> +{
> +rangeset_destroy(mem);
> +return;
Same error handling comment as above, despite failure here being less
likely (hopefully at least). Perhaps worth joining the two paths.
> +}
> +
> +rc = vpci_maybe_defer_map(pdev->domain, mem, map);
> +if ( !rc )
> +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +if ( header->bars[i].type != VPCI_BAR_ROM ||
> + header->bars[i].rom_enabled )
> +header->bars[i].enabled = map;
Hmm, you're updating state here regardless of possible failure in the
deferred operation (see the discarded error code in
vpci_check_pending()).
> +static uint32_t vpci_cmd_read(const struct pci_dev *pdev, unsigned int reg,
> +
Re: [Xen-devel] [PATCH v6 09/11] vpci/msi: add MSI handlers
>>> On 19.09.17 at 17:29, wrote:
> Add handlers for the MSI control, address, data and mask fields in
> order to detect accesses to them and setup the interrupts as requested
> by the guest.
>
> Note that the pending register is not trapped, and the guest can
> freely read/write to it.
>
> Signed-off-by: Roger Pau Monné
> Reviewed-by: Paul Durrant
I wonder how valid this can be with ...
> Changes since v5:
> - Update to new lock usage.
> - Change handlers to match the new type.
> - s/msi_flags/msi_gflags/, remove the local variables and use the new
>DOMCTL_VMSI_* defines.
> - Change the MSI arch function to take a vpci_msi instead of a
>vpci_arch_msi as parameter.
> - Fix the calculation of the guest vector for MSI injection to take
>into account the number of bits that can be modified.
> - Use INVALID_PIRQ everywhere.
> - Simplify exit path of vpci_msi_disable.
> - Remove the conditional when setting address64 and masking fields.
> - Add a process_pending_softirqs to the MSI dump loop.
> - Place the prototypes for the MSI arch-specific functions in
>xen/vpci.h.
> - Add parentheses around the INVALID_PIRQ definition.
... this set of changes.
> +void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev,
> +unsigned int entry, bool mask)
> +{
> +const struct pirq *pinfo;
> +struct irq_desc *desc;
> +unsigned long flags;
> +int irq;
> +
> +ASSERT(msi->arch.pirq >= 0 && entry < msi->vectors);
> +pinfo = pirq_info(pdev->domain, msi->arch.pirq + entry);
> +if ( !pinfo )
> +return;
> +
> +irq = pinfo->arch.irq;
> +if ( irq >= nr_irqs || irq < 0)
Style. However, ...
> +return;
> +
> +desc = irq_to_desc(irq);
> +if ( !desc )
> +return;
> +
> +spin_lock_irqsave(&desc->lock, flags);
... didn't I comment on this already suggesting to use
domain_spin_lock_irq_desc() instead of open coding it?
> +static void vpci_msi_enable(const struct pci_dev *pdev, struct vpci_msi *msi,
> +unsigned int vectors)
> +{
> +int ret;
> +
> +ASSERT(!msi->enabled);
> +ret = vpci_msi_arch_enable(msi, pdev, vectors);
> +if ( ret )
> +return;
> +
> +/* Apply the mask bits. */
> +if ( msi->masking )
> +{
> +unsigned int i;
> +uint32_t mask = msi->mask;
> +
> +for ( i = ffs(mask) - 1; mask && i < vectors; i = ffs(mask) - 1 )
> +{
> +vpci_msi_arch_mask(msi, pdev, i, true);
> +__clear_bit(i, &mask);
> +}
> +}
> +
> +__msi_set_enable(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), msi->pos, 1);
This is very unlikely to be a function that arch-independent code is
permitted to call.
> +void vpci_dump_msi(void)
> +{
> +struct domain *d;
const?
> +for_each_domain ( d )
You need to rcu_read_lock(&domlist_read_lock) in order to validly use
this construct.
> +{
> +const struct pci_dev *pdev;
> +
> +if ( !has_vpci(d) )
> +continue;
> +
> +printk("vPCI MSI information for d%d\n", d->domain_id);
> +
> +list_for_each_entry ( pdev, &d->arch.pdev_list, domain_list )
> +{
> +uint8_t seg = pdev->seg, bus = pdev->bus;
> +uint8_t slot = PCI_SLOT(pdev->devfn), func =
> PCI_FUNC(pdev->devfn);
> +const struct vpci_msi *msi = pdev->vpci->msi;
> +
> +if ( !spin_trylock(&pdev->vpci->lock) )
> +{
> +printk("Unable to get vPCI lock, skipping\n");
> +continue;
> +}
> +
> +if ( msi )
> +{
> +printk("Device %04x:%02x:%02x.%u\n", seg, bus, slot, func);
> +
> +printk(" Enabled: %u Supports masking: %u 64-bit addresses:
> %u\n",
> + msi->enabled, msi->masking, msi->address64);
bool wants to be printed with %d, I think.
> +printk(" Max vectors: %u enabled vectors: %u\n",
> + msi->max_vectors, msi->vectors);
> +
> +vpci_msi_arch_print(msi);
> +
> +if ( msi->masking )
> +printk(" mask=%08x\n", msi->mask);
Is this really worth a separate line? Also please don't separately print
->making as its value will be known from the presence of the field here.
Overall please try to shorten messages such that they're still
meaningful but don't cause unnecesary load on the serial line or extra
wasted space in the ring buffer.
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -72,6 +72,30 @@ struct vpci {
> } bars[7]; /* At most 6 BARS + 1 expansion ROM BAR. */
> /* FIXME: currently there's no support for SR-IOV. */
> } header;
> +
> +/* MSI data. */
> +struct vpci_msi {
> +/* Arch-specific data. */
> +struct vpci_arch_msi arch;
> +/* Address. */
> +
Re: [Xen-devel] [PATCH v6 11/11] vpci/msix: add MSI-X handlers
>>> On 19.09.17 at 17:29, wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -152,6 +152,7 @@ static int vpci_check_bar_overlap(const struct pci_dev
> *pdev,
> static void vpci_modify_bars(const struct pci_dev *pdev, bool map)
> {
> struct vpci_header *header = &pdev->vpci->header;
> +struct vpci_msix *msix = pdev->vpci->msix;
const and please fetch the value only right before you first need it.
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -320,13 +320,17 @@ void vpci_dump_msi(void)
> if ( !has_vpci(d) )
> continue;
>
> -printk("vPCI MSI information for d%d\n", d->domain_id);
> +printk("vPCI MSI/MSI-X information for d%d\n", d->domain_id);
>
> list_for_each_entry ( pdev, &d->arch.pdev_list, domain_list )
> {
> uint8_t seg = pdev->seg, bus = pdev->bus;
> uint8_t slot = PCI_SLOT(pdev->devfn), func =
> PCI_FUNC(pdev->devfn);
> const struct vpci_msi *msi = pdev->vpci->msi;
> +const struct vpci_msix *msix = pdev->vpci->msix;
> +
> +if ( msi || msix )
> +printk("Device %04x:%02x:%02x.%u\n", seg, bus, slot, func);
>
> if ( !spin_trylock(&pdev->vpci->lock) )
> {
> @@ -336,7 +340,7 @@ void vpci_dump_msi(void)
>
> if ( msi )
> {
> -printk("Device %04x:%02x:%02x.%u\n", seg, bus, slot, func);
> +printk(" MSI\n");
>
> printk(" Enabled: %u Supports masking: %u 64-bit addresses:
> %u\n",
> msi->enabled, msi->masking, msi->address64);
> @@ -349,6 +353,20 @@ void vpci_dump_msi(void)
> printk(" mask=%08x\n", msi->mask);
> }
>
> +if ( msix )
> +{
> +unsigned int i;
> +
> +printk(" MSI-X\n");
> +
> +printk(" Max entries: %u maskall: %u enabled: %u\n",
> + msix->max_entries, msix->masked, msix->enabled);
> +
> +printk(" Table entries:\n");
> +for ( i = 0; i < msix->max_entries; i++ )
> +vpci_msix_arch_print_entry(&msix->entries[i]);
> +}
> +
Again, please try to reduce the amount of overall output.
> +static void vpci_msix_control_write(const struct pci_dev *pdev,
> +unsigned int reg, uint32_t val, void
> *data)
> +{
> +uint8_t seg = pdev->seg, bus = pdev->bus;
> +uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +struct vpci_msix *msix = data;
> +bool new_masked, new_enabled;
> +unsigned int i;
> +int rc;
> +
> +new_masked = val & PCI_MSIX_FLAGS_MASKALL;
> +new_enabled = val & PCI_MSIX_FLAGS_ENABLE;
> +
> +/*
> + * According to the PCI 3.0 specification, switching the enable bit
> + * to 1 or the function mask bit to 0 should cause all the cached
> + * addresses and data fields to be recalculated. Xen implements this
> + * as disabling and enabling the entries.
> + *
> + * Note that the disable/enable sequence is only performed when the
> + * guest has written to the entry (ie: updated field set) or MSIX is
> + * enabled.
> + */
> +if ( new_enabled && !new_masked && (!msix->enabled || msix->masked) )
> +{
> +paddr_t table_base =
> +pdev->vpci->header.bars[msix->mem[VPCI_MSIX_TABLE].bir].addr;
> +
> +for ( i = 0; i < msix->max_entries; i++ )
> +{
> +if ( msix->entries[i].masked ||
> + (new_enabled && msix->enabled && !msix->entries[i].updated)
> )
> +continue;
This doesn't look to match up with the earlier comment.
> +static int vpci_msix_read(struct vcpu *v, unsigned long addr,
> + unsigned int len, unsigned long *data)
> +{
> +struct domain *d = v->domain;
const?
> +const struct vpci_bar *bars;
> +struct vpci_msix *msix;
> +const struct vpci_msix_entry *entry;
> +unsigned int offset;
> +
> +*data = ~0ul;
> +
> +msix = vpci_msix_find(d, addr);
> +if ( !msix || !vpci_msix_access_allowed(msix->pdev, addr, len) )
> +return X86EMUL_OKAY;
In the !msix case I'm once again not convinced returning OKAY is correct
here.
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -100,6 +100,40 @@ struct vpci {
> /* 64-bit address capable? */
> bool address64;
> } *msi;
> +
> +/* MSI-X data. */
> +struct vpci_msix {
> +struct pci_dev *pdev;
> +/* List link. */
> +struct list_head next;
> +/* Table information. */
> +struct vpci_msix_mem {
> +/* MSI-X table offset. */
> +unsigned int offset;
> +/* MSI-X table BIR. */
> +unsigned int bir;
> +/* Table size. */
> +unsigned int size
Re: [Xen-devel] [PATCH] x86/pvh: fix memory accounting for Dom0
>>> On 28.09.17 at 17:55, wrote: > On Thu, Sep 28, 2017 at 03:39:08PM +, Roger Pau Monné wrote: >> On Thu, Sep 28, 2017 at 01:18:55PM +, Jan Beulich wrote: >> > >>> On 28.09.17 at 12:16, wrote: >> > > Make sure that the memory for the paging structures in case of a HVM >> > > Dom0 is subtracted from the total amount of memory available for Dom0 >> > > to use. Also take into account whether the IOMMU is sharing the >> > > page tables with HAP, or else also reserve some memory for the IOMMU >> > > page tables. >> > > >> > > While there re-organize the code slightly so that the for loop and the >> > > need_paging local variable can be removed. >> > >> > These two things very definitely should not be merged into a single >> > patch; I'm not convinced the reorg is correct in the first place. Note >> > how avail, which is being changed in the first iteration of the loop, >> > feeds back into the second iteration. >> >> I'm afraid I don't understand why this is done. Also, the second loop >> is only going to happen when need_paging is true, which only happens >> for HVM guests using shadow or without shared pt with the IOMMU. > > OK I think I'm starting to understand this. The need_paging thing it's > only done if the page tables are not shared because the iommu_enabled > has already reserved some memory for the page tables, that can be > shared with EPT. I think this is all very confusing, and the > calculations done for the iommu_enabled case are wrong. Xen should use > dom0_paging_pages instead. You need to look at the history - the IOMMU part was there before the paging portion, I think. And yes, as indicated in an earlier reply, removing the IOMMU part in favor of using dom0_paging_pages() (and doubling the value when not sharing page tables) would seem preferable. > I still don't understand the need for the 'for' loop. The second iteration of the loop uses a shrunk "avail" resulting from the first iteration, to make sure the remaining amount of memory comes reasonably close to what was requested on the command line. This matters specifically when quite a bit of memory is needed for the page tables. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/PV: fix/generalize guest nul selector handling
>>> On 29.09.17 at 17:17, wrote:
> On Thu, Sep 28, 2017 at 08:41:28AM +, Jan Beulich wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1237,6 +1237,18 @@ arch_do_vcpu_op(
>> return rc;
>> }
>>
>> +/*
>> + * Loading a nul selector does not clear bases and limits on AMD CPUs. Be on
>> + * the safe side and re-initialize both to flat segment values before
>> loading
>> + * a nul selector.
>> + */
>> +#define preload_segment(seg, value) do { \
>> +if ( !((value) & ~3) && \
>> + boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) \
>> +asm volatile ( "movl %k0, %%" #seg\
>
> Shouldn't this be a movw? Segment selectors are 16b, not 32b, but I
> might be missing something here.
Ideally it would be "mov" (i.e. without any suffix), but ...
> I see loadsegment is also using movl, so yes, I guess I'm missing
> something.
... my primary goal was to make the two match up in this regard.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 113995: regressions - trouble: blocked/fail
flight 113995 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/113995/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 113972 build-armhf 6 xen-buildfail REGR. vs. 113972 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked n/a version targeted for testing: xen 0ccac4aa461176a056997c34dbf1ef2eeb78303e baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113972 2017-10-03 21:02:43 Z0 days Testing same since 113979 2017-10-04 00:10:13 Z0 days6 attempts People who touched revisions under test: Bhupinder Thakur Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Wei Liu jobs: build-amd64 fail build-armhf fail build-amd64-libvirt blocked test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked test-amd64-amd64-libvirt blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 424 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] xen-pci-passthrough PCI Express support? (Re: [Qemu-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices)
>>> On 03.10.17 at 02:12, wrote:
> On Thu, Sep 28, 2017 at 10:12:34AM -0300, Eduardo Habkost wrote:
>> On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
>> > >>> On 27.09.17 at 21:56, wrote:
>> > > --- a/hw/xen/xen_pt.c
>> > > +++ b/hw/xen/xen_pt.c
>> > > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
>> > > .instance_size = sizeof(XenPCIPassthroughState),
>> > > .instance_finalize = xen_pci_passthrough_finalize,
>> > > .class_init = xen_pci_passthrough_class_init,
>> > > +.interfaces = (InterfaceInfo[]) {
>> > > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> > > +{ },
>> > > +},
>> > > };
>> >
>> > Passed through devices can be both PCI and PCIe, so following
>> > the description of the patch I don't think these can be statically
>> > given either property. Granted quite a bit of PCIe specific
>> > functionality may be missing in the Xen code ...
>>
>> This is just static data about what the device type supports, not
>> about what a given device instance really is. Deciding if the
>> device is PCIe or Conventional at runtime is out of the scope of
>> this series.
>>
>> That said, if passed through PCI Express devices are really
>> supported, it looks like this should be marked as hybrid.
>
> Can anybody confirm if PCI Express devices are really supported
> by xen-pci-passthrough?
I think I've clearly said they're supported, with some limitations.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
Hi,
On 09/29/2017 09:35 PM, Andre Przywara wrote:
> Hi,
>
> On 09/28/2017 03:49 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 09/28/2017 01:03 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 09/26/2017 10:37 AM, Awais Masood wrote:
This patch adds support for Allwinner H5/sun50i SoC.
Makefile updated to enable ARM64 compilation for sunxi.c.
>
> ...
>
--- a/xen/arch/arm/platforms/sunxi.c
+++ b/xen/arch/arm/platforms/sunxi.c
@@ -22,18 +22,18 @@
#include
/* Watchdog constants: */
-#define SUNXI_WDT_BASE0x01c20c90
+#define SUNXI_WDT_A20_BASE0x01c20c90
+#define SUNXI_WDT_H5_BASE 0x01c20cA0
>>>
>>> I know that we hardcoded this value for the A20. However, I am wondering if
>>> we could find this address from the Device-Tree?
>>
>> Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
>> Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, respectively. I
>> have to check what the differences are, but I guess for our purposes these
>> should be small.
>> That seems like a call to some proper DT driven timer/WDT driver?
>
> Scratch that. I just see that this is solely used for the reset function. So
> we should not need this for the H5 (and the A64 for that matter). We may need
> this for the H3 (Cortex-A7) support, however, which seems quite popular on
> cheap boards.
>
Since reset routine will not be required with PSCI, I assume should revert the
reset code changes for this H5 patch and leave the DT retrieval for another
patch that adds H3 support. Or should I try that stuff for next version of this
patch?
Awais
> Cheers,
> Andre
>
#define SUNXI_WDT_MODE0x04
-#define SUNXI_WDT_MODEADDR(SUNXI_WDT_BASE + SUNXI_WDT_MODE)
#define SUNXI_WDT_MODE_EN (1 << 0)
#define SUNXI_WDT_MODE_RST_EN (1 << 1)
-static void sunxi_reset(void)
+static void sunxi_reset(u32 base)
{
void __iomem *wdt;
-wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
+wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, PAGE_SIZE);
if ( !wdt )
{
dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
@@ -42,19 +42,35 @@ static void sunxi_reset(void)
/* Enable watchdog to trigger a reset after 500 ms: */
writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
- wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
+ wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
iounmap(wdt); >
for (;;)
wfi();
}
-static const char * const sunxi_dt_compat[] __initconst =
+static void sunxi_a20_reset(void)
+{
+sunxi_reset(SUNXI_WDT_A20_BASE);
+}
+
+static void sunxi_h5_reset(void)
+{
+sunxi_reset(SUNXI_WDT_H5_BASE);
>>>
>>> If I read correctly the Device-Tree for
>>> (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is
>>> supporting PSCI 0.2.
>>>
>>> PSCI 0.2 provides call for power-off/reset, so implementation the reset
>>> callback should not be necessary.
>>
>> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
>>
>>> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and should
>>> not need the reset. Andre do you know if it is the case for all the A20?
>>
>> It claims 0.2, but in fact it seems not to be fully compliant, as (from
>> looking at the code) U-Boot lacks the reset and poweroff calls. But it looks
>> rather straight-forward to add them, as U-Boot knows how to reset and one
>> would just need to wire up psci_system_reset to this.
>>
>>> For H5, I would impose PSCI 0.2 as the way to reset the platform.
>>
>> Yes.
>>
>>> I am leaning towards the same for A20 given that it would just be a matter
>>> of upgrading the bootloader. Most likely you would have already done that
>>> to get fixes.
>>
>> Not sure we should push people to upgrade U-Boot in general to be able to
>> use Xen, but as even current mainline U-Boot doesn't seem to support it, I
>> would rather leave the current reset support code in. Last time I checked
>> Linux does the same.
>>
>> Cheers,
>> Andre.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH MINI-OS] Link against libxentoolcore
On Tue, Oct 03, 2017 at 07:45:19PM +0100, Ian Jackson wrote: > Bringing this commit into xen.git needs coordination with the > corresponding change to introduce the new library: this commit nees to > be introdeuced to xen.git immediately after >xentoolcore, _restrict_all: Introduce new library and implementation > > Signed-off-by: Ian Jackson Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space
On Wed, Oct 04, 2017 at 08:30:38AM +, Jan Beulich wrote:
> >>> On 19.09.17 at 17:29, wrote:
> > +static int vpci_portio_read(const struct hvm_io_handler *handler,
> > +uint64_t addr, uint32_t size, uint64_t *data)
> > +{
> > +struct domain *d = current->domain;
> > +unsigned int reg;
> > +pci_sbdf_t sbdf;
> > +uint32_t cf8;
> > +
> > +*data = ~(uint64_t)0;
> > +
> > +if ( addr == 0xcf8 )
> > +{
> > +ASSERT(size == 4);
> > +*data = d->arch.hvm_domain.pci_cf8;
> > +return X86EMUL_OKAY;
> > +}
> > +
> > +cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
> > +if ( !CF8_ENABLED(cf8) )
> > +return X86EMUL_OKAY;
>
> Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
> forwarded to qemu if it's not a config space one. Same in the write path
> then.
No, I don't think this should be forwarded to QEMU. It is a config
space access (because vpci_portio_accept returned true). But the value
in CF8 doesn't have the enabled bit set, hence the access is
discarded.
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -124,6 +124,12 @@ SECTIONS
> > __param_start = .;
> > *(.data.param)
> > __param_end = .;
> > +
> > +#if defined(CONFIG_HAS_PCI) && defined(CONFIG_LATE_HWDOM)
> > + __start_vpci_array = .;
> > + *(.data.vpci)
> > + __end_vpci_array = .;
> > +#endif
> >} :text
> >
> > #if defined(BUILD_ID)
> > @@ -213,6 +219,12 @@ SECTIONS
> > *(.init_array)
> > *(SORT(.init_array.*))
> > __ctors_end = .;
> > +
> > +#if defined(CONFIG_HAS_PCI) && !defined(CONFIG_LATE_HWDOM)
> > + __start_vpci_array = .;
> > + *(.data.vpci)
> > + __end_vpci_array = .;
> > +#endif
> >} :text
>
> Suitable alignment needs to be enforced in both cases, or else we risk
> someone adding something immediately ahead of one of your insertions,
> making __start_vpci_array no longer point to the first entry.
OK, I've used . = ALIGN(POINTER_ALIGN); for both x86 and ARM.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
Hi Awais,
On 04/10/17 10:16, Awais Masood wrote:
> Hi,
>
> On 09/29/2017 09:35 PM, Andre Przywara wrote:
>> Hi,
>>
>> On 09/28/2017 03:49 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 09/28/2017 01:03 PM, Julien Grall wrote:
Hi,
On 09/26/2017 10:37 AM, Awais Masood wrote:
> This patch adds support for Allwinner H5/sun50i SoC.
>
> Makefile updated to enable ARM64 compilation for sunxi.c.
>>
>> ...
>>
> --- a/xen/arch/arm/platforms/sunxi.c
> +++ b/xen/arch/arm/platforms/sunxi.c
> @@ -22,18 +22,18 @@
> #include
> /* Watchdog constants: */
> -#define SUNXI_WDT_BASE0x01c20c90
> +#define SUNXI_WDT_A20_BASE0x01c20c90
> +#define SUNXI_WDT_H5_BASE 0x01c20cA0
I know that we hardcoded this value for the A20. However, I am wondering
if we could find this address from the Device-Tree?
>>>
>>> Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
>>> Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, respectively. I
>>> have to check what the differences are, but I guess for our purposes these
>>> should be small.
>>> That seems like a call to some proper DT driven timer/WDT driver?
>>
>> Scratch that. I just see that this is solely used for the reset function. So
>> we should not need this for the H5 (and the A64 for that matter). We may
>> need this for the H3 (Cortex-A7) support, however, which seems quite popular
>> on cheap boards.
>>
>
> Since reset routine will not be required with PSCI, I assume should revert
> the reset code changes for this H5 patch and leave the DT retrieval for
> another patch that adds H3 support. Or should I try that stuff for next
> version of this patch?
Thanks for the offer, but I already made a patch that adds support for
basically all virtualization capable Allwinner SoCs (both v7 and v8
ones). This looks into the DT for ARMv7 SoCs, but relies entirely on
PSCI for ARMv8 SoCs. I just need to test it, then will send it out.
So actually we won't need anything from that patch here at all, since my
patch supersedes it in a more generic way.
Do you plan on reworking/resending the UART fix (which should come
first, btw, as it is a prerequisite for H5 enablement)?
You could either send the UART fix on its own if there are changes or I
include it as patch 1/2 of my Allwinner "series".
Thanks!
Andre.
>> Cheers,
>> Andre
>>
> #define SUNXI_WDT_MODE0x04
> -#define SUNXI_WDT_MODEADDR(SUNXI_WDT_BASE + SUNXI_WDT_MODE)
> #define SUNXI_WDT_MODE_EN (1 << 0)
> #define SUNXI_WDT_MODE_RST_EN (1 << 1)
> -static void sunxi_reset(void)
> +static void sunxi_reset(u32 base)
> {
> void __iomem *wdt;
> -wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
> +wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK,
> PAGE_SIZE);
> if ( !wdt )
> {
> dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
> /* Enable watchdog to trigger a reset after 500 ms: */
> writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
> - wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
> + wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
> iounmap(wdt); >
> for (;;)
> wfi();
> }
>
> -static const char * const sunxi_dt_compat[] __initconst =
> +static void sunxi_a20_reset(void)
> +{
> +sunxi_reset(SUNXI_WDT_A20_BASE);
> +}
> +
> +static void sunxi_h5_reset(void)
> +{
> +sunxi_reset(SUNXI_WDT_H5_BASE);
If I read correctly the Device-Tree for
(linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is
supporting PSCI 0.2.
PSCI 0.2 provides call for power-off/reset, so implementation the reset
callback should not be necessary.
>>>
>>> Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
>>>
Similarly the cubietrucks we have in osstest are using PSCI 0.2 and should
not need the reset. Andre do you know if it is the case for all the A20?
>>>
>>> It claims 0.2, but in fact it seems not to be fully compliant, as (from
>>> looking at the code) U-Boot lacks the reset and poweroff calls. But it
>>> looks rather straight-forward to add them, as U-Boot knows how to reset and
>>> one would just need to wire up psci_system_reset to this.
>>>
For H5, I would impose PSCI 0.2 as the way to reset the platform.
>>>
>>> Yes.
>>>
I am leaning towards the same for A20 given that it would just be a matter
of upgrading the bootloader. Most likely you would have already done that
to get fixes.
>>>
>>> Not sure we should push people to upgrade U-Boot in general to be able to
>>> use Xen, but as even current mainline U-Boot doesn't seem to support it, I
>>> would rather leave the current reset support code in. Last time I checked
Re: [Xen-devel] [PATCH] x86/pvh: fix memory accounting for Dom0
On Wed, Oct 04, 2017 at 08:42:33AM +, Jan Beulich wrote: > >>> On 28.09.17 at 17:55, wrote: > > OK I think I'm starting to understand this. The need_paging thing it's > > only done if the page tables are not shared because the iommu_enabled > > has already reserved some memory for the page tables, that can be > > shared with EPT. I think this is all very confusing, and the > > calculations done for the iommu_enabled case are wrong. Xen should use > > dom0_paging_pages instead. > > You need to look at the history - the IOMMU part was there before > the paging portion, I think. And yes, as indicated in an earlier reply, > removing the IOMMU part in favor of using dom0_paging_pages() > (and doubling the value when not sharing page tables) would seem > preferable. I've done this in the new version that I posted. > > I still don't understand the need for the 'for' loop. > > The second iteration of the loop uses a shrunk "avail" resulting > from the first iteration, to make sure the remaining amount of > memory comes reasonably close to what was requested on the > command line. This matters specifically when quite a bit of > memory is needed for the page tables. Right, in the new series I was able to get rid of the loop because I use max_pdx to calculate the amount of memory needed for the IOMMU and p2m page-tables. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxl: fix generating array of enums in getypes.py
On Mon, Oct 2, 2017 at 4:18 PM, Oleksandr Grytsov wrote:
> On Mon, Oct 2, 2017 at 3:02 PM, Wei Liu wrote:
>> On Fri, Sep 29, 2017 at 04:49:23PM +0300, Oleksandr Grytsov wrote:
>>> From: Oleksandr Grytsov
>>>
>>> Enum always uses "x" value as input argument. In
>>> case of enum array "t" argument should be passed.
>>>
>>> Signed-off-by: Oleksandr Grytsov
>>
>> Checking parent doesn't seem to be necessary. We already have "w" which
>> is passed by the higher level.
>>
>> Can you try the following patch?
>>
>> From c451e88dc64febbbea835563eb3347cbc24874ce Mon Sep 17 00:00:00 2001
>> From: Wei Liu
>> Date: Mon, 2 Oct 2017 12:48:28 +0100
>> Subject: [PATCH] libxl/gentypes: fix generating array of enums
>>
>> There is no reason to hardcode "x" in code. Use "w" which is passed
>> by the higher level.
>>
>> This change requires us to allow "x" to be unused so that the
>> top-level enum parse_json functions continue to compile.
>>
>> Signed-off-by: Wei Liu
>> ---
>> tools/libxl/gentypes.py | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
>> index 76aca76aaa..88e5c5f30e 100644
>> --- a/tools/libxl/gentypes.py
>> +++ b/tools/libxl/gentypes.py
>> @@ -432,7 +432,7 @@ def libxl_C_type_parse_json(ty, w, v, indent = "",
>> parent = None, discrimina
>> s = ""
>> if parent is None:
>> s += "int rc = 0;\n"
>> -s += "const libxl__json_object *x = o;\n"
>> +s += "const libxl__json_object *x __attribute__((__unused__)) =
>> o;\n"
>>
>> if isinstance(ty, idl.Array):
>> if parent is None:
>> @@ -467,11 +467,11 @@ def libxl_C_type_parse_json(ty, w, v, indent = "",
>> parent = None, discrimina
>> raise Exception("Only KeyedUnion can have discriminator")
>> s += "{\n"
>> s += "const char *enum_str;\n"
>> -s += "if (!libxl__json_object_is_string(x)) {\n"
>> +s += "if (!libxl__json_object_is_string(%s)) {\n" % w
>> s += "rc = -1;\n"
>> s += "goto out;\n"
>> s += "}\n"
>> -s += "enum_str = libxl__json_object_get_string(x);\n"
>> +s += "enum_str = libxl__json_object_get_string(%s);\n" % w
>> s += "rc = %s_from_string(enum_str, %s);\n" % (ty.typename,
>> ty.pass_arg(v, parent is None, idl.PASS_BY_REFERENCE))
>> s += "if (rc)\n"
>> s += "goto out;\n"
>> --
>> 2.11.0
>>
>
> Checked this patch. It works.
>
> --
> Best Regards,
> Oleksandr Grytsov.
Hi Wei, will you commit your patch?
--
Best Regards,
Oleksandr Grytsov.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 113997: regressions - trouble: blocked/fail
flight 113997 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/113997/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 113972 build-armhf 6 xen-buildfail REGR. vs. 113972 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked n/a version targeted for testing: xen 0ccac4aa461176a056997c34dbf1ef2eeb78303e baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113972 2017-10-03 21:02:43 Z0 days Testing same since 113979 2017-10-04 00:10:13 Z0 days7 attempts People who touched revisions under test: Bhupinder Thakur Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Wei Liu jobs: build-amd64 fail build-armhf fail build-amd64-libvirt blocked test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked test-amd64-amd64-libvirt blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 424 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
Hi,
On 10/04/2017 02:26 PM, Andre Przywara wrote:
> Hi Awais,
>
> On 04/10/17 10:16, Awais Masood wrote:
>> Hi,
>>
>> On 09/29/2017 09:35 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 09/28/2017 03:49 PM, Andre Przywara wrote:
Hi,
On 09/28/2017 01:03 PM, Julien Grall wrote:
> Hi,
>
> On 09/26/2017 10:37 AM, Awais Masood wrote:
>> This patch adds support for Allwinner H5/sun50i SoC.
>>
>> Makefile updated to enable ARM64 compilation for sunxi.c.
>>>
>>> ...
>>>
>> --- a/xen/arch/arm/platforms/sunxi.c
>> +++ b/xen/arch/arm/platforms/sunxi.c
>> @@ -22,18 +22,18 @@
>> #include
>> /* Watchdog constants: */
>> -#define SUNXI_WDT_BASE0x01c20c90
>> +#define SUNXI_WDT_A20_BASE0x01c20c90
>> +#define SUNXI_WDT_H5_BASE 0x01c20cA0
>
> I know that we hardcoded this value for the A20. However, I am wondering
> if we could find this address from the Device-Tree?
Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, respectively.
I have to check what the differences are, but I guess for our purposes
these should be small.
That seems like a call to some proper DT driven timer/WDT driver?
>>>
>>> Scratch that. I just see that this is solely used for the reset function.
>>> So we should not need this for the H5 (and the A64 for that matter). We may
>>> need this for the H3 (Cortex-A7) support, however, which seems quite
>>> popular on cheap boards.
>>>
>>
>> Since reset routine will not be required with PSCI, I assume should revert
>> the reset code changes for this H5 patch and leave the DT retrieval for
>> another patch that adds H3 support. Or should I try that stuff for next
>> version of this patch?
>
> Thanks for the offer, but I already made a patch that adds support for
> basically all virtualization capable Allwinner SoCs (both v7 and v8
> ones). This looks into the DT for ARMv7 SoCs, but relies entirely on
> PSCI for ARMv8 SoCs. I just need to test it, then will send it out.
>
> So actually we won't need anything from that patch here at all, since my
> patch supersedes it in a more generic way.
> Do you plan on reworking/resending the UART fix (which should come
> first, btw, as it is a prerequisite for H5 enablement)?
>
> You could either send the UART fix on its own if there are changes or I
> include it as patch 1/2 of my Allwinner "series".
I'll send the latest version of UART fix as a standalone patch.
Awais
>
> Thanks!
> Andre.
>
>>> Cheers,
>>> Andre
>>>
>> #define SUNXI_WDT_MODE0x04
>> -#define SUNXI_WDT_MODEADDR(SUNXI_WDT_BASE + SUNXI_WDT_MODE)
>> #define SUNXI_WDT_MODE_EN (1 << 0)
>> #define SUNXI_WDT_MODE_RST_EN (1 << 1)
>> -static void sunxi_reset(void)
>> +static void sunxi_reset(u32 base)
>> {
>> void __iomem *wdt;
>> -wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
>> +wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK,
>> PAGE_SIZE);
>> if ( !wdt )
>> {
>> dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>> @@ -42,19 +42,35 @@ static void sunxi_reset(void)
>> /* Enable watchdog to trigger a reset after 500 ms: */
>> writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
>> - wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
>> + wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
>> iounmap(wdt); >
>> for (;;)
>> wfi();
>> }
>>
>> -static const char * const sunxi_dt_compat[] __initconst =
>> +static void sunxi_a20_reset(void)
>> +{
>> +sunxi_reset(SUNXI_WDT_A20_BASE);
>> +}
>> +
>> +static void sunxi_h5_reset(void)
>> +{
>> +sunxi_reset(SUNXI_WDT_H5_BASE);
>
> If I read correctly the Device-Tree for
> (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is
> supporting PSCI 0.2.
>
> PSCI 0.2 provides call for power-off/reset, so implementation the reset
> callback should not be necessary.
Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.
> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and
> should not need the reset. Andre do you know if it is the case for all
> the A20?
It claims 0.2, but in fact it seems not to be fully compliant, as (from
looking at the code) U-Boot lacks the reset and poweroff calls. But it
looks rather straight-forward to add them, as U-Boot knows how to reset
and one would just need to wire up psci_system_reset to this.
> For H5, I would impose PSCI 0.2 as the way to reset the platform.
Yes.
> I am leaning towards the same for A20 given that it would just be a
> matter of upgrading the bootloader. Most likely you would have alre
Re: [Xen-devel] [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN
On Tue, Oct 03, 2017 at 07:07:53PM +0100, Andrew Cooper wrote: > Tested with GCC 4.9 of Debian Jessie. > > Signed-off-by: Andrew Cooper > Signed-off-by: Wei Liu Series: Reviewed-by: Wei Liu Tested-by: Wei Liu with GCC 6.3 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxl: fix generating array of enums in getypes.py
On Wed, Oct 04, 2017 at 12:32:05PM +0300, Oleksandr Grytsov wrote:
> On Mon, Oct 2, 2017 at 4:18 PM, Oleksandr Grytsov wrote:
> > On Mon, Oct 2, 2017 at 3:02 PM, Wei Liu wrote:
> >> On Fri, Sep 29, 2017 at 04:49:23PM +0300, Oleksandr Grytsov wrote:
> >>> From: Oleksandr Grytsov
> >>>
> >>> Enum always uses "x" value as input argument. In
> >>> case of enum array "t" argument should be passed.
> >>>
> >>> Signed-off-by: Oleksandr Grytsov
> >>
> >> Checking parent doesn't seem to be necessary. We already have "w" which
> >> is passed by the higher level.
> >>
> >> Can you try the following patch?
> >>
> >> From c451e88dc64febbbea835563eb3347cbc24874ce Mon Sep 17 00:00:00 2001
> >> From: Wei Liu
> >> Date: Mon, 2 Oct 2017 12:48:28 +0100
> >> Subject: [PATCH] libxl/gentypes: fix generating array of enums
> >>
> >> There is no reason to hardcode "x" in code. Use "w" which is passed
> >> by the higher level.
> >>
> >> This change requires us to allow "x" to be unused so that the
> >> top-level enum parse_json functions continue to compile.
> >>
> >> Signed-off-by: Wei Liu
> >> ---
> >> tools/libxl/gentypes.py | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> >> index 76aca76aaa..88e5c5f30e 100644
> >> --- a/tools/libxl/gentypes.py
> >> +++ b/tools/libxl/gentypes.py
> >> @@ -432,7 +432,7 @@ def libxl_C_type_parse_json(ty, w, v, indent = "",
> >> parent = None, discrimina
> >> s = ""
> >> if parent is None:
> >> s += "int rc = 0;\n"
> >> -s += "const libxl__json_object *x = o;\n"
> >> +s += "const libxl__json_object *x __attribute__((__unused__)) =
> >> o;\n"
> >>
> >> if isinstance(ty, idl.Array):
> >> if parent is None:
> >> @@ -467,11 +467,11 @@ def libxl_C_type_parse_json(ty, w, v, indent = "
> >> ", parent = None, discrimina
> >> raise Exception("Only KeyedUnion can have discriminator")
> >> s += "{\n"
> >> s += "const char *enum_str;\n"
> >> -s += "if (!libxl__json_object_is_string(x)) {\n"
> >> +s += "if (!libxl__json_object_is_string(%s)) {\n" % w
> >> s += "rc = -1;\n"
> >> s += "goto out;\n"
> >> s += "}\n"
> >> -s += "enum_str = libxl__json_object_get_string(x);\n"
> >> +s += "enum_str = libxl__json_object_get_string(%s);\n" % w
> >> s += "rc = %s_from_string(enum_str, %s);\n" % (ty.typename,
> >> ty.pass_arg(v, parent is None, idl.PASS_BY_REFERENCE))
> >> s += "if (rc)\n"
> >> s += "goto out;\n"
> >> --
> >> 2.11.0
> >>
> >
> > Checked this patch. It works.
> >
> > --
> > Best Regards,
> > Oleksandr Grytsov.
>
> Hi Wei, will you commit your patch?
>
Yes, I plan to fix this bug one way or another for this release.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space
>>> On 04.10.17 at 11:24, wrote:
> On Wed, Oct 04, 2017 at 08:30:38AM +, Jan Beulich wrote:
>> >>> On 19.09.17 at 17:29, wrote:
>> > +static int vpci_portio_read(const struct hvm_io_handler *handler,
>> > +uint64_t addr, uint32_t size, uint64_t *data)
>> > +{
>> > +struct domain *d = current->domain;
>> > +unsigned int reg;
>> > +pci_sbdf_t sbdf;
>> > +uint32_t cf8;
>> > +
>> > +*data = ~(uint64_t)0;
>> > +
>> > +if ( addr == 0xcf8 )
>> > +{
>> > +ASSERT(size == 4);
>> > +*data = d->arch.hvm_domain.pci_cf8;
>> > +return X86EMUL_OKAY;
>> > +}
>> > +
>> > +cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
>> > +if ( !CF8_ENABLED(cf8) )
>> > +return X86EMUL_OKAY;
>>
>> Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
>> forwarded to qemu if it's not a config space one. Same in the write path
>> then.
>
> No, I don't think this should be forwarded to QEMU. It is a config
> space access (because vpci_portio_accept returned true). But the value
> in CF8 doesn't have the enabled bit set, hence the access is
> discarded.
With the enable bit clear it is my understanding that this is then
_not_ a config space access. vpci_portio_accept() simply doesn't
have enough information to tell.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxl: fix generating array of enums in getypes.py
Wei Liu writes ("Re: [PATCH] libxl: fix generating array of enums in
getypes.py"):
> From: Wei Liu
> Date: Mon, 2 Oct 2017 12:48:28 +0100
> Subject: [PATCH] libxl/gentypes: fix generating array of enums
>
> There is no reason to hardcode "x" in code. Use "w" which is passed
> by the higher level.
Acked-by: Ian Jackson
The logic here is rather tangled but I don't fancy refactoring the idl
compiler...
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)
Hi, >>> Since reset routine will not be required with PSCI, I assume should revert >>> the reset code changes for this H5 patch and leave the DT retrieval for >>> another patch that adds H3 support. Or should I try that stuff for next >>> version of this patch? >> >> Thanks for the offer, but I already made a patch that adds support for >> basically all virtualization capable Allwinner SoCs (both v7 and v8 >> ones). This looks into the DT for ARMv7 SoCs, but relies entirely on >> PSCI for ARMv8 SoCs. I just need to test it, then will send it out. >> >> So actually we won't need anything from that patch here at all, since my >> patch supersedes it in a more generic way. >> Do you plan on reworking/resending the UART fix (which should come >> first, btw, as it is a prerequisite for H5 enablement)? >> >> You could either send the UART fix on its own if there are changes or I >> include it as patch 1/2 of my Allwinner "series". > > I'll send the latest version of UART fix as a standalone patch. Thanks Awais, much appreciated. I don't have that other patch here at the moment, but will send it to you for testing ASAP. Cheers, Andre. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 113977: regressions - FAIL
flight 113977 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/113977/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-multivcpu 6 xen-install fail REGR. vs. 113962 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-localmigrate/x10 fail REGR. vs. 113962 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail REGR. vs. 113962 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail like 113959 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 113962 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 113962 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 113962 test-amd64-amd64-xl-rtds 10 debian-install fail like 113962 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 113962 test-amd64-amd64-xl-qemut-ws16-amd64 10 windows-installfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 13 guest-saverestore fail never pass test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass version targeted for testing: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e baseline version: xen 60823b39a1f3788b7ea98bdaf1eda987156f4c87 Last test of basis 113962 2017-10-03 10:20:21 Z0 days Testing same since 113977 2017-10-03 23:47:37 Z0 days1 attempts People who touched revisions under test: Andrew Cooper jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-prev p
Re: [Xen-devel] [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN
On 03/10/17 19:07, Andrew Cooper wrote:
> TODO at some future point: Fix the following known issues:
>
> Clang 3.9 - linker error in shadow/multi.c with fetch_type_names[]. With
> UBSAN enabled, it appears that dead code elimination doesn't remove the
> single reference to fetch_type_names[] which lives behind DEBUG_TRACE_DUMP.
FYI, the linking error is:
prelink.o: In function `_sh_propagate':
/local/xen.git/xen/arch/x86/mm/shadow/multi.c:731: undefined reference
to `fetch_type_names'
And this patch works around the error:
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 28030ac..7a7ad3d 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -75,15 +75,11 @@ typedef enum {
ft_demand_write = FETCH_TYPE_DEMAND | FETCH_TYPE_WRITE,
} fetch_type_t;
-extern const char *const fetch_type_names[];
-
-#if defined(DEBUG_TRACE_DUMP) && CONFIG_PAGING_LEVELS ==
GUEST_PAGING_LEVELS
-const char *const fetch_type_names[] = {
+static const char *const fetch_type_names[] = {
[ft_prefetch] = "prefetch",
[ft_demand_read] = "demand read",
[ft_demand_write] = "demand write",
};
-#endif
/**/
/* Hash table mapping from guest pagetables to shadows
However, this goes against the intended purpose of c/s 89173c1051a0
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list
>>> On 29.09.17 at 17:38, wrote:
>> -Original Message-
>> From: Andrew Cooper
>> Sent: 29 September 2017 16:35
>> To: Paul Durrant ; xen-de...@lists.xenproject.org
>> Cc: Jan Beulich
>> Subject: Re: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
>> servers rather than a list
>>
>> On 29/09/17 15:51, Paul Durrant wrote:
>> > A subsequent patch will remove the current implicit limitation on creation
>> > of ioreq servers which is due to the allocation of gfns for the ioreq
>> > structures and buffered ioreq ring.
>> >
>> > It will therefore be necessary to introduce an explicit limit and, since
>> > this limit should be small, it simplifies the code to maintain an array of
>> > that size rather than using a list.
>> >
>> > Also, by reserving an array slot for the default server and populating
>> > array slots early in create, the need to pass an 'is_default' boolean
>> > to sub-functions can be avoided.
>> >
>> > Some function return values are changed by this patch: Specifically, in
>> > the case where the id of the default ioreq server is passed in, -
>> EOPNOTSUPP
>> > is now returned rather than -ENOENT.
>> >
>> > Signed-off-by: Paul Durrant
>> > Reviewed-by: Roger Pau Monné
>> > ---
>> > Cc: Jan Beulich
>> > Cc: Andrew Cooper
>> >
>> > v8:
>> > - Addressed various comments from Jan.
>> >
>> > v7:
>> > - Fixed assertion failure found in testing.
>> >
>> > v6:
>> > - Updated according to comments made by Roger on v4 that I'd missed.
>> >
>> > v5:
>> > - Switched GET/SET_IOREQ_SERVER() macros to get/set_ioreq_server()
>> >functions to avoid possible double-evaluation issues.
>> >
>> > v4:
>> > - Introduced more helper macros and relocated them to the top of the
>> >code.
>> >
>> > v3:
>> > - New patch (replacing "move is_default into struct hvm_ioreq_server") in
>> >response to review comments.
>> > ---
>> > xen/arch/x86/hvm/ioreq.c | 525 --
>> -
>> > xen/include/asm-x86/hvm/domain.h | 10 +-
>> > 2 files changed, 270 insertions(+), 265 deletions(-)
>> >
>> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> > index f2e0b3f74a..e655d2eab3 100644
>> > --- a/xen/arch/x86/hvm/ioreq.c
>> > +++ b/xen/arch/x86/hvm/ioreq.c
>> > @@ -33,6 +33,41 @@
>> >
>> > #include
>> >
>> > +static void set_ioreq_server(struct domain *d, unsigned int id,
>> > + struct hvm_ioreq_server *s)
>> > +{
>> > +ASSERT(id < MAX_NR_IOREQ_SERVERS);
>> > +ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]);
>> > +
>> > +d->arch.hvm_domain.ioreq_server.server[id] = s;
>> > +}
>> > +
>> > +#define GET_IOREQ_SERVER(d, id) \
>> > +(d)->arch.hvm_domain.ioreq_server.server[id]
>> > +
>> > +static struct hvm_ioreq_server *get_ioreq_server(const struct domain
>> *d,
>> > + unsigned int id)
>> > +{
>> > +if ( id >= MAX_NR_IOREQ_SERVERS )
>> > +return NULL;
>> > +
>> > +return GET_IOREQ_SERVER(d, id);
>> > +}
>> > +
>> > +#define IS_DEFAULT(s) \
>> > +((s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))
>> > +
>> > +/*
>> > + * Iterate over all possible ioreq servers. The use of inline function
>> > + * get_ioreq_server() in the increment is deliberate as use of the
>> > + * GET_IOREQ_SERVER() macro will cause gcc to complain about an array
>> > + * overflow.
>> > + */
>> > +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
>> > +for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
>> > + (id) < MAX_NR_IOREQ_SERVERS; \
>> > + (s) = get_ioreq_server(d, ++(id)) )
>>
>> I'm guessing from the various constructs, the list of ioreq servers
>> might have embedded NULLs in the middle?
>>
>> If so, how about this?
>>
>> #define FOR_EACH_IOREQ_SERVER(d, id, s) \
>> for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
>> (id) < MAX_NR_IOREQ_SERVERS; \
>> (s) = get_ioreq_server(d, ++(id)) ) \
>> if ( !s ) \
>> continue; \
>> else
>
> I'm ok with it but I'll wait for others opinion on whether this is taking
> the macro magic too far.
I'm fine with Andrew's suggestion; I'm surprised though trickery
like this is being suggested at all, as commonly games I happen
to be trying to play once in a while don't seem to be really liked.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 03/11] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas
On Wed, Oct 04, 2017 at 08:31:18AM +, Jan Beulich wrote:
> >>> On 19.09.17 at 17:29, wrote:
> > +static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
> > + unsigned int len, unsigned long *data)
> > +{
> > +struct domain *d = v->domain;
> > +const struct hvm_mmcfg *mmcfg;
> > +unsigned int reg;
> > +pci_sbdf_t sbdf;
> > +
> > +*data = ~0ul;
> > +
> > +read_lock(&d->arch.hvm_domain.mmcfg_lock);
> > +mmcfg = vpci_mmcfg_find(d, addr);
> > +if ( !mmcfg )
> > +{
> > +read_unlock(&d->arch.hvm_domain.mmcfg_lock);
> > +return X86EMUL_OKAY;
> > +}
>
> With the lock dropped between accept() and read() (or write() below),
> is it really appropriate to return OKAY here? The access again should
> be forwarded to qemu, I would think.
That's right, the MCFG area could have been removed in the meantime.
I guess it is indeed more appropriate to return X86EMUL_UNHANDLEABLE
or X86EMUL_RETRY.
It would seem like RETRY is better, since a new call to _accept should
return false now.
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 04 October 2017 11:16
> To: Andrew Cooper ; Paul Durrant
>
> Cc: xen-de...@lists.xenproject.org
> Subject: RE: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
> servers rather than a list
>
> >>> On 29.09.17 at 17:38, wrote:
> >> -Original Message-
> >> From: Andrew Cooper
> >> Sent: 29 September 2017 16:35
> >> To: Paul Durrant ; xen-
> de...@lists.xenproject.org
> >> Cc: Jan Beulich
> >> Subject: Re: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
> >> servers rather than a list
> >>
> >> On 29/09/17 15:51, Paul Durrant wrote:
> >> > A subsequent patch will remove the current implicit limitation on
> creation
> >> > of ioreq servers which is due to the allocation of gfns for the ioreq
> >> > structures and buffered ioreq ring.
> >> >
> >> > It will therefore be necessary to introduce an explicit limit and, since
> >> > this limit should be small, it simplifies the code to maintain an array
> >> > of
> >> > that size rather than using a list.
> >> >
> >> > Also, by reserving an array slot for the default server and populating
> >> > array slots early in create, the need to pass an 'is_default' boolean
> >> > to sub-functions can be avoided.
> >> >
> >> > Some function return values are changed by this patch: Specifically, in
> >> > the case where the id of the default ioreq server is passed in, -
> >> EOPNOTSUPP
> >> > is now returned rather than -ENOENT.
> >> >
> >> > Signed-off-by: Paul Durrant
> >> > Reviewed-by: Roger Pau Monné
> >> > ---
> >> > Cc: Jan Beulich
> >> > Cc: Andrew Cooper
> >> >
> >> > v8:
> >> > - Addressed various comments from Jan.
> >> >
> >> > v7:
> >> > - Fixed assertion failure found in testing.
> >> >
> >> > v6:
> >> > - Updated according to comments made by Roger on v4 that I'd missed.
> >> >
> >> > v5:
> >> > - Switched GET/SET_IOREQ_SERVER() macros to get/set_ioreq_server()
> >> >functions to avoid possible double-evaluation issues.
> >> >
> >> > v4:
> >> > - Introduced more helper macros and relocated them to the top of the
> >> >code.
> >> >
> >> > v3:
> >> > - New patch (replacing "move is_default into struct
> hvm_ioreq_server") in
> >> >response to review comments.
> >> > ---
> >> > xen/arch/x86/hvm/ioreq.c | 525 --
>
> >> -
> >> > xen/include/asm-x86/hvm/domain.h | 10 +-
> >> > 2 files changed, 270 insertions(+), 265 deletions(-)
> >> >
> >> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> >> > index f2e0b3f74a..e655d2eab3 100644
> >> > --- a/xen/arch/x86/hvm/ioreq.c
> >> > +++ b/xen/arch/x86/hvm/ioreq.c
> >> > @@ -33,6 +33,41 @@
> >> >
> >> > #include
> >> >
> >> > +static void set_ioreq_server(struct domain *d, unsigned int id,
> >> > + struct hvm_ioreq_server *s)
> >> > +{
> >> > +ASSERT(id < MAX_NR_IOREQ_SERVERS);
> >> > +ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]);
> >> > +
> >> > +d->arch.hvm_domain.ioreq_server.server[id] = s;
> >> > +}
> >> > +
> >> > +#define GET_IOREQ_SERVER(d, id) \
> >> > +(d)->arch.hvm_domain.ioreq_server.server[id]
> >> > +
> >> > +static struct hvm_ioreq_server *get_ioreq_server(const struct domain
> >> *d,
> >> > + unsigned int id)
> >> > +{
> >> > +if ( id >= MAX_NR_IOREQ_SERVERS )
> >> > +return NULL;
> >> > +
> >> > +return GET_IOREQ_SERVER(d, id);
> >> > +}
> >> > +
> >> > +#define IS_DEFAULT(s) \
> >> > +((s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))
> >> > +
> >> > +/*
> >> > + * Iterate over all possible ioreq servers. The use of inline function
> >> > + * get_ioreq_server() in the increment is deliberate as use of the
> >> > + * GET_IOREQ_SERVER() macro will cause gcc to complain about an
> array
> >> > + * overflow.
> >> > + */
> >> > +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
> >> > +for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
> >> > + (id) < MAX_NR_IOREQ_SERVERS; \
> >> > + (s) = get_ioreq_server(d, ++(id)) )
> >>
> >> I'm guessing from the various constructs, the list of ioreq servers
> >> might have embedded NULLs in the middle?
> >>
> >> If so, how about this?
> >>
> >> #define FOR_EACH_IOREQ_SERVER(d, id, s) \
> >> for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
> >> (id) < MAX_NR_IOREQ_SERVERS; \
> >> (s) = get_ioreq_server(d, ++(id)) ) \
> >> if ( !s ) \
> >> continue; \
> >> else
> >
> > I'm ok with it but I'll wait for others opinion on whether this is taking
> > the macro magic too far.
>
> I'm fine with Andrew's suggestion; I'm surprised though trickery
> like this is being suggested at all, as commonly games I happen
> to be trying to play once in a while don't seem to be really liked.
>
Yeah, I'm always nervous of macros getting too large... mainly because they end
up being a pain to debug in my experienc
Re: [Xen-devel] [PATCH v6 08/10] arm: PSCI: use definitions provided by asm/smccc.h
Hi Julien, On 03.10.17 16:18, Julien Grall wrote: Hi Volodymyr, On 21/09/17 21:00, Volodymyr Babchuk wrote: diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index 34ee97e..be4e8e6 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -31,9 +31,9 @@ * (native-width) function ID. */ #ifdef CONFIG_ARM_64 -#define PSCI_0_2_FN_NATIVE(name)PSCI_0_2_FN64_##name +#define PSCI_0_2_FN_NATIVE(name)PSCI_0_2_FN64(name) While I am ok that you replace the tabulation by space, I believe you are still using a double tabulations. Xen is using 4 spaces tabulation and not 8. Ah, I see. I just tried to leave indentation as it was before, just changed tab to spaces. Okay, I'll reduce spaces to 4. #else -#define PSCI_0_2_FN_NATIVE(name)PSCI_0_2_FN_##name +#define PSCI_0_2_FN_NATIVE(name)PSCI_0_2_FN32(name) Ditto. #endif Cheers, ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list
On 04/10/17 11:16, Jan Beulich wrote:
On 29.09.17 at 17:38, wrote:
>>> -Original Message-
>>> From: Andrew Cooper
>>> Sent: 29 September 2017 16:35
>>> To: Paul Durrant ; xen-de...@lists.xenproject.org
>>> Cc: Jan Beulich
>>> Subject: Re: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
>>> servers rather than a list
>>>
>>> On 29/09/17 15:51, Paul Durrant wrote:
A subsequent patch will remove the current implicit limitation on creation
of ioreq servers which is due to the allocation of gfns for the ioreq
structures and buffered ioreq ring.
It will therefore be necessary to introduce an explicit limit and, since
this limit should be small, it simplifies the code to maintain an array of
that size rather than using a list.
Also, by reserving an array slot for the default server and populating
array slots early in create, the need to pass an 'is_default' boolean
to sub-functions can be avoided.
Some function return values are changed by this patch: Specifically, in
the case where the id of the default ioreq server is passed in, -
>>> EOPNOTSUPP
is now returned rather than -ENOENT.
Signed-off-by: Paul Durrant
Reviewed-by: Roger Pau Monné
---
Cc: Jan Beulich
Cc: Andrew Cooper
v8:
- Addressed various comments from Jan.
v7:
- Fixed assertion failure found in testing.
v6:
- Updated according to comments made by Roger on v4 that I'd missed.
v5:
- Switched GET/SET_IOREQ_SERVER() macros to get/set_ioreq_server()
functions to avoid possible double-evaluation issues.
v4:
- Introduced more helper macros and relocated them to the top of the
code.
v3:
- New patch (replacing "move is_default into struct hvm_ioreq_server") in
response to review comments.
---
xen/arch/x86/hvm/ioreq.c | 525 --
>>> -
xen/include/asm-x86/hvm/domain.h | 10 +-
2 files changed, 270 insertions(+), 265 deletions(-)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index f2e0b3f74a..e655d2eab3 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -33,6 +33,41 @@
#include
+static void set_ioreq_server(struct domain *d, unsigned int id,
+ struct hvm_ioreq_server *s)
+{
+ASSERT(id < MAX_NR_IOREQ_SERVERS);
+ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]);
+
+d->arch.hvm_domain.ioreq_server.server[id] = s;
+}
+
+#define GET_IOREQ_SERVER(d, id) \
+(d)->arch.hvm_domain.ioreq_server.server[id]
+
+static struct hvm_ioreq_server *get_ioreq_server(const struct domain
>>> *d,
+ unsigned int id)
+{
+if ( id >= MAX_NR_IOREQ_SERVERS )
+return NULL;
+
+return GET_IOREQ_SERVER(d, id);
+}
+
+#define IS_DEFAULT(s) \
+((s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))
+
+/*
+ * Iterate over all possible ioreq servers. The use of inline function
+ * get_ioreq_server() in the increment is deliberate as use of the
+ * GET_IOREQ_SERVER() macro will cause gcc to complain about an array
+ * overflow.
+ */
+#define FOR_EACH_IOREQ_SERVER(d, id, s) \
+for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
+ (id) < MAX_NR_IOREQ_SERVERS; \
+ (s) = get_ioreq_server(d, ++(id)) )
>>> I'm guessing from the various constructs, the list of ioreq servers
>>> might have embedded NULLs in the middle?
>>>
>>> If so, how about this?
>>>
>>> #define FOR_EACH_IOREQ_SERVER(d, id, s) \
>>> for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
>>> (id) < MAX_NR_IOREQ_SERVERS; \
>>> (s) = get_ioreq_server(d, ++(id)) ) \
>>> if ( !s ) \
>>> continue; \
>>> else
>> I'm ok with it but I'll wait for others opinion on whether this is taking
>> the macro magic too far.
> I'm fine with Andrew's suggestion; I'm surprised though trickery
> like this is being suggested at all, as commonly games I happen
> to be trying to play once in a while don't seem to be really liked.
I don't specifically recall a previous example. Either way, tricks like
this do need to be evaluated very carefully, based on how much they help
the code vs how much they hide.
In this case, the FOR_EACH doesn't imply sequential access over an
array, and code inside the loop which uses a break rather than a
continue will definitely cause subtle bugs. Therefore IMO, its benefits
outweigh the costs.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space
On Wed, Oct 04, 2017 at 09:54:18AM +, Jan Beulich wrote:
> >>> On 04.10.17 at 11:24, wrote:
> > On Wed, Oct 04, 2017 at 08:30:38AM +, Jan Beulich wrote:
> >> >>> On 19.09.17 at 17:29, wrote:
> >> > +static int vpci_portio_read(const struct hvm_io_handler *handler,
> >> > +uint64_t addr, uint32_t size, uint64_t
> >> > *data)
> >> > +{
> >> > +struct domain *d = current->domain;
> >> > +unsigned int reg;
> >> > +pci_sbdf_t sbdf;
> >> > +uint32_t cf8;
> >> > +
> >> > +*data = ~(uint64_t)0;
> >> > +
> >> > +if ( addr == 0xcf8 )
> >> > +{
> >> > +ASSERT(size == 4);
> >> > +*data = d->arch.hvm_domain.pci_cf8;
> >> > +return X86EMUL_OKAY;
> >> > +}
> >> > +
> >> > +cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
> >> > +if ( !CF8_ENABLED(cf8) )
> >> > +return X86EMUL_OKAY;
> >>
> >> Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
> >> forwarded to qemu if it's not a config space one. Same in the write path
> >> then.
> >
> > No, I don't think this should be forwarded to QEMU. It is a config
> > space access (because vpci_portio_accept returned true). But the value
> > in CF8 doesn't have the enabled bit set, hence the access is
> > discarded.
>
> With the enable bit clear it is my understanding that this is then
> _not_ a config space access. vpci_portio_accept() simply doesn't
> have enough information to tell.
OK, it was my understanding that accesses to cf8/cfc where only used
by the PCI config space.
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 114001: regressions - trouble: blocked/fail
flight 114001 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114001/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 113972 build-armhf 6 xen-buildfail REGR. vs. 113972 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked n/a version targeted for testing: xen 7efab948acd1c1092b1a9e7b3be2a97389911bc5 baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113972 2017-10-03 21:02:43 Z0 days Failing since113979 2017-10-04 00:10:13 Z0 days8 attempts Testing same since 114001 2017-10-04 10:19:20 Z0 days1 attempts People who touched revisions under test: Bhupinder Thakur Jan Beulich Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Wei Liu jobs: build-amd64 fail build-armhf fail build-amd64-libvirt blocked test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked test-amd64-amd64-libvirt blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 436 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] Null scheduler and VM preemption
Hello, Once assigned to pCPU is vCPU ever preempted using null scheduler? I experimented a little and concluded that it is preempted. Is there a way to avoid preemption? Regards, Ivan Pavic. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 03/10] public: xen.h: add definitions for UUID handling
>>> On 03.10.17 at 14:15, wrote:
> On 21/09/17 20:59, Volodymyr Babchuk wrote:
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -930,6 +930,33 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
>> __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
>> __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
>>
>> +typedef struct
>> +{
>> +uint8_t a[16];
>> +} xen_uuid_t;
>> +
>> +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>> +
>> +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \
>> +(xen_uuid_t){{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \
>> + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \
>> + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \
>> + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \
>> + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \
>> + e1, e2, e3, e4, e5, e6}}
>> +
>> +#else
>> +
>> +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \
>> +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \
>> + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \
>> + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \
>> + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \
>> + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \
>> +e1, e2, e3, e4, e5, e6}}
>
> The only difference between the two macros is the former is having the
> cast whilst the latter not.
>
> How about factorizing the code, i.e:
>
> #define __XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6)
> {{ ... }}
>
> #if defined()
>
> #define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \
> (xen_uuid_t)__XEN_DEFINE_UUID(a, b, c, d, ...)
>
> #else
>
> #define XEN_DEFINE_UUID() __XEN_DEFINE_UUID(...)
>
> Any opinons?
I think this would render things more readable. But the helper
macro must not use a reserved name (i.e. neither two leading
underscores nor a single one followed by an upper case X is
acceptable). I'd recommend a trailing underscore here.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
>>> On 02.10.17 at 16:09, wrote: > On 10/02/2017 02:43 PM, George Dunlap wrote: >> On 09/25/2017 01:03 PM, Petre Pircalabu wrote: >>> Enforce the distinction between an instruction not implemented by the >>> emulator and the failure to emulate that instruction by defining a new >>> return code, X86EMUL_UNIMPLEMENTED. >>> >>> This value should only be returned by the core emulator only if it fails to >>> properly decode the current instruction's opcode, and not by any of other >>> functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. >> >> Oh, minor comment: Should this paragraph be changed to match the >> comment in the patch itself? I.e.: >> >> "This value should only be returned by the core emulator when a valid >> opcode is found but the execution logic for that instruction is missing. >> It should NOT be returned by any of the x86_emulate_ops callbacks." > > I'll do this on check-in if I don't hear any objections by tomorrow. This shouldn't really have gone in without a VMX maintainer ack. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-coverity test] 113998: all pass - PUSHED
flight 113998 xen-unstable-coverity real [real]
http://logs.test-lab.xenproject.org/osstest/logs/113998/
Perfect :-)
All tests in this flight passed as required
version targeted for testing:
xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e
baseline version:
xen 5414ba7f5e1ffc88ed2758b1e1b14bbfd3536a61
Last test of basis 113936 2017-10-01 09:21:40 Z3 days
Testing same since 113998 2017-10-04 09:23:45 Z0 days1 attempts
People who touched revisions under test:
Andrew Cooper
George Dunlap
Jan Beulich
Julien Grall
Petre Pircalabu
Razvan Cojocaru
Roger Pau Monne
Roger Pau Monné
Tamas K Lengyel
Wei Liu
jobs:
coverity-amd64 pass
sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images
Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs
Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master
Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary
Pushing revision :
+ branch=xen-unstable-coverity
+ revision=dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
export PERLLIB=.:.
PERLLIB=.:.
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push
xen-unstable-coverity dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e
+ branch=xen-unstable-coverity
+ revision=dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
export PERLLIB=.:.:.
PERLLIB=.:.:.
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
+++ export PERLLIB=.:.:.:.
+++ PERLLIB=.:.:.:.
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-coverity
+ qemuubranch=qemu-upstream-unstable-coverity
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-coverity
+ prevxenbranch=xen-4.9-testing
+ '[' xdbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/xtf.git
++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git
++ : git://xenbits.xen.org/xtf.git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git
++ : git://git.seabios.org/seabios.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git
++ : git://xenbits.xen.org/osstest/seabios.git
++ : https://github.com/tianocore/edk2.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/linux-firmware.git
++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git
++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
++ : osst...@xenbits.xen.org:/home/xen/git/linux-pvops.git
++ : git://xenbits.xen.org/linux-pvops.git
++ : tested/linux-4.9
++ : tested/linux-arm-xen
++ '[' xgit://xenbits.xen.org/linux-pvops.git = x ']'
++ '[' x = x ']'
++ : git://xenbits.xen.org/linu
Re: [Xen-devel] [PATCH v13 1/3] x86emul: New return code for unimplemented instruction
On 10/04/2017 12:11 PM, Jan Beulich wrote: On 02.10.17 at 16:09, wrote: >> On 10/02/2017 02:43 PM, George Dunlap wrote: >>> On 09/25/2017 01:03 PM, Petre Pircalabu wrote: Enforce the distinction between an instruction not implemented by the emulator and the failure to emulate that instruction by defining a new return code, X86EMUL_UNIMPLEMENTED. This value should only be returned by the core emulator only if it fails to properly decode the current instruction's opcode, and not by any of other functions, such as the x86_emulate_ops or the hvm_io_ops callbacks. >>> >>> Oh, minor comment: Should this paragraph be changed to match the >>> comment in the patch itself? I.e.: >>> >>> "This value should only be returned by the core emulator when a valid >>> opcode is found but the execution logic for that instruction is missing. >>> It should NOT be returned by any of the x86_emulate_ops callbacks." >> >> I'll do this on check-in if I don't hear any objections by tomorrow. > > This shouldn't really have gone in without a VMX maintainer ack. Indeed -- sorry I missed that. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: mmio_ro_do_page_fault() must be unreachable for DomU
>>> On 28.09.17 at 17:17, wrote: > On Thu, Sep 28, 2017 at 09:09:21AM -0600, Jan Beulich wrote: >> When combining PTWR and MMIO-R/O PV page fault handlers, the need for >> a second hwdom check was overlooked. >> >> Signed-off-by: Jan Beulich > > Reviewed-by: Wei Liu This should not have gone in without an ack from Andrew. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: mmio_ro_do_page_fault() must be unreachable for DomU
On Wed, Oct 04, 2017 at 05:26:57AM -0600, Jan Beulich wrote: > >>> On 28.09.17 at 17:17, wrote: > > On Thu, Sep 28, 2017 at 09:09:21AM -0600, Jan Beulich wrote: > >> When combining PTWR and MMIO-R/O PV page fault handlers, the need for > >> a second hwdom check was overlooked. > >> > >> Signed-off-by: Jan Beulich > > > > Reviewed-by: Wei Liu > > This should not have gone in without an ack from Andrew. He said on IRC this patch should be committed. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Formal vote on Unicore Proposal (deadline Frid, Oct 6th)
>>> On 29.09.17 at 14:07, wrote: > I propose to tally the votes by Friday the 6th of October. You can reply via > +1: for proposal > -1: against proposal > in public or private. +1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: mmio_ro_do_page_fault() must be unreachable for DomU
>>> On 04.10.17 at 13:27, wrote: > On Wed, Oct 04, 2017 at 05:26:57AM -0600, Jan Beulich wrote: >> >>> On 28.09.17 at 17:17, wrote: >> > On Thu, Sep 28, 2017 at 09:09:21AM -0600, Jan Beulich wrote: >> >> When combining PTWR and MMIO-R/O PV page fault handlers, the need for >> >> a second hwdom check was overlooked. >> >> >> >> Signed-off-by: Jan Beulich >> > >> > Reviewed-by: Wei Liu >> >> This should not have gone in without an ack from Andrew. > > He said on IRC this patch should be committed. Ah, I see - that's not visible from the commit, though. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 03/11] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas
>>> On 04.10.17 at 12:21, wrote:
> On Wed, Oct 04, 2017 at 08:31:18AM +, Jan Beulich wrote:
>> >>> On 19.09.17 at 17:29, wrote:
>> > +static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
>> > + unsigned int len, unsigned long *data)
>> > +{
>> > +struct domain *d = v->domain;
>> > +const struct hvm_mmcfg *mmcfg;
>> > +unsigned int reg;
>> > +pci_sbdf_t sbdf;
>> > +
>> > +*data = ~0ul;
>> > +
>> > +read_lock(&d->arch.hvm_domain.mmcfg_lock);
>> > +mmcfg = vpci_mmcfg_find(d, addr);
>> > +if ( !mmcfg )
>> > +{
>> > +read_unlock(&d->arch.hvm_domain.mmcfg_lock);
>> > +return X86EMUL_OKAY;
>> > +}
>>
>> With the lock dropped between accept() and read() (or write() below),
>> is it really appropriate to return OKAY here? The access again should
>> be forwarded to qemu, I would think.
>
> That's right, the MCFG area could have been removed in the meantime.
> I guess it is indeed more appropriate to return X86EMUL_UNHANDLEABLE
> or X86EMUL_RETRY.
>
> It would seem like RETRY is better, since a new call to _accept should
> return false now.
Ah, yes, I'm fine with RETRY.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space
>>> On 04.10.17 at 12:32, wrote:
> On Wed, Oct 04, 2017 at 09:54:18AM +, Jan Beulich wrote:
>> >>> On 04.10.17 at 11:24, wrote:
>> > On Wed, Oct 04, 2017 at 08:30:38AM +, Jan Beulich wrote:
>> >> >>> On 19.09.17 at 17:29, wrote:
>> >> > +static int vpci_portio_read(const struct hvm_io_handler *handler,
>> >> > +uint64_t addr, uint32_t size, uint64_t
>> >> > *data)
>> >> > +{
>> >> > +struct domain *d = current->domain;
>> >> > +unsigned int reg;
>> >> > +pci_sbdf_t sbdf;
>> >> > +uint32_t cf8;
>> >> > +
>> >> > +*data = ~(uint64_t)0;
>> >> > +
>> >> > +if ( addr == 0xcf8 )
>> >> > +{
>> >> > +ASSERT(size == 4);
>> >> > +*data = d->arch.hvm_domain.pci_cf8;
>> >> > +return X86EMUL_OKAY;
>> >> > +}
>> >> > +
>> >> > +cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
>> >> > +if ( !CF8_ENABLED(cf8) )
>> >> > +return X86EMUL_OKAY;
>> >>
>> >> Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
>> >> forwarded to qemu if it's not a config space one. Same in the write path
>> >> then.
>> >
>> > No, I don't think this should be forwarded to QEMU. It is a config
>> > space access (because vpci_portio_accept returned true). But the value
>> > in CF8 doesn't have the enabled bit set, hence the access is
>> > discarded.
>>
>> With the enable bit clear it is my understanding that this is then
>> _not_ a config space access. vpci_portio_accept() simply doesn't
>> have enough information to tell.
>
> OK, it was my understanding that accesses to cf8/cfc where only used
> by the PCI config space.
Just like with the overlaid byte accesses to port cf9, other such
overlays could exist too; iirc back when PCI was introduced the
enable bit was used to make sure other uses of this port range
remained reasonably unaffected.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3] xen/ns16550: Fix ISR lockup on Allwinner uart
This patch fixes an ISR lockup seen on Allwinner uart
On Allwinner H5, serial driver goes into an infinite loop
when interrupts are enabled. The reason is a residual
"busy detect" interrupt. Since the condition UART_IIR_NOINT
will not be true unless this interrupt is cleared, the
interrupt handler will remain locked up in this while loop.
A HW quirk fix was previously added for designware uart under
commit:
50417cd978aa54930d065ac1f139f935d14af76d
It checks for a busy condition during setup and clears the
condition by reading UART_USR register.
On Allwinner hardware, the "busy detect" condition occurs
later because an LCR write is performed during setup 'after'
this clear and if uart is busy, the "busy detect" condition
will trigger again and cause the ISR lockup.
To solve this problem, the same UART_USR read operation needs
to be performed within the interrupt handler to clear this
condition.
Linux dw 8250 driver also handles this condition within
interrupt handler
http://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/8250/8250_dw.c#L233
Tested on Orange Pi PC2 (H5). This issue is seen on H3
as well and the same fix works.
Signed-off-by: Awais Masood
---
CC: Andrew Cooper
CC: George Dunlap
CC: Ian Jackson
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
Changes since v2
- Updated comments to clarify that fix is for Allwinner hardware
- Removed ns16550 prefix from local function
Changes since v1
- Common quirk fix code moved to a helper function
- Patch description improved with earlier commit link
---
xen/drivers/char/ns16550.c | 38 +-
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6ab5ec3..e0f8199 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -505,6 +505,23 @@ static int ns16550_ioport_invalid(struct ns16550 *uart)
return ns_read_reg(uart, UART_IER) == 0xff;
}
+static void handle_dw_usr_busy_quirk(struct ns16550 *uart)
+{
+if ( uart->dw_usr_bsy &&
+ (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
+{
+/* DesignWare 8250 detects if LCR is written while the UART is
+ * busy and raises a "busy detect" interrupt. Read the UART
+ * Status Register to clear this state.
+ *
+ * Allwinner/sunxi UART hardware is similar to DesignWare 8250
+ * and also contains a "busy detect" interrupt. So this quirk
+ * fix will also be used for Allwinner UART.
+ */
+ns_read_reg(uart, UART_USR);
+}
+}
+
static void ns16550_interrupt(
int irq, void *dev_id, struct cpu_user_regs *regs)
{
@@ -521,6 +538,16 @@ static void ns16550_interrupt(
serial_tx_interrupt(port, regs);
if ( lsr & UART_LSR_DR )
serial_rx_interrupt(port, regs);
+
+/* A "busy-detect" condition is observed on Allwinner/sunxi UART
+ * after LCR is written during setup. It needs to be cleared at
+ * this point or UART_IIR_NOINT will never be set and this loop
+ * will continue forever.
+ *
+ * This state can be cleared by calling the dw_usr_busy quirk
+ * handler that resolves "busy-detect" for DesignWare uart.
+ */
+handle_dw_usr_busy_quirk(uart);
}
}
@@ -623,15 +650,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
/* No interrupts. */
ns_write_reg(uart, UART_IER, 0);
-if ( uart->dw_usr_bsy &&
- (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
-{
-/* DesignWare 8250 detects if LCR is written while the UART is
- * busy and raises a "busy detect" interrupt. Read the UART
- * Status Register to clear this state.
- */
-ns_read_reg(uart, UART_USR);
-}
+/* Handle the DesignWare 8250 'busy-detect' quirk. */
+handle_dw_usr_busy_quirk(uart);
/* Line control and baud-rate generator. */
ns_write_reg(uart, UART_LCR, lcr | UART_LCR_DLAB);
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 114005: regressions - trouble: blocked/fail
flight 114005 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114005/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 113972 build-armhf 6 xen-buildfail REGR. vs. 113972 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked n/a version targeted for testing: xen a71a96a41e273f3943119965a8cb75550cca6ba8 baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113972 2017-10-03 21:02:43 Z0 days Failing since113979 2017-10-04 00:10:13 Z0 days9 attempts Testing same since 114005 2017-10-04 11:17:04 Z0 days1 attempts People who touched revisions under test: Bhupinder Thakur Ian Jackson Jan Beulich Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Wei Liu jobs: build-amd64 fail build-armhf fail build-amd64-libvirt blocked test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked test-amd64-amd64-libvirt blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 451 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/PV: fix/generalize guest nul selector handling
On 28/09/17 09:41, Jan Beulich wrote: > Segment bases (and limits) aren't being cleared by the loading of a nul > selector into a segment register on AMD CPUs. Therefore, if an > outgoing vCPU has a non-zero base in FS or GS and the subsequent > incoming vCPU has a non-zero but nul selector in the respective > register(s), the selector value(s) would be loaded without clearing the > segment base(s) in the hidden register portion. > > Since the ABI states "zero" in its description of the fs and gs fields, > it is worth noting that the chosen approach to fix this alters the > written down ABI. I consider this preferrable over enforcing the > previously written down behavior, as nul selectors are far more likely > to be what was meant from the beginning. > > The adjustments also eliminate an inconsistency between FS and GS > handling: Old code had an extra pointless (gs_base_user was always zero > when DIRTY_GS was set) conditional for GS. The old bitkeeper changeset > has no explanation for this asymmetry. > > Inspired by Linux commit e137a4d8f4dd2e277e355495b6b2cb241a8693c3. > > Additionally for DS and ES a flat selector is being loaded prior to the > loading of a nul one on AMD CPUs, just as a precautionary measure > (we're not currently aware of ways for a guest to deduce the base of a > segment register which has a nul selector loaded). > > Signed-off-by: Jan Beulich Even if not strictly as written, this is clearly what the ABI should be. The use of nul non-zero selectors is basically only for testing and exploiting corner cases these days, so this shouldn't impact plausible usecases. Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/4] xen/ubsan: Implement __ubsan_handle_nonnull_arg()
>>> On 03.10.17 at 20:07, wrote:
> This hook appears to be missing from the Linux ubsan implemention. This patch
> is a forward port of https://lkml.org/lkml/2014/10/20/182
>
> Signed-off-by: Andrew Cooper
Patch 2 as well as this one
Acked-by: Jan Beulich
albeit preferably with ...
> --- a/xen/common/ubsan/ubsan.c
> +++ b/xen/common/ubsan/ubsan.c
> @@ -328,6 +328,26 @@ void __ubsan_handle_type_mismatch(struct
> type_mismatch_data *data,
> }
> EXPORT_SYMBOL(__ubsan_handle_type_mismatch);
>
> +void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data)
> +{
> + unsigned long flags;
> +
> + if (suppress_report(&data->location))
> + return;
> +
> + ubsan_prologue(&data->location, &flags);
> +
> + pr_err("null pointer passed as argument %d, declared with nonnull
> attribute\n",
> +data->arg_index);
> +
> + if (location_is_valid(&data->attr_location))
> + print_source_location("nonnull attribute declared in ",
> + &data->attr_location);
> +
> + ubsan_epilogue(&flags);
> +}
> +EXPORT_SYMBOL(__ubsan_handle_nonnull_arg);
... all the EXPORT_SYMBOL()s dropped - I was really hoping we
could get ris of what we've left instead of adding new ones.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN
>>> On 03.10.17 at 20:07, wrote: > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -38,4 +38,10 @@ config LTO > > If unsure, say N. > > +# > +# For architectures that know their GCC __int128 support is sound > +# > +config ARCH_SUPPORTS_INT128 > + bool Why GCC? What about Clang? > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -121,6 +121,14 @@ config SCRUB_DEBUG > Verify that pages that need to be scrubbed before being allocated to > a guest are indeed scrubbed. > > +config UBSAN > + bool "Undefined behaviour sanitizer" > + depends on X86 I think we should switch away from this model of explicitly stating architectures, and instead have HAVE_* symbols selected by each architecture supporting it, and the main symbol then depending on the HAVE_* one. Us having only two architectures right now doesn't make this a big difference, but Linux has (partially?) switched to that model for a reason, I think. > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y) > $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS > += > -fprofile-arcs -ftest-coverage > endif > > +ifeq ($(CONFIG_UBSAN),y) > +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS > += -fsanitize=undefined > +endif You have no users of noubsan-y, other than what Wei's RFC patch had. Also neither you nor he explain why *.init.o are unilaterally excluded. > --- a/xen/common/ubsan/ubsan.c > +++ b/xen/common/ubsan/ubsan.c > @@ -10,13 +10,21 @@ > * > */ > > -#include > -#include > -#include > -#include > -#include > -#include > -#include > +#include > +#include > +#include > +#include > +#include > +#include I don't think all of these are needed - xen/types.h is certainly being included implicitly by several others, for example, and always will be. > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -15,6 +15,7 @@ > #define noinline __attribute__((__noinline__)) > > #define noreturn __attribute__((__noreturn__)) > +#define __noreturnnoreturn Please let's avoid new name space violations if at all possibly, or at least restrict them to individual source files where eliminating them would be undesirable. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: Make use of pagetable_get_mfn() where appropriate
>>> On 28.09.17 at 20:36, wrote: > ... instead of the opencoded _mfn(pagetable_get_pfn(...)) construct. > > Fix two overly long lines; no functional change. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Tim Deegan > CC: George Dunlap > --- > xen/arch/x86/domain.c | 2 +- > xen/arch/x86/domctl.c | 2 +- > xen/arch/x86/mm/p2m-ept.c | 12 +++- You forgot to Cc the VMX maintainers for this one. Acked-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [xen-unstable-smoke test] 114005: regressions - trouble: blocked/fail
On 04/10/17 12:47, osstest service owner wrote: > flight 114005 xen-unstable-smoke real [real] > http://logs.test-lab.xenproject.org/osstest/logs/114005/ > > Regressions :-( > > Tests which did not succeed and are blocking, > including tests which could not be run: > build-amd64 6 xen-buildfail REGR. vs. > 113972 I think this was the error discussed on IRC: /home/osstest/build.114005.build-amd64/xen/stubdom/cross-root-x86_64/x86_64-xen-elf/include/string.h:24:8: error: conflicting types for 'memcpy' _PTR _EXFUN(memcpy,(_PTR, const _PTR, size_t)); ^ In file included from /home/osstest/build.114005.build-amd64/xen/extras/mini-os-remote/include/console.h:44:0, from main.c:11: /home/osstest/build.114005.build-amd64/xen/stubdom/include/xen/io/console.h:44:485: note: previous implicit declaration of 'memcpy' was here DEFINE_XEN_FLEX_RING(xencons); I am not sure how to fix that one. > build-armhf 6 xen-buildfail REGR. vs. > 113972 daemon/io.c: In function 'console_evtchn_unmask': daemon/io.c:1050:18: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] long long now = (long long)data; ^ daemon/io.c: In function 'handle_io': daemon/io.c:1346:53: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] console_iter_void_arg2(d, console_evtchn_unmask, (void *)now); ^ It seems that 32-bit build has been built test it. I will send a patch for it. > > Tests which did not succeed, but are not blocking: > test-armhf-armhf-xl 1 build-check(1) blocked n/a > test-amd64-amd64-libvirt 1 build-check(1) blocked n/a > build-amd64-libvirt 1 build-check(1) blocked n/a > test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked > n/a > > version targeted for testing: > xen a71a96a41e273f3943119965a8cb75550cca6ba8 > baseline version: > xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e > > Last test of basis 113972 2017-10-03 21:02:43 Z0 days > Failing since113979 2017-10-04 00:10:13 Z0 days9 attempts > Testing same since 114005 2017-10-04 11:17:04 Z0 days1 attempts > > > People who touched revisions under test: >Bhupinder Thakur >Ian Jackson >Jan Beulich >Julien Grall >Konrad Rzeszutek Wilk >Stefano Stabellini >Wei Liu > > jobs: > build-amd64 fail > build-armhf fail > build-amd64-libvirt blocked > test-armhf-armhf-xl blocked > test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked > test-amd64-amd64-libvirt blocked > > > > sg-report-flight on osstest.test-lab.xenproject.org > logs: /home/logs/logs > images: /home/logs/images > > Logs, config files, etc. are available at > http://logs.test-lab.xenproject.org/osstest/logs > > Explanation of these reports, and of osstest in general, is at > > http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master > http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master > > Test harness code can be found at > http://xenbits.xen.org/gitweb?p=osstest.git;a=summary > > > Not pushing. > > (No revision log; it would be 451 lines long.) > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel > -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline baseline-only test] 72195: tolerable trouble: blocked/broken/fail/pass
This run is configured for baseline tests only. flight 72195 qemu-mainline real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/72195/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl 1 build-check(1) blocked n/a build-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a build-arm64-pvops 2 hosts-allocate broken never pass build-arm64-xsm 2 hosts-allocate broken never pass build-arm64 2 hosts-allocate broken never pass build-arm64-xsm 3 capture-logs broken never pass build-arm64-pvops 3 capture-logs broken never pass build-arm64 3 capture-logs broken never pass test-armhf-armhf-libvirt14 saverestore-support-check fail blocked in 72172 test-armhf-armhf-libvirt-xsm 14 saverestore-support-check fail blocked in 72172 test-armhf-armhf-libvirt-raw 13 saverestore-support-check fail blocked in 72172 test-armhf-armhf-libvirt-raw 15 guest-start/debian.repeat fail blocked in 72172 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail blocked in 72172 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stopfail blocked in 72172 test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail like 72172 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail like 72172 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass test-armhf-armhf-xl-midway 13 migrate-support-checkfail never pass test-armhf-armhf-xl-midway 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-amd64-xl-qemuu-win10-i386 17 guest-stop fail never pass version targeted for testing: qemuud147f7e815f97cb477e223586bcb80c316ae10ea baseline version: qemuuab161529261928ae7f3556e3220829c34b2686ec Last test of basis72172 2017-09-29 00:15:41 Z5 days Testing same since72195 2017-10-04 04:47:50 Z0 days1 attempts People who touched revisions under test: Alex Bennée Alexey Perevalov Daniel P. Berrange Eduardo Habkost Eric Blake Fam Zheng Gerd Hoffmann Greg Kurz Jim Somerville Markus Armbruster Michal Privoznik Paolo Bonzini Peter Maydell Philippe Mathieu-Daudé jobs: build-amd64-xsm pass build-arm64-xsm broken build-armhf-xsm pass build-i386-xsm pass build-amd64
Re: [Xen-devel] [PATCH] migration, xen: Fix block image lock issue on live migration
Am 02.10.2017 um 21:18 hat Dr. David Alan Gilbert geschrieben:
> Adding in kwolf; it looks sane to me; Kevin?
> If I'm reading this right, this is just after the device state save.
Is this actual migration? Because the code looks more like it's copied
and adapted from the snapshot code rather than from the actual migration
code.
If Xen doesn't use the standard mechanisms, I don't know what they need
to do. Snapshots don't need to inactivate images, but migration does.
Compared to the normal migration path, this looks very simplistic, so I
wouldn't be surprised if there was more wrong than just file locking.
This looks like it could work as a hack to the problem at hand. Whether
it is a proper solution, I can't say without investing a lot more time.
Kevin
> * Anthony PERARD (anthony.per...@citrix.com) wrote:
> > When doing a live migration of a Xen guest with libxl, the images for
> > block devices are locked by the original QEMU process, and this prevent
> > the QEMU at the destination to take the lock and the migration fail.
> >
> > From QEMU point of view, once the RAM of a domain is migrated, there is
> > two QMP commands, "stop" then "xen-save-devices-state", at which point a
> > new QEMU is spawned at the destination.
> >
> > Release locks in "xen-save-devices-state" so the destination can takes
> > them.
> >
> > Signed-off-by: Anthony PERARD
> > ---
> > CCing libxl maintainers:
> > CC: Ian Jackson
> > CC: Wei Liu
> > ---
> > migration/savevm.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 4a88228614..69d904c179 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2263,6 +2263,20 @@ void qmp_xen_save_devices_state(const char
> > *filename, Error **errp)
> > qemu_fclose(f);
> > if (ret < 0) {
> > error_setg(errp, QERR_IO_ERROR);
> > +} else {
> > +/* libxl calls the QMP command "stop" before calling
> > + * "xen-save-devices-state" and in case of migration failure, libxl
> > + * would call "cont".
> > + * So call bdrv_inactivate_all (release locks) here to let the
> > other
> > + * side of the migration take controle of the images.
> > + */
> > +if (!saved_vm_running) {
> > +ret = bdrv_inactivate_all();
> > +if (ret) {
> > +error_setg(errp, "%s: bdrv_inactivate_all() failed (%d)",
> > + __func__, ret);
> > +}
> > +}
> > }
> >
> > the_end:
> > --
> > Anthony PERARD
> >
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] xen-pci-passthrough PCI Express support? (Re: [Qemu-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices)
On Wed, Oct 04, 2017 at 03:08:15AM -0600, Jan Beulich wrote:
> >>> On 03.10.17 at 02:12, wrote:
> > On Thu, Sep 28, 2017 at 10:12:34AM -0300, Eduardo Habkost wrote:
> >> On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
> >> > >>> On 27.09.17 at 21:56, wrote:
> >> > > --- a/hw/xen/xen_pt.c
> >> > > +++ b/hw/xen/xen_pt.c
> >> > > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
> >> > > .instance_size = sizeof(XenPCIPassthroughState),
> >> > > .instance_finalize = xen_pci_passthrough_finalize,
> >> > > .class_init = xen_pci_passthrough_class_init,
> >> > > +.interfaces = (InterfaceInfo[]) {
> >> > > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
> >> > > +{ },
> >> > > +},
> >> > > };
> >> >
> >> > Passed through devices can be both PCI and PCIe, so following
> >> > the description of the patch I don't think these can be statically
> >> > given either property. Granted quite a bit of PCIe specific
> >> > functionality may be missing in the Xen code ...
> >>
> >> This is just static data about what the device type supports, not
> >> about what a given device instance really is. Deciding if the
> >> device is PCIe or Conventional at runtime is out of the scope of
> >> this series.
> >>
> >> That said, if passed through PCI Express devices are really
> >> supported, it looks like this should be marked as hybrid.
> >
> > Can anybody confirm if PCI Express devices are really supported
> > by xen-pci-passthrough?
>
> I think I've clearly said they're supported, with some limitations.
Sorry, thanks. I thought the possible missing PCIe functionality
could mean the device couldn't appear as PCI Express to the
guest.
I will submit a follow-up patch adding INTERFACE_PCIE_DEVICE to
xen-pci-passthrough.
--
Eduardo
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 04/16] xen/x86: p2m-pod: Fix coding style
On 04/10/17 06:38, Jan Beulich wrote: Julien Grall 09/28/17 9:30 PM >>> On 09/22/2017 10:15 AM, Jan Beulich wrote: On 21.09.17 at 14:40, wrote: Also take the opportunity to: - move from 1 << * to 1UL << *. - use unsigned when possible - move from unsigned int -> unsigned long for some induction variables I don't understand this last point, btw - the largest order page the code needs to deal with right now is 1Gb, so there's no risk of overflow (yet). But you've got George's and Andrew's ack, so no need to revise this... The last one result from the existing 1UL << in the code. We have place where the induction variable is unsigned int but the shift unsigned long. In which case I would have suggested to change the shift to use 1U, since ... Looking at "<< order" within Xen we seems to use a mix of 1UL <<, 1U <<, 1 <<. The variant 1UL << seems to be predominant. At the end we really want to be consistent with the rest of the code base. Similarly the code is using a mix of 1 << and 1UL <<. I moved to UL because even if the code only support up to 1GB superpage at the moment, it would be pain to find all the places the day we decide to use bigger one. ... I doubt this would be too hard. But in the end it's George to judge anyway. order is the number of MFN that is unsigned long and therefore would make sense to use 1UL <<. So I am not sure why you are pushing for 1U << here... Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [linux-linus test] 113982: tolerable FAIL - PUSHED
flight 113982 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/113982/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemut-win7-amd64 17 guest-stopfail REGR. vs. 113945 test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 113945 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 113945 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 113945 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail like 113945 test-amd64-amd64-xl-rtds 10 debian-install fail like 113945 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 113945 test-amd64-amd64-xl-qemut-ws16-amd64 10 windows-installfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 13 guest-saverestore fail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass version targeted for testing: linuxd81fa669e3de7eb8a631d7d95dac5fbcb2bf9d4e baseline version: linux9e66317d3c92ddaab330c125dfe9d06eee268aff Last test of basis 113945 2017-10-02 04:55:30 Z2 days Failing since113970 2017-10-03 16:59:31 Z0 days2 attempts Testing same since 113982 2017-10-04 01:01:34 Z0 days1 attempts People who touched revisions under test: Adam Wallis Akinobu Mita Alan Stern Alan Tull Alexander Shishkin Alexey Khoroshilov Andreas Klinger Andrey Konovalov Ard Biesheuvel Arnd Bergmann Arun Nagendran Baolin Wang Bjørn Mork Christophe JAILLET Colin Ian King Colin Parker Dan Carpenter Dennis Zhou Dexuan Cui Dmitry Fleytman Dmitry Torokhov Dragos Bogdan Fabrice Gasnier Felipe Balbi Fugang Duan Geert Uytterhoeven Geert Uytterhoeven Greg Kroah-Hartman Guy Shapiro Jakub Kicinski Jim Dickerson Jiri Slaby John Keeping Jonathan Cameron Jose Marino K. Y. Srinivasan Kai-Heng Feng Kris Lindgren Lars-Peter Clausen Linus Torvalds Lorenzo Bianconi Lorenzo Bianconi Lu Baolu Ludovic Desroches Lukas Wunner
Re: [Xen-devel] [PATCH 3/4] xen: sched: improve checking soft-affinity
On 09/15/2017 06:35 PM, Dario Faggioli wrote:
> Whether or not a vCPU has a soft-affinity which is
> effective, i.e., with the power of actually affecting
> the scheduling of the vCPU itself, happens in an
> helper function, called has_soft_affinity().
>
> Such function takes a custom cpumask as its third
> parameter, for better flexibility, but that mask is
> different from the vCPU's hard-affinity only in one
> case. Getting rid of that parameter, not only simplify
> the function, but enables for optimizing the soft
> affinity check (which will happen, in a subsequent
> commit).
>
> This commit, therefore, does that. It's mostly
> mechanical, with the only exception _csched_cpu_pick()
> (in Credit1 code).
>
> Signed-off-by: Dario Faggioli
> ---
> Cc: George Dunlap
> Cc: Anshul Makkar
> ---
> xen/common/sched_credit.c | 79
> +---
> xen/common/sched_credit2.c | 10 ++
> xen/common/sched_null.c|8 ++--
> xen/include/xen/sched-if.h |8 ++--
> 4 files changed, 48 insertions(+), 57 deletions(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 3efbfc8..35d0c98 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -410,8 +410,7 @@ static inline void __runq_tickle(struct csched_vcpu *new)
> int new_idlers_empty;
>
> if ( balance_step == BALANCE_SOFT_AFFINITY
> - && !has_soft_affinity(new->vcpu,
> - new->vcpu->cpu_hard_affinity) )
> + && !has_soft_affinity(new->vcpu) )
> continue;
>
> /* Are there idlers suitable for new (for this balance step)? */
> @@ -743,50 +742,42 @@ __csched_vcpu_is_migrateable(struct vcpu *vc, int
> dest_cpu, cpumask_t *mask)
> static int
> _csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc, bool_t commit)
> {
> -cpumask_t cpus;
Is there a reason you couldn't use cpumask_t *cpus=cpumask_scratch_cpu()?
Other than that, looks good.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN
On 04/10/17 13:03, Jan Beulich wrote: On 03.10.17 at 20:07, wrote: >> --- a/xen/Kconfig >> +++ b/xen/Kconfig >> @@ -38,4 +38,10 @@ config LTO >> >>If unsure, say N. >> >> +# >> +# For architectures that know their GCC __int128 support is sound >> +# >> +config ARCH_SUPPORTS_INT128 >> +bool > Why GCC? What about Clang? This came straight from Linux. I can s/GCC/compiler/ if you like? > >> --- a/xen/Kconfig.debug >> +++ b/xen/Kconfig.debug >> @@ -121,6 +121,14 @@ config SCRUB_DEBUG >>Verify that pages that need to be scrubbed before being allocated to >>a guest are indeed scrubbed. >> >> +config UBSAN >> +bool "Undefined behaviour sanitizer" >> +depends on X86 > I think we should switch away from this model of explicitly stating > architectures, and instead have HAVE_* symbols selected by each > architecture supporting it, and the main symbol then depending on > the HAVE_* one. Us having only two architectures right now > doesn't make this a big difference, but Linux has (partially?) > switched to that model for a reason, I think. I'm not fussed. Which would you prefer? > >> --- a/xen/Rules.mk >> +++ b/xen/Rules.mk >> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y) >> $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS >> += >> -fprofile-arcs -ftest-coverage >> endif >> >> +ifeq ($(CONFIG_UBSAN),y) >> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): >> CFLAGS += -fsanitize=undefined >> +endif > You have no users of noubsan-y, other than what Wei's RFC patch > had. Also neither you nor he explain why *.init.o are unilaterally > excluded. The answer is complicated. If you want it to work with .init. files, then the following change is required: diff --git a/xen/Rules.mk b/xen/Rules.mk index cafc67b..9ce5b56 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -189,7 +189,7 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile .text|.text.*|.data|.data.*|.bss) \ test $$sz != 0 || continue; \ echo "Error: size of $<:$$name is 0x$$sz" >&2; \ - exit $$(expr $$idx + 1);; \ + # exit $$(expr $$idx + 1);; \ esac; \ done $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ I was debating whether to keep or remove the noubsan, but I figured that keeping it would be more flexible for developing with. > >> --- a/xen/common/ubsan/ubsan.c >> +++ b/xen/common/ubsan/ubsan.c >> @@ -10,13 +10,21 @@ >> * >> */ >> >> -#include >> -#include >> -#include >> -#include >> -#include >> -#include >> -#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > I don't think all of these are needed - xen/types.h is certainly > being included implicitly by several others, for example, and > always will be. Hmm. This list of headers is ~18 months old. I will see if I can prune it some more. > >> --- a/xen/include/xen/compiler.h >> +++ b/xen/include/xen/compiler.h >> @@ -15,6 +15,7 @@ >> #define noinline __attribute__((__noinline__)) >> >> #define noreturn __attribute__((__noreturn__)) >> +#define __noreturnnoreturn > Please let's avoid new name space violations if at all possibly, or > at least restrict them to individual source files where eliminating > them would be undesirable. This is entirely down to how much we want to diverge the Linux code. For ubsan, I've gone out of my way not to modify the Linux code at all. I can see an argument for making this local to the file in question. However, that needs to be weighed up against other Linux source we choose to take. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 114007: regressions - trouble: blocked/fail
flight 114007 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114007/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 113972 build-armhf 6 xen-buildfail REGR. vs. 113972 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-i386 1 build-check(1) blocked n/a version targeted for testing: xen a71a96a41e273f3943119965a8cb75550cca6ba8 baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113972 2017-10-03 21:02:43 Z0 days Failing since113979 2017-10-04 00:10:13 Z0 days 10 attempts Testing same since 114005 2017-10-04 11:17:04 Z0 days2 attempts People who touched revisions under test: Bhupinder Thakur Ian Jackson Jan Beulich Julien Grall Konrad Rzeszutek Wilk Stefano Stabellini Wei Liu jobs: build-amd64 fail build-armhf fail build-amd64-libvirt blocked test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 blocked test-amd64-amd64-libvirt blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 451 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 05/11] pci: split code to size BARs from pci_add_device
On Wed, Oct 04, 2017 at 08:32:06AM +, Jan Beulich wrote: > >>> On 19.09.17 at 17:29, wrote: > > --- a/xen/include/xen/pci.h > > +++ b/xen/include/xen/pci.h > > @@ -189,6 +189,10 @@ const char *parse_pci(const char *, unsigned int *seg, > > unsigned int *bus, > > const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int > > *bus, > >unsigned int *dev, unsigned int *func, bool > > *def_seg); > > > > +#define _PCI_BAR_VF 0 > > +#define PCI_BAR_VF (1u << _PCI_BAR_VF) > > Do you really need both? I know we have quite a few cases where flags > are being defined this way, but that's usually when bit operations > (test_bit() and alike) are intended on the flags fields. Ack, would you then rather prefer to have 1, or (1u << 0)? (to keep it in line with the other flag that will be added later). Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [MINIOS PATCH] Include string.h before console.h
Starting from Xen commit af8d93564, it is required to include string.h before console.h. Signed-off-by: Wei Liu --- Cc: Ian Jackson Cc: Julien Grall Cc: Samuel Thibault After applying this patch to mini-os I will update Xen's Config.mk to fix the build failure in xen.git. I will also update mini-os's copy of Xen headers later. --- lib/sys.c | 1 + main.c| 1 + 2 files changed, 2 insertions(+) diff --git a/lib/sys.c b/lib/sys.c index b173bc8..23dc2a5 100644 --- a/lib/sys.c +++ b/lib/sys.c @@ -20,6 +20,7 @@ #ifdef HAVE_LIBC #include +#include #include #include #include diff --git a/main.c b/main.c index 263364c..4e42f53 100644 --- a/main.c +++ b/main.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/4] xen: sched: introduce 'adjust_affinity' hook.
On 09/15/2017 06:35 PM, Dario Faggioli wrote: > For now, just as a way to give a scheduler an "heads up", > about the fact that the affinity changed. > > This enables some optimizations, such as pre-computing > and storing (e.g., in flags) facts like a vcpu being > exclusively pinned to a pcpu, or having or not a > soft affinity. I.e., conditions that, despite the fact > that they rarely change, are right now checked very > frequently, even in hot paths. > > Note also that this, in future, may turn out as a useful > mean for, e.g., having the schedulers vet, ack or nack > the changes themselves. > > Signed-off-by: Dario Faggioli Reviewed-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/4] xen: sched: optimize exclusive pinning case (Credit1 & 2)
On 09/15/2017 06:35 PM, Dario Faggioli wrote: > Exclusive pinning of vCPUs is used, sometimes, for > achieving the highest level of determinism, and the > least possible overhead, for the vCPUs in question. > > Although static 1:1 pinning is not recommended, for > general use cases, optimizing the tickling code (of > Credit1 and Credit2) is easy and cheap enough, so go > for it. > > Signed-off-by: Dario Faggioli Reviewed-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [MINIOS PATCH] Include string.h before console.h
On Wed, Oct 04, 2017 at 02:33:36PM +0100, Wei Liu wrote: > Starting from Xen commit af8d93564, it is required to include string.h > before console.h. > > Signed-off-by: Wei Liu Sorry this one isn't complete. There are other instances where string.h are needed. But the basic idea is the same. > --- > Cc: Ian Jackson > Cc: Julien Grall > Cc: Samuel Thibault > > After applying this patch to mini-os I will update Xen's Config.mk to > fix the build failure in xen.git. > > I will also update mini-os's copy of Xen headers later. > --- > lib/sys.c | 1 + > main.c| 1 + > 2 files changed, 2 insertions(+) > > diff --git a/lib/sys.c b/lib/sys.c > index b173bc8..23dc2a5 100644 > --- a/lib/sys.c > +++ b/lib/sys.c > @@ -20,6 +20,7 @@ > > #ifdef HAVE_LIBC > #include > +#include > #include > #include > #include > diff --git a/main.c b/main.c > index 263364c..4e42f53 100644 > --- a/main.c > +++ b/main.c > @@ -8,6 +8,7 @@ > #include > #include > #include > +#include > #include > #include > #include > -- > 2.11.0 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/4] xen: sched: simplify (and speedup) checking soft-affinity
On 09/15/2017 06:35 PM, Dario Faggioli wrote: > The fact of whether or not a vCPU has a soft-affinity > which is effective, i.e., with the power of actually > affecting the scheduling of the vCPU itself rarely > changes. Very, very rarely, as compared to how often > we need to check for the same thing (basically, at > every scheduling decision!). > > That can be improved by storing in a (per-vCPU) flag > (it's actually a boolean field in struct vcpu) whether > or not, considering how hard-affinity and soft-affinity > look like, soft-affinity should or not be taken into > account during scheduling decisions. > > This saves some cpumask manipulations, which is nice, > considering how frequently they were being done. Note > that we can't get rid of 100% of the cpumask operations > involved in the check, because soft-affinity being > effective or not, not only depends on the relationship > between the hard and soft-affinity masks of a vCPU, but > also of the online pCPUs and/or of what pCPUs are part > of the cpupool where the vCPU lives, and that's rather > impractical to store in a per-vCPU flag. Still the > overhead is reduced to "just" one cpumask_subset() (and > only if the newly introduced flag is 'true')! > > Signed-off-by: Dario Faggioli Reviewed-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/4] xen: sched: optimize exclusive pinning and soft-affinity checking
On 09/15/2017 06:35 PM, Dario Faggioli wrote: > Hello, > > This series is a rework of a patch that was, originally, sent as part of a > series (which went in already): > > https://lists.xen.org/archives/html/xen-devel/2017-07/msg02167.html > > As it can be seen in the message above, George suggested doing things a little > bit differently, and I agreed. however, that: > - require a bit more of rework than expected, but here we are; > - opened the possibility for even more optimization. :-) > > Basically, the effect of the series is that: > 1) when a vCPU is exclusively pinned to a pCPU, a lot of checks, while making >scheduling decisions in Credit1 and Credit2, are skipped (patch 2); > 2) the check to see whether or not a vCPU has a soft-affinity that actually >matters for the scheduling of the vCPU itself, and should be considered is >optimized and made faster (patch 4). > > So, the important bits are in patches 2 and 4, but both patches 1 and 3 are > necessary to make the other twos possible. I like it -- thanks, Dario! -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [MINIOS PATCH] Include string.h before console.h
Wei Liu, on mer. 04 oct. 2017 14:33:36 +0100, wrote: > Starting from Xen commit af8d93564, it is required to include string.h > before console.h. > > Signed-off-by: Wei Liu Reviewed-by: Samuel Thibault > --- > Cc: Ian Jackson > Cc: Julien Grall > Cc: Samuel Thibault > > After applying this patch to mini-os I will update Xen's Config.mk to > fix the build failure in xen.git. > > I will also update mini-os's copy of Xen headers later. > --- > lib/sys.c | 1 + > main.c| 1 + > 2 files changed, 2 insertions(+) > > diff --git a/lib/sys.c b/lib/sys.c > index b173bc8..23dc2a5 100644 > --- a/lib/sys.c > +++ b/lib/sys.c > @@ -20,6 +20,7 @@ > > #ifdef HAVE_LIBC > #include > +#include > #include > #include > #include > diff --git a/main.c b/main.c > index 263364c..4e42f53 100644 > --- a/main.c > +++ b/main.c > @@ -8,6 +8,7 @@ > #include > #include > #include > +#include > #include > #include > #include > -- > 2.11.0 > -- Samuel Battery 1: charging, 90%, charging at zero rate - will never fully charge. -+- acpi - et pourtant, ca monte -+- ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [xen-unstable-smoke test] 114005: regressions - trouble: blocked/fail
Hi, On 4 October 2017 at 17:56, Julien Grall wrote: > > > On 04/10/17 12:47, osstest service owner wrote: >> flight 114005 xen-unstable-smoke real [real] >> http://logs.test-lab.xenproject.org/osstest/logs/114005/ >> >> Regressions :-( >> >> Tests which did not succeed and are blocking, >> including tests which could not be run: >> build-amd64 6 xen-buildfail REGR. vs. >> 113972 > > I think this was the error discussed on IRC: > > /home/osstest/build.114005.build-amd64/xen/stubdom/cross-root-x86_64/x86_64-xen-elf/include/string.h:24:8: > error: conflicting types for 'memcpy' > _PTR _EXFUN(memcpy,(_PTR, const _PTR, size_t)); > ^ > In file included from > /home/osstest/build.114005.build-amd64/xen/extras/mini-os-remote/include/console.h:44:0, > from main.c:11: > /home/osstest/build.114005.build-amd64/xen/stubdom/include/xen/io/console.h:44:485: > note: previous implicit declaration of 'memcpy' was here > DEFINE_XEN_FLEX_RING(xencons); > > I am not sure how to fix that one. > It seems that grub/main.c does not include string.h before including console.h which could be causing this implicit declaration. Ideally, ring.h should include string.h explicitly as it depends on string.h. > >> build-armhf 6 xen-buildfail REGR. vs. >> 113972 > > daemon/io.c: In function 'console_evtchn_unmask': > daemon/io.c:1050:18: error: cast from pointer to integer of different size > [-Werror=pointer-to-int-cast] > long long now = (long long)data; > ^ > daemon/io.c: In function 'handle_io': > daemon/io.c:1346:53: error: cast to pointer from integer of different size > [-Werror=int-to-pointer-cast] > console_iter_void_arg2(d, console_evtchn_unmask, (void *)now); > > ^ > > It seems that 32-bit build has been built test it. I will send a patch for it. > Instead of using a generic void *, I can use the long long type itself. I think there was a review comment regarding that. Regards, Bhupinder ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [xen-unstable-smoke test] 114005: regressions - trouble: blocked/fail
On Wed, Oct 04, 2017 at 07:09:22PM +0530, Bhupinder Thakur wrote: > Hi, > > On 4 October 2017 at 17:56, Julien Grall wrote: > > > > > > On 04/10/17 12:47, osstest service owner wrote: > >> flight 114005 xen-unstable-smoke real [real] > >> http://logs.test-lab.xenproject.org/osstest/logs/114005/ > >> > >> Regressions :-( > >> > >> Tests which did not succeed and are blocking, > >> including tests which could not be run: > >> build-amd64 6 xen-buildfail REGR. vs. > >> 113972 > > > > I think this was the error discussed on IRC: > > > > /home/osstest/build.114005.build-amd64/xen/stubdom/cross-root-x86_64/x86_64-xen-elf/include/string.h:24:8: > > error: conflicting types for 'memcpy' > > _PTR _EXFUN(memcpy,(_PTR, const _PTR, size_t)); > > ^ > > In file included from > > /home/osstest/build.114005.build-amd64/xen/extras/mini-os-remote/include/console.h:44:0, > > from main.c:11: > > /home/osstest/build.114005.build-amd64/xen/stubdom/include/xen/io/console.h:44:485: > > note: previous implicit declaration of 'memcpy' was here > > DEFINE_XEN_FLEX_RING(xencons); > > > > I am not sure how to fix that one. > > > It seems that grub/main.c does not include string.h before including > console.h which could be causing this implicit declaration. Ideally, > ring.h should include string.h explicitly as it depends on string.h. Yes. Patch submitted already. :-) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] stable-rc build: 3 warnings 0 failures (stable-rc/v4.4.89-42-g255b4a0)
On Wed, Oct 4, 2017 at 10:41 AM, Olof's autobuilder wrote:
>
> Warnings:
>
> 1 arm/xen/mm.c:202:2: warning: initialization from incompatible pointer
> type
> 1 arm/xen/mm.c:202:2: warning: (near initialization for
> 'xen_swiotlb_dma_ops.mmap')
> 1 drivers/xen/swiotlb-xen.c:697:27: warning: passing argument 6 of
> '__generic_dma_ops(dev)->mmap' makes pointer from integer without a cast
This is caused by backporting 2f0b82b1b830 ("swiotlb-xen: implement
xen_swiotlb_dma_mmap callback") into stable-rc/4.4.
The ->mmap() callback signature changed in linux-4.8 as of commit
00085f1efa38 ("dma-mapping: use unsigned long for dma_attrs"), so
all older kernels are affected.
Please drop the backport patch from 4.4 and 3.18, or apply the trivial fixup
that I'll send in a bit.
Arnd
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [libvirt test] 113990: tolerable all pass - PUSHED
flight 113990 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/113990/
Failures :-/ but no regressions.
Tests which did not succeed, but are not blocking:
test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail like 113960
test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 113960
test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 113960
test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass
test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass
test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass
test-amd64-i386-libvirt 13 migrate-support-checkfail never pass
test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check
fail never pass
test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check
fail never pass
test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail never pass
test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass
test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail never pass
test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass
test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass
version targeted for testing:
libvirt 4ddb4bb861e90170e1354195b11337abd009f6a4
baseline version:
libvirt a55eaced607c9253f8422e2a207ef12039f99558
Last test of basis 113960 2017-10-03 04:21:19 Z1 days
Testing same since 113990 2017-10-04 04:30:10 Z0 days1 attempts
People who touched revisions under test:
Jiri Denemark
Martin Kletzander
jobs:
build-amd64-xsm pass
build-armhf-xsm pass
build-i386-xsm pass
build-amd64 pass
build-armhf pass
build-i386 pass
build-amd64-libvirt pass
build-armhf-libvirt pass
build-i386-libvirt pass
build-amd64-pvopspass
build-armhf-pvopspass
build-i386-pvops pass
test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass
test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
test-amd64-amd64-libvirt-xsm pass
test-armhf-armhf-libvirt-xsm pass
test-amd64-i386-libvirt-xsm pass
test-amd64-amd64-libvirt pass
test-armhf-armhf-libvirt pass
test-amd64-i386-libvirt pass
test-amd64-amd64-libvirt-pairpass
test-amd64-i386-libvirt-pair pass
test-amd64-i386-libvirt-qcow2pass
test-armhf-armhf-libvirt-raw pass
test-amd64-amd64-libvirt-vhd pass
sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images
Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs
Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master
Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary
Pushing revision :
+ branch=libvirt
+ revision=4ddb4bb861e90170e1354195b11337abd009f6a4
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
export PERLLIB=.:.
PERLLIB=.:.
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push libvirt
4ddb4bb861e90170e1354195b11337abd009f6a4
+ branch=libvirt
+ revision=4ddb4bb861e90170e1354195b11337abd009f6a4
+ . ./cri
[Xen-devel] [PATCH] [STABLE-4.4, 3.18]: fix xen_swiotlb_dma_mmap prototype
xen_swiotlb_dma_mmap was backported from v4.10, but older
kernels before commit 00085f1efa38 ("dma-mapping: use unsigned long
for dma_attrs") use a different signature:
arm/xen/mm.c:202:10: error: initialization from incompatible pointer type
[-Werror=incompatible-pointer-types]
.mmap = xen_swiotlb_dma_mmap,
^~~~
arm/xen/mm.c:202:10: note: (near initialization for 'xen_swiotlb_dma_ops.mmap')
This adapts the patch to the old calling conventions.
Fixes: 2f0b82b1b830 ("swiotlb-xen: implement xen_swiotlb_dma_mmap callback")
Signed-off-by: Arnd Bergmann
---
drivers/xen/swiotlb-xen.c | 2 +-
include/xen/swiotlb-xen.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 622f805fb382..f7b19c25c3a4 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -689,7 +689,7 @@ EXPORT_SYMBOL_GPL(xen_swiotlb_set_dma_mask);
int
xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
-unsigned long attrs)
+struct dma_attrs *attrs)
{
#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
if (__generic_dma_ops(dev)->mmap)
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index fab4fb9c6442..4d7fdbf20eff 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -62,5 +62,5 @@ xen_swiotlb_set_dma_mask(struct device *dev, u64 dma_mask);
extern int
xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
void *cpu_addr, dma_addr_t dma_addr, size_t size,
-unsigned long attrs);
+struct dma_attrs *attrs);
#endif /* __LINUX_SWIOTLB_XEN_H */
--
2.9.0
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [MINIOS PATCH v2] Include string.h before console.h
Starting from Xen commit af8d93564, it is required to include string.h before console.h. Signed-off-by: Wei Liu --- Cc: Ian Jackson Cc: Julien Grall Cc: Samuel Thibault After applying this patch to mini-os I will update Xen's Config.mk to fix the build failure in xen.git. There are quite a few places in the not yet functioning arm port. I opt to skip them so that we can unblock staging asap. --- lib/sys.c | 1 + lwip-arch.c | 1 + main.c | 1 + 3 files changed, 3 insertions(+) diff --git a/lib/sys.c b/lib/sys.c index b173bc8..23dc2a5 100644 --- a/lib/sys.c +++ b/lib/sys.c @@ -20,6 +20,7 @@ #ifdef HAVE_LIBC #include +#include #include #include #include diff --git a/lwip-arch.c b/lwip-arch.c index 154617b..21e941e 100644 --- a/lwip-arch.c +++ b/lwip-arch.c @@ -8,6 +8,7 @@ #include #include +#include #include #include #include diff --git a/main.c b/main.c index 263364c..4e42f53 100644 --- a/main.c +++ b/main.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [MINIOS PATCH v2] Include string.h before console.h
Wei Liu, on mer. 04 oct. 2017 14:51:26 +0100, wrote: > Starting from Xen commit af8d93564, it is required to include string.h > before console.h. > > Signed-off-by: Wei Liu Reviewed-by: Samuel Thibault > --- > Cc: Ian Jackson > Cc: Julien Grall > Cc: Samuel Thibault > > After applying this patch to mini-os I will update Xen's Config.mk to > fix the build failure in xen.git. > > There are quite a few places in the not yet functioning arm port. I opt to > skip them so that we can unblock staging asap. > --- > lib/sys.c | 1 + > lwip-arch.c | 1 + > main.c | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/lib/sys.c b/lib/sys.c > index b173bc8..23dc2a5 100644 > --- a/lib/sys.c > +++ b/lib/sys.c > @@ -20,6 +20,7 @@ > > #ifdef HAVE_LIBC > #include > +#include > #include > #include > #include > diff --git a/lwip-arch.c b/lwip-arch.c > index 154617b..21e941e 100644 > --- a/lwip-arch.c > +++ b/lwip-arch.c > @@ -8,6 +8,7 @@ > > #include > #include > +#include > #include > #include > #include > diff --git a/main.c b/main.c > index 263364c..4e42f53 100644 > --- a/main.c > +++ b/main.c > @@ -8,6 +8,7 @@ > #include > #include > #include > +#include > #include > #include > #include > -- > 2.11.0 > -- Samuel j'en ai parlé à xavier, il n'est pas interdit qu'il le change un jour -+- #sos - a le bras long vers le chameau -+- ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 21/27 v11] xen/arm: vpl011: Add support for multiple consoles in xenconsole
>
> @@ -1223,13 +1316,13 @@ void handle_io(void)
> /* Re-calculate any event counter allowances & unblock
> domains with new allowance */
> for (d = dom_head; d; d = d->next) {
> - struct console *con = &d->console;
>
> - console_evtchn_unmask(con, (void *)now);
> + console_iter_void_arg2(d, console_evtchn_unmask, (void
> *)now);
This (together with patch 15's "long long now = (long long)data" cast)
generates fatal warning when building for 32-bit.
The warning can probably be eliminated by casting to uintptr_t (or some
such) but my question is whether 'now' should be a 'long long' --- isn't
time_t a 4-byte type on 32-bit? And if it's not then casting will lose
the upper word.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 21/27 v11] xen/arm: vpl011: Add support for multiple consoles in xenconsole
On Wed, Oct 04, 2017 at 10:03:05AM -0400, Boris Ostrovsky wrote:
>
> >
> > @@ -1223,13 +1316,13 @@ void handle_io(void)
> > /* Re-calculate any event counter allowances & unblock
> >domains with new allowance */
> > for (d = dom_head; d; d = d->next) {
> > - struct console *con = &d->console;
> >
> > - console_evtchn_unmask(con, (void *)now);
> > + console_iter_void_arg2(d, console_evtchn_unmask, (void
> > *)now);
>
> This (together with patch 15's "long long now = (long long)data" cast)
> generates fatal warning when building for 32-bit.
>
> The warning can probably be eliminated by casting to uintptr_t (or some
> such) but my question is whether 'now' should be a 'long long' --- isn't
> time_t a 4-byte type on 32-bit? And if it's not then casting will lose
> the upper word.
>
It would be better to just pass the pointer instead.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/9] x86/np2m: flush all np2m objects on nested INVEPT
On 10/03/2017 04:20 PM, Sergey Dyasli wrote: > At the moment, nvmx_handle_invept() updates the current np2m just to > flush it. Instead introduce a function, np2m_flush_base(), which will > look up the np2m base pointer and call p2m_flush_table() instead. > > Unfortunately, since we don't know which p2m a given vcpu is using, we > must flush all p2ms that share that base pointer. > > Convert p2m_flush_table() into p2m_flush_table_locked() in order not > to release the p2m_lock after np2m_base check. > > Signed-off-by: Sergey Dyasli > Signed-off-by: George Dunlap > --- > v2 --> v3: > - Commit message update > --- > xen/arch/x86/hvm/vmx/vvmx.c | 7 +-- > xen/arch/x86/mm/p2m.c | 35 +-- > xen/include/asm-x86/p2m.h | 2 ++ This needs a VMX maintainer's Ack. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/9] x86/np2m: simplify nestedhvm_hap_nested_page_fault()
On 10/03/2017 04:20 PM, Sergey Dyasli wrote: > There is a possibility for nested_p2m to became stale between > nestedhvm_hap_nested_page_fault() and nestedhap_fix_p2m(). At the moment > this is handled by detecting such a race inside nestedhap_fix_p2m() and > special-casing it. > > Instead, introduce p2m_get_nestedp2m_locked(), which will returned a > still-locked p2m. This allows us to call nestedhap_fix_p2m() with the > lock held and remove the code detecting the special-case. > > Signed-off-by: Sergey Dyasli > Signed-off-by: George Dunlap It seems really strange to have two functions named _locked() which have different semantics (one of which assumes that the p2m has already been locked, one which returns a p2m which is locked). But I can't think of a better option, so: Reviewed-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xenconsole: fix 32bit compilation
Signed-off-by: Wei Liu
---
Cc: Ian Jackson
Cc: Julien Grall
Cc: Bhupinder Thakur
---
tools/console/daemon/io.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 2615b50a4d..afe162e9c2 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -1047,7 +1047,7 @@ static void handle_tty_write(struct console *con)
static void console_evtchn_unmask(struct console *con, void *data)
{
- long long now = (long long)data;
+ long long now = *(long long *)data;
if (!console_enabled(con))
return;
@@ -1343,7 +1343,7 @@ void handle_io(void)
domains with new allowance */
for (d = dom_head; d; d = d->next) {
- console_iter_void_arg2(d, console_evtchn_unmask, (void
*)now);
+ console_iter_void_arg2(d, console_evtchn_unmask, (void
*)&now);
console_iter_void_arg2(d, maybe_add_console_evtchn_fd,
(void *)&next_timeout);
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 21/27 v11] xen/arm: vpl011: Add support for multiple consoles in xenconsole
On 10/04/2017 10:06 AM, Wei Liu wrote:
> On Wed, Oct 04, 2017 at 10:03:05AM -0400, Boris Ostrovsky wrote:
>>>
>>> @@ -1223,13 +1316,13 @@ void handle_io(void)
>>> /* Re-calculate any event counter allowances & unblock
>>>domains with new allowance */
>>> for (d = dom_head; d; d = d->next) {
>>> - struct console *con = &d->console;
>>>
>>> - console_evtchn_unmask(con, (void *)now);
>>> + console_iter_void_arg2(d, console_evtchn_unmask, (void
>>> *)now);
>> This (together with patch 15's "long long now = (long long)data" cast)
>> generates fatal warning when building for 32-bit.
>>
>> The warning can probably be eliminated by casting to uintptr_t (or some
>> such) but my question is whether 'now' should be a 'long long' --- isn't
>> time_t a 4-byte type on 32-bit? And if it's not then casting will lose
>> the upper word.
>>
> It would be better to just pass the pointer instead.
Or we can do that.
I just noticed Bhupinder's message so I assume he is going to take care
of this.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 21/27 v11] xen/arm: vpl011: Add support for multiple consoles in xenconsole
On Wed, Oct 04, 2017 at 10:28:26AM -0400, Boris Ostrovsky wrote:
> On 10/04/2017 10:06 AM, Wei Liu wrote:
> > On Wed, Oct 04, 2017 at 10:03:05AM -0400, Boris Ostrovsky wrote:
> >>>
> >>> @@ -1223,13 +1316,13 @@ void handle_io(void)
> >>> /* Re-calculate any event counter allowances & unblock
> >>> domains with new allowance */
> >>> for (d = dom_head; d; d = d->next) {
> >>> - struct console *con = &d->console;
> >>>
> >>> - console_evtchn_unmask(con, (void *)now);
> >>> + console_iter_void_arg2(d, console_evtchn_unmask, (void
> >>> *)now);
> >> This (together with patch 15's "long long now = (long long)data" cast)
> >> generates fatal warning when building for 32-bit.
> >>
> >> The warning can probably be eliminated by casting to uintptr_t (or some
> >> such) but my question is whether 'now' should be a 'long long' --- isn't
> >> time_t a 4-byte type on 32-bit? And if it's not then casting will lose
> >> the upper word.
> >>
> > It would be better to just pass the pointer instead.
>
> Or we can do that.
>
> I just noticed Bhupinder's message so I assume he is going to take care
> of this.
I sent a patch just now.
xenconsole: fix 32bit compilation
Feel free to test it.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] stable-rc build: 3 warnings 0 failures (stable-rc/v4.4.89-42-g255b4a0)
On Wed, Oct 04, 2017 at 03:49:30PM +0200, Arnd Bergmann wrote:
> On Wed, Oct 4, 2017 at 10:41 AM, Olof's autobuilder wrote:
> >
> > Warnings:
> >
> > 1 arm/xen/mm.c:202:2: warning: initialization from incompatible
> > pointer type
> > 1 arm/xen/mm.c:202:2: warning: (near initialization for
> > 'xen_swiotlb_dma_ops.mmap')
> > 1 drivers/xen/swiotlb-xen.c:697:27: warning: passing argument 6 of
> > '__generic_dma_ops(dev)->mmap' makes pointer from integer without a cast
>
> This is caused by backporting 2f0b82b1b830 ("swiotlb-xen: implement
> xen_swiotlb_dma_mmap callback") into stable-rc/4.4.
>
> The ->mmap() callback signature changed in linux-4.8 as of commit
> 00085f1efa38 ("dma-mapping: use unsigned long for dma_attrs"), so
> all older kernels are affected.
>
> Please drop the backport patch from 4.4 and 3.18, or apply the trivial fixup
> that I'll send in a bit.
Many thanks for the fixup, I've queued that up now for both trees.
greg k-h
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient
On 10/03/2017 04:21 PM, Sergey Dyasli wrote: > At the moment, the shadow EPTP value is written unconditionally in > ept_handle_violation(). > > Instead, write the value on vmentry to the guest; but only write it if > the value needs updating. > > To detect this, add a flag to the nestedvcpu struct, stale_np2m, to > indicate when such an action is necessary. Set it when the nested p2m > changes or when the np2m is flushed by an IPI, and clear it when we > write the new value. > > Since an IPI invalidating the p2m may happen between > nvmx_switch_guest() and vmx_vmenter, but we can't perform the vmwrite > with interrupts disabled, check the flag just before entering the > guest and restart the vmentry if it's set. > > Signed-off-by: Sergey Dyasli > Signed-off-by: George Dunlap Looks good to me; just one question... > --- > v2 --> v3: > - current pointer is now calculated only once in nvmx_eptp_update() > --- > xen/arch/x86/hvm/nestedhvm.c | 2 ++ > xen/arch/x86/hvm/vmx/entry.S | 6 ++ > xen/arch/x86/hvm/vmx/vmx.c | 14 +++--- > xen/arch/x86/hvm/vmx/vvmx.c| 22 ++ > xen/arch/x86/mm/p2m.c | 10 -- > xen/include/asm-x86/hvm/vcpu.h | 1 + > 6 files changed, 46 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c > index f2f7469d86..74a464d162 100644 > --- a/xen/arch/x86/hvm/nestedhvm.c > +++ b/xen/arch/x86/hvm/nestedhvm.c > @@ -56,6 +56,7 @@ nestedhvm_vcpu_reset(struct vcpu *v) > nv->nv_vvmcxaddr = INVALID_PADDR; > nv->nv_flushp2m = 0; > nv->nv_p2m = NULL; > +nv->stale_np2m = false; > > hvm_asid_flush_vcpu_asid(&nv->nv_n2asid); > > @@ -107,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info) > */ > hvm_asid_flush_core(); > vcpu_nestedhvm(v).nv_p2m = NULL; > +vcpu_nestedhvm(v).stale_np2m = true; > } > > void > diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S > index 53eedc6363..9fb8f89220 100644 > --- a/xen/arch/x86/hvm/vmx/entry.S > +++ b/xen/arch/x86/hvm/vmx/entry.S > @@ -79,6 +79,8 @@ UNLIKELY_END(realmode) > > mov %rsp,%rdi > call vmx_vmenter_helper > +cmp $0,%eax > +jne .Lvmx_vmentry_restart > mov VCPU_hvm_guest_cr2(%rbx),%rax > > pop %r15 > @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry) > GET_CURRENT(bx) > jmp .Lvmx_do_vmentry > > +.Lvmx_vmentry_restart: > +sti > +jmp .Lvmx_do_vmentry > + > .Lvmx_goto_emulator: > sti > mov %rsp,%rdi > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 9cfa9b6965..c9a4111267 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3249,12 +3249,6 @@ static void ept_handle_violation(ept_qual_t q, paddr_t > gpa) > case 0: // Unhandled L1 EPT violation > break; > case 1: // This violation is handled completly > -/*Current nested EPT maybe flushed by other vcpus, so need > - * to re-set its shadow EPTP pointer. > - */ > -if ( nestedhvm_vcpu_in_guestmode(current) && > -nestedhvm_paging_mode_hap(current ) ) > -__vmwrite(EPT_POINTER, get_shadow_eptp(current)); > return; > case -1:// This vioaltion should be injected to L1 VMM > vcpu_nestedhvm(current).nv_vmexit_pending = 1; > @@ -4203,13 +4197,17 @@ static void lbr_fixup(void) > bdw_erratum_bdf14_fixup(); > } > > -void vmx_vmenter_helper(const struct cpu_user_regs *regs) > +int vmx_vmenter_helper(const struct cpu_user_regs *regs) ...Andy, did you want a comment here explaining what the return value is supposed to mean? (And/or changing this to a bool?) -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
