[Xen-devel] [xen-4.5-testing test] 114423: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds  7 xen-boot fail REGR. vs. 114263

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-localmigrate/x10 fail like 114142
 test-xtf-amd64-amd64-2   59 leak-check/check fail  like 114263
 test-xtf-amd64-amd64-1   59 leak-check/check fail  like 114263
 test-xtf-amd64-amd64-5   59 leak-check/check fail  like 114263
 test-xtf-amd64-amd64-4   59 leak-check/check fail  like 114263
 test-amd64-amd64-xl-rtds  7 xen-boot fail  like 114263
 test-xtf-amd64-amd64-3   59 leak-check/check fail  like 114263
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 114263
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 114263
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 114263
 test-xtf-amd64-amd64-2   19 xtf/test-hvm32-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-2 33 xtf/test-hvm32pae-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-2 40 xtf/test-hvm32pse-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-2   44 xtf/test-hvm64-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-4   19 xtf/test-hvm32-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-5   19 xtf/test-hvm32-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-1   19 xtf/test-hvm32-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-4 33 xtf/test-hvm32pae-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-1 33 xtf/test-hvm32pae-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-4 40 xtf/test-hvm32pse-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-1 40 xtf/test-hvm32pse-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-4   44 xtf/test-hvm64-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-1   44 xtf/test-hvm64-cpuid-faulting fail  never pass
 test-xtf-amd64-amd64-5 33 xtf/test-hvm32pae-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-5 40 xtf/test-hvm32pse-cpuid-faulting fail never pass
 test-xtf-amd64-amd64-5   44 xtf/test-hvm64-cpuid-faulting fail  never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-xtf-amd64-amd64-2   58 xtf/test-hvm64-xsa-195   fail   never pass
 test-xtf-amd64-amd64-1   58 xtf/test-hvm64-xsa-195   fail   never pass
 test-xtf-amd64-amd64-5   58 xtf/test-hvm64-xsa-195   fail   never pass
 test-xtf-amd64-amd64-4   58 xtf/test-hvm64-xsa-195   fail   never pass
 test-amd64-i386-libvirt-qcow2 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-xtf-amd64-amd64-3   58 xtf/test-hvm64-xsa-195   fail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 13 guest-saverestore  fail never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-xl-vhd  11 guest-start  fail   never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 13 guest-saverestore  fail never pass
 test-amd64-i386-xl-qemut-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-libvirt-raw 11 guest-start  fail   never pass
 test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass

version targeted for testing:
 xen  03b06d38c785ec89817a608470b443d8de2e1b9e
baseline version:
 

[Xen-devel] [xen-4.6-testing test] 114422: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-xtf-amd64-amd64-4 48 xtf/test-hvm64-lbr-tsx-vmentry fail REGR. vs. 114097
 test-armhf-armhf-xl-credit2 16 guest-start/debian.repeat fail REGR. vs. 114097

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds   16 guest-start/debian.repeat fail blocked in 114097
 test-xtf-amd64-amd64-3  48 xtf/test-hvm64-lbr-tsx-vmentry fail like 114097
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 114097
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 114097
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 114097
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 114097
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 114097
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 114097
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 114097
 test-xtf-amd64-amd64-3   72 xtf/test-pv32pae-xsa-194 fail   never pass
 test-xtf-amd64-amd64-4   72 xtf/test-pv32pae-xsa-194 fail   never pass
 test-xtf-amd64-amd64-5   72 xtf/test-pv32pae-xsa-194 fail   never pass
 test-xtf-amd64-amd64-1   72 xtf/test-pv32pae-xsa-194 fail   never pass
 test-xtf-amd64-amd64-2   72 xtf/test-pv32pae-xsa-194 fail   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-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 10 windows-installfail never pass
 test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail  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-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
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemut-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-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-qemut-win10-i386 10 windows-installfail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  aad5a67587b493e2478e1e46f71404c3dd41a937
baseline version:
 xen  78fd0c3765cf89befb2338ac342a0c8a3e29ba3d

Last test of basis   114097  2017-10-07 12:28:11 Z6 days
Testing same since   114422  2017-10-12 14:11:19 Z1 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  George Dunlap 
  Jan Beulich 
  Tim Deegan 

Re: [Xen-devel] [Qemu-devel] [RFC QEMU PATCH v3 00/10] Implement vNVDIMM for Xen HVM guest

[Stefano Stabellini]

On Fri, 13 Oct 2017, Jan Beulich wrote:
> >>> On 13.10.17 at 13:13,  wrote:
> > To Jan, Andrew, Stefano and Anthony,
> > 
> > what do you think about allowing QEMU to build the entire guest ACPI
> > and letting SeaBIOS to load it? ACPI builder code in hvmloader is
> > still there and just bypassed in this case.
> 
> Well, if that can be made work in a non-quirky way and without
> loss of functionality, I'd probably be fine. I do think, however,
> that there's a reason this is being handled in hvmloader right now.

And not to discourage you, just as a clarification, you'll also need to
consider backward compatibility: unless the tables are identical, I
imagine we'll have to keep using the old tables for already installed
virtual machines.

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

[Xen-devel] [xen-4.7-testing test] 114420: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-xtf-amd64-amd64-2 48 xtf/test-hvm64-lbr-tsx-vmentry fail REGR. vs. 114072

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 114072

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 114072
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 114072
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 114072
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 114072
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 114072
 test-amd64-amd64-xl-qemut-ws16-amd64 10 windows-installfail 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
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  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-amd64-i386-xl-qemut-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-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
 test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass

version targeted for testing:
 xen  c10dc54d41695a074c90b8afed950bd63884de0b
baseline version:
 xen  d6aad635097d901b96df650e87f04687c9fb7bd2

Last test of basis   114072  2017-10-06 13:43:18 Z7 days
Testing same since   114420  2017-10-12 13:43:28 Z1 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  George Dunlap 
  Jan Beulich 
  Tim Deegan 
  Vitaly Kuznetsov 

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

Re: [Xen-devel] [PATCH] x86: convert x86_platform_ops to timespec64

[Boris Ostrovsky]

On 10/13/2017 02:37 PM, Arnd Bergmann wrote:
> The x86 platform operations are fairly isolated, so we can
> change them from using timespec to timespec64. I checked that
> All the users and callers are safe, and there is only one
> critical function that is broken beyond 2106:
>
> pvclock_read_wallclock() uses a 32-bit number of seconds since
> the epoch to communicate the boot time between host and guest
> in a virtual environment. This will work until 2106, but we
> should ideally find a replacement anyway. I've added a comment
> about it there.
>
> Signed-off-by: Arnd Bergmann 
> ---
>  arch/x86/include/asm/intel_mid_vrtc.h|  4 ++--
>  arch/x86/include/asm/mc146818rtc.h   |  4 ++--
>  arch/x86/include/asm/pvclock.h   |  2 +-
>  arch/x86/include/asm/x86_init.h  |  6 +++---
>  arch/x86/kernel/kvmclock.c   |  4 ++--
>  arch/x86/kernel/pvclock.c| 12 +---
>  arch/x86/kernel/rtc.c| 16 
>  arch/x86/platform/intel-mid/intel_mid_vrtc.c | 10 +-
>  arch/x86/xen/time.c  | 10 +-
>  9 files changed, 37 insertions(+), 31 deletions(-)

Xen bits:
Reviewed-by: Boris Ostrovsky 

with a couple of nits:

> @@ -136,11 +136,17 @@ void pvclock_read_wallclock(struct pvclock_wall_clock 
> *wall_clock,
>   rmb();  /* fetch time before checking version */
>   } while ((wall_clock->version & 1) || (version != wall_clock->version));
>  
> + /*
> +  * Note: wall_clock->sec is a u32 value, so it can only store dates
> +  * between 1970 and 2106. To allow times beyond that, we need to
> +  * create a new hypercall interface with an extended pvclock_wall_clock
> +  * structure like ARM has.
> +  */

I think this comment block should be moved up above 'now.tv_sec  =
wall_clock->sec;'


>   delta = pvclock_clocksource_read(vcpu_time);/* time since system 
> boot */
>   delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;

Now that tv_sec is a 64-bit quantity the cast can be dropped.

-boris

>  
>   now.tv_nsec = do_div(delta, NSEC_PER_SEC);
>   now.tv_sec = delta;
>  
> - set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
> + set_normalized_timespec64(ts, now.tv_sec, now.tv_nsec);
>  }
>


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

[Xen-devel] Please Welcome Stefano Stabellini as new Security Team Member

[Lars Kurth]

Dear Community members,

I am pleased to announce that Stefano Stabellini has been nominated and
voted to become a new member of the Xen Project security team. The vote
was unanimous with all security team members voting in favour of
Stefano's membership.

Stefano has made significant contributions to the Xen Project over the 
years (see https://xen.biterg.io:443/goto/73478f514bcbb3eafc68354837a6b70e),
has been a maintainer and project leadership team member for a long time,
and is also a member of the QEMU security team.

Best Regards 
Lars Kurth 
Xen Project Community Manager 
Chairman of Xen Project Advisory Board

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

[Xen-devel] [PATCH] x86: convert x86_platform_ops to timespec64

[Arnd Bergmann]

The x86 platform operations are fairly isolated, so we can
change them from using timespec to timespec64. I checked that
All the users and callers are safe, and there is only one
critical function that is broken beyond 2106:

pvclock_read_wallclock() uses a 32-bit number of seconds since
the epoch to communicate the boot time between host and guest
in a virtual environment. This will work until 2106, but we
should ideally find a replacement anyway. I've added a comment
about it there.

Signed-off-by: Arnd Bergmann 
---
 arch/x86/include/asm/intel_mid_vrtc.h|  4 ++--
 arch/x86/include/asm/mc146818rtc.h   |  4 ++--
 arch/x86/include/asm/pvclock.h   |  2 +-
 arch/x86/include/asm/x86_init.h  |  6 +++---
 arch/x86/kernel/kvmclock.c   |  4 ++--
 arch/x86/kernel/pvclock.c| 12 +---
 arch/x86/kernel/rtc.c| 16 
 arch/x86/platform/intel-mid/intel_mid_vrtc.c | 10 +-
 arch/x86/xen/time.c  | 10 +-
 9 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/intel_mid_vrtc.h 
b/arch/x86/include/asm/intel_mid_vrtc.h
index 86ff4685c409..19202320f0be 100644
--- a/arch/x86/include/asm/intel_mid_vrtc.h
+++ b/arch/x86/include/asm/intel_mid_vrtc.h
@@ -3,7 +3,7 @@
 
 extern unsigned char vrtc_cmos_read(unsigned char reg);
 extern void vrtc_cmos_write(unsigned char val, unsigned char reg);
-extern void vrtc_get_time(struct timespec *now);
-extern int vrtc_set_mmss(const struct timespec *now);
+extern void vrtc_get_time(struct timespec64 *now);
+extern int vrtc_set_mmss(const struct timespec64 *now);
 
 #endif
diff --git a/arch/x86/include/asm/mc146818rtc.h 
b/arch/x86/include/asm/mc146818rtc.h
index 24acd9ba7837..1b574e5eb3b2 100644
--- a/arch/x86/include/asm/mc146818rtc.h
+++ b/arch/x86/include/asm/mc146818rtc.h
@@ -94,8 +94,8 @@ static inline unsigned char current_lock_cmos_reg(void)
 unsigned char rtc_cmos_read(unsigned char addr);
 void rtc_cmos_write(unsigned char val, unsigned char addr);
 
-extern int mach_set_rtc_mmss(const struct timespec *now);
-extern void mach_get_cmos_time(struct timespec *now);
+extern int mach_set_rtc_mmss(const struct timespec64 *now);
+extern void mach_get_cmos_time(struct timespec64 *now);
 
 #define RTC_IRQ 8
 
diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 448cfe1b48cf..fc3138fd3aff 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -20,7 +20,7 @@ void pvclock_set_flags(u8 flags);
 unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src);
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
struct pvclock_vcpu_time_info *vcpu,
-   struct timespec *ts);
+   struct timespec64 *ts);
 void pvclock_resume(void);
 
 void pvclock_touch_watchdogs(void);
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index f45acdf45957..0c5007b72916 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -141,7 +141,7 @@ struct x86_cpuinit_ops {
void (*fixup_cpu_id)(struct cpuinfo_x86 *c, int node);
 };
 
-struct timespec;
+struct timespec64;
 
 /**
  * struct x86_legacy_devices - legacy x86 devices
@@ -223,8 +223,8 @@ struct x86_legacy_features {
 struct x86_platform_ops {
unsigned long (*calibrate_cpu)(void);
unsigned long (*calibrate_tsc)(void);
-   void (*get_wallclock)(struct timespec *ts);
-   int (*set_wallclock)(const struct timespec *ts);
+   void (*get_wallclock)(struct timespec64 *ts);
+   int (*set_wallclock)(const struct timespec64 *ts);
void (*iommu_shutdown)(void);
bool (*is_untracked_pat_range)(u64 start, u64 end);
void (*nmi_init)(void);
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d88967659098..01c76e8cd4be 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -58,7 +58,7 @@ EXPORT_SYMBOL_GPL(pvclock_pvti_cpu0_va);
  * have elapsed since the hypervisor wrote the data. So we try to account for
  * that with system time
  */
-static void kvm_get_wallclock(struct timespec *now)
+static void kvm_get_wallclock(struct timespec64 *now)
 {
struct pvclock_vcpu_time_info *vcpu_time;
int low, high;
@@ -77,7 +77,7 @@ static void kvm_get_wallclock(struct timespec *now)
put_cpu();
 }
 
-static int kvm_set_wallclock(const struct timespec *now)
+static int kvm_set_wallclock(const struct timespec64 *now)
 {
return -1;
 }
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 5c3f6d6a5078..1a7084ad07b9 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -121,11 +121,11 @@ u64 pvclock_clocksource_read(struct 
pvclock_vcpu_time_info *src)
 
 void pvclock_read_wallclock(struct pvclock_wall_clock *wall_clock,
   

[Xen-devel] [PATCH for-4.10 2/2] tools/libxc: Fix various code smells in send_memory_live()

[Andrew Cooper]

 * Don't zero ctx->save.stats; it is already zeroed
 * No need for x as it duplicates ctx->save.stats.iteration
 * Defer setting dirty_count until the bitmap has been filled to match the
   behaviour of XEN_DOMCTL_SHADOW_OP_CLEAN
 * Drop spurious blank line

Signed-off-by: Andrew Cooper 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Julien Grall 
---
 tools/libxc/xc_sr_save.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index beceb6a..afc5cb9 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -495,7 +495,6 @@ static int send_memory_live(struct xc_sr_context *ctx)
 xc_interface *xch = ctx->xch;
 xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size };
 char *progress_str = NULL;
-unsigned int x = 0;
 int rc;
 int policy_decision;
 
@@ -506,23 +505,18 @@ static int send_memory_live(struct xc_sr_context *ctx)
 ctx->save.callbacks->precopy_policy ?: simple_precopy_policy;
 
 void *data = ctx->save.callbacks->data;
-
-struct precopy_stats *policy_stats;
+struct precopy_stats *policy_stats = >save.stats;
 
 rc = update_progress_string(ctx, _str);
 if ( rc )
 goto out;
 
-ctx->save.stats = (struct precopy_stats)
-{ .dirty_count   = ctx->save.p2m_size };
-policy_stats = >save.stats;
-
 bitmap_set(dirty_bitmap, ctx->save.p2m_size);
+policy_stats->dirty_count = ctx->save.p2m_size;
 
 for ( ; ; )
 {
 policy_decision = precopy_policy(policy_stats, data);
-x++;
 
 if ( stats.dirty_count > 0 && policy_decision != XGS_POLICY_ABORT )
 {
@@ -538,7 +532,7 @@ static int send_memory_live(struct xc_sr_context *ctx)
 if ( policy_decision != XGS_POLICY_CONTINUE_PRECOPY )
 break;
 
-policy_stats->iteration = x;
+policy_stats->iteration++;
 policy_stats->total_written += policy_stats->dirty_count;
 policy_stats->dirty_count   = -1;
 
@@ -558,7 +552,6 @@ static int send_memory_live(struct xc_sr_context *ctx)
 }
 
 policy_stats->dirty_count = stats.dirty_count;
-
 }
 
  out:
-- 
2.1.4


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

[Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Andrew Cooper]

c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to
justify passing precopy_stats by value.

Under no circumstances can the precopy callback ever be executing in a
separate address space.

Switch the callback to passing by pointer which is far more efficient, and
drop the typedef (because none of the other callback have this oddity).

This is no functional change, as there are no users of this interface yet.

Signed-off-by: Andrew Cooper 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Julien Grall 

Appologies for this being late.  I did intend to get it sorted before code
freeze, but I was otherwise busy.
---
 tools/libxc/include/xenguest.h |  8 +---
 tools/libxc/xc_sr_save.c   | 19 +--
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index b4b2e19..f6a61ab 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -47,12 +47,6 @@ struct precopy_stats
 long dirty_count; /* -1 if unknown */
 };
 
-/*
- * A precopy_policy callback may not be running in the same address
- * space as libxc an so precopy_stats is passed by value.
- */
-typedef int (*precopy_policy_t)(struct precopy_stats, void *);
-
 /* callbacks provided by xc_domain_save */
 struct save_callbacks {
 /* Called after expiration of checkpoint interval,
@@ -72,7 +66,7 @@ struct save_callbacks {
 #define XGS_POLICY_CONTINUE_PRECOPY 0  /* Remain in the precopy phase. */
 #define XGS_POLICY_STOP_AND_COPY1  /* Immediately suspend and transmit the
 * remaining dirty pages. */
-precopy_policy_t precopy_policy;
+int (*precopy_policy)(struct precopy_stats *, void *);
 
 /*
  * Called after the guest's dirty pages have been
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 5a40e58..beceb6a 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -478,11 +478,11 @@ static int update_progress_string(struct xc_sr_context 
*ctx, char **str)
 #define SPP_MAX_ITERATIONS  5
 #define SPP_TARGET_DIRTY_COUNT 50
 
-static int simple_precopy_policy(struct precopy_stats stats, void *user)
+static int simple_precopy_policy(struct precopy_stats *stats, void *user)
 {
-return ((stats.dirty_count >= 0 &&
-stats.dirty_count < SPP_TARGET_DIRTY_COUNT) ||
-stats.iteration >= SPP_MAX_ITERATIONS)
+return ((stats->dirty_count >= 0 &&
+ stats->dirty_count < SPP_TARGET_DIRTY_COUNT) ||
+stats->iteration >= SPP_MAX_ITERATIONS)
 ? XGS_POLICY_STOP_AND_COPY
 : XGS_POLICY_CONTINUE_PRECOPY;
 }
@@ -502,7 +502,9 @@ static int send_memory_live(struct xc_sr_context *ctx)
 DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
 >save.dirty_bitmap_hbuf);
 
-precopy_policy_t precopy_policy = ctx->save.callbacks->precopy_policy;
+typeof(ctx->save.callbacks->precopy_policy) precopy_policy =
+ctx->save.callbacks->precopy_policy ?: simple_precopy_policy;
+
 void *data = ctx->save.callbacks->data;
 
 struct precopy_stats *policy_stats;
@@ -515,14 +517,11 @@ static int send_memory_live(struct xc_sr_context *ctx)
 { .dirty_count   = ctx->save.p2m_size };
 policy_stats = >save.stats;
 
-if ( precopy_policy == NULL )
- precopy_policy = simple_precopy_policy;
-
 bitmap_set(dirty_bitmap, ctx->save.p2m_size);
 
 for ( ; ; )
 {
-policy_decision = precopy_policy(*policy_stats, data);
+policy_decision = precopy_policy(policy_stats, data);
 x++;
 
 if ( stats.dirty_count > 0 && policy_decision != XGS_POLICY_ABORT )
@@ -543,7 +542,7 @@ static int send_memory_live(struct xc_sr_context *ctx)
 policy_stats->total_written += policy_stats->dirty_count;
 policy_stats->dirty_count   = -1;
 
-policy_decision = precopy_policy(*policy_stats, data);
+policy_decision = precopy_policy(policy_stats, data);
 
 if ( policy_decision != XGS_POLICY_CONTINUE_PRECOPY )
break;
-- 
2.1.4


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

Re: [Xen-devel] [PATCH v2] x86/hvm: Add MSR old value

[Razvan Cojocaru]

On 10/13/2017 07:26 PM, Tamas K Lengyel wrote:
> On Fri, Oct 13, 2017 at 9:50 AM, Jan Beulich  wrote:
> On 13.10.17 at 14:50,  wrote:
>>> This patch adds the old value param and the onchangeonly option
>>> to the VM_EVENT_REASON_MOV_TO_MSR event.
>>>
>>> The param was added to the vm_event_mov_to_msr struct and to the
>>> hvm_monitor_msr function. Finally I've changed the bool_t param
>>> to a bool for the hvm_msr_write_intercept function.
>>>
>>> Signed-off-by: Alexandru Isaila 
>>> Acked-by: Tamas K Lengyel 
>>
>> I think this should have been dropped with a bug having been fixed
>> (for only cosmetic changes it would be fine to keep). For the bits
>> where it's applicable
>> Acked-by: Jan Beulich 
> 
> Indeed. It's fine for now, my ack still stands.

That was my mistake - I thought that the fix was so obvious that Tamas
couldn't possibly object, and advised Alexandru to keep the ack. We'll
be more careful next time.


Thanks,
Razvan

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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Julien Grall]

Hi,

Sorry for the top-posting. Bhupinder, can you give a look?

Cheers,

On 13/10/17 16:06, Jan Beulich wrote:

On 13.10.17 at 16:35,  wrote:

Hi Jan,

On 13/10/17 15:03, Jan Beulich wrote:

On 13.10.17 at 15:03,  wrote:

On 13/10/17 13:32, Jan Beulich wrote:

On 13.10.17 at 14:19,  wrote:

On 13/10/17 13:08, Jan Beulich wrote:

On 13.10.17 at 12:44,  wrote:

In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:


flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));

However, xenstore reads this value as a decimal value and tries to map the
wrong address and fails.

Is this generic or vuart specific code in xenstore that does so?
Could you perhaps simply point me at the consuming side?


Introduced a new format string "PRIu_xen_pfn" which formats the value as a
decimal value.

I ask because I'm not really happy about this addition, i.e. I'd
prefer the read side to change.


Can the read side realistically change?


Well, that's why I had asked whether this is generic or specific
code. I would have hoped/assumed that xenstore doesn't
generically try to translate strings into numbers - how would it
know a string is to represent a number in the first place? Hence
I was hoping for this to be specific (and hence) new code.


Its been decimal for a decade now, and there definitely is 3rd party
code which uses these values in xenstore (sadly).


Are you trying to tell me there's been a vuart frontend before
the device type introduction in libxl, or is the new device type
compatible with an existing one?


Then again, the ring-ref key is listed as deprecated in our
documentation, without any reference describing which key should be used
instead.  It is also typically a grant reference, not a gfn, so
something wonky is definitely going on here.


Which put under question how an existing frontend could work
with this new device type.


Well, vuart is replicating the behavior of console (see
libxl__device_console_add). The console is passing a frame number in
decimal in "ring-ref". Confusingly it is an MFN and would even break on
32-bit toolstack using 64-bit Xen...

So this patch is just following the console behavior by passing a
decimal value rather than an hexadecimal value.


Well, that other code path should then also use PRIu_xen_pfn, at
the very least.


By other code path, you mean the console code right? In that case, mfn
should also be moved from unsigned long to xen_pfn_t.


Yes.


It's of course interesting that the apparent consumer
of this (tools/console/daemon/io.c:domain_create_ring()) uses

err = xs_gather(xs, dom->conspath,
"ring-ref", "%u", _ref,
"port", "%i", _port,
NULL);

in order to then cast(!) the result to unsigned long in the
invocation of xc_map_foreign_range(). Suggests to me that
the console can't work reliably on a system with memory
extending past the 1Tb boundary.


It likely a latent bug. Probably a silly question, would there any
compatibility issue to switch the format to the correct one?


I don't think so.


It of course escapes me why %i (or really %lli) wasn't used here
from the beginning, eliminating all radix concerns and matching
what is being done for the port.


Why %i? Should not the GFN be unsigned?


Signedness is secondary here - the important thing is that %i is
the only one allowing all of decimal, hex, and octal formatting of
the string (the latter two of course with the usual 0 / 0x prefixes).
Port numbers are unsigned too, yet %i is being used there.


Although, I can see the field
ring_reg is int and will store -1 as not mapped. This is quite confusing
and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.


Indeed.


But then, xc_map_foreign_range is using unsigned long instead of
xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.


Yes.


Note that the implementation of xc_map_foreign_range is using xen_pfn_t.


And yes again.

Jan



--
Julien Grall

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

Re: [Xen-devel] [PATCH v2] x86/hvm: Add MSR old value

[Tamas K Lengyel]

On Fri, Oct 13, 2017 at 9:50 AM, Jan Beulich  wrote:
 On 13.10.17 at 14:50,  wrote:
>> This patch adds the old value param and the onchangeonly option
>> to the VM_EVENT_REASON_MOV_TO_MSR event.
>>
>> The param was added to the vm_event_mov_to_msr struct and to the
>> hvm_monitor_msr function. Finally I've changed the bool_t param
>> to a bool for the hvm_msr_write_intercept function.
>>
>> Signed-off-by: Alexandru Isaila 
>> Acked-by: Tamas K Lengyel 
>
> I think this should have been dropped with a bug having been fixed
> (for only cosmetic changes it would be fine to keep). For the bits
> where it's applicable
> Acked-by: Jan Beulich 

Indeed. It's fine for now, my ack still stands.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in 
libxl__enum_from_string"):
> On 13/10/17 14:01, Ian Jackson wrote:
> > Instead, what we have actually done so far, is annotate when a pointer
> > parameter *may* be NULL, and, in that case, what that means:
> 
> This is exactly what attribute nonnull exists for.  As a bonus, using
> the attribute will have the compiler complain at you if it spots a way
> NULL gets passed, and UBSAN will add specific instrumentation to check.

Thanks for that excellent suggestion, which I ought to have thought of
myself.

I'd be quite happy with patches to add the nonnull attribute to the
parameters.  We already have that for a number of the *alloc*
functions - git-grep libxl for "NN1".

I don't mind the idea of adding that to some more functions now, even
if we don't have complete coverage.

Ian.

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

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

[Andrew Cooper]

On 13/10/17 17:07, Doug Goldstein wrote:
> On 10/13/17 2:40 AM, Jan Beulich wrote:
> On 12.10.17 at 22:56,  wrote:
>>> On 12/10/2017 21:50, Doug Goldstein wrote:
 From: David Esler 

 The send_chr function sends an entire C-string and not one character and
 doesn't necessarily just send it over the serial UART anymore so rename
 it to print_err so that its closer in name to what it does.

 Reviewed-by: Doug Goldstein 
 Signed-off-by: David Esler 
>>> Reviewed-by: Andrew Cooper 
>>>
>>> This should also be included in 4.10 IMO.
>> I'm not convinced - this is merely a cleanup style patch, the more
>> that the label really serves dual purpose and hence the original
>> name isn't all that wrong anyway.
>>
>> Jan
>>
> I purposefully broke it out so that it could be discussed. Prior to the
> commit I referenced in patch 1 the function sent out a character. Now it
> requires the supplied data to be a C-string (NULL terminated) so I would
> hope that you could agree that at else the "chr" part of "send_chr" is
> incorrect and should likely be "str" or "err".
>
> But again this patch isn't really important its more to try to make the
> label match the behavior of the code below the label and if its unwanted
> then it can be dropped.
>

We've not hit RC1 yet, and this is fixing a misleading bit of
documentation in a hard-to-follow piece of code.

I'm still of the opinion that it is 4.10 material.

~Andrew

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

Re: [Xen-devel] [PATCH 2/2] x86/boot: rename send_chr to print_err

[Doug Goldstein]

On 10/13/17 2:40 AM, Jan Beulich wrote:
 On 12.10.17 at 22:56,  wrote:
>> On 12/10/2017 21:50, Doug Goldstein wrote:
>>> From: David Esler 
>>>
>>> The send_chr function sends an entire C-string and not one character and
>>> doesn't necessarily just send it over the serial UART anymore so rename
>>> it to print_err so that its closer in name to what it does.
>>>
>>> Reviewed-by: Doug Goldstein 
>>> Signed-off-by: David Esler 
>>
>> Reviewed-by: Andrew Cooper 
>>
>> This should also be included in 4.10 IMO.
> 
> I'm not convinced - this is merely a cleanup style patch, the more
> that the label really serves dual purpose and hence the original
> name isn't all that wrong anyway.
> 
> Jan
> 

I purposefully broke it out so that it could be discussed. Prior to the
commit I referenced in patch 1 the function sent out a character. Now it
requires the supplied data to be a C-string (NULL terminated) so I would
hope that you could agree that at else the "chr" part of "send_chr" is
incorrect and should likely be "str" or "err".

But again this patch isn't really important its more to try to make the
label match the behavior of the code below the label and if its unwanted
then it can be dropped.

-- 
Doug Goldstein

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

Re: [Xen-devel] [PATCH v7 08/16] x86: implement set value flow for MBA

[Jan Beulich]

>>> On 13.10.17 at 10:41,  wrote:
> @@ -274,16 +280,18 @@ static enum psr_feat_type psr_type_to_feat_type(enum 
> psr_type type)
>  return feat_type;
>  }
>  
> -static bool psr_check_cbm(unsigned int cbm_len, unsigned long cbm)
> +/* Implementation of allocation features' functions. */
> +static bool cat_check_cbm(const struct feat_node *feat, uint32_t *val)
>  {
>  unsigned int first_bit, zero_bit;
> +unsigned int cbm_len = feat->cat.cbm_len;
> +unsigned long cbm = *val;

These are necessary changes.

> -/* Set bits should only in the range of [0, cbm_len]. */
> -if ( cbm & (~0ul << cbm_len) )
> -return false;
> -
> -/* At least one bit need to be set. */
> -if ( cbm == 0 )
> +/*
> + * Set bits should be only in the range of [0, cbm_len).
> + * And, at least one bit need to be set.
> + */
> +if ( (cbm & (~0ul << cbm_len)) || !cbm )

But all of this doesn't really belong here. I don't outright object to
you leaving it the way it is, but I'd prefer if you dropped these
changes, or moved them to a separate patch if you think this is
worthwhile.

> @@ -501,6 +511,35 @@ static bool mba_get_feat_info(const struct feat_node 
> *feat,
>  static void mba_write_msr(unsigned int cos, uint32_t val,
>enum psr_type type)
>  {
> +wrmsrl(MSR_IA32_PSR_MBA_MASK(cos), val);
> +}
> +
> +static bool mba_sanitize_thrtl(const struct feat_node *feat, uint32_t *thrtl)
> +{
> +if ( *thrtl > feat->mba.thrtl_max )
> +return false;
> +
> +/*
> + * Per SDM (chapter "Memory Bandwidth Allocation Configuration"):
> + * 1. Linear mode: In the linear mode the input precision is defined
> + *as 100-(MBA_MAX). For instance, if the MBA_MAX value is 90, the
> + *input precision is 10%. Values not an even multiple of the
> + *precision (e.g., 12%) will be rounded down (e.g., to 10% delay
> + *applied).
> + * 2. Non-linear mode: Input delay values are powers-of-two from zero
> + *to the MBA_MAX value from CPUID. In this case any values not a
> + *power of two will be rounded down the next nearest power of two.
> + */
> +if ( feat->mba.linear )
> +*thrtl -= *thrtl % (100 - feat->mba.thrtl_max);
> +else
> +{
> +/* Not power of 2. */
> +if ( *thrtl & (*thrtl - 1) )
> +*thrtl &= 1 << (flsl(*thrtl) - 1);

fls() will do now that the parameter type is uint32_t.

Also why do you think &= is better than plain = here?

Jan


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

Re: [Xen-devel] [PATCH v2] x86/hvm: Add MSR old value

[Jan Beulich]

>>> On 13.10.17 at 14:50,  wrote:
> This patch adds the old value param and the onchangeonly option
> to the VM_EVENT_REASON_MOV_TO_MSR event.
> 
> The param was added to the vm_event_mov_to_msr struct and to the
> hvm_monitor_msr function. Finally I've changed the bool_t param
> to a bool for the hvm_msr_write_intercept function.
> 
> Signed-off-by: Alexandru Isaila 
> Acked-by: Tamas K Lengyel 

I think this should have been dropped with a bug having been fixed
(for only cosmetic changes it would be fine to keep). For the bits
where it's applicable
Acked-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Andrew Cooper]

On 13/10/17 14:01, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH for-4.10] libxl: handle NULL in 
> libxl__enum_from_string"):
>> I agree they shouldn't be called with NULL. We should guard against
>> error (here or the libxl_*_type_from_string) or annotate the input can't
>> be NULL.
> I mean, who calls any  libxl_*_from_string  with s==NULL ?
>
> Also I don't think we should annotate that the input value can't be
> NULL, especially in a situation like this where the semantics could
> only be "this is wrong".  The API (and the internal calling
> conventions) are full of functions taking pointer arguments - are we
> going to annotate each one of those to say that it cannot be NULL ?
>
> Instead, what we have actually done so far, is annotate when a pointer
> parameter *may* be NULL, and, in that case, what that means:

This is exactly what attribute nonnull exists for.  As a bonus, using
the attribute will have the compiler complain at you if it spots a way
NULL gets passed, and UBSAN will add specific instrumentation to check.

Alternatively, you could assert(s) which would catch all (ab)uses and
also quiesce Coverity.

~Andrew

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

Re: [Xen-devel] [PATCH v3 6/6] x86/msr: handle VMX MSRs with guest_rd/wrmsr()

[Andrew Cooper]

On 13/10/17 13:35, Sergey Dyasli wrote:
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index a22e3dfaf2..2527fdd1d1 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -426,6 +426,13 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  return 0;
>  }
>  
> +#define vmx_guest_rdmsr(dp, name, msr) \
> +case name: \
> +if ( !dp->msr.available )  \
> +goto gp_fault; \
> +*val = dp->msr.u.raw;  \
> +break;

Eww :(

For blocks of MSRs, it would be far better to go with the same structure
as the cpuid policy.  Something like:

struct {
    union {
        uint64_t raw[NR_VMX_MSRS];
        struct {
            struct {
                ...
            } basic;
            struct {
                ...
            } pinbased_ctls;
        };
    };
} vmx;

This way, the guest_rdmsr() will be far more efficient.

case MSR_IA32_VMX_BASIC ... xxx:
    if ( !cpuid->basic.vmx )
        goto gp_fault;
    *val = dp->vmx.raw[msr - MSR_IA32_VMX_BASIC];
    break;

It would probably be worth splitting into a couple of different blocks
based on the different availability checks.

~Andrew

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

Re: [Xen-devel] [PATCH v3 5/6] x86/msr: update domain policy on CPUID policy changes

[Andrew Cooper]

On 13/10/17 13:35, Sergey Dyasli wrote:
> Availability of some MSRs depends on certain CPUID bits. Add function
> recalculate_domain_msr_policy() which updates availability of per-domain
> MSRs based on current domain's CPUID policy. This function is called
> when CPUID policy is changed from a toolstack.

This is probably acceptable for now.  recalculate_cpuid_policy() is only
a transitory artefact between the current behaviour of the Xen, and the
future behaviour of auditing the toolstack-provided cpuid and msr policy
completely before changing the domains datastructures.

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 205b4cb685..7e6b15f8d7 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -928,9 +928,8 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t 
> value,
>  X86_CR0_CD | X86_CR0_PG)))
>  
>  /* These bits in CR4 can be set by the guest. */
> -unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
> +unsigned long hvm_cr4_domain_valid_bits(const struct domain *d, bool restore)
>  {
> -const struct domain *d = v->domain;
>  const struct cpuid_policy *p;
>  bool mce, vmxe;
>  
> @@ -963,6 +962,11 @@ unsigned long hvm_cr4_guest_valid_bits(const struct vcpu 
> *v, bool restore)
>  (p->feat.pku  ? X86_CR4_PKE   : 0));
>  }
>  
> +unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)

