[Xen-devel] [linux-3.18 test] 122515: tolerable FAIL - PUSHED

[osstest service owner]

flight 122515 linux-3.18 real [real]
http://logs.test-lab.xenproject.org/osstest/logs/122515/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-arm64-arm64-examine  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 122286
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 122286
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 122286
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 122286
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 122286
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 122286
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 122286
 test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass
 test-amd64-amd64-xl-pvhv2-amd 12 guest-start  fail  never pass
 test-amd64-i386-xl-pvshim12 guest-start  fail   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-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
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
 build-arm64-pvops 6 kernel-build fail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-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-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass

version targeted for testing:
 linux754ca08efd83eeb9cccdb109da2fa0b3a27c1172
baseline version:
 linux78db2bbfa06cc39707054093fbbc5e573a643d3e

Last test of basis   122286  2018-04-14 16:36:32 Z   16 days
Failing since122388  2018-04-24 07:40:13 Z6 days4 attempts
Testing same since   122515  2018-04-29 15:01:38 Z1 days1 attempts


People who touched revisions under test:
  Aaron Ma 
  Al Viro 
  Alex Deucher 
  Alex Smith 
  Alexander Aring 
  Alexandre Belloni 
  

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Andrew Cooper]

On 30/04/2018 22:23, Natarajan, Janakarajan wrote:
>>> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr,
>>> bool valid)
>>> +{
>>> +    struct avic_logical_id_entry *entry, new_entry;
>>> +    u32 dfr = vlapic_read_aligned(vcpu_vlapic(v), APIC_DFR);
>>> +
>>> +    entry = avic_get_logical_id_entry(>domain->arch.hvm_domain.svm,
>>> +  ldr, (dfr == APIC_DFR_FLAT));
>>> +    if (!entry)
>> if ( !entry )
>>
>>> +    return -EINVAL;
>>> +
>>> +    new_entry = *entry;
>>> +    smp_rmb();
>>> +    new_entry.guest_phy_apic_id = g_phy_id;
>>> +    new_entry.valid = valid;
>>> +    *entry = new_entry;
>>> +    smp_wmb();
>> These barriers don't do what you want.  The pattern you are looking for
>> would require an smp_mb() between setting valid and writing things
>> back.  However, that is overkill - all that matters is that the compiler
>> doesn't generate multiple partial updates.
>>
>> new_entry.raw = ACCESS_ONCE(entry->raw);
>>
>> new_entry.guest_phy_apic_id = g_phy_id;
>> new_entry.valid = valid;
>>
>> ACCESS_ONCE(entry->raw) = new_entry.raw;
>
> Since it was decided to not use
>
> union ... {
>     uint64_t raw;
>     struct avic_logical_table_entry {
>     
>     
>     };
> };
>
> would smp_mb() make sense here?

Nothing has been decided yet.  I've made an argument for why the
minimalistic approach (as suggested in in the thread hanging off patch
4, which supersedes this) would be correct (and best, IMO) but the
decision as to what code to use is ultimately up to you as the submitter
(subject to it being functionally correct, which is the purpose of review).

In the ACCESS_ONCE() case, all the assignments are strictly dependent on
previous reads, which forces the compiler and pipeline to issue them in
order, so I don't see any reason for an mfence instruction.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Lars Kurth]

On 30/04/2018, 17:21, "Ian Jackson"  wrote:

Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl 
script to optimise the workflow when using git format-patch with 
get_maintainer.pl"):
> Given that git send-email reads CC's anywhere in the body of a *.patch 
file, 
> I am not sure why this is useful. Can you enlighten me? It does also have 
all 
> the bits to ensure that I can do this. Aka I can make sure that all the 
CC's from 
> the commit-message/*.patch files are added to the e-mail block. Right now
> I avoid duplication: aka I only add stuff to the header if it is not 
already 
> In the commit-message/*.patch.  
> 
> I can see though, that some of the tool functionality is useful in 
non-xen 
> trees. Thus, changing it such that it doesn't fall over when 
get_maintainers.pl 
> isn't there is probably a good idea.

The function of your tool is to invoke get_maintainer and put the
addresses from there everywhere appropriate including, in particular,
the CCs of the cover letter.

The only reason your tool is needed is because there is no other tool
that gets the cover letter right.

But I often have a set of patches where I have manually decided who
they should be CCd to, and put appropriate tags in my commit
messages.  I don't blindly use get_maintainer.

When I do this, there is nothing that gets the CC for the cover letter
right.  (I sometimes bodge it.)  Your tool already knows how to
extract CCs from the individual non-cover-letter patches and add them
to the cover letter.  That is the function I want - to do that, but
not run get_maintainer.

That makes sense and can be easily done via an option: e.g.
--insert cover|-i cover or a separate option. Let me know whether
you have a preference regarding naming/options.

Nor do I need, I think, your tool to edit any of the non-cover-letter
patches, since git-send-email will find CCs in their bodies and use
them for the email recipients.

I don't think I really mind where the CCs end up in the cover letter
(whether they are in the body, or just in the email header), but
others have made a convincing argument that they should be in the
body, so that is fine.

Does that make sense ?

Yes

Lars

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [xen-4.9-testing test] 122512: tolerable FAIL - PUSHED

[osstest service owner]

flight 122512 xen-4.9-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/122512/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-debianhvm-amd64 16 guest-localmigrate/x10 fail in 
122472 pass in 122512
 test-amd64-i386-xl-qemut-ws16-amd64 14 guest-localmigrate fail in 122472 pass 
in 122512
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-localmigrate/x10 fail in 122472 
pass in 122512
 test-amd64-amd64-xl-qemuu-ws16-amd64 14 guest-localmigrate fail pass in 122472

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-ws16-amd64 15 guest-saverestore.2 fail in 122472 
like 121358
 test-armhf-armhf-xl-rtds 12 guest-start fail in 122472 like 121358
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-localmigrate/x10 fail in 122472 
like 121704
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop   fail in 122472 like 121704
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-localmigrate/x10 fail in 122472 
like 121761
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop  fail in 122472 like 121761
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-localmigrate/x10 fail like 121728
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 121728
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 16 guest-localmigrate/x10 fail 
like 121761
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 16 guest-localmigrate/x10 fail 
like 121761
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 121761
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 121761
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 121761
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 121761
 test-amd64-i386-xl-qemuu-ws16-amd64 16 guest-localmigrate/x10 fail like 121761
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-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-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-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-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-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail   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-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-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-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-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail 

[Xen-devel] [PATCH] xen: Add RING_COPY_RESPONSE()

[Marek Marczykowski-Górecki]

Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
(i.e., by not considering that the other end may alter the data in the
shared ring while it is being inspected).  Safe usage of a response
generally requires taking a local copy.

Provide a RING_COPY_RESPONSE() macro to use instead of
RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
ensuring that the copy is done correctly regardless of any possible
compiler optimizations.

Use a volatile source to prevent the compiler from reordering or
omitting the copy.

This is complementary to XSA155.

Signed-off-by: Marek Marczykowski-Górecki 
---
 xen/include/public/io/ring.h | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 30342fc8c0..230fc34cba 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -227,22 +227,25 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define RING_GET_REQUEST(_r, _idx)  \
 (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
 
+#define RING_GET_RESPONSE(_r, _idx) \
+(&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
+
 /*
- * Get a local copy of a request.
+ * Get a local copy of a request/response.
  *
- * Use this in preference to RING_GET_REQUEST() so all processing is
- * done on a local copy that cannot be modified by the other end.
+ * Use this in preference to RING_GET_REQUEST()/RING_GET_RESPONSE() so all
+ * processing is done on a local copy that cannot be modified by the other end.
  *
  * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
  * to be ineffective where _req is a struct which consists of only bitfields.
  */
-#define RING_COPY_REQUEST(_r, _idx, _req) do { \
+#define RING_COPY_(action, _r, _idx, _req) do {
\
/* Use volatile to force the copy into _req. */ \
-   *(_req) = *(volatile typeof(_req))RING_GET_REQUEST(_r, _idx);   \
+   *(_req) = *(volatile typeof(_req))RING_GET_##action(_r, _idx);  \
 } while (0)
 
-#define RING_GET_RESPONSE(_r, _idx) \
-(&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
+#define RING_COPY_REQUEST(_r, _idx, _req) RING_COPY_(REQUEST, _r, _idx, _req)
+#define RING_COPY_RESPONSE(_r, _idx, _req) RING_COPY_(RESPONSE, _r, _idx, _req)
 
 /* Loop termination condition: Would the specified index overflow the ring? */
 #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)   \
-- 
2.13.6


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen: Add RING_COPY_RESPONSE()

[Boris Ostrovsky]

On 04/30/2018 05:27 PM, Marek Marczykowski-Górecki wrote:
> On Mon, Apr 30, 2018 at 05:25:52PM -0400, Boris Ostrovsky wrote:
>> Also, perhaps the two can be collapsed together, along the lines of
>>
>> #define RING_COPY_(action, _r, _idx, _msg) do {  \
>>     /* Use volatile to force the copy into _msg. */ \
>>     *(_msg) = *(volatile typeof(_msg))RING_GET_##action(_r, _idx);   \
>> } while (0)
>>
>> #define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx,
>> _req)
>> #define RING_COPY_RESPONSE(_r, _idx, _rsp)  RING_COPY_(RESPONSE, _r,
>> _idx, _rsp)
>>
>>
>> (I have not tried to compile this so it may well be wrong)
> It works, thanks :)
> I'll wait with v2 until I get feedback on other patches.
>


Oh, and one more thing --- the canonical version of this file lives in
Xen (include/public/io/ring.h) so it needs first to be accepted by Xen
maintainers.

-boris


signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [xen-unstable baseline-only test] 74651: tolerable FAIL

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 74651 xen-unstable real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/74651/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-qemuu-ws16-amd64 14 guest-localmigrate fail REGR. vs. 74628

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail like 74628
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail like 74628
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail like 74628
 test-armhf-armhf-libvirt 12 guest-start  fail   like 74628
 test-armhf-armhf-xl-midway   12 guest-start  fail   like 74628
 test-armhf-armhf-xl-multivcpu 12 guest-start  fail  like 74628
 test-armhf-armhf-xl  12 guest-start  fail   like 74628
 test-armhf-armhf-xl-credit2  12 guest-start  fail   like 74628
 test-armhf-armhf-xl-rtds 12 guest-start  fail   like 74628
 test-armhf-armhf-xl-xsm  12 guest-start  fail   like 74628
 test-armhf-armhf-libvirt-xsm 12 guest-start  fail   like 74628
 test-amd64-amd64-qemuu-nested-intel 14 xen-boot/l1 fail like 74628
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail like 74628
 test-armhf-armhf-xl-vhd  10 debian-di-installfail   like 74628
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail   like 74628
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop  fail like 74628
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail like 74628
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-pvhv2-amd 12 guest-start  fail  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-i386-xl-qemut-ws16-amd64 10 windows-install fail never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-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-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass
 test-amd64-i386-xl-qemut-win10-i386 17 guest-stop  fail never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass

version targeted for testing:
 xen  0d16ece0c5adb960ee4e45f12183bcac8fe6d50a
baseline version:
 xen  a6aa678fa380e9369cc44701a181142322b3a4b0

Last test of basis74628  2018-04-17 04:19:08 Z   13 days
Testing same since74651  2018-04-30 10:50:18 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  David Wang 
  George Dunlap 
  Ian Jackson 
  Jan Beulich 
  Juergen Gross 
  Roger Pau Monne 
  Roger Pau Monné 
  Wei Liu 

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-prev pass
 build-i386-prev  pass
 build-amd64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 build-amd64-rumprun  pass
 

Re: [Xen-devel] [PATCH 1/6] xen: Add RING_COPY_RESPONSE()

[Marek Marczykowski-Górecki]

On Mon, Apr 30, 2018 at 05:25:52PM -0400, Boris Ostrovsky wrote:
> Also, perhaps the two can be collapsed together, along the lines of
> 
> #define RING_COPY_(action, _r, _idx, _msg) do {  \
>     /* Use volatile to force the copy into _msg. */ \
>     *(_msg) = *(volatile typeof(_msg))RING_GET_##action(_r, _idx);   \
> } while (0)
> 
> #define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx,
> _req)
> #define RING_COPY_RESPONSE(_r, _idx, _rsp)  RING_COPY_(RESPONSE, _r,
> _idx, _rsp)
> 
> 
> (I have not tried to compile this so it may well be wrong)

It works, thanks :)
I'll wait with v2 until I get feedback on other patches.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

[Natarajan, Janakarajan]

On 4/13/2018 12:48 PM, Andrew Cooper wrote:

On 04/04/18 00:01, Janakarajan Natarajan wrote:

From: Suravee Suthikulpanit 

AVIC introduces two new #vmexit handlers:

VMEXIT_INCOMP_IPI:
This occurs when an IPI could not be delivered to all targeted guest
virtual processors because at least one guest virtual processor
was not allocated to a physical core at the time. In this case,
Xen would try to emulate IPI.

VMEXIT_DO_NOACCEL:
This occurs when a guest access to an APIC register that cannot be
accelerated by AVIC. In this case, Xen tries to emulate register accesses.

This fault is also generated if an EOI is attempted when the highest priority
in-service interrupt is set for level-triggered mode.

This patch also declare vlapic_read_aligned() and vlapic_reg_write()
as non-static to expose them to be used by AVIC.

Signed-off-by: Suravee Suthikulpanit 
Signed-off-by: Janakarajan Natarajan 
---
  xen/arch/x86/hvm/svm/avic.c| 296 +
  xen/arch/x86/hvm/svm/svm.c |   8 +
  xen/arch/x86/hvm/vlapic.c  |   4 +-
  xen/include/asm-x86/hvm/svm/avic.h |   3 +
  xen/include/asm-x86/hvm/svm/vmcb.h |   6 +
  xen/include/asm-x86/hvm/vlapic.h   |   4 +
  6 files changed, 319 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
index 8108698911..e112469774 100644
--- a/xen/arch/x86/hvm/svm/avic.c
+++ b/xen/arch/x86/hvm/svm/avic.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -37,6 +38,8 @@
  
  #define AVIC_VAPIC_BAR_MASK (((1ULL << 40) - 1) << PAGE_SHIFT)
  
+#define AVIC_UNACCEL_ACCESS_OFFSET_MASK0xFF0

+
  /*
   * Note:
   * Currently, svm-avic mode is not supported with nested virtualization.
@@ -101,6 +104,8 @@ int svm_avic_dom_init(struct domain *d)
  d->arch.hvm_domain.svm.avic_physical_id_table_pg = pg;
  d->arch.hvm_domain.svm.avic_physical_id_table = 
__map_domain_page_global(pg);
  
+spin_lock_init(>arch.hvm_domain.svm.avic_dfr_mode_lock);

+
  return ret;
   err_out:
  svm_avic_dom_destroy(d);
@@ -181,6 +186,297 @@ int svm_avic_init_vmcb(struct vcpu *v)
  }
  
  /*

+ * Note:
+ * This function handles the AVIC_INCOMP_IPI #vmexit when AVIC is enabled.
+ * The hardware generates this fault when an IPI could not be delivered
+ * to all targeted guest virtual processors because at least one guest
+ * virtual processor was not allocated to a physical core at the time.
+ */
+void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
+{
+struct vcpu *curr = current;
+struct domain *currd = curr->domain;
+struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
+u32 icrh = vmcb->exitinfo1 >> 32;
+u32 icrl = vmcb->exitinfo1;
+u32 id = vmcb->exitinfo2 >> 32;
+u32 index = vmcb->exitinfo2 && 0xFF;
+
+switch ( id )
+{
+case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
+/*
+ * AVIC hardware handles the delivery of
+ * IPIs when the specified Message Type is Fixed
+ * (also known as fixed delivery mode) and
+ * the Trigger Mode is edge-triggered. The hardware
+ * also supports self and broadcast delivery modes
+ * specified via the Destination Shorthand(DSH)
+ * field of the ICRL. Logical and physical APIC ID
+ * formats are supported. All other IPI types cause
+ * a #VMEXIT, which needs to emulated.
+ */
+vlapic_reg_write(curr, APIC_ICR2, icrh);
+vlapic_reg_write(curr, APIC_ICR, icrl);
+break;
+
+case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
+{
+/*
+ * At this point, we expect that the AVIC HW has already
+ * set the appropriate IRR bits on the valid target
+ * vcpus. So, we just need to kick the appropriate vcpu.
+ */
+struct vcpu *v;
+uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
+uint32_t short_hand = icrl & APIC_SHORT_MASK;
+bool dest_mode = !!(icrl & APIC_DEST_MASK);

No need for !!.  It is the explicit behaviour of the bool type.


+
+for_each_vcpu ( currd,  v )
+{
+if ( v != curr &&
+ vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(curr),
+   short_hand, dest, dest_mode) )
+{
+vcpu_kick(v);
+break;
+}
+}
+break;
+}
+
+case AVIC_INCMP_IPI_ERR_INV_TARGET:
+gprintk(XENLOG_ERR,
+"SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
+__func__, icrh, icrl, index);
+domain_crash(currd);
+break;
+
+case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
+gprintk(XENLOG_ERR,
+"SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
+__func__, icrh, icrl, index);
+domain_crash(currd);
+ 

Re: [Xen-devel] [PATCH 1/6] xen: Add RING_COPY_RESPONSE()

[Boris Ostrovsky]

On 04/30/2018 05:01 PM, Marek Marczykowski-Górecki wrote:
> Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
> (i.e., by not considering that the other end may alter the data in the
> shared ring while it is being inspected).  Safe usage of a response
> generally requires taking a local copy.
>
> Provide a RING_COPY_RESPONSE() macro to use instead of
> RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
> ensuring that the copy is done correctly regardless of any possible
> compiler optimizations.
>
> Use a volatile source to prevent the compiler from reordering or
> omitting the copy.
>
> This is complementary to XSA155.
>
> CC: sta...@vger.kernel.org
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
>  include/xen/interface/io/ring.h | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 3f40501..03702f6 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -201,6 +201,20 @@ struct __name##_back_ring {  
> \
>  #define RING_GET_RESPONSE(_r, _idx)  \
>  (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
>  
> +/*
> + * Get a local copy of a response.
> + *
> + * Use this in preference to RING_GET_RESPONSE() so all processing is
> + * done on a local copy that cannot be modified by the other end.
> + *
> + * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause 
> this
> + * to be ineffective where _rsp is a struct which consists of only bitfields.
> + */
> +#define RING_COPY_RESPONSE(_r, _idx, _rsp) do {  
> \
> + /* Use volatile to force the copy into _rsp. */ \
> + *(_rsp) = *(volatile typeof(_rsp))RING_GET_RESPONSE(_r, _idx);  \
> +} while (0)
> +

To avoid repeating essentially the same comment, can you move
RING_COPY_RESPONSE definition next to RING_COPY_REQUEST and adjust the
existing comment? And probably move RING_GET_RESPONSE next to
RING_GET_REQUEST? In other words

#define RING_GET_REQUEST
#define RING_GET_RESPONSE

/* comment */
#define RING_COPY_REQUEST
#define RING_COPY_RESPONSE


Also, perhaps the two can be collapsed together, along the lines of

#define RING_COPY_(action, _r, _idx, _msg) do {  \
    /* Use volatile to force the copy into _msg. */ \
    *(_msg) = *(volatile typeof(_msg))RING_GET_##action(_r, _idx);   \
} while (0)

#define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx,
_req)
#define RING_COPY_RESPONSE(_r, _idx, _rsp)  RING_COPY_(RESPONSE, _r,
_idx, _rsp)


(I have not tried to compile this so it may well be wrong)

-boris



>  /* Loop termination condition: Would the specified index overflow the ring? 
> */
>  #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)
> \
>  (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 0/6] Fix XSA-155-like bugs in frontend drivers

[Marek Marczykowski-Górecki]

Patches in original Xen Security Advisory 155 cared only about backend drivers
while leaving frontend patches to be "developed and released (publicly) after
the embargo date". This is said series.

Marek Marczykowski-Górecki (6):
  xen: Add RING_COPY_RESPONSE()
  xen-netfront: copy response out of shared buffer before accessing it
  xen-netfront: do not use data already exposed to backend
  xen-netfront: add range check for Tx response id
  xen-blkfront: make local copy of response before using it
  xen-blkfront: prepare request locally, only then put it on the shared ring

 drivers/block/xen-blkfront.c| 110 ++---
 drivers/net/xen-netfront.c  |  61 +-
 include/xen/interface/io/ring.h |  14 -
 3 files changed, 106 insertions(+), 79 deletions(-)

base-commit: 6d08b06e67cd117f6992c46611dfb4ce267cd71e
-- 
git-series 0.9.1

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/6] xen: Add RING_COPY_RESPONSE()

[Marek Marczykowski-Górecki]

Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
(i.e., by not considering that the other end may alter the data in the
shared ring while it is being inspected).  Safe usage of a response
generally requires taking a local copy.

Provide a RING_COPY_RESPONSE() macro to use instead of
RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
ensuring that the copy is done correctly regardless of any possible
compiler optimizations.

Use a volatile source to prevent the compiler from reordering or
omitting the copy.

This is complementary to XSA155.

CC: sta...@vger.kernel.org
Signed-off-by: Marek Marczykowski-Górecki 
---
 include/xen/interface/io/ring.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 3f40501..03702f6 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -201,6 +201,20 @@ struct __name##_back_ring {
\
 #define RING_GET_RESPONSE(_r, _idx)\
 (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
 
+/*
+ * Get a local copy of a response.
+ *
+ * Use this in preference to RING_GET_RESPONSE() so all processing is
+ * done on a local copy that cannot be modified by the other end.
+ *
+ * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
+ * to be ineffective where _rsp is a struct which consists of only bitfields.
+ */
+#define RING_COPY_RESPONSE(_r, _idx, _rsp) do {
\
+   /* Use volatile to force the copy into _rsp. */ \
+   *(_rsp) = *(volatile typeof(_rsp))RING_GET_RESPONSE(_r, _idx);  \
+} while (0)
+
 /* Loop termination condition: Would the specified index overflow the ring? */
 #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)  \
 (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
-- 
git-series 0.9.1

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/6] xen-netfront: do not use data already exposed to backend

[Marek Marczykowski-Górecki]

Backend may freely modify anything on shared page, so use data which was
supposed to be written there, instead of reading it back from the shared
page.

This is complementary to XSA155.

CC: sta...@vger.kernel.org
Signed-off-by: Marek Marczykowski-Górecki 
---
 drivers/net/xen-netfront.c |  9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index dc99763..934b8a4 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -458,7 +458,7 @@ static void xennet_tx_setup_grant(unsigned long gfn, 
unsigned int offset,
tx->flags = 0;
 
info->tx = tx;
-   info->size += tx->size;
+   info->size += len;
 }
 
 static struct xen_netif_tx_request *xennet_make_first_txreq(
@@ -574,7 +574,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
int slots;
struct page *page;
unsigned int offset;
-   unsigned int len;
+   unsigned int len, this_len;
unsigned long flags;
struct netfront_queue *queue = NULL;
unsigned int num_queues = dev->real_num_tx_queues;
@@ -634,14 +634,15 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
}
 
/* First request for the linear area. */
+   this_len = min_t(unsigned int, XEN_PAGE_SIZE - offset, len);
first_tx = tx = xennet_make_first_txreq(queue, skb,
page, offset, len);
-   offset += tx->size;
+   offset += this_len;
if (offset == PAGE_SIZE) {
page++;
offset = 0;
}
-   len -= tx->size;
+   len -= this_len;
 
if (skb->ip_summed == CHECKSUM_PARTIAL)
/* local packet? */
-- 
git-series 0.9.1

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/6] xen-netfront: copy response out of shared buffer before accessing it

[Marek Marczykowski-Górecki]

Make local copy of the response, otherwise backend might modify it while
frontend is already processing it - leading to time of check / time of
use issue.

This is complementary to XSA155.

Cc: sta...@vger.kernel.org
Signed-off-by: Marek Marczykowski-Górecki 
---
 drivers/net/xen-netfront.c | 51 +++
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 4dd0668..dc99763 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -387,13 +387,13 @@ static void xennet_tx_buf_gc(struct netfront_queue *queue)
rmb(); /* Ensure we see responses up to 'rp'. */
 
