[Xen-devel] [libvirt test] 144580: regressions - FAIL
flight 144580 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/144580/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 144517 build-i386-libvirt6 libvirt-buildfail REGR. vs. 144517 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 144517 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 144517 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a version targeted for testing: libvirt 516b8676850bf7d97fac1f1a2b7bd08654ef5281 baseline version: libvirt d0d728c7c00fd3a62731e50c7bc646df323c0622 Last test of basis 144517 2019-12-04 04:18:55 Z2 days Failing since144526 2019-12-05 04:19:27 Z1 days2 attempts Testing same since 144580 2019-12-06 04:19:32 Z0 days1 attempts People who touched revisions under test: Cole Robinson Daniel P. Berrangé Han Han Michal Privoznik Peter Krempa jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt fail build-arm64-libvirt fail build-armhf-libvirt fail build-i386-libvirt fail build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm blocked test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmblocked test-amd64-amd64-libvirt-xsm blocked test-arm64-arm64-libvirt-xsm blocked test-amd64-i386-libvirt-xsm blocked test-amd64-amd64-libvirt blocked test-arm64-arm64-libvirt blocked test-armhf-armhf-libvirt blocked test-amd64-i386-libvirt blocked test-amd64-amd64-libvirt-pairblocked test-amd64-i386-libvirt-pair blocked test-arm64-arm64-libvirt-qcow2 blocked test-armhf-armhf-libvirt-raw blocked test-amd64-amd64-libvirt-vhd 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 474 lines long.) ___ Xen-devel mailing list
Re: [Xen-devel] bug: unable to LZ4 decompress ub1910 installer kernel when launching domU
On Wed, Dec 04, 2019 at 02:27:26PM +0100, Jan Beulich wrote: > On 04.12.2019 08:14, Jeremi Piotrowski wrote: > > Any suggestions how to proceed? > > Actually here's a better version (I think). > > Jan > > lz4: refine commit 9143a6c55ef7 for the 64-bit case > > I clearly went too far there: While the LZ4_WILDCOPY() instances indeed > need prior guarding, LZ4_SECURECOPY() needs this only in the 32-bit case > (where it simply aliases LZ4_WILDCOPY()). "cpy" can validly point > (slightly) below "op" in these cases, due to > > cpy = op + length - (STEPSIZE - 4); > > where length can be as low as 0 and STEPSIZE is 8. However, instead of > removing the check via "#if !LZ4_ARCH64", refine it such that it would > also properly work in the 64-bit case, aborting decompression instead > of continuing on bogus input. > > Reported-by: Mark Pryor > Reported-by: Jeremi Piotrowski > Signed-off-by: Jan Beulich > > --- unstable.orig/xen/common/lz4/decompress.c > +++ unstable/xen/common/lz4/decompress.c > @@ -147,7 +147,7 @@ static int INIT lz4_uncompress(const uns > goto _output_error; > continue; > } > - if (unlikely((unsigned long)cpy < (unsigned long)op)) > + if (unlikely((unsigned long)cpy < (unsigned long)op - (STEPSIZE > - 4))) > goto _output_error; > LZ4_SECURECOPY(ref, op, cpy); > op = cpy; /* correction */ > @@ -279,7 +279,7 @@ static int lz4_uncompress_unknownoutputs > goto _output_error; > continue; > } > - if (unlikely((unsigned long)cpy < (unsigned long)op)) > + if (unlikely((unsigned long)cpy < (unsigned long)op - (STEPSIZE > - 4))) > goto _output_error; > LZ4_SECURECOPY(ref, op, cpy); > op = cpy; /* correction */ > Thanks Jan, this works. I tested it with direct kernel boot. Like Pry wrote pvgrub2 is another story, seems there is no support for lz4 compressed kernels in it , and ubuntu patches in support to the grub that they ship. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
05.12.2019 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2019 15:36, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> 04.12.2019 17:59, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy writes:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
I get what you mean, but I have plenty of context.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
The parenthesis is not part of the goal.
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
The paragraph talks about two cases: 1. and 2.+3.
>>>
>>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>>> fix achieve 2 and 3 by one action.
>>>
Makes me think we
want two paragraphs, each illustrated with an example.
What about you provide the examples, and then I try to polish the prose?
>>>
>>> 1: error_fatal problem
>>>
>>> Assume the following code flow:
>>>
>>> int f1(errp) {
>>> ...
>>> ret = f2(errp);
>>> if (ret < 0) {
>>> error_append_hint(errp, "very useful hint");
>>> return ret;
>>> }
>>> ...
>>> }
>>>
>>> Now, if we call f1 with _fatal argument and f2 fails, the program
>>> will exit immediately inside f2, when setting the errp. User will not
>>> see the hint.
>>>
>>> So, in this case we should use local_err.
>>
>> How does this example look after the transformation?
>
> Good point.
>
> int f1(errp) {
> ERRP_AUTO_PROPAGATE();
> ...
> ret = f2(errp);
> if (ret < 0) {
> error_append_hint(errp, "very useful hint");
> return ret;
> }
> ...
> }
>
> - nothing changed, only add macro at start. But now errp is safe, if it was
> error_fatal it is wrapped by local error, and will only call exit on automatic
> propagation on f1 finish.
>
>>
>>> 2: error_abort problem
>>>
>>> Now, consider functions without return value. We normally use local_err
>>> variable to catch failures:
>>>
>>> void f1(errp) {
>>> Error *local_err = NULL;
>>> ...
>>> f2(_err);
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> ...
>>> }
>>>
>>> Now, if we call f2 with _abort and f2 fails, the stack in resulting
>>> crash dump will point to error_propagate, not to the failure point in f2,
>>> which complicates debugging.
>>>
>>> So, we should never wrap error_abort by local_err.
>>
>> Likewise.
>
> And here:
>
> void f1(errp) {
> ERRP_AUTO_PROPAGATE();
> ...
> f2(errp);
> if (*errp) {
> return;
> }
> ...
>
> - if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
> local error is automatically propagated to original one.
and if it was error_abort, it is not wrapped, so will crash where failed.
>
>>
>>>
>>> ===
>>>
>>> Our solution:
>>>
>>> - Fixes [1.], adding invocation of new macro into functions with
>>> error_appen_hint/error_prepend,
>>> New macro will wrap error_fatal.
>>> - Fixes [2.], by switching from hand-written local_err to smart macro,
>>> which never
>>> wraps error_abort.
>>> - Handles [3.], by switching to macro, which is less code
>>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
>>> propagations
>>> (in fact, error_propagate is called, but returns immediately on first
>>> if (!local_err))
>>
>
>
--
Best regards,
Vladimir
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
05.12.2019 15:36, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> 04.12.2019 17:59, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy writes:
>>>
Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.
It has three goals:
1. Fix issue with error_fatal & error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]
2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
>>>
>>> I get what you mean, but I have plenty of context.
>>>
(the macro itself doesn't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
>>>
>>> The parenthesis is not part of the goal.
>>>
[Reported by Kevin Wolf]
3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).
To achieve these goals, we need to add invocation of the macro at start
of functions, which needs error_prepend/error_append_hint (1.); add
invocation of the macro at start of functions which do
local_err+error_propagate scenario the check errors, drop local errors
from them and just use *errp instead (2., 3.).
>>>
>>> The paragraph talks about two cases: 1. and 2.+3.
>>
>> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
>> fix achieve 2 and 3 by one action.
>>
>>> Makes me think we
>>> want two paragraphs, each illustrated with an example.
>>>
>>> What about you provide the examples, and then I try to polish the prose?
>>
>> 1: error_fatal problem
>>
>> Assume the following code flow:
>>
>> int f1(errp) {
>> ...
>> ret = f2(errp);
>> if (ret < 0) {
>> error_append_hint(errp, "very useful hint");
>> return ret;
>> }
>> ...
>> }
>>
>> Now, if we call f1 with _fatal argument and f2 fails, the program
>> will exit immediately inside f2, when setting the errp. User will not
>> see the hint.
>>
>> So, in this case we should use local_err.
>
> How does this example look after the transformation?
Good point.
int f1(errp) {
ERRP_AUTO_PROPAGATE();
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.
>
>> 2: error_abort problem
>>
>> Now, consider functions without return value. We normally use local_err
>> variable to catch failures:
>>
>> void f1(errp) {
>> Error *local_err = NULL;
>> ...
>> f2(_err);
>> if (local_err) {
>> error_propagate(errp, local_err);
>> return;
>> }
>> ...
>> }
>>
>> Now, if we call f2 with _abort and f2 fails, the stack in resulting
>> crash dump will point to error_propagate, not to the failure point in f2,
>> which complicates debugging.
>>
>> So, we should never wrap error_abort by local_err.
>
> Likewise.
And here:
void f1(errp) {
ERRP_AUTO_PROPAGATE();
...
f2(errp);
if (*errp) {
return;
}
...
- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
local error is automatically propagated to original one.
>
>>
>> ===
>>
>> Our solution:
>>
>> - Fixes [1.], adding invocation of new macro into functions with
>> error_appen_hint/error_prepend,
>> New macro will wrap error_fatal.
>> - Fixes [2.], by switching from hand-written local_err to smart macro, which
>> never
>> wraps error_abort.
>> - Handles [3.], by switching to macro, which is less code
>> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
>> propagations
>> (in fact, error_propagate is called, but returns immediately on first if
>> (!local_err))
>
--
Best regards,
Vladimir
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
Vladimir Sementsov-Ogievskiy writes:
> 04.12.2019 17:59, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy writes:
>>
>>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>>> functions with errp OUT parameter.
>>>
>>> It has three goals:
>>>
>>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>>> can't see this additional information, because exit() happens in
>>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>>
>>> 2. Fix issue with error_abort & error_propagate: when we wrap
>>> error_abort by local_err+error_propagate, resulting coredump will
>>> refer to error_propagate and not to the place where error happened.
>>
>> I get what you mean, but I have plenty of context.
>>
>>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>>> local_err+error_propagate pattern, which will definitely fix the issue)
>>
>> The parenthesis is not part of the goal.
>>
>>> [Reported by Kevin Wolf]
>>>
>>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>>> void functions with errp parameter, when caller wants to know resulting
>>> status. (Note: actually these functions could be merely updated to
>>> return int error code).
>>>
>>> To achieve these goals, we need to add invocation of the macro at start
>>> of functions, which needs error_prepend/error_append_hint (1.); add
>>> invocation of the macro at start of functions which do
>>> local_err+error_propagate scenario the check errors, drop local errors
>>> from them and just use *errp instead (2., 3.).
>>
>> The paragraph talks about two cases: 1. and 2.+3.
>
> Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
> fix achieve 2 and 3 by one action.
>
>> Makes me think we
>> want two paragraphs, each illustrated with an example.
>>
>> What about you provide the examples, and then I try to polish the prose?
>
> 1: error_fatal problem
>
> Assume the following code flow:
>
> int f1(errp) {
> ...
> ret = f2(errp);
> if (ret < 0) {
>error_append_hint(errp, "very useful hint");
>return ret;
> }
> ...
> }
>
> Now, if we call f1 with _fatal argument and f2 fails, the program
> will exit immediately inside f2, when setting the errp. User will not
> see the hint.
>
> So, in this case we should use local_err.
How does this example look after the transformation?
> 2: error_abort problem
>
> Now, consider functions without return value. We normally use local_err
> variable to catch failures:
>
> void f1(errp) {
> Error *local_err = NULL;
> ...
> f2(_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
> ...
> }
>
> Now, if we call f2 with _abort and f2 fails, the stack in resulting
> crash dump will point to error_propagate, not to the failure point in f2,
> which complicates debugging.
>
> So, we should never wrap error_abort by local_err.
Likewise.
>
> ===
>
> Our solution:
>
> - Fixes [1.], adding invocation of new macro into functions with
> error_appen_hint/error_prepend,
>New macro will wrap error_fatal.
> - Fixes [2.], by switching from hand-written local_err to smart macro, which
> never
>wraps error_abort.
> - Handles [3.], by switching to macro, which is less code
> - Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
> propagations
>(in fact, error_propagate is called, but returns immediately on first if
> (!local_err))
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [libvirt bisection] complete build-arm64-libvirt
branch xen-unstable xenbranch xen-unstable job build-arm64-libvirt testid libvirt-build Tree: libvirt git://libvirt.org/libvirt.git Tree: libvirt_gnulib https://git.savannah.gnu.org/git/gnulib.git/ Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: libvirt git://libvirt.org/libvirt.git Bug introduced: c7f75bf04d07506bd4d9e862b9b38a1e423d88b6 Bug not present: bfe9f25b49827f02027b5a5e88226ce933e1bd7c Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/144581/ commit c7f75bf04d07506bd4d9e862b9b38a1e423d88b6 Author: Daniel P. Berrangé Date: Fri Oct 18 14:18:36 2019 +0100 docs: introduce rst2html as a mandatory tool for building docs The rst2html tool is provided by python docutils, and as the name suggests, it converts RST documents into HTML. Basic rules are added for integrating RST docs into the website build process. This enables us to start writing docs on our website in RST format instead of HTML, without changing the rest of our website templating system away from XSLT yet. Reviewed-by: Michal Privoznik Signed-off-by: Daniel P. Berrangé For bisection revision-tuple graph see: http://logs.test-lab.xenproject.org/osstest/results/bisect/libvirt/build-arm64-libvirt.libvirt-build.html Revision IDs in each graph node refer, respectively, to the Trees above. Running cs-bisection-step --graph-out=/home/logs/results/bisect/libvirt/build-arm64-libvirt.libvirt-build --summary-out=tmp/144581.bisection-summary --basis-template=144517 --blessings=real,real-bisect libvirt build-arm64-libvirt libvirt-build Searching for failure / basis pass: 144526 fail [host=laxton1] / 144517 ok. Failure / basis pass flights: 144526 / 144517 Tree: libvirt git://libvirt.org/libvirt.git Tree: libvirt_gnulib https://git.savannah.gnu.org/git/gnulib.git/ Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git Latest 01bf0bafceb5fc9f12ddee23166ceafed9e951cf 1f6fb368c04919243e2c70f2aa514a5f88e95309 6280c94f306df6a20bbc100ba15a5a81af0366e6 c9416efeef0d4a0554db01f3fd1cdaede14856d7 933ebad2470a169504799a1d95b8e410bd9847ef c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d 05de315b00bf2951617b8ef28811b1f1f2dd5742 Basis pass d0d728c7c00fd3a62731e50c7bc646df323c0622 1f6fb368c04919243e2c70f2aa514a5f88e95309 6280c94f306df6a20bbc100ba15a5a81af0366e6 4d613feee57ebd4680f3c23398a9b33723f29fd6 933ebad2470a169504799a1d95b8e410bd9847ef c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d 42c8cdc039d6dc7d6aea8008bb24622eaf4b7bc8 Generating revisions with ./adhoc-revtuple-generator git://libvirt.org/libvirt.git#d0d728c7c00fd3a62731e50c7bc646df323c0622-01bf0bafceb5fc9f12ddee23166ceafed9e951cf https://git.savannah.gnu.org/git/gnulib.git/#1f6fb368c04919243e2c70f2aa514a5f88e95309-1f6fb368c04919243e2c70f2aa514a5f88e95309 https://gitlab.com/keycodemap/keycodemapdb.git#6280c94f306df6a20bbc100ba15a5a81af0366e6-6280c94f306df6a20bbc100ba15a5a81af0366e6 git://xenbits.xen.org/osstest/ovmf.git#4d613feee57ebd4680f3c23398a9b33723f29fd\ 6-c9416efeef0d4a0554db01f3fd1cdaede14856d7 git://xenbits.xen.org/qemu-xen.git#933ebad2470a169504799a1d95b8e410bd9847ef-933ebad2470a169504799a1d95b8e410bd9847ef git://xenbits.xen.org/osstest/seabios.git#c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d-c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d git://xenbits.xen.org/xen.git#42c8cdc039d6dc7d6aea8008bb24622eaf4b7bc8-05de315b00bf2951617b8ef28811b1f1f2dd5742 Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. error: The last gc run reported the following. Please correct the root cause and remove gc.log. Automatic cleanup will not be performed until the file is removed. warning: There are too many unreachable loose objects; run 'git prune' to remove them. >From git://cache:9419/git://xenbits.xen.org/xen c67210f60d..a260e93db7 stable-4.8 -> origin/stable-4.8 Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. error: The last gc run reported the following. Please correct the root cause and remove gc.log. Automatic cleanup will not be performed until the file is removed. warning: There are too many unreachable loose objects; run 'git prune' to remove them. Loaded 15002 nodes in revision graph Searching for test results: 144517 pass d0d728c7c00fd3a62731e50c7bc646df323c0622 1f6fb368c04919243e2c70f2aa514a5f88e95309
[Xen-devel] [xen-4.8-testing test] 144558: tolerable trouble: fail/pass/starved - PUSHED
flight 144558 xen-4.8-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/144558/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-armhf-armhf-libvirt 8 host-ping-check-xen fail in 144544 pass in 144558 test-armhf-armhf-xl-credit2 16 guest-start/debian.repeat fail pass in 144544 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 15 guest-saverestore.2 fail in 144544 like 138770 test-xtf-amd64-amd64-4 50 xtf/test-hvm64-lbr-tsx-vmentry fail in 144544 like 138829 test-xtf-amd64-amd64-470 xtf/test-hvm64-xsa-278 fail in 144544 like 138829 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 138770 test-xtf-amd64-amd64-1 50 xtf/test-hvm64-lbr-tsx-vmentry fail like 138809 test-xtf-amd64-amd64-3 50 xtf/test-hvm64-lbr-tsx-vmentry fail like 138809 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 138829 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 138829 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 138829 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 138829 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 138829 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 138829 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 138829 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 138829 test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-vhd 12 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 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 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-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-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-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 2 hosts-allocate starved n/a version targeted for testing: xen a260e93db794f560502e89859aaf111d178e80e4 baseline version: xen
[Xen-devel] [xen-unstable test] 144552: tolerable FAIL - PUSHED
flight 144552 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/144552/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail like 144525 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144525 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144525 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144525 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144525 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 144525 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 144525 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144525 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144525 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 144525 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144525 test-amd64-i386-xl-pvshim12 guest-start fail 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 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-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-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-xl-multivcpu 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 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-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 13 migrate-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-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 79cf0989175c16994efc1f152eef07bb48cb98df baseline version: xen ad5c7c162519a3f96561ea4791da1319d9bfdfed Last test of basis 144525 2019-12-05 02:49:37 Z0 days Testing same since 144552 2019-12-05 16:06:15 Z0 days1 attempts People who touched revisions under test: Igor Druzhinin jobs: build-amd64-xsm pass build-arm64-xsm
[Xen-devel] [ovmf test] 144564: all pass - PUSHED
flight 144564 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/144564/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069 baseline version: ovmf 94d4efb54ec4ca894287276ce22d29b6261dbc0b Last test of basis 144527 2019-12-05 06:39:42 Z0 days Testing same since 144564 2019-12-05 20:39:22 Z0 days1 attempts People who touched revisions under test: Sami Mujawar jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-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 : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 94d4efb54e..0f9395d7c5 0f9395d7c5cc6ae2beaa2d87008fe158d04a8069 -> xen-tested-master ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
From: Lars Kurth Date: Thursday, 28 November 2019 at 19:39 To: Rich Persaud Cc: 'Jan Beulich' , "lars.ku...@xenproject.org" , Stefano Stabellini , "xen-...@lists.xenproject.org" , "minios-de...@lists.xenproject.org" , "committ...@xenproject.org" , "mirageos-de...@lists.xenproject.org" , xen-devel , "win-pv-de...@lists.xenproject.org" Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide From: Rich Persaud Date: Thursday, 28 November 2019 at 12:21 To: Lars Kurth Cc: 'Jan Beulich' , "lars.ku...@xenproject.org" , Stefano Stabellini , "xen-...@lists.xenproject.org" , "minios-de...@lists.xenproject.org" , "committ...@xenproject.org" , "mirageos-de...@lists.xenproject.org" , xen-devel , "win-pv-de...@lists.xenproject.org" Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide On Nov 28, 2019, at 09:05, Lars Kurth wrote: On 28/11/2019, 07:37, "Jan Beulich" wrote: On 28.11.2019 14:06, Lars Kurth wrote: I can certainly add something on the timing , along the lines of * For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to * For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved * Don’t repost a series, unless all review comments are addressed or the reviewers asked you to do so. The problem with this is that this is somewhat in conflict with the "let's focus on the core issues and not get distracted by details early on in a review cycle". In other words, this can only work, if reviewers focus on major issues in early reviews only and do not focus on style, coding standards, etc. But this doesn't make much sense either, because then full re-reviews need to happen anyway on later versions, to also deal with the minor issues. For RFC kind of series omitting style and alike feedback certainly makes sense, but as soon as a patch is non-RFC, it should be considered good to go in by the submitter. OK, I think we have a disconnect between ideal and reality. I see two issues today * Key maintainers don't always review RFC series [they end up at the bottom of the priority list, even though spending time on RFCs will save time elsewhere later]. So the effect is that then the contributor assumes there are no major issues and ends it as a proper series * In practice what has happened often in the past is that design, architecture, assumption flaws are found in early versions of a series. - This usually happens because of an oversight or because there was no design discussion prior to the series being posted and agreed - Common sense would dictate that the biggest benefit for both the reviewer, the contributor and the community as a whole would be to try and focus on such flaws and leave everything aside - Of course there may be value in doing a detailed review of parts of such a series as there may be bits that are unaffected by such a flaw - But there will likely be parts which are not: doing a detailed review of such portions wastes everyone's time So coming back to your point. Ideally, it would be nice if we had the capability to call out parts of a series as "problematic" and treating such parts differently. We may be able to reuse some "Shift Left" terminology, including citations of previous Xen code reviews to illustrate categories of design issues that can be shifted left: https://devopedia.org/shift-left I like that idea. We seem to not have come to a conclusion on this specific topic, but maybe for now it is sufficient to call this out as a potential issue in the guide. Before I send out a new version, it would be good to get at least Jan’s view on the issue. Lars I have a draft version of this series ready, but wanted to check how some of it resonates. Also, I do have open questions, where I am looking for input from seasoned reviewers I propose to add the following section to code-review-guide.md ## Problematic Patch Reviews A typical waterfall software development process is sequential with the following steps: define requirements, analyse, design, code, test and deploy. Problems uncovered by code review or testing at such a late stage can cause costly redesign and delays. The principle of **[Shift Left](https://devopedia.org/shift-left)** is to take a task that is traditionally performed at a late stage in the process and perform that task at earlier stages. The goal is to save time by avoiding refactoring. Typically, problematic patch reviews uncover issues such as wrong or missed assumptions, a problematic architecture or design, or other bugs that require significant re-implementation of a patch series to fix the issue. The principle of
[Xen-devel] [PATCH 4/6] xen/hypercall: Cope with -ERESTART on more hypercall paths
These hypercalls each use continue_hypercall_on_cpu(), whose API is about to
switch to use -ERESTART. Update the soon-to-be affected paths to cope,
folding existing contination logic where applicable.
In addition:
* For platform op and sysctl, insert a cpu_relax() into what is otherwise a
tight spinlock loop, and make the continuation logic common at the
epilogue.
* Contrary to the comment in the code, kexec_exec() does return in the
KEXEC_REBOOT case, needs to pass ret back to the caller.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
---
xen/arch/x86/platform_hypercall.c | 14 --
xen/common/compat/domain.c| 9 -
xen/common/domain.c | 8
xen/common/kexec.c| 20
xen/common/sysctl.c | 13 +++--
5 files changed, 47 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/platform_hypercall.c
b/xen/arch/x86/platform_hypercall.c
index b19f6ec4ed..c0c209baac 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -201,9 +201,12 @@ ret_t
do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
* with this vcpu.
*/
while ( !spin_trylock(_lock) )
+{
+cpu_relax();
+
if ( hypercall_preempt_check() )
-return hypercall_create_continuation(
-__HYPERVISOR_platform_op, "h", u_xenpf_op);
+goto create_continuation;
+}
switch ( op->cmd )
{
@@ -816,6 +819,13 @@ ret_t
do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
out:
spin_unlock(_lock);
+if ( ret == -ERESTART )
+{
+create_continuation:
+ret = hypercall_create_continuation(__HYPERVISOR_platform_op,
+"h", u_xenpf_op);
+}
+
return ret;
}
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 11c6afc463..1a14403672 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -79,11 +79,6 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid,
XEN_GUEST_HANDLE_PARAM(void) ar
xfree(ctxt);
}
-
-if ( rc == -ERESTART )
-rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
- cmd, vcpuid, arg);
-
break;
}
@@ -130,6 +125,10 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid,
XEN_GUEST_HANDLE_PARAM(void) ar
break;
}
+if ( rc == -ERESTART )
+rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
+ cmd, vcpuid, arg);
+
return rc;
}
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 865a1cb9d7..ab7e4d09c0 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1422,10 +1422,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid,
XEN_GUEST_HANDLE_PARAM(void) arg)
return -EINVAL;
rc = arch_initialise_vcpu(v, arg);
-if ( rc == -ERESTART )
-rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
- cmd, vcpuid, arg);
-
break;
case VCPUOP_up:
@@ -1598,6 +1594,10 @@ long do_vcpu_op(int cmd, unsigned int vcpuid,
XEN_GUEST_HANDLE_PARAM(void) arg)
break;
}
+if ( rc == -ERESTART )
+rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
+ cmd, vcpuid, arg);
+
return rc;
}
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index a262cc5a18..2fca75cec0 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -842,7 +842,7 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
break;
}
-return -EINVAL; /* never reached */
+return ret;
}
static int kexec_swap_images(int type, struct kexec_image *new,
@@ -1220,7 +1220,7 @@ static int do_kexec_op_internal(unsigned long op,
return ret;
if ( test_and_set_bit(KEXEC_FLAG_IN_HYPERCALL, _flags) )
-return hypercall_create_continuation(__HYPERVISOR_kexec_op, "lh", op,
uarg);
+return -ERESTART;
switch ( op )
{
@@ -1263,13 +1263,25 @@ static int do_kexec_op_internal(unsigned long op,
long do_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
{
-return do_kexec_op_internal(op, uarg, 0);
+int ret = do_kexec_op_internal(op, uarg, 0);
+
+if ( ret == -ERESTART )
+ret = hypercall_create_continuation(__HYPERVISOR_kexec_op,
+"lh", op, uarg);
+
+return ret;
}
#ifdef CONFIG_COMPAT
int compat_kexec_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) uarg)
{
-return do_kexec_op_internal(op, uarg, 1);
+int ret = do_kexec_op_internal(op, uarg, 1);
+
+if ( ret == -ERESTART )
+ret
[Xen-devel] [PATCH 2/6] xen/tasklet: Switch data parameter from unsigned long to void *.
Most users pass a vcpu pointer, and only stopmachine_action() takes an integer
parameter. Switch to using void * to substantially reduce the number of
explicit casts.
No functional change.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
---
xen/arch/x86/hvm/hvm.c| 6 ++
xen/arch/x86/hvm/vlapic.c | 8 +++-
xen/arch/x86/mm/shadow/common.c | 4 ++--
xen/common/domain.c | 15 ++-
xen/common/keyhandler.c | 19 +--
xen/common/stop_machine.c | 5 +++--
xen/common/tasklet.c | 6 ++
xen/common/trace.c| 4 ++--
xen/drivers/char/console.c| 4 ++--
xen/drivers/passthrough/amd/iommu_guest.c | 7 +++
xen/drivers/passthrough/amd/iommu_init.c | 6 +++---
xen/drivers/passthrough/iommu.c | 4 ++--
xen/drivers/passthrough/vtd/iommu.c | 4 ++--
xen/include/asm-x86/shadow.h | 5 ++---
xen/include/xen/tasklet.h | 10 --
15 files changed, 47 insertions(+), 60 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 47573f71b8..d909fec30d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1525,10 +1525,8 @@ int hvm_vcpu_initialise(struct vcpu *v)
if ( (rc = hvm_funcs.vcpu_initialise(v)) != 0 ) /* teardown:
hvm_funcs.vcpu_destroy */
goto fail3;
-softirq_tasklet_init(
->arch.hvm.assert_evtchn_irq_tasklet,
-(void(*)(unsigned long))hvm_assert_evtchn_irq,
-(unsigned long)v);
+softirq_tasklet_init(>arch.hvm.assert_evtchn_irq_tasklet,
+ (void (*)(void *))hvm_assert_evtchn_irq, v);
v->arch.hvm.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 9b8afb72e8..06235f183e 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -309,9 +309,9 @@ static void vlapic_init_sipi_one(struct vcpu *target,
uint32_t icr)
vcpu_unpause(target);
}
-static void vlapic_init_sipi_action(unsigned long _vcpu)
+static void vlapic_init_sipi_action(void *data)
{
-struct vcpu *origin = (struct vcpu *)_vcpu;
+struct vcpu *origin = data;
uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
uint32_t short_hand = icr & APIC_SHORT_MASK;
@@ -1637,9 +1637,7 @@ int vlapic_init(struct vcpu *v)
spin_lock_init(>esr_lock);
-tasklet_init(>init_sipi.tasklet,
- vlapic_init_sipi_action,
- (unsigned long)v);
+tasklet_init(>init_sipi.tasklet, vlapic_init_sipi_action, v);
if ( v->vcpu_id == 0 )
register_mmio_handler(v->domain, _mmio_ops);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 314d837602..6212ec2c4a 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3471,9 +3471,9 @@ void shadow_audit_tables(struct vcpu *v)
#ifdef CONFIG_PV
-void pv_l1tf_tasklet(unsigned long data)
+void pv_l1tf_tasklet(void *data)
{
-struct domain *d = (void *)data;
+struct domain *d = data;
domain_pause(d);
paging_lock(d);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ccf689fcbe..865a1cb9d7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -150,7 +150,7 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int
vcpu_id)
spin_lock_init(>virq_lock);
-tasklet_init(>continue_hypercall_tasklet, NULL, 0);
+tasklet_init(>continue_hypercall_tasklet, NULL, NULL);
grant_table_init_vcpu(v);
@@ -1661,9 +1661,9 @@ struct migrate_info {
static DEFINE_PER_CPU(struct migrate_info *, continue_info);
-static void continue_hypercall_tasklet_handler(unsigned long _info)
+static void continue_hypercall_tasklet_handler(void *data)
{
-struct migrate_info *info = (struct migrate_info *)_info;
+struct migrate_info *info = data;
struct vcpu *v = info->vcpu;
long res = -EINVAL;
@@ -1707,12 +1707,9 @@ int continue_hypercall_on_cpu(
info->vcpu = curr;
info->nest = 0;
-tasklet_kill(
->continue_hypercall_tasklet);
-tasklet_init(
->continue_hypercall_tasklet,
-continue_hypercall_tasklet_handler,
-(unsigned long)info);
+tasklet_kill(>continue_hypercall_tasklet);
+tasklet_init(>continue_hypercall_tasklet,
+ continue_hypercall_tasklet_handler, info);
get_knownalive_domain(curr->domain);
vcpu_pause_nosync(curr);
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index dc6396b225..f50490d0f3 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -71,12 +71,12 @@ static struct
[Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
More paths are going start using hypercall continuations. We could add extra
calls to hypercall_create_continuation() but it is much easier to handle
-ERESTART once at the top level.
One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
compatibility in a security fix, turn a DOMCTL continuation into
__HYPERVISOR_arch_1. This remains as it was, gaining a comment explaining
what is going on.
With -ERESTART handling in place, the !domctl_lock_acquire() path can use the
normal exit path, instead of opencoding a subset of it locally.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
---
xen/arch/x86/domctl.c | 5 -
xen/arch/x86/mm/hap/hap.c | 3 +--
xen/arch/x86/mm/shadow/common.c | 3 +--
xen/common/domctl.c | 19 +--
4 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b461aadbd6..2fa0e7dda5 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -326,9 +326,12 @@ long arch_do_domctl(
switch ( domctl->cmd )
{
-
case XEN_DOMCTL_shadow_op:
ret = paging_domctl(d, >u.shadow_op, u_domctl, 0);
+/*
+ * Continuations from paging_domctl() switch index to arch_1, and
+ * can't use the common domctl continuation path.
+ */
if ( ret == -ERESTART )
return hypercall_create_continuation(__HYPERVISOR_arch_1,
"h", u_domctl);
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..3996e17b7e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -600,8 +600,7 @@ int hap_domctl(struct domain *d, struct
xen_domctl_shadow_op *sc,
paging_unlock(d);
if ( preempted )
/* Not finished. Set up to re-run the call. */
-rc = hypercall_create_continuation(__HYPERVISOR_domctl, "h",
- u_domctl);
+rc = -ERESTART;
else
/* Finished. Return the new allocation */
sc->mb = hap_get_allocation(d);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 6212ec2c4a..17ca21104f 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3400,8 +3400,7 @@ int shadow_domctl(struct domain *d,
paging_unlock(d);
if ( preempted )
/* Not finished. Set up to re-run the call. */
-rc = hypercall_create_continuation(
-__HYPERVISOR_domctl, "h", u_domctl);
+rc = -ERESTART;
else
/* Finished. Return the new allocation */
sc->mb = shadow_get_allocation(d);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 03d0226039..cb0295085d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -415,10 +415,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
if ( !domctl_lock_acquire() )
{
-if ( d && d != dom_io )
-rcu_unlock_domain(d);
-return hypercall_create_continuation(
-__HYPERVISOR_domctl, "h", u_domctl);
+ret = -ERESTART;
+goto domctl_out_unlock_domonly;
}
switch ( op->cmd )
@@ -438,9 +436,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
if ( guest_handle_is_null(op->u.vcpucontext.ctxt) )
{
ret = vcpu_reset(v);
-if ( ret == -ERESTART )
-ret = hypercall_create_continuation(
- __HYPERVISOR_domctl, "h", u_domctl);
break;
}
@@ -469,10 +464,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
domain_pause(d);
ret = arch_set_info_guest(v, c);
domain_unpause(d);
-
-if ( ret == -ERESTART )
-ret = hypercall_create_continuation(
- __HYPERVISOR_domctl, "h", u_domctl);
}
free_vcpu_guest_context(c.nat);
@@ -585,9 +576,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
domain_lock(d);
ret = domain_kill(d);
domain_unlock(d);
-if ( ret == -ERESTART )
-ret = hypercall_create_continuation(
-__HYPERVISOR_domctl, "h", u_domctl);
goto domctl_out_unlock_domonly;
case XEN_DOMCTL_setnodeaffinity:
@@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
if ( copyback && __copy_to_guest(u_domctl, op, 1) )
ret = -EFAULT;
+if ( ret == -ERESTART )
+ret = hypercall_create_continuation(__HYPERVISOR_domctl,
+"h", u_domctl);
return ret;
}
--
2.11.0
___
Xen-devel mailing list
[Xen-devel] [PATCH 5/6] xen/tasklet: Return -ERESTART from continue_hypercall_on_cpu()
Some hypercalls tasklets want to create a continuation, rather than fail the
hypercall with a hard error. By the time the tasklet is executing, it is too
late to create the continuation, and even continue_hypercall_on_cpu() doesn't
have enough state to do it correctly.
All callers of continue_hypercall_on_cpu() have been updated to turn -ERESTART
into a continuation, where appropriate modifications can be made to register
and/or memory parameters.
This changes the continue_hypercall_on_cpu() behaviour to unconditionally
create a hypercall continuation, in case the tasklet wants to use it, and then
to have arch_hypercall_tasklet_result() cancel the continuation when a result
is available. None of these hypercalls are fast paths.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
There is one RFC point. The statement in the header file of "If this function
returns 0 then the function is guaranteed to run at some point in the future."
was never true. In the case of a CPU miss, the hypercall would be blindly
failed with -EINVAL.
The current behaviour with this patch is to not cancel the continuation, which
I think is less bad, but still not great. Thoughts?
---
xen/arch/arm/traps.c | 1 +
xen/arch/x86/hypercall.c | 7 +++
xen/common/domain.c | 9 +
xen/include/xen/domain.h | 11 ---
4 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a20474f87c..5d35d2b7e9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1489,6 +1489,7 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long
res)
{
struct cpu_user_regs *regs = >arch.cpu_info->guest_cpu_user_regs;
+regs->pc += 4; /* Skip over 'hvc #XEN_HYPERCALL_TAG' */
HYPERCALL_RESULT_REG(regs) = res;
}
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 7f299d45c6..42d95f9b9a 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -170,6 +170,13 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long
res)
{
struct cpu_user_regs *regs = >arch.user_regs;
+/*
+ * PV hypercalls are all 2-byte instructions (INT $0x82, SYSCALL). HVM
+ * hypercalls are all 3-byte instructions (VMCALL, VMMCALL).
+ *
+ * Move %rip forwards to complete the continuation.
+ */
+regs->rip += 2 + is_hvm_vcpu(v);
regs->rax = res;
}
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ab7e4d09c0..eb69db3078 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1665,7 +1665,7 @@ static void continue_hypercall_tasklet_handler(void *data)
{
struct migrate_info *info = data;
struct vcpu *v = info->vcpu;
-long res = -EINVAL;
+long res = -ERESTART;
/* Wait for vcpu to sleep so that we can access its register state. */
vcpu_sleep_sync(v);
@@ -1675,7 +1675,8 @@ static void continue_hypercall_tasklet_handler(void *data)
if ( likely(info->cpu == smp_processor_id()) )
res = info->func(info->data);
-arch_hypercall_tasklet_result(v, res);
+if ( res != -ERESTART )
+arch_hypercall_tasklet_result(v, res);
this_cpu(continue_info) = NULL;
@@ -1726,8 +1727,8 @@ int continue_hypercall_on_cpu(
tasklet_schedule_on_cpu(>vcpu->continue_hypercall_tasklet, cpu);
-/* Dummy return value will be overwritten by tasklet. */
-return 0;
+/* Start a continuation. Value will be overwritten by the tasklet. */
+return -ERESTART;
}
/*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1cb205d977..83c737bca4 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -96,9 +96,11 @@ void domctl_lock_release(void);
/*
* Continue the current hypercall via func(data) on specified cpu.
- * If this function returns 0 then the function is guaranteed to run at some
- * point in the future. If this function returns an error code then the
- * function has not been and will not be executed.
+ *
+ * This function returns -ERESTART in the success case, and a higher level
+ * caller is required to set up a hypercall continuation. func() will be run
+ * at some point in the future. If this function returns any other error code
+ * then func() has not, and will not be executed.
*/
int continue_hypercall_on_cpu(
unsigned int cpu, long (*func)(void *data), void *data);
@@ -106,6 +108,9 @@ int continue_hypercall_on_cpu(
/*
* Companion to continue_hypercall_on_cpu(), to feed func()'s result back into
* vcpu regsiter state.
+ *
+ * Must undo the effects of the hypercall continuation created by
+ * continue_hypercall_on_cpu()'s caller.
*/
void arch_hypercall_tasklet_result(struct vcpu *v, long res);
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 6/6] x86/smt: Don't use -EBUSY for smt_up_down_helper() continuations
Tasklet context is now capable of using handling continuations. Use this
rather than -EBUSY as it is a more efficient way to restart the hypercall.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
---
xen/arch/x86/sysctl.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 4a76f0f47f..06955fdc3e 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -85,6 +85,9 @@ long cpu_up_helper(void *data)
/* On EBUSY, flush RCU work and have one more go. */
rcu_barrier();
ret = cpu_up(cpu);
+
+if ( ret == -EBUSY )
+ret = -ERESTART;
}
if ( !ret && !opt_smt &&
@@ -110,6 +113,9 @@ long cpu_down_helper(void *data)
/* On EBUSY, flush RCU work and have one more go. */
rcu_barrier();
ret = cpu_down(cpu);
+
+if ( ret == -EBUSY )
+ret = -ERESTART;
}
return ret;
}
@@ -143,8 +149,7 @@ static long smt_up_down_helper(void *data)
*/
if ( ret != -EEXIST && general_preempt_check() )
{
-/* In tasklet context - can't create a contination. */
-ret = -EBUSY;
+ret = -ERESTART;
break;
}
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/6] xen: Support continuations from tasklets
This is the Post "x86/smt: Support for enabling/disabling SMT at runtime" work which stumbled into discovering XSA-296, for the purpose of making continuations more efficient. From testing this series, I have re-confirmed the previous reported observation that: # while :; do xen-hptool smt-enable; xen-hptool smt-disable; done in dom0 eventually causes the serial console to cease working (wedge midway through printing a line). There are sporadic "Broke affinity for IRQ26, new: " messages, but the serial always seems to break shortly after the first "Broke affinity for IRQ30, new: ". Both IRQs are non-descript PCI-MSI/-X interrupts bound to dom0. Andrew Cooper (6): xen/tasklet: Fix return value truncation on arm64 xen/tasklet: Switch data parameter from unsigned long to void *. xen/domctl: Consolidate hypercall continuation handling at the top level xen/hypercall: Cope with -ERESTART on more hypercall paths xen/tasklet: Return -ERESTART from continue_hypercall_on_cpu() x86/smt: Don't use -EBUSY for smt_up_down_helper() continuations xen/arch/arm/traps.c | 8 +++ xen/arch/x86/domctl.c | 5 - xen/arch/x86/hvm/hvm.c| 6 ++--- xen/arch/x86/hvm/vlapic.c | 8 +++ xen/arch/x86/hypercall.c | 14 xen/arch/x86/mm/hap/hap.c | 3 +-- xen/arch/x86/mm/shadow/common.c | 7 +++--- xen/arch/x86/platform_hypercall.c | 14 ++-- xen/arch/x86/sysctl.c | 9 ++-- xen/common/compat/domain.c| 9 xen/common/domain.c | 37 +-- xen/common/domctl.c | 19 +--- xen/common/kexec.c| 20 + xen/common/keyhandler.c | 19 xen/common/stop_machine.c | 5 +++-- xen/common/sysctl.c | 13 +-- xen/common/tasklet.c | 6 ++--- xen/common/trace.c| 4 ++-- xen/drivers/char/console.c| 4 ++-- xen/drivers/passthrough/amd/iommu_guest.c | 7 +++--- xen/drivers/passthrough/amd/iommu_init.c | 6 ++--- xen/drivers/passthrough/iommu.c | 4 ++-- xen/drivers/passthrough/vtd/iommu.c | 4 ++-- xen/include/asm-arm/regs.h| 2 -- xen/include/asm-x86/regs.h| 2 -- xen/include/asm-x86/shadow.h | 5 ++--- xen/include/xen/domain.h | 17 +++--- xen/include/xen/tasklet.h | 10 - 28 files changed, 158 insertions(+), 109 deletions(-) -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/6] xen/tasklet: Fix return value truncation on arm64
The use of return_reg() assumes ARM's 32bit ABI. Therefore, a failure such as
-EINVAL will appear as a large positive number near 4 billion to a 64bit ARM
guest which happens to use continue_hypercall_on_cpu().
Introduce a new arch_hypercall_tasklet_result() hook which is implemented by
both architectures, and drop the return_reg() macros. This logic will be
extended in a later change to make continuations out of the tasklet work.
Signed-off-by: Andrew Cooper
Acked-by: Jan Beulich
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
This was posted on its own previously, but is reset back to v1 now it is in
its series. This can't be made static inline due to header constraints, but
there is no inherent issue with doing so if the headers become less tangled.
Failing the call with -EINVAL for missing the correct CPU is very rude, and
addressed in a later patch.
---
xen/arch/arm/traps.c | 7 +++
xen/arch/x86/hypercall.c | 7 +++
xen/common/domain.c| 9 +++--
xen/include/asm-arm/regs.h | 2 --
xen/include/asm-x86/regs.h | 2 --
xen/include/xen/domain.h | 6 ++
6 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index d028ec9224..a20474f87c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1485,6 +1485,13 @@ static void do_trap_hypercall(struct cpu_user_regs
*regs, register_t *nr,
regs->pc -= 4; /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
}
+void arch_hypercall_tasklet_result(struct vcpu *v, long res)
+{
+struct cpu_user_regs *regs = >arch.cpu_info->guest_cpu_user_regs;
+
+HYPERCALL_RESULT_REG(regs) = res;
+}
+
static bool check_multicall_32bit_clean(struct multicall_entry *multi)
{
int i;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 1d42702c6a..7f299d45c6 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -166,6 +166,13 @@ unsigned long hypercall_create_continuation(
#undef NEXT_ARG
+void arch_hypercall_tasklet_result(struct vcpu *v, long res)
+{
+struct cpu_user_regs *regs = >arch.user_regs;
+
+regs->rax = res;
+}
+
int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
unsigned int mask, ...)
{
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 66c7fc..ccf689fcbe 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1665,13 +1665,18 @@ static void continue_hypercall_tasklet_handler(unsigned
long _info)
{
struct migrate_info *info = (struct migrate_info *)_info;
struct vcpu *v = info->vcpu;
+long res = -EINVAL;
/* Wait for vcpu to sleep so that we can access its register state. */
vcpu_sleep_sync(v);
this_cpu(continue_info) = info;
-return_reg(v) = (info->cpu == smp_processor_id())
-? info->func(info->data) : -EINVAL;
+
+if ( likely(info->cpu == smp_processor_id()) )
+res = info->func(info->data);
+
+arch_hypercall_tasklet_result(v, res);
+
this_cpu(continue_info) = NULL;
if ( info->nest-- == 0 )
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 0e3e56b452..ec091a28a2 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -57,8 +57,6 @@ static inline bool guest_mode(const struct cpu_user_regs *r)
return (diff == 0);
}
-#define return_reg(v) ((v)->arch.cpu_info->guest_cpu_user_regs.r0)
-
register_t get_user_reg(struct cpu_user_regs *regs, int reg);
void set_user_reg(struct cpu_user_regs *regs, int reg, register_t val);
diff --git a/xen/include/asm-x86/regs.h b/xen/include/asm-x86/regs.h
index 725a664e0a..dc00b854e3 100644
--- a/xen/include/asm-x86/regs.h
+++ b/xen/include/asm-x86/regs.h
@@ -15,6 +15,4 @@
(diff == 0); \
})
-#define return_reg(v) ((v)->arch.user_regs.rax)
-
#endif /* __X86_REGS_H__ */
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 769302057b..1cb205d977 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -103,6 +103,12 @@ void domctl_lock_release(void);
int continue_hypercall_on_cpu(
unsigned int cpu, long (*func)(void *data), void *data);
+/*
+ * Companion to continue_hypercall_on_cpu(), to feed func()'s result back into
+ * vcpu regsiter state.
+ */
+void arch_hypercall_tasklet_result(struct vcpu *v, long res);
+
extern unsigned int xen_processor_pmbits;
extern bool_t opt_dom0_vcpus_pin;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 144562: tolerable all pass - PUSHED
flight 144562 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/144562/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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 version targeted for testing: xen 131c89ce1e1dfd0b57a249615a92de4f120d9100 baseline version: xen 79cf0989175c16994efc1f152eef07bb48cb98df Last test of basis 144547 2019-12-05 13:01:23 Z0 days Testing same since 144562 2019-12-05 19:01:07 Z0 days1 attempts People who touched revisions under test: Andrew Cooper jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt 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 : To xenbits.xen.org:/home/xen/git/xen.git 79cf098917..131c89ce1e 131c89ce1e1dfd0b57a249615a92de4f120d9100 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Community Call: Minutes for call on Thursday Dec 5, 16:00 - 17:00 UTC
Hi all, the minutes are at https://cryptpad.fr/pad/#/2/pad/view/T-vK-ZiXDrnpve64l4hvP+evA5najcmoxOOVJ8TGeBs/ and attached There were a few items, where the connection was bad and where I am missing things ACTIONS and everything important are marked in red The next meeting is on Jan 9, not Jan 2 Regards Lars 2019-12 Community Call.pdf Description: 2019-12 Community Call.pdf ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [libvirt bisection] complete build-i386-libvirt
branch xen-unstable xenbranch xen-unstable job build-i386-libvirt testid libvirt-build Tree: libvirt git://libvirt.org/libvirt.git Tree: libvirt_gnulib https://git.savannah.gnu.org/git/gnulib.git/ Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: libvirt git://libvirt.org/libvirt.git Bug introduced: c7f75bf04d07506bd4d9e862b9b38a1e423d88b6 Bug not present: bfe9f25b49827f02027b5a5e88226ce933e1bd7c Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/144563/ commit c7f75bf04d07506bd4d9e862b9b38a1e423d88b6 Author: Daniel P. Berrangé Date: Fri Oct 18 14:18:36 2019 +0100 docs: introduce rst2html as a mandatory tool for building docs The rst2html tool is provided by python docutils, and as the name suggests, it converts RST documents into HTML. Basic rules are added for integrating RST docs into the website build process. This enables us to start writing docs on our website in RST format instead of HTML, without changing the rest of our website templating system away from XSLT yet. Reviewed-by: Michal Privoznik Signed-off-by: Daniel P. Berrangé For bisection revision-tuple graph see: http://logs.test-lab.xenproject.org/osstest/results/bisect/libvirt/build-i386-libvirt.libvirt-build.html Revision IDs in each graph node refer, respectively, to the Trees above. Running cs-bisection-step --graph-out=/home/logs/results/bisect/libvirt/build-i386-libvirt.libvirt-build --summary-out=tmp/144563.bisection-summary --basis-template=144517 --blessings=real,real-bisect libvirt build-i386-libvirt libvirt-build Searching for failure / basis pass: 144526 fail [host=baroque0] / 144517 [host=huxelrebe1] 144501 [host=italia0] 144408 [host=elbling0] 144368 [host=huxelrebe1] 144345 [host=huxelrebe0] 144318 [host=huxelrebe1] 144304 ok. Failure / basis pass flights: 144526 / 144304 (tree with no url: minios) Tree: libvirt git://libvirt.org/libvirt.git Tree: libvirt_gnulib https://git.savannah.gnu.org/git/gnulib.git/ Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git Latest 01bf0bafceb5fc9f12ddee23166ceafed9e951cf 1f6fb368c04919243e2c70f2aa514a5f88e95309 6280c94f306df6a20bbc100ba15a5a81af0366e6 c9416efeef0d4a0554db01f3fd1cdaede14856d7 d0d8ad39ecb51cd7497cd524484fe09f50876798 933ebad2470a169504799a1d95b8e410bd9847ef c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d 05de315b00bf2951617b8ef28811b1f1f2dd5742 Basis pass 9d6920bd7de3f92be1894790adeb689060ab25eb 1f6fb368c04919243e2c70f2aa514a5f88e95309 6280c94f306df6a20bbc100ba15a5a81af0366e6 bd85bf54c268204c7a698a96f3ccd96cd77952cd d0d8ad39ecb51cd7497cd524484fe09f50876798 933ebad2470a169504799a1d95b8e410bd9847ef c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d 183f354e1430087879de071f0c7122e42703916e Generating revisions with ./adhoc-revtuple-generator git://libvirt.org/libvirt.git#9d6920bd7de3f92be1894790adeb689060ab25eb-01bf0bafceb5fc9f12ddee23166ceafed9e951cf https://git.savannah.gnu.org/git/gnulib.git/#1f6fb368c04919243e2c70f2aa514a5f88e95309-1f6fb368c04919243e2c70f2aa514a5f88e95309 https://gitlab.com/keycodemap/keycodemapdb.git#6280c94f306df6a20bbc100ba15a5a81af0366e6-6280c94f306df6a20bbc100ba15a5a81af0366e6 git://xenbits.xen.org/osstest/ovmf.git#bd85bf54c268204c7a698a96f3ccd96cd77952c\ d-c9416efeef0d4a0554db01f3fd1cdaede14856d7 git://xenbits.xen.org/qemu-xen-traditional.git#d0d8ad39ecb51cd7497cd524484fe09f50876798-d0d8ad39ecb51cd7497cd524484fe09f50876798 git://xenbits.xen.org/qemu-xen.git#933ebad2470a169504799a1d95b8e410bd9847ef-933ebad2470a169504799a1d95b8e410bd9847ef git://xenbits.xen.org/osstest/seabios.git#c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d-c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d git://xenbits.xen.org/xen.git#183f354e1430087879de071f0c7122e42703916e-05de315b00bf2951\ 617b8ef28811b1f1f2dd5742 Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. error: The last gc run reported the following. Please correct the root cause and remove gc.log. Automatic cleanup will not be performed until the file is removed. warning: There are too many unreachable loose objects; run 'git prune' to remove them. >From git://cache:9419/git://xenbits.xen.org/xen 8d2a688015..8d1ee9f2c4 stable-4.9 -> origin/stable-4.9 Auto packing the
[Xen-devel] [xen-4.9-testing test] 144545: tolerable trouble: fail/pass/starved - PUSHED
flight 144545 xen-4.9-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/144545/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 138748 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 138919 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 138919 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-localmigrate/x10 fail like 138951 test-amd64-amd64-xl-qemuu-ws16-amd64 16 guest-localmigrate/x10 fail like 139019 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 139019 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 139047 test-amd64-i386-xl-qemut-win7-amd64 16 guest-localmigrate/x10 fail like 139047 test-amd64-i386-xl-qemut-ws16-amd64 16 guest-localmigrate/x10 fail like 139047 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-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-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-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail 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-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-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-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-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-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 2 hosts-allocate starved n/a version targeted for testing: xen 8d1ee9f2c473fec54b5018c01ad556d7afd62c17 baseline version: xen 8d2a688015193e20ae47fe3c208f99128240f821 Last test of basis 139047 2019-07-16 10:23:42 Z 142 days Failing since143735 2019-11-04 14:46:46 Z 31 days8 attempts Testing same since 144545 2019-12-05 12:05:32 Z0 days1 attempts People who touched revisions under test: Andrew Cooper George Dunlap Ian Jackson Jan Beulich Julien Grall Paul Durrant jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm
Re: [Xen-devel] [PATCH v2] xen/arm: remove physical timer offset
Hi,
On 05/12/2019 19:17, Jeff Kubascik wrote:
On 12/3/2019 1:04 PM, Julien Grall wrote:
Hi,
On 26/11/2019 21:13, Jeff Kubascik wrote:
The physical timer traps apply an offset so that time starts at 0 for
the guest. However, this offset is not currently applied to the physical
counter. Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section
D11.2.4 Timers, the "Offset" between the counter and timer should be
zero for a physical timer. This removes the offset to make the timer and
counter consistent.
Furthermore, section D11.2.4 specifies that the values in the TimerValue
view of the timers are signed in standard two's complement form. When
writing to the TimerValue register, it should be signed extended as
described by the equation
CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
I am a bit confused, is it a new bug introduced by the change or
previously existing? If the latter, then I think this should be modified
in a separate patch.
This would be a previously existing bug - a quirk in the timer design that
wasn't emulated correctly before. I can break this out into a separate patch.
It would be great if you can split it. Thank you!
[...]
@@ -185,7 +184,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs,
uint32_t *r, bool read)
if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
{
set_timer(>arch.phys_timer.timer,
- v->arch.phys_timer.cval +
v->domain->arch.phys_timer_base.offset);
+ ticks_to_ns(v->arch.phys_timer.cval - boot_count));
cval may be smaller than boot_count. In that case, we will set the timer
to expire a very long time. This is not the expected behavior from the
guest.
Instead, we should either use 0 to create the timer or call
phys_timer_expired directly.
I disagree - if you set cval to a value smaller than boot_count, you are setting
cval to a value less than the physical counter value. This would result in the
timer having a long expiration time.
boot_count refers to when Xen began to boot, not the start of the
physical counter. If you look at the condition to fire the timer (see
below), then it means the timer will fire right now because the physical
counter is past CompareValue (cval).
TimerConditionMet = (((Counter[63:0] – Offset[63:0])[63:0] -
CompareValue[63:0]) >= 0)
We only subtract boot_count here as the timer subsystem expects a
relative number of nanoseconds from when Xen booted.
However, set_timer expects a signed 64 bit value in ns. The conversion of cval
(unsigned 64 bit) from ticks to ns is going to overflow this. I'm not sure what
would be the best way to work around this limitation. At the very least, I think
we should print a warning message.
A warning message in emulation is definitely not the right solution. If
a user asks something that is valid from the spec PoV then we should
implement it correctly. The more that I don't think boot_count store
what you expect (see above).
But we definitely can't allow the caller of ticks_to_ns() to pass a
negative value as argument because (cval - boot_count) may be over 2^63
for instance if the user requests a timer to be set in a million of year
(I didn't do the math!).
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: remove physical timer offset
On 12/3/2019 1:04 PM, Julien Grall wrote:
> Hi,
>
> On 26/11/2019 21:13, Jeff Kubascik wrote:
>> The physical timer traps apply an offset so that time starts at 0 for
>> the guest. However, this offset is not currently applied to the physical
>> counter. Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section
>> D11.2.4 Timers, the "Offset" between the counter and timer should be
>> zero for a physical timer. This removes the offset to make the timer and
>> counter consistent.
>>
>> Furthermore, section D11.2.4 specifies that the values in the TimerValue
>> view of the timers are signed in standard two's complement form. When
>> writing to the TimerValue register, it should be signed extended as
>> described by the equation
>>
>>CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
>
> I am a bit confused, is it a new bug introduced by the change or
> previously existing? If the latter, then I think this should be modified
> in a separate patch.
This would be a previously existing bug - a quirk in the timer design that
wasn't emulated correctly before. I can break this out into a separate patch.
>>
>> Signed-off-by: Jeff Kubascik
>> ---
>> Changes in v2:
>> - Update commit message to specify reference manual version and section
>> - Change physical timer cval to hold hardware value
>
> I think this change should be explained in the commit message.
I will update the commit message accordingly.
>> - Make sure to sign extend TimerValue on writes. This was done by first
>>casting the r pointer to (int32_t *), dereferencing it, then casting
>>to uint64_t. Please let me know if there is a more correct way to do
>>this
>> ---
>> xen/arch/arm/vtimer.c| 21 +
>> xen/include/asm-arm/domain.h | 3 ---
>> 2 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index e6aebdac9e..eb12a08acf 100644
>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -62,7 +62,6 @@ static void virt_timer_expired(void *data)
>>
>> int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig
>> *config)
>> {
>> -d->arch.phys_timer_base.offset = NOW();
>> d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>> d->time_offset_seconds = ticks_to_ns(d->arch.virt_timer_base.offset -
>> boot_count);
>> do_div(d->time_offset_seconds, 10);
>
> I think you need to update the initialization of cval to avoid storing
> ns. But CTNP_CVAL_EL0 is reset to a unknown value at reboot, so we
> should not need to set a value at all as the guest would have to set it.
Good catch, I missed this. I will remove the "t->cval = NOW();" line in
vcpu_vtimer_init.
>> @@ -185,7 +184,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs,
>> uint32_t *r, bool read)
>> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>> {
>> set_timer(>arch.phys_timer.timer,
>> - v->arch.phys_timer.cval +
>> v->domain->arch.phys_timer_base.offset);
>> + ticks_to_ns(v->arch.phys_timer.cval - boot_count));
>
> cval may be smaller than boot_count. In that case, we will set the timer
> to expire a very long time. This is not the expected behavior from the
> guest.
>
> Instead, we should either use 0 to create the timer or call
> phys_timer_expired directly.
I disagree - if you set cval to a value smaller than boot_count, you are setting
cval to a value less than the physical counter value. This would result in the
timer having a long expiration time.
However, set_timer expects a signed 64 bit value in ns. The conversion of cval
(unsigned 64 bit) from ticks to ns is going to overflow this. I'm not sure what
would be the best way to work around this limitation. At the very least, I think
we should print a warning message.
>> }
>> else
>> stop_timer(>arch.phys_timer.timer);
>> @@ -197,26 +196,25 @@ static bool vtimer_cntp_tval(struct cpu_user_regs
>> *regs, uint32_t *r,
>>bool read)
>> {
>> struct vcpu *v = current;
>> -s_time_t now;
>> +uint64_t cntpct;
>>
>> if ( !ACCESS_ALLOWED(regs, EL0PTEN) )
>> return false;
>>
>> -now = NOW() - v->domain->arch.phys_timer_base.offset;
>> +cntpct = get_cycles();
>>
>> if ( read )
>> {
>> -*r = (uint32_t)(ns_to_ticks(v->arch.phys_timer.cval - now) &
>> 0xull);
>> +*r = (uint32_t)((v->arch.phys_timer.cval - cntpct) & 0xull);
>> }
>> else
>> {
>> -v->arch.phys_timer.cval = now + ticks_to_ns(*r);
>> +v->arch.phys_timer.cval = cntpct + (uint64_t)(*((int32_t *)r));
>
> I would prefer (uint64_t)(int32_t)*r.
I will update accordingly.
>> if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>> {
>> v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
>>
Re: [Xen-devel] [PATCH v2 16/22] golang/xenlight: implement keyed union C to Go marshaling
> It actually occurs to me that the "named struct elements of union" would
> still technically open up a window for divergence: i.e., if somehow the
> type of the named struct didn't match up with the union element.
>
> I.e., the following *shouldn't* happen, but technically it *could*:
>
>
> struct libxl_domain_build_info_union_hvm {
> ...
> }
>
> struct libxl_domain_build_info {
> union {
> libxl_domain_struct_build_info_hvm2 hvm;
> } u;
> }
> ---
>
> Using the `typeof` trick above guarantees that the types the marshaling
> functions are using are identical to the types actually specified in the
> union itself. Particularly as this is just generated code nobody's
> going to look at, I'm inclined to think the cost is near-zero. Since
> the benefit is non-zero, I'd be inclined to say just go with that instead.
>
> And it's easier!
>
> Thoughts?
In that case I'll just use the typeof trick :)
-NR
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 15/22] golang/xenlight: begin C to Go type marshaling
> So first of all, I noticed that the marshalling code for Union structs
> does what I suggest. :-)
Yeah I realized that. I must have figured out that my previous way
wasn't necessary, but forgot to go back and change it.
> I'm not super-strong on this, so I don't want to bike-shed. But I think
> the way you generate the marshalling code for the union structs is better:
>
> if err := x.Pae.fromC(); err != nil {
> return err
> }
I'll change it. I'd prefer not to have those inconsistencies in the
code generation.
Thanks,
-NR
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86: support Atom Tremont
On 05/12/2019 15:43, Jan Beulich wrote: > Add model 0x86 to relevant switch() statements, as per SDM 069 Vol 4. > > Signed-off-by: Jan Beulich Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86: don't offer Hyper-V option when "PV Shim Exclusive"
On 05/12/2019 15:42, Jan Beulich wrote: > This only added dead code. Use "if" instead of "depends on" to make > (halfway) clear that other guest options should also go in thae same > block. Move the option down such that the shim related options get > presented first, avoiding to ask the question when the answer may end > up being discarded. > > While in the neighborhood also bring PV_SHIM_EXCLUSIVE into more > "canonical" shape. > > Signed-off-by: Jan Beulich Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 15/22] golang/xenlight: begin C to Go type marshaling
On 12/5/19 4:38 PM, Nick Rosbrook wrote:
>> You should probably say here explicitly what kinds of elements you're
>> supporting and not supporting in this patch; specifically:
>>
>> - You're converting built-ins (or is this any struct-like type?)
>
> Any struct-like type, since the fromC functions are all defined in
> this patch (excluding array fields and keyed unions as you said
> below).
>
>> - You handle nested anonymous structs
>> - But you're not handling keyed unions or arrays (anything else)?
>
> I think this covers it, thanks.
>
>>> +func (x *VncInfo) fromC(xc *C.libxl_vnc_info) error {
>>> + var defboolEnable Defbool
>>> + if err := defboolEnable.fromC(); err != nil {
>>> + return err
>>> + }
>>
>> Is there a reason in these cases that we don't simply call .fromC on the
>> elemet itself?
>
> This ensures that when we call fromC, we have an initialized variable.
> This might be overkill here, as this would matter more if we had
> structs with a nested struct pointer. E.g., [1] will panic since only
> the outer struct is initialized.
So first of all, I noticed that the marshalling code for Union structs
does what I suggest. :-)
I can see how such a construct would be needed when there was a pointer
type. But if there was a pointer type, you'd have to special-case
things anyway. Take the following code that has a non-pointer element:
var defboolEnable Defbool // The same type as x.Enable
if err := defboolEnable.fromC(); err != nil {
return err
}
x.Enable = defboolEnable
Now suppose you had a pointer element instead; what would it look like?
var defboolEnable Defbool // NOT the same type as x.Enable!
if err := defboolEnable.fromC(); err != nil {
return err
}
x.Enable =
The generation code would have to replace the type of "Enable", which is
a pointer, with the actual instance of the thing; and then assign the
reference, rather than the value. So if we run across something like
that, we can sort that out when we come to it.
I'm not super-strong on this, so I don't want to bike-shed. But I think
the way you generate the marshalling code for the union structs is better:
if err := x.Pae.fromC(); err != nil {
return err
}
Thanks,
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-4.8-testing test] 144544: regressions - trouble: fail/pass/starved
flight 144544 xen-4.8-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/144544/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt 8 host-ping-check-xen fail REGR. vs. 138829 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 15 guest-saverestore.2fail like 138770 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 138770 test-xtf-amd64-amd64-1 50 xtf/test-hvm64-lbr-tsx-vmentry fail like 138809 test-xtf-amd64-amd64-4 50 xtf/test-hvm64-lbr-tsx-vmentry fail like 138829 test-xtf-amd64-amd64-4 70 xtf/test-hvm64-xsa-278 fail like 138829 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 138829 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 138829 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 138829 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 138829 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 138829 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 138829 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 138829 test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-vhd 12 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-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-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-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-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 2 hosts-allocate starved n/a version targeted for testing: xen a260e93db794f560502e89859aaf111d178e80e4 baseline version: xen c67210f60dfa83565d26ae710e4f5e729a95dce5 Last test of basis 138829 2019-07-08 14:58:57 Z 150 days Failing since143733 2019-11-04 14:46:41 Z 31 days9 attempts Testing same since 144544 2019-12-05 12:05:17 Z0 days1 attempts People who touched revisions under test: Andrew Cooper George Dunlap Ian Jackson Jan Beulich
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
On 12/5/19 8:58 AM, Vladimir Sementsov-Ogievskiy wrote:
What about you provide the examples, and then I try to polish the prose?
1: error_fatal problem
Assume the following code flow:
int f1(errp) {
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
Now, if we call f1 with _fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.
So, in this case we should use local_err.
How does this example look after the transformation?
Without ERRP_AUTO_PROPAGATE(), the transformation is a lot of boilerplate:
int f1(errp) {
Error *err = NULL;
ret = f2();
if (ret < 0) {
error_append_hint(, "very useful hint");
error_propagate(errp, err);
return ret;
}
}
what's worse, that boilerplate to solve problem 1 turns out to be...
Good point.
int f1(errp) {
ERRP_AUTO_PROPAGATE();
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.
2: error_abort problem
Now, consider functions without return value. We normally use local_err
variable to catch failures:
void f1(errp) {
Error *local_err = NULL;
...
f2(_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
...
}
the very same code as the cause of problem 2.
Now, if we call f2 with _abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.
So, we should never wrap error_abort by local_err.
Likewise.
And here:
void f1(errp) {
ERRP_AUTO_PROPAGATE();
...
f2(errp);
if (*errp) {
return;
}
...
- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
local error is automatically propagated to original one.
So, the use of ERRP_AUTO_PROPAGATE() solves BOTH problems 1 and 2 - we
avoid the boilerplate that trades one problem for another, by
consolidating ALL of the boilerplate into a single-line macro, such that
error_propagate() no longer needs to be called anywhere except inside
the ERRP_AUTO_PROPAGATE macro.
===
Our solution:
- Fixes [1.], adding invocation of new macro into functions with
error_appen_hint/error_prepend,
New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which
never
wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
propagations
(in fact, error_propagate is called, but returns immediately on first if
(!local_err))
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 16/22] golang/xenlight: implement keyed union C to Go marshaling
On 12/5/19 4:53 PM, Nick Rosbrook wrote:
>>> It looks like this is duplicating (differently!) the field-copying code
>>> from golang_define_from_C. Is there any reason you couldn't have a
>>> single function, `xenlight_golang_fields_from_C`, which would be used
>>> for both?
>
> No, I should be able to re-factor that. Thanks.
>
>> Actually, it turns out we don't strictly need to duplicate this at all,
>> if we use the `typeof` operator, like this:
>>
>> ---
>> typedef typeof(((struct libxl_channelinfo *)NULL)->u.pty)
>> libxl_channelinfo_connection_union_pty;
>>
>> typedef typeof(((struct libxl_domain_build_info *)NULL)->u.hvm)
>> libxl_domain_build_info_type_union_hvm;
>>
>> typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pv)
>> libxl_domain_build_info_type_union_pv;
>>
>> typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pvh)
>> libxl_domain_build_info_type_union_pvh;
>>
>> typedef typeof(((struct libxl_device_usbdev *)NULL)->u.hostdev)
>> libxl_device_usbdev_type_union_hostdev;
>>
>> typedef typeof(((struct libxl_device_channel *)NULL)->u.socket)
>> libxl_device_channel_connection_union_socket;
>> ---
>>
>> This guarantees we'll have the correct layout for the resulting type.
>
> Well that's pretty cool.
>
>> I talked to Ian Jackson, and he agreed that long-term it would be good
>> for the C generator to generate named types for these union elements
>> (likke you have here). If you felt really motivated you could do that
>> now; but I think using the `typeof` trick would be suitable to get this
>> patch in.
>
> I'll take a look at least and see if I can get it done fairly easily.
> Otherwise, I'll use this trick.
It actually occurs to me that the "named struct elements of union" would
still technically open up a window for divergence: i.e., if somehow the
type of the named struct didn't match up with the union element.
I.e., the following *shouldn't* happen, but technically it *could*:
struct libxl_domain_build_info_union_hvm {
...
}
struct libxl_domain_build_info {
union {
libxl_domain_struct_build_info_hvm2 hvm;
} u;
}
---
Using the `typeof` trick above guarantees that the types the marshaling
functions are using are identical to the types actually specified in the
union itself. Particularly as this is just generated code nobody's
going to look at, I'm inclined to think the cost is near-zero. Since
the benefit is non-zero, I'd be inclined to say just go with that instead.
And it's easier!
Thoughts?
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [OSSTEST PATCH] ts-xen-build-prep: Install python3-docutils
This is the package (or, one of the packages) containing rst2html.
This is now needed for builds of libvirt upstream.
Really this packages install call should be ts-libvirt-build, but:
Historically we have done it all in ts-xen-build-prep. In the
meantime we have put a lock around the call to the package manager,
but this has only been lightly tested. At this stage of the Xen
release we would rather be cautious.
CC: Juergen Gross
Signed-off-by: Ian Jackson
---
ts-xen-build-prep | 1 +
1 file changed, 1 insertion(+)
diff --git a/ts-xen-build-prep b/ts-xen-build-prep
index 0f07648e..5d2f50ba 100755
--- a/ts-xen-build-prep
+++ b/ts-xen-build-prep
@@ -208,6 +208,7 @@ sub prep () {
libxml2-utils libxml2-dev
libdevmapper-dev libxml-xpath-perl libelf-dev
ccache nasm checkpolicy ebtables
+ python3-docutils
libgnutls28-dev);
if ($ho->{Suite} =~ m/squeeze|wheezy|jessie/) {
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/22] golang/xenlight: generate enum types from IDL
> Sorry to come back to this... I think this would be better explicitly > listing out the files that are needed. As I said, the current way of > doing things means gengotypes.py will *always* be re-run; and the > result, when experimenting with the `typeof` before, was that my local > changes to helpes.gen.py were being overwritten. > > Having this be `xenlight.go types.gen.go helpers.gen.go` instead means > > 1) gengotypes is only run when one of its inputs changes > 2) you can make local changes to the generated files and have them > copied over > 3) The copying only happens when one of the above two things happens > 4) The final compile is slightly faster because the go compiler knows > that the files hasn't changed. > > I can change this on check-in if you're OK with it, and I end up > checking part of the series in before you send v3. That all makes sense to me. I have no problem with you making the change on check-in if you go that route. Thanks, -NR ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] gnttab: don't expose host physical address without need
On 05.12.2019 16:47, Andrew Cooper wrote: > On 05/12/2019 15:34, Jan Beulich wrote: >> Translated domains shouldn't see host physical addresses. While the >> address is also not supposed to be handed back even to non-translated >> domains when GNTMAP_device_map is not set (as explicitly stated by a >> comment in the public header), PV kernels (Linux at least) assume the >> field to get populated nevertheless. > > This really means that the public header needs correcting. The field > may not have intended to escape out of Xen, but it is defacto part of > the ABI now. Well, that's one of two possible routes. The other is to have, like you did suggest earlier on, a mode in which we behave more strictly, and current Linux then wouldn't work on such a Xen until fixed. >> (Similarly mapkind() should check only GNTMAP_device_map.) > > Is this comment stale, or have I misunderstood some of the reasoning? It's certainly not stale. mapkind() is used to determine whether IOMMU mapping adjustments are needed. With this, it should in principle only consider whether the current operation would possibly alter IOMMU mapping needs. What needs doing should, according to my interpretation of the originally intended design, only depend on current and prior requests with GNTMAP_device_map set. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 16/22] golang/xenlight: implement keyed union C to Go marshaling
> > It looks like this is duplicating (differently!) the field-copying code > > from golang_define_from_C. Is there any reason you couldn't have a > > single function, `xenlight_golang_fields_from_C`, which would be used > > for both? No, I should be able to re-factor that. Thanks. > Actually, it turns out we don't strictly need to duplicate this at all, > if we use the `typeof` operator, like this: > > --- > typedef typeof(((struct libxl_channelinfo *)NULL)->u.pty) > libxl_channelinfo_connection_union_pty; > > typedef typeof(((struct libxl_domain_build_info *)NULL)->u.hvm) > libxl_domain_build_info_type_union_hvm; > > typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pv) > libxl_domain_build_info_type_union_pv; > > typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pvh) > libxl_domain_build_info_type_union_pvh; > > typedef typeof(((struct libxl_device_usbdev *)NULL)->u.hostdev) > libxl_device_usbdev_type_union_hostdev; > > typedef typeof(((struct libxl_device_channel *)NULL)->u.socket) > libxl_device_channel_connection_union_socket; > --- > > This guarantees we'll have the correct layout for the resulting type. Well that's pretty cool. > I talked to Ian Jackson, and he agreed that long-term it would be good > for the C generator to generate named types for these union elements > (likke you have here). If you felt really motivated you could do that > now; but I think using the `typeof` trick would be suitable to get this > patch in. I'll take a look at least and see if I can get it done fairly easily. Otherwise, I'll use this trick. Thanks, -NR ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] cmdline: treat hyphens and underscores the same
On 05.12.2019 17:27, Julien Grall wrote:
> On 05/12/2019 15:33, Jan Beulich wrote:
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -23,6 +23,49 @@ enum system_state system_state = SYS_STA
>> xen_commandline_t saved_cmdline;
>> static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
>>
>> +static int cdiff(unsigned char c1, unsigned char c2)
>
> This is not obvious from the name and the implementation what it does
> (it took me a few minutes to figure it out). So I think you want to add
> a comment.
Sure, done.
>> +{
>> +int res = c1 - c2;
>> +
>> +if ( res && (c1 ^ c2) == ('-' ^ '_') &&
>> + (c1 == '-' || c1 == '_') )
>> +res = 0;
>> +
>> +return res;
>> +}
>> +
>> +/*
>> + * String comparison functions mostly matching strcmp() / strncmp(),
>> + * except that they treat '-' and '_' as matching one another.
>> + */
>> +static int _strcmp(const char *s1, const char *s2)
>
> I thought we were trying to avoid new function name with leading _?
We're trying to avoid new name space violations. Such are
- identifiers starting with two underscores,
- identifiers starting with an underscore and an upper case letter,
- identifiers of non-static symbols starting with an underscore.
> But it is really worth to implement both strcmp and strncmp rather than
> using the latter to implement the former?
>
> I know this involve using strlen, but I am not convinced this will be
> noticeable at boot.
To be honest - it didn't even occur to me. The functions seemed
simple enough to not have one use the other. If others agree
with you, I'd be fine doing as you suggest.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 15/22] golang/xenlight: begin C to Go type marshaling
> You should probably say here explicitly what kinds of elements you're
> supporting and not supporting in this patch; specifically:
>
> - You're converting built-ins (or is this any struct-like type?)
Any struct-like type, since the fromC functions are all defined in
this patch (excluding array fields and keyed unions as you said
below).
> - You handle nested anonymous structs
> - But you're not handling keyed unions or arrays (anything else)?
I think this covers it, thanks.
> > +func (x *VncInfo) fromC(xc *C.libxl_vnc_info) error {
> > + var defboolEnable Defbool
> > + if err := defboolEnable.fromC(); err != nil {
> > + return err
> > + }
>
> Is there a reason in these cases that we don't simply call .fromC on the
> elemet itself?
This ensures that when we call fromC, we have an initialized variable.
This might be overkill here, as this would matter more if we had
structs with a nested struct pointer. E.g., [1] will panic since only
the outer struct is initialized.
-NR
[1] https://play.golang.org/p/1gDVjbXgWd-
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] cmdline: treat hyphens and underscores the same
Hi,
On 05/12/2019 15:33, Jan Beulich wrote:
In order to avoid permanently having to ask that no new command line
options using underscores be introduced (albeit I'm likely to still make
remarks), and in order to also allow extending the use of hyphens to
pre-existing ones, introduce custom comparison functions treating both
characters as matching.
Signed-off-by: Jan Beulich
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -72,6 +72,11 @@ Some options take a comma separated list
Some parameters act as combinations of the above, most commonly a mix
of Boolean and String. These are noted in the relevant sections.
+### Spelling
+
+Parameter names may include hyphens or underscores. These are
+generally being treated as matching one another by the parsing logic.
+
## Parameter details
### acpi
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -23,6 +23,49 @@ enum system_state system_state = SYS_STA
xen_commandline_t saved_cmdline;
static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
+static int cdiff(unsigned char c1, unsigned char c2)
This is not obvious from the name and the implementation what it does
(it took me a few minutes to figure it out). So I think you want to add
a comment.
+{
+int res = c1 - c2;
+
+if ( res && (c1 ^ c2) == ('-' ^ '_') &&
+ (c1 == '-' || c1 == '_') )
+res = 0;
+
+return res;
+}
+
+/*
+ * String comparison functions mostly matching strcmp() / strncmp(),
+ * except that they treat '-' and '_' as matching one another.
+ */
+static int _strcmp(const char *s1, const char *s2)
I thought we were trying to avoid new function name with leading _?
But it is really worth to implement both strcmp and strncmp rather than
using the latter to implement the former?
I know this involve using strlen, but I am not convinced this will be
noticeable at boot.
+{
+int res;
+
+for ( ; ; ++s1, ++s2 )
+{
+res = cdiff(*s1, *s2);
+if ( res || !*s1 )
+break;
+}
+
+return res;
+}
+
+static int _strncmp(const char *s1, const char *s2, size_t n)
+{
+int res = 0;
+
+for ( ; n--; ++s1, ++s2 )
+{
+res = cdiff(*s1, *s2);
+if ( res || !*s1 )
+break;
+}
+
+return res;
+}
+
static int assign_integer_param(const struct kernel_param *param, uint64_t
val)
{
switch ( param->len )
@@ -94,7 +137,7 @@ static int parse_params(const char *cmdl
/* Boolean parameters can be inverted with 'no-' prefix. */
key = optkey;
-bool_assert = !!strncmp("no-", optkey, 3);
+bool_assert = !!_strncmp("no-", optkey, 3);
if ( !bool_assert )
optkey += 3;
@@ -105,11 +148,11 @@ static int parse_params(const char *cmdl
int rctmp;
const char *s;
-if ( strcmp(param->name, optkey) )
+if ( _strcmp(param->name, optkey) )
{
if ( param->type == OPT_CUSTOM && q &&
strlen(param->name) == q + 1 - opt &&
- !strncmp(param->name, opt, q + 1 - opt) )
+ !_strncmp(param->name, opt, q + 1 - opt) )
{
found = true;
optval[-1] = '=';
@@ -275,7 +318,7 @@ int parse_bool(const char *s, const char
int parse_boolean(const char *name, const char *s, const char *e)
{
size_t slen, nlen;
-int val = !!strncmp(s, "no-", 3);
+int val = !!_strncmp(s, "no-", 3);
if ( !val )
s += 3;
@@ -284,7 +327,7 @@ int parse_boolean(const char *name, cons
nlen = strlen(name);
/* Does s now start with name? */
-if ( slen < nlen || strncmp(s, name, nlen) )
+if ( slen < nlen || _strncmp(s, name, nlen) )
return -1;
/* Exact, unadorned name? Result depends on the 'no-' prefix. */
@@ -304,7 +347,7 @@ int cmdline_strcmp(const char *frag, con
for ( ; ; frag++, name++ )
{
unsigned char f = *frag, n = *name;
-int res = f - n;
+int res = cdiff(f, n);
if ( res || n == '\0' )
{
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86: don't offer Hyper-V option when "PV Shim Exclusive"
On Thu, Dec 05, 2019 at 04:42:08PM +0100, Jan Beulich wrote: > > This only added dead code. Use "if" instead of "depends on" to make > (halfway) clear that other guest options should also go in thae same s/thae/the/ > block. Move the option down such that the shim related options get > presented first, avoiding to ask the question when the answer may end > up being discarded. > > While in the neighborhood also bring PV_SHIM_EXCLUSIVE into more > "canonical" shape. > Alan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86: don't offer Hyper-V option when "PV Shim Exclusive"
On Thu, 5 Dec 2019 at 15:41, Jan Beulich wrote: > > This only added dead code. Use "if" instead of "depends on" to make > (halfway) clear that other guest options should also go in thae same > block. Move the option down such that the shim related options get > presented first, avoiding to ask the question when the answer may end > up being discarded. > > While in the neighborhood also bring PV_SHIM_EXCLUSIVE into more > "canonical" shape. > > Signed-off-by: Jan Beulich > Reviewed-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] gnttab: don't expose host physical address without need
On 05/12/2019 15:47, Andrew Cooper wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -1394,7 +1401,7 @@ unmap_common( >> >> op->mfn = act->mfn; >> >> -if ( op->dev_bus_addr && >> +if ( op->dev_bus_addr && (flags & GNTMAP_device_map) && > Drop the first clause entirely? act->mfn will never be 0 so can subsume > the check with one fewer branch. Never mind. I've just remembered why 0 is special, and the check needs to stay. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/nEPT: ditch nept_sp_entry()
On 12/5/19 3:41 PM, Jan Beulich wrote:
> It's bogusly non-static. It making the call sites actually less easy to
> read, and there being another open-coded use in the file - let's just
> get rid of it.
>
> Signed-off-by: Jan Beulich
>
> --- a/xen/arch/x86/mm/hap/nested_ept.c
> +++ b/xen/arch/x86/mm/hap/nested_ept.c
> @@ -54,11 +54,6 @@
> #define NEPT_2M_ENTRY_FLAG (1 << 10)
> #define NEPT_4K_ENTRY_FLAG (1 << 9)
>
> -bool_t nept_sp_entry(ept_entry_t e)
> -{
> -return !!(e.sp);
> -}
What a strange function!
Acked-by: George Dunlap
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 144547: tolerable all pass - PUSHED
flight 144547 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/144547/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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 version targeted for testing: xen 79cf0989175c16994efc1f152eef07bb48cb98df baseline version: xen ad5c7c162519a3f96561ea4791da1319d9bfdfed Last test of basis 144523 2019-12-04 20:00:43 Z0 days Testing same since 144547 2019-12-05 13:01:23 Z0 days1 attempts People who touched revisions under test: Igor Druzhinin jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt 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 : To xenbits.xen.org:/home/xen/git/xen.git ad5c7c1625..79cf098917 79cf0989175c16994efc1f152eef07bb48cb98df -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/nEPT: ditch nept_sp_entry()
On 05/12/2019 15:41, Jan Beulich wrote: > It's bogusly non-static. It making the call sites actually less easy to > read, and there being another open-coded use in the file - let's just > get rid of it. > > Signed-off-by: Jan Beulich Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] gnttab: don't expose host physical address without need
On 05/12/2019 15:34, Jan Beulich wrote:
> Translated domains shouldn't see host physical addresses. While the
> address is also not supposed to be handed back even to non-translated
> domains when GNTMAP_device_map is not set (as explicitly stated by a
> comment in the public header), PV kernels (Linux at least) assume the
> field to get populated nevertheless.
This really means that the public header needs correcting. The field
may not have intended to escape out of Xen, but it is defacto part of
the ABI now.
> (Similarly mapkind() should check only GNTMAP_device_map.)
Is this comment stale, or have I misunderstood some of the reasoning?
>
> Along these lines split the paging mode related check near the top of
> map_grant_ref() to handle the "external" and "translated" cases
> separately (GNTMAP_device_map use getting tied to being non-translated
> rather than non-external).
>
> Still along these lines in the unmapping case there's no point checking
> ->dev_bus_addr when GNTMAP_device_map isn't set (and hence the field
> isn't going to be consumed).
>
> Signed-off-by: Jan Beulich
> ---
> v4: Re-base over dropped patches.
> v3: New.
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -967,10 +967,16 @@ map_grant_ref(
> }
>
> if ( unlikely(paging_mode_external(ld) &&
> - (op->flags & (GNTMAP_device_map|GNTMAP_application_map|
> -GNTMAP_contains_pte))) )
> + (op->flags &
> (GNTMAP_application_map|GNTMAP_contains_pte))) )
> {
> -gdprintk(XENLOG_INFO, "No device mapping in HVM domain\n");
> +gdprintk(XENLOG_INFO, "No app/pte mapping in HVM domain\n");
> +op->status = GNTST_general_error;
> +return;
> +}
> +
> +if ( paging_mode_translate(ld) && unlikely(op->flags &
> GNTMAP_device_map) )
> +{
> +gdprintk(XENLOG_INFO, "No device mapping in translated domain\n");
> op->status = GNTST_general_error;
> return;
> }
> @@ -1213,7 +1219,8 @@ map_grant_ref(
> if ( need_iommu )
> double_gt_unlock(lgt, rgt);
>
> -op->dev_bus_addr = mfn_to_maddr(mfn);
> +op->dev_bus_addr = paging_mode_translate(ld) ? op->host_addr
> + : mfn_to_maddr(mfn);
> op->handle = handle;
> op->status = GNTST_okay;
>
> @@ -1394,7 +1401,7 @@ unmap_common(
>
> op->mfn = act->mfn;
>
> -if ( op->dev_bus_addr &&
> +if ( op->dev_bus_addr && (flags & GNTMAP_device_map) &&
Drop the first clause entirely? act->mfn will never be 0 so can subsume
the check with one fewer branch.
~Andrew
> unlikely(op->dev_bus_addr != mfn_to_maddr(act->mfn)) )
> PIN_FAIL(act_release_out, GNTST_general_error,
> "Bus address doesn't match gntref (%"PRIx64" !=
> %"PRIpaddr")\n",
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] x86: support Atom Tremont
Add model 0x86 to relevant switch() statements, as per SDM 069 Vol 4. Signed-off-by: Jan Beulich --- v2: Drop spec_ctrl.c adjustments. --- a/xen/arch/x86/acpi/cpu_idle.c +++ b/xen/arch/x86/acpi/cpu_idle.c @@ -238,6 +238,8 @@ static void do_get_hw_residencies(void * case 0x5F: /* Goldmont Plus */ case 0x7A: +/* Tremont */ +case 0x86: GET_PC2_RES(hw_res->pc2); GET_PC3_RES(hw_res->pc3); GET_PC6_RES(hw_res->pc6); --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2778,6 +2778,8 @@ static const struct lbr_info *last_branc case 0x66: /* Goldmont Plus */ case 0x7a: +/* Tremont */ +case 0x86: /* Kaby Lake */ case 0x8e: case 0x9e: return sk_lbr; ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/nEPT: ditch nept_sp_entry()
It's bogusly non-static. It making the call sites actually less easy to
read, and there being another open-coded use in the file - let's just
get rid of it.
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm/hap/nested_ept.c
+++ b/xen/arch/x86/mm/hap/nested_ept.c
@@ -54,11 +54,6 @@
#define NEPT_2M_ENTRY_FLAG (1 << 10)
#define NEPT_4K_ENTRY_FLAG (1 << 9)
-bool_t nept_sp_entry(ept_entry_t e)
-{
-return !!(e.sp);
-}
-
static bool_t nept_rsv_bits_check(ept_entry_t e, uint32_t level)
{
uint64_t rsv_bits = EPT_MUST_RSV_BITS;
@@ -68,7 +63,7 @@ static bool_t nept_rsv_bits_check(ept_en
case 1:
break;
case 2 ... 3:
-if ( nept_sp_entry(e) )
+if ( e.sp )
rsv_bits |= ((1ull << (9 * (level - 1))) - 1) << PAGE_SHIFT;
else
rsv_bits |= EPTE_EMT_MASK | EPTE_IGMT_MASK;
@@ -181,7 +176,7 @@ nept_walk_tables(struct vcpu *v, unsigne
if ( nept_misconfiguration_check(gw->lxe[lvl], lvl) )
goto misconfig_err;
-if ( (lvl == 2 || lvl == 3) && nept_sp_entry(gw->lxe[lvl]) )
+if ( (lvl == 2 || lvl == 3) && gw->lxe[lvl].sp )
{
/* Generate a fake l1 table entry so callers don't all
* have to understand superpages. */
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86: don't offer Hyper-V option when "PV Shim Exclusive"
This only added dead code. Use "if" instead of "depends on" to make (halfway) clear that other guest options should also go in thae same block. Move the option down such that the shim related options get presented first, avoiding to ask the question when the answer may end up being discarded. While in the neighborhood also bring PV_SHIM_EXCLUSIVE into more "canonical" shape. Signed-off-by: Jan Beulich --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -164,14 +164,6 @@ endchoice config GUEST bool -config HYPERV_GUEST - bool "Hyper-V Guest" - select GUEST - ---help--- - Support for Xen detecting when it is running under Hyper-V. - - If unsure, say N. - config XEN_GUEST bool "Xen Guest" select GUEST @@ -201,8 +193,7 @@ config PV_SHIM If unsure, say Y. config PV_SHIM_EXCLUSIVE - def_bool n - prompt "PV Shim Exclusive" + bool "PV Shim Exclusive" depends on PV_SHIM ---help--- Build Xen in a way which unconditionally assumes PV_SHIM mode. This @@ -211,6 +202,18 @@ config PV_SHIM_EXCLUSIVE If unsure, say N. +if !PV_SHIM_EXCLUSIVE + +config HYPERV_GUEST + bool "Hyper-V Guest" + select GUEST + ---help--- + Support for Xen detecting when it is running under Hyper-V. + + If unsure, say N. + +endif + config MEM_SHARING bool "Xen memory sharing support" if EXPERT = "y" depends on HVM ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 12/22] golang/xenlight: re-factor Hwcap type implementation
> Same thing with casting. Ack for all such cases of this casting. That's a good way to simplify. -NR ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v4] gnttab: don't expose host physical address without need
Translated domains shouldn't see host physical addresses. While the
address is also not supposed to be handed back even to non-translated
domains when GNTMAP_device_map is not set (as explicitly stated by a
comment in the public header), PV kernels (Linux at least) assume the
field to get populated nevertheless. (Similarly mapkind() should check
only GNTMAP_device_map.)
Along these lines split the paging mode related check near the top of
map_grant_ref() to handle the "external" and "translated" cases
separately (GNTMAP_device_map use getting tied to being non-translated
rather than non-external).
Still along these lines in the unmapping case there's no point checking
->dev_bus_addr when GNTMAP_device_map isn't set (and hence the field
isn't going to be consumed).
Signed-off-by: Jan Beulich
---
v4: Re-base over dropped patches.
v3: New.
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -967,10 +967,16 @@ map_grant_ref(
}
if ( unlikely(paging_mode_external(ld) &&
- (op->flags & (GNTMAP_device_map|GNTMAP_application_map|
-GNTMAP_contains_pte))) )
+ (op->flags & (GNTMAP_application_map|GNTMAP_contains_pte))) )
{
-gdprintk(XENLOG_INFO, "No device mapping in HVM domain\n");
+gdprintk(XENLOG_INFO, "No app/pte mapping in HVM domain\n");
+op->status = GNTST_general_error;
+return;
+}
+
+if ( paging_mode_translate(ld) && unlikely(op->flags & GNTMAP_device_map) )
+{
+gdprintk(XENLOG_INFO, "No device mapping in translated domain\n");
op->status = GNTST_general_error;
return;
}
@@ -1213,7 +1219,8 @@ map_grant_ref(
if ( need_iommu )
double_gt_unlock(lgt, rgt);
-op->dev_bus_addr = mfn_to_maddr(mfn);
+op->dev_bus_addr = paging_mode_translate(ld) ? op->host_addr
+ : mfn_to_maddr(mfn);
op->handle = handle;
op->status = GNTST_okay;
@@ -1394,7 +1401,7 @@ unmap_common(
op->mfn = act->mfn;
-if ( op->dev_bus_addr &&
+if ( op->dev_bus_addr && (flags & GNTMAP_device_map) &&
unlikely(op->dev_bus_addr != mfn_to_maddr(act->mfn)) )
PIN_FAIL(act_release_out, GNTST_general_error,
"Bus address doesn't match gntref (%"PRIx64" !=
%"PRIpaddr")\n",
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] cmdline: treat hyphens and underscores the same
In order to avoid permanently having to ask that no new command line
options using underscores be introduced (albeit I'm likely to still make
remarks), and in order to also allow extending the use of hyphens to
pre-existing ones, introduce custom comparison functions treating both
characters as matching.
Signed-off-by: Jan Beulich
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -72,6 +72,11 @@ Some options take a comma separated list
Some parameters act as combinations of the above, most commonly a mix
of Boolean and String. These are noted in the relevant sections.
+### Spelling
+
+Parameter names may include hyphens or underscores. These are
+generally being treated as matching one another by the parsing logic.
+
## Parameter details
### acpi
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -23,6 +23,49 @@ enum system_state system_state = SYS_STA
xen_commandline_t saved_cmdline;
static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
+static int cdiff(unsigned char c1, unsigned char c2)
+{
+int res = c1 - c2;
+
+if ( res && (c1 ^ c2) == ('-' ^ '_') &&
+ (c1 == '-' || c1 == '_') )
+res = 0;
+
+return res;
+}
+
+/*
+ * String comparison functions mostly matching strcmp() / strncmp(),
+ * except that they treat '-' and '_' as matching one another.
+ */
+static int _strcmp(const char *s1, const char *s2)
+{
+int res;
+
+for ( ; ; ++s1, ++s2 )
+{
+res = cdiff(*s1, *s2);
+if ( res || !*s1 )
+break;
+}
+
+return res;
+}
+
+static int _strncmp(const char *s1, const char *s2, size_t n)
+{
+int res = 0;
+
+for ( ; n--; ++s1, ++s2 )
+{
+res = cdiff(*s1, *s2);
+if ( res || !*s1 )
+break;
+}
+
+return res;
+}
+
static int assign_integer_param(const struct kernel_param *param, uint64_t val)
{
switch ( param->len )
@@ -94,7 +137,7 @@ static int parse_params(const char *cmdl
/* Boolean parameters can be inverted with 'no-' prefix. */
key = optkey;
-bool_assert = !!strncmp("no-", optkey, 3);
+bool_assert = !!_strncmp("no-", optkey, 3);
if ( !bool_assert )
optkey += 3;
@@ -105,11 +148,11 @@ static int parse_params(const char *cmdl
int rctmp;
const char *s;
-if ( strcmp(param->name, optkey) )
+if ( _strcmp(param->name, optkey) )
{
if ( param->type == OPT_CUSTOM && q &&
strlen(param->name) == q + 1 - opt &&
- !strncmp(param->name, opt, q + 1 - opt) )
+ !_strncmp(param->name, opt, q + 1 - opt) )
{
found = true;
optval[-1] = '=';
@@ -275,7 +318,7 @@ int parse_bool(const char *s, const char
int parse_boolean(const char *name, const char *s, const char *e)
{
size_t slen, nlen;
-int val = !!strncmp(s, "no-", 3);
+int val = !!_strncmp(s, "no-", 3);
if ( !val )
s += 3;
@@ -284,7 +327,7 @@ int parse_boolean(const char *name, cons
nlen = strlen(name);
/* Does s now start with name? */
-if ( slen < nlen || strncmp(s, name, nlen) )
+if ( slen < nlen || _strncmp(s, name, nlen) )
return -1;
/* Exact, unadorned name? Result depends on the 'no-' prefix. */
@@ -304,7 +347,7 @@ int cmdline_strcmp(const char *frag, con
for ( ; ; frag++, name++ )
{
unsigned char f = *frag, n = *name;
-int res = f - n;
+int res = cdiff(f, n);
if ( res || n == '\0' )
{
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Xen 4.14 and future work
On 02/12/2019 19:51, Andrew Cooper wrote: > Hello, > > Now that 4.13 is on its way out of the door, it is time to look to > ongoing work. (and some more...) * Shim: Removal of an M2P. Within the shim, two M2P's are constructed, and one of them is entirely unused. Both take up a decent chunk of memory, an contribute to reduced packing density By inspecting the kernel width earlier during boot, we can avoid creating the unneeded M2P. This would require teaching Xen to operate with only a compat p2m when running a 32bit guest, but this will be fine to run with. * CONFIG_PV32 Despite being deprecated in 64bit processors, we still use ring 1 for 32bit PV kernels. A relic of a bygone era, this causes problems with newer pagetable based features (SMEP and SMAP in particular), resulting in complexity in the entry paths and a substantial performance penalty (a CR4 write on the way in and out of 32bit guests). There are also some speculative mitigation actions we take unilaterally, just because 32bit PV guest kernels run on supervisor mappings rather than user mappings (RSB-stuffing being the most obvious one). From an attack surface point of view, being able to remove all ring 1 facilities would be great for deployments not intending to run 32bit PV guests (and/or relegate them to PV-shim), and there will be some (probably minor) performance gains for 64bit PV guests as a consequence of the simplified entry paths. (And who knows... A combination of this and Paul's idea for emulation thunk-ing in a pseudo SMM-like mode inside HVM guests could remove the need for CONFIG_COMPAT entirely at L0.) * CONFIG_PDX Here is one I prepared earlier. https://andrewcoop-xen.readthedocs.io/en/docs-devel/misc/tech-debt.html#config-pdx * CONFIG_$VENDOR For restricted usecases, dropping one of AMD or INTEL would drop a substantial chunk of code, and a sufficiently capable LTO compiler with devirtualisation support could even remove the function pointers themselves. More usefully to the general project however, would be RANDCONFIG's ability to check and keep our vendor-specific interfaces clean. They most definitely are not right now. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 02/22] golang/xenlight: define Defbool builtin type
On 12/5/19 3:23 PM, Nick Rosbrook wrote:
>> I implemented some tests for these methods to make sure everything
>> worked as expected (they did); but there's an unexpected side-effect:
>>
>> - *_test.go files cannot `import "C"`
>
> Yeah, this is unfortunate.
>
>> - The fromC / toC methods aren't exported
>>
>> So it's not possible to do the following check:
>>
>> var b Defbool
>>
>> b.Set(true)
>> cb, err := b.toC()
>> if ( !C.libxl_defbool_val(cb) ) {
>> // report an error
>> }
>>
>> defbool_test.go can't `import "C"`, so it can't call
>> C.libxl_defbool_val(). We could make an external xenlighttest package,
>> but that wouldn't be able to call toC().
>>
>> (I suppose we could write "proxy" functions for every such function we
>> might want to check, but that seems excessive.)
>
> If by "proxy" functions you mean something like:
>
> func libxlDefboolVal(db Defbool) bool {
> return C.libxl_defbool_val(C.libxl_defbool(db))
> }
>
> I agree it could be a bit excessive. But, it might be necessary in
> order to leverage go test. And, we could make sure that those extra
> "proxy" functions are only included in test builds (maybe by making
> internal package to house them).
>
> Maybe there is a better solution to this. I'll need to think about it some
> more.
I mean, we already copy the source files over somewhere else. We could
generate ctestproxies.go, and make a xenlighttestable package which
includes that file (and the _test.go files), and a xenlight package
which doesn't.
But I'd say that's a lower priority at this point.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 02/22] golang/xenlight: define Defbool builtin type
> I implemented some tests for these methods to make sure everything
> worked as expected (they did); but there's an unexpected side-effect:
>
> - *_test.go files cannot `import "C"`
Yeah, this is unfortunate.
> - The fromC / toC methods aren't exported
>
> So it's not possible to do the following check:
>
> var b Defbool
>
> b.Set(true)
> cb, err := b.toC()
> if ( !C.libxl_defbool_val(cb) ) {
> // report an error
> }
>
> defbool_test.go can't `import "C"`, so it can't call
> C.libxl_defbool_val(). We could make an external xenlighttest package,
> but that wouldn't be able to call toC().
>
> (I suppose we could write "proxy" functions for every such function we
> might want to check, but that seems excessive.)
If by "proxy" functions you mean something like:
func libxlDefboolVal(db Defbool) bool {
return C.libxl_defbool_val(C.libxl_defbool(db))
}
I agree it could be a bit excessive. But, it might be necessary in
order to leverage go test. And, we could make sure that those extra
"proxy" functions are only included in test builds (maybe by making
internal package to house them).
Maybe there is a better solution to this. I'll need to think about it some more.
-NR
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/1] xen/blkback: Aggressively shrink page pools if a memory pressure is detected
On 05.12.19 16:09, SeongJae Park wrote:
> From: SeongJae Park
>
> Each `blkif` has a free pages pool for the grant mapping. The size of
> the pool starts from zero and be increased on demand while processing
> the I/O requests. If current I/O requests handling is finished or 100
> milliseconds has passed since last I/O requests handling, it checks and
> shrinks the pool to not exceed the size limit, `max_buffer_pages`.
>
> Therefore, `blkfront` running guests can cause a memory pressure in the
> `blkback` running guest by attaching a large number of block devices and
> inducing I/O. System administrators can avoid such problematic
> situations by limiting the maximum number of devices each guest can
> attach. However, finding the optimal limit is not so easy. Improper
> set of the limit can result in the memory pressure or a resource
> underutilization. This commit avoids such problematic situations by
> shrinking the pools aggressively (further the limit) for a while (users
> can set this duration via a module parameter) if a memory pressure is
> detected.
>
> Discussions
> ===
>
> The `blkback`'s original shrinking mechanism returns only pages in the
> pool which are not currently be used by `blkback`. In other words, the
> pages that will be shrunk are not mapped with foreign pages. Because
> this commit is changing only the shrink limit but uses the shrinking
> mechanism as is, this commit does not introduce improper mappings
> related security issues.
>
> Once a memory pressure is detected, this commit keeps the aggressive
> shrinking limit for a user-specified time duration. The duration should
> be neither too long nor too short. If it is too long, free pages pool
> shrinking overhead can reduce the I/O performance. If it is too short,
> `blkback` will not free enough pages to reduce the memory pressure.
> This commit sets the value as `10 milliseconds` by default because it is
> a short time in terms of I/O while it is a long time in terms of memory
> operations. Also, as the original shrinking mechanism works for at
> least every 100 milliseconds, this could be a somewhat reasonable
> choice. I also tested other durations (refer to the below section for
> more details) and confirmed that 10 milliseconds is the one that works
> best. That said, the proper duration depends on actual configurations
> and workloads. That's why this commit is allowing users to set it as
> their optimal value via the module parameter.
>
> Memory Pressure Test
>
>
> To show how this commit fixes the above mentioned memory pressure
> situation well, I configured a test environment on a xen-running system.
> On the `blkfront` running guest instances, I attach a large number of
> network-backed volume devices and induce I/O to those. Meanwhile, I
> measure the number of pages that swapped in and out on the `blkback`
> running guest. The test ran twice, once for the `blkback` before this
> commit and once for that after this commit. As shown below, this commit
> has dramatically reduced the memory pressure:
>
> pswpin pswpout
> before 76,672 185,799
> after 2123,325
>
> Optimal Aggressive Shrinking Duration
> -
>
> To find a best aggressive shrinking duration, I repeated the test with
> three different durations (1ms, 10ms, and 100ms). The results are as
> below:
>
> durationpswpin pswpout
> 1 852 6,424
> 10 212 3,325
> 100 203 3,340
>
> As expected, the numbers have further decreased by increasing the
> duration, but the reduction stopped from the `10ms`. Based on this
> results, I chose the default duration as 10ms.
>
> Performance Overhead Test
> =
>
> This commit could incur I/O performance degradation under severe memory
> pressure because the aggressive shrinking will require more page
> allocations per I/O. To show the overhead, I artificially made an
> aggressive pages pool shrinking situation and measured the I/O
> performance of a `blkfront` running guest.
>
> For the artificial shrinking, I set the `blkback.max_buffer_pages` using
> the `/sys/module/xen_blkback/parameters/max_buffer_pages` file. We set
> the value to `1024` and `0`. The `1024` is the default value. Setting
> the value as `0` is same to a situation doing the aggressive shrinking
> always (worst-case).
>
> For the I/O performance measurement, I use a simple `dd` command.
>
> Default Performance
> ---
>
> [dom0]# echo 1024 >
> /sys/module/xen_blkback/parameters/max_buffer_pages
> [instance]$ for i in {1..5}; do dd if=/dev/zero of=file bs=4k
> count=$((256*512)); sync; done
> 131072+0 records in
> 131072+0 records out
> 536870912 bytes (537 MB) copied, 11.7257 s, 45.8 MB/s
> 131072+0 records in
> 131072+0 records out
> 536870912 bytes (537 MB) copied, 13.8827 s, 38.7 MB/s
> 131072+0
Re: [Xen-devel] [PATCH v2 0/1] xen/blkback: Aggressively shrink page pools if a memory pressure is detected
On 05.12.19 16:09, SeongJae Park wrote: > Each `blkif` has a free pages pool for the grant mapping. The size of > the pool starts from zero and be increased on demand while processing > the I/O requests. If current I/O requests handling is finished or 100 > milliseconds has passed since last I/O requests handling, it checks and > shrinks the pool to not exceed the size limit, `max_buffer_pages`. > > Therefore, `blkfront` running guests can cause a memory pressure in the > `blkback` running guest by attaching a large number of block devices and > inducing I/O. System administrators can avoid such problematic > situations by limiting the maximum number of devices each guest can > attach. However, finding the optimal limit is not so easy. Improper > set of the limit can result in the memory pressure or a resource > underutilization. This commit avoids such problematic situations by > shrinking the pools aggressively (further the limit) for a while (users > can set this duration via a module parameter) if a memory pressure is > detected. > > > Base Version > > > This patch is based on v5.4. A complete tree is also available at my > public git repo: > https://github.com/sjp38/linux/tree/blkback_aggressive_shrinking_v2 > > > Patch History > - > > Changes from v1 > (https://lore.kernel.org/xen-devel/20191204113419.2298-1-sjp...@amazon.com/) > - Adjust the description to not use the term, `arbitrarily` (suggested >by Paul Durrant) > - Specify time unit of the duration in the parameter description, >(suggested by Maximilian Heyne) > - Change default aggressive shrinking duration from 1ms to 10ms > - Merged two patches into one single patch > > > SeongJae Park (1): > xen/blkback: Aggressively shrink page pools if a memory pressure is > detected > > drivers/block/xen-blkback/blkback.c | 35 +++-- > 1 file changed, 33 insertions(+), 2 deletions(-) CC-ing xen-devel@lists.xenproject.org Thanks, SeongJae Park > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 01/22] golang/xenlight: generate enum types from IDL
On 11/15/19 7:44 PM, Nick Rosbrook wrote: > diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile > index 0987305224..681f32c234 100644 > --- a/tools/golang/xenlight/Makefile > +++ b/tools/golang/xenlight/Makefile > @@ -7,20 +7,21 @@ GOCODE_DIR ?= $(prefix)/share/gocode/ > GOXL_PKG_DIR = /src/$(XEN_GOCODE_URL)/xenlight/ > GOXL_INSTALL_DIR = $(GOCODE_DIR)$(GOXL_PKG_DIR) > > -# PKGSOURCES: Files which comprise the distributed source package > -PKGSOURCES = xenlight.go > - > GO ?= go > > .PHONY: all > all: build > > .PHONY: package > -package: $(XEN_GOPATH)$(GOXL_PKG_DIR)$(PKGSOURCES) > +package: $(XEN_GOPATH)$(GOXL_PKG_DIR) > > -$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/$(PKGSOURCES): $(PKGSOURCES) > +$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/: %.gen.go Sorry to come back to this... I think this would be better explicitly listing out the files that are needed. As I said, the current way of doing things means gengotypes.py will *always* be re-run; and the result, when experimenting with the `typeof` before, was that my local changes to helpes.gen.py were being overwritten. Having this be `xenlight.go types.gen.go helpers.gen.go` instead means 1) gengotypes is only run when one of its inputs changes 2) you can make local changes to the generated files and have them copied over 3) The copying only happens when one of the above two things happens 4) The final compile is slightly faster because the go compiler knows that the files hasn't changed. I can change this on check-in if you're OK with it, and I end up checking part of the series in before you send v3. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] Xen Security Advisory 306 v3 (CVE-2019-19579) - Device quarantine for alternate pci assignment methods
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Xen Security Advisory CVE-2019-19579 / XSA-306 version 3 Device quarantine for alternate pci assignment methods UPDATES IN VERSION 3 CVE assigned. ISSUE DESCRIPTION = XSA-302 relies on the use of libxl's "assignable-add" feature to prepare devices to be assigned to untrusted guests. Unfortunately, this is not considered a strictly required step for device assignment. The PCI passthrough documentation on the wiki describes alternate ways of preparing devices for assignment, and libvirt uses its own ways as well. Hosts where these "alternate" methods are used will still leave the system in a vulnerable state after the device comes back from a guest. IMPACT == An untrusted domain with access to a physical device can DMA into host memory, leading to privilege escalation. VULNERABLE SYSTEMS == Only systems where guests are given direct access to physical devices capable of DMA (PCI pass-through) are vulnerable. Systems which do not use PCI pass-through are not vulnerable. Only systems which use "alternate" methods to assign devices to pciback before assignment are vulnerable. These methods include: - Assigning devices on the Linux command-line using `xen-pciback.hide` - Assigning devices via xen-pciback module parameters - Assigning devices manually via sysfs - Assigning devices using libvirt Systems which use `xl pci-assignable-add` or libxl_device_pci_assignable_add, or have the assignable state handled automatically via setting the `seize` parameter, are not affected. MITIGATION == For xl and libvirt, before assigning a device to a guest, manually run `xl pci-assignable-add`. This will quarantine the device even if the device has already been assigned to pciback by one of the alternate methods. This may also work for other libxl-based toolstacks, depending on the particular implementation. CREDITS === This issue was discovered by Marek Marczykowski-Górecki of Invisible Things Lab. RESOLUTION == Applying the appropriate attached patch resolves this issue. Note that this patch will quarantine the device after the domain is destroyed by default. It must be un-quarantined before it can be used by domain 0 again. This can be done by executing `xl pci-assignable-remove`. This will be effective even if the device was assigned to pciback with one of the alternate methods. xsa306.patch xen-unstable xsa306-4.12.patch Xen 4.12.x xsa306-4.11.patch Xen 4.11.x, Xen 4.10.x xsa306-4.9.patch Xen 4.9.x, Xen 4.8.x $ sha256sum xsa306* 07468dcdfbe34b794fd0618bce7d6d1edb6b10b234dccf1e5dd1f1120a0affe7 xsa306.meta 3534ec46f03bb8dac3011e0e3739fc75400559078e4361bbe5385d97b7892650 xsa306.patch 426e32bfa7d7787fe6778685e623966f8762857f7920443a0ca73347df9d6624 xsa306-4.9.patch b00e58c9f96b0ff654dfd4904c675a54356148af718eb9b2adca0253b900dfc1 xsa306-4.11.patch 69857d08969903452fbf009905a145e06a5aef9966e969de9fbb22e62c557ffd xsa306-4.12.patch $ DEPLOYMENT DURING EMBARGO = Deployment of the patches and/or mitigations described above (or others which are substantially similar) is permitted during the embargo, even on public-facing systems with untrusted guest users and administrators. But: Distribution of updated software is prohibited (except to other members of the predisclosure list). Predisclosure list members who wish to deploy significantly different patches and/or mitigations, please contact the Xen Project Security Team. (Note: this during-embargo deployment notice is retained in post-embargo publicly released Xen Project advisories, even though it is then no longer applicable. This is to enable the community to have oversight of the Xen Project Security Team's decisionmaking.) For more information about permissible uses of embargoed information, consult the Xen Project community's agreed Security Policy: http://www.xenproject.org/security-policy.html -BEGIN PGP SIGNATURE- iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAl3pEgkMHHBncEB4ZW4u b3JnAAoJEIP+FMlX6CvZawYIAJ1rXxormDa8TB3hgabjaFGEBtEptWEf0eI/zqxJ AC0l9TIdXSkcv2ZBFjxx3YDHetC8MjloBZOP84blVWH+Y9voOvDQPf2Q2AHEoHm7 KwEBFox8eyy0H1mKuhda+QqxO7XEuGUn0a0kxHiO1HMg7xY4FmxYv51E3B17ytAD TyDOsJq3MevQg+GNPwranDPS7UtpYKFBqEEf63KsA9bU5OS+BaAijRQ379qwh//8 bpWoEFBPRWK6Pf46iSlhifnTUDZiAVOSAxolH3b1UZKOWFaVIrLOpY49QLFg5zfC yhvCgVumONdyIX+x35kGuIDvYFbrEswFPmrn0pmXtdKyBEI= =8lme -END PGP SIGNATURE- xsa306.meta Description: Binary data xsa306.patch Description: Binary data xsa306-4.9.patch Description: Binary data xsa306-4.11.patch Description: Binary data xsa306-4.12.patch Description: Binary data ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/4] xenbus: move xenbus_dev_shutdown() into frontend code...
...and make it static
xenbus_dev_shutdown() is seemingly intended to cause clean shutdown of PV
frontends when a guest is rebooted. Indeed the function waits for a
conpletion which is only set by a call to xenbus_frontend_closed().
This patch removes the shutdown() method from backends and moves
xenbus_dev_shutdown() from xenbus_probe.c into xenbus_probe_frontend.c,
renaming it appropriately and making it static.
Signed-off-by: Paul Durrant
---
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
---
drivers/xen/xenbus/xenbus.h| 2 --
drivers/xen/xenbus/xenbus_probe.c | 23 -
drivers/xen/xenbus/xenbus_probe_backend.c | 1 -
drivers/xen/xenbus/xenbus_probe_frontend.c | 24 +-
4 files changed, 23 insertions(+), 27 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h
index d75a2385b37c..5f5b8a7d5b80 100644
--- a/drivers/xen/xenbus/xenbus.h
+++ b/drivers/xen/xenbus/xenbus.h
@@ -116,8 +116,6 @@ int xenbus_probe_devices(struct xen_bus_type *bus);
void xenbus_dev_changed(const char *node, struct xen_bus_type *bus);
-void xenbus_dev_shutdown(struct device *_dev);
-
int xenbus_dev_suspend(struct device *dev);
int xenbus_dev_resume(struct device *dev);
int xenbus_dev_cancel(struct device *dev);
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index 4461f4583476..a10311c348b9 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -281,29 +281,6 @@ int xenbus_dev_remove(struct device *_dev)
}
EXPORT_SYMBOL_GPL(xenbus_dev_remove);
-void xenbus_dev_shutdown(struct device *_dev)
-{
- struct xenbus_device *dev = to_xenbus_device(_dev);
- unsigned long timeout = 5*HZ;
-
- DPRINTK("%s", dev->nodename);
-
- get_device(>dev);
- if (dev->state != XenbusStateConnected) {
- pr_info("%s: %s: %s != Connected, skipping\n",
- __func__, dev->nodename, xenbus_strstate(dev->state));
- goto out;
- }
- xenbus_switch_state(dev, XenbusStateClosing);
- timeout = wait_for_completion_timeout(>down, timeout);
- if (!timeout)
- pr_info("%s: %s timeout closing device\n",
- __func__, dev->nodename);
- out:
- put_device(>dev);
-}
-EXPORT_SYMBOL_GPL(xenbus_dev_shutdown);
-
int xenbus_register_driver_common(struct xenbus_driver *drv,
struct xen_bus_type *bus,
struct module *owner, const char *mod_name)
diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c
b/drivers/xen/xenbus/xenbus_probe_backend.c
index b0bed4faf44c..14876faff3b0 100644
--- a/drivers/xen/xenbus/xenbus_probe_backend.c
+++ b/drivers/xen/xenbus/xenbus_probe_backend.c
@@ -198,7 +198,6 @@ static struct xen_bus_type xenbus_backend = {
.uevent = xenbus_uevent_backend,
.probe = xenbus_dev_probe,
.remove = xenbus_dev_remove,
- .shutdown = xenbus_dev_shutdown,
.dev_groups = xenbus_dev_groups,
},
};
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
b/drivers/xen/xenbus/xenbus_probe_frontend.c
index a7d90a719cea..8a1650bbe18f 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -126,6 +126,28 @@ static int xenbus_frontend_dev_probe(struct device *dev)
return xenbus_dev_probe(dev);
}
+static void xenbus_frontend_dev_shutdown(struct device *_dev)
+{
+ struct xenbus_device *dev = to_xenbus_device(_dev);
+ unsigned long timeout = 5*HZ;
+
+ DPRINTK("%s", dev->nodename);
+
+ get_device(>dev);
+ if (dev->state != XenbusStateConnected) {
+ pr_info("%s: %s: %s != Connected, skipping\n",
+ __func__, dev->nodename, xenbus_strstate(dev->state));
+ goto out;
+ }
+ xenbus_switch_state(dev, XenbusStateClosing);
+ timeout = wait_for_completion_timeout(>down, timeout);
+ if (!timeout)
+ pr_info("%s: %s timeout closing device\n",
+ __func__, dev->nodename);
+ out:
+ put_device(>dev);
+}
+
static const struct dev_pm_ops xenbus_pm_ops = {
.suspend= xenbus_dev_suspend,
.resume = xenbus_frontend_dev_resume,
@@ -146,7 +168,7 @@ static struct xen_bus_type xenbus_frontend = {
.uevent = xenbus_uevent_frontend,
.probe = xenbus_frontend_dev_probe,
.remove = xenbus_dev_remove,
- .shutdown = xenbus_dev_shutdown,
+ .shutdown = xenbus_frontend_dev_shutdown,
.dev_groups = xenbus_dev_groups,
.pm = _pm_ops,
--
2.20.1
___
Xen-devel mailing list
[Xen-devel] [PATCH 0/4] xen-blkback: support live update
Patch #1 is clean-up for an apparent mis-feature. Paul Durrant (4): xenbus: move xenbus_dev_shutdown() into frontend code... xenbus: limit when state is forced to closed xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH xen-blkback: support dynamic unbind/bind drivers/block/xen-blkback/xenbus.c | 12 drivers/xen/xenbus/xenbus.h| 2 -- drivers/xen/xenbus/xenbus_probe.c | 34 ++ drivers/xen/xenbus/xenbus_probe_backend.c | 1 - drivers/xen/xenbus/xenbus_probe_frontend.c | 24 ++- include/xen/interface/io/ring.h| 4 +-- 6 files changed, 40 insertions(+), 37 deletions(-) --- Cc: Boris Ostrovsky Cc: Jens Axboe Cc: Juergen Gross Cc: Konrad Rzeszutek Wilk Cc: "Roger Pau Monné" Cc: Stefano Stabellini -- 2.20.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to closed
Only force state to closed in the case when the toolstack may need to
clean up. This can be detected by checking whether the state in xenstore
has been set to closing prior to device removal.
Signed-off-by: Paul Durrant
---
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
---
drivers/xen/xenbus/xenbus_probe.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index a10311c348b9..c54a53da0106 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -255,7 +255,6 @@ int xenbus_dev_probe(struct device *_dev)
module_put(drv->driver.owner);
fail:
xenbus_dev_error(dev, err, "xenbus_dev_probe on %s", dev->nodename);
- xenbus_switch_state(dev, XenbusStateClosed);
return err;
}
EXPORT_SYMBOL_GPL(xenbus_dev_probe);
@@ -264,6 +263,7 @@ int xenbus_dev_remove(struct device *_dev)
{
struct xenbus_device *dev = to_xenbus_device(_dev);
struct xenbus_driver *drv = to_xenbus_driver(_dev->driver);
+ enum xenbus_state state;
DPRINTK("%s", dev->nodename);
@@ -276,7 +276,14 @@ int xenbus_dev_remove(struct device *_dev)
free_otherend_details(dev);
- xenbus_switch_state(dev, XenbusStateClosed);
+ /*
+* If the toolstack had force the device state to closing then set
+* the state to closed now to allow it to be cleaned up.
+*/
+ state = xenbus_read_driver_state(dev->nodename);
+ if (state == XenbusStateClosing)
+ xenbus_switch_state(dev, XenbusStateClosed);
+
return 0;
}
EXPORT_SYMBOL_GPL(xenbus_dev_remove);
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH
Currently these macros will skip over any requests/responses that are
added to the shared ring whilst it is detached. This, in general, is not
a desirable semantic since most frontend implementations will eventually
block waiting for a response which would either never appear or never be
processed.
NOTE: These macros are currently unused. BACK_RING_ATTACH(), however, will
be used in a subsequent patch.
Signed-off-by: Paul Durrant
---
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
---
include/xen/interface/io/ring.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 3f40501fc60b..405adfed87e6 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -143,14 +143,14 @@ struct __name##_back_ring {
\
#define FRONT_RING_ATTACH(_r, _s, __size) do { \
(_r)->sring = (_s);
\
(_r)->req_prod_pvt = (_s)->req_prod; \
-(_r)->rsp_cons = (_s)->rsp_prod; \
+(_r)->rsp_cons = (_s)->req_prod; \
(_r)->nr_ents = __RING_SIZE(_s, __size); \
} while (0)
#define BACK_RING_ATTACH(_r, _s, __size) do { \
(_r)->sring = (_s);
\
(_r)->rsp_prod_pvt = (_s)->rsp_prod; \
-(_r)->req_cons = (_s)->req_prod; \
+(_r)->req_cons = (_s)->rsp_prod; \
(_r)->nr_ents = __RING_SIZE(_s, __size); \
} while (0)
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 4/4] xen-blkback: support dynamic unbind/bind
By simply re-attaching to shared rings during connect_ring() rather than
assuming they are freshly allocated (i.e assuming the counters are zero)
it is possible for vbd instances to be unbound and re-bound from and to
(respectively) a running guest.
This has been tested by running:
while true; do dd if=/dev/urandom of=test.img bs=1M count=1024; done
in a PV guest whilst running:
while true;
do echo vbd-$DOMID-$VBD >unbind;
echo unbound;
sleep 5;
echo vbd-$DOMID-$VBD >bind;
echo bound;
sleep 3;
done
in dom0 from /sys/bus/xen-backend/drivers/vbd to continuously unbind and
re-bind its system disk image.
This is a highly useful feature for a backend module as it allows it to be
unloaded and re-loaded (i.e. updated) without requiring domUs to be halted.
This was also tested by running:
while true;
do echo vbd-$DOMID-$VBD >unbind;
echo unbound;
sleep 5;
rmmod xen-blkback;
echo unloaded;
sleep 1;
modprobe xen-blkback;
echo bound;
cd $(pwd);
sleep 3;
done
in dom0 whilst running the same loop as above in the (single) PV guest.
Some (less stressful) testing has also been done using a Windows HVM guest
with the latest 9.0 PV drivers installed.
Signed-off-by: Paul Durrant
---
Cc: Konrad Rzeszutek Wilk
Cc: "Roger Pau Monné"
Cc: Jens Axboe
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
---
drivers/block/xen-blkback/xenbus.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/block/xen-blkback/xenbus.c
b/drivers/block/xen-blkback/xenbus.c
index e8c5c54e1d26..0b82740c4a9d 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -196,24 +196,24 @@ static int xen_blkif_map(struct xen_blkif_ring *ring,
grant_ref_t *gref,
{
struct blkif_sring *sring;
sring = (struct blkif_sring *)ring->blk_ring;
- BACK_RING_INIT(>blk_rings.native, sring,
- XEN_PAGE_SIZE * nr_grefs);
+ BACK_RING_ATTACH(>blk_rings.native, sring,
+XEN_PAGE_SIZE * nr_grefs);
break;
}
case BLKIF_PROTOCOL_X86_32:
{
struct blkif_x86_32_sring *sring_x86_32;
sring_x86_32 = (struct blkif_x86_32_sring *)ring->blk_ring;
- BACK_RING_INIT(>blk_rings.x86_32, sring_x86_32,
- XEN_PAGE_SIZE * nr_grefs);
+ BACK_RING_ATTACH(>blk_rings.x86_32, sring_x86_32,
+XEN_PAGE_SIZE * nr_grefs);
break;
}
case BLKIF_PROTOCOL_X86_64:
{
struct blkif_x86_64_sring *sring_x86_64;
sring_x86_64 = (struct blkif_x86_64_sring *)ring->blk_ring;
- BACK_RING_INIT(>blk_rings.x86_64, sring_x86_64,
- XEN_PAGE_SIZE * nr_grefs);
+ BACK_RING_ATTACH(>blk_rings.x86_64, sring_x86_64,
+XEN_PAGE_SIZE * nr_grefs);
break;
}
default:
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen
I mean... I was taught so as well but I was also taught an exception which is using it for error handling and cleaning up. I am not sure if using alternatives would result in cleaner code in this situation. Hongyan On Thu, 2019-12-05 at 12:12 +0100, Jan Beulich wrote: > On 05.12.2019 12:02, Durrant, Paul wrote: > > > -Original Message- > > > From: Xen-devel On > > > Behalf Of Jan > > > Beulich > > > Sent: 05 December 2019 10:26 > > > To: Xia, Hongyan > > > Cc: andrew.coop...@citrix.com; xen-devel@lists.xenproject.org; > > > w...@xen.org; > > > roger@citrix.com > > > Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an > > > end_of_loop label > > > in map_pages_to_xen > > > > > > On 05.12.2019 11:21, Xia, Hongyan wrote: > > > > > On 02.10.2019 19:16, Hongyan Xia wrote: > > > > > > We will soon need to clean up mappings whenever the out > > > > > > most loop is > > > > > > ended. Add a new label and turn relevant continue's into > > > > > > goto's. > > > > > > > > > > I think already when this still was RFC I did indicate that > > > > > I'm not > > > > > happy about the introduction of these labels (including also > > > > > patch 8). > > > > > I realize it's quite a lot to ask, but both functions would > > > > > benefit > > > > > from splitting up into per-level helper functions, which - > > > > > afaict - > > > > > would avoid the need for such labels, and which would at the > > > > > same > > > > > time likely make it quite a bit easier to extend these to the > > > > > 5-level page tables case down the road. > > > > > > > > A common pattern I have found when mapping PTE pages on-demand > > > > (and I > > > > think is the exact intention of these labels from Wei, also > > > > described > > > > in the commit message) is that we often need to do: > > > > > > > > map some pages - process those pages - error occurs or this > > > > iteration > > > > of loop can be skipped - _clean up the mappings_ - continue or > > > > return > > > > > > > > As long as cleaning up is required, these labels will likely be > > > > needed > > > > as the clean-up path before skipping or returning, so I would > > > > say we > > > > will see such labels even if we split it into helper functions > > > > (virt_to_xen_l[123]e() later in the patch series is an > > > > example). I see > > > > the labels more or less as orthogonal to modularising into > > > > helper > > > > functions. > > > > > > I think differently: The fact that labels are needed is because > > > of > > > the complexity of the functions. Simpler functions would allow > > > goto-free handling of such error conditions (by instead being > > > able > > > to use continue, break, or return without making the code less > > > readable, often even improving readability). > > > > And what is wrong with using goto-s? It is a *very* common style of > > error handling use widely in e.g. the linux kernel. IMO it often > > makes error paths much more obvious and easier to reason about. In > > fact I very much dislike returns from the middle of functions as > > they can easily lead to avoidance of necessary error cleanup. > > Whereas I personally dislike goto-s (and I've been taught so when > first learning programming languages). In private code I avoid them > by all means. In projects I'm the maintainer for I accept them when > the alternative is noticeably more ugly. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Community Call: Call for Agenda Items and call details for Thursday Dec 5, 16:00 - 17:00 UTC
Hi all, a quick note to say that George may start the call: however, the call-in details remain the same. We have a scheduled power outage this morning, which I was only informed about yesterday. As the power outage affects almost the entire pacific coast in CR, it is conceivable that I won't have a data connection via 4G either. In any case, this is just a quick note: most likely all will work as usual. Best Regards Lars On 03/12/2019, 06:21, "Lars Kurth" wrote: Correction: the meeting is this Thursday, the 5th Apologies for the typo Lars On 02/12/2019, 20:05, "Lars Kurth" wrote: Dear community members, please send me agenda items for this Friday’s community call (sorry for the late notice, I was on PTO last week). A draft agenda is at https://cryptpad.fr/pad/#/2/pad/edit/PCtBphoXkCTiXABJ8cdL0KuZ/ Please add agenda items to the document or reply to this e-mail @Juergen: I added a slot re the 4.13 release @Doug: I saw some activity recently about the CI Loop stuff - maybe worth discussing, if you have time @Ian: you mentioned that you wanted to find a way to get sysadmin help from someone in the community to help maintain test infra - I wanted to run this past this group first to see whether any names come to mind. The required skillset is likely to be different to that of a developer UPDATE: Paul Durrant will be release manager for 4.14 - congratulations Last month’s minutes are at https://cryptpad.fr/pad/#/2/pad/view/7l3a4mhZTU4xs0GE415OXiAj0ScKl39xdQ9wm0cwASs/ Best Regards Lars ## Meeting time (please double check the times) 16:00 - 17:00 UTC 08:00 - 09:00 PST (San Francisco) - sorry for the early time slot. If this is a problem, let's discuss at the call 10:00 - 11:00 CST (Austin, Costa Rica) 11:00 - 12:00 EST (New York) 16:00 - 17:00 FMT (London) 17:00 - 18:00 CET (Berlin) 00:00 - 01:00+1 CST (Beijing) Further International meeting times: https://www.timeanddate.com/worldclock/meetingdetails.html?year=2019=12=5=16=0=0=224=24=179=136=37=33 ## Dial in details Web: https://www.gotomeet.me/larskurth You can also dial in using your phone Access Code: 906-886-965 China (Toll Free): 4008 811084 Germany: +49 692 5736 7317 Poland (Toll Free): 00 800 1124759 United Kingdom: +44 330 221 0088 United States: +1 (571) 317-3129 More phone numbers Australia: +61 2 9087 3604 Austria: +43 7 2081 5427 Argentina (Toll Free): 0 800 444 3375 Bahrain (Toll Free): 800 81 111 Belarus (Toll Free): 8 820 0011 0400 Belgium: +32 28 93 7018 Brazil (Toll Free): 0 800 047 4906 Bulgaria (Toll Free): 00800 120 4417 Canada: +1 (647) 497-9391 Chile (Toll Free): 800 395 150 Colombia (Toll Free): 01 800 518 4483 Czech Republic (Toll Free): 800 500448 Denmark: +45 32 72 03 82 Finland: +358 923 17 0568 France: +33 170 950 594 Greece (Toll Free): 00 800 4414 3838 Hong Kong (Toll Free): 30713169 Hungary (Toll Free): (06) 80 986 255 Iceland (Toll Free): 800 7204 India (Toll Free): 18002669272 Indonesia (Toll Free): 007 803 020 5375 Ireland: +353 15 360 728 Israel (Toll Free): 1 809 454 830 Italy: +39 0 247 92 13 01 Japan (Toll Free): 0 120 663 800 Korea, Republic of (Toll Free): 00798 14 207 4914 Luxembourg (Toll Free): 800 85158 Malaysia (Toll Free): 1 800 81 6854 Mexico (Toll Free): 01 800 522 1133 Netherlands: +31 207 941 377 New Zealand: +64 9 280 6302 Norway: +47 21 93 37 51 Panama (Toll Free): 00 800 226 7928 Peru (Toll Free): 0 800 77023 Philippines (Toll Free): 1 800 1110 1661 Portugal (Toll Free): 800 819 575 Romania (Toll Free): 0 800 410 029 Russian Federation (Toll Free): 8 800 100 6203 Saudi Arabia (Toll Free): 800 844 3633 Singapore (Toll Free): 18007231323 South Africa (Toll Free): 0 800 555 447 Spain: +34 932 75 2004 Sweden: +46 853 527 827 Switzerland: +41 225 4599 78 Taiwan (Toll Free): 0 800 666 854 Thailand (Toll Free): 001 800 011 023 Turkey (Toll Free): 00 800 4488 23683 Ukraine (Toll Free): 0 800 50 1733 United Arab Emirates (Toll Free): 800 044 40439 Uruguay (Toll Free): 0004 019 1018 Viet Nam (Toll Free): 122 80 481 First GoToMeeting? Let's do a quick system check: https://link.gotomeeting.com/system-check ___ Xen-devel mailing list
Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
On 04/12/2019 09:43, Andrew Cooper wrote:
> The type name is poor because the type is also used for the IDT vectoring
> field, not just for the event injection field. Rename it to intinfo_t which
> is how the APM refers to the data.
>
> Rearrange the union to drop the .fields infix, and rename bytes to the more
> common raw.
>
> While adjusting all call sites, fix up style issues and make use of structure
> assignments where applicable.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> ---
> xen/arch/x86/hvm/svm/intr.c| 32 --
> xen/arch/x86/hvm/svm/nestedsvm.c | 28 +++-
> xen/arch/x86/hvm/svm/svm.c | 68
> ++
> xen/arch/x86/hvm/svm/svmdebug.c| 12 +++
> xen/include/asm-x86/hvm/svm/vmcb.h | 22 ++--
> 5 files changed, 75 insertions(+), 87 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
> index ff755165cd..4eede5cc23 100644
> --- a/xen/arch/x86/hvm/svm/intr.c
> +++ b/xen/arch/x86/hvm/svm/intr.c
> @@ -43,15 +43,13 @@ static void svm_inject_nmi(struct vcpu *v)
> {
> struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> u32 general1_intercepts = vmcb_get_general1_intercepts(vmcb);
> -eventinj_t event;
>
> -event.bytes = 0;
> -event.fields.v = 1;
> -event.fields.type = X86_EVENTTYPE_NMI;
> -event.fields.vector = 2;
> -
> -ASSERT(vmcb->eventinj.fields.v == 0);
> -vmcb->eventinj = event;
> +ASSERT(!vmcb->eventinj.v);
> +vmcb->eventinj = (intinfo_t){
> +.vector = 2,
> +.type = X86_EVENTTYPE_NMI,
> +.v = true,
> +};
And in a surprise move (not really), CentOS 6 isn't happy with this.
I'll revert back to the previous way of filling in eventinj (until we
can actually decide on a newer tools baseline).
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] passthrough: drop break statement following c/s cd7dedad820
On 05.12.2019 13:20, Durrant, Paul wrote: >> -Original Message- >> From: Igor Druzhinin >> Sent: 05 December 2019 12:14 >> To: xen-devel@lists.xenproject.org >> Cc: jbeul...@suse.com; li...@eikelenboom.it; Durrant, Paul >> ; Igor Druzhinin >> Subject: [PATCH v2] passthrough: drop break statement following c/s >> cd7dedad820 >> >> The locking responsibilities have changed and a premature break in >> this section now causes the following assertion: >> >> Assertion '!preempt_count()' failed at preempt.c:36 >> >> Suggested-by: Paul Durrant > > Actually, it was suggested by Jan, but you can put my R-b on the patch. And mine: Reviewed-by: Jan Beulich I'll get this committed soon. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] passthrough: drop break statement following c/s cd7dedad820
On 05/12/2019 12:20, Durrant, Paul wrote: >> -Original Message- >> From: Igor Druzhinin >> Sent: 05 December 2019 12:14 >> To: xen-devel@lists.xenproject.org >> Cc: jbeul...@suse.com; li...@eikelenboom.it; Durrant, Paul >> ; Igor Druzhinin >> Subject: [PATCH v2] passthrough: drop break statement following c/s >> cd7dedad820 >> >> The locking responsibilities have changed and a premature break in >> this section now causes the following assertion: >> >> Assertion '!preempt_count()' failed at preempt.c:36 >> >> Suggested-by: Paul Durrant > > Actually, it was suggested by Jan, but you can put my R-b on the patch. > Oh, indeed :) Please fix up while committing as you wish. Igor ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 16/22] golang/xenlight: implement keyed union C to Go marshaling
On 12/4/19 6:40 PM, George Dunlap wrote:
> On 11/15/19 7:44 PM, Nick Rosbrook wrote:
>> From: Nick Rosbrook
>>
>> Switch over union key to determine how to populate 'union' in Go struct.
>>
>> Since the unions of C types cannot be directly accessed, add C structs in
>> cgo preamble to assist in marshaling keyed unions. This allows the C
>> type defined in the preamble to be populated first, and then accessed
>> directly to populate the Go struct.
>
> Blech. :-(
>
>> +def xenlight_golang_union_fields_from_C(ty = None):
>> +s = ''
>> +
>> +for f in ty.fields:
>> +gotypename = xenlight_golang_fmt_name(f.type.typename)
>> +ctypename = f.type.typename
>> +gofname= xenlight_golang_fmt_name(f.name)
>> +cfname = f.name
>> +
>> +is_castable = (f.type.json_parse_type == 'JSON_INTEGER' or
>> + isinstance(f.type, idl.Enumeration) or
>> + gotypename in go_builtin_types)
>> +
>> +if not is_castable:
>> +s += 'if err := x.{}.fromC({});'.format(gofname,cfname)
>> +s += 'err != nil {\n return err \n}\n'
>> +
>> +# We just did an unsafe.Pointer cast from []byte to the 'union' type
>> +# struct, so we need to make sure that any string fields are
>> actually
>> +# converted properly.
>> +elif gotypename == 'string':
>> +s += 'x.{} = C.GoString(tmp.{})\n'.format(gofname,cfname)
>> +
>> +else:
>> +s += 'x.{} = {}(tmp.{})\n'.format(gofname,gotypename,cfname)
>
> It looks like this is duplicating (differently!) the field-copying code
> from golang_define_from_C. Is there any reason you couldn't have a
> single function, `xenlight_golang_fields_from_C`, which would be used
> for both?
>
>
>> +typedef struct libxl_channelinfo_connection_union_pty {
>> +char * path;
>> +} libxl_channelinfo_connection_union_pty;
>
> It would be nice if there were some way we could verify that the
> structures generated here matched the C unions generated. It would
> stink pretty badly if they drifted and nobody noticed until we started
> getting weird errors.
>
> We don't have to solve it now, but let's put it on the to-do list and
> have a think about it.
Actually, it turns out we don't strictly need to duplicate this at all,
if we use the `typeof` operator, like this:
---
typedef typeof(((struct libxl_channelinfo *)NULL)->u.pty)
libxl_channelinfo_connection_union_pty;
typedef typeof(((struct libxl_domain_build_info *)NULL)->u.hvm)
libxl_domain_build_info_type_union_hvm;
typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pv)
libxl_domain_build_info_type_union_pv;
typedef typeof(((struct libxl_domain_build_info *)NULL)->u.pvh)
libxl_domain_build_info_type_union_pvh;
typedef typeof(((struct libxl_device_usbdev *)NULL)->u.hostdev)
libxl_device_usbdev_type_union_hostdev;
typedef typeof(((struct libxl_device_channel *)NULL)->u.socket)
libxl_device_channel_connection_union_socket;
---
This guarantees we'll have the correct layout for the resulting type.
I talked to Ian Jackson, and he agreed that long-term it would be good
for the C generator to generate named types for these union elements
(likke you have here). If you felt really motivated you could do that
now; but I think using the `typeof` trick would be suitable to get this
patch in.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] passthrough: drop break statement following c/s cd7dedad820
> -Original Message- > From: Igor Druzhinin > Sent: 05 December 2019 12:14 > To: xen-devel@lists.xenproject.org > Cc: jbeul...@suse.com; li...@eikelenboom.it; Durrant, Paul > ; Igor Druzhinin > Subject: [PATCH v2] passthrough: drop break statement following c/s > cd7dedad820 > > The locking responsibilities have changed and a premature break in > this section now causes the following assertion: > > Assertion '!preempt_count()' failed at preempt.c:36 > > Suggested-by: Paul Durrant Actually, it was suggested by Jan, but you can put my R-b on the patch. Paul > Reported-by: Sander Eikelenboom > Signed-off-by: Igor Druzhinin > --- > xen/drivers/passthrough/pci.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index ced0c28..c07a639 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1705,7 +1705,6 @@ int iommu_do_pci_domctl( > seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > ret = -EINVAL; > } > -break; > } > else if ( !ret ) > ret = assign_device(d, seg, bus, devfn, flags); > -- > 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] passthrough: drop break statement following c/s cd7dedad820
The locking responsibilities have changed and a premature break in this section now causes the following assertion: Assertion '!preempt_count()' failed at preempt.c:36 Suggested-by: Paul Durrant Reported-by: Sander Eikelenboom Signed-off-by: Igor Druzhinin --- xen/drivers/passthrough/pci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index ced0c28..c07a639 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1705,7 +1705,6 @@ int iommu_do_pci_domctl( seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); ret = -EINVAL; } -break; } else if ( !ret ) ret = assign_device(d, seg, bus, devfn, flags); -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Community Call: Call for Agenda Items and call details for Dec 4, 16:00 - 17:00 UTC
On 03.12.19 03:05, Lars Kurth wrote: Dear community members, please send me agenda items for this Friday’s community call (sorry for the late notice, I was on PTO last week). A draft agenda is at https://cryptpad.fr/pad/#/2/pad/edit/PCtBphoXkCTiXABJ8cdL0KuZ/ Please add agenda items to the document or reply to this e-mail @Juergen: I added a slot re the 4.13 release Sorry, I can't make it today. I just broke off a part of a tooth and I'm at the dentist this afternoon. There isn't much to say about 4.13: I expect one more RC next week and I'm hoping for a release the week after. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
04.12.2019 17:59, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy writes:
>
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>
> I get what you mean, but I have plenty of context.
>
>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>> local_err+error_propagate pattern, which will definitely fix the issue)
>
> The parenthesis is not part of the goal.
>
>> [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, we need to add invocation of the macro at start
>> of functions, which needs error_prepend/error_append_hint (1.); add
>> invocation of the macro at start of functions which do
>> local_err+error_propagate scenario the check errors, drop local errors
>> from them and just use *errp instead (2., 3.).
>
> The paragraph talks about two cases: 1. and 2.+3.
Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
fix achieve 2 and 3 by one action.
> Makes me think we
> want two paragraphs, each illustrated with an example.
>
> What about you provide the examples, and then I try to polish the prose?
1: error_fatal problem
Assume the following code flow:
int f1(errp) {
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
Now, if we call f1 with _fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.
So, in this case we should use local_err.
2: error_abort problem
Now, consider functions without return value. We normally use local_err
variable to catch failures:
void f1(errp) {
Error *local_err = NULL;
...
f2(_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
...
}
Now, if we call f2 with _abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.
So, we should never wrap error_abort by local_err.
===
Our solution:
- Fixes [1.], adding invocation of new macro into functions with
error_appen_hint/error_prepend,
New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which
never
wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
propagations
(in fact, error_propagate is called, but returns immediately on first if
(!local_err))
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> Reviewed-by: Eric Blake
>> ---
>>
>> CC: Gerd Hoffmann
>> CC: "Gonglei (Arei)"
>> CC: Eduardo Habkost
>> CC: Igor Mammedov
>> CC: Laurent Vivier
>> CC: Amit Shah
>> CC: Kevin Wolf
>> CC: Max Reitz
>> CC: John Snow
>> CC: Ari Sundholm
>> CC: Pavel Dovgalyuk
>> CC: Paolo Bonzini
>> CC: Stefan Hajnoczi
>> CC: Fam Zheng
>> CC: Stefan Weil
>> CC: Ronnie Sahlberg
>> CC: Peter Lieven
>> CC: Eric Blake
>> CC: "Denis V. Lunev"
>> CC: Markus Armbruster
>> CC: Alberto Garcia
>> CC: Jason Dillaman
>> CC: Wen Congyang
>> CC: Xie Changlong
>> CC: Liu Yuan
>> CC: "Richard W.M. Jones"
>> CC: Jeff Cody
>> CC: "Marc-André Lureau"
>> CC: "Daniel P. Berrangé"
>> CC: Richard Henderson
>> CC: Greg Kurz
>> CC: "Michael S. Tsirkin"
>> CC: Marcel Apfelbaum
>> CC: Beniamino Galvani
>> CC: Peter Maydell
>> CC: "Cédric Le Goater"
>> CC: Andrew Jeffery
>> CC: Joel Stanley
>> CC: Andrew Baumann
>> CC: "Philippe Mathieu-Daudé"
>> CC: Antony Pavlov
>> CC: Jean-Christophe Dubois
>> CC: Peter Chubb
>> CC: Subbaraya Sundeep
>> CC: Eric Auger
>> CC: Alistair Francis
>> CC: "Edgar E. Iglesias"
>> CC: Stefano Stabellini
>> CC: Anthony Perard
>> CC: Paul Durrant
>> CC: Paul Burton
>> CC: Aleksandar Rikalo
>> CC: Chris Wulff
>> CC: Marek Vasut
>> CC: David Gibson
>> CC: Cornelia Huck
>> CC: Halil Pasic
>> CC: Christian Borntraeger
>> CC: "Hervé Poussineau"
>> CC: Xiao Guangrong
>> CC: Aurelien Jarno
>> CC: Aleksandar Markovic
>> CC: Mark Cave-Ayland
>> CC: Jason Wang
>> CC: Laszlo Ersek
>> CC: Yuval Shaia
>> CC: Palmer Dabbelt
>> CC: Sagar Karandikar
>> CC: Bastian Koppelmann
>> CC: David Hildenbrand
>> CC: Thomas Huth
>> CC: Eric Farman
Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains
On 02.08.2019 11:22, Roger Pau Monne wrote:
> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> establish RMRR mappings for PV domains, like it's done in
> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> not properly updating the iommu page tables.
Aiui we still didn't get to the bottom of this. Together with
my (much) earlier reply I think I'll drop this from my list
of pending patches, unless you indicate otherwise.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen
On 05.12.2019 12:02, Durrant, Paul wrote: >> -Original Message- >> From: Xen-devel On Behalf Of Jan >> Beulich >> Sent: 05 December 2019 10:26 >> To: Xia, Hongyan >> Cc: andrew.coop...@citrix.com; xen-devel@lists.xenproject.org; w...@xen.org; >> roger@citrix.com >> Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label >> in map_pages_to_xen >> >> On 05.12.2019 11:21, Xia, Hongyan wrote: On 02.10.2019 19:16, Hongyan Xia wrote: > We will soon need to clean up mappings whenever the out most loop is > ended. Add a new label and turn relevant continue's into goto's. I think already when this still was RFC I did indicate that I'm not happy about the introduction of these labels (including also patch 8). I realize it's quite a lot to ask, but both functions would benefit >>> >from splitting up into per-level helper functions, which - afaict - would avoid the need for such labels, and which would at the same time likely make it quite a bit easier to extend these to the 5-level page tables case down the road. >>> >>> A common pattern I have found when mapping PTE pages on-demand (and I >>> think is the exact intention of these labels from Wei, also described >>> in the commit message) is that we often need to do: >>> >>> map some pages - process those pages - error occurs or this iteration >>> of loop can be skipped - _clean up the mappings_ - continue or return >>> >>> As long as cleaning up is required, these labels will likely be needed >>> as the clean-up path before skipping or returning, so I would say we >>> will see such labels even if we split it into helper functions >>> (virt_to_xen_l[123]e() later in the patch series is an example). I see >>> the labels more or less as orthogonal to modularising into helper >>> functions. >> >> I think differently: The fact that labels are needed is because of >> the complexity of the functions. Simpler functions would allow >> goto-free handling of such error conditions (by instead being able >> to use continue, break, or return without making the code less >> readable, often even improving readability). > > And what is wrong with using goto-s? It is a *very* common style of > error handling use widely in e.g. the linux kernel. IMO it often > makes error paths much more obvious and easier to reason about. In > fact I very much dislike returns from the middle of functions as > they can easily lead to avoidance of necessary error cleanup. Whereas I personally dislike goto-s (and I've been taught so when first learning programming languages). In private code I avoid them by all means. In projects I'm the maintainer for I accept them when the alternative is noticeably more ugly. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen
> -Original Message- > From: Xen-devel On Behalf Of Jan > Beulich > Sent: 05 December 2019 10:26 > To: Xia, Hongyan > Cc: andrew.coop...@citrix.com; xen-devel@lists.xenproject.org; w...@xen.org; > roger@citrix.com > Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label > in map_pages_to_xen > > On 05.12.2019 11:21, Xia, Hongyan wrote: > >> On 02.10.2019 19:16, Hongyan Xia wrote: > >>> We will soon need to clean up mappings whenever the out most loop is > >>> ended. Add a new label and turn relevant continue's into goto's. > >> > >> I think already when this still was RFC I did indicate that I'm not > >> happy about the introduction of these labels (including also patch 8). > >> I realize it's quite a lot to ask, but both functions would benefit > >>from splitting up into per-level helper functions, which - afaict - > >> would avoid the need for such labels, and which would at the same > >> time likely make it quite a bit easier to extend these to the > >> 5-level page tables case down the road. > > > > A common pattern I have found when mapping PTE pages on-demand (and I > > think is the exact intention of these labels from Wei, also described > > in the commit message) is that we often need to do: > > > > map some pages - process those pages - error occurs or this iteration > > of loop can be skipped - _clean up the mappings_ - continue or return > > > > As long as cleaning up is required, these labels will likely be needed > > as the clean-up path before skipping or returning, so I would say we > > will see such labels even if we split it into helper functions > > (virt_to_xen_l[123]e() later in the patch series is an example). I see > > the labels more or less as orthogonal to modularising into helper > > functions. > > I think differently: The fact that labels are needed is because of > the complexity of the functions. Simpler functions would allow > goto-free handling of such error conditions (by instead being able > to use continue, break, or return without making the code less > readable, often even improving readability). And what is wrong with using goto-s? It is a *very* common style of error handling use widely in e.g. the linux kernel. IMO it often makes error paths much more obvious and easier to reason about. In fact I very much dislike returns from the middle of functions as they can easily lead to avoidance of necessary error cleanup. Paul > > Jan > > ___ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [libvirt bisection] complete build-amd64-libvirt
branch xen-unstable xenbranch xen-unstable job build-amd64-libvirt testid libvirt-build Tree: libvirt git://libvirt.org/libvirt.git Tree: libvirt_gnulib https://git.savannah.gnu.org/git/gnulib.git/ Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: libvirt git://libvirt.org/libvirt.git Bug introduced: c7f75bf04d07506bd4d9e862b9b38a1e423d88b6 Bug not present: bfe9f25b49827f02027b5a5e88226ce933e1bd7c Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/144540/ commit c7f75bf04d07506bd4d9e862b9b38a1e423d88b6 Author: Daniel P. Berrangé Date: Fri Oct 18 14:18:36 2019 +0100 docs: introduce rst2html as a mandatory tool for building docs The rst2html tool is provided by python docutils, and as the name suggests, it converts RST documents into HTML. Basic rules are added for integrating RST docs into the website build process. This enables us to start writing docs on our website in RST format instead of HTML, without changing the rest of our website templating system away from XSLT yet. Reviewed-by: Michal Privoznik Signed-off-by: Daniel P. Berrangé For bisection revision-tuple graph see: http://logs.test-lab.xenproject.org/osstest/results/bisect/libvirt/build-amd64-libvirt.libvirt-build.html Revision IDs in each graph node refer, respectively, to the Trees above. Running cs-bisection-step --graph-out=/home/logs/results/bisect/libvirt/build-amd64-libvirt.libvirt-build --summary-out=tmp/144540.bisection-summary --basis-template=144517 --blessings=real,real-bisect libvirt build-amd64-libvirt libvirt-build Searching for failure / basis pass: 144526 fail [host=godello0] / 144517 ok. Failure / basis pass flights: 144526 / 144517 (tree with no url: minios) Tree: libvirt git://libvirt.org/libvirt.git Tree: libvirt_gnulib https://git.savannah.gnu.org/git/gnulib.git/ Tree: libvirt_keycodemapdb https://gitlab.com/keycodemap/keycodemapdb.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://xenbits.xen.org/qemu-xen.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git Latest 01bf0bafceb5fc9f12ddee23166ceafed9e951cf 1f6fb368c04919243e2c70f2aa514a5f88e95309 6280c94f306df6a20bbc100ba15a5a81af0366e6 c9416efeef0d4a0554db01f3fd1cdaede14856d7 d0d8ad39ecb51cd7497cd524484fe09f50876798 933ebad2470a169504799a1d95b8e410bd9847ef c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d 05de315b00bf2951617b8ef28811b1f1f2dd5742 Basis pass d0d728c7c00fd3a62731e50c7bc646df323c0622 1f6fb368c04919243e2c70f2aa514a5f88e95309 6280c94f306df6a20bbc100ba15a5a81af0366e6 4d613feee57ebd4680f3c23398a9b33723f29fd6 d0d8ad39ecb51cd7497cd524484fe09f50876798 933ebad2470a169504799a1d95b8e410bd9847ef c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d 42c8cdc039d6dc7d6aea8008bb24622eaf4b7bc8 Generating revisions with ./adhoc-revtuple-generator git://libvirt.org/libvirt.git#d0d728c7c00fd3a62731e50c7bc646df323c0622-01bf0bafceb5fc9f12ddee23166ceafed9e951cf https://git.savannah.gnu.org/git/gnulib.git/#1f6fb368c04919243e2c70f2aa514a5f88e95309-1f6fb368c04919243e2c70f2aa514a5f88e95309 https://gitlab.com/keycodemap/keycodemapdb.git#6280c94f306df6a20bbc100ba15a5a81af0366e6-6280c94f306df6a20bbc100ba15a5a81af0366e6 git://xenbits.xen.org/osstest/ovmf.git#4d613feee57ebd4680f3c23398a9b33723f29fd\ 6-c9416efeef0d4a0554db01f3fd1cdaede14856d7 git://xenbits.xen.org/qemu-xen-traditional.git#d0d8ad39ecb51cd7497cd524484fe09f50876798-d0d8ad39ecb51cd7497cd524484fe09f50876798 git://xenbits.xen.org/qemu-xen.git#933ebad2470a169504799a1d95b8e410bd9847ef-933ebad2470a169504799a1d95b8e410bd9847ef git://xenbits.xen.org/osstest/seabios.git#c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d-c9ba5276e3217ac6a1ec772dbebf568ba3a8a55d git://xenbits.xen.org/xen.git#42c8cdc039d6dc7d6aea8008bb24622eaf4b7bc8-05de315b00bf2951\ 617b8ef28811b1f1f2dd5742 Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. error: The last gc run reported the following. Please correct the root cause and remove gc.log. Automatic cleanup will not be performed until the file is removed. warning: There are too many unreachable loose objects; run 'git prune' to remove them. Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. error: The last gc run reported the following. Please correct the root cause and remove gc.log. Automatic cleanup will not be performed until the file is removed.
Re: [Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm()
On 05.12.2019 11:51, Andrew Cooper wrote: > The function is init, so can use boot_cpu_data directly. > > There is no need to write 0 to svm_feature_flags in the case of a CPUID > mismatch (not least because this is dead code on real hardware), and no need > to use locked atomic operations. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 5/4] x86/svm: Minor cleanup to start_svm()
The function is init, so can use boot_cpu_data directly.
There is no need to write 0 to svm_feature_flags in the case of a CPUID
mismatch (not least because this is dead code on real hardware), and no need
to use locked atomic operations.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
---
xen/arch/x86/hvm/svm/svm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index af3d45fe56..806bf91171 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1632,14 +1632,14 @@ const struct hvm_function_table * __init start_svm(void)
setup_vmcb_dump();
-svm_feature_flags = (current_cpu_data.extended_cpuid_level >= 0x800A ?
- cpuid_edx(0x800A) : 0);
+if ( boot_cpu_data.extended_cpuid_level >= 0x800a )
+svm_feature_flags = cpuid_edx(0x800a);
printk("SVM: Supported advanced features:\n");
/* DecodeAssists fast paths assume nextrip is valid for fast rIP update. */
if ( !cpu_has_svm_nrips )
-clear_bit(SVM_FEATURE_DECODEASSISTS, _feature_flags);
+__clear_bit(SVM_FEATURE_DECODEASSISTS, _feature_flags);
if ( cpu_has_tsc_ratio )
svm_function_table.tsc_scaling.ratio_frac_bits = 32;
--
2.11.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs
Okay let me be explicit this time. Cross checked with mails from lore.kernel.org, issues not touched from v3 to v4: - _new or not _new - splitting map_pages_to_xen; introduction of labels. Although I just responded to these two issues. - const not added to suggested variables since a lot of them are stuck with the old API for now. I can review relevant functions and const qualify other applicable ones. Hongyan On Thu, 2019-12-05 at 10:51 +0100, Jan Beulich wrote: > On 05.12.2019 10:41, Xia, Hongyan wrote: > > I have addressed the comments that I can find in the archive. > > That's still pretty vague - is there reason to assume you were > not able to find some comments there? > > > Some big > > debates (like _new or not _new, whether to modularise > > map_pages_to_xen) > > have not been touched. > > The _new suffix discussion you've meanwhile responded you, > which is therefore fine. The modularization question, otoh, > I don't recall seeing any reply for, and hence for now I'd > skip re-reviewing the respective patches. Furthermore, is > your use of "like" implying there were more than the two > examples you gave? As much as I can understand difficulties > on your part resulting from your email issues, please also > understand my reservations regarding having to re-do things > where quite a bit of time had already been invested into. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen
On 05.12.2019 11:21, Xia, Hongyan wrote: >> On 02.10.2019 19:16, Hongyan Xia wrote: >>> We will soon need to clean up mappings whenever the out most loop is >>> ended. Add a new label and turn relevant continue's into goto's. >> >> I think already when this still was RFC I did indicate that I'm not >> happy about the introduction of these labels (including also patch 8). >> I realize it's quite a lot to ask, but both functions would benefit >>from splitting up into per-level helper functions, which - afaict - >> would avoid the need for such labels, and which would at the same >> time likely make it quite a bit easier to extend these to the >> 5-level page tables case down the road. > > A common pattern I have found when mapping PTE pages on-demand (and I > think is the exact intention of these labels from Wei, also described > in the commit message) is that we often need to do: > > map some pages - process those pages - error occurs or this iteration > of loop can be skipped - _clean up the mappings_ - continue or return > > As long as cleaning up is required, these labels will likely be needed > as the clean-up path before skipping or returning, so I would say we > will see such labels even if we split it into helper functions > (virt_to_xen_l[123]e() later in the patch series is an example). I see > the labels more or less as orthogonal to modularising into helper > functions. I think differently: The fact that labels are needed is because of the complexity of the functions. Simpler functions would allow goto-free handling of such error conditions (by instead being able to use continue, break, or return without making the code less readable, often even improving readability). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label in map_pages_to_xen
>On 02.10.2019 19:16, Hongyan Xia wrote: >> We will soon need to clean up mappings whenever the out most loop is >> ended. Add a new label and turn relevant continue's into goto's. > >I think already when this still was RFC I did indicate that I'm not >happy about the introduction of these labels (including also patch 8). >I realize it's quite a lot to ask, but both functions would benefit >from splitting up into per-level helper functions, which - afaict - >would avoid the need for such labels, and which would at the same >time likely make it quite a bit easier to extend these to the >5-level page tables case down the road. > >Thoughts? > >Jan A common pattern I have found when mapping PTE pages on-demand (and I think is the exact intention of these labels from Wei, also described in the commit message) is that we often need to do: map some pages - process those pages - error occurs or this iteration of loop can be skipped - _clean up the mappings_ - continue or return As long as cleaning up is required, these labels will likely be needed as the clean-up path before skipping or returning, so I would say we will see such labels even if we split it into helper functions (virt_to_xen_l[123]e() later in the patch series is an example). I see the labels more or less as orthogonal to modularising into helper functions. I would also like to see some helpers because these functions are getting too long and convoluted, but I wonder if they could be another mini-series outside the directmap-removal series. Hongyan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable test] 144525: tolerable FAIL - PUSHED
flight 144525 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/144525/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 144522 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail blocked in 144522 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144522 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144522 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144522 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144522 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 144522 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144522 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144522 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 144522 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144522 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-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-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 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 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-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 13 migrate-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-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen ad5c7c162519a3f96561ea4791da1319d9bfdfed baseline version: xen 05de315b00bf2951617b8ef28811b1f1f2dd5742 Last test of basis 144522 2019-12-04 18:37:22 Z0 days Testing same since 144525 2019-12-05 02:49:37 Z0 days1 attempts People who touched revisions under test: Adrian Pop Andrew Cooper Jan Beulich Razvan Cojocaru jobs:
Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs
On 05.12.2019 10:41, Xia, Hongyan wrote: > I have addressed the comments that I can find in the archive. That's still pretty vague - is there reason to assume you were not able to find some comments there? > Some big > debates (like _new or not _new, whether to modularise map_pages_to_xen) > have not been touched. The _new suffix discussion you've meanwhile responded you, which is therefore fine. The modularization question, otoh, I don't recall seeing any reply for, and hence for now I'd skip re-reviewing the respective patches. Furthermore, is your use of "like" implying there were more than the two examples you gave? As much as I can understand difficulties on your part resulting from your email issues, please also understand my reservations regarding having to re-do things where quite a bit of time had already been invested into. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 4/4] x86/apic: allow enabling x2APIC mode regardless of interrupt remapping
On 04.12.2019 17:20, Roger Pau Monne wrote:
> x2APIC mode doesn't mandate interrupt remapping, and hence can be
> enabled independently. This patch enables x2APIC when available,
> regardless of whether there's interrupt remapping support.
>
> This is beneficial specially when running on virtualized environments,
> since it reduces the amount of vmexits. For example when sending an
> IPI in xAPIC mode Xen performs at least 3 different accesses to the
> APIC MMIO region, while when using x2APIC mode a single wrmsr is used.
>
> The following numbers are from a lock profiling of a Xen PV shim
> running a Linux PV kernel with 32 vCPUs and xAPIC mode:
>
> (XEN) Global lock flush_lock: addr=82d0804af1c0, lockval=03190319, not
> locked
> (XEN) lock:656153(892606463454), block:602183(9495067321843)
>
> Average lock time: 1360363ns
> Average block time: 15767743ns
>
> While the following are from the same configuration but with the shim
> using x2APIC mode:
>
> (XEN) Global lock flush_lock: addr=82d0804b01c0, lockval=1adb1adb, not
> locked
> (XEN) lock:1841883(1375128998543), block:1658716(10193054890781)
>
> Average lock time: 746588ns
> Average block time: 6145147ns
>
> Enabling x2APIC has halved the average lock time, thus reducing
> contention.
>
> Signed-off-by: Roger Pau Monné
> ---
> Changes since v2:
> - Cache the result of iommu_enable_x2apic so it can be used in the
>lapic suspend/resume paths.
>
> Changes since v1:
> - Fix error paths of iommu_enable_x2apic call in x2apic_bsp_setup.
> ---
> NB: should enabling x2APIC without interrupt remapping be limited to
> running on virtualized environments?
>
> The bigger performance benefit is indeed achieved when using x2APIC on
> virt environments, but I also don't see why we wouldn't want to try
> using it everywhere where it's supported.
> ---
> xen/arch/x86/apic.c | 94 ++---
> 1 file changed, 45 insertions(+), 49 deletions(-)
>
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index a8ee18636f..333115bc88 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -44,6 +44,8 @@ static bool __read_mostly tdt_enabled;
> static bool __initdata tdt_enable = true;
> boolean_param("tdt", tdt_enable);
>
> +static bool __read_mostly iommu_x2apic_enabled;
> +
> static struct {
> int active;
> /* r/w apic fields */
> @@ -492,7 +494,8 @@ static void __enable_x2apic(void)
>
> static void resume_x2apic(void)
> {
> -iommu_enable_x2apic();
> +if ( iommu_x2apic_enabled )
> +iommu_enable_x2apic();
> __enable_x2apic();
> }
>
> @@ -695,7 +698,8 @@ int lapic_suspend(void)
>
> local_irq_save(flags);
> disable_local_APIC();
> -iommu_disable_x2apic();
> +if ( iommu_x2apic_enabled )
> +iommu_disable_x2apic();
> local_irq_restore(flags);
> return 0;
> }
> @@ -860,7 +864,6 @@ void __init x2apic_bsp_setup(void)
> {
> struct IO_APIC_route_entry **ioapic_entries = NULL;
> const char *orig_name;
> -bool intremap_enabled;
>
> if ( !cpu_has_x2apic )
> return;
> @@ -875,56 +878,46 @@ void __init x2apic_bsp_setup(void)
> printk("x2APIC: Already enabled by BIOS: Ignoring cmdline
> disable.\n");
> }
>
> -if ( !iommu_supports_x2apic() )
> +if ( iommu_supports_x2apic() )
> {
> -if ( !x2apic_enabled )
> +if ( (ioapic_entries = alloc_ioapic_entries()) == NULL )
> {
> -printk("Not enabling x2APIC: depends on IOMMU support\n");
> -return;
> +printk("Allocate ioapic_entries failed\n");
> +goto out;
> }
> -panic("x2APIC: already enabled by BIOS, but no IOMMU support\n");
> -}
> -
> -if ( (ioapic_entries = alloc_ioapic_entries()) == NULL )
> -{
> -printk("Allocate ioapic_entries failed\n");
> -goto out;
> -}
>
> -if ( save_IO_APIC_setup(ioapic_entries) )
> -{
> -printk("Saving IO-APIC state failed\n");
> -goto out;
> -}
> +if ( save_IO_APIC_setup(ioapic_entries) )
> +{
> +printk("Saving IO-APIC state failed\n");
> +goto out;
> +}
>
> -mask_8259A();
> -mask_IO_APIC_setup(ioapic_entries);
> +mask_8259A();
> +mask_IO_APIC_setup(ioapic_entries);
>
> -switch ( iommu_enable_x2apic() )
> -{
> -case 0:
> -intremap_enabled = true;
> -break;
> -case -ENXIO: /* ACPI_DMAR_X2APIC_OPT_OUT set */
> -if ( !x2apic_enabled )
> +switch ( iommu_enable_x2apic() )
> {
> +case 0:
> +iommu_x2apic_enabled = true;
> +break;
> +
> +case -ENXIO: /* ACPI_DMAR_X2APIC_OPT_OUT set */
> +if ( x2apic_enabled )
> +panic("IOMMU requests xAPIC mode, but x2APIC already enabled
> by firmware\n");
> +
> printk("Not enabling x2APIC (upon firmware
Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs
I have addressed the comments that I can find in the archive. Some big debates (like _new or not _new, whether to modularise map_pages_to_xen) have not been touched. Various acked-by and reviewed-by added. Hongyan On Thu, 2019-12-05 at 10:14 +0100, Jan Beulich wrote: > On 04.12.2019 18:10, Hongyan Xia wrote: > > NOTE: My email address has changed due to some HR management. I > > have > > lost all my previous emails and I could only salvage some of the > > comments to v3 from the mailing list archive. I will reply to the > > comments from v3 in this v4 series. > > I'm afraid this isn't very helpful. In particular, does this mean > v4 is effectively v3, i.e. no review comments taken care of? Or > just some of them, and others left out? I'm not fancying re- > reviewing a version that doesn't have prior comments taken care > of. Please clarify. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 3/4] x86/smp: check APIC ID on AP bringup
On 04.12.2019 17:20, Roger Pau Monne wrote:
> Check that the processor to be woken up APIC ID is addressable in the
> current APIC mode.
>
> Note that in practice systems with APIC IDs > 255 should already have
> x2APIC enabled by the firmware, and hence this is mostly a safety
> belt.
>
> Signed-off-by: Roger Pau Monné
Reviewed-by: Jan Beulich
with ...
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -1317,6 +1317,13 @@ int __cpu_up(unsigned int cpu)
> if ( (apicid = x86_cpu_to_apicid[cpu]) == BAD_APICID )
> return -ENODEV;
>
> +if ( (!x2apic_enabled || !iommu_intremap) && (apicid >> 8) )
> +{
> +printk("Unsupported: APIC ID %#x in xAPIC mode w/o interrupt
> remapping",
> + apicid);
... the lost newline added back (can be done while commiting).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 2/4] x86/apic: force phys mode if interrupt remapping is disabled
On 04.12.2019 17:20, Roger Pau Monne wrote:
> Cluster mode can only be used with interrupt remapping support, since
> the top 16bits of the APIC ID are filled with the cluster ID, and
> hence on systems where the physical ID is still smaller than 255 the
> cluster ID is not. Force x2APIC to use physical mode if there's no
> interrupt remapping support.
>
> Note that this requires a further patch in order to enable x2APIC
> without interrupt remapping support.
>
> Signed-off-by: Roger Pau Monné
Reviewed-by: Jan Beulich
albeit ...
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -226,7 +226,23 @@ boolean_param("x2apic_phys", x2apic_phys);
> const struct genapic *__init apic_x2apic_probe(void)
> {
> if ( x2apic_phys < 0 )
> -x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> +{
> +if ( !iommu_intremap )
> +/*
> + * Force physical mode if there's no interrupt remapping support:
> + * the ID in clustered mode requires a 32 bit destination field
> due
> + * to the usage of the high 16 bits to store the cluster ID.
> + */
> +x2apic_phys = true;
> +else
> +x2apic_phys = !!(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
... I wonder why you didn't make this
x2apic_phys = !iommu_intremap || (acpi_gbl_FADT.flags &
ACPI_FADT_APIC_PHYSICAL);
(not the least because of allowing to drop the somewhat ugly !!).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/4] x86/ioapic: only use dest32 with x2apic and interrupt remapping enabled
On 04.12.2019 17:20, Roger Pau Monne wrote: > The IO-APIC code assumes that x2apic being enabled also implies > interrupt remapping being enabled, and hence will use the 32bit > destination field in the IO-APIC entry. > > This is safe now, but there's no reason to not enable x2APIC even > without interrupt remapping, and hence the IO-APIC code needs to use > the 32 bit destination field only when both interrupt remapping and > x2APIC are enabled. > > Signed-off-by: Roger Pau Monné Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 0/9] Add alternative API for Xen PTEs
On 04.12.2019 18:10, Hongyan Xia wrote: > NOTE: My email address has changed due to some HR management. I have > lost all my previous emails and I could only salvage some of the > comments to v3 from the mailing list archive. I will reply to the > comments from v3 in this v4 series. I'm afraid this isn't very helpful. In particular, does this mean v4 is effectively v3, i.e. no review comments taken care of? Or just some of them, and others left out? I'm not fancying re- reviewing a version that doesn't have prior comments taken care of. Please clarify. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/4] x86/svm: Clean up intinfo_t variables
On 04.12.2019 20:38, Andrew Cooper wrote:
> On 04/12/2019 09:43, Andrew Cooper wrote:
>> @@ -420,10 +420,10 @@ struct vmcb_struct {
>> u64 exitcode; /* offset 0x70 */
>> u64 exitinfo1; /* offset 0x78 */
>> u64 exitinfo2; /* offset 0x80 */
>> -eventinj_t exitintinfo;/* offset 0x88 */
>> +intinfo_t exitintinfo; /* offset 0x88 */
>> u64 _np_enable; /* offset 0x90 - cleanbit 4 */
>> u64 res08[2];
>> -eventinj_t eventinj; /* offset 0xA8 */
>> +intinfo_t eventinj; /* offset 0xA8 */
>> u64 _h_cr3; /* offset 0xB0 - cleanbit 4 */
>> virt_ext_t virt_ext;/* offset 0xB8 */
>> vmcbcleanbits_t cleanbits; /* offset 0xC0 */
>
> On second thoughts, I'm considering using this opportunity to switch to
> exit_int_info and event_inj.
>
> There are a lot of exit-prefixed names which are easy to confuse at a
> glance.
Fine with me, my R-b stands with this extra purely mechanical
adjustment.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] passthrough: add missed pcidevs_unlock following c/s cd7dedad820
On 04.12.2019 22:31, Igor Druzhinin wrote: > The locking responsibilities have changed and a premature break in > this section now causes the following assertion: > > Assertion '!preempt_count()' failed at preempt.c:36 > > Reported-by: Sander Eikelenboom > Signed-off-by: Igor Druzhinin > --- > xen/drivers/passthrough/pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index ced0c28..2593fe4 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1705,6 +1705,7 @@ int iommu_do_pci_domctl( > seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > ret = -EINVAL; > } > +pcidevs_unlock(); > break; As discussed on the thread of Sander's report, I think we'd be better off simply deleting the break statement. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] x86/svm: Use named (bit)fields for task switch exit info
On 04.12.2019 21:04, Andrew Cooper wrote:
> Net delta is:
>
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
> b/xen/include/asm-x86/hvm/svm/vmcb.h
> index 02b5e86b49..864618ccf9 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -419,17 +419,15 @@ struct vmcb_struct {
> u64 interrupt_shadow; /* offset 0x68 */
> u64 exitcode; /* offset 0x70 */
> union {
> - u64 exitinfo1; /* offset 0x78 */
> + struct {
> + uint64_t exitinfo1; /* offset 0x78 */
> + uint64_t exitinfo2; /* offset 0x80 */
> + };
> union {
> struct {
> uint16_t sel;
> - } task_switch;
> - } e1;
> - };
> - union {
> - u64 exitinfo2; /* offset 0x80 */
> - union {
> - struct {
> + uint64_t :48;
> +
> uint32_t ec;
> uint32_t :4;
> bool iret:1;
> @@ -440,7 +438,7 @@ struct vmcb_struct {
> uint32_t :3;
> bool rf:1;
> } task_switch;
> - } e2;
> + } ei;
> };
> intinfo_t exitintinfo; /* offset 0x88 */
> u64 _np_enable; /* offset 0x90 - cleanbit 4 */
>
> LGTM.
And the result then
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 144527: all pass - PUSHED
flight 144527 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/144527/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 94d4efb54ec4ca894287276ce22d29b6261dbc0b baseline version: ovmf c9416efeef0d4a0554db01f3fd1cdaede14856d7 Last test of basis 144524 2019-12-05 00:39:50 Z0 days Testing same since 144527 2019-12-05 06:39:42 Z0 days1 attempts People who touched revisions under test: Heinrich Schuchardt Sami Mujawar Zhichao Gao jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-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 : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git c9416efeef..94d4efb54e 94d4efb54ec4ca894287276ce22d29b6261dbc0b -> xen-tested-master ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] xen-unstable (4.14 to be): Assertion '!preempt_count()' failed at preempt.c:36
> -Original Message-
> From: Jan Beulich
> Sent: 05 December 2019 08:44
> To: Durrant, Paul
> Cc: Sander Eikelenboom ; xen-
> de...@lists.xenproject.org; Igor Druzhinin ;
> Paul Durrant
> Subject: Re: xen-unstable (4.14 to be): Assertion '!preempt_count()'
> failed at preempt.c:36
>
> On 05.12.2019 09:35, Durrant, Paul wrote:
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1696,16 +1696,12 @@ int iommu_do_pci_domctl(
> >
> > pcidevs_lock();
> > ret = device_assigned(seg, bus, devfn);
> > -if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> > +if ( ret && domctl->cmd == XEN_DOMCTL_test_assign_device )
> > {
> > -if ( ret )
> > -{
> > -printk(XENLOG_G_INFO
> > - "%04x:%02x:%02x.%u already assigned, or non-
> existent\n",
> > - seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> > -ret = -EINVAL;
> > -}
> > -break;
> > +printk(XENLOG_G_INFO
> > + "%04x:%02x:%02x.%u already assigned, or non-
> existent\n",
> > + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> > +ret = -EINVAL;
> > }
>
> But this seems wrong - you'd end up calling assign_device() even
> for the XEN_DOMCTL_test_assign_device case, when ret is 0. All we
> want is to delete the break statement afaict.
>
Ah, yes; that logic is quite confusing. The patch should indeed be:
---8<---
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index ced0c28e4f..c07a63981a 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1705,7 +1705,6 @@ int iommu_do_pci_domctl(
seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
ret = -EINVAL;
}
-break;
}
else if ( !ret )
ret = assign_device(d, seg, bus, devfn, flags);
---8<---
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] xen-unstable (4.14 to be): Assertion '!preempt_count()' failed at preempt.c:36
On 05.12.2019 09:35, Durrant, Paul wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1696,16 +1696,12 @@ int iommu_do_pci_domctl(
>
> pcidevs_lock();
> ret = device_assigned(seg, bus, devfn);
> -if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> +if ( ret && domctl->cmd == XEN_DOMCTL_test_assign_device )
> {
> -if ( ret )
> -{
> -printk(XENLOG_G_INFO
> - "%04x:%02x:%02x.%u already assigned, or
> non-existent\n",
> - seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> -ret = -EINVAL;
> -}
> -break;
> +printk(XENLOG_G_INFO
> + "%04x:%02x:%02x.%u already assigned, or non-existent\n",
> + seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +ret = -EINVAL;
> }
But this seems wrong - you'd end up calling assign_device() even
for the XEN_DOMCTL_test_assign_device case, when ret is 0. All we
want is to delete the break statement afaict.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 3/9] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen
On 04.12.2019 19:01, Xia, Hongyan wrote:
>>> @@ -5272,6 +5279,7 @@ int map_pages_to_xen(
>>> ((1u << PAGETABLE_ORDER) - 1)) == 0)) )
>>> {
>>> unsigned long base_mfn;
>>> +l1_pgentry_t *l1t;
>>
>> const, as this looks to be used for lookup only?
>
> I cannot do this for now since variables like l1t are still using the
> old API which is non-const. I can change it once they are switched to
> the new const API in later patches.
Maybe I've indeed picked an example where this won't work yet,
but there look to be cases where the old interface wouldn't
get in the way. I'd appreciate if at least those cases could
have const added right away.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] xen-unstable (4.14 to be): Assertion '!preempt_count()' failed at preempt.c:36
> -Original Message-
> From: Xen-devel On Behalf Of
> Sander Eikelenboom
> Sent: 04 December 2019 21:04
> To: Jan Beulich
> Cc: xen-devel@lists.xenproject.org; Igor Druzhinin
> ; Paul Durrant
> Subject: Re: [Xen-devel] xen-unstable (4.14 to be): Assertion
> '!preempt_count()' failed at preempt.c:36
>
> On 04/12/2019 18:30, Jan Beulich wrote:
> > On 04.12.2019 18:21, Sander Eikelenboom wrote:
> >> On current xen-unstable (4.14 to be) and AMD cpu:
> >>
> >> After rebooting the host, while the guests are starting, I hit the
> assertion below.
> >> xen-staging-4.13 seems fine on the same machine.
> >
> > Nothing between 4.13 RC4 and the tip of staging stands out,
> > so I wonder if you could bisect over this range? Or perhaps
> > someone else sees something I don't see (right now).
> >
> > Jan
>
> Bisection came up with:
>
> commit cd7dedad8209753e0fc8a97e61d04b74912b53dc
> Author: Paul Durrant
> Date: Fri Nov 15 18:59:30 2019 +
>
> passthrough: simplify locking and logging
>
> Dropping the pcidevs lock between calling device_assigned() and
> assign_device() means that the latter has to do the same check as the
> former for no obvious gain. Also, since long running operations under
> pcidevs lock already drop the lock and return -ERESTART periodically
> there
> is little point in immediately failing an assignment operation with
> -ERESTART just because the pcidevs lock could not be acquired (for the
> second time, having already blocked on acquiring the lock in
> device_assigned()).
>
> This patch instead acquires the lock once for assignment (or test
> assign)
> operations directly in iommu_do_pci_domctl() and thus can remove the
> duplicate domain ownership check in assign_device(). Whilst in the
> neighbourhood, the patch also removes some debug logging from
> assign_device() and deassign_device() and replaces it with proper
> error
> logging, which allows error logging in iommu_do_pci_domctl() to be
> removed.
>
> Signed-off-by: Paul Durrant
> Signed-off-by: Igor Druzhinin
> Acked-by: Jan Beulich
>
Going through the code, I notice a missing pcidevs_unlock() in the case of a
device already assigned. I fixed it with a bit of re-structuring. Could you try
the following patch?
---8<---
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index ced0c28e4f..c7207998a5 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1696,16 +1696,12 @@ int iommu_do_pci_domctl(
pcidevs_lock();
ret = device_assigned(seg, bus, devfn);
-if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
+if ( ret && domctl->cmd == XEN_DOMCTL_test_assign_device )
{
-if ( ret )
-{
-printk(XENLOG_G_INFO
- "%04x:%02x:%02x.%u already assigned, or non-existent\n",
- seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-ret = -EINVAL;
-}
-break;
+printk(XENLOG_G_INFO
+ "%04x:%02x:%02x.%u already assigned, or non-existent\n",
+ seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ret = -EINVAL;
}
---8<---
Thanks,
Paul
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