I'd split this change out into a separate patch and change the existing
guest valid bits to taking a domain *.

It needed to take vcpu in the past because of the old cpuid
infrastructure, but it doesn't need to any more because of the
domain-wide struct cpuid policy.

~Andrew

> +{
> +return hvm_cr4_domain_valid_bits(v->domain, restore);
> +}
> +
>  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  {
>  int vcpuid;
>


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

Re: [Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Andrew Cooper]

On 13/10/17 13:35, Sergey Dyasli wrote:
> @@ -210,6 +375,255 @@ struct msr_domain_policy
>  bool available; /* This MSR is non-architectural */
>  bool cpuid_faulting;
>  } plaform_info;
> +
> +/* 0x0480  MSR_IA32_VMX_BASIC */
> +struct {
> +bool available;

We don't need available bits for any of these MSRs.  Their availability
is cpuid->basic.vmx, and we don't want (let alone need) to duplicate
information like this.

The PLATFORM_INFO and MISC_FEATURES_ENABLE are special, because they
have no architecturally defined indication of availability.

> +union {
> +uint64_t raw;
> +struct {
> +uint32_t vmcs_revision_id:31;
> +bool  mbz:1;  /* 31 always zero */
> +uint32_t vmcs_region_size:13;
> +uint32_t :3;  /* 45:47 reserved */
> +bool  addresses_32bit:1;
> +bool dual_monitor:1;
> +uint32_t  memory_type:4;
> +bool ins_out_info:1;
> +booldefault1_zero:1;
> +uint32_t :8;  /* 56:63 reserved */
> +};
> +} u;

The code will be rather shorter if you drop this .u and make the union
anonymous.

~Andrew

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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 16:35,  wrote:
> Hi Jan,
> 
> On 13/10/17 15:03, Jan Beulich wrote:
> On 13.10.17 at 15:03,  wrote:
>>> On 13/10/17 13:32, Jan Beulich wrote:
>>> On 13.10.17 at 14:19,  wrote:
> On 13/10/17 13:08, Jan Beulich wrote:
> On 13.10.17 at 12:44,  wrote:
>>> In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:
>>>
 flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, 
 state->vuart_gfn));
>>> However, xenstore reads this value as a decimal value and tries to map 
>>> the
>>> wrong address and fails.
>> Is this generic or vuart specific code in xenstore that does so?
>> Could you perhaps simply point me at the consuming side?
>>
>>> Introduced a new format string "PRIu_xen_pfn" which formats the value 
>>> as a
>>> decimal value.
>> I ask because I'm not really happy about this addition, i.e. I'd
>> prefer the read side to change.
>
> Can the read side realistically change?

 Well, that's why I had asked whether this is generic or specific
 code. I would have hoped/assumed that xenstore doesn't
 generically try to translate strings into numbers - how would it
 know a string is to represent a number in the first place? Hence
 I was hoping for this to be specific (and hence) new code.

> Its been decimal for a decade now, and there definitely is 3rd party
> code which uses these values in xenstore (sadly).

 Are you trying to tell me there's been a vuart frontend before
 the device type introduction in libxl, or is the new device type
 compatible with an existing one?

> Then again, the ring-ref key is listed as deprecated in our
> documentation, without any reference describing which key should be used
> instead.  It is also typically a grant reference, not a gfn, so
> something wonky is definitely going on here.

 Which put under question how an existing frontend could work
 with this new device type.
>>>
>>> Well, vuart is replicating the behavior of console (see
>>> libxl__device_console_add). The console is passing a frame number in
>>> decimal in "ring-ref". Confusingly it is an MFN and would even break on
>>> 32-bit toolstack using 64-bit Xen...
>>>
>>> So this patch is just following the console behavior by passing a
>>> decimal value rather than an hexadecimal value.
>> 
>> Well, that other code path should then also use PRIu_xen_pfn, at
>> the very least.
> 
> By other code path, you mean the console code right? In that case, mfn 
> should also be moved from unsigned long to xen_pfn_t.

Yes.

>> It's of course interesting that the apparent consumer
>> of this (tools/console/daemon/io.c:domain_create_ring()) uses
>> 
>>  err = xs_gather(xs, dom->conspath,
>>  "ring-ref", "%u", _ref,
>>  "port", "%i", _port,
>>  NULL);
>> 
>> in order to then cast(!) the result to unsigned long in the
>> invocation of xc_map_foreign_range(). Suggests to me that
>> the console can't work reliably on a system with memory
>> extending past the 1Tb boundary.
> 
> It likely a latent bug. Probably a silly question, would there any 
> compatibility issue to switch the format to the correct one?

I don't think so.

>> It of course escapes me why %i (or really %lli) wasn't used here
>> from the beginning, eliminating all radix concerns and matching
>> what is being done for the port.
> 
> Why %i? Should not the GFN be unsigned?

Signedness is secondary here - the important thing is that %i is
the only one allowing all of decimal, hex, and octal formatting of
the string (the latter two of course with the usual 0 / 0x prefixes).
Port numbers are unsigned too, yet %i is being used there.

> Although, I can see the field 
> ring_reg is int and will store -1 as not mapped. This is quite confusing 
> and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.

Indeed.

> But then, xc_map_foreign_range is using unsigned long instead of 
> xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.

Yes.

> Note that the implementation of xc_map_foreign_range is using xen_pfn_t.

And yes again.

Jan

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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Julien Grall]

Hi Jan,

On 13/10/17 15:03, Jan Beulich wrote:

On 13.10.17 at 15:03,  wrote:

On 13/10/17 13:32, Jan Beulich wrote:

On 13.10.17 at 14:19,  wrote:

On 13/10/17 13:08, Jan Beulich wrote:

On 13.10.17 at 12:44,  wrote:

In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:


flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));

However, xenstore reads this value as a decimal value and tries to map the
wrong address and fails.

Is this generic or vuart specific code in xenstore that does so?
Could you perhaps simply point me at the consuming side?


Introduced a new format string "PRIu_xen_pfn" which formats the value as a
decimal value.

I ask because I'm not really happy about this addition, i.e. I'd
prefer the read side to change.


Can the read side realistically change?


Well, that's why I had asked whether this is generic or specific
code. I would have hoped/assumed that xenstore doesn't
generically try to translate strings into numbers - how would it
know a string is to represent a number in the first place? Hence
I was hoping for this to be specific (and hence) new code.


Its been decimal for a decade now, and there definitely is 3rd party
code which uses these values in xenstore (sadly).


Are you trying to tell me there's been a vuart frontend before
the device type introduction in libxl, or is the new device type
compatible with an existing one?


Then again, the ring-ref key is listed as deprecated in our
documentation, without any reference describing which key should be used
instead.  It is also typically a grant reference, not a gfn, so
something wonky is definitely going on here.


Which put under question how an existing frontend could work
with this new device type.


Well, vuart is replicating the behavior of console (see
libxl__device_console_add). The console is passing a frame number in
decimal in "ring-ref". Confusingly it is an MFN and would even break on
32-bit toolstack using 64-bit Xen...

So this patch is just following the console behavior by passing a
decimal value rather than an hexadecimal value.


Well, that other code path should then also use PRIu_xen_pfn, at
the very least.


By other code path, you mean the console code right? In that case, mfn 
should also be moved from unsigned long to xen_pfn_t.



It's of course interesting that the apparent consumer
of this (tools/console/daemon/io.c:domain_create_ring()) uses

err = xs_gather(xs, dom->conspath,
"ring-ref", "%u", _ref,
"port", "%i", _port,
NULL);

in order to then cast(!) the result to unsigned long in the
invocation of xc_map_foreign_range(). Suggests to me that
the console can't work reliably on a system with memory
extending past the 1Tb boundary.


It likely a latent bug. Probably a silly question, would there any 
compatibility issue to switch the format to the correct one?




It of course escapes me why %i (or really %lli) wasn't used here
from the beginning, eliminating all radix concerns and matching
what is being done for the port.


Why %i? Should not the GFN be unsigned? Although, I can see the field 
ring_reg is int and will store -1 as not mapped. This is quite confusing 
and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.


But then, xc_map_foreign_range is using unsigned long instead of 
xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.


Note that the implementation of xc_map_foreign_range is using xen_pfn_t.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

[Tamas K Lengyel]

On Fri, Oct 13, 2017 at 6:17 AM, Jan Beulich  wrote:
 On 13.10.17 at 12:36,  wrote:
>> On 13.10.2017 13:29, Jan Beulich wrote:
 +__set_bit(index + sizeof(struct monitor_msr_bitmap), bitmap);
>>>
>>> I think you miss "* 8" here - a bit position plus sizeof() doesn't
>>> produce any useful value.
>>>
>>> But what's worse - having read till the end of the patch I don't
>>> see you change any allocation, yet you clearly need to double
>>> the space now that you need two bits per MSR.
>>
>> We did this:
>>
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index e59f1f5..a3046c6 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -25,7 +25,7 @@
>>   int arch_monitor_init_domain(struct domain *d)
>>   {
>>   if ( !d->arch.monitor.msr_bitmap )
>> -d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
>> +d->arch.monitor.msr_bitmap = xzalloc_array(struct 
>> monitor_msr_bitmap, 2);
>>
>>   if ( !d->arch.monitor.msr_bitmap )
>>   return -ENOMEM;
>> @@ -67,7 +67,7 @@ static unsigned long *monitor_bitmap_for_msr(const struct 
>> domain *d, u32 *msr)
>>   }
>>   }
>>
>> I.e., we are now allocating an array of size 2 of struct
>> monitor_msr_bitmaps with xzalloc_array().
>
> Oh, I'm not sure how I could overlook this considering that I
> specifically looked up the allocation point and searched through
> the patch for a respective change. I'm sorry for the noise in
> this regard. I do think though that the chosen model is a little
> odd and fragile, but that's something you and Tamas as the
> maintainers of the code have to judge about.
>

It looks fine to me.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v7 02/16] Rename PSR sysctl/domctl interfaces and xsm policy to make them be general

[Daniel De Graaf]

On 10/13/2017 04:40 AM, Yi Sun wrote:

This patch renames PSR sysctl/domctl interfaces and related xsm policy to
make them be general for all resource allocation features but not only
for CAT. Then, we can resuse the interfaces for all allocation features.

Basically, it changes 'psr_cat_op' to 'psr_alloc', and remove 'CAT_' from some
macros. E.g.:
1. psr_cat_op -> psr_alloc
2. XEN_DOMCTL_psr_cat_op -> XEN_DOMCTL_psr_alloc
3. XEN_SYSCTL_psr_cat_op -> XEN_SYSCTL_psr_alloc
4. XEN_DOMCTL_PSR_CAT_SET_L3_CBM -> XEN_DOMCTL_PSR_SET_L3_CBM
5. XEN_SYSCTL_PSR_CAT_get_l3_info -> XEN_SYSCTL_PSR_get_l3_info

Signed-off-by: Yi Sun 
Reviewed-by: Wei Liu 
Reviewed-by: Roger Pau Monné 
Acked-by: Jan Beulich 


Acked-by: Daniel De Graaf 

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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 15:03,  wrote:
> On 13/10/17 13:32, Jan Beulich wrote:
> On 13.10.17 at 14:19,  wrote:
>>> On 13/10/17 13:08, Jan Beulich wrote:
>>> On 13.10.17 at 12:44,  wrote:
> In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:
>
>> flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
> However, xenstore reads this value as a decimal value and tries to map the
> wrong address and fails.
 Is this generic or vuart specific code in xenstore that does so?
 Could you perhaps simply point me at the consuming side?

> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
> decimal value.
 I ask because I'm not really happy about this addition, i.e. I'd
 prefer the read side to change.
>>>
>>> Can the read side realistically change?
>> 
>> Well, that's why I had asked whether this is generic or specific
>> code. I would have hoped/assumed that xenstore doesn't
>> generically try to translate strings into numbers - how would it
>> know a string is to represent a number in the first place? Hence
>> I was hoping for this to be specific (and hence) new code.
>> 
>>> Its been decimal for a decade now, and there definitely is 3rd party
>>> code which uses these values in xenstore (sadly).
>> 
>> Are you trying to tell me there's been a vuart frontend before
>> the device type introduction in libxl, or is the new device type
>> compatible with an existing one?
>> 
>>> Then again, the ring-ref key is listed as deprecated in our
>>> documentation, without any reference describing which key should be used
>>> instead.  It is also typically a grant reference, not a gfn, so
>>> something wonky is definitely going on here.
>> 
>> Which put under question how an existing frontend could work
>> with this new device type.
> 
> Well, vuart is replicating the behavior of console (see 
> libxl__device_console_add). The console is passing a frame number in 
> decimal in "ring-ref". Confusingly it is an MFN and would even break on 
> 32-bit toolstack using 64-bit Xen...
> 
> So this patch is just following the console behavior by passing a 
> decimal value rather than an hexadecimal value.

Well, that other code path should then also use PRIu_xen_pfn, at
the very least. It's of course interesting that the apparent consumer
of this (tools/console/daemon/io.c:domain_create_ring()) uses

err = xs_gather(xs, dom->conspath,
"ring-ref", "%u", _ref,
"port", "%i", _port,
NULL);

in order to then cast(!) the result to unsigned long in the
invocation of xc_map_foreign_range(). Suggests to me that
the console can't work reliably on a system with memory
extending past the 1Tb boundary.

It of course escapes me why %i (or really %lli) wasn't used here
from the beginning, eliminating all radix concerns and matching
what is being done for the port.

Jan


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

Re: [Xen-devel] [PATCH 27/27 v12] arm/xen: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Dave Martin]

On Fri, Oct 13, 2017 at 04:10:31PM +0530, Bhupinder Thakur wrote:
> This patch fixes the issue observed when pl011 patches were tested on
> the junos hardware by Andre/Julien. It was observed that when large
> output is generated such as on running 'find /', output was getting
> truncated intermittently due to OUT ring buffer getting full.
> 
> This issue was due to the fact that the SBSA UART driver expects that
> when a TX interrupt is asserted then the FIFO queue should be atleast
> half empty and that it can write N bytes in the FIFO, where N is half
> the FIFO queue size, without the bytes getting dropped due to FIFO
> getting full.
> 
> The SBSA UART emulation logic was asserting the TX interrupt as soon
> as any space became available in the FIFO and the SBSA UART driver
> tried to write more data (upto 16 bytes) in the FIFO expecting that
> there is enough space available leading to dropped bytes.
> 
> The SBSA spec [1] does not specify when the TX interrupt should be
> asserted or de-asserted. Due to lack of clarity on the expected
> behavior, it is assumed for now that TX interrupt should be asserted
> only when the FIFO goes half empty.
> 
> TBD: Once the SBSA spec is updated with the expected behavior, the
> implementation will be modified to align with the spec requirement.
> 
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf
> 
> Signed-off-by: Bhupinder Thakur 
> ---

[...]

> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c

[...]

> @@ -355,28 +382,46 @@ static void vpl011_data_avail(struct domain *d)

[...]

> +/*
> + * Currently, the RXI bit is getting set even if there is a single
> + * byte of data in the rx fifo. Ideally, the RXI bit should be set
> + * only if the rx fifo level reaches the threshold.
> + *
> + * However, since currently RX timeout interrupt is not
> + * implemented as there is not enough clarity in the SBSA spec,
> + * the guest may keep waiting for an interrupt to read more
> + * data. To ensure that guest reads all the data without
> + * any delay, the RXI interrupt is raised if there is RX data
> + * available without checking whether fifo level has reached
> + * the threshold.
> + *
> + * TBD: Once there is more clarity in the SBSA spec on whether RX
> + * timeout interrupt needs to be implemented, the RXI interrupt
> + * will be raised only when rx fifo level reaches the threshold.
> + */

This looks OK to me: it makes the issues clear to future maintainers
of this code.

[...]

Cheers
---Dave

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

Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 13.10.17 at 15:00,  wrote:
> On Vi, 2017-10-13 at 03:51 -0600, Jan Beulich wrote:
>> > 
>> > 
>> > +BUILD_BUG_ON(sizeof(struct
>> > xen_hvm_altp2m_set_mem_access_multi) <
>> > + sizeof(struct
>> > compat_hvm_altp2m_set_mem_access_multi));
>> What good does this do?
>> Sorry, I don't understand how these size checks should look (are we
>> testing that the left hand side is at least as wide as the right hand
>> side?). Could you please give an example? Thanks!
> 
>> > + */
>> > +nat.altp2m_op->version  = a.version;
>> > +nat.altp2m_op->cmd  = a.cmd;
>> > +nat.altp2m_op->domain   = a.domain;
>> > +nat.altp2m_op->pad1 = a.pad1;
>> > +nat.altp2m_op->pad2 = a.pad2;
>> There are _still_ no size checks here.
> I'm not sure I understand what kind of size checks should be used here.
> Are you expecting something similar with the CHECK_FIELD_ macro?

Yes, as I've been stating repeatedly. You want to demonstrate
that field sizes match, or that at least no truncation can occur.

>> > 
>> > +if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0
>> > )
>> Please also check rc here to avoid needlessly copying back. In
>> fact _only_ checking rc ought to be fine.
> Actually, in this case rc is overwritten in do_altp2m_op (-EFAULT if
> the copy didn't work or the result of hypercall_create_continuation
> otherwise).

And it's the result of hypercall_create_continuation() which you
want to check against (provided there's no other way for rc to
obtain a positive value).

Jan


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

[Xen-devel] [qemu-mainline test] 114409: regressions - FAIL

[osstest service owner]

flight 114409 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/114409/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-pvh-amd  12 guest-start  fail REGR. vs. 114042
 test-amd64-amd64-xl-pvh-intel 12 guest-start fail REGR. vs. 114042
 test-armhf-armhf-xl-credit2 16 guest-start/debian.repeat fail REGR. vs. 114042

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop   fail REGR. vs. 114042

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 114042
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 114042
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 114042
 test-amd64-amd64-xl-rtds 10 debian-install   fail  like 114042
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 114042
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail  like 114042
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail  never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-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-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-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-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
 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-libvirt-raw 12 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass

version targeted for testing:
 qemuubac960832015bf4c4c1b873011612e2675e4464c
baseline version:
 qemuu5456c6a4ec9cd8fc314ddc303e88bf85c110975c

Last test of basis   114042  2017-10-05 12:15:47 Z8 days
Failing since114060  2017-10-06 05:53:34 Z7 days8 attempts
Testing same since   114409  2017-10-12 09:21:56 Z1 days1 attempts


People who touched revisions under test:
  Alex Williamson 
  Alistair Francis 
  Brandon Carpenter 
  Christian Borntraeger 
  Collin L. Walling 
  Cornelia Huck 
  Daniel P. Berrange 
  David Hildenbrand 
  Dr. David Alan Gilbert 
  Eduardo Habkost 
  Emilio G. Cota 
  Eric Blake 
  Fam Zheng 
  Gerd Hoffmann 
  Halil Pasic 
  Igor Mammedov 
  Jan Kiszka 
  Jason J . Herne 
  Jiang Biao 
  Kevin Wolf 

[Xen-devel] [OSSTEST PATCH] Bump HostDiskRoot to 20G

[Wei Liu]

Windows tests consume quite a bit more disk space under /root.

Signed-off-by: Wei Liu 
---
 Osstest.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest.pm b/Osstest.pm