for (cons = queue->tx.rsp_cons; cons != prod; cons++) {
-   struct xen_netif_tx_response *txrsp;
+   struct xen_netif_tx_response txrsp;
 
-   txrsp = RING_GET_RESPONSE(>tx, cons);
-   if (txrsp->status == XEN_NETIF_RSP_NULL)
+   RING_COPY_RESPONSE(>tx, cons, );
+   if (txrsp.status == XEN_NETIF_RSP_NULL)
continue;
 
-   id  = txrsp->id;
+   id  = txrsp.id;
skb = queue->tx_skbs[id].skb;
if (unlikely(gnttab_query_foreign_access(
queue->grant_tx_ref[id]) != 0)) {
@@ -741,7 +741,7 @@ static int xennet_get_extras(struct netfront_queue *queue,
 RING_IDX rp)
 
 {
-   struct xen_netif_extra_info *extra;
+   struct xen_netif_extra_info extra;
struct device *dev = >info->netdev->dev;
RING_IDX cons = queue->rx.rsp_cons;
int err = 0;
@@ -757,24 +757,23 @@ static int xennet_get_extras(struct netfront_queue *queue,
break;
}
 
-   extra = (struct xen_netif_extra_info *)
-   RING_GET_RESPONSE(>rx, ++cons);
+   RING_COPY_RESPONSE(>rx, ++cons, );
 
-   if (unlikely(!extra->type ||
-extra->type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
+   if (unlikely(!extra.type ||
+extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
if (net_ratelimit())
dev_warn(dev, "Invalid extra type: %d\n",
-   extra->type);
+   extra.type);
err = -EINVAL;
} else {
-   memcpy([extra->type - 1], extra,
-  sizeof(*extra));
+   memcpy([extra.type - 1], ,
+  sizeof(extra));
}
 
skb = xennet_get_rx_skb(queue, cons);
ref = xennet_get_rx_ref(queue, cons);
xennet_move_rx_slot(queue, skb, ref);
-   } while (extra->flags & XEN_NETIF_EXTRA_FLAG_MORE);
+   } while (extra.flags & XEN_NETIF_EXTRA_FLAG_MORE);
 
queue->rx.rsp_cons = cons;
return err;
@@ -784,28 +783,28 @@ static int xennet_get_responses(struct netfront_queue 
*queue,
struct netfront_rx_info *rinfo, RING_IDX rp,
struct sk_buff_head *list)
 {
-   struct xen_netif_rx_response *rx = >rx;
+   struct xen_netif_rx_response rx = rinfo->rx;
struct xen_netif_extra_info *extras = rinfo->extras;
struct device *dev = >info->netdev->dev;
RING_IDX cons = queue->rx.rsp_cons;
struct sk_buff *skb = xennet_get_rx_skb(queue, cons);
grant_ref_t ref = xennet_get_rx_ref(queue, cons);
-   int max = MAX_SKB_FRAGS + (rx->status <= RX_COPY_THRESHOLD);
+   int max = MAX_SKB_FRAGS + (rx.status <= RX_COPY_THRESHOLD);
int slots = 1;
int err = 0;
unsigned long ret;
 
-   if (rx->flags & XEN_NETRXF_extra_info) {
+   if (rx.flags & XEN_NETRXF_extra_info) {
err = xennet_get_extras(queue, extras, rp);
cons = queue->rx.rsp_cons;
}
 
for (;;) {
-   if (unlikely(rx->status < 0 ||
-rx->offset + rx->status > XEN_PAGE_SIZE)) {
+   if (unlikely(rx.status < 0 ||
+rx.offset + rx.status > XEN_PAGE_SIZE)) {
if (net_ratelimit())
dev_warn(dev, "rx->offset: %u, size: %d\n",
-rx->offset, rx->status);
+rx.offset, rx.status);
xennet_move_rx_slot(queue, skb, ref);
err = -EINVAL;
goto next;
@@ -819,7 +818,7 @@ static int xennet_get_responses(struct netfront_queue 
*queue,
if (ref == GRANT_INVALID_REF) {
  

[Xen-devel] [PATCH 5/6] xen-blkfront: make local copy of response before using it

[Marek Marczykowski-Górecki]

Data on the shared page can be changed at any time by the backend. Make
a local copy, which is no longer controlled by the backend. And only
then access it.

This is complementary to XSA155.

CC: sta...@vger.kernel.org
Signed-off-by: Marek Marczykowski-Górecki 
---
 drivers/block/xen-blkfront.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2a8e781..3926811 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1549,7 +1549,7 @@ static bool blkif_completion(unsigned long *id,
 static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 {
struct request *req;
-   struct blkif_response *bret;
+   struct blkif_response bret;
RING_IDX i, rp;
unsigned long flags;
struct blkfront_ring_info *rinfo = (struct blkfront_ring_info *)dev_id;
@@ -1566,8 +1566,8 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
for (i = rinfo->ring.rsp_cons; i != rp; i++) {
unsigned long id;
 
-   bret = RING_GET_RESPONSE(>ring, i);
-   id   = bret->id;
+   RING_COPY_RESPONSE(>ring, i, );
+   id   = bret.id;
/*
 * The backend has messed up and given us an id that we would
 * never have given to it (we stamp it up to BLK_RING_SIZE -
@@ -1575,39 +1575,39 @@ static irqreturn_t blkif_interrupt(int irq, void 
*dev_id)
 */
if (id >= BLK_RING_SIZE(info)) {
WARN(1, "%s: response to %s has incorrect id (%ld)\n",
-info->gd->disk_name, op_name(bret->operation), id);
+info->gd->disk_name, op_name(bret.operation), id);
/* We can't safely get the 'struct request' as
 * the id is busted. */
continue;
}
req  = rinfo->shadow[id].request;
 
-   if (bret->operation != BLKIF_OP_DISCARD) {
+   if (bret.operation != BLKIF_OP_DISCARD) {
/*
 * We may need to wait for an extra response if the
 * I/O request is split in 2
 */
-   if (!blkif_completion(, rinfo, bret))
+   if (!blkif_completion(, rinfo, ))
continue;
}
 
if (add_id_to_freelist(rinfo, id)) {
WARN(1, "%s: response to %s (id %ld) couldn't be 
recycled!\n",
-info->gd->disk_name, op_name(bret->operation), id);
+info->gd->disk_name, op_name(bret.operation), id);
continue;
}
 
-   if (bret->status == BLKIF_RSP_OKAY)
+   if (bret.status == BLKIF_RSP_OKAY)
blkif_req(req)->error = BLK_STS_OK;
else
blkif_req(req)->error = BLK_STS_IOERR;
 
-   switch (bret->operation) {
+   switch (bret.operation) {
case BLKIF_OP_DISCARD:
-   if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
+   if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
struct request_queue *rq = info->rq;
printk(KERN_WARNING "blkfront: %s: %s op 
failed\n",
-  info->gd->disk_name, 
op_name(bret->operation));
+  info->gd->disk_name, 
op_name(bret.operation));
blkif_req(req)->error = BLK_STS_NOTSUPP;
info->feature_discard = 0;
info->feature_secdiscard = 0;
@@ -1617,15 +1617,15 @@ static irqreturn_t blkif_interrupt(int irq, void 
*dev_id)
break;
case BLKIF_OP_FLUSH_DISKCACHE:
case BLKIF_OP_WRITE_BARRIER:
-   if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
+   if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
printk(KERN_WARNING "blkfront: %s: %s op 
failed\n",
-  info->gd->disk_name, 
op_name(bret->operation));
+  info->gd->disk_name, 
op_name(bret.operation));
blkif_req(req)->error = BLK_STS_NOTSUPP;
}
-   if (unlikely(bret->status == BLKIF_RSP_ERROR &&
+   if (unlikely(bret.status == BLKIF_RSP_ERROR &&
 rinfo->shadow[id].req.u.rw.nr_segments == 
0)) {
printk(KERN_WARNING "blkfront: 

[Xen-devel] [PATCH 4/6] xen-netfront: add range check for Tx response id

[Marek Marczykowski-Górecki]

Tx response ID is fetched from shared page, so make sure it is sane
before using it as an array index.

CC: sta...@vger.kernel.org
Signed-off-by: Marek Marczykowski-Górecki 
---
 drivers/net/xen-netfront.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 934b8a4..55c9b25 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -394,6 +394,7 @@ static void xennet_tx_buf_gc(struct netfront_queue *queue)
continue;
 
id  = txrsp.id;
+   BUG_ON(id >= NET_TX_RING_SIZE);
skb = queue->tx_skbs[id].skb;
if (unlikely(gnttab_query_foreign_access(
queue->grant_tx_ref[id]) != 0)) {
-- 
git-series 0.9.1

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 6/6] xen-blkfront: prepare request locally, only then put it on the shared ring

[Marek Marczykowski-Górecki]

Do not reuse data which theoretically might be already modified by the
backend. This is mostly about private copy of the request
(info->shadow[id].req) - make sure the request saved there is really the
one just filled.

This is complementary to XSA155.

CC: sta...@vger.kernel.org
Signed-off-by: Marek Marczykowski-Górecki 
---
 drivers/block/xen-blkfront.c | 76 +
 1 file changed, 44 insertions(+), 32 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 3926811..b100b55 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -525,19 +525,16 @@ static int blkif_ioctl(struct block_device *bdev, fmode_t 
mode,
 
 static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo,
struct request *req,
-   struct blkif_request **ring_req)
+   struct blkif_request *ring_req)
 {
unsigned long id;
 
-   *ring_req = RING_GET_REQUEST(>ring, rinfo->ring.req_prod_pvt);
-   rinfo->ring.req_prod_pvt++;
-
id = get_id_from_freelist(rinfo);
rinfo->shadow[id].request = req;
rinfo->shadow[id].status = REQ_WAITING;
rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID;
 
-   (*ring_req)->u.rw.id = id;
+   ring_req->u.rw.id = id;
 
return id;
 }
@@ -545,23 +542,28 @@ static unsigned long blkif_ring_get_request(struct 
blkfront_ring_info *rinfo,
 static int blkif_queue_discard_req(struct request *req, struct 
blkfront_ring_info *rinfo)
 {
struct blkfront_info *info = rinfo->dev_info;
-   struct blkif_request *ring_req;
+   struct blkif_request ring_req = { 0 };
unsigned long id;
 
/* Fill out a communications ring structure. */
id = blkif_ring_get_request(rinfo, req, _req);
 
-   ring_req->operation = BLKIF_OP_DISCARD;
-   ring_req->u.discard.nr_sectors = blk_rq_sectors(req);
-   ring_req->u.discard.id = id;
-   ring_req->u.discard.sector_number = (blkif_sector_t)blk_rq_pos(req);
+   ring_req.operation = BLKIF_OP_DISCARD;
+   ring_req.u.discard.nr_sectors = blk_rq_sectors(req);
+   ring_req.u.discard.id = id;
+   ring_req.u.discard.sector_number = (blkif_sector_t)blk_rq_pos(req);
if (req_op(req) == REQ_OP_SECURE_ERASE && info->feature_secdiscard)
-   ring_req->u.discard.flag = BLKIF_DISCARD_SECURE;
+   ring_req.u.discard.flag = BLKIF_DISCARD_SECURE;
else
-   ring_req->u.discard.flag = 0;
+   ring_req.u.discard.flag = 0;
+
+   /* make the request available to the backend */
+   *RING_GET_REQUEST(>ring, rinfo->ring.req_prod_pvt) = ring_req;
+   wmb();
+   rinfo->ring.req_prod_pvt++;
 
/* Keep a private copy so we can reissue requests when recovering. */
-   rinfo->shadow[id].req = *ring_req;
+   rinfo->shadow[id].req = ring_req;
 
return 0;
 }
@@ -693,7 +695,7 @@ static void blkif_setup_extra_req(struct blkif_request 
*first,
 static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info 
*rinfo)
 {
struct blkfront_info *info = rinfo->dev_info;
-   struct blkif_request *ring_req, *extra_ring_req = NULL;
+   struct blkif_request ring_req = { 0 }, extra_ring_req = { 0 };
unsigned long id, extra_id = NO_ASSOCIATED_ID;
bool require_extra_req = false;
int i;
@@ -758,16 +760,16 @@ static int blkif_queue_rw_req(struct request *req, struct 
blkfront_ring_info *ri
 * BLKIF_OP_WRITE
 */
BUG_ON(req_op(req) == REQ_OP_FLUSH || req->cmd_flags & REQ_FUA);
-   ring_req->operation = BLKIF_OP_INDIRECT;
-   ring_req->u.indirect.indirect_op = rq_data_dir(req) ?
+   ring_req.operation = BLKIF_OP_INDIRECT;
+   ring_req.u.indirect.indirect_op = rq_data_dir(req) ?
BLKIF_OP_WRITE : BLKIF_OP_READ;
-   ring_req->u.indirect.sector_number = 
(blkif_sector_t)blk_rq_pos(req);
-   ring_req->u.indirect.handle = info->handle;
-   ring_req->u.indirect.nr_segments = num_grant;
+   ring_req.u.indirect.sector_number = 
(blkif_sector_t)blk_rq_pos(req);
+   ring_req.u.indirect.handle = info->handle;
+   ring_req.u.indirect.nr_segments = num_grant;
} else {
-   ring_req->u.rw.sector_number = (blkif_sector_t)blk_rq_pos(req);
-   ring_req->u.rw.handle = info->handle;
-   ring_req->operation = rq_data_dir(req) ?
+   ring_req.u.rw.sector_number = (blkif_sector_t)blk_rq_pos(req);
+   ring_req.u.rw.handle = info->handle;
+   ring_req.operation = rq_data_dir(req) ?
BLKIF_OP_WRITE : BLKIF_OP_READ;
if (req_op(req) == 

[Xen-devel] [xen-4.8-testing test] 122508: tolerable FAIL - PUSHED

[osstest service owner]

flight 122508 xen-4.8-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/122508/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-xtf-amd64-amd64-3 50 xtf/test-hvm64-lbr-tsx-vmentry fail in 122466 pass 
in 122508
 test-xtf-amd64-amd64-4 50 xtf/test-hvm64-lbr-tsx-vmentry fail in 122466 pass 
in 122508
 test-armhf-armhf-xl-rtds 12 guest-start  fail in 122466 pass in 122508
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 16 guest-localmigrate/x10 fail pass 
in 122466
 test-amd64-amd64-xl-qemut-debianhvm-amd64 16 guest-localmigrate/x10 fail pass 
in 122466

Tests which did not succeed, but are not blocking:
 test-xtf-amd64-amd64-2  50 xtf/test-hvm64-lbr-tsx-vmentry fail like 122132
 test-xtf-amd64-amd64-5  50 xtf/test-hvm64-lbr-tsx-vmentry fail like 122161
 test-xtf-amd64-amd64-1  50 xtf/test-hvm64-lbr-tsx-vmentry fail like 122161
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 122161
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 122161
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 122161
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 122161
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 122161
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 122161
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 122161
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 122161
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 122161
 build-i386-prev   7 xen-build/dist-test  fail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 build-amd64-prev  7 xen-build/dist-test  fail   never pass
 test-amd64-amd64-libvirt 13 migrate-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-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-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-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 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-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 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-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-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemut-win10-i386 10 

[Xen-devel] [xen-unstable-smoke test] 122543: tolerable all pass - PUSHED

[osstest service owner]

flight 122543 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/122543/

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  2bc87d85c0a1b1fc13ede98ebe059e5a6e84d535
baseline version:
 xen  08641a9e8870d3b174d95aaa55ecba43387563b5

Last test of basis   122540  2018-04-30 14:00:50 Z0 days
Testing same since   122543  2018-04-30 17:00:28 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

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-i386 pass
 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
   08641a9e88..2bc87d85c0  2bc87d85c0a1b1fc13ede98ebe059e5a6e84d535 -> smoke

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

[Dongwon Kim]

On Wed, Apr 25, 2018 at 08:12:08AM +0200, Juergen Gross wrote:
> On 24/04/18 22:35, Dongwon Kim wrote:
> > Had a meeting with Daniel and talked about bringing out generic
> > part of hyper-dmabuf to the userspace, which means we most likely
> > reuse IOCTLs defined in xen-zcopy for our use-case if we follow
> > his suggestion.
> > 
> > So assuming we use these IOCTLs as they are,
> > Several things I would like you to double-check..
> > 
> > 1. returning gref as is to the user space is still unsafe because
> > it is a constant, easy to guess and any process that hijacks it can easily
> > exploit the buffer. So I am wondering if it's possible to keep dmabuf-to
> > -gref or gref-to-dmabuf in kernel space and add other layers on top
> > of those in actual IOCTLs to add some safety.. We introduced flink like
> > hyper_dmabuf_id including random number but many says even that is still
> > not safe.
> 
> grefs are usable by root only. When you have root access in dom0 you can
> do evil things to all VMs even without using grants. That is in no way
> different to root being able to control all other processes on the
> system.

I honestly didn't know about this. I believed kernel code simply can map those
pages. However, out of curiosity, how is non-root usage of gref prevented in
current design? Is there privilege check in grant table driver or hypercalls
needed by this page mapping is only enabled for root in hypervisor level?

And this is pretty critical information for any use-case using grant-table.
Is there any place(doc/website) this is specified/explained?

Thanks,
DW


> 
> 
> Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[Jason Cooper]

correction:

On Mon, Apr 30, 2018 at 06:17:54PM +, Jason Cooper wrote:
> On Mon, Apr 30, 2018 at 05:38:55PM +0100, George Dunlap wrote:
> > On Mon, Apr 30, 2018 at 5:16 PM, Jason Cooper  wrote:
> > > Hi Ian,
> > >
> > > On Mon, Apr 30, 2018 at 04:22:30PM +0100, Ian Jackson wrote:
> > >> Wei Liu writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = 
> > >> NO-CARRIER?"):
> > >> > To implement reuse_domid in a sane way, either the toolstack needs to
> > >> > manage all domids and always sets domid when creating domain or the
> > >> > hypervisor needs to cooperate -- to have interface to reserve /
> > >> > pre-allocate domids.
> > >>
> > >> I think this is entirely the wrong approach.
> > >
> > > Whew.  Glad I didn't start hacking yet...
> > >
> > >> I think the right answer is that this is simply a bug in the
> > >> frontends.  frontends should cope if the backend path pointer in the
> > >> frontend directory is updated, and should start reading the new
> > >> backend instead.
> > >
> > > Ok, so I'm new to the guts of Xen.  The bug, at a high level, is that
> > > "When a driver domain is rebooted (domid changed), previously connected
> > > client domUs can't gain network connectivity to/through the driver
> > > domain via 'xl network-attach client_domu mac=... bridge=...
> > > backend=drv_dom'"
> > 
> > Hang on -- just to clarify, something like the following doesn't work
> > (or wouldn't, you suspect, work)?
> > 
> > * Start driver domain
> > * Start domU A with no network
> 
> My setup is different here.  I include the vif = [... backend=...]
> declaration in my domain config.
> 
> > * xl network-attach A backend=drv_dom
> 
> So I don't do this step manually.
> 
> > * [do some stuff]
> > * xl network-detach A [network devid]
> > * Restart driver domain
> > * xl network-attach A backend=drv_dom
> 
> Otherwise, this is all correct.  Then I get the NO-CARRIER in domU A.

Sorry, I get NO-CARRIER in the just rebooted driver domain.  And the
interface is still UP in domU A.

thx,

Jason.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[Jason Cooper]

Hi George,

On Mon, Apr 30, 2018 at 05:38:55PM +0100, George Dunlap wrote:
> On Mon, Apr 30, 2018 at 5:16 PM, Jason Cooper  wrote:
> > Hi Ian,
> >
> > On Mon, Apr 30, 2018 at 04:22:30PM +0100, Ian Jackson wrote:
> >> Wei Liu writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = 
> >> NO-CARRIER?"):
> >> > To implement reuse_domid in a sane way, either the toolstack needs to
> >> > manage all domids and always sets domid when creating domain or the
> >> > hypervisor needs to cooperate -- to have interface to reserve /
> >> > pre-allocate domids.
> >>
> >> I think this is entirely the wrong approach.
> >
> > Whew.  Glad I didn't start hacking yet...
> >
> >> I think the right answer is that this is simply a bug in the
> >> frontends.  frontends should cope if the backend path pointer in the
> >> frontend directory is updated, and should start reading the new
> >> backend instead.
> >
> > Ok, so I'm new to the guts of Xen.  The bug, at a high level, is that
> > "When a driver domain is rebooted (domid changed), previously connected
> > client domUs can't gain network connectivity to/through the driver
> > domain via 'xl network-attach client_domu mac=... bridge=...
> > backend=drv_dom'"
> 
> Hang on -- just to clarify, something like the following doesn't work
> (or wouldn't, you suspect, work)?
> 
> * Start driver domain
> * Start domU A with no network

My setup is different here.  I include the vif = [... backend=...]
declaration in my domain config.

> * xl network-attach A backend=drv_dom

So I don't do this step manually.

> * [do some stuff]
> * xl network-detach A [network devid]
> * Restart driver domain
> * xl network-attach A backend=drv_dom

Otherwise, this is all correct.  Then I get the NO-CARRIER in domU A.

thx,

Jason.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[Jason Cooper]

On Mon, Apr 30, 2018 at 05:26:38PM +0100, Ian Jackson wrote:
> Jason Cooper writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = 
> NO-CARRIER?"):
> > On Mon, Apr 30, 2018 at 04:22:30PM +0100, Ian Jackson wrote:
...
> > Ok, so I'm new to the guts of Xen.  The bug, at a high level, is that
> > "When a driver domain is rebooted (domid changed), previously connected
> > client domUs can't gain network connectivity to/through the driver
> > domain via 'xl network-attach client_domu mac=... bridge=...
> > backend=drv_dom'"
> > 
> > This is due to the fact that the frontend net driver doesn't / can't
> > follow the backend driver to the new domid in xenstore.
> 
> Yes.
> 
> > > I'm a bit surprised that this doesn't already work.
> > 
> > I'm currently running Xen 4.9.1 as patched in the standard Gentoo
> > ebuild.  I've been putting off upgrading to 4.9.2, now marked stable in
> > portage, until I nail this down.  I'm happy to move to 4.10 if needed.
> > 
> > Do you think this is something that is definitely fixed in a more recent
> > version of Xen?  I'm happy to test if so.  Is there a commit id I can
> > look for?
> 
> I think that in my view (which others may disagree with) this is not a
> bug in Xen but in the Linux kernel frontend.  So changing the Xen
> version won't help.

I'm running vanilla v4.16.4 based on allnoconfig in all of these
mini-domu's.  It doesn't look there's been any pertinent recent changes
in drivers/net/xen-netfront.c since v4.16.

Based on an initial scan of the code, it looks like xen-netback watches
for hotplug events on the frontend (xen-netback/xenbus.c:1041-1046 in
connect()).  xen-netfront.c:1995-2036, netback_changed(), is the
registered callback for netfront.

Is the xenbus netback/netfront state machine documented anywhere?
include/xen/interface/io/netif.h has a great description of tx/rx queue
setup and teardown, but doesn't seem to have anything specific to the
high-level signalling that 'xl network-attach' would cause.

Any pointers?

thx,

Jason.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

On 04/30/2018 12:57 PM, Roger Pau Monné wrote:
> On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
>> Latest binutils release (2.29.1) will no longer allow proper computation
>> of GDT entries on 32-bits, with warning:
>>
>> arch/x86/xen/xen-pvh.S: Assembler messages:
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not 
>> between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not 
>> between 0 and 31)
>> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not 
>> between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not 
>> between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not 
>> between 0 and 31)
>> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not 
>> between 0 and 31)
>>
>> Use explicit value of the entry instead of using GDT_ENTRY() macro.
>>
>> Signed-off-by: Boris Ostrovsky 
>> Cc: sta...@vger.kernel.org
>> ---
>>  arch/x86/xen/xen-pvh.S | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
>> index e1a5fbe..934f7d4 100644
>> --- a/arch/x86/xen/xen-pvh.S
>> +++ b/arch/x86/xen/xen-pvh.S
>> @@ -145,11 +145,11 @@ gdt_start:
>>  .quad 0x/* NULL descriptor */
>>  .quad 0x/* reserved */
>>  #ifdef CONFIG_X86_64
>> -.quad GDT_ENTRY(0xa09a, 0, 0xf) /* __KERNEL_CS */
>> +.quad 0x00af9a00/* __BOOT_CS */
>>  #else
>> -.quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
>> +.quad 0x00cf9a00/* __BOOT_CS */
> Maybe it would be cleaner to use something like:

I actually considered all of these and ended up with a raw number
because it seems to be a convention in kernel (and Xen too, apparently) 
to use raw values in .S files.

Kernel is using now GDT_ENTRY_INIT() which is a C macro. There is one
other location where GDT_INIT() is used (arch/x86/boot/pm.c) and,
incidentally, it also generates this warning IIRC.

I really don't want to move definition to C code just to use a macro ---
I don't think C code needs to be exposed to this GDT.


>
> .word 0x /* limit */
> .word 0  /* base */
> .byte 0  /* base */
> .byte 0x9a   /* access */
> #ifdef CONFIG_X86_64
> .byte 0xaf   /* flags plus limit */
> #else
> .byte 0xcf   /* flags plus limit */
> #endif
> .byte 0  /* base */


I, in fact, started with something like this. But if you repeat this 4
times you will probably see why I decided against it ;-)


-boris


>
> Or try to fix the GDT_ENTRY macro, maybe if you prepend extra 0's to
> make the values 64bit that would prevent the warnings?
>
> Or declare the GDT in enlighten_pvh in C and use it here?
>
> Also, IIRC the base/limit values are ignored in long mode.
>
> Thanks, Roger.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] SVM: re-work VMCB sync-ing

[Boris Ostrovsky]

On 04/30/2018 01:07 PM, Andrew Cooper wrote:
> On 30/04/18 12:37, Jan Beulich wrote:
>> While the main problem to be addressed here is the issue of what so far
>> was named "vmcb_in_sync" starting out with the wrong value (should have
>> been true instead of false, to prevent performing a VMSAVE without ever
>> having VMLOADed the vCPU's state), go a step further and make the
>> sync-ed state a tristate: CPU and memory may be in sync or an update
>> may be required in either direction. Rename the field and introduce an
>> enum. Callers of svm_sync_vmcb() now indicate the intended new state
>> (with a slight "anomaly" when requesting VMLOAD: we could store
>> vmcb_needs_vmsave in those cases as the callers request, but the VMCB
>> really is in sync at that point, and hence there's no need to VMSAVE in
>> case we don't make it out to guest context), and all syncing goes
>> through that function.
>>
>> With that, there's no need to VMLOAD the state perhaps multiple times;
>> all that's needed is loading it once before VM entry.
>>
>> Signed-off-by: Jan Beulich 
>> ---
>> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum
>> vmcb_sync_state.
> -1 from me.  This is even more confusing to use than v1.
>
> It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave);
> means "vmload", and its actively wrong that the state doesn't remain
> in-sync.

It does become in-sync:


+if ( new_state == vmcb_needs_vmsave )
+{
+ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload);
+svm_vmload(arch_svm->vmcb);
+arch_svm->vmcb_sync_state = vmcb_in_sync;
+}
+else

(although Jan is questioning whether to drop that change in the comments to 
patch 2, if I understood him correctly)

-boris


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Should PV frontend drivers trust the backends?

[Marek Marczykowski-Górecki]

On Thu, Apr 26, 2018 at 11:47:41AM +0300, Oleksandr Andrushchenko wrote:
> On 04/26/2018 11:16 AM, Paul Durrant wrote:
> > > -Original Message-
> > > From: Oleksandr Andrushchenko [mailto:andr2...@gmail.com]
> > > Sent: 26 April 2018 07:00
> > > To: Paul Durrant ; 'Juergen Gross'
> > > ; xen-devel 
> > > Subject: Re: [Xen-devel] Should PV frontend drivers trust the backends?
> > > 
> > > On 04/25/2018 04:47 PM, Paul Durrant wrote:
> > > > > -Original Message-
> > > > > From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On
> > > Behalf
> > > > > Of Juergen Gross
> > > > > Sent: 25 April 2018 13:43
> > > > > To: xen-devel 
> > > > > Subject: [Xen-devel] Should PV frontend drivers trust the backends?
> > > > > 
> > > > > This is a followup of a discussion on IRC:
> > > > > 
> > > > > The main question of the discussion was: "Should frontend drivers
> > > > > trust their backends not doing malicious actions?"
> > > > > 
> > > > > This IMO includes:
> > > > > 
> > > > > 1. The data put by the backend on the ring page(s) is sane and
> > > > >  consistent, meaning that e.g. the response producer index is 
> > > > > always
> > > > >  ahead of the consumer index.
> > > > > 
> > > > > 2. Response data won't be modified by the backend after the producer
> > > > >  index has been incremented signaling the response is valid.
> > > > > 
> > > > > 3. Response data is sane, e.g. an I/O data length is not larger than
> > > > >  the buffer originally was.
> > > > > 
> > > > > 4. When a response has been sent all grants belonging to the request
> > > > >  have been unmapped again by the backend, meaning that the 
> > > > > frontend
> > > > >  can assume the grants can be removed without conflict.
> > > > > 
> > > > > Today most frontend drivers (at least in the Linux kernel) seem to
> > > > > assume all of the above is true (there are some exceptions, but never
> > > > > for all items):
> > > > > 
> > > > > - they don't check sanity of ring index values
> > > > > - they don't copy response data into local memory before looking at it
> > > > > - they don't verify returned data length (or do so via BUG_ON())
> > > > > - they BUG() in case of a conflict when trying to remove a grant
> > > > > 
> > > > > So the basic question is: should all Linux frontend drivers be 
> > > > > modified
> > > > > in order to be able to tolerate buggy or malicious backends? Or is the
> > > > > list of trust above fine?
> > > > > 
> > > > > IMO even in case the frontends do trust the backends to behave sane 
> > > > > this
> > > > > doesn't mean driver domains don't make sense. Driver domains still 
> > > > > make
> > > > > a Xen host more robust as they e.g. protect the host against driver
> > > > > failures normally leading to a crash of dom0.
> > > > > 
> > > > I see the general question as being analogous to 'should a Linux device
> > > driver trust its hardware' and I think the answer for a general purpose 
> > > OS like
> > > linux is 'yes'.
> > > > Now, having worked on fault tolerant systems in a past life, there are
> > > definitely cases where you want your OS not to implicitly trust its 
> > > peripheral
> > > hardware and hence special device drivers are used.
> > > So what do you do if counters provided by the untrusted HW are ok
> > > and the payload is not?
> > Well, that depends on whether there is actually any way to verify the 
> > payload in a driver. Whatever layer in the system is responsible for the 
> > data needs to verify its integrity in a fault tolerant system. Generally 
> > the driver can only attempt to verify that it's hardware is working as 
> > expect and quiesce it if not. For that reason, in the systems I worked on, 
> > the driver had the ability to control FETs that disconnected peripheral h/w 
> > from the PCI bus.
> > 
> > > > I think the same would apply for virtual machines in situations where a
> > > driver domain is not wholly controlled by a host administrator or is not
> > > trusted to the same extent as dom0 for other reasons; i.e. they should 
> > > have
> > > specialist frontends.
> > > I believe we might be able to express some common strategy for the
> > > frontends.
> > > I do understand though that it all needs to be decided on case by case
> > > basis,
> > > but common things could still be there, e.g. if prod/cons counters are
> > > not in sync
> > > what a frontend needs to do:
> > >    - should it keep trying to get in sync - might be a bad idea as the
> > > req/resp data
> > >      may already become inconsistent (net can probably survive, but not
> > > block)
> > >    - should it tear down the connection with the backend - this may
> > > render in the whole
> > >      system instability, e.g. imagine you tear down a "/" block device
> > >    - should it BUG_ON and die
> > > To me the second option (tear down the connection) seems to 

Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Roger Pau Monné]

On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
> Latest binutils release (2.29.1) will no longer allow proper computation
> of GDT entries on 32-bits, with warning:
> 
> arch/x86/xen/xen-pvh.S: Assembler messages:
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not 
> between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not 
> between 0 and 31)
> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not 
> between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not 
> between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not 
> between 0 and 31)
> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not 
> between 0 and 31)
> 
> Use explicit value of the entry instead of using GDT_ENTRY() macro.
> 
> Signed-off-by: Boris Ostrovsky 
> Cc: sta...@vger.kernel.org
> ---
>  arch/x86/xen/xen-pvh.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
> index e1a5fbe..934f7d4 100644
> --- a/arch/x86/xen/xen-pvh.S
> +++ b/arch/x86/xen/xen-pvh.S
> @@ -145,11 +145,11 @@ gdt_start:
>   .quad 0x/* NULL descriptor */
>   .quad 0x/* reserved */
>  #ifdef CONFIG_X86_64
> - .quad GDT_ENTRY(0xa09a, 0, 0xf) /* __KERNEL_CS */
> + .quad 0x00af9a00/* __BOOT_CS */
>  #else
> - .quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
> + .quad 0x00cf9a00/* __BOOT_CS */

Maybe it would be cleaner to use something like:

.word 0x /* limit */
.word 0  /* base */
.byte 0  /* base */
.byte 0x9a   /* access */
#ifdef CONFIG_X86_64
.byte 0xaf   /* flags plus limit */
#else
.byte 0xcf   /* flags plus limit */
#endif
.byte 0  /* base */

Or try to fix the GDT_ENTRY macro, maybe if you prepend extra 0's to
make the values 64bit that would prevent the warnings?

Or declare the GDT in enlighten_pvh in C and use it here?

Also, IIRC the base/limit values are ignored in long mode.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[George Dunlap]

On Mon, Apr 30, 2018 at 5:16 PM, Jason Cooper  wrote:
> Hi Ian,
>
> On Mon, Apr 30, 2018 at 04:22:30PM +0100, Ian Jackson wrote:
>> Wei Liu writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = 
>> NO-CARRIER?"):
>> > To implement reuse_domid in a sane way, either the toolstack needs to
>> > manage all domids and always sets domid when creating domain or the
>> > hypervisor needs to cooperate -- to have interface to reserve /
>> > pre-allocate domids.
>>
>> I think this is entirely the wrong approach.
>
> Whew.  Glad I didn't start hacking yet...
>
>> I think the right answer is that this is simply a bug in the
>> frontends.  frontends should cope if the backend path pointer in the
>> frontend directory is updated, and should start reading the new
>> backend instead.
>
> Ok, so I'm new to the guts of Xen.  The bug, at a high level, is that
> "When a driver domain is rebooted (domid changed), previously connected
> client domUs can't gain network connectivity to/through the driver
> domain via 'xl network-attach client_domu mac=... bridge=...
> backend=drv_dom'"

Hang on -- just to clarify, something like the following doesn't work
(or wouldn't, you suspect, work)?

* Start driver domain
* Start domU A with no network
* xl network-attach A backend=drv_dom
* [do some stuff]
* xl network-detach A [network devid]
* Restart driver domain
* xl network-attach A backend=drv_dom

 -George

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[Ian Jackson]

Jason Cooper writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = 
NO-CARRIER?"):
> On Mon, Apr 30, 2018 at 04:22:30PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = 
> > NO-CARRIER?"):
> > > To implement reuse_domid in a sane way, either the toolstack needs to
> > > manage all domids and always sets domid when creating domain or the
> > > hypervisor needs to cooperate -- to have interface to reserve /
> > > pre-allocate domids.
> > 
> > I think this is entirely the wrong approach.
> 
> Whew.  Glad I didn't start hacking yet...

Well, it might be that you end up having to use this fixed-domid thing
as a workaround :-/.

> > I think the right answer is that this is simply a bug in the
> > frontends.  frontends should cope if the backend path pointer in the
> > frontend directory is updated, and should start reading the new
> > backend instead.
> 
> Ok, so I'm new to the guts of Xen.  The bug, at a high level, is that
> "When a driver domain is rebooted (domid changed), previously connected
> client domUs can't gain network connectivity to/through the driver
> domain via 'xl network-attach client_domu mac=... bridge=...
> backend=drv_dom'"
> 
> This is due to the fact that the frontend net driver doesn't / can't
> follow the backend driver to the new domid in xenstore.

Yes.

> > I'm a bit surprised that this doesn't already work.
> 
> I'm currently running Xen 4.9.1 as patched in the standard Gentoo
> ebuild.  I've been putting off upgrading to 4.9.2, now marked stable in
> portage, until I nail this down.  I'm happy to move to 4.10 if needed.
> 
> Do you think this is something that is definitely fixed in a more recent
> version of Xen?  I'm happy to test if so.  Is there a commit id I can
> look for?

I think that in my view (which others may disagree with) this is not a
bug in Xen but in the Linux kernel frontend.  So changing the Xen
version won't help.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Boris Ostrovsky]

Latest binutils release (2.29.1) will no longer allow proper computation
of GDT entries on 32-bits, with warning:

arch/x86/xen/xen-pvh.S: Assembler messages:
arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not 
between 0 and 31)
arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not 
between 0 and 31)
arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not 
between 0 and 31)
arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not 
between 0 and 31)
arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not 
between 0 and 31)
arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not 
between 0 and 31)

Use explicit value of the entry instead of using GDT_ENTRY() macro.

Signed-off-by: Boris Ostrovsky 
Cc: sta...@vger.kernel.org
---
 arch/x86/xen/xen-pvh.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index e1a5fbe..934f7d4 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -145,11 +145,11 @@ gdt_start:
.quad 0x/* NULL descriptor */
.quad 0x/* reserved */
 #ifdef CONFIG_X86_64
-   .quad GDT_ENTRY(0xa09a, 0, 0xf) /* __KERNEL_CS */
+   .quad 0x00af9a00/* __BOOT_CS */
 #else
-   .quad GDT_ENTRY(0xc09a, 0, 0xf) /* __KERNEL_CS */
+   .quad 0x00cf9a00/* __BOOT_CS */
 #endif
-   .quad GDT_ENTRY(0xc092, 0, 0xf) /* __KERNEL_DS */
+   .quad 0x00cf9200/* __BOOT_DS */
 gdt_end:
 
.balign 4
-- 
2.9.3


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 0/4] PVH GDT fixes and cleanup

[Boris Ostrovsky]

Switching to new binutils release triggered the first bug.

Not sure if stack canary bug is related to the new tools as well
(haven't checked it with old tools, but they are really old, from
Fedora 13 days).

64-bit guests run fine even without adding the entry for GS but my
guess is that's because Xen toolstack sets cached portions of the
register to sane values and HW makes fewer checks in long mode.
Since those values are not part of the ABI I figured I should fix
it for both 32- and 64-bit mode.


Boris Ostrovsky (4):
  xen/PVH: Replace GDT_ENTRY with explicit constant
  xen/PVH: Use proper CS selector in long mode
  xen/PVH: Set up GS segment for stack canary
  xen/PVH: Remove reserved entry in PVH GDT

 arch/x86/xen/xen-pvh.S | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

-- 
2.9.3


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl 
script to optimise the workflow when using git format-patch with 
get_maintainer.pl"):
> Given that git send-email reads CC's anywhere in the body of a *.patch file, 
> I am not sure why this is useful. Can you enlighten me? It does also have all 
> the bits to ensure that I can do this. Aka I can make sure that all the CC's 
> from 
> the commit-message/*.patch files are added to the e-mail block. Right now
> I avoid duplication: aka I only add stuff to the header if it is not already 
> In the commit-message/*.patch.  
> 
> I can see though, that some of the tool functionality is useful in non-xen 
> trees. Thus, changing it such that it doesn't fall over when 
> get_maintainers.pl 
> isn't there is probably a good idea.

The function of your tool is to invoke get_maintainer and put the
addresses from there everywhere appropriate including, in particular,
the CCs of the cover letter.

The only reason your tool is needed is because there is no other tool
that gets the cover letter right.

But I often have a set of patches where I have manually decided who
they should be CCd to, and put appropriate tags in my commit
messages.  I don't blindly use get_maintainer.

When I do this, there is nothing that gets the CC for the cover letter
right.  (I sometimes bodge it.)  Your tool already knows how to
extract CCs from the individual non-cover-letter patches and add them
to the cover letter.  That is the function I want - to do that, but
not run get_maintainer.

Nor do I need, I think, your tool to edit any of the non-cover-letter
patches, since git-send-email will find CCs in their bodies and use
them for the email recipients.

I don't think I really mind where the CCs end up in the cover letter
(whether they are in the body, or just in the email header), but
others have made a convincing argument that they should be in the
body, so that is fine.

Does that make sense ?

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/4] xen/PVH: Use proper CS selector in long mode

[Boris Ostrovsky]

Signed-off-by: Boris Ostrovsky 
Cc: sta...@vger.kernel.org
---
 arch/x86/xen/xen-pvh.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 934f7d4..373fef0 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -93,7 +93,7 @@ ENTRY(pvh_start_xen)
mov %eax, %cr0
 
/* Jump to 64-bit mode. */
-   ljmp $__KERNEL_CS, $_pa(1f)
+   ljmp $__BOOT_CS, $_pa(1f)
 
/* 64-bit entry point. */
.code64
-- 
2.9.3


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/4] xen/PVH: Set up GS segment for stack canary

[Boris Ostrovsky]

We are making calls to C code (e.g. xen_prepare_pvh()) which may use
stack canary (stored in GS segment).

Signed-off-by: Boris Ostrovsky 
Cc: sta...@vger.kernel.org
---
 arch/x86/xen/xen-pvh.S | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 373fef0..4eed586 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -54,6 +54,9 @@
  * charge of setting up it's own stack, GDT and IDT.
  */
 
+#define PVH_GDT_ENTRY_CANARY4
+#define PVH_CANARY_SEL  (PVH_GDT_ENTRY_CANARY * 8)
+
 ENTRY(pvh_start_xen)
cld
 
@@ -64,6 +67,9 @@ ENTRY(pvh_start_xen)
mov %eax,%es
mov %eax,%ss
 
+   mov $(PVH_CANARY_SEL),%eax
+   mov %eax,%gs
+
/* Stash hvm_start_info. */
mov $_pa(pvh_start_info), %edi
mov %ebx, %esi
@@ -150,6 +156,7 @@ gdt_start:
.quad 0x00cf9a00/* __BOOT_CS */
 #endif
.quad 0x00cf9200/* __BOOT_DS */
+   .quad 0x00409018/* PVH_CANARY_SEL */
 gdt_end:
 
.balign 4
-- 
2.9.3


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/4] xen/PVH: Remove reserved entry in PVH GDT

[Boris Ostrovsky]

And without it we can't use _BOOT_XX macros any longer so define new ones.

Signed-off-by: Boris Ostrovsky 
---
 arch/x86/xen/xen-pvh.S | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
index 4eed586..30dd067 100644
--- a/arch/x86/xen/xen-pvh.S
+++ b/arch/x86/xen/xen-pvh.S
@@ -54,7 +54,11 @@
  * charge of setting up it's own stack, GDT and IDT.
  */
 
-#define PVH_GDT_ENTRY_CANARY4
+#define PVH_GDT_ENTRY_CS1
+#define PVH_GDT_ENTRY_DS2
+#define PVH_GDT_ENTRY_CANARY3
+#define PVH_CS_SEL  (PVH_GDT_ENTRY_CS * 8)
+#define PVH_DS_SEL  (PVH_GDT_ENTRY_DS * 8)
 #define PVH_CANARY_SEL  (PVH_GDT_ENTRY_CANARY * 8)
 
 ENTRY(pvh_start_xen)
@@ -62,7 +66,7 @@ ENTRY(pvh_start_xen)
 
lgdt (_pa(gdt))
 
-   mov $(__BOOT_DS),%eax
+   mov $(PVH_DS_SEL),%eax
mov %eax,%ds
mov %eax,%es
mov %eax,%ss
@@ -99,7 +103,7 @@ ENTRY(pvh_start_xen)
mov %eax, %cr0
 
/* Jump to 64-bit mode. */
-   ljmp $__BOOT_CS, $_pa(1f)
+   ljmp $PVH_CS_SEL, $_pa(1f)
 
/* 64-bit entry point. */
.code64
@@ -122,13 +126,13 @@ ENTRY(pvh_start_xen)
or $(X86_CR0_PG | X86_CR0_PE), %eax
mov %eax, %cr0
 
-   ljmp $__BOOT_CS, $1f
+   ljmp $PVH_CS_SEL, $1f
 1:
call xen_prepare_pvh
mov $_pa(pvh_bootparams), %esi
 
/* startup_32 doesn't expect paging and PAE to be on. */
-   ljmp $__BOOT_CS, $_pa(2f)
+   ljmp $PVH_CS_SEL, $_pa(2f)
 2:
mov %cr0, %eax
and $~X86_CR0_PG, %eax
@@ -137,7 +141,7 @@ ENTRY(pvh_start_xen)
and $~X86_CR4_PAE, %eax
mov %eax, %cr4
 
-   ljmp $__BOOT_CS, $_pa(startup_32)
+   ljmp $PVH_CS_SEL, $_pa(startup_32)
 #endif
 END(pvh_start_xen)
 
@@ -149,13 +153,12 @@ gdt:
.word 0
 gdt_start:
.quad 0x/* NULL descriptor */
-   .quad 0x/* reserved */
 #ifdef CONFIG_X86_64
-   .quad 0x00af9a00/* __BOOT_CS */
+   .quad 0x00af9a00/* PVH_CS_SEL */
 #else
-   .quad 0x00cf9a00/* __BOOT_CS */
+   .quad 0x00cf9a00/* PVH_CS_SEL */
 #endif
-   .quad 0x00cf9200/* __BOOT_DS */
+   .quad 0x00cf9200/* PVH_DS_SEL */
.quad 0x00409018/* PVH_CANARY_SEL */
 gdt_end:
 
-- 
2.9.3


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[Jason Cooper]

Hi Ian,

On Mon, Apr 30, 2018 at 04:22:30PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?"):
> > To implement reuse_domid in a sane way, either the toolstack needs to
> > manage all domids and always sets domid when creating domain or the
> > hypervisor needs to cooperate -- to have interface to reserve /
> > pre-allocate domids.
> 
> I think this is entirely the wrong approach.

Whew.  Glad I didn't start hacking yet...

> I think the right answer is that this is simply a bug in the
> frontends.  frontends should cope if the backend path pointer in the
> frontend directory is updated, and should start reading the new
> backend instead.

Ok, so I'm new to the guts of Xen.  The bug, at a high level, is that
"When a driver domain is rebooted (domid changed), previously connected
client domUs can't gain network connectivity to/through the driver
domain via 'xl network-attach client_domu mac=... bridge=...
backend=drv_dom'"

This is due to the fact that the frontend net driver doesn't / can't
follow the backend driver to the new domid in xenstore.

Does that sound right?

> I'm a bit surprised that this doesn't already work.

I'm currently running Xen 4.9.1 as patched in the standard Gentoo
ebuild.  I've been putting off upgrading to 4.9.2, now marked stable in
portage, until I nail this down.  I'm happy to move to 4.10 if needed.

Do you think this is something that is definitely fixed in a more recent
version of Xen?  I'm happy to test if so.  Is there a commit id I can
look for?


thx,

Jason.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 10/10] xen/arm: Enable errata for secondary CPU on hotplug after the boot

[Julien Grall]

Hi Mirela,

On 27/04/18 18:12, Mirela Simonovic wrote:

On boot, enabling errata workarounds will be triggered by the boot CPU
from start_xen(). On CPU hotplug (non-boot scenario) this would not be
done. This patch adds the code required to enable errata workarounds
for a CPU being hotplugged after the system boots. This is triggered
using a notifier. If the CPU fails to enable the errata Xen will panic.
This is done because it is assumed that the CPU which is hotplugged
after the system/Xen boots, was initially hotplugged during the
system/Xen boot. Therefore, enabling errata workarounds should never
fail.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
  xen/arch/arm/cpuerrata.c | 35 +++
  xen/arch/arm/cpufeature.c| 23 +++
  xen/include/asm-arm/cpufeature.h |  1 +
  3 files changed, 59 insertions(+)

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 1baa20654b..4040f781ec 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -5,6 +5,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -349,6 +351,39 @@ void __init enable_errata_workarounds(void)
  enable_cpu_capabilities(arm_errata);
  }
  