index a78728c..34b5b6d 100644
--- a/Osstest.pm
+++ b/Osstest.pm
@@ -94,7 +94,7 @@ our %c = qw(
 
 HostDiskESP300
 HostDiskBoot   300
-HostDiskRoot 1
+HostDiskRoot 2
 HostDiskSwap  2000
 
 Baud  115200
-- 
2.11.0


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

[Xen-devel] [OSSTEST PATCH] HostDiskRoot: bump to 20G

[Ian Jackson]

Some of our Windows guests have more RAM now, and some of them have
big ISOs too.  The guest memory ends up in /root as a save image, and
the ISO ends up there too.

Double the size of / to 20G.  That will probably do for now and is
unlikely to break anything.

Signed-off-by: Ian Jackson 
Reported-by: Wei Liu 
---
 Osstest.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest.pm b/Osstest.pm
index a78728c..34b5b6d 100644
--- a/Osstest.pm
+++ b/Osstest.pm
@@ -94,7 +94,7 @@ our %c = qw(
 
 HostDiskESP300
 HostDiskBoot   300
-HostDiskRoot 1
+HostDiskRoot 2
 HostDiskSwap  2000
 
 Baud  115200
-- 
2.1.4


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

[Xen-devel] [PATCH v2] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

[Chao Gao]

pt_update_irq() is expected to return the vector number of periodic
timer interrupt, which should be set in vIRR of vlapic or in PIR.
Otherwise it would trigger the assertion in vmx_intr_assist(), please
seeing 
https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.

But it fails to achieve that in the following two case:
1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
mask field of IOAPIC RTE is set. Please refer to the call tree
vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
checks whether the vector is set or not in vIRR of vlapic or PIR before
returning.

2. someone changes the vector field of IOAPIC RTE between asserting
the irq and getting the vector of the irq, leading to setting the
old vector number but returning a different vector number. This patch
allows hvm_isa_irq_assert() to accept a callback which can get the
interrupt vector with irq_lock held. Thus, no one can change the vector
between the two operations.

Signed-off-by: Chao Gao 
---
passed the two simple xtf tests in 
https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
, which are designed to produce the above two cases.

v2:
- add a callback to hvm_isa_irq_assert() to avoid code duplication
- Constify vlapic argument of vlapic_test_irq()
---
 xen/arch/x86/hvm/dm.c |  2 +-
 xen/arch/x86/hvm/irq.c| 11 +--
 xen/arch/x86/hvm/pmtimer.c|  2 +-
 xen/arch/x86/hvm/rtc.c|  2 +-
 xen/arch/x86/hvm/vlapic.c | 12 
 xen/arch/x86/hvm/vmx/vmx.c|  7 +++
 xen/arch/x86/hvm/vpt.c| 39 ++-
 xen/include/asm-x86/hvm/hvm.h |  1 +
 xen/include/asm-x86/hvm/irq.h | 12 ++--
 xen/include/asm-x86/hvm/vlapic.h  |  1 +
 xen/include/asm-x86/hvm/vmx/vmx.h |  5 +
 11 files changed, 74 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 32ade95..a787f43 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -143,7 +143,7 @@ static int set_isa_irq_level(struct domain *d, uint8_t 
isa_irq,
 hvm_isa_irq_deassert(d, isa_irq);
 break;
 case 1:
-hvm_isa_irq_assert(d, isa_irq);
+hvm_isa_irq_assert(d, isa_irq, NULL);
 break;
 default:
 return -EINVAL;
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index e425df9..d79a367 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -168,11 +168,13 @@ void hvm_gsi_deassert(struct domain *d, unsigned int gsi)
 spin_unlock(>arch.hvm_domain.irq_lock);
 }
 
-void hvm_isa_irq_assert(
-struct domain *d, unsigned int isa_irq)
+int hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq,
+   int (*get_vector)(const struct domain *d,
+ unsigned int gsi))
 {
 struct hvm_irq *hvm_irq = hvm_domain_irq(d);
 unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
+int vector = 0;
 
 ASSERT(isa_irq <= 15);
 
@@ -182,7 +184,12 @@ void hvm_isa_irq_assert(
  (hvm_irq->gsi_assert_count[gsi]++ == 0) )
 assert_irq(d, gsi, isa_irq);
 
+if ( get_vector )
+vector = get_vector(d, gsi);
+
 spin_unlock(>arch.hvm_domain.irq_lock);
+
+return vector;
 }
 
 void hvm_isa_irq_deassert(
diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
index b70c299..435647f 100644
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -61,7 +61,7 @@ static void pmt_update_sci(PMTState *s)
 ASSERT(spin_is_locked(>lock));
 
 if ( acpi->pm1a_en & acpi->pm1a_sts & SCI_MASK )
-hvm_isa_irq_assert(s->vcpu->domain, SCI_IRQ);
+hvm_isa_irq_assert(s->vcpu->domain, SCI_IRQ, NULL);
 else
 hvm_isa_irq_deassert(s->vcpu->domain, SCI_IRQ);
 }
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index bcfa169..cb75b99 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -75,7 +75,7 @@ static void rtc_update_irq(RTCState *s)
 s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
 if ( rtc_mode_is(s, no_ack) )
 hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
-hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
+hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ, NULL);
 }
 
 /* Called by the VPT code after it's injected a PF interrupt for us.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4bfc53e..50f53bd 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -137,6 +137,18 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
int errmask)
 spin_unlock_irqrestore(>esr_lock, flags);
 }
 
+bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec)
+{
+if ( unlikely(vec < 16) )
+return false;
+
+if ( hvm_funcs.test_pir &&
+ hvm_funcs.test_pir(const_vlapic_vcpu(vlapic), vec) 

Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Ian Jackson]

Wei Liu writes ("Re: [PATCH for-4.10] libxl: handle NULL in 
libxl__enum_from_string"):
> I agree they shouldn't be called with NULL. We should guard against
> error (here or the libxl_*_type_from_string) or annotate the input can't
> be NULL.

I mean, who calls any  libxl_*_from_string  with s==NULL ?

Also I don't think we should annotate that the input value can't be
NULL, especially in a situation like this where the semantics could
only be "this is wrong".  The API (and the internal calling
conventions) are full of functions taking pointer arguments - are we
going to annotate each one of those to say that it cannot be NULL ?

Instead, what we have actually done so far, is annotate when a pointer
parameter *may* be NULL, and, in that case, what that means:

 * If ao_how==NULL, the function will be synchronous.
 * If ao_how->callback==NULL, a libxl_event will be generated which
  /* if old_name is NULL, any old name is OK; otherwise we check
/* May be called with info_r == NULL to check for domain's existence.
   * event_occurs may be NULL if mask is 0.
   * disaster may be NULL.  If it is, or if _register_callbacks has
 * The application may call beforepoll with fds==NULL and
_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr /* may be * NULL */) 
NN1;
_hidden char *libxl__strdup(libxl__gc *gc_opt,
const char *c /* may be NULL */) NN1;

etc. etc.

Ian.

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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Julien Grall]

Hi,

On 13/10/17 13:32, Jan Beulich wrote:

On 13.10.17 at 14:19,  wrote:

On 13/10/17 13:08, Jan Beulich wrote:

On 13.10.17 at 12:44,  wrote:

In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:


flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));

However, xenstore reads this value as a decimal value and tries to map the
wrong address and fails.

Is this generic or vuart specific code in xenstore that does so?
Could you perhaps simply point me at the consuming side?


Introduced a new format string "PRIu_xen_pfn" which formats the value as a
decimal value.

I ask because I'm not really happy about this addition, i.e. I'd
prefer the read side to change.


Can the read side realistically change?


Well, that's why I had asked whether this is generic or specific
code. I would have hoped/assumed that xenstore doesn't
generically try to translate strings into numbers - how would it
know a string is to represent a number in the first place? Hence
I was hoping for this to be specific (and hence) new code.


Its been decimal for a decade now, and there definitely is 3rd party
code which uses these values in xenstore (sadly).


Are you trying to tell me there's been a vuart frontend before
the device type introduction in libxl, or is the new device type
compatible with an existing one?


Then again, the ring-ref key is listed as deprecated in our
documentation, without any reference describing which key should be used
instead.  It is also typically a grant reference, not a gfn, so
something wonky is definitely going on here.


Which put under question how an existing frontend could work
with this new device type.


Well, vuart is replicating the behavior of console (see 
libxl__device_console_add). The console is passing a frame number in 
decimal in "ring-ref". Confusingly it is an MFN and would even break on 
32-bit toolstack using 64-bit Xen...


So this patch is just following the console behavior by passing a 
decimal value rather than an hexadecimal value.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Petre Ovidiu PIRCALABU]

On Vi, 2017-10-13 at 03:51 -0600, Jan Beulich wrote:
> > 
> > 
> > +BUILD_BUG_ON(sizeof(struct
> > xen_hvm_altp2m_set_mem_access_multi) <
> > + sizeof(struct
> > compat_hvm_altp2m_set_mem_access_multi));
> What good does this do?
> Sorry, I don't understand how these size checks should look (are we
> testing that the left hand side is at least as wide as the right hand
> side?). Could you please give an example? Thanks!

> > + */
> > +nat.altp2m_op->version  = a.version;
> > +nat.altp2m_op->cmd  = a.cmd;
> > +nat.altp2m_op->domain   = a.domain;
> > +nat.altp2m_op->pad1 = a.pad1;
> > +nat.altp2m_op->pad2 = a.pad2;
> There are _still_ no size checks here.
I'm not sure I understand what kind of size checks should be used here.
Are you expecting something similar with the CHECK_FIELD_ macro?
> > 
> > +if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0
> > )
> Please also check rc here to avoid needlessly copying back. In
> fact _only_ checking rc ought to be fine.
Actually, in this case rc is overwritten in do_altp2m_op (-EFAULT if
the copy didn't work or the result of hypercall_create_continuation
otherwise).
I have used the check for nat.altp2m_op->u.set_mem_access_multi.opaque
as it usually gets set to a positive value only if the hypercall gets
preempted,

Many thanks for your support,
Petre
> 
> > 
> 
> Jan
> 
> 
> 
> This email was scanned by Bitdefender
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Wei Liu]

On Fri, Oct 13, 2017 at 01:46:57PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH for-4.10] libxl: handle NULL in 
> libxl__enum_from_string"):
> > Discovered by Coverity.
> 
> But.  Surely it is very wrong
> 
> > @@ -1017,7 +1017,7 @@ int libxl_get_max_nodes(libxl_ctx *ctx)
> >  int libxl__enum_from_string(const libxl_enum_string_table *t,
> >  const char *s, int *e)
> >  {
> > -if (!t) return ERROR_INVAL;
> > +if (!t || !s) return ERROR_INVAL;
> 
> to call this function with s==NULL.
> 
> I'm not generally in favour of turning such mistakes from
> easy-to-debug crashes into hard-to-track-down error codes (especially
> with our nonspecific error codes).
> 
> Where does that occur ?
> 

libxl_*_type_from_string.

I agree they shouldn't be called with NULL. We should guard against
error (here or the libxl_*_type_from_string) or annotate the input can't
be NULL.

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

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  46aaf41ee099a048d7a554c03ae01bcdaa07f776
baseline version:
 xen  cc08c73c8c1f5ba5ed0f8274548db6725e1c3157

Last test of basis   114426  2017-10-12 16:05:15 Z0 days
Testing same since   114460  2017-10-13 11:06:15 Z0 days1 attempts


People who touched revisions under test:
  Ian Jackson 
  Jan Beulich 
  Wei Liu 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  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 :

+ branch=xen-unstable-smoke
+ revision=46aaf41ee099a048d7a554c03ae01bcdaa07f776
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
 export PERLLIB=.:.
 PERLLIB=.:.
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
46aaf41ee099a048d7a554c03ae01bcdaa07f776
+ branch=xen-unstable-smoke
+ revision=46aaf41ee099a048d7a554c03ae01bcdaa07f776
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
 export PERLLIB=.:.:.
 PERLLIB=.:.:.
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
+++ export PERLLIB=.:.:.:.
+++ PERLLIB=.:.:.:.
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.9-testing
+ '[' x46aaf41ee099a048d7a554c03ae01bcdaa07f776 = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/xtf.git
++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git
++ : git://xenbits.xen.org/xtf.git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git
++ : git://git.seabios.org/seabios.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git
++ : git://xenbits.xen.org/osstest/seabios.git
++ : https://github.com/tianocore/edk2.git
++ : 

[Xen-devel] [PATCH v2] x86/hvm: Add MSR old value

[Alexandru Isaila]

This patch adds the old value param and the onchangeonly option
to the VM_EVENT_REASON_MOV_TO_MSR event.

The param was added to the vm_event_mov_to_msr struct and to the
hvm_monitor_msr function. Finally I've changed the bool_t param
to a bool for the hvm_msr_write_intercept function.

Signed-off-by: Alexandru Isaila 
Acked-by: Tamas K Lengyel 

---
Changes since V1:
- Removed Stray blanks inside the inner parentheses
- Added space after the if statement
- Added * 8 to the set/clear/test_bit statements
- Removed the blank line after monitored_msr.
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_monitor.c  |  3 ++-
 xen/arch/x86/hvm/hvm.c| 10 --
 xen/arch/x86/hvm/monitor.c|  9 ++---
 xen/arch/x86/monitor.c| 26 +++---
 xen/include/asm-x86/hvm/monitor.h |  2 +-
 xen/include/asm-x86/hvm/support.h |  2 +-
 xen/include/asm-x86/monitor.h |  1 +
 xen/include/public/domctl.h   |  2 ++
 xen/include/public/vm_event.h |  5 +++--
 10 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bcab3c..b99d6eb 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2048,7 +2048,7 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t 
domain_id,
  * non-architectural indices.
  */
 int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
-  bool enable);
+  bool enable, bool onchangeonly);
 int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
 int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 6046680..09d04be 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -90,7 +90,7 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t 
domain_id,
 }
 
 int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
-  bool enable)
+  bool enable, bool onchangeonly)
 {
 DECLARE_DOMCTL;
 
@@ -100,6 +100,7 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t 
domain_id, uint32_t msr,
 : XEN_DOMCTL_MONITOR_OP_DISABLE;
 domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR;
 domctl.u.monitor_op.u.mov_to_msr.msr = msr;
+domctl.u.monitor_op.u.mov_to_msr.onchangeonly = onchangeonly;
 
 return do_domctl(xch, );
 }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb..0238787 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3489,7 +3489,7 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t 
*msr_content)
 }
 
 int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
-bool_t may_defer)
+bool may_defer)
 {
 struct vcpu *v = current;
 struct domain *d = v->domain;
@@ -3500,6 +3500,12 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t 
msr_content,
 
 if ( may_defer && unlikely(monitored_msr(v->domain, msr)) )
 {
+uint64_t msr_old_content;
+
+ret = hvm_msr_read_intercept(msr, _old_content);
+if ( ret != X86EMUL_OKAY )
+return ret;
+
 ASSERT(v->arch.vm_event);
 
 /* The actual write will occur in hvm_do_resume() (if permitted). */
@@ -3507,7 +3513,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t 
msr_content,
 v->arch.vm_event->write_data.msr = msr;
 v->arch.vm_event->write_data.value = msr_content;
 
-hvm_monitor_msr(msr, msr_content);
+hvm_monitor_msr(msr, msr_content, msr_old_content);
 return X86EMUL_OKAY;
 }
 
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 4ce778c..131b852 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -74,16 +74,19 @@ bool hvm_monitor_emul_unimplemented(void)
 monitor_traps(curr, true, ) == 1;
 }
 
-void hvm_monitor_msr(unsigned int msr, uint64_t value)
+void hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t old_value)
 {
 struct vcpu *curr = current;
 
-if ( monitored_msr(curr->domain, msr) )
+if ( monitored_msr(curr->domain, msr) &&
+ (!monitored_msr_onchangeonly(curr->domain, msr) ||
+   new_value != old_value) )
 {
 vm_event_request_t req = {
 .reason = VM_EVENT_REASON_MOV_TO_MSR,
 .u.mov_to_msr.msr = msr,
-.u.mov_to_msr.value = value,
+.u.mov_to_msr.new_value = new_value,
+.u.mov_to_msr.old_value = old_value
 };
 
 monitor_traps(curr, 1, );
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index e59f1f5..ecfa9e5 

Re: [Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Ian Jackson]

Wei Liu writes ("[PATCH for-4.10] libxl: handle NULL in 
libxl__enum_from_string"):
> Discovered by Coverity.

But.  Surely it is very wrong

> @@ -1017,7 +1017,7 @@ int libxl_get_max_nodes(libxl_ctx *ctx)
>  int libxl__enum_from_string(const libxl_enum_string_table *t,
>  const char *s, int *e)
>  {
> -if (!t) return ERROR_INVAL;
> +if (!t || !s) return ERROR_INVAL;

to call this function with s==NULL.

I'm not generally in favour of turning such mistakes from
easy-to-debug crashes into hard-to-track-down error codes (especially
with our nonspecific error codes).

Where does that occur ?

Ian.

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

[Xen-devel] [PATCH v3 3/6] x86/msr: read VMX MSRs values into Raw policy

[Sergey Dyasli]

Add calculate_raw_vmx_policy() which fills Raw policy with H/W values
of VMX MSRs. Host policy will contain a copy of these values.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/msr.c | 77 ++
 1 file changed, 77 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 24029a2ac1..955aba0849 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -32,6 +32,81 @@ struct msr_domain_policy __read_mostly 
raw_msr_domain_policy,
 struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
__read_mostly  pv_max_msr_vcpu_policy;
 
+static void __init calculate_raw_vmx_policy(struct msr_domain_policy *dp)
+{
+if ( !cpu_has_vmx )
+return;
+
+dp->vmx_basic.available = true;
+rdmsrl(MSR_IA32_VMX_BASIC, dp->vmx_basic.u.raw);
+
+dp->vmx_pinbased_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_PINBASED_CTLS, dp->vmx_pinbased_ctls.u.raw);
+
+dp->vmx_procbased_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_PROCBASED_CTLS, dp->vmx_procbased_ctls.u.raw);
+
+dp->vmx_exit_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_EXIT_CTLS, dp->vmx_exit_ctls.u.raw);
+
+dp->vmx_entry_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_ENTRY_CTLS, dp->vmx_entry_ctls.u.raw);
+
+dp->vmx_misc.available = true;
+rdmsrl(MSR_IA32_VMX_MISC, dp->vmx_misc.u.raw);
+
+dp->vmx_cr0_fixed0.available = true;
+rdmsrl(MSR_IA32_VMX_CR0_FIXED0, dp->vmx_cr0_fixed0.u.raw);
+
+dp->vmx_cr0_fixed1.available = true;
+rdmsrl(MSR_IA32_VMX_CR0_FIXED1, dp->vmx_cr0_fixed1.u.raw);
+
+dp->vmx_cr4_fixed0.available = true;
+rdmsrl(MSR_IA32_VMX_CR4_FIXED0, dp->vmx_cr4_fixed0.u.raw);
+
+dp->vmx_cr4_fixed1.available = true;
+rdmsrl(MSR_IA32_VMX_CR4_FIXED1, dp->vmx_cr4_fixed1.u.raw);
+
+dp->vmx_vmcs_enum.available = true;
+rdmsrl(MSR_IA32_VMX_VMCS_ENUM, dp->vmx_vmcs_enum.u.raw);
+
+if ( dp->vmx_procbased_ctls.u.allowed_1.activate_secondary_controls )
+{
+dp->vmx_procbased_ctls2.available = true;
+rdmsrl(MSR_IA32_VMX_PROCBASED_CTLS2, dp->vmx_procbased_ctls2.u.raw);
+
+if ( dp->vmx_procbased_ctls2.u.allowed_1.enable_ept ||
+ dp->vmx_procbased_ctls2.u.allowed_1.enable_vpid )
+{
+dp->vmx_ept_vpid_cap.available = true;
+rdmsrl(MSR_IA32_VMX_EPT_VPID_CAP, dp->vmx_ept_vpid_cap.u.raw);
+}
+}
+
+if ( dp->vmx_basic.u.default1_zero )
+{
+dp->vmx_true_pinbased_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_TRUE_PINBASED_CTLS,
+   dp->vmx_true_pinbased_ctls.u.raw);
+
+dp->vmx_true_procbased_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_TRUE_PROCBASED_CTLS,
+   dp->vmx_true_procbased_ctls.u.raw);
+
+dp->vmx_true_exit_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_TRUE_EXIT_CTLS, dp->vmx_true_exit_ctls.u.raw);
+
+dp->vmx_true_entry_ctls.available = true;
+rdmsrl(MSR_IA32_VMX_TRUE_ENTRY_CTLS, dp->vmx_true_entry_ctls.u.raw);
+}
+
+if ( dp->vmx_procbased_ctls2.u.allowed_1.enable_vm_functions )
+{
+dp->vmx_vmfunc.available = true;
+rdmsrl(MSR_IA32_VMX_VMFUNC, dp->vmx_vmfunc.u.raw);
+}
+}
+
 static void __init calculate_raw_policy(void)
 {
 struct msr_domain_policy *dp = _msr_domain_policy;
@@ -43,6 +118,8 @@ static void __init calculate_raw_policy(void)
 if ( val & MSR_PLATFORM_INFO_CPUID_FAULTING )
 dp->plaform_info.cpuid_faulting = true;
 }
+
+calculate_raw_vmx_policy(dp);
 }
 
 static void __init calculate_host_policy(void)
-- 
2.11.0


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

[Xen-devel] [PATCH v3 4/6] x86/msr: add VMX MSRs into HVM_max domain policy

[Sergey Dyasli]

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.

Add calculate_hvm_max_vmx_policy() which will save the end result of
nvmx_msr_read_intercept() on current H/W into HVM_max domain policy.
There will be no functional change to what L1 sees in VMX MSRs. But the
actual use of HVM_max domain policy will happen later, when VMX MSRs
are handled by guest_rd/wrmsr().

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/msr.c | 140 +
 1 file changed, 140 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 955aba0849..388f19e50d 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -129,6 +129,144 @@ static void __init calculate_host_policy(void)
 *dp = raw_msr_domain_policy;
 }
 
+#define vmx_host_allowed_cpy(dp, msr, field) \
+do { \
+dp->msr.u.allowed_1.field =  \
+host_msr_domain_policy.msr.u.allowed_1.field;\
+dp->msr.u.allowed_0.field =  \
+host_msr_domain_policy.msr.u.allowed_0.field;\
+} while (0)
+
+static void __init calculate_hvm_max_vmx_policy(struct msr_domain_policy *dp)
+{
+if ( !cpu_has_vmx )
+return;
+
+dp->vmx_basic.available = true;
+dp->vmx_basic.u.raw = host_msr_domain_policy.vmx_basic.u.raw;
+
+dp->vmx_pinbased_ctls.available = true;
+dp->vmx_pinbased_ctls.u.raw =
+((uint64_t) VMX_PINBASED_CTLS_DEFAULT1 << 32) |
+VMX_PINBASED_CTLS_DEFAULT1;
+vmx_host_allowed_cpy(dp, vmx_pinbased_ctls, ext_intr_exiting);
+vmx_host_allowed_cpy(dp, vmx_pinbased_ctls, nmi_exiting);
+vmx_host_allowed_cpy(dp, vmx_pinbased_ctls, preempt_timer);
+
+dp->vmx_procbased_ctls.available = true;
+dp->vmx_procbased_ctls.u.raw =
+((uint64_t) VMX_PROCBASED_CTLS_DEFAULT1 << 32) |
+VMX_PROCBASED_CTLS_DEFAULT1;
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, virtual_intr_pending);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, use_tsc_offseting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, hlt_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, invlpg_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, mwait_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, rdpmc_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, rdtsc_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, cr8_load_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, cr8_store_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, tpr_shadow);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, virtual_nmi_pending);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, mov_dr_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, uncond_io_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, activate_io_bitmap);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, monitor_trap_flag);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, activate_msr_bitmap);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, monitor_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, pause_exiting);
+vmx_host_allowed_cpy(dp, vmx_procbased_ctls, activate_secondary_controls);
+
+dp->vmx_exit_ctls.available = true;
+dp->vmx_exit_ctls.u.raw =
+((uint64_t) VMX_EXIT_CTLS_DEFAULT1 << 32) |
+VMX_EXIT_CTLS_DEFAULT1;
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, ia32e_mode);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, load_perf_global_ctrl);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, ack_intr_on_exit);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, save_guest_pat);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, load_host_pat);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, save_guest_efer);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, load_host_efer);
+vmx_host_allowed_cpy(dp, vmx_exit_ctls, save_preempt_timer);
+
+dp->vmx_entry_ctls.available = true;
+dp->vmx_entry_ctls.u.raw =
+((uint64_t) VMX_ENTRY_CTLS_DEFAULT1 << 32) |
+VMX_ENTRY_CTLS_DEFAULT1;
+vmx_host_allowed_cpy(dp, vmx_entry_ctls, ia32e_mode);
+vmx_host_allowed_cpy(dp, vmx_entry_ctls, load_perf_global_ctrl);
+vmx_host_allowed_cpy(dp, vmx_entry_ctls, load_guest_pat);
+vmx_host_allowed_cpy(dp, vmx_entry_ctls, load_guest_efer);
+
+dp->vmx_misc.available = true;
+dp->vmx_misc.u.raw = host_msr_domain_policy.vmx_misc.u.raw;
+/* Do not support CR3-target feature now */
+dp->vmx_misc.u.cr3_target = false;
+
+dp->vmx_cr0_fixed0.available = true;
+/* PG, PE bits must be 1 in VMX operation */
+dp->vmx_cr0_fixed0.u.allowed_0.pe = true;
+dp->vmx_cr0_fixed0.u.allowed_0.pg = true;
+
+ 

[Xen-devel] [PATCH v3 0/6] VMX MSRs policy for Nested Virt: part 1

[Sergey Dyasli]

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.

v2 --> v3:
- Rebase on top of Generic MSR Policy
- Each VMX MSR now has its own availability flag
- VMX MSRs are now completely defined during domain creation
  (all CPUID policy changes are taken into account)

Sergey Dyasli (6):
  x86/msr: add Raw and Host domain policies
  x86/msr: add VMX MSRs into struct msr_domain_policy
  x86/msr: read VMX MSRs values into Raw policy
  x86/msr: add VMX MSRs into HVM_max domain policy
  x86/msr: update domain policy on CPUID policy changes
  x86/msr: handle VMX MSRs with guest_rd/wrmsr()

 xen/arch/x86/domctl.c  |   1 +
 xen/arch/x86/hvm/hvm.c |   8 +-
 xen/arch/x86/hvm/vmx/vmx.c |   6 -
 xen/arch/x86/hvm/vmx/vvmx.c| 178 
 xen/arch/x86/msr.c | 387 +-
 xen/include/asm-x86/hvm/hvm.h  |   1 +
 xen/include/asm-x86/hvm/vmx/vvmx.h |   2 -
 xen/include/asm-x86/msr.h  | 417 +
 8 files changed, 811 insertions(+), 189 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH v3 6/6] x86/msr: handle VMX MSRs with guest_rd/wrmsr()

[Sergey Dyasli]

Now that each domain has a correct view of VMX MSRs in it's per-domain
MSR policy, it's possible to handle guest's RD/WRMSR with the new
handlers. Do it and remove the old nvmx_msr_read_intercept() and
associated bits.

There is no functional change to what a guest sees in VMX MSRs.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/hvm/vmx/vmx.c |   6 --
 xen/arch/x86/hvm/vmx/vvmx.c| 178 -
 xen/arch/x86/msr.c |  34 +++
 xen/include/asm-x86/hvm/vmx/vvmx.h |   2 -
 4 files changed, 34 insertions(+), 186 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c2148701ee..1a1cb98069 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2906,10 +2906,6 @@ static int vmx_msr_read_intercept(unsigned int msr, 
uint64_t *msr_content)
 if ( nestedhvm_enabled(curr->domain) )
 *msr_content |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;
 break;
-case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
-if ( !nvmx_msr_read_intercept(msr, msr_content) )
-goto gp_fault;
-break;
 case MSR_IA32_MISC_ENABLE:
 rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
 /* Debug Trace Store is not supported. */
@@ -3133,8 +3129,6 @@ static int vmx_msr_write_intercept(unsigned int msr, 
uint64_t msr_content)
 break;
 }
 case MSR_IA32_FEATURE_CONTROL:
-case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
-/* None of these MSRs are writeable. */
 goto gp_fault;
 
 case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index dde02c076b..b0474ad310 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1976,184 +1976,6 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
 return X86EMUL_OKAY;
 }
 
-#define __emul_value(enable1, default1) \
-((enable1 | default1) << 32 | (default1))
-
-#define gen_vmx_msr(enable1, default1, host_value) \
-(((__emul_value(enable1, default1) & host_value) & (~0ul << 32)) | \
-((uint32_t)(__emul_value(enable1, default1) | host_value)))
-
-/*
- * Capability reporting
- */
-int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
-{
-struct vcpu *v = current;
-struct domain *d = v->domain;
-u64 data = 0, host_data = 0;
-int r = 1;
-
-/* VMX capablity MSRs are available only when guest supports VMX. */
-if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
-return 0;
-
-/*
- * These MSRs are only available when flags in other MSRs are set.
- * These prerequisites are listed in the Intel 64 and IA-32
- * Architectures Software Developer’s Manual, Vol 3, Appendix A.
- */
-switch ( msr )
-{
-case MSR_IA32_VMX_PROCBASED_CTLS2:
-if ( !cpu_has_vmx_secondary_exec_control )
-return 0;
-break;
-
-case MSR_IA32_VMX_EPT_VPID_CAP:
-if ( !(cpu_has_vmx_ept || cpu_has_vmx_vpid) )
-return 0;
-break;
-
-case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
-case MSR_IA32_VMX_TRUE_EXIT_CTLS:
-case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
-if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) )
-return 0;
-break;
-
-case MSR_IA32_VMX_VMFUNC:
-if ( !cpu_has_vmx_vmfunc )
-return 0;
-break;
-}
-
-rdmsrl(msr, host_data);
-
-/*
- * Remove unsupport features from n1 guest capability MSR
- */
-switch (msr) {
-case MSR_IA32_VMX_BASIC:
-{
-const struct vmcs_struct *vmcs =
-map_domain_page(_mfn(PFN_DOWN(v->arch.hvm_vmx.vmcs_pa)));
-
-data = (host_data & (~0ul << 32)) |
-   (vmcs->vmcs_revision_id & 0x7fff);
-unmap_domain_page(vmcs);
-break;
-}
-case MSR_IA32_VMX_PINBASED_CTLS:
-case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-/* 1-settings */
-data = PIN_BASED_EXT_INTR_MASK |
-   PIN_BASED_NMI_EXITING |
-   PIN_BASED_PREEMPT_TIMER;
-data = gen_vmx_msr(data, VMX_PINBASED_CTLS_DEFAULT1, host_data);
-break;
-case MSR_IA32_VMX_PROCBASED_CTLS:
-case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
-{
-u32 default1_bits = VMX_PROCBASED_CTLS_DEFAULT1;
-/* 1-settings */
-data = CPU_BASED_HLT_EXITING |
-   CPU_BASED_VIRTUAL_INTR_PENDING |
-   CPU_BASED_CR8_LOAD_EXITING |
-   CPU_BASED_CR8_STORE_EXITING |
-   CPU_BASED_INVLPG_EXITING |
-   CPU_BASED_CR3_LOAD_EXITING |
-   CPU_BASED_CR3_STORE_EXITING |
-   CPU_BASED_MONITOR_EXITING |
-   CPU_BASED_MWAIT_EXITING |
-   CPU_BASED_MOV_DR_EXITING |
-   CPU_BASED_ACTIVATE_IO_BITMAP |
-   CPU_BASED_USE_TSC_OFFSETING |
-   

[Xen-devel] [PATCH v3 1/6] x86/msr: add Raw and Host domain policies

[Sergey Dyasli]

Raw policy contains the actual values from H/W MSRs. PLATFORM_INFO msr
needs to be read again because probe_intel_cpuid_faulting() records
the presence of X86_FEATURE_CPUID_FAULTING but not the presence of msr
itself (if cpuid faulting is not available).

Host policy might have certain features disabled if Xen decides not
to use them. For now, make Host policy equal to Raw policy.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/msr.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index baba44f43d..9737ed706e 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -24,12 +24,34 @@
 #include 
 #include 
 
-struct msr_domain_policy __read_mostly hvm_max_msr_domain_policy,
+struct msr_domain_policy __read_mostly raw_msr_domain_policy,
+ __read_mostlyhost_msr_domain_policy,
+ __read_mostly hvm_max_msr_domain_policy,
  __read_mostly  pv_max_msr_domain_policy;
 
 struct msr_vcpu_policy __read_mostly hvm_max_msr_vcpu_policy,
__read_mostly  pv_max_msr_vcpu_policy;
 
+static void __init calculate_raw_policy(void)
+{
+struct msr_domain_policy *dp = _msr_domain_policy;
+uint64_t val;
+
+if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val) == 0 )
+{
+dp->plaform_info.available = true;
+if ( val & MSR_PLATFORM_INFO_CPUID_FAULTING )
+dp->plaform_info.cpuid_faulting = true;
+}
+}
+
+static void __init calculate_host_policy(void)
+{
+struct msr_domain_policy *dp = _msr_domain_policy;
+
+*dp = raw_msr_domain_policy;
+}
+
 static void __init calculate_hvm_max_policy(void)
 {
 struct msr_domain_policy *dp = _max_msr_domain_policy;
@@ -67,6 +89,8 @@ static void __init calculate_pv_max_policy(void)
 
 void __init init_guest_msr_policy(void)
 {
+calculate_raw_policy();
+calculate_host_policy();
 calculate_hvm_max_policy();
 calculate_pv_max_policy();
 }
-- 
2.11.0


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

[Xen-devel] [PATCH v3 2/6] x86/msr: add VMX MSRs into struct msr_domain_policy

[Sergey Dyasli]

New definitions provide a convenient way of accessing contents of
VMX MSRs: every bit value is accessible by its name and there is a
"raw" 64-bit msr value. Bit names match existing Xen's definitions
as close as possible.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/msr.c|  42 +
 xen/include/asm-x86/msr.h | 414 ++
 2 files changed, 456 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9737ed706e..24029a2ac1 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -216,6 +216,48 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 return X86EMUL_EXCEPTION;
 }
 
+static void __init __maybe_unused build_assertions(void)
+{
+struct msr_domain_policy p;
+
+BUILD_BUG_ON(sizeof(p.vmx_basic.u) !=
+ sizeof(p.vmx_basic.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_pinbased_ctls.u) !=
+ sizeof(p.vmx_pinbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_procbased_ctls.u) !=
+ sizeof(p.vmx_procbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_exit_ctls.u) !=
+ sizeof(p.vmx_exit_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_entry_ctls.u) !=
+ sizeof(p.vmx_entry_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_misc.u) !=
+ sizeof(p.vmx_misc.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr0_fixed0.u) !=
+ sizeof(p.vmx_cr0_fixed0.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr0_fixed1.u) !=
+ sizeof(p.vmx_cr0_fixed1.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr4_fixed0.u) !=
+ sizeof(p.vmx_cr4_fixed0.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_cr4_fixed1.u) !=
+ sizeof(p.vmx_cr4_fixed1.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_vmcs_enum.u) !=
+ sizeof(p.vmx_vmcs_enum.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_procbased_ctls2.u) !=
+ sizeof(p.vmx_procbased_ctls2.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_ept_vpid_cap.u) !=
+ sizeof(p.vmx_ept_vpid_cap.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_pinbased_ctls.u) !=
+ sizeof(p.vmx_true_pinbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_procbased_ctls.u) !=
+ sizeof(p.vmx_true_procbased_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_exit_ctls.u) !=
+ sizeof(p.vmx_true_exit_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_true_entry_ctls.u) !=
+ sizeof(p.vmx_true_entry_ctls.u.raw));
+BUILD_BUG_ON(sizeof(p.vmx_vmfunc.u) !=
+ sizeof(p.vmx_vmfunc.u.raw));
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 751fa25a36..fc99612cca 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -202,6 +202,171 @@ void write_efer(u64 val);
 
 DECLARE_PER_CPU(u32, ler_msr);
 
+union vmx_pin_based_exec_control_bits {
+uint32_t raw;
+struct {
+bool ext_intr_exiting:1;
+uint32_t :2;  /* 1:2 reserved */
+bool  nmi_exiting:1;
+uint32_t :1;  /* 4 reserved */
+bool virtual_nmis:1;
+boolpreempt_timer:1;
+bool posted_interrupt:1;
+uint32_t :24; /* 8:31 reserved */
+};
+};
+
+union vmx_cpu_based_exec_control_bits {
+uint32_t raw;
+struct {
+uint32_t:2;  /* 0:1 reserved */
+boolvirtual_intr_pending:1;
+bool   use_tsc_offseting:1;
+uint32_t:3;  /* 4:6 reserved */
+bool hlt_exiting:1;
+uint32_t:1;  /* 8 reserved */
+bool  invlpg_exiting:1;
+bool   mwait_exiting:1;
+bool   rdpmc_exiting:1;
+bool   rdtsc_exiting:1;
+uint32_t:2;  /* 13:14 reserved */
+boolcr3_load_exiting:1;
+bool   cr3_store_exiting:1;
+uint32_t:2;  /* 17:18 reserved */
+boolcr8_load_exiting:1;
+bool   cr8_store_exiting:1;
+bool  tpr_shadow:1;
+bool virtual_nmi_pending:1;
+bool  mov_dr_exiting:1;
+bool   uncond_io_exiting:1;
+bool  activate_io_bitmap:1;
+uint32_t:1;  /* 26 reserved */
+bool   monitor_trap_flag:1;
+bool activate_msr_bitmap:1;
+bool monitor_exiting:1;
+bool   pause_exiting:1;
+bool activate_secondary_controls:1;
+};
+};
+
+union vmx_vmexit_control_bits {
+uint32_t raw;
+struct {
+uint32_t:2;  /* 0:1 reserved */
+bool   save_debug_cntrls:1;
+uint32_t:6;  /* 3:8 reserved */
+bool  

[Xen-devel] [PATCH v3 5/6] x86/msr: update domain policy on CPUID policy changes

[Sergey Dyasli]

Availability of some MSRs depends on certain CPUID bits. Add function
recalculate_domain_msr_policy() which updates availability of per-domain
MSRs based on current domain's CPUID policy. This function is called
when CPUID policy is changed from a toolstack.

Add recalculate_domain_vmx_msr_policy() which changes availability of
VMX MSRs based on domain's nested virt settings.

Introduce hvm_cr4_domain_valid_bits() which accepts struct domain *
instead of struct vcpu *.

Signed-off-by: Sergey Dyasli 
---
 xen/arch/x86/domctl.c |  1 +
 xen/arch/x86/hvm/hvm.c|  8 +++--
 xen/arch/x86/msr.c| 70 ++-
 xen/include/asm-x86/hvm/hvm.h |  1 +
 xen/include/asm-x86/msr.h |  3 ++
 5 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 80b4df9ec9..334c67d261 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -124,6 +124,7 @@ static int update_domain_cpuid_info(struct domain *d,
 }
 
 recalculate_cpuid_policy(d);
+recalculate_domain_msr_policy(d);
 
 switch ( ctl->input[0] )
 {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb685..7e6b15f8d7 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -928,9 +928,8 @@ const char *hvm_efer_valid(const struct vcpu *v, uint64_t 
value,
 X86_CR0_CD | X86_CR0_PG)))
 
 /* These bits in CR4 can be set by the guest. */
-unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
+unsigned long hvm_cr4_domain_valid_bits(const struct domain *d, bool restore)
 {
-const struct domain *d = v->domain;
 const struct cpuid_policy *p;
 bool mce, vmxe;
 
@@ -963,6 +962,11 @@ unsigned long hvm_cr4_guest_valid_bits(const struct vcpu 
*v, bool restore)
 (p->feat.pku  ? X86_CR4_PKE   : 0));
 }
 
+unsigned long hvm_cr4_guest_valid_bits(const struct vcpu *v, bool restore)
+{
+return hvm_cr4_domain_valid_bits(v->domain, restore);
+}
+
 static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 {
 int vcpuid;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 388f19e50d..a22e3dfaf2 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct msr_domain_policy __read_mostly raw_msr_domain_policy,
  __read_mostlyhost_msr_domain_policy,
@@ -220,7 +221,7 @@ static void __init calculate_hvm_max_vmx_policy(struct 
msr_domain_policy *dp)
 dp->vmx_cr4_fixed1.available = true;
 /*
  * Allowed CR4 bits will be updated during domain creation by
- * hvm_cr4_guest_valid_bits()
+ * hvm_cr4_domain_valid_bits()
  */
 dp->vmx_cr4_fixed1.u.raw = host_msr_domain_policy.vmx_cr4_fixed1.u.raw;
 
@@ -312,6 +313,72 @@ void __init init_guest_msr_policy(void)
 calculate_pv_max_policy();
 }
 
+static void recalculate_domain_vmx_msr_policy(struct domain *d)
+{
+struct msr_domain_policy *dp = d->arch.msr;
+
+if ( !nestedhvm_enabled(d) || !d->arch.cpuid->basic.vmx )
+{
+dp->vmx_basic.available = false;
+dp->vmx_pinbased_ctls.available = false;
+dp->vmx_procbased_ctls.available = false;
+dp->vmx_exit_ctls.available = false;
+dp->vmx_entry_ctls.available = false;
+dp->vmx_misc.available = false;
+dp->vmx_cr0_fixed0.available = false;
+dp->vmx_cr0_fixed1.available = false;
+dp->vmx_cr4_fixed0.available = false;
+dp->vmx_cr4_fixed1.available = false;
+dp->vmx_vmcs_enum.available = false;
+dp->vmx_procbased_ctls2.available = false;
+dp->vmx_ept_vpid_cap.available = false;
+dp->vmx_true_pinbased_ctls.available = false;
+dp->vmx_true_procbased_ctls.available = false;
+dp->vmx_true_exit_ctls.available = false;
+dp->vmx_true_entry_ctls.available = false;
+}
+else
+{
+dp->vmx_basic.available = true;
+dp->vmx_pinbased_ctls.available = true;
+dp->vmx_procbased_ctls.available = true;
+dp->vmx_exit_ctls.available = true;
+dp->vmx_entry_ctls.available = true;
+dp->vmx_misc.available = true;
+dp->vmx_cr0_fixed0.available = true;
+dp->vmx_cr0_fixed1.available = true;
+dp->vmx_cr4_fixed0.available = true;
+dp->vmx_cr4_fixed1.available = true;
+/* Get allowed CR4 bits from CPUID policy */
+dp->vmx_cr4_fixed1.u.raw = hvm_cr4_domain_valid_bits(d, false);
+dp->vmx_vmcs_enum.available = true;
+
+if ( dp->vmx_procbased_ctls.u.allowed_1.activate_secondary_controls )
+{
+dp->vmx_procbased_ctls2.available = true;
+
+if ( dp->vmx_procbased_ctls2.u.allowed_1.enable_ept ||
+ dp->vmx_procbased_ctls2.u.allowed_1.enable_vpid )
+dp->vmx_ept_vpid_cap.available = true;
+}
+
+

[Xen-devel] [linux-4.9 test] 114402: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl  broken  in 114321
 test-armhf-armhf-xl-rtds broken  in 114321
 test-armhf-armhf-libvirt broken  in 114321
 build-armhf-xsm  broken  in 114321
 test-amd64-amd64-xl-pvh-intel 12 guest-start fail REGR. vs. 114036
 test-amd64-amd64-xl-pvh-amd  12 guest-start  fail REGR. vs. 114036
 build-armhf-xsm  5 host-build-prep fail in 114321 REGR. vs. 114036

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-libvirt 4 host-install(4) broken in 114321 pass in 114402
 test-armhf-armhf-xl-rtds 4 host-install(4) broken in 114321 pass in 114402
 test-armhf-armhf-xl  4 host-install(4) broken in 114321 pass in 114402
 test-armhf-armhf-libvirt  6 xen-installfail pass in 114230
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-localmigrate/x10 fail pass in 
114321

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-xsm  1 build-check(1)   blocked in 114321 n/a
 test-armhf-armhf-xl-xsm   1 build-check(1)   blocked in 114321 n/a
 test-armhf-armhf-libvirt13 migrate-support-check fail in 114230 never pass
 test-armhf-armhf-libvirt 14 saverestore-support-check fail in 114230 never pass
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 114036
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 114036
 test-amd64-amd64-xl-rtds 10 debian-install   fail  like 114036
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 10 windows-installfail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail  never pass
 test-amd64-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
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  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-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-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore   fail never pass
 test-amd64-i386-xl-qemut-ws16-amd64 13 guest-saverestore   fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 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-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
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail 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:
 linuxf37eb7b586f1dd24a86c50278c65322fc6787722
baseline version:
 linux1852eae92c460813692808234da35d142a405ab7

Last test of basis   114036  2017-10-05 08:21:13 Z   

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 14:19,  wrote:
> On 13/10/17 13:08, Jan Beulich wrote:
> On 13.10.17 at 12:44,  wrote:
>>> In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:
>>>
 flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
>>> However, xenstore reads this value as a decimal value and tries to map the
>>> wrong address and fails.
>> Is this generic or vuart specific code in xenstore that does so?
>> Could you perhaps simply point me at the consuming side?
>>
>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>> decimal value.
>> I ask because I'm not really happy about this addition, i.e. I'd
>> prefer the read side to change.
> 
> Can the read side realistically change?

Well, that's why I had asked whether this is generic or specific
code. I would have hoped/assumed that xenstore doesn't
generically try to translate strings into numbers - how would it
know a string is to represent a number in the first place? Hence
I was hoping for this to be specific (and hence) new code.

> Its been decimal for a decade now, and there definitely is 3rd party
> code which uses these values in xenstore (sadly).

Are you trying to tell me there's been a vuart frontend before
the device type introduction in libxl, or is the new device type
compatible with an existing one?

> Then again, the ring-ref key is listed as deprecated in our
> documentation, without any reference describing which key should be used
> instead.  It is also typically a grant reference, not a gfn, so
> something wonky is definitely going on here.

Which put under question how an existing frontend could work
with this new device type.

Jan


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

Re: [Xen-devel] [PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Jan Beulich]

>>> On 13.10.17 at 12:44,  wrote:
> On Fri, Oct 13, 2017 at 08:59:29AM +, Jan Beulich wrote:
>> >>> On 13.10.17 at 10:49,  wrote:
>>  On 29.09.17 at 13:25,  wrote:
>> >> nr_pages doesn't take into account holes or MMIO regions, and
>> >> underestimates the amount of memory needed for paging. Be on the safe
>> >> side and use max_pdx instead.
>> >> 
>> >> Note that both cases are just approximations, but using max_pdx yields
>> >> a number of free pages after Dom0 build always greater than the
>> >> minimum reserve (either 1/16 of memory or 128MB, whatever is
>> >> smaller).
>> >> 
>> >> Without this patch on a 16GB box the amount of free memory after
>> >> building Dom0 without specifying any dom0_mem parameter would be
>> >> 122MB, with this patch applied the amount of free memory after Dom0
>> >> build is 144MB, which is greater than the reserved 128MB.
>> > 
>> > For the case of there not being a "dom0_mem=" this may indeed
>> > be acceptable (albeit I notice the gap is larger than before, just
>> > this time in the right direction). For the supposedly much more
>> > common case of there being "dom0_mem=" (and with a positive
>> > value), however, not using nr_pages ...
>> >> @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages(
>> >>  break;
>> >>  
>> >>  /* Reserve memory for shadow or HAP. */
>> >> -avail -= dom0_paging_pages(d, nr_pages);
>> >> +avail -= paging_pgs;
>> > 
>> > ... here is likely going to result in a huge overestimation.
>> 
>> Which I realize may or may not be a problem - the question is
>> whether and if so how far the clamping done by
>> 
>> nr_pages = min(nr_pages, avail);
>> 
>> above here would result in a meaningfully different amount of
>> memory Dom0 may get for certain command line option / total
>> amount of memory combinations. I.e. quite a bit more than a
>> single data point would need to be provided to prove this isn't
>> going to be perceived as a regression by anyone.
> 
> What about using something like max_pdx - total_pages + nr_pages, that
> seems like a good compromise that should take into account the MMIO
> holes without overestimating as much as just using max_pdx.

I'm not sure - I see no clear correlation between max_pdx,
total_pages, and possible large MMIO space needs. In fact,
max_pdx doesn't express MMIO space needs at all, you'd
need to use max_page in that case.

Jan


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

Re: [Xen-devel] [PATCH v6 09/11] vpci/msi: add MSI handlers

[Jan Beulich]

>>> On 13.10.17 at 13:17,  wrote:
> On Tue, Oct 10, 2017 at 11:35:26AM +, Roger Pau Monné wrote:
>> On Wed, Oct 04, 2017 at 08:34:13AM +, Jan Beulich wrote:
>> > >>> On 19.09.17 at 17:29,  wrote:
>> > > +static void vpci_msi_enable(const struct pci_dev *pdev, struct vpci_msi 
>> > > *msi,
>> > > +unsigned int vectors)
>> > > +{
>> > > +int ret;
>> > > +
>> > > +ASSERT(!msi->enabled);
>> > > +ret = vpci_msi_arch_enable(msi, pdev, vectors);
>> > > +if ( ret )
>> > > +return;
>> > > +
>> > > +/* Apply the mask bits. */
>> > > +if ( msi->masking )
>> > > +{
>> > > +unsigned int i;
>> > > +uint32_t mask = msi->mask;
>> > > +
>> > > +for ( i = ffs(mask) - 1; mask && i < vectors; i = ffs(mask) - 1 
>> > > )
>> > > +{
>> > > +vpci_msi_arch_mask(msi, pdev, i, true);
>> > > +__clear_bit(i, );
>> > > +}
>> > > +}
>> > > +
>> > > +__msi_set_enable(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> > > + PCI_FUNC(pdev->devfn), msi->pos, 1);
>> > 
>> > This is very unlikely to be a function that arch-independent code is
>> > permitted to call.
>> 
>> Right, I could remove the '__' prefix, or introduce a
>> vpci_msi_arch_dev_enable helper that calls this function.
> 
> So would using msi_set_enable instead be acceptable?

Not really, no, the more that it's static (and should remain so);
__msi_set_enable() not being static is also just because of an
AMD IOMMU oddity. These are low level functions that higher
layers aren't supposed to call directly.

Jan

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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Andrew Cooper]

On 13/10/17 13:08, Jan Beulich wrote:
 On 13.10.17 at 12:44,  wrote:
>> In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:
>>
>>> flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
>> However, xenstore reads this value as a decimal value and tries to map the
>> wrong address and fails.
> Is this generic or vuart specific code in xenstore that does so?
> Could you perhaps simply point me at the consuming side?
>
>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>> decimal value.
> I ask because I'm not really happy about this addition, i.e. I'd
> prefer the read side to change.

Can the read side realistically change?

Its been decimal for a decade now, and there definitely is 3rd party
code which uses these values in xenstore (sadly).

Then again, the ring-ref key is listed as deprecated in our
documentation, without any reference describing which key should be used
instead.  It is also typically a grant reference, not a gfn, so
something wonky is definitely going on here.

~Andrew

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

Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

[Jan Beulich]

>>> On 13.10.17 at 12:36,  wrote:
> On 13.10.2017 13:29, Jan Beulich wrote:
>>> +__set_bit(index + sizeof(struct monitor_msr_bitmap), bitmap);
>> 
>> I think you miss "* 8" here - a bit position plus sizeof() doesn't
>> produce any useful value.
>> 
>> But what's worse - having read till the end of the patch I don't
>> see you change any allocation, yet you clearly need to double
>> the space now that you need two bits per MSR.
> 
> We did this:
> 
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index e59f1f5..a3046c6 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -25,7 +25,7 @@
>   int arch_monitor_init_domain(struct domain *d)
>   {
>   if ( !d->arch.monitor.msr_bitmap )
> -d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
> +d->arch.monitor.msr_bitmap = xzalloc_array(struct 
> monitor_msr_bitmap, 2);
> 
>   if ( !d->arch.monitor.msr_bitmap )
>   return -ENOMEM;
> @@ -67,7 +67,7 @@ static unsigned long *monitor_bitmap_for_msr(const struct 
> domain *d, u32 *msr)
>   }
>   }
> 
> I.e., we are now allocating an array of size 2 of struct 
> monitor_msr_bitmaps with xzalloc_array().

Oh, I'm not sure how I could overlook this considering that I
specifically looked up the allocation point and searched through
the patch for a respective change. I'm sorry for the noise in
this regard. I do think though that the chosen model is a little
odd and fragile, but that's something you and Tamas as the
maintainers of the code have to judge about.

Jan


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

Re: [Xen-devel] [Qemu-devel] [RFC QEMU PATCH v3 00/10] Implement vNVDIMM for Xen HVM guest

[Jan Beulich]

>>> On 13.10.17 at 13:13,  wrote:
> To Jan, Andrew, Stefano and Anthony,
> 
> what do you think about allowing QEMU to build the entire guest ACPI
> and letting SeaBIOS to load it? ACPI builder code in hvmloader is
> still there and just bypassed in this case.

Well, if that can be made work in a non-quirky way and without
loss of functionality, I'd probably be fine. I do think, however,
that there's a reason this is being handled in hvmloader right now.

Jan


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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 12:44,  wrote:
> In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:
> 
>> flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
> 
> However, xenstore reads this value as a decimal value and tries to map the
> wrong address and fails.

Is this generic or vuart specific code in xenstore that does so?
Could you perhaps simply point me at the consuming side?

> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
> decimal value.

I ask because I'm not really happy about this addition, i.e. I'd
prefer the read side to change.

Jan


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

Re: [Xen-devel] preparations for 4.9.1 and 4.7.4

[Lars Kurth]

Jan,

I had sent the mail an hour after I ran the scripts (had a meeting in between). 

I will look into the issue with XSA 226
On commits c7783d9c26fc191862d9883da22387340b1fab18 & 
d6aad635097d901b96df650e87f04687c9fb7bd2: I have to look into why these didn’t 
get picked up.
Maybe there is a bug in the tool

Lars

On 13/10/2017, 08:13, "Jan Beulich"  wrote:

>>> On 12.10.17 at 19:08,  wrote:
> for 4.9.1 the XSA status is
> 
> XSA 226 : Some patches not applied => check
> There is an extra chunk in the tree: see xsa226.png

I see there are outdated patches for this XSA still in xsa.git,
which I've now removed.

I can't seem to be able to confirm the difference your attached
image indicates - both xen.git and xsa.git have that change. I
could understand the tool spotting a difference in there being a
seemingly extra hunk changing xen/common/compat/grant_table.c
in xsa.git - this change was applied incrementally to the various
xen.git branches, as the issue was found (by osstest iirc) only
after the original patches had already gone in.

> XSA 237 : No patch found => check
> XSA 238 : No patch found => check
> XSA 239 : No patch found => check
> XSA 240 : No patch found => check
> XSA 241 : No patch found => check
> XSA 242 : No patch found => check
> XSA 243 : No patch found => check
> XSA 244 : No patch found => check
> These have only been released today and have not yet been applied

By the time you had sent the message, these had been applied for
several hours. But of course I don't know when you ran the script.

> For 4.7.4 the XSA status is
> 
> XSA 226 : Some patches not applied => check
> There is an extra chunk in the tree: see xsa226.png

Same as for 4.9.

> XSA 234 : No patch found => check
> ISSUE: This is genuinely missing (aka xsa234-4.8.patch)

Commit c7783d9c26fc191862d9883da22387340b1fab18.

> XSA 245 : Some patches not applied => check
> This is missing 
> xsa245/0002-xen-arm-Correctly-report-the-memory-region-in-the-du.patch

Commit d6aad635097d901b96df650e87f04687c9fb7bd2.

Jan



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

Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Wei Liu]

On Fri, Oct 13, 2017 at 04:14:32PM +0530, Bhupinder Thakur wrote:
> In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:
> 
> > flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
> 
> However, xenstore reads this value as a decimal value and tries to map the
> wrong address and fails.
> 
> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
> decimal value.
> 
> Signed-off-by: Bhupinder Thakur 

Acked-by: Wei Liu 

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

[Xen-devel] [PATCH for-4.10] libxl: handle NULL in libxl__enum_from_string

[Wei Liu]

Discovered by Coverity.

Signed-off-by: Wei Liu 
---
Cc: Ian Jackson 
---
 tools/libxl/libxl_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 507ee56c7c..9433693e72 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1017,7 +1017,7 @@ int libxl_get_max_nodes(libxl_ctx *ctx)
 int libxl__enum_from_string(const libxl_enum_string_table *t,
 const char *s, int *e)
 {
-if (!t) return ERROR_INVAL;
+if (!t || !s) return ERROR_INVAL;
 
 for( ; t->s; t++) {
 if (!strcasecmp(t->s, s)) {
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v6 09/11] vpci/msi: add MSI handlers

[Roger Pau Monné]

On Tue, Oct 10, 2017 at 11:35:26AM +, Roger Pau Monné wrote:
> On Wed, Oct 04, 2017 at 08:34:13AM +, Jan Beulich wrote:
> > >>> On 19.09.17 at 17:29,  wrote:
> > > +static void vpci_msi_enable(const struct pci_dev *pdev, struct vpci_msi 
> > > *msi,
> > > +unsigned int vectors)
> > > +{
> > > +int ret;
> > > +
> > > +ASSERT(!msi->enabled);
> > > +ret = vpci_msi_arch_enable(msi, pdev, vectors);
> > > +if ( ret )
> > > +return;
> > > +
> > > +/* Apply the mask bits. */
> > > +if ( msi->masking )
> > > +{
> > > +unsigned int i;
> > > +uint32_t mask = msi->mask;
> > > +
> > > +for ( i = ffs(mask) - 1; mask && i < vectors; i = ffs(mask) - 1 )
> > > +{
> > > +vpci_msi_arch_mask(msi, pdev, i, true);
> > > +__clear_bit(i, );
> > > +}
> > > +}
> > > +
> > > +__msi_set_enable(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> > > + PCI_FUNC(pdev->devfn), msi->pos, 1);
> > 
> > This is very unlikely to be a function that arch-independent code is
> > permitted to call.
> 
> Right, I could remove the '__' prefix, or introduce a
> vpci_msi_arch_dev_enable helper that calls this function.

So would using msi_set_enable instead be acceptable?

Thanks, Roger.

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

Re: [Xen-devel] [Qemu-devel] [RFC QEMU PATCH v3 00/10] Implement vNVDIMM for Xen HVM guest

[Haozhong Zhang]

On 10/13/17 10:44 +0200, Igor Mammedov wrote:
> On Fri, 13 Oct 2017 15:53:26 +0800
> Haozhong Zhang  wrote:
> 
> > On 10/12/17 17:45 +0200, Paolo Bonzini wrote:
> > > On 12/10/2017 14:45, Haozhong Zhang wrote:  
> > > > Basically, QEMU builds two ROMs for guest, /rom@etc/acpi/tables and
> > > > /rom@etc/table-loader. The former is unstructured to guest, and
> > > > contains all data of guest ACPI. The latter is a BIOSLinkerLoader
> > > > organized as a set of commands, which direct the guest (e.g., SeaBIOS
> > > > on KVM/QEMU) to relocate data in the former file, recalculate checksum
> > > > of specified area, and fill guest address in specified ACPI field.
> > > > 
> > > > One part of my patches is to implement a mechanism to tell Xen which
> > > > part of ACPI data is a table (NFIT), and which part defines a
> > > > namespace device and what the device name is. I can add two new loader
> > > > commands for them respectively.
> > > > 
> > > > Because they just provide information and SeaBIOS in non-xen
> > > > environment ignores unrecognized commands, they will not break SeaBIOS
> > > > in non-xen environment.
> > > > 
> > > > On QEMU side, most Xen-specific hacks in ACPI builder could be
> > > > dropped, and replaced by adding the new loader commands (though they
> > > > may be used only by Xen).
> > > > 
> > > > On Xen side, a fw_cfg driver and a BIOSLinkerLoader command executor
> > > > are needed in, perhaps, hvmloader.  
> > > 
> > > If Xen has to parse BIOSLinkerLoader, it can use the existing commands
> > > to process a reduced set of ACPI tables.  In other words,
> > > etc/acpi/tables would only include the NFIT, the SSDT with namespace
> > > devices, and the XSDT.  etc/acpi/rsdp would include the RSDP table as 
> > > usual.
> > >
> > > hvmloader can then:
> > > 
> > > 1) allocate some memory for where the XSDT will go
> > > 
> > > 2) process the BIOSLinkerLoader like SeaBIOS would do
> > > 
> > > 3) find the RSDP in low memory, since the loader script must have placed
> > > it there.  If it cannot find it, allocate some low memory, fill it with
> > > the RSDP header and revision, and and jump to step 6
> > > 
> > > 4) If it found QEMU's RSDP, use it to find QEMU's XSDT
> > > 
> > > 5) Copy ACPI table pointers from QEMU to hvmloader's RSDT and/or XSDT.
> > > 
> > > 6) build hvmloader tables and link them into the RSDT and/or XSDT as 
> > > usual.
> > > 
> > > 7) overwrite the RSDP in low memory with a pointer to hvmloader's own
> > > RSDT and/or XSDT, and updated the checksums
> > > 
> > > QEMU's XSDT remains there somewhere in memory, unused but harmless.
> > >   
> +1 to Paolo's suggestion, i.e.
>  1. add BIOSLinkerLoader into hvmloader
>  2. load/process QEMU's tables with #1
>  3. get pointers to QEMU generated NFIT and NVDIMM SSDT from QEMU's RSDT/XSDT
> and put them in hvmloader's RSDT
> 
> > It can work for plan tables which do not contain AML.
> > 
> > However, for a namespace device, Xen needs to know its name in order
> > to detect the potential name conflict with those used in Xen built
> > ACPI. Xen does not (and is not going to) introduce an AML parser, so
> > it cannot get those device names from QEMU built ACPI by its own.
> > 
> > The idea of either this patch series or the new BIOSLinkerLoader
> > command is to let QEMU tell Xen where the definition body of a
> > namespace device (i.e. that part within the outmost "Device(NAME)") is
> > and what the device name is. Xen, after the name conflict check, can
> > re-package the definition body in a namespace device (w/ minimal AML
> > builder code added in Xen) and then in SSDT.
> 
> I'd skip conflict check at runtime as hvmloader doesn't currently
> have "\\_SB\NVDR" device so instead of doing runtime check it might
> do primitive check at build time that ASL sources in hvmloader do
> not contain reserved for QEMU "NVDR" keyword to avoid its addition
> by accident in future. (it also might be reused in future if some
> other tables from QEMU will be reused).
> It's a bit hackinsh but at least it does the job and keeps
> BIOSLinkerLoader interface the same for all supported firmwares
> (I'd consider it as a temporary hack on the way to fully build
> by QEMU ACPI tables for Xen).
> 
> Ideally it would be better for QEMU to build all ACPI tables for
> hvmloader to avoid split brain issues and need to invent extra
> interfaces every time a feature is added to pass configuration
> data from QEMU to firmware.
> But that's probably out of scope of this project, it could be
> done on top of this if Xen folks would like to do it. Adding
> BIOSLinkerLoader to hvmloader would be a good starting point
> for that future effort.