+static int cpu_errata_callback(

+struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+switch ( action )
+{
+case CPU_STARTING:
+enable_nonboot_cpu_caps(arm_errata);
+break;
+default:
+break;
+}
+
+return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_errata_nfb = {
+.notifier_call = cpu_errata_callback,
+};
+
+static int __init cpu_errata_notifier_init(void)
+{
+register_cpu_notifier(_errata_nfb);
+return 0;
+}
+/*
+ * Initialization has to be done at init rather than presmp_init phase because
+ * the callback should execute only after the secondary CPUs are initially
+ * booted (in hotplug scenarios when the system state is not boot). On boot,
+ * the enabling of errata workarounds will be triggered by the boot CPU from
+ * start_xen().
+ */
+__initcall(cpu_errata_notifier_init);
+
  /*
   * Local variables:
   * mode: C
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index 525b45e22f..dd30f0d29c 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -68,6 +68,29 @@ void __init enable_cpu_capabilities(const struct 
arm_cpu_capabilities *caps)
  }
  }
  
+/* Run through the enabled capabilities and enable() them on the calling CPU */

+void enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps)
+{
+ASSERT(system_state != SYS_STATE_boot);
+
+for ( ; caps->matches; caps++ )
+{
+if ( !cpus_have_cap(caps->capability) )
+continue;
+
+if ( caps->enable )
+{
+/*
+ * Since the CPU has enabled errata workarounds on boot, it should


This function is not really about errata, it is about capabilities. 
Errata is just a sub-category of them.



+ * never fail to enable them here.
+ */
+if ( caps->enable((void *)caps) )
+panic("CPU%u failed to enable capability %u\n",
+  smp_processor_id(), caps->capability);


We should really avoid to use panic(...) if this is something the system 
can survive. In that specific case, it would only affect the current 
CPU. So it would be better to return an error and let the caller decide 
what to do.


Cheers,


+}
+}
+}
+
  /*
   * Local variables:
   * mode: C
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index e557a095af..b14e226401 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -88,6 +88,7 @@ void update_cpu_capabilities(const struct 
arm_cpu_capabilities *caps,
   const char *info);
  
  void enable_cpu_capabilities(const struct arm_cpu_capabilities *caps);

+void enable_nonboot_cpu_caps(const struct arm_cpu_capabilities *caps);
  
  #endif /* __ASSEMBLY__ */
  



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 1/2] SVM: re-work VMCB sync-ing