If we can let QEMU build the entire guest ACPI, we may even not need
to introduce fw_cfg and BIOSLinkerLoader code to hvmloader.  SeaBIOS
is currently loaded after hvmloader and can be used to load QEMU built
ACPI.

To Jan, Andrew, Stefano and Anthony,

what do you think about allowing QEMU 

[Xen-devel] [PATCH v1 1/2] tools/libs/evtchn: Add support for restricting a handle

[Ross Lagerwall]

Implement support for restricting evtchn handles to a particular domain
on Linux by calling the IOCTL_EVTCHN_RESTRICT_DOMID ioctl (support added
in Linux v4.8).

Signed-off-by: Ross Lagerwall 
---
 tools/include/xen-sys/Linux/evtchn.h  | 15 +++
 tools/libs/evtchn/Makefile|  2 +-
 tools/libs/evtchn/core.c  |  5 +
 tools/libs/evtchn/freebsd.c   |  6 ++
 tools/libs/evtchn/include/xenevtchn.h | 10 ++
 tools/libs/evtchn/libxenevtchn.map|  4 
 tools/libs/evtchn/linux.c |  9 +
 tools/libs/evtchn/minios.c|  6 ++
 tools/libs/evtchn/netbsd.c|  6 ++
 tools/libs/evtchn/private.h   |  3 +++
 tools/libs/evtchn/solaris.c   |  6 ++
 tools/libvchan/init.c |  1 +
 tools/libvchan/libxenvchan.h  |  1 +
 13 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/tools/include/xen-sys/Linux/evtchn.h 
b/tools/include/xen-sys/Linux/evtchn.h
index 938d4da..08ee0b7 100644
--- a/tools/include/xen-sys/Linux/evtchn.h
+++ b/tools/include/xen-sys/Linux/evtchn.h
@@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
 #define IOCTL_EVTCHN_RESET \
_IOC(_IOC_NONE, 'E', 5, 0)
 
+/*
+ * Restrict this file descriptor so that it can only be used to bind
+ * new interdomain events from one domain.
+ *
+ * Once a file descriptor has been restricted it cannot be
+ * de-restricted, and must be closed and re-opened.  Event channels
+ * which were bound before restricting remain bound afterwards, and
+ * can be notified as usual.
+ */
+#define IOCTL_EVTCHN_RESTRICT_DOMID\
+   _IOC(_IOC_NONE, 'E', 6, sizeof(struct ioctl_evtchn_restrict_domid))
+struct ioctl_evtchn_restrict_domid {
+   domid_t domid;
+};
+
 #endif /* __LINUX_PUBLIC_EVTCHN_H__ */
diff --git a/tools/libs/evtchn/Makefile b/tools/libs/evtchn/Makefile
index 5444ec7..bc98aed 100644
--- a/tools/libs/evtchn/Makefile
+++ b/tools/libs/evtchn/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 MAJOR= 1
-MINOR= 0
+MINOR= 1
 SHLIB_LDFLAGS += -Wl,--version-script=libxenevtchn.map
 
 CFLAGS   += -Werror -Wmissing-prototypes
diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c
index c31e08c..41621ff 100644
--- a/tools/libs/evtchn/core.c
+++ b/tools/libs/evtchn/core.c
@@ -61,6 +61,11 @@ int xenevtchn_close(xenevtchn_handle *xce)
 return rc;
 }
 
+int xenevtchn_restrict(xenevtchn_handle *xce, domid_t domid)
+{
+return osdep_evtchn_restrict(xce, domid);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/evtchn/freebsd.c b/tools/libs/evtchn/freebsd.c
index 30eaa70..ba82f06 100644
--- a/tools/libs/evtchn/freebsd.c
+++ b/tools/libs/evtchn/freebsd.c
@@ -47,6 +47,12 @@ int osdep_evtchn_close(xenevtchn_handle *xce)
 return close(xce->fd);
 }
 
+int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid)
+{
+errno = -EOPNOTSUPP;
+return -1;
+}
+
 int xenevtchn_fd(xenevtchn_handle *xce)
 {
 return xce->fd;
diff --git a/tools/libs/evtchn/include/xenevtchn.h 
b/tools/libs/evtchn/include/xenevtchn.h
index 93b80cb..91821ee 100644
--- a/tools/libs/evtchn/include/xenevtchn.h
+++ b/tools/libs/evtchn/include/xenevtchn.h
@@ -151,6 +151,16 @@ xenevtchn_pending(xenevtchn_handle *xce);
  */
 int xenevtchn_unmask(xenevtchn_handle *xce, evtchn_port_t port);
 
+/**
+ * This function restricts the use of this handle to the specified
+ * domain.
+ *
+ * @parm xce handle to the open evtchn interface
+ * @parm domid the domain id
+ * @return 0 on success, -1 on failure with errno set appropriately.
+ */
+int xenevtchn_restrict(xenevtchn_handle *xce, domid_t domid);
+
 #endif
 
 /*
diff --git a/tools/libs/evtchn/libxenevtchn.map 
b/tools/libs/evtchn/libxenevtchn.map
index 625a1e2..33a38f9 100644
--- a/tools/libs/evtchn/libxenevtchn.map
+++ b/tools/libs/evtchn/libxenevtchn.map
@@ -17,3 +17,7 @@ VERS_1.0 {
xenevtchn_pending;
local: *; /* Do not expose anything by default */
 };
+VERS_1.1 {
+   global:
+   xenevtchn_restrict;
+} VERS_1.0;
diff --git a/tools/libs/evtchn/linux.c b/tools/libs/evtchn/linux.c
index a581c5d..17e64ae 100644
--- a/tools/libs/evtchn/linux.c
+++ b/tools/libs/evtchn/linux.c
@@ -21,9 +21,11 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
@@ -49,6 +51,13 @@ int osdep_evtchn_close(xenevtchn_handle *xce)
 return close(xce->fd);
 }
 
+int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid)
+{
+struct ioctl_evtchn_restrict_domid restrict_domid = { domid };
+
+return ioctl(xce->fd, IOCTL_EVTCHN_RESTRICT_DOMID, _domid);
+}
+
 int xenevtchn_fd(xenevtchn_handle *xce)
 {
 return xce->fd;
diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
index ccf37f0..414c21b 100644
--- a/tools/libs/evtchn/minios.c
+++ 

[Xen-devel] [PATCH v2 2/2] xentoolcore_restrict_all: Implement for libxenevtchn

[Ross Lagerwall]

Signed-off-by: Ross Lagerwall 
---
 tools/Rules.mk|  2 +-
 tools/libs/evtchn/Makefile|  4 ++--
 tools/libs/evtchn/core.c  | 13 +
 tools/libs/evtchn/private.h   |  3 +++
 tools/libs/toolcore/include/xentoolcore.h |  5 -
 5 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index be92f0a..61515d3 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -109,7 +109,7 @@ LDLIBS_libxentoolcore = $(SHDEPS_libxentoolcore) 
$(XEN_LIBXENTOOLCORE)/libxentoo
 SHLIB_libxentoolcore  = $(SHDEPS_libxentoolcore) 
-Wl,-rpath-link=$(XEN_LIBXENTOOLCORE)
 
 CFLAGS_libxenevtchn = -I$(XEN_LIBXENEVTCHN)/include $(CFLAGS_xeninclude)
-SHDEPS_libxenevtchn =
+SHDEPS_libxenevtchn = $(SHLIB_libxentoolcore)
 LDLIBS_libxenevtchn = $(SHDEPS_libxenevtchn) 
$(XEN_LIBXENEVTCHN)/libxenevtchn$(libextension)
 SHLIB_libxenevtchn  = $(SHDEPS_libxenevtchn) 
-Wl,-rpath-link=$(XEN_LIBXENEVTCHN)
 
diff --git a/tools/libs/evtchn/Makefile b/tools/libs/evtchn/Makefile
index bc98aed..9952b30 100644
--- a/tools/libs/evtchn/Makefile
+++ b/tools/libs/evtchn/Makefile
@@ -7,7 +7,7 @@ SHLIB_LDFLAGS += -Wl,--version-script=libxenevtchn.map
 
 CFLAGS   += -Werror -Wmissing-prototypes
 CFLAGS   += -I./include $(CFLAGS_xeninclude)
-CFLAGS   += $(CFLAGS_libxentoollog)
+CFLAGS   += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore)
 
 SRCS-y += core.c
 SRCS-$(CONFIG_Linux)   += linux.c
@@ -61,7 +61,7 @@ libxenevtchn.so.$(MAJOR): libxenevtchn.so.$(MAJOR).$(MINOR)
$(SYMLINK_SHLIB) $< $@
 
 libxenevtchn.so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxenevtchn.map
-   $(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenevtchn.so.$(MAJOR) 
$(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(APPEND_LDFLAGS)
+   $(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenevtchn.so.$(MAJOR) 
$(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) 
$(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: build
diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c
index 41621ff..14b7549 100644
--- a/tools/libs/evtchn/core.c
+++ b/tools/libs/evtchn/core.c
@@ -18,6 +18,16 @@
 
 #include "private.h"
 
+static int all_restrict_cb(Xentoolcore__Active_Handle *ah, domid_t domid) {
+xenevtchn_handle *xce = CONTAINER_OF(ah, *xce, tc_ah);
+
+if (xce->fd < 0)
+/* just in case */
+return 0;
+
+return xenevtchn_restrict(xce, domid);
+}
+
 xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned 
open_flags)
 {
 xenevtchn_handle *xce = malloc(sizeof(*xce));
@@ -29,6 +39,9 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, 
unsigned open_flags)
 xce->logger = logger;
 xce->logger_tofree  = NULL;
 
+xce->tc_ah.restrict_callback = all_restrict_cb;
+xentoolcore__register_active_handle(>tc_ah);
+
 if (!xce->logger) {
 xce->logger = xce->logger_tofree =
 (xentoollog_logger*)
diff --git a/tools/libs/evtchn/private.h b/tools/libs/evtchn/private.h
index 3d34862..31e595b 100644
--- a/tools/libs/evtchn/private.h
+++ b/tools/libs/evtchn/private.h
@@ -4,11 +4,14 @@
 #include 
 #include 
 
+#include 
+
 #include 
 
 struct xenevtchn_handle {
 xentoollog_logger *logger, *logger_tofree;
 int fd;
+Xentoolcore__Active_Handle tc_ah;
 };
 
 int osdep_evtchn_open(xenevtchn_handle *xce);
diff --git a/tools/libs/toolcore/include/xentoolcore.h 
b/tools/libs/toolcore/include/xentoolcore.h
index be6c570..ef9c670 100644
--- a/tools/libs/toolcore/include/xentoolcore.h
+++ b/tools/libs/toolcore/include/xentoolcore.h
@@ -31,11 +31,6 @@
  * Arranges that Xen library handles (fds etc.) which are currently held
  * by Xen libraries, can no longer be used other than to affect domid.
  *
- * Does not prevent effects that amount only to
- *   - denial of service, possibly host-wide, by resource exhaustion etc.
- *   - leak of not-very-interesting metainformation about other domains
- * eg, specifically, event channel signals relating to other domains
- *
  * If this cannot be achieved, returns -1 and sets errno.
  * If called again with the same domid, it may succeed, or it may
  * fail (even though such a call is potentially meaningful).
-- 
2.9.5


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

Re: [Xen-devel] [PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Roger Pau Monné]

On Fri, Oct 13, 2017 at 08:59:29AM +, Jan Beulich wrote:
> >>> On 13.10.17 at 10:49,  wrote:
>  On 29.09.17 at 13:25,  wrote:
> >> nr_pages doesn't take into account holes or MMIO regions, and
> >> underestimates the amount of memory needed for paging. Be on the safe
> >> side and use max_pdx instead.
> >> 
> >> Note that both cases are just approximations, but using max_pdx yields
> >> a number of free pages after Dom0 build always greater than the
> >> minimum reserve (either 1/16 of memory or 128MB, whatever is
> >> smaller).
> >> 
> >> Without this patch on a 16GB box the amount of free memory after
> >> building Dom0 without specifying any dom0_mem parameter would be
> >> 122MB, with this patch applied the amount of free memory after Dom0
> >> build is 144MB, which is greater than the reserved 128MB.
> > 
> > For the case of there not being a "dom0_mem=" this may indeed
> > be acceptable (albeit I notice the gap is larger than before, just
> > this time in the right direction). For the supposedly much more
> > common case of there being "dom0_mem=" (and with a positive
> > value), however, not using nr_pages ...
> >> @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages(
> >>  break;
> >>  
> >>  /* Reserve memory for shadow or HAP. */
> >> -avail -= dom0_paging_pages(d, nr_pages);
> >> +avail -= paging_pgs;
> > 
> > ... here is likely going to result in a huge overestimation.
> 
> Which I realize may or may not be a problem - the question is
> whether and if so how far the clamping done by
> 
> nr_pages = min(nr_pages, avail);
> 
> above here would result in a meaningfully different amount of
> memory Dom0 may get for certain command line option / total
> amount of memory combinations. I.e. quite a bit more than a
> single data point would need to be provided to prove this isn't
> going to be perceived as a regression by anyone.

What about using something like max_pdx - total_pages + nr_pages, that
seems like a good compromise that should take into account the MMIO
holes without overestimating as much as just using max_pdx.

Thanks, Roger.

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

[Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Bhupinder Thakur]

In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:

> flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));

However, xenstore reads this value as a decimal value and tries to map the
wrong address and fails.

Introduced a new format string "PRIu_xen_pfn" which formats the value as a
decimal value.

Signed-off-by: Bhupinder Thakur 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Jan Beulich 
CC: Andrew Cooper 

 tools/libxl/libxl_console.c   | 2 +-
 xen/include/public/arch-arm.h | 1 +
 xen/include/public/arch-x86/xen.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index c05dc28..6bfc0e5 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -376,7 +376,7 @@ int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid,
 flexarray_append(ro_front, "port");
 flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
 flexarray_append(ro_front, "ring-ref");
-flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
+flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->vuart_gfn));
 flexarray_append(ro_front, "limit");
 flexarray_append(ro_front, GCSPRINTF("%d", LIBXL_XENCONSOLE_LIMIT));
 flexarray_append(ro_front, "type");
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 5708cd2..05fd11c 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -274,6 +274,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
 
 typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn PRIx64
+#define PRIu_xen_pfn PRIu64
 
 /* Maximum number of virtual CPUs in legacy multi-processor guests. */
 /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
diff --git a/xen/include/public/arch-x86/xen.h 
b/xen/include/public/arch-x86/xen.h
index ff91831..3b0b1d6 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -75,6 +75,7 @@ __DeFiNe__ __DECL_REG_LO16(name) e ## name
 #ifndef __ASSEMBLY__
 typedef unsigned long xen_pfn_t;
 #define PRI_xen_pfn "lx"
+#define PRIu_xen_pfn "lu"
 #endif
 
 #define XEN_HAVE_PV_GUEST_ENTRY 1
-- 
2.7.4


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

Re: [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until 
just before os_setup_post"):
> This works for normally starting a VM but doesn't seem to work when 
> resuming/migrating.
> 
> Here is the order of selected operations when starting a VM normally:
>  VNC server running on 127.0.0.1:5901
>  xen_change_state_handler()
>  xenstore_record_dm_state: running
>  xen_setup_post()
>  xentoolcore_restrict_all: rc = 0
>  os_setup_post()
>  main_loop()
> 
> Here is the order of selected operations when starting QEMU with 
> -incoming fd:... :
>  VNC server running on 127.0.0.1:5902
>  migration_fd_incoming()
>  xen_setup_post()
>  xentoolcore_restrict_all: rc = 0
>  os_setup_post()
>  main_loop()
>  migration_set_incoming_channel()
>  migrate_set_state()
>  xen_change_state_handler()
>  xenstore_record_dm_state: running
>  error recording dm state
>  qemu exited with code 1
> 
> The issue is that QEMU needs xenstore access to write "running" but this 
> is after it has already been restricted. Moving xen_setup_post() into 
> xen_change_state_handler() works fine. The only issue is that in the 
> migration case, it executes after os_setup_post() so QEMU might be 
> chrooted in which case opening /dev/null to restrict fds doesn't work 
> (unless its new root has a /dev/null).

Thanks for the extensive diagnosis.

We do in fact want the restriction to occur before the migration
stream is read.  This is because we are trying to defend against a
guest which can exploit a bug in qemu.  That means that the sending
qemu must be assumed to be compromised.  In this context I don't think
qemu's migration stream receiver can be regarded as hardened against
hostile input; it's far too complicated, even if efforts have been
made in that direction (I haven't checked).  So to avoid the receiving
qemu being compromised while still unrestricted, we should restrict
before starting to read the migration stream.

The correct fix is to use a different technique to notify the
toolstack that qemu is up and running.  That obviously requires
changes on the Xen tools side.  I will look into that for the Xen 4.11
release cycle.

Ian.

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

[Xen-devel] [PATCH 27/27 v12] arm/xen: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt

[Bhupinder Thakur]

This patch fixes the issue observed when pl011 patches were tested on
the junos hardware by Andre/Julien. It was observed that when large
output is generated such as on running 'find /', output was getting
truncated intermittently due to OUT ring buffer getting full.

This issue was due to the fact that the SBSA UART driver expects that
when a TX interrupt is asserted then the FIFO queue should be atleast
half empty and that it can write N bytes in the FIFO, where N is half
the FIFO queue size, without the bytes getting dropped due to FIFO
getting full.

The SBSA UART emulation logic was asserting the TX interrupt as soon
as any space became available in the FIFO and the SBSA UART driver
tried to write more data (upto 16 bytes) in the FIFO expecting that
there is enough space available leading to dropped bytes.

The SBSA spec [1] does not specify when the TX interrupt should be
asserted or de-asserted. Due to lack of clarity on the expected
behavior, it is assumed for now that TX interrupt should be asserted
only when the FIFO goes half empty.

TBD: Once the SBSA spec is updated with the expected behavior, the
implementation will be modified to align with the spec requirement.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183f/DDI0183.pdf

Signed-off-by: Bhupinder Thakur 
---
CC: Julien Grall 
CC: Andre Przywara 
CC: Stefano Stabellini 
CC: Dave Martin 

Changes since v11:
- Add a build assert to check that ring buffer size is more than minimum rx fif 
size of 32
- Added a comment to explain why threshold based logic is not implemented for 
rx fifo
- Moved calls to vpl011_update_interrupt_status() near where TXI/RXI status bit 
is set
 
Changes since v8:
- Used variables fifo_level/fifo_threshold for more clarity
- Added a new macro SBSA_UART_FIFO_SIZE instead of using a magic number
- Renamed ring_qsize variables to fifo_level for consistency 

 xen/arch/arm/vpl011.c| 113 ++-
 xen/include/asm-arm/vpl011.h |   2 +
 2 files changed, 82 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 0b07436..adf1711 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -93,24 +93,27 @@ static uint8_t vpl011_read_data(struct domain *d)
  */
 if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) > 0 )
 {
+unsigned int fifo_level;
+
 data = intf->in[xencons_mask(in_cons, sizeof(intf->in))];
 in_cons += 1;
 smp_mb();
 intf->in_cons = in_cons;
+
+fifo_level = xencons_queued(in_prod, in_cons, sizeof(intf->in));
+
+if ( fifo_level == 0 )
+{
+vpl011->uartfr |= RXFE;
+vpl011->uartris &= ~RXI;
+vpl011_update_interrupt_status(d);
+}
 }
 else
 gprintk(XENLOG_ERR, "vpl011: Unexpected IN ring buffer empty\n");
 
-if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == 0 )
-{
-vpl011->uartfr |= RXFE;
-vpl011->uartris &= ~RXI;
-}
-
 vpl011->uartfr &= ~RXFF;
 
-vpl011_update_interrupt_status(d);
-
 VPL011_UNLOCK(d, flags);
 
 /*
@@ -122,6 +125,26 @@ static uint8_t vpl011_read_data(struct domain *d)
 return data;
 }
 
+static void vpl011_update_tx_fifo_status(struct vpl011 *vpl011,
+ unsigned int fifo_level)
+{
+struct xencons_interface *intf = vpl011->ring_buf;
+unsigned int fifo_threshold;
+
+BUILD_BUG_ON(sizeof (intf->out) < SBSA_UART_FIFO_SIZE);
+
+/*
+ * Set the TXI bit only when there is space for fifo_size/2 bytes which
+ * is the trigger level for asserting/de-assterting the TX interrupt.
+ */
+fifo_threshold = sizeof(intf->out) - SBSA_UART_FIFO_SIZE/2;
+
+if ( fifo_level <= fifo_threshold )
+vpl011->uartris |= TXI;
+else
+vpl011->uartris &= ~TXI;
+}
+
 static void vpl011_write_data(struct domain *d, uint8_t data)
 {
 unsigned long flags;
@@ -146,33 +169,37 @@ static void vpl011_write_data(struct domain *d, uint8_t 
data)
 if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) !=
  sizeof (intf->out) )
 {
+unsigned int fifo_level;
+
 intf->out[xencons_mask(out_prod, sizeof(intf->out))] = data;
 out_prod += 1;
 smp_wmb();
 intf->out_prod = out_prod;
-}
-else
-gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n");
 
-if ( xencons_queued(out_prod, out_cons, sizeof(intf->out)) ==
- sizeof (intf->out) )
-{
-vpl011->uartfr |= TXFF;
-vpl011->uartris &= ~TXI;
+fifo_level = xencons_queued(out_prod, out_cons, sizeof(intf->out));
 
-/*
- * This bit is set only when FIFO becomes full. This ensures that
- * the SBSA UART driver can write the early console data as fast as
-  

[Xen-devel] [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Bhupinder Thakur]

The early console output uses pl011_early_write() to write data. This
function waits for BUSY bit to get cleared before writing the next byte.

In the SBSA UART emulation logic, the BUSY bit was set as soon one
byte was written in the FIFO and it remained set until the FIFO was
emptied. This meant that the output was delayed as each character needed
the BUSY to get cleared.

Since the SBSA UART is getting emulated in Xen using ring buffers, it
ensures that once the data is enqueued in the FIFO, it will be received
by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
full. This will ensure that pl011_early_write() is not delayed unduly
to write the data.

Signed-off-by: Bhupinder Thakur 
---
CC: Julien Grall 
CC: Andre Przywara 
CC: Stefano Stabellini 

 xen/arch/arm/vpl011.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index f7ddccb..0b07436 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, uint8_t 
data)
 {
 vpl011->uartfr |= TXFF;
 vpl011->uartris &= ~TXI;
-}
 
-vpl011->uartfr |= BUSY;
+/*
+ * This bit is set only when FIFO becomes full. This ensures that
+ * the SBSA UART driver can write the early console data as fast as
+ * possible, without waiting for the BUSY bit to get cleared before
+ * writing each byte.
+ */
+vpl011->uartfr |= BUSY;
+}
 
 vpl011->uartfr &= ~TXFE;
 
@@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
 {
 vpl011->uartfr &= ~TXFF;
 vpl011->uartris |= TXI;
+
+/*
+ * Clear the BUSY bit as soon as space becomes available
+ * so that the SBSA UART driver can start writing more data
+ * without any further delay.
+ */
+vpl011->uartfr &= ~BUSY;
+
 if ( out_ring_qsize == 0 )
-{
-vpl011->uartfr &= ~BUSY;
 vpl011->uartfr |= TXFE;
-}
 }
 
 vpl011_update_interrupt_status(d);
-- 
2.7.4


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

Re: [Xen-devel] [PATCH v7 05/16] x86: implement data structure and CPU init flow for MBA

[Jan Beulich]

>>> On 13.10.17 at 10:40,  wrote:
> @@ -319,11 +340,13 @@ static bool cat_init_feature(const struct cpuid_leaf 
> *regs,
>  feat->cos_max = (feat->cos_max - 1) >> 1;
>  
>  /* We reserve cos=0 as default cbm (all bits within cbm_len are 1). 
> */
> -get_cdp_code(feat, 0) = cat_default_val(feat->cbm_len);
> -get_cdp_data(feat, 0) = cat_default_val(feat->cbm_len);
> +get_cdp_code(feat, 0) = cat_default_val(feat->cat.cbm_len);
> +get_cdp_data(feat, 0) = cat_default_val(feat->cat.cbm_len);
>  
> -wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> -wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len));
> +wrmsrl(MSR_IA32_PSR_L3_MASK(0),
> +   cat_default_val(feat->cat.cbm_len));
> +wrmsrl(MSR_IA32_PSR_L3_MASK(1),
> +   cat_default_val(feat->cat.cbm_len));

No need to split lines here afaics.

> +static bool mba_init_feature(const struct cpuid_leaf *regs,
> +struct feat_node *feat,
> +struct psr_socket_info *info,
> +enum psr_feat_type type)
> +{
> +/* No valid value so do not enable feature. */
> +if ( !regs->a || !regs->d || type != FEAT_TYPE_MBA )
> +return false;
> +
> +feat->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);

Even if this and ...

> +if ( feat->cos_max < 1 )
> +return false;
> +
> +feat->mba.thrtl_max = (regs->a & MBA_THRTL_MAX_MASK) + 1;

... this mask don't strictly require it (as they're starting at bit 0),
please use MASK_EXTR() in such cases.

With both taken care of 
Reviewed-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH v4 11/12] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

On 10/12/2017 04:38 PM, Jan Beulich wrote:
 On 11.10.17 at 19:52,  wrote:
>> The Intel manual claims that, "If [certain CPUID bits] are set, the
>> processor deprecates FCS and FDS, and the field is saved as h";
>> but experimentally it would be more accurate to say, "the field is
>> occasionally saved as h".  This causes the --rerun checking to
>> trip non-deterministically.  Sanitize them to zero.
> 
> I think we've meanwhile settled on the field being saved as zero
> being a side effect of using 32-bit fxsave plus a context switch in
> the OS kernel.
> 
>> @@ -594,6 +595,75 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>  };
>>  #undef SET
>>  
>> +/*
>> + * This funciton will read or write fxsave to the fpu.  When writing,
>> + * it 'sanitizes' the state: It will mask off the appropriate bits in
>> + * the mxcsr, 'restore' the state to the fpu, then 'save' it again so
>> + * that the data in fxsave reflects what's actually in the FPU.
>> + *
>> + * TODO: Extend state beyond just FPU (ymm registers, )
>> + */
>> +static void _set_fpu_state(char *fxsave, bool write)
>> +{
>> +if ( cpu_has_fxsr )
>> +{
>> +static union __attribute__((__aligned__(16))) {
>> +char x[512];
>> +struct {
>> +uint16_t cw, sw;
>> +uint8_t  tw, _rsvd1;
>> +uint16_t op;
>> +uint32_t ip;
>> +uint16_t cs, _rsvd2;
>> +uint32_t dp;
>> +uint16_t ds, _rsvd3;
>> +uint32_t mxcsr;
>> +uint32_t mxcsr_mask;
>> +/* ... */
>> +};
>> +} *fxs;
>> +
>> +fxs = (typeof(fxs))fxsave;
>> +
>> +if ( write )
>> +{
>> +/* 
>> + * Clear reserved bits to make sure we don't get any
>> + * exceptions
>> + */
>> +fxs->mxcsr &= mxcsr_mask;
>> +
>> +/*
>> + * The Intel manual says that on newer models CS/DS are
>> + * deprecated and that these fields "are saved as h".
>> + * Experimentally, however, at least on my test box,
>> + * whether this saved as h or as the previously
>> + * written value is random; meaning that when run with
>> + * --rerun, we occasionally detect a "state mismatch" in these
>> + * bytes.  Instead, simply sanitize them to zero.
>> + *
>> + * TODO Check CPUID as specified in the manual before
>> + * clearing
>> + */
>> +fxs->cs = fxs->ds = 0;
> 
> Shouldn't be needed anymore with ...
> 
>> +asm volatile( "fxrstor %0" :: "m" (*fxs) );
> 
> rex64 (or fxrstor64) used here and ...
> 
>> +}
>> +
>> +asm volatile( "fxsave %0" : "=m" (*fxs) );
> 
> ... here (of course the alternative here then is fxsave64).
> 
> Also please add blanks before the opening parentheses.
> 
>> @@ -732,6 +806,18 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>>  printf("Setting cpu_user_regs offset %x\n", offset);
>>  continue;
>>  }
>> +offset -= sizeof(struct cpu_user_regs);
>> +
>> +/* Fuzz fxsave state */
>> +if ( offset < sizeof(s->fxsave) / 4 )
> 
> You've switched to sizeof() here but ...
> 
>> +{
>> +/* 32-bit size is arbitrary; see comment above */
>> +if ( !input_read(s, s->fxsave + (offset * 4), 4) )
>> +return;
>> +printf("Setting fxsave offset %x\n", offset * 4);
>> +continue;
>> +}
>> +offset -= 128;
> 
> ... not here.
> 
>> @@ -1008,6 +1098,16 @@ static void compare_states(struct fuzz_state state[2])
>>  if ( memcmp([0].ops, [1].ops, sizeof(state[0].ops)) )
>>  printf("ops differ!\n");
>>  
>> +if ( memcmp([0].fxsave, [1].fxsave, 
>> sizeof(state[0].fxsave)) )
>> +{
>> +printf("fxsave differs!\n");
>> +for ( i = 0;  i < sizeof(state[0].fxsave)/sizeof(unsigned); i++ 
>> )
> 
> Blanks around / again please.
> 
>> +{
>> +printf("[%04lu] %08x %08x\n",
> 
> I think I've indicated before that I consider leading zeros on decimal
> numbers misleading. 

Come to think of it I agree with you.

> Could I talk you into using %4lu instead (or
> really %4zu, considering the expression type) in places like this one
> (i.e. also in the earlier patch, where I notice only now the l -> z
> conversion wasn't done consistently either)?

/me looks up what %zu is supposed to do

Sure.

> 
>> +i * sizeof(unsigned), ((unsigned 
>> *)[0].fxsave)[i], ((unsigned *)[1].fxsave)[i]);
> 
> Long line.

Ack.

 -George

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

Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

[Razvan Cojocaru]

On 13.10.2017 13:29, Jan Beulich wrote:

+__set_bit(index + sizeof(struct monitor_msr_bitmap), bitmap);


I think you miss "* 8" here - a bit position plus sizeof() doesn't
produce any useful value.

But what's worse - having read till the end of the patch I don't
see you change any allocation, yet you clearly need to double
the space now that you need two bits per MSR.


We did this:

diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index e59f1f5..a3046c6 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -25,7 +25,7 @@
 int arch_monitor_init_domain(struct domain *d)
 {
 if ( !d->arch.monitor.msr_bitmap )
-d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
+d->arch.monitor.msr_bitmap = xzalloc_array(struct 
monitor_msr_bitmap, 2);


 if ( !d->arch.monitor.msr_bitmap )
 return -ENOMEM;
@@ -67,7 +67,7 @@ static unsigned long *monitor_bitmap_for_msr(const 
struct domain *d, u32 *msr)

 }
 }

I.e., we are now allocating an array of size 2 of struct 
monitor_msr_bitmaps with xzalloc_array().



Thanks,
Razvan

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

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

On 10/13/2017 11:31 AM, Jan Beulich wrote:
 On 13.10.17 at 12:23,  wrote:
>> On 10/13/2017 10:20 AM, Jan Beulich wrote:
>> On 13.10.17 at 11:10,  wrote:
 On 10/13/2017 10:06 AM, Jan Beulich wrote:
 On 13.10.17 at 11:00,  wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> @@ -99,13 +99,17 @@ int main(int argc, char **argv)
>>  exit(-1);
>>  }
>>  
>> -if ( !feof(fp) )
>> +/* Only run the test if the input file was smaller than 
>> INPUT_SIZE */
>> +if ( feof(fp) )
>> +{
>> +LLVMFuzzerTestOneInput(input, size);
>> +}
>
> ... ideally with the unnecessary braces dropped here
> Reviewed-by: Jan Beulich 

 Do you really want this to look like this?

 if ( ... )
foo();
 else
 {
...
 }
>>>
>>> Yes. It's Linux and qemu who dislike non-matched if/else bodies,
>>> but our ./CODING_STYLE only says
>>>
>>> "Braces should be omitted for blocks with a single statement. e.g.,
>>>  
>>>  if ( condition )
>>>  single_statement();"
>>>
>>> and personally I'm happy that it doesn't say anything more.
>>
>> Hmm, I personally think it's ugly enough that I'd rather restructure the
>> code to avoid it looking like that. :-)
>>
>> I'll see what I can do.
> 
> Well, assuming you would think that way I've intentionally said
> "ideally", i.e. if you really don't want to change it, I can live with
> the braces.

OK, thanks. :-)

 -George

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

Re: [Xen-devel] [PATCH] libxl: dm_restrict: DEFINE_USERLOOKUP_HELPER returned a pointer to an auto

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] libxl: dm_restrict: DEFINE_USERLOOKUP_HELPER 
returned a pointer to an auto"):
> On Fri, Oct 13, 2017 at 11:25:59AM +0100, Ian Jackson wrote:
> > When I converted the previous open-coded user lookup functionality
> > into DEFINE_USERLOOKUP_HELPER, I moved the struct passwd buffer into
> > the function generated by the macro.  This is wrong because that
> > buffer is used by get{pw,gr}* for its return value, so the helper
> > function would contrive to return a pointer to the buffer on its own
> > stack.
> > 
> > Fix this by adding a buffer parameter to the generated helpers, that
> > the caller must supply, and updating all the call sites.
...
> Acked-by: Wei Liu 

Thanks, pushed.

Ian.

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

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Jan Beulich]

>>> On 13.10.17 at 12:23,  wrote:
> On 10/13/2017 10:20 AM, Jan Beulich wrote:
> On 13.10.17 at 11:10,  wrote:
>>> On 10/13/2017 10:06 AM, Jan Beulich wrote:
>>> On 13.10.17 at 11:00,  wrote:
> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
> @@ -99,13 +99,17 @@ int main(int argc, char **argv)
>  exit(-1);
>  }
>  
> -if ( !feof(fp) )
> +/* Only run the test if the input file was smaller than 
> INPUT_SIZE */
> +if ( feof(fp) )
> +{
> +LLVMFuzzerTestOneInput(input, size);
> +}

 ... ideally with the unnecessary braces dropped here
 Reviewed-by: Jan Beulich 
>>>
>>> Do you really want this to look like this?
>>>
>>> if ( ... )
>>>foo();
>>> else
>>> {
>>>...
>>> }
>> 
>> Yes. It's Linux and qemu who dislike non-matched if/else bodies,
>> but our ./CODING_STYLE only says
>> 
>> "Braces should be omitted for blocks with a single statement. e.g.,
>>  
>>  if ( condition )
>>  single_statement();"
>> 
>> and personally I'm happy that it doesn't say anything more.
> 
> Hmm, I personally think it's ugly enough that I'd rather restructure the
> code to avoid it looking like that. :-)
> 
> I'll see what I can do.

Well, assuming you would think that way I've intentionally said
"ideally", i.e. if you really don't want to change it, I can live with
the braces.

Jan


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

Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

[Jan Beulich]

>>> On 12.10.17 at 11:10,  wrote:
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -74,16 +74,19 @@ bool hvm_monitor_emul_unimplemented(void)
>  monitor_traps(curr, true, ) == 1;
>  }
>  
> -void hvm_monitor_msr(unsigned int msr, uint64_t value)
> +void hvm_monitor_msr(unsigned int msr, uint64_t new_value, uint64_t 
> old_value)
>  {
>  struct vcpu *curr = current;
>  
> -if ( monitored_msr(curr->domain, msr) )
> +if ( monitored_msr(curr->domain, msr) &&
> + ( !monitored_msr_onchangeonly(curr->domain, msr) ||
> +   new_value != old_value ) )

Stray blanks inside the inner parentheses.

> @@ -84,6 +84,11 @@ static int monitor_enable_msr(struct domain *d, u32 msr)
>  
>  hvm_enable_msr_interception(d, msr);
>  
> +if( onchangeonly )

Style.

> +__set_bit(index + sizeof(struct monitor_msr_bitmap), bitmap);

I think you miss "* 8" here - a bit position plus sizeof() doesn't
produce any useful value.

But what's worse - having read till the end of the patch I don't
see you change any allocation, yet you clearly need to double
the space now that you need two bits per MSR.

> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -105,4 +105,6 @@ void arch_monitor_cleanup_domain(struct domain *d);
>  
>  bool monitored_msr(const struct domain *d, u32 msr);
>  
> +bool monitored_msr_onchangeonly(const struct domain *d, u32 msr);
> +

Them belonging together, please have them together (without an
intervening blank line).

Jan


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

Re: [Xen-devel] [PATCH] libxl: dm_restrict: DEFINE_USERLOOKUP_HELPER returned a pointer to an auto

[Wei Liu]

On Fri, Oct 13, 2017 at 11:25:59AM +0100, Ian Jackson wrote:
> When I converted the previous open-coded user lookup functionality
> into DEFINE_USERLOOKUP_HELPER, I moved the struct passwd buffer into
> the function generated by the macro.  This is wrong because that
> buffer is used by get{pw,gr}* for its return value, so the helper
> function would contrive to return a pointer to the buffer on its own
> stack.
> 
> Fix this by adding a buffer parameter to the generated helpers, that
> the caller must supply, and updating all the call sites.
> 
> Reported-by: Andrew Cooper 
> Signed-off-by: Ian Jackson 
> CC: Wei Liu 

Acked-by: Wei Liu 

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

[Xen-devel] [PATCH] libxl: dm_restrict: DEFINE_USERLOOKUP_HELPER returned a pointer to an auto

[Ian Jackson]

When I converted the previous open-coded user lookup functionality
into DEFINE_USERLOOKUP_HELPER, I moved the struct passwd buffer into
the function generated by the macro.  This is wrong because that
buffer is used by get{pw,gr}* for its return value, so the helper
function would contrive to return a pointer to the buffer on its own
stack.

Fix this by adding a buffer parameter to the generated helpers, that
the caller must supply, and updating all the call sites.

Reported-by: Andrew Cooper 
Signed-off-by: Ian Jackson 
CC: Wei Liu 
---
 tools/libxl/libxl_dm.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 7caf471..a2ea95a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -762,9 +762,10 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
 #define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF) \
 static int userlookup_helper_##NAME(libxl__gc *gc,  \
 SPEC_TYPE spec, \
-   struct STRUCTNAME **out) \
+struct STRUCTNAME *resultbuf,   \
+struct STRUCTNAME **out)\
 {   \
-struct STRUCTNAME resultbuf, *resultp = NULL;   \
+struct STRUCTNAME *resultp = NULL;  \
 char *buf = NULL;   \
 long buf_size;  \
 int ret;\
@@ -779,7 +780,7 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
 \
 while (1) { \
 buf = libxl__realloc(gc, buf, buf_size);\
-ret = NAME##_r(spec, , buf, buf_size, );  \
+ret = NAME##_r(spec, resultbuf, buf, buf_size, );   \
 if (ret == ERANGE) {\
 buf_size += 128;\
 continue;   \
@@ -956,7 +957,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
 uint64_t ram_size;
 const char *path, *chardev;
 char *user = NULL;
-struct passwd *user_base;
+struct passwd *user_base, user_pwbuf;
 
 dm_args = flexarray_make(gc, 16, 1);
 dm_envs = flexarray_make(gc, 16, 1);
@@ -1660,20 +1661,21 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 }
 
 user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
-ret = userlookup_helper_getpwnam(gc, user, 0);
+ret = userlookup_helper_getpwnam(gc, user, _pwbuf, 0);
 if (ret < 0)
 return ret;
 if (ret > 0)
 goto end_search;
 
 ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
- _base);
+ _pwbuf, _base);
 if (ret < 0)
 return ret;
 if (ret > 0) {
-struct passwd *user_clash;
+struct passwd *user_clash, user_clash_pwbuf;
 uid_t intended_uid = user_base->pw_uid + guest_domid;
-ret = userlookup_helper_getpwuid(gc, intended_uid, _clash);
+ret = userlookup_helper_getpwuid(gc, intended_uid,
+ _clash_pwbuf, _clash);
 if (ret < 0)
 return ret;
 if (ret > 0) {
@@ -1693,7 +1695,7 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 }
 
 user = LIBXL_QEMU_USER_SHARED;
-ret = userlookup_helper_getpwnam(gc, user, 0);
+ret = userlookup_helper_getpwnam(gc, user, _pwbuf, 0);
 if (ret < 0)
 return ret;
 if (ret > 0) {
-- 
2.1.4


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

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

On 10/13/2017 10:20 AM, Jan Beulich wrote:
 On 13.10.17 at 11:10,  wrote:
>> On 10/13/2017 10:06 AM, Jan Beulich wrote:
>> On 13.10.17 at 11:00,  wrote:
 --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
 +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
 @@ -99,13 +99,17 @@ int main(int argc, char **argv)
  exit(-1);
  }
  
 -if ( !feof(fp) )
 +/* Only run the test if the input file was smaller than 
 INPUT_SIZE */
 +if ( feof(fp) )
 +{
 +LLVMFuzzerTestOneInput(input, size);
 +}
>>>
>>> ... ideally with the unnecessary braces dropped here
>>> Reviewed-by: Jan Beulich 
>>
>> Do you really want this to look like this?
>>
>> if ( ... )
>>foo();
>> else
>> {
>>...
>> }
> 
> Yes. It's Linux and qemu who dislike non-matched if/else bodies,
> but our ./CODING_STYLE only says
> 
> "Braces should be omitted for blocks with a single statement. e.g.,
>  
>  if ( condition )
>  single_statement();"
> 
> and personally I'm happy that it doesn't say anything more.

Hmm, I personally think it's ugly enough that I'd rather restructure the
code to avoid it looking like that. :-)

I'll see what I can do.

 -George

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

Re: [Xen-devel] [PATCH 1/2] x86/boot: fix early error display

[Jan Beulich]

>>> On 13.10.17 at 11:51,  wrote:
> On 12/10/17 21:55, Andrew Cooper wrote:
>> On 12/10/2017 21:50, Doug Goldstein wrote:
>>> From: David Esler 
>>>
>>> In 9180f5365524 a change was made to the send_chr function to take in
>>> C-strings and print out a character at a time until a NULL was
>>> encountered. However there is no code to increment the current character
>>> position resulting in an endless loop of the first character. This adds
>>> a simple increment.
>>>
>>> Reviewed-by: Doug Goldstein 
>>> Signed-off-by: David Esler 
>> 
>> Reviewed-by: Andrew Cooper 
>> 
>> CC'ing Julien as release manager. This should be included in 4.10 IMO,
>> and is a backport candidate.
> 
> I agree.
> 
> Release-acked-by: Julien Grall 

So I take this is a pre-ack to the eventual patch with the bug fixed
that Daniel has pointed out?

Jan


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

Re: [Xen-devel] [PATCH v4 10/12] fuzz/x86_emulate: Add --rerun option to try to track down instability

[Jan Beulich]