[Jan Beulich]

>>> On 30.04.18 at 17:56,  wrote:
> On 04/30/2018 11:50 AM, Jan Beulich wrote:
> On 30.04.18 at 17:30,  wrote:
>>> On 04/30/2018 07:37 AM, Jan Beulich wrote:
 @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc
  
  hvm_do_resume(v);
  
 +if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
 +svm_sync_vmcb(v, vmcb_needs_vmsave);
>>>
>>> Is it not possible (or advisable) to move the test into svm_sync_vmcb()
>>> (and drop the ASSERT there)?
>> It is possible; I'm not sure myself if it's advisable, but I take you asking 
>> the
>> question as you thinking it is, so I'll change it.
> 
> That was really the main reason for me asking to move svm_vmload into
> svm_sync_vmcb() --- to hide all logic related to the sync state machine
> there.

Well, there's still the code in svm_vmexit_do_vmload(). Depending on
your opinion on the post-commit-message question in patch 2, the one
at the top of svm_vmexit_handler() might go away in that patch.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [xen-unstable-smoke test] 122540: tolerable all pass - PUSHED

[osstest service owner]

flight 122540 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/122540/

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  08641a9e8870d3b174d95aaa55ecba43387563b5
baseline version:
 xen  eff2fbe4dd71b3e4fe2dbb2696882252c1cc7897

Last test of basis   122471  2018-04-27 15:00:34 Z3 days
Testing same since   122540  2018-04-30 14:00:50 Z0 days1 attempts


People who touched revisions under test:
  Ian Jackson 
  Stewart Hildebrand 

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-i386 pass
 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
   eff2fbe4dd..08641a9e88  08641a9e8870d3b174d95aaa55ecba43387563b5 -> smoke

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 08/10] xen/arm: Release timer interrupts when CPU is hot-unplugged

[Julien Grall]

Hi,

On 27/04/18 18:12, Mirela Simonovic wrote:

When a CPU is hot-unplugged timer interrupts have to be released
in order to free the memory that was allocated when the interrupts
were requested (using request_irq()). The request_irq is called
for each timer interrupt when the CPU gets hotplugged
(start_secondary->init_timer_interrupt->request_irq).
With this patch the timer interrupts will be released when the
newly added callback receives CPU_DYING event.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v3:
-Trigger releasing of timer interrupts using notifiers
---
  xen/arch/arm/time.c | 39 +++
  1 file changed, 39 insertions(+)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index c11fcfeadd..c7317e4639 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -29,6 +29,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -312,6 +314,17 @@ void init_timer_interrupt(void)
  check_timer_irq_cfg(timer_irq[TIMER_PHYS_NONSECURE_PPI], "NS-physical");
  }
  
+/*

+ * Revert actions done in init_timer_interrupt that are required to properly
+ * disable this CPU.
+ */
+static void deinit_timer_interrupt(void)
+{


Any reason to not disable the timer here? But I think we need to finish 
the discussion on the previous series regarding the purpose of the 
mdelay before going further with that patch. See patch v2 7/10. I would 
also appreciate answer to my question there.



+release_irq(timer_irq[TIMER_HYP_PPI], NULL);
+release_irq(timer_irq[TIMER_VIRT_PPI], NULL);
+release_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], NULL);
+}
+
  /* Wait a set number of microseconds */
  void udelay(unsigned long usecs)
  {
@@ -340,6 +353,32 @@ void domain_set_time_offset(struct domain *d, int64_t 
time_offset_seconds)
  /* XXX update guest visible wallclock time */
  }
  
+static int cpu_time_callback(

+struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+switch ( action )
+{
+case CPU_DYING:
+deinit_timer_interrupt();
+break;
+default:
+break;
+}
+
+return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_time_nfb = {
+.notifier_call = cpu_time_callback,
+};
+
+static int __init cpu_time_notifier_init(void)
+{
+register_cpu_notifier(_time_nfb);
+return 0;
+}
+__initcall(cpu_time_notifier_init);
+
  /*
   * Local variables:
   * mode: C



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 1/2] SVM: re-work VMCB sync-ing

[Boris Ostrovsky]

On 04/30/2018 11:50 AM, Jan Beulich wrote:
 On 30.04.18 at 17:30,  wrote:
>> On 04/30/2018 07:37 AM, Jan Beulich wrote:
>>> @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc
>>>  
>>>  hvm_do_resume(v);
>>>  
>>> +if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
>>> +svm_sync_vmcb(v, vmcb_needs_vmsave);
>>
>> Is it not possible (or advisable) to move the test into svm_sync_vmcb()
>> (and drop the ASSERT there)?
> It is possible; I'm not sure myself if it's advisable, but I take you asking 
> the
> question as you thinking it is, so I'll change it.

That was really the main reason for me asking to move svm_vmload into
svm_sync_vmcb() --- to hide all logic related to the sync state machine
there.


-boris

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RFC 1/7] xen: Introduce XEN_COMPILE_POSIX_TIME

[Jan Beulich]

>>> On 08.07.17 at 23:53,  wrote:
> We need the POSIX time to properly fill the TimeDateStamp field in the PE 
> header.
> 
> Signed-off-by: Daniel Kiper 
> ---
>  xen/Makefile |   14 --
>  xen/include/xen/compile.h.in |1 +
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index f6a6bc2..2424690 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -6,12 +6,13 @@ export XEN_EXTRAVERSION ?= -unstable$(XEN_VENDORVERSION)
>  export XEN_FULLVERSION   = 
> $(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION)
>  -include xen-version
>  
> -export XEN_WHOAMI?= $(USER)
> -export XEN_DOMAIN?= $(shell ([ -x /bin/dnsdomainname ] && 
> /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo 
> [unknown]))
> -export XEN_BUILD_DATE?= $(shell LC_ALL=C date)
> -export XEN_BUILD_TIME?= $(shell LC_ALL=C date +%T)
> -export XEN_BUILD_HOST?= $(shell hostname)
> -export XEN_CONFIG_EXPERT ?= n
> +export XEN_WHOAMI?= $(USER)
> +export XEN_DOMAIN?= $(shell ([ -x /bin/dnsdomainname ] && 
> /bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo 
> [unknown]))
> +export XEN_BUILD_DATE?= $(shell LC_ALL=C date)
> +export XEN_BUILD_TIME?= $(shell LC_ALL=C date +%T)
> +export XEN_BUILD_POSIX_TIME  ?= $(shell LC_ALL=C date +%s)

If you run two independent commands anyway, I don't see why you need to
obtain the POSIX time here. If you're really after the time stamps agreeing,
then you should invoke date only once (strictly speaking this also applies to
the DATE vs TIME invocation, but lets hope people sleep at midnight rather
than building Xen).

> @@ -164,6 +165,7 @@ delete-unfresh-files:
>  include/xen/compile.h: include/xen/compile.h.in .banner
>   @sed -e 's/@@date@@/$(XEN_BUILD_DATE)/g' \
>   -e 's/@@time@@/$(XEN_BUILD_TIME)/g' \
> + -e 's/@@posix_time@@/$(XEN_BUILD_POSIX_TIME)/g' \

In order to fill a PE header, do you really need to make this available in
compile.h?

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 09/10] xen/arm: Free memory allocated for sibling/core maps on CPU hot-unplug

[Julien Grall]

Hi Mirela,

On 27/04/18 18:12, Mirela Simonovic wrote:

The memory allocated in setup_cpu_sibling_map() when a CPU is hotplugged
has to be freed when the CPU is hot-unplugged. This is done in
remove_cpu_sibling_map() and called when the CPU dies. The call to
remove_cpu_sibling_map() is made from a notifier callback when
CPU_DEAD event is received.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v3:
-Use notifier to trigger remove_cpu_sibling_map() when the CPU dies.
---
  xen/arch/arm/smpboot.c | 34 ++
  1 file changed, 34 insertions(+)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ad1f6b751b..b833e3a754 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -89,6 +89,12 @@ static void setup_cpu_sibling_map(int cpu)
  cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
  }
  
+static void remove_cpu_sibling_map(int cpu)

+{
+free_cpumask_var(per_cpu(cpu_sibling_mask, cpu));
+free_cpumask_var(per_cpu(cpu_core_mask, cpu));
+}
+
  void __init
  smp_clear_cpu_maps (void)
  {
@@ -499,6 +505,34 @@ void __cpu_die(unsigned int cpu)
  smp_mb();
  }
  
+static int cpu_smpboot_callback(

+struct notifier_block *nfb, unsigned long action, void *hcpu)
+{
+unsigned int cpu = (unsigned long)hcpu;
+
+switch ( action )
+{
+case CPU_DEAD:
+remove_cpu_sibling_map(cpu);
+break;
+default:
+break;
+}
+
+return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_smpboot_nfb = {
+.notifier_call = cpu_smpboot_callback,
+};
+
+static int __init cpu_smpboot_notifier_init(void)
+{
+register_cpu_notifier(_smpboot_nfb);
+return 0;
+}
+__initcall(cpu_smpboot_notifier_init);


I think this notifiers should go in preinit smp to cover the case where 
a secondary CPU dies beforehand the initcall.


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 1/2] SVM: re-work VMCB sync-ing

[Jan Beulich]

>>> On 30.04.18 at 17:30,  wrote:
> On 04/30/2018 07:37 AM, Jan Beulich wrote:
>> @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc
>>  
>>  hvm_do_resume(v);
>>  
>> +if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
>> +svm_sync_vmcb(v, vmcb_needs_vmsave);
> 
> 
> Is it not possible (or advisable) to move the test into svm_sync_vmcb()
> (and drop the ASSERT there)?

It is possible; I'm not sure myself if it's advisable, but I take you asking the
question as you thinking it is, so I'll change it.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 0/5] VMX MSRs policy for Nested Virt: part 1

[Jan Beulich]

>>> On 22.03.18 at 12:32,  wrote:
> The end goal of having VMX MSRs policy is to be able to manage
> L1 VMX features. This patch series is the first part of this work.
> There is no functional change to what L1 sees in VMX MSRs at this
> point. But each domain will have a policy object which allows to
> sensibly query what VMX features the domain has. This will unblock
> some other nested virtualization work items.
> 
> Currently, when nested virt is enabled, the set of L1 VMX features
> is fixed and calculated by nvmx_msr_read_intercept() as an intersection
> between the full set of Xen's supported L1 VMX features, the set of
> actual H/W features and, for MSR_IA32_VMX_EPT_VPID_CAP, the set of
> features that Xen uses.
> 
> The above makes L1 VMX feature set inconsistent between different H/W
> and there is no ability to control what features are available to L1.
> The overall set of issues has much in common with CPUID policy.
> 
> Part 1 adds VMX MSRs into struct msr_domain_policy and initializes them
> during domain creation based on CPUID policy. In the future it should be
> possible to independently configure values of VMX MSRs for each domain.
> 
> v5 --> v6:
> - Various shortenings of control bit names
> - Added Reviewed-by: Andrew Cooper to pathes 3,4 and 5
> - Other changes are provided on per-patch basis
> 
> Sergey Dyasli (5):
>   x86/msr: add VMX MSRs definitions and populate Raw domain policy
>   x86/msr: add VMX MSRs into HVM_max domain policy
>   x86/cpuid: update signature of hvm_cr4_guest_valid_bits()
>   x86/msr: update domain policy on CPUID policy changes
>   x86/msr: handle VMX MSRs with guest_rd/wrmsr()

Provided there's going to be a R-b by one of the VMX maintainers,
Acked-by: Jan Beulich 

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RESEND v1 3/7] x86: add intel proecessor trace support for cpuid

[Konrad Rzeszutek Wilk]

On Tue, Jan 16, 2018 at 02:12:29AM +0800, Luwei Kang wrote:
> This patch add Intel processor trace support
> for cpuid handling.

The 0x14 usage screams of wanting an #define.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling

[Konrad Rzeszutek Wilk]

On Tue, Jan 16, 2018 at 02:12:26AM +0800, Luwei Kang wrote:
> Hi All,
> 
> Here is a patch-series which adding Processor Trace enabling in XEN guest. 
> You can get It's software developer manuals from:
> https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf
> In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS.

Now Chapter 4.
> 
> Introduction:
> Intel Processor Trace (Intel PT) is an extension of Intel Architecture that 
> captures information about software execution using dedicated hardware 
> facilities that cause only minimal performance perturbation to the software 
> being traced. Details on the Intel PT infrastructure and trace capabilities 
> can be found in the Intel 64 and IA-32 Architectures Software Developer’s 
> Manual, Volume 3C.
> 
> The suite of architecture changes serve to simplify the process of 
> virtualizing Intel PT for use by a guest software. There are two primary 
> elements to this new architecture support for VMX support improvements made 
> for Intel PT.
> 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS.
>   — This serves to speed and simplify the process of disabling trace on VM 
> exit, and restoring it on VM entry.
> 2. Enabling use of EPT to redirect PT output.
>   — This enables the VMM to elect to virtualize the PT output buffer using 
> EPT. In this mode, the CPU will treat PT output addresses as Guest Physical 
> Addresses (GPAs) and translate them using EPT. This means that Intel PT 
> output reads (of the ToPA table) and writes (of trace output) can cause EPT 
> violations, and other output events.
> 

How does one test this functionality in Linux? As in does 'perf' take advantage 
of the Processor Trace
functionality? 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [xen-4.7-testing test] 122500: trouble: broken/fail/pass

[osstest service owner]

flight 122500 xen-4.7-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/122500/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-vhd  broken
 test-armhf-armhf-xl-vhd   4 host-install(4)broken REGR. vs. 122131

Tests which did not succeed, but are not blocking:
 test-xtf-amd64-amd64-5  50 xtf/test-hvm64-lbr-tsx-vmentry fail like 122131
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 122131
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 122131
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 122131
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 122131
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 122131
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 122131
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 122131
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 122131
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 122131
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 122131
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 122131
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 122131
 test-amd64-amd64-libvirt-xsm 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-amd64-i386-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-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  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-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-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-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 migrate-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-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass

version targeted for testing:
 xen  a8ef07566fa8fe9a2e8db745014d93e259b66785
baseline version:
 xen  9680710bed1c174ced7a170cb94e30b4ae4fff5e

Last test of basis   122131  2018-04-09 10:53:16 Z   21 days
Failing since122353  2018-04-23 11:05:56 Z7 days5 attempts
Testing same since   122459  2018-04-27 07:28:31 Z3 days2 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 

Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant map/unmap

[Paul Durrant]

> -Original Message-
> From: Roger Pau Monne
> Sent: 30 April 2018 16:28
> To: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org; qemu-bl...@nongnu.org; qemu-
> de...@nongnu.org; Anthony Perard ; Kevin
> Wolf ; Stefano Stabellini ; Max
> Reitz 
> Subject: Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant
> map/unmap
> 
> On Mon, Apr 30, 2018 at 04:16:52PM +0100, Paul Durrant wrote:
> > > -Original Message-
> > > From: Roger Pau Monne
> > > Sent: 30 April 2018 16:12
> > > To: Paul Durrant 
> > > Cc: xen-devel@lists.xenproject.org; qemu-bl...@nongnu.org; qemu-
> > > de...@nongnu.org; Anthony Perard ;
> Kevin
> > > Wolf ; Stefano Stabellini ;
> Max
> > > Reitz 
> > > Subject: Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of
> grant
> > > map/unmap
> > >
> > > On Mon, Apr 30, 2018 at 01:01:37PM +0100, Paul Durrant wrote:
> > > > The grant copy operation was added to libxengnttab in Xen 4.8.0. If
> grant
> > > > copy is available then data from the guest will be copied rather than
> > > > mapped.
> > > > The xen_disk source can be significantly simplified by removing this
> now
> > > > redundant code.
> > >
> > > Hm, I know this is a PITA, but FreeBSD gntdev hasn't implemented the
> > > grant-copy operation yet.
> > >
> > > I could try to implement it, but I can't make any promises on the time
> > > ATM, since I'm quite busy.
> > >
> >
> > I guess we could carry a compat patch in QEMU that implements grant copy
> by doing a map/memcpy/unmap , but QEMU feels like the wrong place for
> that. I could try putting together a similar patch for the freebsd.c component
> of libxengnttab in the xen source rather than it simply failing with ENOSYS as
> it does now. Would either of those help?
> 
> Maybe this could be implemented in gnttab_core.c, so it can also be
> used by MiniOS and Linux versions not supporting the copy ioctl as a
> fallback?

That sounds like a reasonable idea. I'll put something together so that it can 
go in early in 4.12.

  Paul

> 
> Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant map/unmap

[Roger Pau Monné]

On Mon, Apr 30, 2018 at 04:16:52PM +0100, Paul Durrant wrote:
> > -Original Message-
> > From: Roger Pau Monne
> > Sent: 30 April 2018 16:12
> > To: Paul Durrant 
> > Cc: xen-devel@lists.xenproject.org; qemu-bl...@nongnu.org; qemu-
> > de...@nongnu.org; Anthony Perard ; Kevin
> > Wolf ; Stefano Stabellini ; Max
> > Reitz 
> > Subject: Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant
> > map/unmap
> > 
> > On Mon, Apr 30, 2018 at 01:01:37PM +0100, Paul Durrant wrote:
> > > The grant copy operation was added to libxengnttab in Xen 4.8.0. If grant
> > > copy is available then data from the guest will be copied rather than
> > > mapped.
> > > The xen_disk source can be significantly simplified by removing this now
> > > redundant code.
> > 
> > Hm, I know this is a PITA, but FreeBSD gntdev hasn't implemented the
> > grant-copy operation yet.
> > 
> > I could try to implement it, but I can't make any promises on the time
> > ATM, since I'm quite busy.
> > 
> 
> I guess we could carry a compat patch in QEMU that implements grant copy by 
> doing a map/memcpy/unmap , but QEMU feels like the wrong place for that. I 
> could try putting together a similar patch for the freebsd.c component of 
> libxengnttab in the xen source rather than it simply failing with ENOSYS as 
> it does now. Would either of those help?

Maybe this could be implemented in gnttab_core.c, so it can also be
used by MiniOS and Linux versions not supporting the copy ioctl as a
fallback?

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] SVM: re-work VMCB sync-ing

[Boris Ostrovsky]

On 04/30/2018 07:37 AM, Jan Beulich wrote:
> @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc
>  
>  hvm_do_resume(v);
>  
> +if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
> +svm_sync_vmcb(v, vmcb_needs_vmsave);


Is it not possible (or advisable) to move the test into svm_sync_vmcb()
(and drop the ASSERT there)?

-boris

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] x86/microcode: Synchronize late microcode loading

[Jan Beulich]

>>> On 25.04.18 at 13:46,  wrote:
> @@ -281,24 +288,56 @@ static int microcode_update_cpu(const void *buf, size_t 
> size)
>  return err;
>  }
>  
> -static long do_microcode_update(void *_info)
> +/* Wait for all CPUs to rendezvous with a timeout (us) */
> +static int wait_for_cpus(atomic_t *cnt, int timeout)

unsigned int

> +static int do_microcode_update(void *_info)
> +{
> +struct microcode_info *info = _info;
> +unsigned int cpu = smp_processor_id();
> +int ret;
> +
> +ret = wait_for_cpus(>cpu_in, MICROCODE_DEFAULT_TIMEOUT);
> +if ( ret )
> +return ret;
> +/*
> + * Logical threads which set the first bit in cpu_sibling_mask can do
> + * the update. Other sibling threads just await the completion of
> + * microcode update.
> + */
> +if ( cpumask_test_and_set_cpu(
> +cpumask_first(per_cpu(cpu_sibling_mask, cpu)), >cpus) )
> +ret = microcode_update_cpu(info->buffer, info->buffer_size);

Isn't the condition inverted (i.e. missing a ! )?

Also I take it that you've confirmed that loading ucode in parallel on multiple
cores of the same socket is not a problem? The comment in the last hunk
suggests otherwise.

> +/*
> + * Increase the wait timeout to a safe value here since we're serializing
> + * the microcode update and that could take a while on a large number of
> + * CPUs. And that is fine as the *actual* timeout will be determined by
> + * the last CPU finished updating and thus cut short
> + */
> +if ( wait_for_cpus(>cpu_out, MICROCODE_DEFAULT_TIMEOUT *
> +   num_online_cpus()) )
> +panic("Timeout when finishing updating microcode");

A 3s timeout (as an example for a system with 100 CPU threads) is still
absurdly high to me, but considering you panic() anyway if you hit the
timeout the question mainly is whether there's a slim chance for this to
complete a brief moment before the timeout expires. If all goes well,
you won't come close to even 1s, but as said before - there may be
guests running, and they may become utterly confused if they don't
get any time within a second or more.

With you no longer doing things sequentially I don't, however, see why
you need to scale the timeout by CPU count.

> +
> +return ret;
>  }

You're losing this return value (once for every CPU making it into this
function).

> @@ -318,26 +357,52 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) 
> buf, unsigned long len)
>  
>  ret = copy_from_guest(info->buffer, buf, len);
>  if ( ret != 0 )
> -{
> -xfree(info);
> -return ret;
> -}
> +goto free;
>  
>  info->buffer_size = len;
> -info->error = 0;
> -info->cpu = cpumask_first(_online_map);
> +
> +/* cpu_online_map must not change during update */
> +if ( !get_cpu_maps() )
> +{
> +ret = -EBUSY;
> +goto free;
> +}
>  
>  if ( microcode_ops->start_update )
>  {
>  ret = microcode_ops->start_update();
>  if ( ret != 0 )
> -{
> -xfree(info);
> -return ret;
> -}
> +goto put;
>  }
>  
> -return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> +cpumask_empty(>cpus);

DYM cpumask_clear()?

> +atomic_set(>cpu_in, 0);
> +atomic_set(>cpu_out, 0);
> +
> +/*
> + * We intend to disable interrupt for long time, which may lead to
> + * watchdog timeout.
> + */
> +watchdog_disable();
> +/*
> + * Late loading dance. Why the heavy-handed stop_machine effort?
> + *
> + * -HT siblings must be idle and not execute other code while the other
> + *  sibling is loading microcode in order to avoid any negative
> + *  interactions cause by the loading.
> + *
> + * -In addition, microcode update on the cores must be serialized until
> + *  this requirement can be relaxed in the feature. Right now, this is
> + *  conservative and good.

This is the comment I've referred to above.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] reboot driver domain, vifX.Y = NO-CARRIER?"):
> To implement reuse_domid in a sane way, either the toolstack needs to
> manage all domids and always sets domid when creating domain or the
> hypervisor needs to cooperate -- to have interface to reserve /
> pre-allocate domids.

I think this is entirely the wrong approach.

I think the right answer is that this is simply a bug in the
frontends.  frontends should cope if the backend path pointer in the
frontend directory is updated, and should start reading the new
backend instead.

I'm a bit surprised that this doesn't already work.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant map/unmap

[Paul Durrant]

> -Original Message-
> From: Roger Pau Monne
> Sent: 30 April 2018 16:12
> To: Paul Durrant 
> Cc: xen-devel@lists.xenproject.org; qemu-bl...@nongnu.org; qemu-
> de...@nongnu.org; Anthony Perard ; Kevin
> Wolf ; Stefano Stabellini ; Max
> Reitz 
> Subject: Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant
> map/unmap
> 
> On Mon, Apr 30, 2018 at 01:01:37PM +0100, Paul Durrant wrote:
> > The grant copy operation was added to libxengnttab in Xen 4.8.0. If grant
> > copy is available then data from the guest will be copied rather than
> > mapped.
> > The xen_disk source can be significantly simplified by removing this now
> > redundant code.
> 
> Hm, I know this is a PITA, but FreeBSD gntdev hasn't implemented the
> grant-copy operation yet.
> 
> I could try to implement it, but I can't make any promises on the time
> ATM, since I'm quite busy.
> 

I guess we could carry a compat patch in QEMU that implements grant copy by 
doing a map/memcpy/unmap , but QEMU feels like the wrong place for that. I 
could try putting together a similar patch for the freebsd.c component of 
libxengnttab in the xen source rather than it simply failing with ENOSYS as it 
does now. Would either of those help?

  Cheers,

Paul

> Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant map/unmap

[Roger Pau Monné]

On Mon, Apr 30, 2018 at 01:01:37PM +0100, Paul Durrant wrote:
> The grant copy operation was added to libxengnttab in Xen 4.8.0. If grant
> copy is available then data from the guest will be copied rather than
> mapped.
> The xen_disk source can be significantly simplified by removing this now
> redundant code.

Hm, I know this is a PITA, but FreeBSD gntdev hasn't implemented the
grant-copy operation yet.

I could try to implement it, but I can't make any promises on the time
ATM, since I'm quite busy.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged

[Julien Grall]

Hi Mirela,

On 27/04/18 18:12, Mirela Simonovic wrote:

When a CPU is hot-unplugged the maintenance interrupt has to be
released in order to free the memory that was allocated when the CPU
was hotplugged and interrupt requested. The interrupt was requested
using request_irq() which is called from start_secondary->
init_maintenance_interrupt. With this patch the interrupt will be
released when the CPU_DYING event is received by the callback which
is added in gic.c.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v3:
-Add notifier in order to trigger releasing of the  maintenance
  interrupt when the CPU is dying.
---
  xen/arch/arm/gic.c | 29 +
  1 file changed, 29 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 653a815127..89abc49950 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -27,6 +27,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -462,6 +464,33 @@ int gic_iomem_deny_access(const struct domain *d)
  return gic_hw_ops->iomem_deny_access(d);
  }
  
+static int cpu_gic_callback(

+struct notifier_block *nfb, unsigned long action, void *hcpu)


Please fix the indentation.

With that:

Acked-by: Julien Grall 


+{
+switch ( action )
+{
+case CPU_DYING:
+/* This is reverting the work done in init_maintenance_interrupt */