>>> On 13.10.17 at 11:43,  wrote:
> On 10/12/2017 04:24 PM, Jan Beulich wrote:
> On 11.10.17 at 19:52,  wrote:
>>> +if ( memcmp([0].regs, [1].regs, sizeof(state[0].regs)) 
>>> )
>>> +{
>>> +printf("registers differ!\n");
>>> +/* Print If Not Equal */
>>> +#define PRINT_NE(elem)\
>>> +if ( state[0].elem != state[1].elem ) \
>>> +printf(#elem " differ: %lx != %lx\n", \
>>> +   (unsigned long)state[0].elem, \
>>> +   (unsigned long)state[1].elem)
>>> +PRINT_NE(regs.r15);
>>> +PRINT_NE(regs.r14);
>>> +PRINT_NE(regs.r13);
>>> +PRINT_NE(regs.r12);
>>> +PRINT_NE(regs.rbp);
>>> +PRINT_NE(regs.rbx);
>>> +PRINT_NE(regs.r10);
>>> +PRINT_NE(regs.r11);
>>> +PRINT_NE(regs.r9);
>>> +PRINT_NE(regs.r8);
>>> +PRINT_NE(regs.rax);
>>> +PRINT_NE(regs.rcx);
>>> +PRINT_NE(regs.rdx);
>>> +PRINT_NE(regs.rsi);
>>> +PRINT_NE(regs.rdi);
>> 
>> Aren't these register fields all of the same type? If so, why do you
>> need to casts to unsigned long in the macro?
> 
> As it happens, they're all the same size; when I wrote the macro it was
> designed such that the same macro could be used for all the elements
> regardless of what size they were.  Since there's no time pressure,
> would you rather I add the segment registers (and leave the cast), or
> only add rflags (and remove the cast)?

Printing the selector registers separately is more important than
macro cosmetics, so if you end up using the macro for them, then
I'm of course fine with the cast left in place.

Jan


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

Re: [Xen-devel] [PATCH v4 08/12] fuzz/x86_emulate: Move all state into fuzz_state

[George Dunlap]

On 10/13/2017 10:54 AM, Jan Beulich wrote:
 On 13.10.17 at 11:22,  wrote:
>> On 10/12/2017 04:16 PM, Jan Beulich wrote:
>> On 11.10.17 at 19:52,  wrote:
 @@ -761,12 +757,11 @@ static void disable_hooks(struct x86_emulate_ctxt 
 *ctxt)
  static void sanitize_input(struct x86_emulate_ctxt *ctxt)
  {
  struct fuzz_state *s = ctxt->data;
 -struct fuzz_corpus *c = s->corpus;
 -struct cpu_user_regs *regs = >regs;
 -unsigned long bitmap = c->options;
 +struct cpu_user_regs *regs = ctxt->regs;
 +unsigned long bitmap = s->options;
  
  /* Some hooks can't be disabled. */
 -c->options &= ~((1<options &= ~((1<>>
>>> Mind adding the missing blanks here while you touch this?
>>
>> Like this?
>>
>> s->options &= ~((1< 
> Even farther (at the same time adding the missing number suffixes):
> 
> s->options &= ~((1UL << HOOK_read) | (1UL << HOOK_insn_fetch));

Got it.  (I was actually trying to verify the 'snuggly' outer braces,
but missed spaces around the '<<'s).

 -George

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

Re: [Xen-devel] [PATCH v4 08/12] fuzz/x86_emulate: Move all state into fuzz_state

[Jan Beulich]

>>> On 13.10.17 at 11:22,  wrote:
> On 10/12/2017 04:16 PM, Jan Beulich wrote:
> On 11.10.17 at 19:52,  wrote:
>>> @@ -761,12 +757,11 @@ static void disable_hooks(struct x86_emulate_ctxt 
>>> *ctxt)
>>>  static void sanitize_input(struct x86_emulate_ctxt *ctxt)
>>>  {
>>>  struct fuzz_state *s = ctxt->data;
>>> -struct fuzz_corpus *c = s->corpus;
>>> -struct cpu_user_regs *regs = >regs;
>>> -unsigned long bitmap = c->options;
>>> +struct cpu_user_regs *regs = ctxt->regs;
>>> +unsigned long bitmap = s->options;
>>>  
>>>  /* Some hooks can't be disabled. */
>>> -c->options &= ~((1<>> +s->options &= ~((1<> 
>> Mind adding the missing blanks here while you touch this?
> 
> Like this?
> 
> s->options &= ~((1<options &= ~((1UL << HOOK_read) | (1UL << HOOK_insn_fetch));

Jan


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

Re: [Xen-devel] [PATCH 1/2] x86/boot: fix early error display

[Julien Grall]

Hi Andrew,

On 12/10/17 21:55, Andrew Cooper wrote:

On 12/10/2017 21:50, Doug Goldstein wrote:

From: David Esler 

In 9180f5365524 a change was made to the send_chr function to take in
C-strings and print out a character at a time until a NULL was
encountered. However there is no code to increment the current character
position resulting in an endless loop of the first character. This adds
a simple increment.

Reviewed-by: Doug Goldstein 
Signed-off-by: David Esler 


Reviewed-by: Andrew Cooper 

CC'ing Julien as release manager. This should be included in 4.10 IMO,
and is a backport candidate.


I agree.

Release-acked-by: Julien Grall 

Cheers,




---
  xen/arch/x86/boot/head.S | 1 +
  1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index fd6fc337fe..f48bbbd2e5 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -174,6 +174,7 @@ not_multiboot:
  mov sym_esi(vga_text_buffer),%edi
  .Lsend_chr:
  mov (%esi),%bl
+inc %esi
  test%bl,%bl# Terminate on '\0' sentinel
  je  .Lhalt
  mov $0x3f8+5,%dx   # UART Line Status Register



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



--
Julien Grall

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

Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 11.10.17 at 18:26,  wrote:
> @@ -4568,6 +4571,32 @@ static int do_altp2m_op(
>  a.u.set_mem_access.view);
>  break;
>  
> +case HVMOP_altp2m_set_mem_access_multi:
> +if ( a.u.set_mem_access_multi.pad ||
> + a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +{
> +rc = -EINVAL;
> +break;
> +}
> +
> +rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> +  a.u.set_mem_access_multi.access_list,
> +  a.u.set_mem_access_multi.nr,
> +  a.u.set_mem_access_multi.opaque,
> +  0x3F,

Please add /* pretty arbitrary */ or something similar here.

> @@ -4586,6 +4615,80 @@ static int do_altp2m_op(
>  return rc;
>  }
>  
> +DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
> +
> +static int compat_altp2m_op(
> +XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +int rc = 0;
> +struct compat_hvm_altp2m_op a;
> +union
> +{
> +XEN_GUEST_HANDLE_PARAM(void) hnd;
> +struct xen_hvm_altp2m_op *altp2m_op;
> +} nat;
> +
> +if ( !hvm_altp2m_supported() )
> +return -EOPNOTSUPP;
> +
> +if ( copy_from_guest(, arg, 1) )
> +return -EFAULT;
> +
> +if ( a.pad1 || a.pad2 ||
> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> +return -EINVAL;
> +
> +set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> +
> +switch ( a.cmd )
> +{
> +case HVMOP_altp2m_set_mem_access_multi:

Indentation.

> +BUILD_BUG_ON(sizeof(struct xen_hvm_altp2m_set_mem_access_multi) <
> + sizeof(struct 
> compat_hvm_altp2m_set_mem_access_multi));

What good does this do?

> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \
> +guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list)
> +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \
> +guest_from_compat_handle((_d_)->access_list, (_s_)->access_list)
> +
> XLAT_hvm_altp2m_set_mem_access_multi(_op->u.set_mem_access_multi,
> +_mem_access_multi);
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> +break;
> +default:
> +return do_altp2m_op(arg);
> +}
> +
> +/*
> + * Manually fill the common part of the xen_hvm_altp2m_op structure 
> because
> + * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the
> + * translation of all fields from compat_hvm_altp2m_op to 
> xen_hvm_altp2m_op.
> + */
> +nat.altp2m_op->version  = a.version;
> +nat.altp2m_op->cmd  = a.cmd;
> +nat.altp2m_op->domain   = a.domain;
> +nat.altp2m_op->pad1 = a.pad1;
> +nat.altp2m_op->pad2 = a.pad2;

There are _still_ no size checks here.

> +rc = do_altp2m_op(nat.hnd);
> +
> +switch ( a.cmd )
> +{
> +case HVMOP_altp2m_set_mem_access_multi:

Indentation.

> +if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0 )

Please also check rc here to avoid needlessly copying back. In
fact _only_ checking rc ought to be fine.

> +{
> +a.u.set_mem_access_multi.opaque =
> +nat.altp2m_op->u.set_mem_access_multi.opaque;

You also need a size check here.

Jan


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

Re: [Xen-devel] [PATCH for 4.10 0/3] XSA-243 followup

[Julien Grall]

Hi Andrew,

On 12/10/17 14:54, Andrew Cooper wrote:

The important change here is in patch 3, which is intended to remove the
correct-but-dangerous-looking construction of linear pagetables slots for
shadowed guests.


Release-acked-by: Julien Grall 

Cheers,



Andrew Cooper (3):
   Revert "x86/mm: move PV l4 table setup code" and "x86/mm: factor out
 pv_arch_init_memory"
   x86/mm: Consolidate all Xen L2 slot writing into
 init_xen_pae_l2_slots()
   x86/mm: Consolidate all Xen L4 slot writing into init_xen_l4_slots()

  xen/arch/x86/mm.c  | 144 ---
  xen/arch/x86/mm/hap/hap.c  |  31 ++---
  xen/arch/x86/mm/shadow/multi.c | 148 +++--
  xen/arch/x86/pv/dom0_build.c   |   5 +-
  xen/arch/x86/pv/domain.c   |   7 +-
  xen/arch/x86/pv/mm.c   |  82 ---
  xen/arch/x86/pv/mm.h   |   3 -
  xen/include/asm-x86/mm.h   |   3 +
  xen/include/asm-x86/pv/mm.h|   4 --
  9 files changed, 187 insertions(+), 240 deletions(-)



--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 10/12] fuzz/x86_emulate: Add --rerun option to try to track down instability

[George Dunlap]

On 10/12/2017 04:24 PM, Jan Beulich wrote:
 On 11.10.17 at 19:52,  wrote:
>> @@ -884,20 +891,146 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
>>  return 0;
>>  }
>>  
>> -int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>> +static void setup_fuzz_state(struct fuzz_state *state, const void *data_p, 
>> size_t size)
>>  {
>> -struct fuzz_state state = {
>> -.ops = all_fuzzer_ops,
>> -};
>> -struct x86_emulate_ctxt ctxt = {
>> -.data = ,
>> -.regs = ,
>> -.addr_size = 8 * sizeof(void *),
>> -.sp_size = 8 * sizeof(void *),
>> -};
>> +memset(state, 0, sizeof(*state));
>> +state->corpus = data_p;
>> +state->data_num = size;
>> +}
>> +
>> +static int runtest(struct fuzz_state *state) {
>>  int rc;
>>  
>> -if ( size <= fuzz_minimal_input_size() )
>> +struct x86_emulate_ctxt *ctxt = >ctxt;
> 
> Please don't leave a blank line between declarations.
> 
>> +static void compare_states(struct fuzz_state state[2])
>> +{
>> +/* First zero any "internal" pointers */
>> +state[0].corpus = state[1].corpus = NULL;
>> +state[0].ctxt.data = state[1].ctxt.data = NULL;
>> +state[0].ctxt.regs = state[1].ctxt.regs = NULL;
>> +
>> +if ( memcmp([0], [1], sizeof(struct fuzz_state)) )
>> +{
>> +unsigned int i;
>> +
>> +printf("State mismatch\n");
>> +
>> +for ( i = 0; i < ARRAY_SIZE(state[0].cr); i++ )
>> +if ( state[0].cr[i] != state[1].cr[i] )
>> +printf("cr[%u]: %lx != %lx\n",
>> +   i, state[0].cr[i], state[1].cr[i]);
>> +
>> +for ( i = 0; i < ARRAY_SIZE(state[0].msr); i++ )
>> +if ( state[0].msr[i] != state[1].msr[i] )
>> +printf("msr[%u]: %lx != %lx\n",
>> +   i, state[0].msr[i], state[1].msr[i]);
>> +
>> +for ( i = 0; i < ARRAY_SIZE(state[0].segments); i++ )
>> +if ( memcmp([0].segments[i], [1].segments[i],
>> +sizeof(state[0].segments[0])) )
>> +printf("segments[%u]: [%x:%x:%x:%lx] != [%x:%x:%x:%lx]!\n", 
>> i,
>> +   (unsigned)state[0].segments[i].sel,
>> +   (unsigned)state[0].segments[i].attr,
>> +   state[0].segments[i].limit,
>> +   state[0].segments[i].base,
>> +   (unsigned)state[1].segments[i].sel,
>> +   (unsigned)state[1].segments[i].attr,
>> +   state[1].segments[i].limit,
>> +   state[1].segments[i].base);
>> +
>> +if ( state[0].data_num != state[1].data_num )
>> +printf("data_num: %lx !=  %lx\n", state[0].data_num,
>> +   state[1].data_num);
>> +if ( state[0].data_index != state[1].data_index )
>> +printf("data_index: %lx !=  %lx\n", state[0].data_index,
>> +   state[1].data_index);
>> +
>> +if ( memcmp([0].regs, [1].regs, sizeof(state[0].regs)) )
>> +{
>> +printf("registers differ!\n");
>> +/* Print If Not Equal */
>> +#define PRINT_NE(elem)\
>> +if ( state[0].elem != state[1].elem ) \
>> +printf(#elem " differ: %lx != %lx\n", \
>> +   (unsigned long)state[0].elem, \
>> +   (unsigned long)state[1].elem)
>> +PRINT_NE(regs.r15);
>> +PRINT_NE(regs.r14);
>> +PRINT_NE(regs.r13);
>> +PRINT_NE(regs.r12);
>> +PRINT_NE(regs.rbp);
>> +PRINT_NE(regs.rbx);
>> +PRINT_NE(regs.r10);
>> +PRINT_NE(regs.r11);
>> +PRINT_NE(regs.r9);
>> +PRINT_NE(regs.r8);
>> +PRINT_NE(regs.rax);
>> +PRINT_NE(regs.rcx);
>> +PRINT_NE(regs.rdx);
>> +PRINT_NE(regs.rsi);
>> +PRINT_NE(regs.rdi);
> 
> Aren't these register fields all of the same type? If so, why do you
> need to casts to unsigned long in the macro?

As it happens, they're all the same size; when I wrote the macro it was
designed such that the same macro could be used for all the elements
regardless of what size they were.  Since there's no time pressure,
would you rather I add the segment registers (and leave the cast), or
only add rflags (and remove the cast)?

> 
>> +for ( i = offsetof(struct cpu_user_regs, error_code) / 
>> sizeof(unsigned);
>> +  i < sizeof(state[1].regs)/sizeof(unsigned); i++ )
> 
> Blanks around binary operators please (also elsewhere).

Ack

 -George

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

Re: [Xen-devel] [PATCH 0/3] x86: adjustments to page table updates

[Julien Grall]

Hi Jan,

On 12/10/17 10:38, Jan Beulich wrote:

The first two patches are bug fixes and hence candidates for 4.10.
The 3rd is mostly cleanup, and hence intended only for after 4.10.

1: request page table page-in for the correct domain
2: fix do_update_va_mapping_otherdomain() wrt translated domains


Release-acked-by: Julien Grall 

Cheers,


3: tighten MMU_*PT_UPDATE* check and combine error paths

Signed-off-by: Jan Beulich 


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



--
Julien Grall

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

Re: [Xen-devel] [PATCH] x86/vpt: fix a bug in pt_update_irq()

[Jan Beulich]

>>> On 13.10.17 at 10:14,  wrote:
> On Fri, Oct 13, 2017 at 02:25:38AM -0600, Jan Beulich wrote:
> On 09.10.17 at 23:32,  wrote:
>>> +{
>>> +if ( unlikely(vec < 16) )
>>> +return false;
>>> +
>>> +if ( hvm_funcs.sync_pir_to_irr )
>>> +hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
>>
>>Question is whether this is really necessary, of whether instead
>>you could just return the state of the respective PIR bit here. I'd
>>prefer that over giving the function a name no longer suggesting
>>it leaves all state alone.
> 
> It is a good suggestion. But I incline to check the PIR bit and if the
> bit is set, return true and otherwise return the state of the vIRR bit
> in case PIR bits are already synced to vIRR.

Oh, right, of course you should return the effective "or" of the
two bits.

Jan


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

Re: [Xen-devel] [PATCH v4 08/12] fuzz/x86_emulate: Move all state into fuzz_state

[George Dunlap]

On 10/12/2017 04:16 PM, Jan Beulich wrote:
 On 11.10.17 at 19:52,  wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -22,34 +22,31 @@
>>  
>>  #define SEG_NUM x86_seg_none
>>  
>> -/* Layout of data expected as fuzzing input. */
>> -struct fuzz_corpus
>> +/*
>> + * State of the fuzzing harness and emulated cpu.  Calculated
>> + * initially from the input corpus, and later mutated by the emulation
>> + * callbacks (and the emulator itself, in the case of regs).
>> + */
>> +struct fuzz_state
>>  {
>> +/* Emulated CPU state */
>> +unsigned long options;
>>  unsigned long cr[5];
>>  uint64_t msr[MSR_INDEX_MAX];
>> -struct cpu_user_regs regs;
>>  struct segment_register segments[SEG_NUM];
>> -unsigned long options;
>> -unsigned char data[INPUT_SIZE];
>> -} input;
>> -#define DATA_OFFSET offsetof(struct fuzz_corpus, data)
>> +struct cpu_user_regs regs;
>>  
>> -/*
>> - * Internal state of the fuzzing harness.  Calculated initially from the 
>> input
>> - * corpus, and later mutates by the emulation callbacks.
>> - */
>> -struct fuzz_state
>> -{
>>  /* Fuzzer's input data. */
>> -struct fuzz_corpus *corpus;
>> +#define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>> +const unsigned char * corpus;
> 
> Stray blank after *. Also any reason this can't be uint8_t,
> matching LLVMFuzzerTestOneInput()'s parameter and making
> it possible to avoid the cast you currently use on that
> assignment?

For some reason I thought this would make things uglier; but it actually
works pretty well.

>> @@ -646,11 +634,20 @@ static void set_sizes(struct x86_emulate_ctxt *ctxt)
>>  ctxt->addr_size = ctxt->sp_size = 64;
>>  else
>>  {
>> -ctxt->addr_size = c->segments[x86_seg_cs].db ? 32 : 16;
>> -ctxt->sp_size   = c->segments[x86_seg_ss].db ? 32 : 16;
>> +ctxt->addr_size = s->segments[x86_seg_cs].db ? 32 : 16;
>> +ctxt->sp_size   = s->segments[x86_seg_ss].db ? 32 : 16;
>>  }
>>  }
>>  
>> +static void setup_state(struct x86_emulate_ctxt *ctxt)
>> +{
>> +struct fuzz_state *s = ctxt->data;
>> +
>> +/* Fuzz all of the emulated state in one go */
>> +if (!input_read(s, s, DATA_OFFSET))
> 
> Missing blanks.

Ack

> 
>> @@ -761,12 +757,11 @@ static void disable_hooks(struct x86_emulate_ctxt 
>> *ctxt)
>>  static void sanitize_input(struct x86_emulate_ctxt *ctxt)
>>  {
>>  struct fuzz_state *s = ctxt->data;
>> -struct fuzz_corpus *c = s->corpus;
>> -struct cpu_user_regs *regs = >regs;
>> -unsigned long bitmap = c->options;
>> +struct cpu_user_regs *regs = ctxt->regs;
>> +unsigned long bitmap = s->options;
>>  
>>  /* Some hooks can't be disabled. */
>> -c->options &= ~((1<> +s->options &= ~((1< 
> Mind adding the missing blanks here while you touch this?

Like this?

s->options &= ~((1<

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Jan Beulich]

>>> On 13.10.17 at 11:10,  wrote:
> On 10/13/2017 10:06 AM, Jan Beulich wrote:
> On 13.10.17 at 11:00,  wrote:
>>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
>>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
>>> @@ -99,13 +99,17 @@ int main(int argc, char **argv)
>>>  exit(-1);
>>>  }
>>>  
>>> -if ( !feof(fp) )
>>> +/* Only run the test if the input file was smaller than INPUT_SIZE 
>>> */
>>> +if ( feof(fp) )
>>> +{
>>> +LLVMFuzzerTestOneInput(input, size);
>>> +}
>> 
>> ... ideally with the unnecessary braces dropped here
>> Reviewed-by: Jan Beulich 
> 
> Do you really want this to look like this?
> 
> if ( ... )
>foo();
> else
> {
>...
> }

Yes. It's Linux and qemu who dislike non-matched if/else bodies,
but our ./CODING_STYLE only says

"Braces should be omitted for blocks with a single statement. e.g.,
 
 if ( condition )
 single_statement();"

and personally I'm happy that it doesn't say anything more.

Jan


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

Re: [Xen-devel] ARM32 - build-issues with xen/arm: vpl011: Add a new vuart node in the xenstore

[Bhupinder Thakur]

On 13 October 2017 at 00:34, Andrew Cooper  wrote:
> On 12/10/17 19:54, Bhupinder Thakur wrote:
>> On 5 October 2017 at 15:07, Wei Liu  wrote:
>>> On Wed, Oct 04, 2017 at 09:58:32PM -0400, Konrad Rzeszutek Wilk wrote:
 I get this when compiling under ARM32 (Ubuntu 15.04,
 gcc (Ubuntu/Linaro 4.9.2-10ubuntu13) 4.9.2):

 libxl_console.c: In function ‘libxl__device_vuart_add’:
 libxl_console.c:379:5: error: format ‘%lu’ expects argument of type ‘long 
 unsigned int’, but argument 3 has type ‘xen_pfn_t’ [-Werror=format=]
  flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_gfn));
  ^
 ;
>>> My Wheezy 32bit chroot didn't catch this, sigh.
>>>
>>> Does the following patch work?
>>>
>>> From ae531197382bf0bc003606a9712075bdd22cfc24 Mon Sep 17 00:00:00 2001
>>> From: Wei Liu 
>>> Date: Thu, 5 Oct 2017 10:35:28 +0100
>>> Subject: [PATCH] libxl: use correct type modifier for vuart_gfn
>>> MIME-Version: 1.0
>>> Content-Type: text/plain; charset=UTF-8
>>> Content-Transfer-Encoding: 8bit
>>>
>>> Fixes compilation error like:
>>>
>>> libxl_console.c: In function ‘libxl__device_vuart_add’:
>>> libxl_console.c:379:5: error: format ‘%lu’ expects argument of type ‘long 
>>> unsigned int’, but argument 3 has type ‘xen_pfn_t’ [-Werror=format=]
>>>   flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_gfn));
>>>
>>> Reported-by: Konrad Rzeszutek Wilk 
>>> Signed-off-by: Wei Liu 
>>> ---
>>>  tools/libxl/libxl_console.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
>>> index 13ecf128e2..c05dc28b99 100644
>>> --- a/tools/libxl/libxl_console.c
>>> +++ b/tools/libxl/libxl_console.c
>>> @@ -376,7 +376,7 @@ int libxl__device_vuart_add(libxl__gc *gc, uint32_t 
>>> domid,
>>>  flexarray_append(ro_front, "port");
>>>  flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
>>>  flexarray_append(ro_front, "ring-ref");
>>> -flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_gfn));
>>> +flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, 
>>> state->vuart_gfn));
>> Unfortunately, this is causing an issue as PRI_xen_pfn formats the
>> value as a hexadecimal value but xenconsole later reads it as a
>> decimal value and tries to map it, which fails and therefore vuart
>> console initialization fails.
>>
>> Earlier, I verified only 32-bit compilation but did not test the
>> change. It was a miss from my side. I have tested now with the format
>> string changed to PRIu64 and the vuart console is working fine.
>
> That however, would break x86.
>
> andrewcoop@andrewcoop:/local/xen.git/xen$ git grep 'define PRI_xen_pfn' -- :/
> include/public/arch-arm.h:276:#define PRI_xen_pfn PRIx64
> include/public/arch-x86/xen.h:77:#define PRI_xen_pfn "lx"
>
> The best way to fix this is to introduce a new define for both
> architectures which is PRIu64 and "lu" as appropriate.
>
> Suggestions:
>
> PRI_xen_pfn_dec
> PRIu_xen_pfn
>
> Neither are great, but the latter does follow the PRI nomenclature.
Thanks. I will introduce this new format specifier.

Regards,
Bhupinder

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

Re: [Xen-devel] [PATCH] x86/vpt: fix a bug in pt_update_irq()

[Chao Gao]

On Fri, Oct 13, 2017 at 02:25:38AM -0600, Jan Beulich wrote:
 On 09.10.17 at 23:32,  wrote:
>
>First of all - please use a better subject. If someone finds another
>bug in this function in, say, half a year's time, how will we tell apart
>the two patches from looking at just the list of titles several years
>later?

Will update.

>
>> pt_update_irq() is expected to return the vector number of periodic
>> timer interrupt, which should be set in vIRR of vlapic. Otherwise it
>> would trigger the assertion in vmx_intr_assist(), please seeing
>> https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00915.html.
>> 
>> But it fails to achieve that in the following two case:
>> 1. hvm_isa_irq_assert() may not set the corresponding bit in vIRR for
>> mask field of IOAPIC RTE is set. Please refer to the call tree
>> vmx_intr_assist() -> pt_update_irq() -> hvm_isa_irq_assert() ->
>> assert_irq() -> assert_gsi() -> vioapic_irq_positive_edge(). The patch
>> checks whether the vector is set or not in vIRR of vlapic before
>> returning.
>> 
>> 2. someone changes the vector field of IOAPIC RTE between asserting
>> the irq and getting the vector of the irq, leading to setting the
>> old vector number but returning a different vector number. This patch
>> holds the irq_lock when doing the two operations to prevent the case.
>> 
>> Signed-off-by: Chao Gao 
>
>Point 2 is very unlikely to be the cause of the failed assertion that
>osstest keeps hitting once in a while. Did your analysis yield
>indication that point 1 is what is happening there?

I believe it is likely to be the case. On the other hand, the
assertion can be triggered in above two cases; it needs to be
fixed.

>
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -168,20 +168,23 @@ void hvm_gsi_deassert(struct domain *d, unsigned int 
>> gsi)
>>  spin_unlock(>arch.hvm_domain.irq_lock);
>>  }
>>  
>> -void hvm_isa_irq_assert(
>> -struct domain *d, unsigned int isa_irq)
>> +void hvm_isa_irq_assert_locked(struct domain *d, unsigned int isa_irq)
>
>Please don't introduce a non-static function like this. Instead I
>would suggest you introduce a new helper function doing what
>you introduce as replacement to the call to
>hvm_isa_irq_assert(). That'll presumably involve passing a
>get_vector() callback to a wrapper of pt_irq_vector() (or to an
>abbreviated form of it, as "src" is hvm_intsrc_lapic), since I
>understand you need this called with the lock held.
>
>And once you do this I don't think it'll be worthwhile breaking
>out hvm_isa_irq_assert_locked() at all - you'll just have a
>sibling to hvm_isa_irq_assert(). Or, considering the few callers
>the function has, simply giving that function itself an optional
>callback parameter might be even better (eliminating any code
>duplication).

Ok. I understand what you suggestion. Will give it a shot.

>
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -137,6 +137,17 @@ static void vlapic_error(struct vlapic *vlapic, 
>> unsigned int errmask)
>>  spin_unlock_irqrestore(>esr_lock, flags);
>>  }
>>  
>> +bool vlapic_test_irq(struct vlapic *vlapic, uint8_t vec)
>
>The way the function is named, the pointer should be const
>qualified. However, the function does more than just testing
>current state:
>
>> +{
>> +if ( unlikely(vec < 16) )
>> +return false;
>> +
>> +if ( hvm_funcs.sync_pir_to_irr )
>> +hvm_funcs.sync_pir_to_irr(vlapic_vcpu(vlapic));
>
>Question is whether this is really necessary, of whether instead
>you could just return the state of the respective PIR bit here. I'd
>prefer that over giving the function a name no longer suggesting
>it leaves all state alone.

It is a good suggestion. But I incline to check the PIR bit and if the
bit is set, return true and otherwise return the state of the vIRR bit
in case PIR bits are already synced to vIRR.

Thanks
Chao

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

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

On 10/13/2017 10:06 AM, Jan Beulich wrote:
 On 13.10.17 at 11:00,  wrote:
>> Changeset  introduced "batch mode" to afl-harness, which allowed
> 
> With (part of) the commit hash and the title inserted here and ...

This should be `2b1cde7783` BTW.

 -George

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

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

On 10/13/2017 10:06 AM, Jan Beulich wrote:
 On 13.10.17 at 11:00,  wrote:
>> Changeset  introduced "batch mode" to afl-harness, which allowed
> 
> With (part of) the commit hash and the title inserted here and ...

Gah. :-)

> 
>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> @@ -99,13 +99,17 @@ int main(int argc, char **argv)
>>  exit(-1);
>>  }
>>  
>> -if ( !feof(fp) )
>> +/* Only run the test if the input file was smaller than INPUT_SIZE 
>> */
>> +if ( feof(fp) )
>> +{
>> +LLVMFuzzerTestOneInput(input, size);
>> +}
> 
> ... ideally with the unnecessary braces dropped here
> Reviewed-by: Jan Beulich 

Do you really want this to look like this?

if ( ... )
   foo();
else
{
   ...
}

 -George

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

Re: [Xen-devel] [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Jan Beulich]

>>> On 13.10.17 at 11:00,  wrote:
> Changeset  introduced "batch mode" to afl-harness, which allowed

With (part of) the commit hash and the title inserted here and ...

> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
> @@ -99,13 +99,17 @@ int main(int argc, char **argv)
>  exit(-1);
>  }
>  
> -if ( !feof(fp) )
> +/* Only run the test if the input file was smaller than INPUT_SIZE */
> +if ( feof(fp) )
> +{
> +LLVMFuzzerTestOneInput(input, size);
> +}

... ideally with the unnecessary braces dropped here
Reviewed-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
> Andrew Cooper
> Sent: 13 October 2017 10:00
> To: Ross Lagerwall ; Ian Jackson
> ; qemu-de...@nongnu.org
> Cc: Anthony Perard ; Juergen Gross
> ; Stefano Stabellini ; xen-
> de...@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until 
> just
> before os_setup_post
> 
> On 13/10/2017 09:37, Ross Lagerwall wrote:
> > On 10/09/2017 05:01 PM, Ian Jackson wrote:
> >> We need to restrict *all* the control fds that qemu opens.  Looking in
> >> /proc/PID/fd shows there are many; their allocation seems scattered
> >> throughout Xen support code in qemu.
> >>
> >> We must postpone the restrict call until roughly the same time as qemu
> >> changes its uid, chroots (if applicable), and so on.
> >>
> >> There doesn't seem to be an appropriate hook already.  The RunState
> >> change hook fires at different times depending on exactly what mode
> >> qemu is operating in.
> >>
> >> And it appears that no-one but the Xen code wants a hook at this phase
> >> of execution.  So, introduce a bare call to a new function
> >> xen_setup_post, just before os_setup_post.  Also provide the
> >> appropriate stub for when Xen compilation is disabled.
> >>
> >> We do the restriction before rather than after os_setup_post, because
> >> xen_restrict may need to open /dev/null, and os_setup_post might have
> >> called chroot.
> >>
> > This works for normally starting a VM but doesn't seem to work when
> > resuming/migrating.
> >
> > Here is the order of selected operations when starting a VM normally:
> >     VNC server running on 127.0.0.1:5901
> >     xen_change_state_handler()
> >     xenstore_record_dm_state: running
> >     xen_setup_post()
> >     xentoolcore_restrict_all: rc = 0
> >     os_setup_post()
> >     main_loop()
> >
> > Here is the order of selected operations when starting QEMU with
> > -incoming fd:... :
> >     VNC server running on 127.0.0.1:5902
> >     migration_fd_incoming()
> >     xen_setup_post()
> >     xentoolcore_restrict_all: rc = 0
> >     os_setup_post()
> >     main_loop()
> >     migration_set_incoming_channel()
> >     migrate_set_state()
> >     xen_change_state_handler()
> >     xenstore_record_dm_state: running
> >     error recording dm state
> >     qemu exited with code 1
> >
> > The issue is that QEMU needs xenstore access to write "running" but
> > this is after it has already been restricted. Moving xen_setup_post()
> > into xen_change_state_handler() works fine. The only issue is that in
> > the migration case, it executes after os_setup_post() so QEMU might be
> > chrooted in which case opening /dev/null to restrict fds doesn't work
> > (unless its new root has a /dev/null).
> >
> 
> Wasn't the agreement in the end to remove all use of xenstore from the
> device mode?  This running notification can and should be QMP, at which
> point we break a causal dependency.
> 

Yes, that was the agreement. One problem is that there is not yet adequate 
separation in either QEMU's common and pv/hvm init routines to do this yet. 
Nor, I believe, is there support in libxl to spawn separate xenpv and xenfv 
instances of QEMU for the same guest.

  Paul

> For safety reasons, qemu needs to have restricted/dropped/etc all
> permissions before it looks at a single byte of incoming migration data,
> to protect against buggy or malicious alterations to the migration stream.
> 
> ~Andrew
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v7 16/16] docs: add MBA description in docs

[Yi Sun]

This patch adds MBA description in related documents.

Signed-off-by: Yi Sun 
Acked-by: Wei Liu 
Reviewed-by: Roger Pau Monné 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Chao Peng 

v5:
- remove 'closed-loop' in 'xl-psr.markdown'
  (suggested by Roger Pau Monné)
v4:
- modify description of MBA in 'xl.pod.1.in' to be same as feature doc.
  (suggested by Roger Pau Monné)
- fix words issue.
  (suggested by Roger Pau Monné)
v2:
- state the value type shown by 'psr-mba-show'. For linear mode,
  it shows decimal value. For non-linear mode, it shows hexadecimal
  value.
  (suggested by Chao Peng)
---
 docs/man/xl.pod.1.in  | 33 +
 docs/misc/xl-psr.markdown | 62 +++
 2 files changed, 95 insertions(+)

diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
index cd8bb1c..324ef9e 100644
--- a/docs/man/xl.pod.1.in
+++ b/docs/man/xl.pod.1.in
@@ -1845,6 +1845,39 @@ processed.
 
 =back
 
+=head2 Memory Bandwidth Allocation
+
+Intel Skylake and later server platforms offer capabilities to configure and
+make use of the Memory Bandwidth Allocation (MBA) mechanisms, which provides
+OS/VMMs the ability to slow misbehaving apps/VMs by using a credit-based
+throttling mechanism. In the Xen implementation, MBA is used to control memory
+bandwidth on VM basis. To enforce bandwidth on a specific domain, just set
+throttling value (THRTL) for the domain.
+
+=over 4
+
+=item B [I] I I
+
+Set throttling value (THRTL) for a domain. For how to specify I
+please refer to L.
+
+B
+
+=over 4
+
+=item B<-s SOCKET>, B<--socket=SOCKET>
+
+Specify the socket to process, otherwise all sockets are processed.
+
+=back
+
+=item B [I]
+
+Show MBA settings for a certain domain or all domains. For linear mode, it
+shows the decimal value. For non-linear mode, it shows hexadecimal value.
+
+=back
+
 =head1 IGNORED FOR COMPATIBILITY WITH XM
 
 xl is mostly command-line compatible with the old xm utility used with
diff --git a/docs/misc/xl-psr.markdown b/docs/misc/xl-psr.markdown
index 04dd957..3d196ed 100644
--- a/docs/misc/xl-psr.markdown
+++ b/docs/misc/xl-psr.markdown
@@ -186,6 +186,68 @@ Setting data CBM for a domain:
 Setting the same code and data CBM for a domain:
 `xl psr-cat-set  `
 
+## Memory Bandwidth Allocation (MBA)
+
+Memory Bandwidth Allocation (MBA) is a new feature available on Intel
+Skylake and later server platforms that allows an OS or Hypervisor/VMM to
+slow misbehaving apps/VMs by using a credit-based throttling mechanism. To
+enforce bandwidth on a specific domain, just set throttling value (THRTL)
+into Class of Service (COS). MBA provides two THRTL mode. One is linear mode
+and the other is non-linear mode.
+
+In the linear mode the input precision is defined as 100-(THRTL_MAX). Values
+not an even multiple of the precision (e.g., 12%) will be rounded down (e.g.,
+to 10% delay by the hardware).
+
+If linear values are not supported then input delay values are powers-of-two
+from zero to the THRTL_MAX value from CPUID. In this case any values not a 
power
+of two will be rounded down the next nearest power of two.
+
+For example, assuming a system with 2 domains:
+
+ * A THRTL of 0x0 for every domain means each domain can access the whole cache
+   without any delay. This is the default.
+
+ * Linear mode: Giving one domain a THRTL of 0xC and the other domain's 0 means
+   that the first domain gets 10% delay to access the cache and the other one
+   without any delay.
+
+ * Non-linear mode: Giving one domain a THRTL of 0xC and the other domain's 0
+   means that the first domain gets 8% delay to access the cache and the other
+   one without any delay.
+
+For more detailed information please refer to Intel SDM chapter
+"Introduction to Memory Bandwidth Allocation".
+
+In Xen's implementation, THRTL can be configured with libxl/xl interfaces but
+COS is maintained in hypervisor only. The cache partition granularity is per
+domain, each domain has COS=0 assigned by default, the corresponding THRTL is
+0, which means all the cache resource can be accessed without delay.
+
+### xl interfaces
+
+System MBA information such as maximum COS and maximum THRTL can be obtained 
by:
+
+`xl psr-hwinfo --mba`
+
+The simplest way to change a domain's THRTL from its default is running:
+
+`xl psr-mba-set  [OPTIONS]  `
+
+In a multi-socket system, the same thrtl will be set on each socket by default.
+Per socket thrtl can be specified with the `--socket SOCKET` option.
+
+Setting the THRTL may not be successful if insufficient COS is available. In
+such case unused COS(es) may be freed by setting THRTL of all related domains 
to
+its default value(0).
+
+Per domain THRTL settings can be shown 

[Xen-devel] [PATCH v7 06/16] x86: implement get hw info flow for MBA

[Yi Sun]

This patch implements get HW info flow for MBA including its callback
function and sysctl interface.

Signed-off-by: Yi Sun 
Reviewed-by: Roger Pau Monné 
Acked-by: Jan Beulich 
---
CC: Jan Beulich 
CC: Andrew Cooper 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Chao Peng 

v7:
- change 'PSR_INFO_IDX_MBA_FLAG' to 'PSR_INFO_IDX_MBA_FLAGS'.
  (suggested by Jan Beulich)
v5:
- use ASSERT in 'mba_get_feat_info'.
  (suggested by Roger Pau Monné)
- correct initialization format of 'data[PSR_INFO_ARRAY_SIZE]'.
  (suggested by Roger Pau Monné and Jan Beulich)
v4:
- remove 'ALLOC_' from macro names.
  (suggested by Roger Pau Monné)
- initialize 'data[PSR_INFO_ARRAY_SIZE]' to 0 to prevent to leak stack data.
  (suggested by Roger Pau Monné)
v3:
- replace 'PSR_VAL_TYPE_MBA' to 'PSR_TYPE_MBA_THRTL'.
  (suggested by Roger Pau Monné)
v2:
- use 'XEN_SYSCTL_PSR_MBA_LINEAR' to set MBA feature HW info.
  (suggested by Chao Peng)
v1:
- sort 'PSR_INFO_IDX_' macros as feature.
  (suggested by Chao Peng)
- rename 'PSR_INFO_IDX_MBA_LINEAR' to 'PSR_INFO_IDX_MBA_FLAG'.
- rename 'linear' in 'struct mba_info' to 'flags' for future extension.
  (suggested by Chao Peng)
---
 xen/arch/x86/psr.c  | 14 +-
 xen/arch/x86/sysctl.c   | 21 -
 xen/include/asm-x86/psr.h   |  2 ++
 xen/include/public/sysctl.h |  8 
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 64d30b9..f5b395b 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -263,6 +263,10 @@ static enum psr_feat_type psr_type_to_feat_type(enum 
psr_type type)
 feat_type = FEAT_TYPE_L2_CAT;
 break;
 
+case PSR_TYPE_MBA_THRTL:
+feat_type = FEAT_TYPE_MBA;
+break;
+
 default:
 ASSERT_UNREACHABLE();
 }
@@ -483,7 +487,15 @@ static const struct feat_props l2_cat_props = {
 static bool mba_get_feat_info(const struct feat_node *feat,
   uint32_t data[], unsigned int array_len)
 {
-return false;
+ASSERT(array_len == PSR_INFO_ARRAY_SIZE);
+
+data[PSR_INFO_IDX_COS_MAX] = feat->cos_max;
+data[PSR_INFO_IDX_MBA_THRTL_MAX] = feat->mba.thrtl_max;
+
+if ( feat->mba.linear )
+data[PSR_INFO_IDX_MBA_FLAGS] |= XEN_SYSCTL_PSR_MBA_LINEAR;
+
+return true;
 }
 
 static void mba_write_msr(unsigned int cos, uint32_t val,
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 6d48cac..ffad585 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -174,7 +174,7 @@ long arch_do_sysctl(
 case XEN_SYSCTL_psr_alloc:
 switch ( sysctl->u.psr_alloc.cmd )
 {
-uint32_t data[PSR_INFO_ARRAY_SIZE];
+uint32_t data[PSR_INFO_ARRAY_SIZE] = { };
 
 case XEN_SYSCTL_PSR_get_l3_info:
 {
@@ -214,6 +214,25 @@ long arch_do_sysctl(
 break;
 }
 
+case XEN_SYSCTL_PSR_get_mba_info:
+{
+ret = psr_get_info(sysctl->u.psr_alloc.target,
+   PSR_TYPE_MBA_THRTL, data, ARRAY_SIZE(data));
+if ( ret )
+break;
+
+sysctl->u.psr_alloc.u.mba_info.cos_max =
+  data[PSR_INFO_IDX_COS_MAX];
+sysctl->u.psr_alloc.u.mba_info.thrtl_max =
+  data[PSR_INFO_IDX_MBA_THRTL_MAX];
+sysctl->u.psr_alloc.u.mba_info.flags =
+  data[PSR_INFO_IDX_MBA_FLAGS];
+
+if ( __copy_field_to_guest(u_sysctl, sysctl, u.psr_alloc) )
+ret = -EFAULT;
+break;
+}
+
 default:
 ret = -EOPNOTSUPP;
 break;
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index 3cf544a..c2257da 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -39,6 +39,8 @@
 #define PSR_INFO_IDX_COS_MAX0
 #define PSR_INFO_IDX_CAT_CBM_LEN1
 #define PSR_INFO_IDX_CAT_FLAGS  2
+#define PSR_INFO_IDX_MBA_THRTL_MAX  1
+#define PSR_INFO_IDX_MBA_FLAGS  2
 #define PSR_INFO_ARRAY_SIZE 3
 
 struct psr_cmt_l3 {
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index a50e345..f7f26c3 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -698,6 +698,7 @@ struct xen_sysctl_pcitopoinfo {
 
 #define XEN_SYSCTL_PSR_get_l3_info   0
 #define XEN_SYSCTL_PSR_get_l2_info   1
+#define XEN_SYSCTL_PSR_get_mba_info  2
 struct xen_sysctl_psr_alloc {
 uint32_t cmd;   /* IN: XEN_SYSCTL_PSR_* */
 uint32_t target;/* IN */
@@ -708,6 +709,13 @@ struct xen_sysctl_psr_alloc {
 #define 

[Xen-devel] [PATCH v7 07/16] x86: implement get value interface for MBA

[Yi Sun]

This patch implements get value domctl interface for MBA.

Signed-off-by: Yi Sun 
Reviewed-by: Roger Pau Monné 
Acked-by: Jan Beulich 
---
CC: Andrew Cooper 
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Chao Peng 

v5:
- use newly defined macro to get MBA thrtl.
  (suggested by Roger Pau Monné)
v4:
- remove 'ALLOC_' from macro names.
  (suggested by Roger Pau Monné)
v3:
- change 'PSR_VAL_TYPE_MBA' to 'PSR_TYPE_MBA_THRTL'.
  (suggested by Roger Pau Monné)
---
 xen/arch/x86/domctl.c   | 4 
 xen/include/public/domctl.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ae06627..ffb038c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1491,6 +1491,10 @@ long arch_do_domctl(
 ret = domctl_psr_get_val(d, domctl, PSR_TYPE_L2_CBM, copyback);
 break;
 
+case XEN_DOMCTL_PSR_GET_MBA_THRTL:
+ret = domctl_psr_get_val(d, domctl, PSR_TYPE_MBA_THRTL, copyback);
+break;
+
 #undef domctl_psr_get_val
 
 default:
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index c099334..e8f4c4c 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1069,6 +1069,7 @@ struct xen_domctl_psr_alloc {
 #define XEN_DOMCTL_PSR_GET_L3_DATA5
 #define XEN_DOMCTL_PSR_SET_L2_CBM 6
 #define XEN_DOMCTL_PSR_GET_L2_CBM 7
+#define XEN_DOMCTL_PSR_GET_MBA_THRTL  9
 uint32_t cmd;   /* IN: XEN_DOMCTL_PSR_* */
 uint32_t target;/* IN */
 uint64_t data;  /* IN/OUT */
-- 
1.9.1


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

[Xen-devel] [PATCH v7 08/16] x86: implement set value flow for MBA

[Yi Sun]

This patch implements set value flow for MBA including its callback
function and domctl interface.

Signed-off-by: Yi Sun 
---
CC: Jan Beulich 
CC: Andrew Cooper 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Chao Peng 

v7:
- change name of 'check_val' to 'sanitize'.
  (suggested by Jan Beulich)
- fix comments.
  (suggested by Jan Beulich)
- add parentheses and change '== 0' to '!'.
  (suggested by Jan Beulich)
- remove unnecessary check of 'mba.thrtl_max'.
  (suggested by Jan Beulich)
- remove unnecessary intermediate variable 'mod'.
  (suggested by Jan Beulich)
- refine an assignement sentence to use '&='.
  (suggested by Jan Beulich)
- change type of last parameter of 'sanitize' to 'uint32_t' and
  apply same change to 'cat_check_cbm'.
  (suggested by Jan Beulich)
v6:
- split co-exist features' values setting flow to a new patch.
  (suggested by Jan Beulich)
- restore codes related to 'mba_check_thrtl' and 'check_value'.
  (suggested by Jan Beulich)
v5:
- adjust position of 'cat_check_cbm' to not to make changes so big.
  (suggested by Roger Pau Monné)
- remove 'props' from 'struct cos_write_info'.
  (suggested by Roger Pau Monné)
- make a single return statement in 'mba_check_thrtl'.
  (suggested by Jan Beulich)
v4:
- remove 'ALLOC_' from macro names.
  (suggested by Roger Pau Monné)
- join two checks into a single if.
  (suggested by Roger Pau Monné)
- remove redundant local variable 'array_len'.
  (suggested by Roger Pau Monné)
v3:
- modify commit message to make it clear.
  (suggested by Roger Pau Monné)
- modify functionality of 'check_val' to make it simple to only check value.
  Change the last parameter type from 'unsigned long *' to 'unsigned long'.
  (suggested by Roger Pau Monné)
- call rdmsrl to get value just written into MSR for MBA. Because HW can
  automatically change input value to what it wants.
  (suggested by Roger Pau Monné)
- change type of 'write_msr' to 'uint32_t' to return the value actually
  written into MSR. Then, change 'do_write_psr_msrs' to set the returned
  value into 'cos_reg_val[]'
- move the declaration of 'j' into loop in 'do_write_psr_msrs'.
  (suggested by Roger Pau Monné)
- change 'mba_info' to 'mba'.
  (suggested by Roger Pau Monné)
- change 'cat_info' to 'cat'.
  (suggested by Roger Pau Monné)
- rename 'psr_cat/PSR_CAT' to 'psr_alloc/PSR_ALLOC' and remove 'op/OP'
  from name.
  (suggested by Roger Pau Monné)
- change 'PSR_VAL_TYPE_MBA' to 'PSR_TYPE_MBA_THRTL'.
  (suggested by Roger Pau Monné)
v2:
- remove linear mode 'thrtl_max' check in 'mba_check_thrtl' because it has
  been checked in 'mba_init_feature'.
  (suggested by Chao Peng)
- for non-linear mode, check if '*thrtl' is not 0 in 'mba_check_thrtl'. If
  it is 0, we do not need to change it.
  (suggested by Chao Peng)
- move comments to explain changes of 'cos_write_info' from psr.c to commit
  message.
  (suggested by Chao Peng)
---
 xen/arch/x86/domctl.c   |  6 +
 xen/arch/x86/psr.c  | 58 ++---
 xen/include/public/domctl.h |  1 +
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ffb038c..e95004b 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1465,6 +1465,12 @@ long arch_do_domctl(
   PSR_TYPE_L2_CBM);
 break;
 
+case XEN_DOMCTL_PSR_SET_MBA_THRTL:
+ret = psr_set_val(d, domctl->u.psr_alloc.target,
+  domctl->u.psr_alloc.data,
+  PSR_TYPE_MBA_THRTL);
+break;
+
 #define domctl_psr_get_val(d, domctl, type, copyback) ({\
 uint32_t v_;\
 int r_ = psr_get_val((d), (domctl)->u.psr_alloc.target, \
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index f5b395b..c80ba25 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -138,6 +138,12 @@ static const struct feat_props {
 
 /* write_msr is used to write out feature MSR register. */
 void (*write_msr)(unsigned int cos, uint32_t val, enum psr_type type);
+
+/*
+ * sanitize is used to check if input val fulfills SDM requirement.
+ * And change it to valid value if SDM allows.
+ */
+bool (*sanitize)(const struct feat_node *feat, uint32_t *val);
 } *feat_props[FEAT_TYPE_NUM];
 
 /*
@@ -274,16 +280,18 @@ static enum psr_feat_type psr_type_to_feat_type(enum 
psr_type type)
 return feat_type;
 }
 
-static bool psr_check_cbm(unsigned int cbm_len, unsigned long cbm)
+/* Implementation of allocation features' functions. */
+static 

[Xen-devel] [PATCH v7 01/16] docs: create Memory Bandwidth Allocation (MBA) feature document

[Yi Sun]

This patch creates MBA feature document in doc/features/. It describes
key points to implement MBA which is described in details in Intel SDM
"Introduction to Memory Bandwidth Allocation".

Signed-off-by: Yi Sun 
Reviewed-by: Roger Pau Monné 
---
CC: Jan Beulich 
CC: Andrew Cooper 
CC: Wei Liu 
CC: Ian Jackson 
CC: Daniel De Graaf 
CC: Roger Pau Monné 
CC: Konrad Rzeszutek Wilk 
CC: Chao Peng 
CC: Julien Grall 

v6:
- fix some words.
  (suggested by Roger Pau Monné)
v5:
- correct some words.
  (suggested by Roger Pau Monné)
- change 'xl psr-mba-set 1 0xa' to 'xl psr-mba-set 1 10'.
  (suggested by Roger Pau Monné)
v4:
- add 'domain-name' as parameter of 'psr-mba-show/psr-mba-set'.
  (suggested by Roger Pau Monné)
- fix some wordings.
  (suggested by Roger Pau Monné)
- explain how user can know the MBA_MAX.
  (suggested by Roger Pau Monné)
- move the description of 'Linear mode/Non-linear mode' into section
  of 'psr-mba-show'.
  (suggested by Roger Pau Monné)
- change 'per-thread' to 'per-hyper-thread' to make it clearer.
  (suggested by Roger Pau Monné)
- upgrade revision number.
v3:
- remove 'closed-loop' related description.
  (suggested by Roger Pau Monné)
- explain 'linear' and 'non-linear' before mentioning them.
  (suggested by Roger Pau Monné)
- adjust desription of 'psr-mba-set'.
  (suggested by Roger Pau Monné)
- explain 'MBA_MAX'.
  (suggested by Roger Pau Monné)
- remove 'n<64'.
  (suggested by Roger Pau Monné)
- fix some wordings.
  (suggested by Roger Pau Monné)
- add context in 'Testing' part to make things more clear.
  (suggested by Roger Pau Monné)
v2:
- declare 'HW' in Terminology.
  (suggested by Chao Peng)
- replace 'COS ID of VCPU' to 'COS ID of domain'.
  (suggested by Chao Peng)
- replace 'COS register' to 'Thrtl MSR'.
  (suggested by Chao Peng)
- add description for 'psr-mba-show' to state that the decimal value is
  shown for linear mode but hexadecimal value is shown for non-linear mode.
  (suggested by Chao Peng)
- remove content in 'Areas for improvement'.
  (suggested by Chao Peng)
- use '<>' to specify mandatory argument to a command.
  (suggested by Wei Liu)
v1:
- remove a special character to avoid the error when building pandoc.
---
 docs/features/intel_psr_mba.pandoc | 297 +
 1 file changed, 297 insertions(+)
 create mode 100644 docs/features/intel_psr_mba.pandoc

diff --git a/docs/features/intel_psr_mba.pandoc 
b/docs/features/intel_psr_mba.pandoc
new file mode 100644
index 000..86df661
--- /dev/null
+++ b/docs/features/intel_psr_mba.pandoc
@@ -0,0 +1,297 @@
+% Intel Memory Bandwidth Allocation (MBA) Feature
+% Revision 1.8
+
+\clearpage
+
+# Basics
+
+ 
+ Status: **Tech Preview**
+
+Architecture(s): Intel x86
+
+   Component(s): Hypervisor, toolstack
+
+   Hardware: MBA is supported on Skylake Server and beyond
+ 
+
+# Terminology
+
+* CAT Cache Allocation Technology
+* CBM Capacity BitMasks
+* CDP Code and Data Prioritization
+* COS/CLOSClass of Service
+* HW  Hardware
+* MBA Memory Bandwidth Allocation
+* MSRsMachine Specific Registers
+* PSR Intel Platform Shared Resource
+* THRTL   Throttle value or delay value
+
+# Overview
+
+The Memory Bandwidth Allocation (MBA) feature provides indirect and approximate
+control over memory bandwidth available per-core. This feature provides OS/
+hypervisor the ability to slow misbehaving apps/domains by using a credit-based
+throttling mechanism.
+
+# User details
+
+* Feature Enabling:
+
+  Add "psr=mba" to boot line parameter to enable MBA feature.
+
+* xl interfaces:
+
+  1. `psr-mba-show [domain-id|domain-name]`:
+
+ Show memory bandwidth throttling for domain. Under different modes, it
+ shows different type of data.
+
+ There are two modes:
+ Linear mode: the input precision is defined as 100-(MBA_MAX). For 
instance,
+ if the MBA_MAX value is 90, the input precision is 10%. Values not an even
+ multiple of the precision (e.g., 12%) will be rounded down (e.g., to 10%
+ delay applied) by HW automatically. The response of throttling value is
+ linear.
+
+ Non-linear mode: input delay values are powers-of-two from zero to the
+ MBA_MAX value from CPUID. In this case any values not a power of two will
+ be rounded down the next nearest power of two by HW automatically. The
+ response of 

[Xen-devel] [PATCH v7 13/16] tools: rename 'xc_psr_cat_type' to 'xc_psr_type'

[Yi Sun]

This patch renames 'xc_psr_cat_type' to 'xc_psr_type' so that
the structure name is common for all allocation features.

Signed-off-by: Yi Sun 
Acked-by: Wei Liu 
Reviewed-by: Chao Peng 
Reviewed-by: Roger Pau Monné 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Chao Peng 

v5:
- remove a duplicated ';'.
  (suggested by Roger Pau Monné)
v4:
- move assignment of xc_type to its declaration place.
  (suggested by Roger Pau Monné)
v3:
- change 'xc_psr_val_type' to 'xc_psr_type'.
  (suggested by Roger Pau Monné)
---
 tools/libxc/include/xenctrl.h |  8 
 tools/libxc/xc_psr.c  |  4 ++--
 tools/libxl/libxl_psr.c   | 11 +--
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2d977c8..2736bc5 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2466,13 +2466,13 @@ enum xc_psr_cmt_type {
 };
 typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 
-enum xc_psr_cat_type {
+enum xc_psr_type {
 XC_PSR_CAT_L3_CBM  = 1,
 XC_PSR_CAT_L3_CBM_CODE = 2,
 XC_PSR_CAT_L3_CBM_DATA = 3,
 XC_PSR_CAT_L2_CBM  = 4,
 };
-typedef enum xc_psr_cat_type xc_psr_cat_type;
+typedef enum xc_psr_type xc_psr_type;
 
 enum xc_psr_feat_type {
 XC_PSR_CAT_L3,
@@ -2512,10 +2512,10 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t 
rmid, uint32_t cpu,
 int xc_psr_cmt_enabled(xc_interface *xch);
 
 int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
-   xc_psr_cat_type type, uint32_t target,
+   xc_psr_type type, uint32_t target,
uint64_t data);
 int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
-   xc_psr_cat_type type, uint32_t target,
+   xc_psr_type type, uint32_t target,
uint64_t *data);
 int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
xc_psr_feat_type type, xc_psr_hw_info *hw_info);
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index 2c605a7..01f4ba7 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -249,7 +249,7 @@ int xc_psr_cmt_enabled(xc_interface *xch)
 return 0;
 }
 int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
-   xc_psr_cat_type type, uint32_t target,
+   xc_psr_type type, uint32_t target,
uint64_t data)
 {
 DECLARE_DOMCTL;
@@ -284,7 +284,7 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t 
domid,
 }
 
 int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
-   xc_psr_cat_type type, uint32_t target,
+   xc_psr_type type, uint32_t target,
uint64_t *data)
 {
 int rc;
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index b053abd..c54cb6f 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -303,11 +303,11 @@ out:
 return rc;
 }
 
-static inline xc_psr_cat_type libxl__psr_cbm_type_to_libxc_psr_cat_type(
+static inline xc_psr_type libxl__psr_cbm_type_to_libxc_psr_type(
 libxl_psr_cbm_type type)
 {
-BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
-return (xc_psr_cat_type)type;
+BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_type));
+return (xc_psr_type)type;
 }
 
 int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
@@ -325,12 +325,11 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
 }
 
 libxl_for_each_set_bit(socketid, *target_map) {
-xc_psr_cat_type xc_type;
+xc_psr_type xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
 
 if (socketid >= nr_sockets)
 break;
 
-xc_type = libxl__psr_cbm_type_to_libxc_psr_cat_type(type);
 if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type,
socketid, cbm)) {
 libxl__psr_cat_log_err_msg(gc, errno);
@@ -349,7 +348,7 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
 {
 GC_INIT(ctx);
 int rc = 0;
-xc_psr_cat_type xc_type = libxl__psr_cbm_type_to_libxc_psr_cat_type(type);
+xc_psr_type xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
 
 if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
target, cbm_r)) {
-- 
1.9.1


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