In the future we probably want to move init_maintenance_interrupt in the 
notifier. But that's a clean-up after this has been merged :).



+release_irq(gic_hw_ops->info->maintenance_irq, NULL);
+break;
+default:
+break;
+}
+
+return NOTIFY_DONE;
+}
+
+static struct notifier_block cpu_gic_nfb = {
+.notifier_call = cpu_gic_callback,
+};
+
+static int __init cpu_gic_notifier_init(void)
+{
+register_cpu_notifier(_gic_nfb);
+return 0;
+}
+__initcall(cpu_gic_notifier_init);
+
  /*
   * Local variables:
   * mode: C



Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume

[Julien Grall]

Hi Mirela,

On 27/04/18 18:12, Mirela Simonovic wrote:

In existing code the virtual paging for non-boot CPUs is setup only on boot.
The setup is triggered from start_xen() after all CPUs are brought online.
In other words, the initialization of VTCR_EL2 register is done out of the
cpu_up/start_secondary() control flow. However, the cpu_up flow is also used
to hotplug non-boot CPUs on resume from suspend to RAM state, in which case
the virtual paging will not be configured.

With this patch the setting of paging is triggered from start_secondary()
function using cpu starting notifier (notify_cpu_starting() call). The
notifier is registered in p2m.c using init call. This has to be done with
init call rather than presmp_init because the registered callback depends
on vtcr configuration value which is setup after the presmp init calls
are executed (do_presmp_initcalls() called from start_xen()). Init calls
are executed after initial virtual paging is set up for all CPUs on boot.
This ensures that no callback can fire until the vtcr value is calculated
by Xen and virtual paging is set up initially for all CPUs. Also, this way
the virtual paging setup in boot scenario remains unchanged.

It is assumed here that after the system completed the boot, CPUs that
execute start_secondary() were booted as well when the Xen itself was
booted. According to this assumption non-boot CPUs will always be compliant
with the VTCR_EL2 value that was selected by Xen on boot.
Currently, there is no mechanism to trigger hotplugging of a CPU. This
will be added with the suspend to RAM support for ARM, where the hotplug
of non-boot CPUs will be triggered via enable_nonboot_cpus() call.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Fix commit message
-Save configured VTCR_EL2 value into static variable that will be used
  by non-boot CPUs on hotplug
-Add setup_virt_paging_secondary() and invoke it from start_secondary()
  if that CPU has to setup virtual paging (if the system state is not boot)

Changes in v3:
-Fix commit message
-Remove setup_virt_paging_secondary() and use notifier to setup virtual
  paging for non-boot CPU on hotplug.
-In setup_virt_paging() use vtcr static variable instead of local val
-In setup_virt_paging_one() use vtcr static variable instead of provided
  argument
---
  xen/arch/arm/p2m.c | 82 +-
  1 file changed, 62 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d43c3aa896..98a1fe6de9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -8,6 +8,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 


Please add them alphabetically.


  #include 
  #include 
  #include 
@@ -1451,24 +1453,17 @@ err:
  return page;
  }
  
-static void __init setup_virt_paging_one(void *data)

+/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
+static uint64_t __read_mostly vtcr;
+
+static void setup_virt_paging_one(void *data)
  {
-unsigned long val = (unsigned long)data;
-WRITE_SYSREG32(val, VTCR_EL2);
+WRITE_SYSREG32(vtcr, VTCR_EL2);
  isb();
  }
  
  void __init setup_virt_paging(void)

  {
-/* Setup Stage 2 address translation */
-unsigned long val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
-
-#ifdef CONFIG_ARM_32
-printk("P2M: 40-bit IPA\n");
-p2m_ipa_bits = 40;
-val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
-val |= VTCR_SL0(0x1); /* P2M starts at first level */
-#else /* CONFIG_ARM_64 */
  const struct {
  unsigned int pabits; /* Physical Address Size */
  unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
@@ -1491,6 +1486,16 @@ void __init setup_virt_paging(void)
  unsigned int pa_range = 0x10; /* Larger than any possible value */
  bool vmid_8_bit = false;


That's not going to build on arm32 because vmid_8_bit & co are not used. 
While I will not ask you to test the changes on 32-bit board, I would at 
least want each change to be build test it on impacted architecture.


In that particular case, you can just move the initialization of vtcr at 
after the endif because there is nothing that required that to be setup 
very early.


  
+/* Setup Stage 2 address translation */

+vtcr = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
+
+#ifdef CONFIG_ARM_32
+printk("P2M: 40-bit IPA\n");
+p2m_ipa_bits = 40;
+vtcr |= VTCR_T0SZ(0x18); /* 40 bit IPA */
+vtcr |= VTCR_SL0(0x1); /* P2M starts at first level */
+#else /* CONFIG_ARM_64 */
+
  for_each_online_cpu ( cpu )
  {
  const struct cpuinfo_arm *info = _data[cpu];
@@ -1513,14 +1518,14 @@ void __init setup_virt_paging(void)
  if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
!pa_range_info[pa_range].pabits )
  panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", 

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Jan Beulich]

>>> On 30.04.18 at 16:41,  wrote:
> Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) 
> to MAINTAINERS (plus a test case)"):
>> On 30.04.18 at 15:29,  wrote:
>> > That is clearer: I copied the text from the Linux maintainers file. 
>> 
>> Ah, indeed. So far it wasn't really clear to me whether "designated" implies
>> further privilege.
> 
> Right.  It does have such a connotation.  Hence my suggestion.
> I take it that you are happy with this patch pair now.

Yes.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) 
to MAINTAINERS (plus a test case)"):
> On 30.04.18 at 15:29,  wrote:
> > That is clearer: I copied the text from the Linux maintainers file. 
> 
> Ah, indeed. So far it wasn't really clear to me whether "designated" implies
> further privilege.

Right.  It does have such a connotation.  Hence my suggestion.
I take it that you are happy with this patch pair now.

> > @Jan: let me know whether you want me to re-roll the series with the text 
> > change.
> 
> Since I take Ian's reply as him volunteering to commit both, that would then
> really be up to him.

Well, I wouldn't want to just commit willy-nilly something that you
had unresolved questions about.  But I think we are all content now,
so, yes, Lars, please respin the two patches with that text.  In the
right order please :-).

Thanks,
Ia.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to 
MAINTAINERS (plus a test case)"):
> On 30/04/2018, 14:39, "George Dunlap"  wrote:
>
> I wouldn't object to someone checking it in now, however; I think all
> the committers have had a chance to object, and most have expressed 
> support.
> 
> Agreed. And it was discussed at the x86 community call, which is why I put 
> the patch together

I'm sure that the x86 community call is very useful and as you can
tell I support this proposal, but the x86 community call cannot make
decisions about what should be committed.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH for-4.11 v2 1/2] Replace occurances of xen.org 
with xenproject.org"):
> On 30/04/2018, 14:38, "Ian Jackson"  wrote:
> 
> Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org 
> with xenproject.org"):
> > This is a general clean-up activity. It also avoids mails being
> > sent to xen-devel@lists.xenproject.org and xen-de...@lists.xen.org
> > when used with add_maintainers.pl/git send-email
> 
> Acked-by: Ian Jackson 
> 
> It would be nice to replace many of the http:// urls with https.  But
> that shouldn't block this patch.
>
> Sure, I can do that. I don't mind re-sending it with those changes.

Thanks - but as a separate patch, if you do, please !

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Ian Jackson]

Thanks for this.  It looks like it will be very useful.  I have
reviewed your script in detail.

My review is quite picky, mainly about error handling.

Also I have a lot of style comments: where I say "so and so would be
more perlish" you should feel free to leave it as it is if you prefer.
(I would need some convincing that the continued use of `index' was
appropriate.)

Lars Kurth writes ("[PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script 
to optimise the workflow when using git format-patch with get_maintainer.pl"):
...
> +my $toolversion = "1.0";

I think in practice this version number will quickly become out of
date as it won't be updated.  I think it is harmless but I would drop
it.

> +my @tags= ("Acked-by:",
> +   "Release-acked-by:",
> +   "Reviewed-by:");
> +my $CC  = "Cc:"; # Note: git-send-mail requires Cc:
> +my $TO  = "To:";
> +my $AT  = "@";

This is a very odd variable and needs to be abolished.

> +my $cc_insert_before= "Signed-off-by:";
> +my $to_insert_before= "Date:";
> +my $cover_letter= "-cover-letter.patch";
> +my $get_maintainer  = "./scripts/get_maintainer.pl";
> +my $patch_ext   = ".patch";
> +my $mailing_lists   = $AT."lists.";

I notice that your usual quoting character for literal strings is "
rather than '.  You might find it easier to make more use of ', which
does much less interpolation.

> +my @lists; #Needed for < +my $usage = < +USAGE: $tool [options] (--patchdir|-d) 
> +VERSION: $toolversion
> +
> +OPTIONS:
> +
> +  [(--reroll-count|-v) ]

This syntax with the ( ) seems clumsy to me.  I would just write

> +  --reroll-count  | -v

but this is a matter of taste, so no reason to withhold my ack.

> +  [--verbose]
> +Show more output

These [ ] are clearly wrong.

> +  [--version]
> +Show version
> +  --h|help|usage

This is clearly wrong because it's -h, not --h.  And spaces make this
easier to read.  I suggest

  +  -h | --help | --usage

(TBH I don't see the need to support --usage, but whatever.)

> +if ($help != 0) {

Conventional (idiomatic perl) style would be

   if ($help) {

(throughout).  But what you have is not wrong.

> +if (! -e $get_maintainer) {
> +die "$tool: The tool requires $get_maintainer\n";
> +}

You may remember me saying I wanted a mode that simply transfers
maintainer information already provided in the patches.  That is
useful when the CCs have been manually specified.

I don't think such a mode is essential for me to ack this patch.

But in that mode get_maintainer is not essential.

In any case, if you do want to check it, you should stat, rather than
using a file test.  File tests on other than _ make it hard to produce
correct and useful messages on failure.  In this case, you fail to
print the errno, which you could do if you called stat.

Anyway, is it really worthwhile specifically testing that
get_maintainer exists ?  If it doesn't presumably you will try to run
it, and then get an error later which will print something sensible ?

> +if (! -e $patch_dir) {
> +die "$tool: Directory $patch_dir does not exist\n";
> +}

Same comment as above about stat.  Also, again, it might be better not
to check and simply allow the later code to handle errors correctly.

> +# Get the list of patches
> +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;

This goes wrong if $patch_dir (or $patch_prefix) contains whitespace
or glob characters.  This will be fine in any reasonable Unix
environment, but there are corner cases where it may go wrong.  For
example, I am told that modern desktop environments mount removeable
storage media on a pathname containing the volume label (this seems
very unwise to me, but there you are).

I don't think this is a problem for this script, but I thought I would
mention it.

> +my @patches = glob($pattern);

You should set $!=0 first because that makes proper error handling
possible.  The error handling should come immediately after the call,
so dealing with that now:

> +if (!scalar @patches) {
> +die "$tool: Directory $patch_dir contains no patches\n";
> +}

Here you should check $!.  If $! then there was a read error on the
directory, which should be reported (with the $! value).  Doing this
also obviates the need to check $patch_dir's existence, because if it
doesn't exist you get $!=ENOENT.

> +foreach my $file (@patches) {

It would be nice to exclude ~ and .bak files here.  That way manually
editing files won't require trickery to exclude them.

> +if (index($file, $cover_letter) != -1) {

This is quite an un-perlish way to do things.  Also it goes wrong if
the patch *directory* is called -cover-letter.patch (which would
be mad, but it would be better not to make things worse), or if there
is a -cover-letter.patch~ but no -cover-letter.patch.

I suggest something like
   if ($file =~ 

Re: [Xen-devel] [PATCH v3 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)

[Julien Grall]

Hi Mirela,

On 27/04/18 18:12, Mirela Simonovic wrote:

During the system suspend to RAM non-boot CPUs will be hotplugged.
This will be triggered via disable_nonboot_cpus() call. When
hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
down the calling CPU during the suspend. The CPU_OFF call will be made
only if the PSCI version is higher than v0.1 (Note that the CPU_OFF
function is mandatory since PSCI v0.2).
If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
when the PSCI CPU_OFF call returns we'll raise panic, because the
calling CPU couldn't be enabled afterwards (stays in WFI loop forever).
Note that if the PSCI version is higher than v0.1 the CPU_OFF will be
called regardless of the system state. This is done because scenarios
other than suspend may benefit from powering off the CPU.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Issue PSCI CPU_OFF only if the system is suspending
-If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
-Fixed commit message

Changes in v3:
-Check for PSCI version prior to calling CPU_OFF
-Don't check for system state - invoke CPU_OFF in all system states
-Don't check if returned error is not zero because it's always not
  zero if CPU_OFF SMC returns
-Fixed commit message
---
  xen/arch/arm/psci.c| 13 +
  xen/arch/arm/smpboot.c |  2 ++
  xen/include/asm-arm/psci.h |  1 +
  3 files changed, 16 insertions(+)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 94b616df9b..7c1124e45f 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -46,6 +46,19 @@ int call_psci_cpu_on(int cpu)
  return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), 
__pa(init_secondary), 0);
  }
  
+void call_psci_cpu_off(void)

+{
+if ( psci_ver > PSCI_VERSION(0, 1) )
+{
+int errno;
+
+/* If successfull the PSCI cpu_off call doesn't return */
+errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
+panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),


Please use smp_processor_id() here.


+errno);


The indentation looks wrong.

With that:

Acked-by: Julien Grall 

Cheers,



+}
+}
+
  void call_psci_system_off(void)
  {
  if ( psci_ver > PSCI_VERSION(0, 1) )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b2116f0d2d..8b1e274bf3 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -395,6 +395,8 @@ void stop_cpu(void)
  /* Make sure the write happens before we sleep forever */
  dsb(sy);
  isb();
+call_psci_cpu_off();
+
  while ( 1 )
  wfi();
  }
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 9ac820e94a..832f77afff 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -20,6 +20,7 @@ extern uint32_t psci_ver;
  
  int psci_init(void);

  int call_psci_cpu_on(int cpu);
+void call_psci_cpu_off(void);
  void call_psci_system_off(void);
  void call_psci_system_reset(void);
  



--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)

[Julien Grall]

Hi,

On 27/04/18 18:12, Mirela Simonovic wrote:

Guests attempt to write into these registers on resume (for example Linux).
Without this patch a data abort exception will be raised to the guest.
This patch handles the write access by ignoring it, but only if the value
to be written is zero. This should be fine because reading these registers
is already handled as 'read as zero'.

Signed-off-by: Mirela Simonovic 


Reviewed-by: Julien Grall 

Cheers,



---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
- Write should be ignored only if the value to be written is zero
  (in v1 the write was ignored regardless of the value)

Changes in v3:
- Print warning only if the value to be written is not zero
---
  xen/arch/arm/vgic-v2.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 646d1f3d12..f6c11f1e41 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -485,6 +485,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
mmio_info_t *info,
  
  case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):

  if ( dabt.size != DABT_WORD ) goto bad_width;
+if ( r == 0 )
+goto write_ignore_32;
  printk(XENLOG_G_ERR
 "%pv: vGICD: unhandled word write %#"PRIregister" to 
ISACTIVER%d\n",
 v, r, gicd_reg - GICD_ISACTIVER);



--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: char: Remove unnecessary (uart->irq > 0) check

[Andre Przywara]

Hi,

On 28/04/18 10:08, Amit Singh Tomar wrote:
> While working on MVEBU uart driver, Julien pointed out that (uart->irq > 0)
> check is unnecessary during irq set up.if ever there is an invalid irq, driver
> initialization itself would be bailed out from platform_get_irq.
> 
> This patch would remove similar check for other uart drivers present in XEN.
> 
> Signed-off-by: Amit Singh Tomar 
> ---
> * This patch is only compiled tested.
> ---
>  xen/drivers/char/cadence-uart.c | 15 ---
>  xen/drivers/char/ns16550.c  | 35 ++-
>  xen/drivers/char/omap-uart.c|  2 +-
>  xen/drivers/char/pl011.c| 13 +++--
>  4 files changed, 30 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c
> index 22905ba..1575787 100644
> --- a/xen/drivers/char/cadence-uart.c
> +++ b/xen/drivers/char/cadence-uart.c
> @@ -72,13 +72,14 @@ static void __init cuart_init_postirq(struct serial_port 
> *port)
>  struct cuart *uart = port->uart;
>  int rc;
>  
> -if ( uart->irq > 0 )
> +uart->irqaction.handler = cuart_interrupt;
> +uart->irqaction.name= "cadence-uart";
> +uart->irqaction.dev_id  = port;
> +
> +if ( (rc = setup_irq(uart->irq, 0, >irqaction)) != 0 )
>  {
> -uart->irqaction.handler = cuart_interrupt;
> -uart->irqaction.name= "cadence-uart";
> -uart->irqaction.dev_id  = port;
> -if ( (rc = setup_irq(uart->irq, 0, >irqaction)) != 0 )
> -printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", 
> uart->irq);
> +printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", uart->irq);
> +return;

Careful, this changes the behaviour here:
Formerly a failure in setup_irq() led to just the warning, but then
execution (and initialisation) continued, even without an IRQ properly
set up. Depending on the UART and its driver this may or may not work.
But at least it deserves some mentioning in the commit message.

I quickly tested with an PL011: if setup_irq() does not succeed (hacked
it to use the VGIC IRQ, which is already taken), the UART ignores any
input, because it never actively polls or checks for incoming
characters. But output works perfectly fine, and the system works as
excepted (I can login via ssh, but not on the console).
So we might want to upgrade the error message to state the fatality of
this failure, but proceed anyway (as we do right now).

I haven't (and mostly can't) test other UARTs, but I expect the
behaviour to be the same (even with 16550), at least on ARM, as nothing
polls the UART periodically.

Cheers,
Andre.

>  }
>  
>  /* Clear pending error interrupts */
> @@ -130,7 +131,7 @@ static int __init cuart_irq(struct serial_port *port)
>  {
>  struct cuart *uart = port->uart;
>  
> -return ( (uart->irq > 0) ? uart->irq : -1 );
> +return uart->irq;
>  }
>  
>  static const struct vuart_info *cuart_vuart(struct serial_port *port)
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index f32dbd3..ba50a1e 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -714,18 +714,12 @@ static void __init ns16550_init_preirq(struct 
> serial_port *port)
>  
>  static void ns16550_setup_postirq(struct ns16550 *uart)
>  {
> -if ( uart->irq > 0 )
> -{
> -/* Master interrupt enable; also keep DTR/RTS asserted. */
> -ns_write_reg(uart,
> - UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
> -
> -/* Enable receive interrupts. */
> -ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
> -}
> +/* Master interrupt enable; also keep DTR/RTS asserted. */
> +ns_write_reg(uart, UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | 
> UART_MCR_RTS);
> +/* Enable receive interrupts. */
> +ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
>  
> -if ( uart->irq >= 0 )
> -set_timer(>timer, NOW() + MILLISECS(uart->timeout_ms));
> +set_timer(>timer, NOW() + MILLISECS(uart->timeout_ms));
>  }
>  
>  static void __init ns16550_init_postirq(struct serial_port *port)
> @@ -733,9 +727,6 @@ static void __init ns16550_init_postirq(struct 
> serial_port *port)
>  struct ns16550 *uart = port->uart;
>  int rc, bits;
>  
> -if ( uart->irq < 0 )
> -return;
> -
>  serial_async_transmit(port);
>  
>  init_timer(>timer, ns16550_poll, port, 0);
> @@ -746,13 +737,14 @@ static void __init ns16550_init_postirq(struct 
> serial_port *port)
>  uart->timeout_ms = max_t(
>  unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>  
> -if ( uart->irq > 0 )
> +uart->irqaction.handler = ns16550_interrupt;
> +uart->irqaction.name= "ns16550";
> +uart->irqaction.dev_id  = port;
> +
> +if ( (rc = setup_irq(uart->irq, 0, >irqaction)) != 0 )
>  {
> -uart->irqaction.handler = ns16550_interrupt;
> -   

Re: [Xen-devel] [PATCH] xen: char: Remove unnecessary (uart->irq > 0) check

[Julien Grall]

Hi,

On 30/04/18 09:19, Jan Beulich wrote:

On 28.04.18 at 11:08,  wrote:

While working on MVEBU uart driver, Julien pointed out that (uart->irq > 0)
check is unnecessary during irq set up.if ever there is an invalid irq, driver
initialization itself would be bailed out from platform_get_irq.

This patch would remove similar check for other uart drivers present in XEN.


At the example of the changes to ns16550.c you do, this is not correct. I
can't judge about the various ARM specific drivers, but the 16550 can well
be run in polling mode, and hence failure to set up an interrupt is not fatal
to overall driver initialization.


This makes sense for any ARM only UART driver because they don't support 
polling. However, I agree that if the driver is supporting polling (such 
as NS16550) then you should keep the irq check around.


Cheers,




Signed-off-by: Amit Singh Tomar 
---
 * This patch is only compiled tested.


In which case this should be marked RFC imo.

Jan




--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] x86/cpu: Add supports for zhaoxin x86 platform

[Jan Beulich]

>>> On 25.04.18 at 11:51,  wrote:
> --- a/xen/arch/x86/cpu/intel_cacheinfo.c
> +++ b/xen/arch/x86/cpu/intel_cacheinfo.c
> @@ -103,7 +103,7 @@ int cpuid4_cache_lookup(int index, struct cpuid4_info 
> *this_leaf)
>   return 0;
>  }
>  
> -static int find_num_cache_leaves(void)
> +int find_num_cache_leaves(void)

Instead of making this function non-static, please consider re-using
init_intel_cacheinfo(): All you want is skip the CPUID leaf 2 handling,
and you'd better to this by altering the single if() controlling it in that
function than by effectively introducing a clone. If you're concerned
of some other dead code in that function, attached you'll find a patch
deleting at least some of that.

> --- /dev/null
> +++ b/xen/arch/x86/cpu/shanghai.c
> @@ -0,0 +1,90 @@
> +#include 
> +#include 
> +#include 
> +#include "cpu.h"
> +
> +void init_shanghai_cache(struct cpuinfo_x86 *c)
> +{
> + unsigned int i = 0, l1d = 0, l1i = 0, l2 = 0, l3 = 0;
> +struct cpuid4_info leaf;
> + static bool is_initialized = false;
> + static unsigned int cache_leaves = 0;
> +
> + if ( (!is_initialized) && (c->cpuid_level > 0x0003) )
> +{

If there was a convincing argument that this clone of the original
function was really needed, then you'd need to go through here
and clean up style (various aspects of it are broken, most notably
the mix of space and tab indentation).

> --- a/xen/include/asm-x86/iommu.h
> +++ b/xen/include/asm-x86/iommu.h
> @@ -54,6 +54,7 @@ static inline const struct iommu_ops *iommu_get_ops(void)
>  switch ( boot_cpu_data.x86_vendor )
>  {
>  case X86_VENDOR_INTEL:
> +case X86_VENDOR_SHANGHAI:
>  return _iommu_ops;
>  case X86_VENDOR_AMD:
>  return _iommu_ops;
> @@ -69,6 +70,7 @@ static inline int iommu_hardware_setup(void)
>  switch ( boot_cpu_data.x86_vendor )
>  {
>  case X86_VENDOR_INTEL:
> +case X86_VENDOR_SHANGHAI:
>  return intel_vtd_setup();
>  case X86_VENDOR_AMD:
>  return amd_iov_detect();

There are numerous further occurrences of X86_VENDOR_INTEL throughout
the code base - is it really the case that no single one of them needs similar
amendment?

Jan


x86: remove read code from cpuid4_cache_lookup()

... and make num_cache_leaves local to the only function using it.

Signed-off-by: Jan Beulich 

--- unstable.orig/xen/arch/x86/cpu/intel_cacheinfo.c2017-03-03 
14:08:33.0 +0100
+++ unstable/xen/arch/x86/cpu/intel_cacheinfo.c 2018-04-30 15:59:54.637217413 
+0200
@@ -80,8 +80,6 @@ static const struct _cache_table cache_t
{ 0x00, 0, 0}
 };
 
-unsigned short num_cache_leaves;
-
 int cpuid4_cache_lookup(int index, struct cpuid4_info *this_leaf)
 {
union _cpuid4_leaf_eax  eax;
@@ -123,7 +121,7 @@ unsigned int init_intel_cacheinfo(struct
unsigned int trace = 0, l1i = 0, l1d = 0, l2 = 0, l3 = 0; /* Cache 
sizes */
unsigned int new_l1d = 0, new_l1i = 0; /* Cache sizes from cpuid(4) */
unsigned int new_l2 = 0, new_l3 = 0, i; /* Cache sizes from cpuid(4) */
-   unsigned int l2_id = 0, l3_id = 0, num_threads_sharing, index_msb;
+   static unsigned int num_cache_leaves;
 
if (c->cpuid_level > 3) {
static int is_initialized;
@@ -156,15 +154,9 @@ unsigned int init_intel_cacheinfo(struct
break;
case 2:
new_l2 = this_leaf.size/1024;
-   num_threads_sharing = 1 + 
this_leaf.eax.split.num_threads_sharing;
-   index_msb = 
get_count_order(num_threads_sharing);
-   l2_id = c->apicid >> index_msb;
break;
case 3:
new_l3 = this_leaf.size/1024;
-   num_threads_sharing = 1 + 
this_leaf.eax.split.num_threads_sharing;
-   index_msb = 
get_count_order(num_threads_sharing);
-   l3_id = c->apicid >> index_msb;
break;
default:
break;
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org

[Lars Kurth]

On 30/04/2018, 14:38, "Ian Jackson"  wrote:

Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org 
with xenproject.org"):
> This is a general clean-up activity. It also avoids mails being
> sent to xen-devel@lists.xenproject.org and xen-de...@lists.xen.org
> when used with add_maintainers.pl/git send-email

Acked-by: Ian Jackson 

It would be nice to replace many of the http:// urls with https.  But
that shouldn't block this patch.
   
Sure, I can do that. I don't mind re-sending it with those changes.

Lars


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Lars Kurth]

On 30/04/2018, 14:39, "George Dunlap"  wrote:

On 04/30/2018 02:23 PM, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer 
(R:) to MAINTAINERS (plus a test case)"):
>> On 30.04.18 at 10:21,  wrote:
>>> On 30/04/2018, 08:57, "Jan Beulich"  wrote:
> ...
>>> That is my fault: I got into trouble with git and must have done 
something 
>>> wrong. If it helps, I can switch the order and re-send. 
>>
>> I don't think that's necessary - whoever ends up committing them can 
easily
>> enough switch them around.
> 
> I will do so when all is clear.
> 
>> I would commit them right away, if only I was
>> really clear whether we've all settled on this.
> 
> I have been following this.  I think this is a good idea.
> 
> Basically it is a way for someone to declare an interest in an area of
> code, and get copied on changes, without having to grant that person
> any formal decisionmaking authority.
> 
> If this is not sufficiently clear, do you think we should document
> this more clearly ?  Perhaps we could write:
> 
>   +   R: Designated reviewer: FullName 
>   +Reviewers should be CCed on patches.  However, they do not
>   +have a formal governance role, and are listed here
>   +simply because of their own request.
> 
> or something ?

+1 to this description; but I took Jan to mean that it wasn't clear
whether we as a community had decided having such a framework was a good
one (although it sounded like he was personally in favor).

Lazy consensus says that if people don't object, it's assumed that
they're OK with it.  The first version of this series was posted 25
April; if people are concerned about giving people a chance to object,
we could wait until 2 May to check it in.  That would give people a week
in which to object if they want.

I wouldn't object to someone checking it in now, however; I think all
the committers have had a chance to object, and most have expressed support.

Agreed. And it was discussed at the x86 community call, which is why I put the 
patch together

Lars
 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[George Dunlap]

On 04/30/2018 02:23 PM, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) 
> to MAINTAINERS (plus a test case)"):
>> On 30.04.18 at 10:21,  wrote:
>>> On 30/04/2018, 08:57, "Jan Beulich"  wrote:
> ...
>>> That is my fault: I got into trouble with git and must have done something 
>>> wrong. If it helps, I can switch the order and re-send. 
>>
>> I don't think that's necessary - whoever ends up committing them can easily
>> enough switch them around.
> 
> I will do so when all is clear.
> 
>> I would commit them right away, if only I was
>> really clear whether we've all settled on this.
> 
> I have been following this.  I think this is a good idea.
> 
> Basically it is a way for someone to declare an interest in an area of
> code, and get copied on changes, without having to grant that person
> any formal decisionmaking authority.
> 
> If this is not sufficiently clear, do you think we should document
> this more clearly ?  Perhaps we could write:
> 
>   +   R: Designated reviewer: FullName 
>   +Reviewers should be CCed on patches.  However, they do not
>   +have a formal governance role, and are listed here
>   +simply because of their own request.
> 
> or something ?

+1 to this description; but I took Jan to mean that it wasn't clear
whether we as a community had decided having such a framework was a good
one (although it sounded like he was personally in favor).

Lazy consensus says that if people don't object, it's assumed that
they're OK with it.  The first version of this series was posted 25
April; if people are concerned about giving people a chance to object,
we could wait until 2 May to check it in.  That would give people a week
in which to object if they want.

I wouldn't object to someone checking it in now, however; I think all
the committers have had a chance to object, and most have expressed support.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Jan Beulich]

>>> On 30.04.18 at 15:29,  wrote:

> 
> On 30/04/2018, 14:23, "Ian Jackson"  wrote:
> 
> Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer 
> (R:) to MAINTAINERS (plus a test case)"):
> > On 30.04.18 at 10:21,  wrote:
> > > On 30/04/2018, 08:57, "Jan Beulich"  wrote:
> ...
> > > That is my fault: I got into trouble with git and must have done 
> something 
> > > wrong. If it helps, I can switch the order and re-send. 
> > 
> > I don't think that's necessary - whoever ends up committing them can 
> easily
> > enough switch them around.
> 
> I will do so when all is clear.
> 
> > I would commit them right away, if only I was
> > really clear whether we've all settled on this.
> 
> I have been following this.  I think this is a good idea.
> 
> Basically it is a way for someone to declare an interest in an area of
> code, and get copied on changes, without having to grant that person
> any formal decisionmaking authority.
> 
> If this is not sufficiently clear, do you think we should document
> this more clearly ?  Perhaps we could write:
> 
>   +   R: Designated reviewer: FullName 
>   +Reviewers should be CCed on patches.  However, they do not
>   +have a formal governance role, and are listed here
>   +simply because of their own request.
> 
> or something ?
> 
> That is clearer: I copied the text from the Linux maintainers file. 

Ah, indeed. So far it wasn't really clear to me whether "designated" implies
further privilege.

> @Jan: let me know whether you want me to re-roll the series with the text 
> change.

Since I take Ian's reply as him volunteering to commit both, that would then
really be up to him.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[George Dunlap]

On 04/30/2018 02:23 PM, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) 
> to MAINTAINERS (plus a test case)"):
>> On 30.04.18 at 10:21,  wrote:
>>> On 30/04/2018, 08:57, "Jan Beulich"  wrote:
> ...
>>> That is my fault: I got into trouble with git and must have done something 
>>> wrong. If it helps, I can switch the order and re-send. 
>>
>> I don't think that's necessary - whoever ends up committing them can easily
>> enough switch them around.
> 
> I will do so when all is clear.
> 
>> I would commit them right away, if only I was
>> really clear whether we've all settled on this.
> 
> I have been following this.  I think this is a good idea.
> 
> Basically it is a way for someone to declare an interest in an area of
> code, and get copied on changes, without having to grant that person
> any formal decisionmaking authority.
> 
> If this is not sufficiently clear, do you think we should document
> this more clearly ?  Perhaps we could write:
> 
>   +   R: Designated reviewer: FullName 
>   +Reviewers should be CCed on patches.  However, they do not
>   +have a formal governance role, and are listed here
>   +simply because of their own request.
> 
> or something ?

+1 to this description; but I took Jan to mean that it wasn't clear
whether we as a community had decided having such a framework was a good
one (although it sounded like he was personally in favor).

Lazy consensus says that if people don't object, it's assumed that
they're OK with it.  The first version of this series was posted 25
April; if people are concerned about giving people a chance to object,
we could wait until 2 May to check it in.  That would give people a week
in which to object if they want.

I wouldn't object to someone checking it in now, however; I think all
the committers have had a chance to object, and most have expressed support.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org

[Ian Jackson]

Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org with 
xenproject.org"):
> This is a general clean-up activity. It also avoids mails being
> sent to xen-devel@lists.xenproject.org and xen-de...@lists.xen.org
> when used with add_maintainers.pl/git send-email

Acked-by: Ian Jackson 

It would be nice to replace many of the http:// urls with https.  But
that shouldn't block this patch.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Lars Kurth]

On 30/04/2018, 14:23, "Ian Jackson"  wrote:

Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer 
(R:) to MAINTAINERS (plus a test case)"):
> On 30.04.18 at 10:21,  wrote:
> > On 30/04/2018, 08:57, "Jan Beulich"  wrote:
...
> > That is my fault: I got into trouble with git and must have done 
something 
> > wrong. If it helps, I can switch the order and re-send. 
> 
> I don't think that's necessary - whoever ends up committing them can 
easily
> enough switch them around.

I will do so when all is clear.

> I would commit them right away, if only I was
> really clear whether we've all settled on this.

I have been following this.  I think this is a good idea.

Basically it is a way for someone to declare an interest in an area of
code, and get copied on changes, without having to grant that person
any formal decisionmaking authority.

If this is not sufficiently clear, do you think we should document
this more clearly ?  Perhaps we could write:

  + R: Designated reviewer: FullName 
  +Reviewers should be CCed on patches.  However, they do not
  +have a formal governance role, and are listed here
  +simply because of their own request.

or something ?

That is clearer: I copied the text from the Linux maintainers file. 
@Jan: let me know whether you want me to re-roll the series with the text 
change.

Lars


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] 4.11.0 RC1 panic

[Jan Beulich]

>>> On 25.04.18 at 16:42,  wrote:
> On Wed, Apr 25, 2018 at 12:42:42PM +0200, Manuel Bouyer wrote:
>> > Without line numbers associated with at least the top stack trace entry
>> > I can only guess what it might be - could you give the patch below a try?
>> > (This may not be the final patch, as I'm afraid there may be some race
>> > here, but I'd have to work this out later.)
>> 
>> Yes, this works. thanks !
>> I'll now put this version on the NetBSD testbed I'm running.
>> This should put some pressure on it.
> 
> Running NetBSD tests in several guests I got:
> (XEN) 
> (XEN) 
> (XEN) Panic on CPU 1:
> (XEN) Assertion 'oc > 0' failed at mm.c:628
> (XEN) 
> (see attached file for complete report).

So in combination with your later reply I'm confused: Are you observing
this with 64-bit guests as well (your later reply appears to hint towards
64-bit-ness), or (as the stack trace suggests) only 32-bit ones? Knowing
this may already narrow areas where to look.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools: prepend to PKG_CONFIG_PATH when configuring qemu

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH] tools: prepend to PKG_CONFIG_PATH when 
configuring qemu"):
> On 26/04/18 19:41, Stewart Hildebrand wrote:
> > A user may choose to set his/her own PKG_CONFIG_PATH, which is useful in the
> > case of cross-compiling.  We don't want to completely override the
> > PKG_CONFIG_PATH, just add to it.
> > 
> > Signed-off-by: Stewart Hildebrand 
> 
> Release-acked-by: Juergen Gross 

Thanks all.  Applied and queued for backport.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) 
to MAINTAINERS (plus a test case)"):
> On 30.04.18 at 10:21,  wrote:
> > On 30/04/2018, 08:57, "Jan Beulich"  wrote:
...
> > That is my fault: I got into trouble with git and must have done something 
> > wrong. If it helps, I can switch the order and re-send. 
> 
> I don't think that's necessary - whoever ends up committing them can easily
> enough switch them around.

I will do so when all is clear.

> I would commit them right away, if only I was
> really clear whether we've all settled on this.

I have been following this.  I think this is a good idea.

Basically it is a way for someone to declare an interest in an area of
code, and get copied on changes, without having to grant that person
any formal decisionmaking authority.

If this is not sufficiently clear, do you think we should document
this more clearly ?  Perhaps we could write:

  + R: Designated reviewer: FullName 
  +Reviewers should be CCed on patches.  However, they do not
  +have a formal governance role, and are listed here
  +simply because of their own request.

or something ?

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] where can I find the 'address translation' code in Xen?

[Andrew Cooper]

On 29/04/18 11:11, Minjun Hong wrote:
> Hi.
> I'm looking for a point where address translation (guest virtual
> address to machine address) occurs in Xen.
> Of course, I mean when TLB miss has occured.

This question makes me wonder whether you are more familiar with PowerPC
than x86.  In x86, the TLB is automatically maintained by hardware, and
new entries will be populated as necessary.  All pagetable related
errors result in a pagefault exception, which Xen handles.

> I'm using a PV guest and I've found 'guest_walk_tables()' function in
> "xen/arch/x86/mm/guest_walk.c".
> However, in the comment of the function, it says "Walk the guest
> pagetables, after the manner of a hardware walker".
> I'm confused because I'm not sure if the function is called after the
> 'hardware page table walker'.

"after the manner" is an uncommon phrase in English, which AFAICT
derives from Latin originally.  It means "in the style of", "in the same
way as", etc.

guest_walk_tables() is a function which tries to match the behaviour of
the hardware pagewalker.  However, it is only used for cases where we
can't use regular hardware support, such as emulation of instructions.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] Xen Security Advisory 259 (CVE-2018-10471) - x86: PV guest may crash Xen with XPTI

[Xen . org security team]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Xen Security Advisory CVE-2018-10471 / XSA-259
  version 3

 x86: PV guest may crash Xen with XPTI

UPDATES IN VERSION 3


CVE assigned.

ISSUE DESCRIPTION
=

The workaround for the Meltdown vulnerability (XSA-254) failed to deal
with an error code path connecting the INT 80 handling with general
exception handling.  This results in an unconditional write attempt of
the value zero to an address near 2^64, in cases where a PV guest has no
handler installed for INT 80 on one of its vCPU-s.

IMPACT
==

A malicious or buggy guest may cause a hypervisor crash, resulting in
a Denial of Service (DoS) affecting the entire host.

VULNERABLE SYSTEMS
==

All Xen versions which the XSA-254 fixes were applied to are vulnerable.

Only x86 systems are vulnerable.  ARM systems are not vulnerable.

Only x86 PV guests can exploit the vulnerability.  x86 PVH and HVM
guests cannot exploit the vulnerability.

MITIGATION
==

Running only PVH or HVM guests avoids the vulnerability.

CREDITS
===

This issue was discovered by Andrew Cooper of Citrix.

RESOLUTION
==

Applying the appropriate attached patch resolves this issue.

xsa259.patch  xen-unstable, Xen 4.10.x ... xen 4.7.x
xsa259-4.6.patch  Xen 4.6.x

$ sha256sum xsa259*
5c14a90af066c952974324b361e2a428c280f876b854f0c85a78e8579054a4d1  xsa259.meta
ff2efb5eb2502ded988d0aa15351030a15494a9e2223eafbb88377a8e4d39dcb  xsa259.patch
c40bc8802077cf73f8393fb50574b7c7efbc4d127e202b0ebd757d34aa07aac3  
xsa259-4.6.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-
Version: GnuPG v1

iQEcBAEBCAAGBQJa5xa0AAoJEIP+FMlX6CvZDGEIAL5KbzcBUVjNsguU0HQ2Q6k8
WejwrXdKkncObK3yoxuybDE4NS+A5o0FbhdpJ86ukemZd2pMutgz79Z14UhSiURk
Owdj7BlzD64O42OftKqXiNKVp4QhOlOh02TU08Q4m6GKAtCi+HlBcK8EQFR8URhX
E2zLtpqGv5z6qx26raTDWQAssak4qL/NPSQ7oc3Eqo7P7H8B3Jw+F7DoR9a1g2ye
gwuINHuk0ea9+jLoinNTDDn17xDAwp8KHPGrI/ivlwGyFipBISICdReDHe/EfIWS
BNvrZl4ccDe95B1SosN8d0/qGYPLfpSN910hmm0ZTit0XffDseLv/odxoLuDvuQ=
=clOX
-END PGP SIGNATURE-


xsa259.meta
Description: Binary data


xsa259.patch
Description: Binary data


xsa259-4.6.patch
Description: Binary data
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] Xen Security Advisory 258 (CVE-2018-10472) - Information leak via crafted user-supplied CDROM

[Xen . org security team]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Xen Security Advisory CVE-2018-10472 / XSA-258
  version 3

   Information leak via crafted user-supplied CDROM

UPDATES IN VERSION 3


CVE assigned.

ISSUE DESCRIPTION
=

QEMU handles many different file formats for virtual disks (e.g., raw,
qcow2, vhd, ).  Some of these formats are "snapshots" that specify
"patches" to an alternate disk image, whose filename is included in
the snapshot file.

When qemu is given a disk but the type is not specified, it attempts
to guess the file format by reading it.  If a disk image is intended
to be 'raw', but the image is entirely controlled by an attacker, the
attacker could write a header to the image, describing one of these
"snapshot" formats, and pointing to an arbitrary file as the "backing"
file.

When attaching disks via command-line parameters at boot time
(including both "normal" disks and CDROMs), libxl specifies the
format; however, when inserting a CDROM live via QMP, the format was
not specified.

IMPACT
==

An attacker supplying a crafted CDROM image can read any file (or
device node) on the dom0 filesystem with the permissions of the qemu
devicemodel process.  (The virtual CDROM device is read-only, so
no data can be written.)

VULNERABLE SYSTEMS
==

Only x86 HVM guests with a virtual CDROM device are affected.  ARM
guests, x86 PV guests, x86 PVH guests, and x86 HVM guests without a
virtual CDROM device are not affected.

Only systems with qemu running in dom0 are affected; systems running
stub domains are not affected.  Only systems using qemu-xen (aka
"qemu-upstream" are affected; systems running qemu-xen-traditional
are not affected.

Only systems in which an attacker can provide a raw CDROM image, and
cause that image to be virtually inserted while the guest is running,
are affected.  Systems which only have host administrator-supplied
CDROM images, or systems which allow images to be added only at boot
time, are not affected.

MITIGATION
==

One workaround is to "wrap" the guest-supplied image in a specific
format; i.e., accept a raw image from the untrusted user, and convert
it into qcow2 format; for example:

qemu-img convert -f raw -O qcow2 untrusted.raw wrapped.qcow2

WARNING: Make sure to specify `-f raw` if you do this, or qemu will
"guess" the format of "untrusted.raw" (which the attacker may have
crafted to look like a qcow2 snapshot image with an alternativee base).

Another workaround is to allow guests to only change CDROMs at boot
time, not while the guest is running.

CREDITS
===

This issue was discovered by Anthony Perard of Citrix.

RESOLUTION
==

Applying the appropriate attached patch resolves this issue.

xsa258.patch   xen-unstable, Xen 4.10.x, Xen 4.9.x
xsa258-4.8.patch   Xen 4.8.x, Xen 4.7.x
xsa258-4.6.patch   Xen 4.6.x

$ sha256sum xsa258*
2c35a77eeca5579b5c32517c5ba511c836fa70f8b824ca8883fc6e1a7e608405  xsa258.meta
7e8014deae4fa19464fe6570d0719f8f0d7730dd153d58b2fa38b0cd5ed2e459  xsa258.patch
2c58060a42dafbf65563941dd8c737732124b49eb47007cc60f647553227f557  
xsa258-4.6.patch
ebba2f1f084249cd1e1c2f59e338412161884c31c83dbba03fc1e10bf4ba57a1  
xsa258-4.8.patch
$

DEPLOYMENT DURING EMBARGO
=

Deployment of the patches and/or the "wrap" mitigation described above
(or others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.

However, deploying the "only allow guests to change CDROMs at boot
time" is NOT permitted (except where all the affected systems and VMs
are administered and used only by organisations which are members of
the Xen Project Security Issues Predisclosure List).  Specifically,
deployment on public cloud systems is NOT permitted.  This is because
it may give attackers a hint of where to look for the vulnerability.
Deployment of this mitigation is permitted only AFTER the embargo
ends.

Additionally, 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-
Version: GnuPG v1

iQEcBAEBCAAGBQJa5xaxAAoJEIP+FMlX6CvZYdgIAMiidM7VGBh2l+DUooYZjKm/
BQEzqlM7EMqq8IiK7lNSXrZIXdLiR8S4oNhRZlqv3m2zxjDmdpS1N2F/6Xt37qOv

[Xen-devel] [PATCH 1/4] block/xen_disk: remove persistent grant code

[Paul Durrant]

The grant copy operation was added to libxengnttab in Xen 4.8.0. If grant
copy is available then persistent grants will not be used.
The xen_disk source can be siginificantly simplified by removing this now
redundant code.

Signed-off-by: Paul Durrant 
---
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Kevin Wolf 
Cc: Max Reitz 
---
 hw/block/xen_disk.c | 237 +---
 1 file changed, 21 insertions(+), 216 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index f74fcd4..b33611a 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -43,20 +43,6 @@ static int batch_maps   = 0;
 #define BLOCK_SIZE  512
 #define IOCB_COUNT  (BLKIF_MAX_SEGMENTS_PER_REQUEST + 2)
 
-struct PersistentGrant {
-void *page;
-struct XenBlkDev *blkdev;
-};
-
-typedef struct PersistentGrant PersistentGrant;
-
-struct PersistentRegion {
-void *addr;
-int num;
-};
-
-typedef struct PersistentRegion PersistentRegion;
-
 struct ioreq {
 blkif_request_t req;
 int16_t status;
@@ -73,7 +59,6 @@ struct ioreq {
 int prot;
 void*page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 void*pages;
-int num_unmap;
 
 /* aio status */
 int aio_inflight;
@@ -115,13 +100,7 @@ struct XenBlkDev {
 int requests_finished;
 unsigned intmax_requests;
 
-/* Persistent grants extension */
 gbooleanfeature_discard;
-gbooleanfeature_persistent;
-GTree   *persistent_gnts;
-GSList  *persistent_regions;
-unsigned intpersistent_gnt_count;
-unsigned intmax_grants;
 
 /* qemu block driver */
 DriveInfo   *dinfo;
@@ -158,46 +137,6 @@ static void ioreq_reset(struct ioreq *ioreq)
 qemu_iovec_reset(>v);
 }
 
-static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
-{
-uint ua = GPOINTER_TO_UINT(a);
-uint ub = GPOINTER_TO_UINT(b);
-return (ua > ub) - (ua < ub);
-}
-
-static void destroy_grant(gpointer pgnt)
-{
-PersistentGrant *grant = pgnt;
-xengnttab_handle *gnt = grant->blkdev->xendev.gnttabdev;
-
-if (xengnttab_unmap(gnt, grant->page, 1) != 0) {
-xen_pv_printf(>blkdev->xendev, 0,
-  "xengnttab_unmap failed: %s\n",
-  strerror(errno));
-}
-grant->blkdev->persistent_gnt_count--;
-xen_pv_printf(>blkdev->xendev, 3,
-  "unmapped grant %p\n", grant->page);
-g_free(grant);
-}
-
-static void remove_persistent_region(gpointer data, gpointer dev)
-{
-PersistentRegion *region = data;
-struct XenBlkDev *blkdev = dev;
-xengnttab_handle *gnt = blkdev->xendev.gnttabdev;
-
-if (xengnttab_unmap(gnt, region->addr, region->num) != 0) {
-xen_pv_printf(>xendev, 0,
-  "xengnttab_unmap region %p failed: %s\n",
-  region->addr, strerror(errno));
-}
-xen_pv_printf(>xendev, 3,
-  "unmapped grant region %p with %d pages\n",
-  region->addr, region->num);
-g_free(region);
-}
-
 static struct ioreq *ioreq_start(struct XenBlkDev *blkdev)
 {
 struct ioreq *ioreq = NULL;
@@ -327,22 +266,22 @@ static void ioreq_unmap(struct ioreq *ioreq)
 xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
 int i;
 
-if (ioreq->num_unmap == 0 || ioreq->mapped == 0) {
+if (ioreq->v.niov == 0 || ioreq->mapped == 0) {
 return;
 }
 if (batch_maps) {
 if (!ioreq->pages) {
 return;
 }
-if (xengnttab_unmap(gnt, ioreq->pages, ioreq->num_unmap) != 0) {
+if (xengnttab_unmap(gnt, ioreq->pages, ioreq->v.niov) != 0) {
 xen_pv_printf(>blkdev->xendev, 0,
   "xengnttab_unmap failed: %s\n",
   strerror(errno));
 }
-ioreq->blkdev->cnt_map -= ioreq->num_unmap;
+ioreq->blkdev->cnt_map -= ioreq->v.niov;
 ioreq->pages = NULL;
 } else {
-for (i = 0; i < ioreq->num_unmap; i++) {
+for (i = 0; i < ioreq->v.niov; i++) {
 if (!ioreq->page[i]) {
 continue;
 }
@@ -361,138 +300,44 @@ static void ioreq_unmap(struct ioreq *ioreq)
 static int ioreq_map(struct ioreq *ioreq)
 {
 xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
-uint32_t domids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-uint32_t refs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-void *page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-int i, j, new_maps = 0;
-PersistentGrant *grant;
-PersistentRegion *region;
-/* domids and refs variables will contain the information necessary
- * to map the grants that are needed to fulfill this request.
- *
- * After mapping 

[Xen-devel] [PATCH 4/4] block/xen_disk: be consistent with use of xendev and blkdev->xendev

[Paul Durrant]

Certain functions in xen_disk are called with a pointer to xendev
(struct XenDevice *). They then use continer_of() to acces the surrounding
blkdev (struct XenBlkDev) but then in various places use >xendev
when use of the original xendev pointer is shorter to express and clearly
equivalent.

This patch is a purely cosmetic patch which makes sure there is a xendev
pointer on stack for any function where the pointer is need on multiple
occasions modified those functions to use it consistently.

Signed-off-by: Paul Durrant 
---
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Kevin Wolf 
Cc: Max Reitz 
---
 hw/block/xen_disk.c | 116 +++-
 1 file changed, 60 insertions(+), 56 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 6d737fd..b538d21 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -178,10 +178,11 @@ static void ioreq_release(struct ioreq *ioreq, bool 
finish)
 static int ioreq_parse(struct ioreq *ioreq)
 {
 struct XenBlkDev *blkdev = ioreq->blkdev;
+struct XenDevice *xendev = >xendev;
 size_t len;
 int i;
 
-xen_pv_printf(>xendev, 3,
+xen_pv_printf(xendev, 3,
   "op %d, nr %d, handle %d, id %" PRId64 ", sector %" PRId64 
"\n",
   ioreq->req.operation, ioreq->req.nr_segments,
   ioreq->req.handle, ioreq->req.id, ioreq->req.sector_number);
@@ -199,28 +200,28 @@ static int ioreq_parse(struct ioreq *ioreq)
 case BLKIF_OP_DISCARD:
 return 0;
 default:
-xen_pv_printf(>xendev, 0, "error: unknown operation (%d)\n",
+xen_pv_printf(xendev, 0, "error: unknown operation (%d)\n",
   ioreq->req.operation);
 goto err;
 };
 
 if (ioreq->req.operation != BLKIF_OP_READ && blkdev->mode[0] != 'w') {
-xen_pv_printf(>xendev, 0, "error: write req for ro device\n");
+xen_pv_printf(xendev, 0, "error: write req for ro device\n");
 goto err;
 }
 
 ioreq->start = ioreq->req.sector_number * blkdev->file_blk;
 for (i = 0; i < ioreq->req.nr_segments; i++) {
 if (i == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
-xen_pv_printf(>xendev, 0, "error: nr_segments too big\n");
+xen_pv_printf(xendev, 0, "error: nr_segments too big\n");
 goto err;
 }
 if (ioreq->req.seg[i].first_sect > ioreq->req.seg[i].last_sect) {
-xen_pv_printf(>xendev, 0, "error: first > last sector\n");
+xen_pv_printf(xendev, 0, "error: first > last sector\n");
 goto err;
 }
 if (ioreq->req.seg[i].last_sect * BLOCK_SIZE >= XC_PAGE_SIZE) {
-xen_pv_printf(>xendev, 0, "error: page crossing\n");
+xen_pv_printf(xendev, 0, "error: page crossing\n");
 goto err;
 }
 
@@ -228,7 +229,7 @@ static int ioreq_parse(struct ioreq *ioreq)
 ioreq->size += len;
 }
 if (ioreq->start + ioreq->size > blkdev->file_size) {
-xen_pv_printf(>xendev, 0, "error: access beyond end of 
file\n");
+xen_pv_printf(xendev, 0, "error: access beyond end of file\n");
 goto err;
 }
 return 0;
@@ -241,7 +242,8 @@ err:
 static int ioreq_grant_copy(struct ioreq *ioreq)
 {
 struct XenBlkDev *blkdev = ioreq->blkdev;
-xengnttab_handle *gnt = blkdev->xendev.gnttabdev;
+struct XenDevice *xendev = >xendev;
+xengnttab_handle *gnt = xendev->gnttabdev;
 void *virt = ioreq->buf;
 xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 int i, rc;
@@ -251,13 +253,13 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
 if (ioreq->req.operation == BLKIF_OP_READ) {
 segs[i].flags = GNTCOPY_dest_gref;
 segs[i].dest.foreign.ref = ioreq->req.seg[i].gref;
-segs[i].dest.foreign.domid = blkdev->xendev.dom;
+segs[i].dest.foreign.domid = xendev->dom;
 segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * 
file_blk;
 segs[i].source.virt = virt;
 } else {
 segs[i].flags = GNTCOPY_source_gref;
 segs[i].source.foreign.ref = ioreq->req.seg[i].gref;
-segs[i].source.foreign.domid = blkdev->xendev.dom;
+segs[i].source.foreign.domid = xendev->dom;
 segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * 
file_blk;
 segs[i].dest.virt = virt;
 }
@@ -269,7 +271,7 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
 rc = xengnttab_grant_copy(gnt, ioreq->req.nr_segments, segs);
 
 if (rc) {
-xen_pv_printf(>xendev, 0,
+xen_pv_printf(xendev, 0,
   "failed to copy data %d\n", rc);
 ioreq->aio_errors++;
 return -1;
@@ -277,10 +279,10 @@ static int ioreq_grant_copy(struct ioreq *ioreq)
 
 for (i = 0; i < 

[Xen-devel] [PATCH 3/4] block/xen_disk: use a single entry iovec

[Paul Durrant]

Since xen_disk now always copies data to and from a guest there is no need
to maintain a vector entry corresponding to every page of a request.
This means there is less per-request state to maintain so the ioreq
structure can shrink significantly.

Signed-off-by: Paul Durrant 
---
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Kevin Wolf 
Cc: Max Reitz 
---
 hw/block/xen_disk.c | 103 
 1 file changed, 31 insertions(+), 72 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 8f4e229..6d737fd 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -46,13 +46,10 @@ struct ioreq {
 /* parsed request */
 off_t   start;
 QEMUIOVectorv;
+void*buf;
+size_t  size;
 int presync;
 
-uint32_tdomids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-uint32_trefs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-void*page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-void*pages;
-
 /* aio status */
 int aio_inflight;
 int aio_errors;
@@ -110,13 +107,10 @@ static void ioreq_reset(struct ioreq *ioreq)
 memset(>req, 0, sizeof(ioreq->req));
 ioreq->status = 0;
 ioreq->start = 0;
+ioreq->buf = NULL;
+ioreq->size = 0;
 ioreq->presync = 0;
 
-memset(ioreq->domids, 0, sizeof(ioreq->domids));
-memset(ioreq->refs, 0, sizeof(ioreq->refs));
-memset(ioreq->page, 0, sizeof(ioreq->page));
-ioreq->pages = NULL;
-
 ioreq->aio_inflight = 0;
 ioreq->aio_errors = 0;
 
@@ -139,7 +133,7 @@ static struct ioreq *ioreq_start(struct XenBlkDev *blkdev)
 ioreq = g_malloc0(sizeof(*ioreq));
 ioreq->blkdev = blkdev;
 blkdev->requests_total++;
-qemu_iovec_init(>v, BLKIF_MAX_SEGMENTS_PER_REQUEST);
+qemu_iovec_init(>v, 1);
 } else {
 /* get one from freelist */
 ioreq = QLIST_FIRST(>freelist);
@@ -184,7 +178,6 @@ static void ioreq_release(struct ioreq *ioreq, bool finish)
 static int ioreq_parse(struct ioreq *ioreq)
 {
 struct XenBlkDev *blkdev = ioreq->blkdev;
-uintptr_t mem;
 size_t len;
 int i;
 
@@ -231,14 +224,10 @@ static int ioreq_parse(struct ioreq *ioreq)
 goto err;
 }
 
-ioreq->domids[i] = blkdev->xendev.dom;
-ioreq->refs[i]   = ioreq->req.seg[i].gref;
-
-mem = ioreq->req.seg[i].first_sect * blkdev->file_blk;
 len = (ioreq->req.seg[i].last_sect - ioreq->req.seg[i].first_sect + 1) 
* blkdev->file_blk;
-qemu_iovec_add(>v, (void*)mem, len);
+ioreq->size += len;
 }
-if (ioreq->start + ioreq->v.size > blkdev->file_size) {
+if (ioreq->start + ioreq->size > blkdev->file_size) {
 xen_pv_printf(>xendev, 0, "error: access beyond end of 
file\n");
 goto err;
 }
@@ -249,85 +238,55 @@ err:
 return -1;
 }
 
-static void ioreq_free_copy_buffers(struct ioreq *ioreq)
-{
-int i;
-
-for (i = 0; i < ioreq->v.niov; i++) {
-ioreq->page[i] = NULL;
-}
-
-qemu_vfree(ioreq->pages);
-}
-
-static int ioreq_init_copy_buffers(struct ioreq *ioreq)
-{
-int i;
-
-if (ioreq->v.niov == 0) {
-return 0;
-}
-
-ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
-
-for (i = 0; i < ioreq->v.niov; i++) {
-ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
-ioreq->v.iov[i].iov_base = ioreq->page[i];
-}
-
-return 0;
-}
-
 static int ioreq_grant_copy(struct ioreq *ioreq)
 {
-xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
+struct XenBlkDev *blkdev = ioreq->blkdev;
+xengnttab_handle *gnt = blkdev->xendev.gnttabdev;
+void *virt = ioreq->buf;
 xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-int i, count, rc;
-int64_t file_blk = ioreq->blkdev->file_blk;
-
-if (ioreq->v.niov == 0) {
-return 0;
-}
+int i, rc;
+int64_t file_blk = blkdev->file_blk;
 
-count = ioreq->v.niov;
-
-for (i = 0; i < count; i++) {
+for (i = 0; i < ioreq->req.nr_segments; i++) {
 if (ioreq->req.operation == BLKIF_OP_READ) {
 segs[i].flags = GNTCOPY_dest_gref;
-segs[i].dest.foreign.ref = ioreq->refs[i];
-segs[i].dest.foreign.domid = ioreq->domids[i];
+segs[i].dest.foreign.ref = ioreq->req.seg[i].gref;
+segs[i].dest.foreign.domid = blkdev->xendev.dom;
 segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * 
file_blk;
-segs[i].source.virt = ioreq->v.iov[i].iov_base;
+segs[i].source.virt = virt;
 } else {
 segs[i].flags = GNTCOPY_source_gref;
-segs[i].source.foreign.ref = ioreq->refs[i];
-

[Xen-devel] [PATCH 2/4] block/xen_disk: remove use of grant map/unmap

[Paul Durrant]

The grant copy operation was added to libxengnttab in Xen 4.8.0. If grant
copy is available then data from the guest will be copied rather than
mapped.
The xen_disk source can be significantly simplified by removing this now
redundant code.

Signed-off-by: Paul Durrant 
---
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Kevin Wolf 
Cc: Max Reitz 
---
 hw/block/xen_disk.c | 194 
 1 file changed, 27 insertions(+), 167 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index b33611a..8f4e229 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -36,10 +36,6 @@
 
 /* - */
 
-static int batch_maps   = 0;
-
-/* - */
-
 #define BLOCK_SIZE  512
 #define IOCB_COUNT  (BLKIF_MAX_SEGMENTS_PER_REQUEST + 2)
 
@@ -51,12 +47,9 @@ struct ioreq {
 off_t   start;
 QEMUIOVectorv;
 int presync;
-uint8_t mapped;
 
-/* grant mapping */
 uint32_tdomids[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 uint32_trefs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
-int prot;
 void*page[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 void*pages;
 
@@ -89,7 +82,6 @@ struct XenBlkDev {
 int protocol;
 blkif_back_rings_t  rings;
 int more_work;
-int cnt_map;
 
 /* request lists */
 QLIST_HEAD(inflight_head, ioreq) inflight;
@@ -119,11 +111,9 @@ static void ioreq_reset(struct ioreq *ioreq)
 ioreq->status = 0;
 ioreq->start = 0;
 ioreq->presync = 0;
-ioreq->mapped = 0;
 
 memset(ioreq->domids, 0, sizeof(ioreq->domids));
 memset(ioreq->refs, 0, sizeof(ioreq->refs));
-ioreq->prot = 0;
 memset(ioreq->page, 0, sizeof(ioreq->page));
 ioreq->pages = NULL;
 
@@ -204,7 +194,6 @@ static int ioreq_parse(struct ioreq *ioreq)
   ioreq->req.handle, ioreq->req.id, ioreq->req.sector_number);
 switch (ioreq->req.operation) {
 case BLKIF_OP_READ:
-ioreq->prot = PROT_WRITE; /* to memory */
 break;
 case BLKIF_OP_FLUSH_DISKCACHE:
 ioreq->presync = 1;
@@ -213,7 +202,6 @@ static int ioreq_parse(struct ioreq *ioreq)
 }
 /* fall through */
 case BLKIF_OP_WRITE:
-ioreq->prot = PROT_READ; /* from memory */
 break;
 case BLKIF_OP_DISCARD:
 return 0;
@@ -261,88 +249,6 @@ err:
 return -1;
 }
 
-static void ioreq_unmap(struct ioreq *ioreq)
-{
-xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
-int i;
-
-if (ioreq->v.niov == 0 || ioreq->mapped == 0) {
-return;
-}
-if (batch_maps) {
-if (!ioreq->pages) {
-return;
-}
-if (xengnttab_unmap(gnt, ioreq->pages, ioreq->v.niov) != 0) {
-xen_pv_printf(>blkdev->xendev, 0,
-  "xengnttab_unmap failed: %s\n",
-  strerror(errno));
-}
-ioreq->blkdev->cnt_map -= ioreq->v.niov;
-ioreq->pages = NULL;
-} else {
-for (i = 0; i < ioreq->v.niov; i++) {
-if (!ioreq->page[i]) {
-continue;
-}
-if (xengnttab_unmap(gnt, ioreq->page[i], 1) != 0) {
-xen_pv_printf(>blkdev->xendev, 0,
-  "xengnttab_unmap failed: %s\n",
-  strerror(errno));
-}
-ioreq->blkdev->cnt_map--;
-ioreq->page[i] = NULL;
-}
-}
-ioreq->mapped = 0;
-}
-
-static int ioreq_map(struct ioreq *ioreq)
-{
-xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
-int i;
-
-if (ioreq->v.niov == 0 || ioreq->mapped == 1) {
-return 0;
-}
-if (batch_maps) {
-ioreq->pages = xengnttab_map_grant_refs
-(gnt, ioreq->v.niov, ioreq->domids, ioreq->refs, ioreq->prot);
-if (ioreq->pages == NULL) {
-xen_pv_printf(>blkdev->xendev, 0,
-  "can't map %d grant refs (%s, %d maps)\n",
-  ioreq->v.niov, strerror(errno),
-  ioreq->blkdev->cnt_map);
-return -1;
-}
-for (i = 0; i < ioreq->v.niov; i++) {
-ioreq->v.iov[i].iov_base = ioreq->pages + i * XC_PAGE_SIZE +
-(uintptr_t)ioreq->v.iov[i].iov_base;
-}
-ioreq->blkdev->cnt_map += ioreq->v.niov;
-} else  {
-for (i = 0; i < ioreq->v.niov; i++) {
-ioreq->page[i] = xengnttab_map_grant_ref
-(gnt, ioreq->domids[i], ioreq->refs[i], ioreq->prot);
-if (ioreq->page[i] == NULL) {
-xen_pv_printf(>blkdev->xendev, 0,
-   

Re: [Xen-devel] [PATCH v2 2/2] SVM: introduce a VM entry helper

[Andrew Cooper]

On 30/04/18 12:37, Jan Beulich wrote:
> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -61,24 +61,14 @@ UNLIKELY_START(ne, nsvm_hap)
>  jmp  .Lsvm_do_resume
>  __UNLIKELY_END(nsvm_hap)
>  
> -call svm_asid_handle_vmrun
> +mov  %rsp, %rdi
> +call svm_vmenter_helper
>  
>  cmpb $0,tb_init_done(%rip)
>  UNLIKELY_START(nz, svm_trace)
>  call svm_trace_vmentry
>  UNLIKELY_END(svm_trace)

This trace call can also be moved up into C, at which point you can fold
svm_trace_vmentry() (which is a 1-liner anyway) into
svm_vmenter_helper() which would be its sole caller.

Otherwise, Reivewed-by: Andrew Cooper 

>  
> -mov  VCPU_svm_vmcb(%rbx),%rcx
> -mov  UREGS_rax(%rsp),%rax
> -mov  %rax,VMCB_rax(%rcx)
> -mov  UREGS_rip(%rsp),%rax
> -mov  %rax,VMCB_rip(%rcx)
> -mov  UREGS_rsp(%rsp),%rax
> -mov  %rax,VMCB_rsp(%rcx)
> -mov  UREGS_eflags(%rsp),%rax
> -or   $X86_EFLAGS_MBS,%rax
> -mov  %rax,VMCB_rflags(%rcx)
> -
>  mov VCPU_arch_msr(%rbx), %rax
>  mov VCPUMSR_spec_ctrl_raw(%rax), %eax
>  
>

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/2] SVM: re-work VMCB sync-ing

[Jan Beulich]

While the main problem to be addressed here is the issue of what so far
was named "vmcb_in_sync" starting out with the wrong value (should have
been true instead of false, to prevent performing a VMSAVE without ever
having VMLOADed the vCPU's state), go a step further and make the
sync-ed state a tristate: CPU and memory may be in sync or an update
may be required in either direction. Rename the field and introduce an
enum. Callers of svm_sync_vmcb() now indicate the intended new state
(with a slight "anomaly" when requesting VMLOAD: we could store
vmcb_needs_vmsave in those cases as the callers request, but the VMCB
really is in sync at that point, and hence there's no need to VMSAVE in
case we don't make it out to guest context), and all syncing goes
through that function.

With that, there's no need to VMLOAD the state perhaps multiple times;
all that's needed is loading it once before VM entry.

Signed-off-by: Jan Beulich 
---
v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum
vmcb_sync_state.
---
I've been considering to put the VMLOAD invocation in
svm_asid_handle_vmrun() (instead of the two copies in svm_do_resume()
and svm_vmexit_handler()), but that seemed a little too abusive of the
function. See patch 2.
I'm also not really certain about svm_vmexit_do_vmload(): All I'm doing
here is a 1:1 change from previous behavior, but I'm unconvinced this
was/is really correct.

--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -112,7 +112,6 @@ UNLIKELY_END(svm_trace)
 /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
 mov  VCPU_svm_vmcb(%rbx),%rcx
-movb $0,VCPU_svm_vmcb_in_sync(%rbx)
 mov  VMCB_rax(%rcx),%rax
 mov  %rax,UREGS_rax(%rsp)
 mov  VMCB_rip(%rcx),%rax
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -680,16 +680,24 @@ static void svm_cpuid_policy_changed(str
   cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
 }
 
-static void svm_sync_vmcb(struct vcpu *v)
+static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
 {
 struct arch_svm_struct *arch_svm = >arch.hvm_svm;
 
-if ( arch_svm->vmcb_in_sync )
-return;
-
-arch_svm->vmcb_in_sync = 1;
+if ( new_state == vmcb_needs_vmsave )
+{
+ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload);
+svm_vmload(arch_svm->vmcb);
+arch_svm->vmcb_sync_state = vmcb_in_sync;
+}
+else
+{
+if ( arch_svm->vmcb_sync_state == vmcb_needs_vmsave )
+svm_vmsave(arch_svm->vmcb);
 
-svm_vmsave(arch_svm->vmcb);
+if ( arch_svm->vmcb_sync_state != vmcb_needs_vmload )
+arch_svm->vmcb_sync_state = new_state;
+}
 }
 
 static unsigned int svm_get_cpl(struct vcpu *v)
@@ -707,7 +715,7 @@ static void svm_get_segment_register(str
 switch ( seg )
 {
 case x86_seg_fs ... x86_seg_gs:
-svm_sync_vmcb(v);
+svm_sync_vmcb(v, vmcb_in_sync);
 
 /* Fallthrough. */
 case x86_seg_es ... x86_seg_ds:
@@ -718,7 +726,7 @@ static void svm_get_segment_register(str
 break;
 
 case x86_seg_tr:
-svm_sync_vmcb(v);
+svm_sync_vmcb(v, vmcb_in_sync);
 *reg = vmcb->tr;
 break;
 
@@ -731,7 +739,7 @@ static void svm_get_segment_register(str
 break;
 
 case x86_seg_ldtr:
-svm_sync_vmcb(v);
+svm_sync_vmcb(v, vmcb_in_sync);
 *reg = vmcb->ldtr;
 break;
 
@@ -746,7 +754,6 @@ static void svm_set_segment_register(str
  struct segment_register *reg)
 {
 struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-bool sync = false;
 
 ASSERT((v == current) || !vcpu_runnable(v));
 
@@ -768,7 +775,8 @@ static void svm_set_segment_register(str
 case x86_seg_gs:
 case x86_seg_tr:
 case x86_seg_ldtr:
-sync = (v == current);
+if ( v == current )
+svm_sync_vmcb(v, vmcb_needs_vmload);
 break;
 
 default:
@@ -777,9 +785,6 @@ static void svm_set_segment_register(str
 return;
 }
 
-if ( sync )
-svm_sync_vmcb(v);
-
 switch ( seg )
 {
 case x86_seg_ss:
@@ -813,9 +818,6 @@ static void svm_set_segment_register(str
 ASSERT_UNREACHABLE();
 break;
 }
-
-if ( sync )
-svm_vmload(vmcb);
 }
 
 static unsigned long svm_get_shadow_gs_base(struct vcpu *v)
@@ -1086,7 +1088,7 @@ static void svm_ctxt_switch_from(struct
 svm_lwp_save(v);
 svm_tsc_ratio_save(v);
 
-svm_sync_vmcb(v);
+svm_sync_vmcb(v, vmcb_needs_vmload);
 svm_vmload_pa(per_cpu(host_vmcb, cpu));
 
 /* Resume use of ISTs now that the host TR is reinstated. */
@@ -1114,7 +1116,6 @@ static void svm_ctxt_switch_to(struct vc
 svm_restore_dr(v);
 
 svm_vmsave_pa(per_cpu(host_vmcb, cpu));
-svm_vmload(vmcb);
 vmcb->cleanbits.bytes = 0;
 svm_lwp_load(v);
 

[Xen-devel] [PATCH v2 2/2] SVM: introduce a VM entry helper

[Jan Beulich]

The register values copying doesn't need doing in assembly. The VMLOAD
invocation can also be further deferred (and centralized). Therefore
replace the svm_asid_handle_vmrun() invocation wiht one of the new
helper.

Similarly move the VM exit side register value copying into
svm_vmexit_handler().

Signed-off-by: Jan Beulich 
---
v2: New.
---
TBD: Now that we always make it out to guest context after VMLOAD, perhaps
 svm_sync_vmcb() should no longer override vmcb_needs_vmsave, and
 svm_vmexit_handler() would then no longer need to to set the field at all.

--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -61,24 +61,14 @@ UNLIKELY_START(ne, nsvm_hap)
 jmp  .Lsvm_do_resume
 __UNLIKELY_END(nsvm_hap)
 
-call svm_asid_handle_vmrun
+mov  %rsp, %rdi
+call svm_vmenter_helper
 
 cmpb $0,tb_init_done(%rip)
 UNLIKELY_START(nz, svm_trace)
 call svm_trace_vmentry
 UNLIKELY_END(svm_trace)
 
-mov  VCPU_svm_vmcb(%rbx),%rcx
-mov  UREGS_rax(%rsp),%rax
-mov  %rax,VMCB_rax(%rcx)
-mov  UREGS_rip(%rsp),%rax
-mov  %rax,VMCB_rip(%rcx)
-mov  UREGS_rsp(%rsp),%rax
-mov  %rax,VMCB_rsp(%rcx)
-mov  UREGS_eflags(%rsp),%rax
-or   $X86_EFLAGS_MBS,%rax
-mov  %rax,VMCB_rflags(%rcx)
-
 mov VCPU_arch_msr(%rbx), %rax
 mov VCPUMSR_spec_ctrl_raw(%rax), %eax
 
@@ -111,16 +101,6 @@ UNLIKELY_END(svm_trace)
 SPEC_CTRL_ENTRY_FROM_VMEXIT /* Req: b=curr %rsp=regs/cpuinfo, Clob: 
acd */
 /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
-mov  VCPU_svm_vmcb(%rbx),%rcx
-mov  VMCB_rax(%rcx),%rax
-mov  %rax,UREGS_rax(%rsp)
-mov  VMCB_rip(%rcx),%rax
-mov  %rax,UREGS_rip(%rsp)
-mov  VMCB_rsp(%rcx),%rax
-mov  %rax,UREGS_rsp(%rsp)
-mov  VMCB_rflags(%rcx),%rax
-mov  %rax,UREGS_eflags(%rsp)
-
 STGI
 GLOBAL(svm_stgi_label)
 mov  %rsp,%rdi
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1169,12 +1169,25 @@ static void noreturn svm_do_resume(struc
 
 hvm_do_resume(v);
 
-if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
-svm_sync_vmcb(v, vmcb_needs_vmsave);
-
 reset_stack_and_jump(svm_asm_do_resume);
 }
 
+void svm_vmenter_helper(const struct cpu_user_regs *regs)
+{
+struct vcpu *curr = current;
+struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
+
+svm_asid_handle_vmrun();
+
+if ( curr->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
+svm_sync_vmcb(curr, vmcb_needs_vmsave);
+
+vmcb->rax = regs->rax;
+vmcb->rip = regs->rip;
+vmcb->rsp = regs->rsp;
+vmcb->rflags = regs->rflags | X86_EFLAGS_MBS;
+}
+
 static void svm_guest_osvw_init(struct vcpu *vcpu)
 {
 if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD )
@@ -2621,6 +2634,12 @@ void svm_vmexit_handler(struct cpu_user_
 struct vlapic *vlapic = vcpu_vlapic(v);
 
 v->arch.hvm_svm.vmcb_sync_state = vmcb_needs_vmsave;
+
+regs->rax = vmcb->rax;
+regs->rip = vmcb->rip;
+regs->rsp = vmcb->rsp;
+regs->rflags = vmcb->rflags;
+
 hvm_invalidate_regs_fields(regs);
 
 if ( paging_mode_hap(v->domain) )
@@ -3107,9 +3126,6 @@ void svm_vmexit_handler(struct cpu_user_
 }
 
   out:
-if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
-svm_sync_vmcb(v, vmcb_needs_vmsave);
-
 if ( vcpu_guestmode || vlapic_hw_disabled(vlapic) )
 return;
 
@@ -3118,7 +3134,6 @@ void svm_vmexit_handler(struct cpu_user_
 intr.fields.tpr =
 (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4;
 vmcb_set_vintr(vmcb, intr);
-ASSERT(v->arch.hvm_svm.vmcb_sync_state != vmcb_needs_vmload);
 }
 
 void svm_trace_vmentry(void)
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -119,12 +119,6 @@ void __dummy__(void)
 OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.is_32bit_pv);
 BLANK();
 
-OFFSET(VMCB_rax, struct vmcb_struct, rax);
-OFFSET(VMCB_rip, struct vmcb_struct, rip);
-OFFSET(VMCB_rsp, struct vmcb_struct, rsp);
-OFFSET(VMCB_rflags, struct vmcb_struct, rflags);
-BLANK();
-
 OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
 OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
 BLANK();
--- a/xen/include/asm-x86/hvm/svm/asid.h
+++ b/xen/include/asm-x86/hvm/svm/asid.h
@@ -23,6 +23,7 @@
 #include 
 
 void svm_asid_init(const struct cpuinfo_x86 *c);
+void svm_asid_handle_vmrun(void);
 
 static inline void svm_asid_g_invlpg(struct vcpu *v, unsigned long g_vaddr)
 {



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [xen-4.10-testing baseline-only test] 74649: tolerable FAIL

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 74649 xen-4.10-testing real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/74649/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-xsm   6 xen-install fail baseline untested
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass
 test-armhf-armhf-libvirt 12 guest-start  fail   never pass
 test-armhf-armhf-libvirt-xsm 12 guest-start  fail   never pass
 test-armhf-armhf-xl-credit2  12 guest-start  fail   never pass
 test-armhf-armhf-xl-midway   12 guest-start  fail   never pass
 test-armhf-armhf-xl  12 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 12 guest-start  fail   never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass
 test-amd64-i386-xl-qemut-ws16-amd64 10 windows-install fail never pass
 test-armhf-armhf-xl-multivcpu 12 guest-start  fail  never pass
 test-amd64-amd64-xl-pvhv2-amd 12 guest-start  fail  never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass
 test-amd64-amd64-xl-qemut-win10-i386 17 guest-stop fail never pass
 test-amd64-i386-xl-qemut-win10-i386 17 guest-stop  fail never pass
 test-armhf-armhf-xl-vhd  10 debian-di-installfail   never pass
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass

version targeted for testing:
 xen  c30ab3d97c8ff0d2ed8948dd013737befc7a2223
baseline version:
 xen  8d37ee1d101248ba9cf44d79352ade3b376db55c

Last test of basis74609  2018-04-14 10:46:04 Z   16 days
Testing same since74649  2018-04-30 02:16:41 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  Doug Goldstein 
  George Dunlap 
  Ian Jackson 
  Jan Beulich 
  Juergen Gross 
  Lars Kurth 

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-prev pass
 build-i386-prev  pass
 build-amd64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 build-amd64-rumprun  pass
 build-i386-rumprun   pass
 test-xtf-amd64-amd64-1   pass
 test-xtf-amd64-amd64-2   pass
 test-xtf-amd64-amd64-3   pass
 

Re: [Xen-devel] [xen-unstable test] 122451: regressions - FAIL [and 1 more messages]

[Ian Jackson]

osstest service owner writes ("[xen-unstable test] 122451: regressions - FAIL"):
> flight 122451 xen-unstable real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/122451/
> 
> Regressions :-(
> 
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  test-armhf-armhf-libvirt-raw  6 xen-install  fail REGR. vs. 
> 122343

This passed in:

osstest service owner writes ("[xen-unstable test] 122493: regressions - FAIL"):
> flight 122493 xen-unstable real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/122493/

So I don't think it's a repeatable regression.  I have therefore force
pushed 0d16ece0c5adb960ee4e45f12183bcac8fe6d50a (tested in 122451).

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 0/2] SVM: guest state handling adjustments

[Jan Beulich]

Only patch 1 is clearly meant for 4.11. The second patch, however, eliminates
a (theoretical) window the first patch still leaves, so should at least be 
considered.

1: re-work VMCB sync-ing
2: introduce a VM entry helper

Signed-off-by: Jan Beulich 



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] where can I find the 'address translation' code in Xen?

[Petr Tesarik]

Hi Minjun,

On Mon, 30 Apr 2018 17:17:36 +0900
Minjun Hong  wrote:

> On Mon, Apr 30, 2018 at 3:44 PM, Petr Tesarik  wrote:
> 
> > Hi Minjun,
> >
> > On Sun, 29 Apr 2018 19:11:30 +0900
> > Minjun Hong  wrote:
> >  
> > >[...]
> > > My question is,
> > >
> > > 1. Is it sure that the function will be called even though the HW already
> > > translates the address and populates the TLB entry?  
> >
> > I think you miss the point. The hardware page tables for PV domains
> > already contain machine addresses. In other words, virtual addresses
> > get translated directly to machine addresses by the hardware page table
> > walker.
> >  
> > > 2. I'm just asking, is there any code in Xen that is related to the
> > > behavior of the 'hardware walker'?  
> >
> > Not sure what you mean. Maybe this question is no longer pertinent
> > given the explanation above?
> >
> > HTH,
> > Petr T
> >
> > ___
> > Xen-devel mailing list
> > Xen-devel@lists.xenproject.org
> > https://lists.xenproject.org/mailman/listinfo/xen-devel  
> 
> 
> 
> Thanks for your kind answer, Petr.
> 
> In your answer on my first question, I was wondering why the function (
> guest_walk_tables()) should be called even after the address translation
> and TLB filling has been completed by the hardware page table walker.
> As you mentioned, original goal (address translation) is achieved by the
> hardware. If so, is its role like bottom half of interrupt?

Ah, now I'm starting to get your point. So, you're interested in
what happens after guest page tables are updated.

> I want to know this function is always called after the translation
> by the hardware page walker.

It seems to me that there are quite a few ways to avoid a full page
table walk, but I'm not an expert on this topic, sorry.

Petr T


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Jan Beulich]

>>> On 30.04.18 at 10:21,  wrote:

> 
> On 30/04/2018, 08:57, "Jan Beulich"  wrote:
> 
> >>> On 27.04.18 at 20:01,  wrote:
> > This follows up from a conversation after the April x86 community call, 
> in 
> > which I had
> > the following action: Lars to propose fixing CC issue in 
> xen.git:MAINTAINERS 
> > copying 
> > the R section entries from Linux.git:MAINTAINERS (will need changes to 
> > get_maintainers.pl also)
> > 
> > On 27/4/18 Juergen gave a RAB via IRC
> > 
> > Cc: Lars Kurth 
> > Cc: Andrew Cooper 
> > Cc: George Dunlap 
> > Cc: Ian Jackson 
> > Cc: Jan Beulich 
> > Cc: Julien Grall 
> > Cc: Konrad Rzeszutek Wilk 
> > Cc: Stefano Stabellini 
> > Cc: Tim Deegan 
> > Cc: Wei Liu 
> > Cc: Brian Woods 
> > Cc: Juergen Gross 
> > 
> > Release-acked-by: Juergen Gross 
> > Acked-by: Wei Liu 
> > 
> > Lars Kurth (2):
> >   Add Brian Woods as Designated reviewer to AMD IOMMU and AMD SVM
> >   Add Designated Reviewer (R:) to MAINTAINERS file and add support for
> > it in get_maintainer.pl
> 
> The order of the patches has been unexpectedly swapped, and seems wrong to
> me now; granted this is only cosmetic if both go in at the same time.
> 
> That is my fault: I got into trouble with git and must have done something 
> wrong. If it helps, I can switch the order and re-send. 

I don't think that's necessary - whoever ends up committing them can easily
enough switch them around. I would commit them right away, if only I was
really clear whether we've all settled on this.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5] x86/mm: Suppresses vm_events caused by page-walks

[Razvan Cojocaru]

On 04/30/2018 11:11 AM, Jan Beulich wrote:
 On 28.04.18 at 08:13,  wrote:
>> On 04/28/2018 12:30 AM, Tamas K Lengyel wrote:
>>> On Mon, Apr 23, 2018 at 2:00 AM, Alexandru Isaila
>>>  wrote:
 This patch is adding a way to enable/disable inguest pagefault
 events. It introduces the xc_monitor_inguest_pagefault function
 and adds the inguest_pagefault_disabled in the monitor structure.
 This is needed by the introspection so it will only get gla
 faults and not get spammed with other faults.
 In p2m_mem_access_check() we emulate so no event will get sent.
>>>
>>> This looks good to me, but is the emulator able to handle all
>>> instructions that may trigger it here?
>>
>> That's a very good question. We think not, but we now have the 
>> UNIMPLEMENTED emulator event. The thought here is that the emulator 
>> would be able to handle most cases, and then the ones it can't handle we 
>> can handle with altp2m.
>>
>> Of course, it's not ideal - we'd rather have a mechanism that's 
>> consistently foolproof, but I believe that Jan's objection is correct: 
>> we can't really be sure that the first time we get into access_check() 
>> with a specific [RIP:GLA] pair we need to set the A bit and the second 
>> time the D bit (interrupts may trip this logic up).
> 
> Interrupts are only one aspect. Insns sent back to guest context for
> retry (like AVX2 gathers would commonly do) are another afaict.
> 
>> Furthermore, with 
>> SVM the GLA is not available for page faults (although that's fixable by 
>> comparing GPAs).
> 
> I may not have enough context here, but is that true when multiple
> linear addresses are mapped to the same physical page?

No, you are right. Quite possibly a case like that can happen where
comparing GPAs is not enough.

So as far as I can tell, we can either do this best-effort thing with
trying to emulate the instruction and hope for the best (and handle
UNIMPLEMENTED when necessary), or A) know exactly when we need to set
the A bit and when the D bit - I've not been able to find a foolproof
way of doing that -, or B) single-step GPT page faults directly on
hardware _in_the_hypervisor_, for which there is currently no mechanism
- although one can be seen as doable on top of the altp2m infrastructure
in the future.


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Lars Kurth]

On 30/04/2018, 08:57, "Jan Beulich"  wrote:

>>> On 27.04.18 at 20:01,  wrote:
> This follows up from a conversation after the April x86 community call, 
in 
> which I had
> the following action: Lars to propose fixing CC issue in 
xen.git:MAINTAINERS 
> copying 
> the R section entries from Linux.git:MAINTAINERS (will need changes to 
> get_maintainers.pl also)
> 
> On 27/4/18 Juergen gave a RAB via IRC
> 
> Cc: Lars Kurth 
> Cc: Andrew Cooper 
> Cc: George Dunlap 
> Cc: Ian Jackson 
> Cc: Jan Beulich 
> Cc: Julien Grall 
> Cc: Konrad Rzeszutek Wilk 
> Cc: Stefano Stabellini 
> Cc: Tim Deegan 
> Cc: Wei Liu 
> Cc: Brian Woods 
> Cc: Juergen Gross 
> 
> Release-acked-by: Juergen Gross 
> Acked-by: Wei Liu 
> 
> Lars Kurth (2):
>   Add Brian Woods as Designated reviewer to AMD IOMMU and AMD SVM
>   Add Designated Reviewer (R:) to MAINTAINERS file and add support for
> it in get_maintainer.pl

The order of the patches has been unexpectedly swapped, and seems wrong to
me now; granted this is only cosmetic if both go in at the same time.

That is my fault: I got into trouble with git and must have done something 
wrong. If it helps, I can switch the order and re-send. 

The first patch "Add Brian Woods..." depends on syntax and tools support 
introduced by "Add Designated Reviewer (R:)...". I would say that they probably 
should go in together.

Lars 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: char: Remove unnecessary (uart->irq > 0) check

[Jan Beulich]

>>> On 28.04.18 at 11:08,  wrote:
> While working on MVEBU uart driver, Julien pointed out that (uart->irq > 0)
> check is unnecessary during irq set up.if ever there is an invalid irq, driver
> initialization itself would be bailed out from platform_get_irq.
> 
> This patch would remove similar check for other uart drivers present in XEN.

At the example of the changes to ns16550.c you do, this is not correct. I
can't judge about the various ARM specific drivers, but the 16550 can well
be run in polling mode, and hence failure to set up an interrupt is not fatal
to overall driver initialization.

> Signed-off-by: Amit Singh Tomar 
> ---
> * This patch is only compiled tested.

In which case this should be marked RFC imo.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] where can I find the 'address translation' code in Xen?

[Minjun Hong]

On Mon, Apr 30, 2018 at 3:44 PM, Petr Tesarik  wrote:

> Hi Minjun,
>
> On Sun, 29 Apr 2018 19:11:30 +0900
> Minjun Hong  wrote:
>
> >[...]
> > My question is,
> >
> > 1. Is it sure that the function will be called even though the HW already
> > translates the address and populates the TLB entry?
>
> I think you miss the point. The hardware page tables for PV domains
> already contain machine addresses. In other words, virtual addresses
> get translated directly to machine addresses by the hardware page table
> walker.
>
> > 2. I'm just asking, is there any code in Xen that is related to the
> > behavior of the 'hardware walker'?
>
> Not sure what you mean. Maybe this question is no longer pertinent
> given the explanation above?
>
> HTH,
> Petr T
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel



Thanks for your kind answer, Petr.

In your answer on my first question, I was wondering why the function (
guest_walk_tables()) should be called even after the address translation
and TLB filling has been completed by the hardware page table walker.
As you mentioned, original goal (address translation) is achieved by the
hardware. If so, is its role like bottom half of interrupt?

I want to know this function is always called after the translation by the
hardware page walker.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5] x86/mm: Suppresses vm_events caused by page-walks

[Jan Beulich]

>>> On 28.04.18 at 08:13,  wrote:
> On 04/28/2018 12:30 AM, Tamas K Lengyel wrote:
>> On Mon, Apr 23, 2018 at 2:00 AM, Alexandru Isaila
>>  wrote:
>>> This patch is adding a way to enable/disable inguest pagefault
>>> events. It introduces the xc_monitor_inguest_pagefault function
>>> and adds the inguest_pagefault_disabled in the monitor structure.
>>> This is needed by the introspection so it will only get gla
>>> faults and not get spammed with other faults.
>>> In p2m_mem_access_check() we emulate so no event will get sent.
>> 
>> This looks good to me, but is the emulator able to handle all
>> instructions that may trigger it here?
> 
> That's a very good question. We think not, but we now have the 
> UNIMPLEMENTED emulator event. The thought here is that the emulator 
> would be able to handle most cases, and then the ones it can't handle we 
> can handle with altp2m.
> 
> Of course, it's not ideal - we'd rather have a mechanism that's 
> consistently foolproof, but I believe that Jan's objection is correct: 
> we can't really be sure that the first time we get into access_check() 
> with a specific [RIP:GLA] pair we need to set the A bit and the second 
> time the D bit (interrupts may trip this logic up).

Interrupts are only one aspect. Insns sent back to guest context for
retry (like AVX2 gathers would commonly do) are another afaict.

> Furthermore, with 
> SVM the GLA is not available for page faults (although that's fixable by 
> comparing GPAs).

I may not have enough context here, but is that true when multiple
linear addresses are mapped to the same physical page?

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.11 v2 0/2] Add Designated Reviewer (R:) to MAINTAINERS (plus a test case)

[Jan Beulich]

>>> On 27.04.18 at 20:01,  wrote:
> This follows up from a conversation after the April x86 community call, in 
> which I had
> the following action: Lars to propose fixing CC issue in xen.git:MAINTAINERS 
> copying 
> the R section entries from Linux.git:MAINTAINERS (will need changes to 
> get_maintainers.pl also)
> 
> On 27/4/18 Juergen gave a RAB via IRC
> 
> Cc: Lars Kurth 
> Cc: Andrew Cooper 
> Cc: George Dunlap 
> Cc: Ian Jackson 
> Cc: Jan Beulich 
> Cc: Julien Grall 
> Cc: Konrad Rzeszutek Wilk 
> Cc: Stefano Stabellini 
> Cc: Tim Deegan 
> Cc: Wei Liu 
> Cc: Brian Woods 
> Cc: Juergen Gross 
> 
> Release-acked-by: Juergen Gross 
> Acked-by: Wei Liu 
> 
> Lars Kurth (2):
>   Add Brian Woods as Designated reviewer to AMD IOMMU and AMD SVM
>   Add Designated Reviewer (R:) to MAINTAINERS file and add support for
> it in get_maintainer.pl

The order of the patches has been unexpectedly swapped, and seems wrong to
me now; granted this is only cosmetic if both go in at the same time.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] SVM: re-work VMCB sync-ing

[Jan Beulich]

>>> On 27.04.18 at 20:29,  wrote:
> On 04/27/2018 11:52 AM, Jan Beulich wrote:
> On 26.04.18 at 19:27,  wrote:
>>> On 04/26/2018 11:55 AM, Jan Beulich wrote:
>>> On 26.04.18 at 17:20,  wrote:
> On 04/26/2018 09:33 AM, Jan Beulich wrote:
 -static void svm_sync_vmcb(struct vcpu *v)
 +static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state 
 new_state)
  {
  struct arch_svm_struct *arch_svm = >arch.hvm_svm;
  
 -if ( arch_svm->vmcb_in_sync )
 -return;
 -
 -arch_svm->vmcb_in_sync = 1;
 +if ( arch_svm->vmcb_sync_state == vmcb_needs_vmsave )
 +svm_vmsave(arch_svm->vmcb);
  
 -svm_vmsave(arch_svm->vmcb);
 +if ( arch_svm->vmcb_sync_state != vmcb_needs_vmload )
 +arch_svm->vmcb_sync_state = new_state;
>>> This is slightly awkward for a couple of reasons.  First, passing
>>> vmcb_in_sync in forget the fact that a vmload is needed.
>> Certainly not - that's the purpose of the if() around it.
>>
>>> In my patch, I introduced svm_sync_vmcb_for_update(), rather than
>>> requiring a parameter to be passed in.  I think this is a better API,
>>> and it shrinks the size of the patch.
>> I'm not convinced of the "better", and even less so of the "shrinks". But
>> I'll wait to see what the SVM maintainers say.
> I think a single function is better. In fact, I was wondering whether
> svm_vmload() could also be folded into svm_sync_vmcb() since it is also
> a syncing operation.
 That doesn't look like it would produce a usable interface: How would
 you envision the state transition to be specified by the caller? Right
 now the intended new state gets passed in, but in your model
 vmcb_in_sync could mean either vmload or vmsave is needed. The
 two svm_vmload() uses right now would pass that value in addition
 to the svm_sync_vmcb() calls already doing so. And the function
 couldn't tell what to do from the current state (if it's
 vmcb_needs_vmload, a load is only needed in the cases where
 svm_vmload() is called right now). Adding a 3rd parameter or a
 second enum 
>>> I was thinking about another enum value, e.g. sync_to_cpu (and
>>> sync_to_vmcb replacing vmcb_needs_vmsave).
>>>
>>> This will allow us to hide (v->arch.hvm_svm.vmcb_sync_state ==
>>> vmcb_needs_vmload) test.
>> I'm still not entirely clear how you want that to look like. At the example
>> of svm_get_segment_register() and svm_set_segment_register(), how
>> would the svm_sync_vmcb() calls look like? I.e. how do you distinguish
>> the sync_to_vmcb/transition-to-clean case from the
>> sync_to_vmcb/transition-to-dirty one?
> 
> 
> Something like
> 
> static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state)
> {
>  struct arch_svm_struct *arch_svm = >arch.hvm_svm;
> 
>  
> if ( new_state == vmcb_sync_to_cpu )

But that's not a state, but a transition. An enum should consist of only
states or only transitions. I'll produce a variant of this as v2, but I'm
afraid you won't be overly happy with it.

Jan

> {
>   if (v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload )
> { 
> svm_vmload(vmcb);
> v->arch.hvm_svm.vmcb_sync_state = vmcb_in_sync;
> }
>   return;
> }
> 
> if ( arch_svm->vmcb_sync_state == vmcb_needs_vmsave )
> svm_vmsave(arch_svm->vmcb);
>  
> if ( arch_svm->vmcb_sync_state != vmcb_needs_vmload )
> arch_svm->vmcb_sync_state = new_state;
>  }
> 
> 
> -boris





___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH RESEND v1 2/7] x86: configure vmcs for Intel processor trace virtualization

[Jan Beulich]

>>> On 28.04.18 at 03:07,  wrote:
>> > @@ -383,13 +388,28 @@ static int vmx_init_vmcs_config(void)
>> >  _vmx_secondary_exec_control &=
>> > ~SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS;
>> >
>> >  min = 0;
>> > -opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS;
>> > +opt = VM_ENTRY_LOAD_GUEST_PAT | VM_ENTRY_LOAD_BNDCFGS |
>> > +  VM_ENTRY_CONCEAL_PT_PIP | VM_ENTRY_LOAD_IA32_RTIT_CTL;
>> >  _vmx_vmentry_control = adjust_vmx_controls(
>> >  "VMEntry Control", min, opt, MSR_IA32_VMX_ENTRY_CTLS,
>> > );
>> >
>> >  if ( mismatch )
>> >  return -EINVAL;
>> >
>> > +if ( !(_vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT)
>> ||
>> > + !(_vmx_secondary_exec_control & SECONDARY_EXEC_PT_USE_GPA)
>> ||
>> > + !(_vmx_vmexit_control & VM_EXIT_CLEAR_IA32_RTIT_CTL) ||
>> > + !(_vmx_vmentry_control & VM_ENTRY_LOAD_IA32_RTIT_CTL) )
>> > +{
>> > +_vmx_secondary_exec_control &=
>> ~(SECONDARY_EXEC_PT_USE_GPA |
>> > + SECONDARY_EXEC_CONCEAL_PT_PIP);
>> > +_vmx_vmexit_control &= ~(VM_EXIT_CONCEAL_PT_PIP |
>> > + VM_EXIT_CLEAR_IA32_RTIT_CTL);
>> > +_vmx_vmentry_control &= ~(VM_ENTRY_CONCEAL_PT_PIP |
>> > +  VM_ENTRY_LOAD_IA32_RTIT_CTL);
>> > +opt_intel_pt = 0;
>> > +}
>> 
>> Besides clearing the flag here, shouldn't you also check it further up?
> 
> If " opt_intel_pt =0" represent user don't want to use this feature to all 
> guest or hardware don't support it at all. If flag "opt_intel_pt " still true 
> after this check represent the user want to use this feature and hardware 
> have capability to support PT in guest.  This is depend on hardware 
> capability and the parameter set of xen command line "ipt=1".

I'm having some difficulty to follow this, so let me explain my point a little
further: If (part of) the required features is available in hardware, but
the user opted to not use IPT, wouldn't it be better for consistency to
turn off the individual IPT features (so that e.g. checks of them elsewhere
in the code won't go wrong), i.e. pretend the hardware doesn't support
them?

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel