Re: [Xen-devel] [PATCH v4] x86: psr: support co-exist features' values setting
On Wed, 2017-10-11 at 09:55 +0800, Yi Sun wrote: > The whole value array is transferred into 'do_write_psr_msrs'. Then, > we can > write all features values on the cos id into MSRs. > > Because multiple features may co-exist, we need handle all features to > write > values of them into a COS register with new COS ID. E.g: > 1. L3 CAT and L2 CAT co-exist. > 2. Dom1 and Dom2 share the same COS ID (2). The L3 CAT CBM of Dom1 is > 0x1ff, > the L2 CAT CBM of Dom1 is 0x1f. > 3. User wants to change L2 CBM of Dom1 to be 0xf. Because COS ID 2 is > used by Dom2 too, we have to pick a new COS ID 3. The values of > Dom1 on > COS ID 3 are all default values as below: > - > | COS 3 | > - > L3 CAT | 0x7ff | > - > L2 CAT | 0xff | > - > 4. After setting, the L3 CAT CBM value of Dom1 should be kept and the > new L2 > CAT CBM is set. So, the values on COS ID 3 should be below. > - > | COS 3 | > - > L3 CAT | 0x1ff | > - > L2 CAT | 0xf | > - > > Signed-off-by: Yi Sun > --- > CC: Jan Beulich > CC: Andrew Cooper > CC: Wei Liu > CC: Roger Pau Monné > CC: Julien Grall > > v4: > - remove init of 'result'. > (suggested by Roger Pau Monné) > - remove 'features' in 'cos_write_info' and get socket info in > 'do_write_psr_msrs' to get features array. > (suggested by Jan Beulich) > - fix a typo in commit message. > (suggested by Kent R. Spillner) > v3: > - add 'result' in 'cos_write_info' to return error code. > (suggested by Roger Pau Monné) > v2: > - fix issues in commit message. > (suggested by Roger Pau Monné) > - remove unnecessary local variable 'val_array'. > (suggested by Roger Pau Monné) > --- > xen/arch/x86/psr.c | 62 +++ > --- > 1 file changed, 36 insertions(+), 26 deletions(-) > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c > index daa2aeb..a812124 100644 > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -,25 +,48 @@ static unsigned int get_socket_cpu(unsigned > int socket) > struct cos_write_info > { > unsigned int cos; > -struct feat_node *feature; > const uint32_t *val; > -const struct feat_props *props; > +unsigned int array_len; > +int result; > }; > > static void do_write_psr_msrs(void *data) > { > -const struct cos_write_info *info = data; > -struct feat_node *feat = info->feature; > -const struct feat_props *props = info->props; > -unsigned int i, cos = info->cos, cos_num = props->cos_num; > +struct cos_write_info *info = data; > +unsigned int i, index = 0, cos = info->cos; > +struct psr_socket_info *socket_info = > +get_socket_info(cpu_to_socket(smp_process > or_id())); > > -for ( i = 0; i < cos_num; i++ ) > +/* > + * Iterate all featuers to write different value (not same as > MSR) for > + * each feature. > + */ > +for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) > { > -if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) > +struct feat_node *feat = socket_info->features[i]; > +const struct feat_props *props = feat_props[i]; > +unsigned int cos_num, j; > + > +if ( !feat || !props ) > +continue; > + > +cos_num = props->cos_num; > +if ( info->array_len < index + cos_num ) > +{ > +info->result = -ENOSPC; > +return; This will have side effect (You may have run write_msr for some features already) when you return the error. It probably will not cause logic error here, there is performance penalty however (writing MSR and sending IPI). Another thing is this error is a real error that we want to propagate to user? E.g, I don't quite understand in which case the condition can happen? If this is only a program error then ASSERT can be used. Chao > +} > + > +for ( j = 0; j < cos_num; j++ ) > { > -feat->cos_reg_val[cos * cos_num + i] = info->val[i]; > -props->write_msr(cos, info->val[i], props->type[i]); > +if ( feat->cos_reg_val[cos * cos_num + j] != info- > >val[index + j] ) > +{ > +feat->cos_reg_val[cos * cos_num + j] = info- > >val[index + j]; > +props->write_msr(cos, info->val[index + j], props- > >type[j]); > +} > } > + > +index += cos_num; > } > } > > @@ -1137,30 +1160,17 @@ static int write_psr_msrs(unsigned int socket, > unsigned int cos, > const uint32_t val[], unsigned int > array_len, > enum psr_feat_type feat_type) > { > -int ret; > struct psr_socket_info *info = get_socket_info(socket); > struct cos_write_info data = > { >
[Xen-devel] [Intel-gfx] [GVT-g] [ANNOUNCE] 2017-Q3 release of XenGT (Intel GVT-g for Xen)
Hi all, We are pleased to announce an update of Intel GVT-g for Xen. Intel GVT-g is a full GPU virtualization solution with mediated pass-through, starting from 4th generation Intel Core(TM) processors with Intel processor graphics. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. GVT-g for Xen hypervisor is XenGT. Repositories - Xen : https://github.com/01org/igvtg-xen (tag: 2017-q3-xengt-stable-4.9) - Kernel: https://github.com/01org/gvt-linux/ (tag: 2017-q3-gvt-stable-4.12) - Qemu: https://github.com/01org/igvtg-qemu (tag: 2017-q3-stable-2.9.0) This update consists of: - Kernel version upgraded to 4.12 from 4.11. - Live migration feature preliminary supported. - QoS feature preliminary supported. - IOMMU feature supported. - OVMF feature supported. - VGPU reset feature optimization, with related issues be fixed. - Supported server platforms: Intel(r) Xeon(r) E3_v4, E3_v5 and E3_v6 with Intel Graphics processor, E3_v6 is new supported platform. - Supported client platforms: Intel(r) Core(tm) 5th generation (code name: Broadwell), 6th generation (code name: Skylake) and 7th generation (code name: Kabylake), 7th generation is new supported platform. - Validated Guest OS: Windows7 32bit, Window7 64bit, Windows8.1 64bit, Windows10 64bit and Linux. - GVT-g only supports remote display not local display by this release. - Remote protocol: only guest-side remoting protocol is supported, host-side remoting connection like SPICE is working in progress. For example, user can use X11VNC for Guest Linux VM or TightVNC for Guest Windows VM. Limitation or known issues: - GVT-g can support maximum 7 Guest VMs due to host graphics resource limitation. When user runs 7 VMs simultaneously, host OS can only run in text mode. - In order to support Guest Windows7 32bit VM, user is recommended to configure vgt_low_gm_sz=128 / 256 / 512 in HVM file because Guest Windows7 32bit VM needs more graphics resource than other Guest VM. - In order to support Guest VM high resolution and screen resolution adjustable in Guest Windows8.1 64bit VM and Guest Windows10 64bit VM, user is recommended to configure vgt_low_gm_sz=64 / 128 / 256 / 512 in HVM file to get larger VM aperture size. - Some 3rd party applications/tools like 3DMark which including special DirectX12 feature test ,it will trigger Guest VM GPU reset. - In corner case, Guest Windows 7 32bit VM may be killed automatically by Xen when Guest VM runs into TDR. This issues happens only on Broadwell platform. The workaround is to disable part of viridian feature in Guest VM hvm file by adding viridian=["all", "!apic_assist"]. - In corner case, Linux Guest VM may GPU hang while running special Intel-GPU-Tools test case on it. - For live migration feature, we cannot migrate Guest Windows VM when Guest VM memory is 2048M or 4096M, user is recommended to configure Guest VM memory to 1024MB. Setup guide: https://github.com/01org/gvt-linux/wiki/GVTg_Setup_Guide This is the first GVT-g community release based on new Upstream architecture design, refer to the following document for new architecture introduction: https://01.org/igvt-g/documentation/intel-gvt-g-new-architecture-introduction Please subscribe to join the mailing list if you want to learn more about GVT-g project: https://lists.01.org/mailman/listinfo/igvt-g Please subscribe to join the mailing list if you want to contribute/review latest GVT-g upstream patches: https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev Official GVT-g portal: https://01.org/igvt-g More information about background, architecture and others about Intel GVT-g, can be found at: https://01.org/igvt-g https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20Summit-v7_0.pdf http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20Summit-REWRITE%203RD%20v4.pdf https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt Note: The XenGT project should be considered a work in progress. As such it is not a complete product nor should it be considered one. Extra care should be taken when testing and configuring a system to use the XenGT project. Thanks Terrence Tel: +86-21-6116 5390 MP: +86-1356 4367 024 Mail: terrence...@intel.com ___ GVT-g mailing list igv...@lists.01.org https://lists.01.org/mailman/listinfo/igvt-g ___ Intel-gfx mailing list intel-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx __
Re: [Xen-devel] [PATCH v4] x86: psr: support co-exist features' values setting
On 17-10-11 14:59:23, Chao Peng wrote: > On Wed, 2017-10-11 at 09:55 +0800, Yi Sun wrote: > > static void do_write_psr_msrs(void *data) > > { > > -const struct cos_write_info *info = data; > > -struct feat_node *feat = info->feature; > > -const struct feat_props *props = info->props; > > -unsigned int i, cos = info->cos, cos_num = props->cos_num; > > +struct cos_write_info *info = data; > > +unsigned int i, index = 0, cos = info->cos; > > +struct psr_socket_info *socket_info = > > +get_socket_info(cpu_to_socket(smp_process > > or_id())); > > > > -for ( i = 0; i < cos_num; i++ ) > > +/* > > + * Iterate all featuers to write different value (not same as > > MSR) for > > + * each feature. > > + */ > > +for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) > > { > > -if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) > > +struct feat_node *feat = socket_info->features[i]; > > +const struct feat_props *props = feat_props[i]; > > +unsigned int cos_num, j; > > + > > +if ( !feat || !props ) > > +continue; > > + > > +cos_num = props->cos_num; > > +if ( info->array_len < index + cos_num ) > > +{ > > +info->result = -ENOSPC; > > +return; > > This will have side effect (You may have run write_msr for some features > already) when you return the error. It probably will not cause logic > error here, there is performance penalty however (writing MSR and > sending IPI). > > Another thing is this error is a real error that we want to propagate to > user? E.g, I don't quite understand in which case the condition can > happen? If this is only a program error then ASSERT can be used. > Thanks! If error happens, this error is a program error. So, an ASSERT here is better. > Chao ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 114332: regressions - FAIL
flight 114332 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114332/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl 7 xen-boot fail REGR. vs. 114299 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass version targeted for testing: xen f17d2cd2ffeda70aba8788910e9d088415562c8b baseline version: xen 3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a Last test of basis 114299 2017-10-10 21:02:54 Z0 days Failing since114308 2017-10-10 23:01:10 Z0 days3 attempts Testing same since 114318 2017-10-11 02:19:34 Z0 days2 attempts People who touched revisions under test: Andre Przywara Julien Grall Stefano Stabellini jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl fail 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 Not pushing. commit f17d2cd2ffeda70aba8788910e9d088415562c8b Author: Andre Przywara Date: Sat Oct 7 01:06:40 2017 +0100 ARM: sunxi: support more Allwinner SoCs So far we only supported the Allwinner A20 SoC. Add support for most of the other virtualization capable Allwinner SoCs by: - supporting the watchdog in newer (sun8i) SoCs - getting the watchdog address from DT - adding compatible strings for other 32-bit SoCs - adding compatible strings for 64-bit SoCs As all 64-bit SoCs support system reset via PSCI, we don't use the platform specific reset routine there. Should the 32-bit SoCs start to properly support the PSCI 0.2 SYSTEM_RESET call, we will use it for them automatically, as we try PSCI first, then fall back to platform reset. Signed-off-by: Andre Przywara Signed-off-by: Stefano Stabellini Reviewed-by: Stefano Stabellini commit e2dfe4a037b0c6ccfd2375e4b60668109a0118e5 Author: Julien Grall Date: Mon Oct 9 14:23:41 2017 +0100 xen/arm: mm: Use memory flags for modify_xen_mappings rather than custom one This will help to consolidate the page-table code and avoid different path depending on the action to perform. Signed-off-by: Julien Grall Reviewed-by: Andre Przywara Reviewed-by: Stefano Stabellini Reviewed-by: Konrad Rzeszutek Wilk commit 6b88beed40c756aaff018d286f4de31351240a93 Author: Julien Grall Date: Mon Oct 9 14:23:40 2017 +0100 xen/arm: mm: Handle permission flags when adding a new mapping Currently, all the new mappings will be read-write non-executable. Allow the caller to use other permissions. Signed-off-by: Julien Grall Reviewed-by: Stefano Stabellini commit 28f2ad440a08908010abec43b7ccc3283051e943 Author: Julien Grall Date: Mon Oct 9 14:23:39 2017 +0100 xen/arm: mm: Embed permission in the flags Currently, it is not possible to specify the permission of a new mapping. It would be necessary to use the function modify_xen_mappings with a different set of flags. Introduce a couple of new flags for the permissions (Non-eXecutable, Read-Only) and also provides definition that combine the memory attribute and permission for common combinations. PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write, non-executable mappings). This does not affect the current mapping using PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be non-executable by default (see mfn_to_xen_entry). A follow-up patch will change modify_xen_mappings to use the new flags. Signed-off-by: Julien Grall Reviewed-by: Stefano Stabellini Signed-off-by: Stefano Stabellini commit 5f3edb5f32e511b915d173403d0b7b5ea38e00ad Author: Julien Grall Date: Mon Oct 9 14:23:38 2017 +0100 xen/arm: page: Describe the layout of flags used to update page tables
Re: [Xen-devel] [xen-unstable-smoke test] 114332: regressions - FAIL
Adding Julien and Stefano. On Wed, Oct 11, 2017 at 07:17:59AM +, osstest service owner wrote: > flight 114332 xen-unstable-smoke real [real] > http://logs.test-lab.xenproject.org/osstest/logs/114332/ > > Regressions :-( > > Tests which did not succeed and are blocking, > including tests which could not be run: > test-armhf-armhf-xl 7 xen-boot fail REGR. vs. > 114299 Seems like Xen is not able to start on the Exynos or Cubietrucks anymore: Oct 11 06:52:11.479093 U-Boot 2014.10-7-g0052a7d (Sep 29 2015 - 10:21:38) for ARNDALE Oct 11 06:52:11.487075 Oct 11 06:52:11.487090 CPU:Exynos5250@1000MHz Oct 11 06:52:11.487106 Oct 11 06:52:11.487118 Board: Arndale Oct 11 06:52:11.487132 I2C: i2c_init: failed to init bus 0 for speed = 10 Oct 11 06:52:11.495063 ready Oct 11 06:52:11.495078 DRAM: 2 GiB Oct 11 06:52:11.495092 trace: copying 000ab188 bytes of early data from 5000 to beff Oct 11 06:52:11.511080 trace: enabled Oct 11 06:52:11.551032 MMC: EXYNOS DWMMC: 0, EXYNOS DWMMC: 1 Oct 11 06:52:11.911146 i2c_init: failed to init bus 0 for speed = 10 Oct 11 06:52:11.967154 In:serial Oct 11 06:52:11.967185 Out: serial Oct 11 06:52:11.967211 Err: serial Oct 11 06:52:11.967236 SCSI: ARNDALE SCSI INIT Oct 11 06:52:11.975119 Target spinup took 0 ms. Oct 11 06:52:11.983147 AHCI 0001.0300 32 slots 1 ports 6 Gbps 0x1 impl SATA mode Oct 11 06:52:11.983188 flags: ncq stag pm led clo only pmp pio slum part ccc apst Oct 11 06:52:11.991155 Net: Net Initialization Skipped Oct 11 06:52:11.991189 No ethernet found. Oct 11 06:52:11.991216 Hostname: arndale-westfield Oct 11 06:52:11.999141 Hit any key to stop autoboot: 2 1 0 Oct 11 06:52:13.919168 (Re)start USB... Oct 11 06:52:13.919223 USB0: USB EHCI 1.00 Oct 11 06:52:13.927090 scanning bus 0 for devices... 4 USB Device(s) found Oct 11 06:52:19.523158 scanning usb for storage devices... 0 Storage Device(s) found Oct 11 06:52:19.531073 scanning usb for ethernet devices... 1 Ethernet Device(s) found Oct 11 06:52:19.971149 Waiting for Ethernet connection... done. Oct 11 06:52:21.571140 BOOTP broadcast 1 Oct 11 06:52:21.571189 BOOTP broadcast 2 Oct 11 06:52:22.619159 DHCP client bound to address 172.16.144.45 (1092 ms) Oct 11 06:52:22.627153 Using asx0 device Oct 11 06:52:22.627188 TFTP from server 172.16.144.3; our IP address is 172.16.144.45 Oct 11 06:52:22.635136 Filename 'pxelinux.0'. Oct 11 06:52:22.635157 Load address: 0x43e0 Oct 11 06:52:22.635173 Loading: *## Oct 11 06:52:22.651084 1.8 MiB/s Oct 11 06:52:22.651119 done Oct 11 06:52:22.651144 Bytes transferred = 26474 (676a hex) Oct 11 06:52:22.651177 missing environment variable: pxeuuid Oct 11 06:52:22.659068 Retrieving file: pxelinux.cfg/AC10902D Oct 11 06:52:22.659103 Using asx0 device Oct 11 06:52:22.659161 TFTP from server 172.16.144.3; our IP address is 172.16.144.45 Oct 11 06:52:22.667076 Filename 'pxelinux.cfg/AC10902D'. Oct 11 06:52:22.675051 Load address: 0x5100 Oct 11 06:52:22.675082 Loading: *# Oct 11 06:52:22.675106 15.6 KiB/s Oct 11 06:52:22.675131 done Oct 11 06:52:22.675153 Bytes transferred = 65 (41 hex) Oct 11 06:52:22.683067 Config file found Oct 11 06:52:22.683096 Ignoring unknown command: serial Oct 11 06:52:22.683124 1: local Oct 11 06:52:22.691033 scanning bus for devices... Oct 11 06:52:22.691064 Device 0: (0:0) Vendor: ATA Prod.: HGST HTS545050A7 Rev: GG2O Oct 11 06:52:22.715075 Type: Hard Disk Oct 11 06:52:22.723062 Capacity: 476940.0 MB = 465.7 GB (976773168 x 512) Oct 11 06:52:22.723100 Found 1 device(s). Oct 11 06:52:22.723126 Oct 11 06:52:22.723147 SCSI device 0: Oct 11 06:52:22.731046 Device 0: (0:0) Vendor: ATA Prod.: HGST HTS545050A7 Rev: GG2O Oct 11 06:52:22.731093 Type: Hard Disk Oct 11 06:52:22.739063 Capacity: 476940.0 MB = 465.7 GB (976773168 x 512) Oct 11 06:52:22.739100 ... is now current device Oct 11 06:52:22.747037 Scanning scsi 0... Oct 11 06:52:22.747066 Found U-Boot script /boot.scr Oct 11 06:52:22.907117 1710 bytes read in 34 ms (48.8 KiB/s) Oct 11 06:52:22.947138 ## Executing script at 5000 Oct 11 06:52:22.955124 Loading dtbs/4.9.20+/exynos5250-arndale.dtb Oct 11 06:52:22.955175 43128 bytes read in 1186 ms (35.2 KiB/s) Oct 11 06:52:24.115168 917512 bytes read in 111 ms (7.9 MiB/s) Oct 11 06:52:24.251222 Loaded xen-4.10-unstable to 0x4100 (e0008) Oct 11 06:52:24.259243 command line: conswitch=x watchdog noreboot console=dtuart dtuart=serial2 dom0_mem=512M,max:512M Oct 11 06:52:24.267256 6798760 bytes read in 305 ms (21.3 MiB/s) Oct 11 06:52:24.587101 Loaded vmlinuz-4.9.20+ to 0x4200 (67bda8) Oct 11 06:52:24.595066 command line: ro root=/dev/mapper/arndale--westfield--vg-root rootdelay=3 ro root=/dev/mapper/arndale--westfield--vg-root rootdelay=3 console=hvc0 clk_ignore_unused clk_ignore_unused Oct 11 06:52:24.61
Re: [Xen-devel] [PATCH 1/1] sched/cputime: do not decrease steal time after live migration on xen
Hi Stanislaw and Peter, On 10/10/2017 08:42 PM, Stanislaw Gruszka wrote: > On Tue, Oct 10, 2017 at 12:59:26PM +0200, Ingo Molnar wrote: >> >> (Cc:-ed more gents involved in kernel/sched/cputime.c work. Full patch >> quoted >> below.) >> >> * Dongli Zhang wrote: >> >>> After guest live migration on xen, steal time in /proc/stat >>> (cpustat[CPUTIME_STEAL]) might decrease because steal returned by >>> paravirt_steal_clock() might be less than this_rq()->prev_steal_time. >>> >>> For instance, steal time of each vcpu is 335 before live migration. >>> >>> cpu 198 0 368 200064 1962 0 0 1340 0 0 >>> cpu0 38 0 81 50063 492 0 0 335 0 0 >>> cpu1 65 0 97 49763 634 0 0 335 0 0 >>> cpu2 38 0 81 50098 462 0 0 335 0 0 >>> cpu3 56 0 107 50138 374 0 0 335 0 0 >>> >>> After live migration, steal time is reduced to 312. >>> >>> cpu 200 0 370 200330 1971 0 0 1248 0 0 >>> cpu0 38 0 82 50123 500 0 0 312 0 0 >>> cpu1 65 0 97 49832 634 0 0 312 0 0 >>> cpu2 39 0 82 50167 462 0 0 312 0 0 >>> cpu3 56 0 107 50207 374 0 0 312 0 0 >>> >>> The code in this patch is borrowed from do_stolen_accounting() which has >>> already been removed from linux source code since commit ecb23dc6 ("xen: >>> add steal_clock support on x86"). >>> >>> Similar and more severe issue would impact prior linux 4.8-4.10 as >>> discussed by Michael Las at >>> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest. >>> Unlike the issue discussed by Michael Las which would overflow steal time >>> and lead to 100% st usage in top command for linux 4.8-4.10, the issue for >>> linux 4.11+ would only decrease but not overflow steal time after live >>> migration. >>> >>> References: >>> https://0xstubs.org/debugging-a-flaky-cpu-steal-time-counter-on-a-paravirtualized-xen-guest >>> Signed-off-by: Dongli Zhang >>> --- >>> kernel/sched/cputime.c | 13 ++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c >>> index 14d2dbf..57d09cab 100644 >>> --- a/kernel/sched/cputime.c >>> +++ b/kernel/sched/cputime.c >>> @@ -238,10 +238,17 @@ static __always_inline u64 >>> steal_account_process_time(u64 maxtime) >>> { >>> #ifdef CONFIG_PARAVIRT >>> if (static_key_false(¶virt_steal_enabled)) { >>> - u64 steal; >>> + u64 steal, steal_time; >>> + s64 steal_delta; >>> + >>> + steal_time = paravirt_steal_clock(smp_processor_id()); >>> + steal = steal_delta = steal_time - this_rq()->prev_steal_time; >>> + >>> + if (unlikely(steal_delta < 0)) { >>> + this_rq()->prev_steal_time = steal_time; > > I don't think setting prev_steal_time to smaller value is right > thing to do. If we do not set prev_steal_time to smaller steal (obtained from paravirt_steal_clock()), it will take a while for kernel to wait for new steal to catch up with this_rq()->prev_steal_time, and cpustat[CPUTIME_STEAL] will stay unchanged until steal is more than this_rq()->prev_steal_time again. Do you think it is fine? If it is fine, I will try to limit the fix to xen specific code in driver/xen/time.c so that we would not taint kernel/sched/cputime.c, as Peter has asked why not just fix up paravirt_steal_time() on migration. Thank you very much! Dongli Zhang > > Beside, I don't think we need to check for overflow condition for > cputime variables (it will happen after 279 years :-). So instead > of introducing signed steal_delta variable I would just add > below check, which should be sufficient to fix the problem: > > if (unlikely(steal <= this_rq()->prev_steal_time)) > return 0; > > Thanks > Stanislaw > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [linux-3.18 bisection] complete test-amd64-amd64-xl-pvh-intel
On Wed, Oct 11, 2017 at 04:54:40AM +, osstest service owner wrote: > branch xen-unstable > xenbranch xen-unstable > job test-amd64-amd64-xl-pvh-intel > testid guest-start > > Tree: linux > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git > Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git > Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git > Tree: qemuu git://xenbits.xen.org/qemu-xen.git > Tree: xen git://xenbits.xen.org/xen.git > > *** Found and reproduced problem changeset *** > > Bug is in tree: xen git://xenbits.xen.org/xen.git > Bug introduced: c7dfe4ac58dd9c8678126b78da961b233a49f3f9 > Bug not present: 3c44f8ed44ab559c7e5b58316751bea53adfd83b > Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/114323/ > > > commit c7dfe4ac58dd9c8678126b78da961b233a49f3f9 > Author: Roger Pau Monne > Date: Fri Sep 22 16:25:06 2017 +0100 > > xl: introduce a domain type option > > Introduce a new type option to xl configuration files in order to > specify the domain type. This supersedes the current builder option. > > The new option is documented in the xl.cfg man page, and the previous > builder option is marked as deprecated. > > Signed-off-by: Roger Pau Monn?? > Acked-by: Ian Jackson This branch will have to be force pushed, together with the 4.9 one AFAICT. This test succeed previously because we where testing a classic PV guest instead of a PVH one. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5] x86: psr: support co-exist features' values setting
The whole value array is transferred into 'do_write_psr_msrs'. Then, we can write all features values on the cos id into MSRs. Because multiple features may co-exist, we need handle all features to write values of them into a COS register with new COS ID. E.g: 1. L3 CAT and L2 CAT co-exist. 2. Dom1 and Dom2 share the same COS ID (2). The L3 CAT CBM of Dom1 is 0x1ff, the L2 CAT CBM of Dom1 is 0x1f. 3. User wants to change L2 CBM of Dom1 to be 0xf. Because COS ID 2 is used by Dom2 too, we have to pick a new COS ID 3. The values of Dom1 on COS ID 3 are all default values as below: - | COS 3 | - L3 CAT | 0x7ff | - L2 CAT | 0xff | - 4. After setting, the L3 CAT CBM value of Dom1 should be kept and the new L2 CAT CBM is set. So, the values on COS ID 3 should be below. - | COS 3 | - L3 CAT | 0x1ff | - L2 CAT | 0xf | - Signed-off-by: Yi Sun --- CC: Jan Beulich CC: Andrew Cooper CC: Wei Liu CC: Roger Pau Monné CC: Julien Grall v5: - remove 'result' and use an ASSERT to handle error case. (suggested by Chao Peng) v4: - remove init of 'result'. (suggested by Roger Pau Monné) - remove 'features' in 'cos_write_info' and get socket info in 'do_write_psr_msrs' to get features array. (suggested by Jan Beulich) - fix a typo in commit message. (suggested by Kent R. Spillner) v3: - add 'result' in 'cos_write_info' to return error code. (suggested by Roger Pau Monné) v2: - fix issues in commit message. (suggested by Roger Pau Monné) - remove unnecessary local variable 'val_array'. (suggested by Roger Pau Monné) --- xen/arch/x86/psr.c | 55 +- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index daa2aeb..8936cf7 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -,25 +,43 @@ static unsigned int get_socket_cpu(unsigned int socket) struct cos_write_info { unsigned int cos; -struct feat_node *feature; const uint32_t *val; -const struct feat_props *props; +unsigned int array_len; }; static void do_write_psr_msrs(void *data) { -const struct cos_write_info *info = data; -struct feat_node *feat = info->feature; -const struct feat_props *props = info->props; -unsigned int i, cos = info->cos, cos_num = props->cos_num; +struct cos_write_info *info = data; +unsigned int i, index = 0, cos = info->cos; +struct psr_socket_info *socket_info = +get_socket_info(cpu_to_socket(smp_processor_id())); -for ( i = 0; i < cos_num; i++ ) +/* + * Iterate all featuers to write different value (not same as MSR) for + * each feature. + */ +for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) { -if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) +struct feat_node *feat = socket_info->features[i]; +const struct feat_props *props = feat_props[i]; +unsigned int cos_num, j; + +if ( !feat || !props ) +continue; + +cos_num = props->cos_num; +ASSERT(info->array_len >= index + cos_num); + +for ( j = 0; j < cos_num; j++ ) { -feat->cos_reg_val[cos * cos_num + i] = info->val[i]; -props->write_msr(cos, info->val[i], props->type[i]); +if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index + j] ) +{ +feat->cos_reg_val[cos * cos_num + j] = info->val[index + j]; +props->write_msr(cos, info->val[index + j], props->type[j]); +} } + +index += cos_num; } } @@ -1137,30 +1155,17 @@ static int write_psr_msrs(unsigned int socket, unsigned int cos, const uint32_t val[], unsigned int array_len, enum psr_feat_type feat_type) { -int ret; struct psr_socket_info *info = get_socket_info(socket); struct cos_write_info data = { .cos = cos, -.feature = info->features[feat_type], -.props = feat_props[feat_type], +.val = val, +.array_len = array_len, }; if ( cos > info->features[feat_type]->cos_max ) return -EINVAL; -/* Skip to the feature's value head. */ -ret = skip_prior_features(&array_len, feat_type); -if ( ret < 0 ) -return ret; - -val += ret; - -if ( array_len < feat_props[feat_type]->cos_num ) -return -ENOSPC; - -data.val = val; - if ( socket == cpu_to_socket(smp_processor_id()) ) do_write_psr_msrs(&data); else -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: handle address wrapping for VMASKMOVP{S, D}
>>> On 10.10.17 at 14:43, wrote: > On 10/10/17 11:43, Jan Beulich wrote: >> There's another issue here, but I'll first have to think about possible >> (preferably non-intrusive) solutions: An access crossing a page >> boundary and having >> - a set mask bit corresponding to an element fully living in the first >> page, >> - one or more clear mask bits corresponding to the initial elements on >> the second page, >> - another higher mask bit being set >> would result in a wrong CR2 value to be reported in case the access to >> the second page would cause a fault (it would point to the start of the >> page instead of the element being accessed). Neither splitting the >> access here into multiple ones nor uniformly passing a byte or word >> enable mask into ->write() seem very desirable. > > Is this just supposition, or have you confirmed what really happens on > hardware? I had done some experiments already at the time I wrote this. I've now done more. If the fault occurs on the high page, the CR2 value (on Haswell) depends on whether there was an enabled access to the low page. If so, CR2 holds the address of the last byte of the overall range (even if the highest mask bit is clear). If not, CR2 holds the address of the lowest byte actually being accessed. > I expect that the mask operations turn into multi-part operations, which > means their behaviour on a straddled fault is implementation defined > (and behaves differently between Atoms and Xeons). > > One option we could do is to have a variation of the "Implement > hvmemul_write() using real mappings" logic where we pull mappings into > the vmap individually, but that would require some part of the code to > convert ea + mask => linear address of each unit, so the eventual > mapping can be constructed piece-wise. I certainly have no Atom to play with - on the Haswell, the write to the first page does not take effect when the access to the second page faults. Hence splitting the accesses (as suggested as an option above) clearly would not be valid. On my Dinar (AMD Fam15), otoh, CR2 always points at the lowest byte of the actually accessed field(s) on the page causing the fault (i.e. the behavior I had implied in my original remark to the patch). Perhaps to avoid passing byte/word enables into ->read and ->write() (not sure why originally I had thought of the latter only) we could extend struct pagefault_info to include an "offset from start of accessed range" field, to be passed into x86_emul_pagefault() in addition to the current arguments? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/1] sched/cputime: do not decrease steal time after live migration on xen
Hi Rik, On 10/10/2017 10:01 PM, Rik van Riel wrote: > On Tue, 2017-10-10 at 14:48 +0200, Peter Zijlstra wrote: >> On Tue, Oct 10, 2017 at 02:42:01PM +0200, Stanislaw Gruszka wrote: > + u64 steal, steal_time; > + s64 steal_delta; > + > + steal_time = > paravirt_steal_clock(smp_processor_id()); > + steal = steal_delta = steal_time - this_rq()- >> prev_steal_time; > + > + if (unlikely(steal_delta < 0)) { > + this_rq()->prev_steal_time = > steal_time; >>> >>> I don't think setting prev_steal_time to smaller value is right >>> thing to do. >>> >>> Beside, I don't think we need to check for overflow condition for >>> cputime variables (it will happen after 279 years :-). So instead >>> of introducing signed steal_delta variable I would just add >>> below check, which should be sufficient to fix the problem: >>> >>> if (unlikely(steal <= this_rq()->prev_steal_time)) >>> return 0; >> >> How about you just fix up paravirt_steal_time() on migration and not >> muck with the users ? > > Not just migration, either. CPU hotplug is another time to fix up > the steal time. I think this issue might be hit when we add and online vcpu after a very very long time since boot (or the last time vcpu is offline). Please correct me if I am wrong. Thank you very much! Dongli Zhang > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 27/27 v10] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt
Hi Dave, On 26 September 2017 at 20:08, Dave Martin wrote: > On Fri, Sep 22, 2017 at 01:53:26PM +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 >> --- >> CC: Julien Grall >> CC: Andre Przywara >> CC: Stefano Stabellini > > (Taking a quick look at this because I remember fighthing with FIFO > behaviour issues when hacking the Linux driver -- but beware, I'm not a > Xen guy...) > > > Should this patch be flattened into the patches is fixes? Keeping > known-wrong code in the series does not help reviewers (but maybe it's > the Xen way). > >> 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 > > What's sizeof(intf->in), sizeof(intf->out)? > > For correct operation, you assume that the total ring buffer size is >= > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring > buffer size is set elsewhere and can't be chosen by a driver, you may at > least add a build-time assert to check that it's big enough. > I will add an assert to check this condition. > [...] > >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, >> uint8_t data) > > [...] > >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark >> which >> + * is the trigger level for asserting/de-assterting the TX >> interrupt. >> */ >> -vpl011->uartfr |= BUSY; >> +fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; >> + >> +if ( fifo_level <= fifo_threshold ) >> +vpl011->uartris |= TXI; >> +else >> +vpl011->uartris &= ~TXI; >> } >> +else >> +gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); >> >> vpl011->uartfr &= ~TXFE; >> > > [...] > >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) >> >> smp_rmb(); >> >> -in_ring_qsize = xencons_queued(in_prod, >> +in_fifo_level = xencons_queued(in_prod, >> in_cons, >> sizeof(intf->in)); >> >> -out_ring_qsize = xencons_queued(out_prod, >> +out_fifo_level = xencons_queued(out_prod, >> out_cons, >> sizeof(intf->out)); >> >> /* Update the uart rx state if the buffer is not empty. */ >> -if ( in_ring_qsize != 0 ) >> +if ( in_fifo_level != 0 ) >> { >> vpl011->uartfr &= ~RXFE; >> -if ( in_ring_qsize == sizeof(intf->in) ) >> + >> +if ( in_fifo_level == sizeof(intf->in) ) >> vpl011->uartfr |= RXFF; >> + >> vpl011->uartris |= RXI; >> } >> >> /* Update the uart tx state if the buffer is not full. */ >> -if ( out_ring_qsize != sizeof(intf->out) ) >> +if ( out_fifo_level != sizeof(intf->out) ) >> { >> +unsigned int out_fifo_threshold; >> + >> vpl011->uartfr &= ~TXFF; >> -vpl011->uartris |= TXI; >> >> /* >> - * Clear the BUSY bit as soon as space becomes available >> + * Clear the BUSY bit as soon as space becomes avaliable >> * so that the SBSA UART driver can start writing more data >> * without any further delay. >> */ >> vpl011->uartfr &= ~BUSY; >> >> -if ( out_ring_qsize == 0 ) >> +/* >> + * 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. >> + */ >> +out_fifo_threshold = sizeof(int
Re: [Xen-devel] [PATCH v8 04/11] public: xen.h: add definitions for UUID handling
>>> On 10.10.17 at 19:03, wrote: > On 10.10.17 19:12, Jan Beulich wrote: > On 10.10.17 at 17:52, wrote: >>> +uint8_t a[16]; >>> +} xen_uuid_t; >>> + >>> +/* >>> + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, >>> + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) >>> + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as >>> + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, >>> + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; >>> + * >>> + * NB: This is compatible with Linux kernel and with libuuid, but it is not >>> + * compatible with Microsoft, as they use mixed-endian encoding (some >>> + * components are little-endian, some are big-endian). >>> + */ >>> +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ >>> +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ >>> + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ >>> + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ >>> + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ >>> + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ >>> +e1, e2, e3, e4, e5, e6}} >>> + >>> +/* Compound literals are supported in C99 and later. */ >>> +#if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L >> >> I didn't notice this earlier - why no check for __GNUC__? > I have seen pattern "#if defined (__STDC_VERSION__) && __STDC_VERSION__ But if you check, all of the existing ones have "#elif defined(__GNUC__)". > >= 199901" in various places in XEN, so I did it alike. > Also, I'm using C99 feature, not gcc-only one. I didn't ask you to replace the conditional, but to (possibly) extend it. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
>>> On 10.10.17 at 20:08, wrote: > On 10/10/2017 05:24 PM, Jan Beulich wrote: > On 10.10.17 at 16:13, wrote: >>> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > >> >>> >>> a.u.set_mem_access_multi.pfn_list, > + a.u.set_mem_access_multi.acc > ess_list, > + a.u.set_mem_access_multi.nr, > + a.u.set_mem_access_multi.opa > que, > + MEMOP_CMD_MASK, This not being a memop, re-using this value looks arbitrary. Unless it absolutely has to be this value, please either add a brief comment saying this just happens to fit your need or use a literal constant. >>> I will add a comment to clarify the reason for using MEMOP_CMD_MASK: >>> e.g. >>> /* MEMOP_CMD_MASK is used here to mirror the way >>> p2m_set_mem_access_multi() is called for the >>> XENMEM_access_op_set_access_multi case. */ >> >> But that's neither brief nor an actual reason (if at all it is a cosmetic >> one). A wider or more narrow mask would do, afaict. > > Revisiting the p2m_set_mem_access_multi() code, the mask is used here: > > 447 /* Check for continuation if it's not the last iteration. */ > 448 if ( nr > ++start && !(start & mask) && hypercall_preempt_check() > ) > 449 { > 450 rc = start; > 451 break; > 452 } > > If I'm reading the code correctly, MEMOP_CMD_MASK happens to be > 0011, which allows an interruption in the processing loop > every 64th time we go through it. > > Now, it does indeed make more sense to see MEMOP_CMD_MASK being used > with XENMEM_access_op_set_access_multi than it does with altp2m (where > we're not dealing with memops). However, indeed almost any mask would do > here (0x1f, for example, or 0x7f, or something else entirely). > > The reason I've initially picked MEMOP_CMD_MASK for this patch is that > it had seemed like a reasonable default (currenly made use of with > XENMEM_access_op_set_access_multi). I'm quite likely missing something, > but I don't know what criteria we should use for picking a good value > here, and how to express it. And if we go with the tried and true > MEMOP_CMD_MASK (because it seems to work well), is it not appropriate to > express that intent by using its name in the code? My point is that MEMOP_CMD_MASK can't be changed, i.e. other than the value to be used here it is not arbitrary. > Should we just use it's value directly (i.e. 0x3f)? That's certainly an option. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline test] 114279: regressions - FAIL
flight 114279 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/114279/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-xsm broken in 114194 test-armhf-armhf-xl-credit2 broken in 114194 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 Tests which are failing intermittently (not blocking): test-armhf-armhf-xl-credit2 4 host-install(4) broken in 114194 pass in 114279 test-amd64-amd64-xl-xsm 4 host-install(4) broken in 114194 pass in 114279 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail in 114194 pass in 114279 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail in 114194 pass in 114279 test-amd64-i386-xl-raw 10 debian-di-install fail in 114194 pass in 114279 test-amd64-i386-libvirt-qcow2 10 debian-di-install fail in 114194 pass in 114279 test-armhf-armhf-xl-cubietruck 6 xen-install fail pass in 114194 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-xl-cubietruck 13 migrate-support-check fail in 114194 never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-check fail in 114194 never pass 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-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-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-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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-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: qemuu530049bc1dcc24c1178a29d99ca08b6dd08413e0 baseline version: qemuu5456c6a4ec9cd8fc314ddc303e88bf85c110975c Last test of basis 114042 2017-10-05 12:15:47 Z5 days Failing since114060 2017-10-06 05:53:34 Z5 days6 attempts Testing same since 114083 2017-10-06 19:16:18 Z4 days5 attempts People who touched revisions under test: Alex Williamson Brandon Carpenter Christian Borntraeger Collin L. Walling Cornelia Huck Daniel P. Berrange David Hildenbrand Dr. David Alan Gi
Re: [Xen-devel] [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
>>> On 10.10.17 at 15:26, wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 09 October 2017 14:06 >> >>> On 06.10.17 at 14:25, wrote: >> > --- a/xen/common/memory.c >> > +++ b/xen/common/memory.c >> > @@ -965,6 +965,67 @@ static long xatp_permission_check(struct domain >> *d, unsigned int space) >> > return xsm_add_to_physmap(XSM_TARGET, current->domain, d); >> > } >> > >> > +#ifdef CONFIG_X86 >> >> Could you try to (a) have only a single such #ifdef and (b) reduce >> its scope as much as possible? Even if for now the code is dead on >> ARM, making sure it continues to compile there would be helpful. >> > > Ok. I don't have an arm machine to build on so I can't tell if anything is > broken though. Well, that's the case for most of us x86 people. I use a cross compiler to be able to at least check the ARM build is okay. >> > --- a/xen/include/public/memory.h >> > +++ b/xen/include/public/memory.h >> > @@ -599,6 +599,36 @@ struct xen_reserved_device_memory_map { >> > typedef struct xen_reserved_device_memory_map >> xen_reserved_device_memory_map_t; >> > DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t); >> > >> > +/* >> > + * Get the pages for a particular guest resource, so that they can be >> > + * mapped directly by a tools domain. >> > + */ >> > +#define XENMEM_acquire_resource 28 >> > +struct xen_mem_acquire_resource { >> > +/* IN - the domain whose resource is to be mapped */ >> > +domid_t domid; >> > +/* IN - the type of resource */ >> > +uint16_t type; >> > +/* >> > + * IN - a type-specific resource identifier, which must be zero >> > + * unless stated otherwise. >> > + */ >> > +uint32_t id; >> > +/* IN - number of (4K) frames of the resource to be mapped */ >> > +uint32_t nr_frames; >> > +uint32_t pad; >> > +/* IN - the index of the initial frame to be mapped */ >> > +uint64_aligned_t frame; >> >> Does this really need to be 64 bits wide? > > I'd prefer not to limit to 32 bits just in case we want to use this > hypercall to map something with a large frame space. In which case you'll need to take care of the truncation issues I've pointed out elsewhere. >> > +/* IN/OUT - If the tools domain is PV then, upon return, gmfn_list >> > + * will be populated with the MFNs of the resource. >> > + * If the tools domain is HVM then it is expected that, on >> > + * entry, gmfn_list will be populated with a list of GFNs >> > + * that will be mapped to the MFNs of the resource. >> > + */ >> > +XEN_GUEST_HANDLE(xen_ulong_t) gmfn_list; >> >> Why not xen_pfn_t? > > I've debated this with myself a few times. I'm not convinced that something > that could be an mfn or a gfn should have a xen_pfn_t type. Well, looking over the present uses of xen_pfn_t it looks like quite a few are for mixed meaning uses between PV and HVM. And btw, since we try to get rid of the term "GMFN", perhaps "frame_list" might be a better field name here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 0/6] Allow setting up shared memory areas between VMs from xl config files
Hi Stefano, Sorry for the long delay. I've been busy with other school stuff recently so I can only use my spare time on this. The current situation is: the code has already been completed, but I'm still drafting the man pages and there are approximately 20% left. I'll try my best to update it within the following one week or so. Sorry again for failing to schedule my time effectively. Cheers, Zhongze Liu. 2017-10-11 7:55 GMT+08:00 Stefano Stabellini : > On Sun, 27 Aug 2017, Zhongze Liu wrote: >> This series implements the new xl config entry proposed in [1]. Users can use >> the new config entry to statically setup shared memory areas among VMs that >> don't have grant table support so that they could communicate with each other >> through the static shared memory areas. >> >> [1] Proposla to allow setting up shared memory areas between VMs from xl >> config file: >> https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html >> >> v2: >> * fixed several code style issues. >> * introduce MMU__SHARE_MEM in flask av permissions, and add a check to >> this. >> permission in the flask hook for xsm_map_gmfn_foreign. >> * support rolling back during creation on partial failure. >> * refcounting the sshm path instead of using "alive" and "zombie" to label >> the >> master and counting the slaves. > > Hey Zhongze, > > any plans on sending an update of this series? > > >> Cheers, >> >> Zhongze Liu (6): >> libxc: add xc_domain_remove_from_physmap to wrap >> XENMEM_remove_from_physmap >> libxl: introduce a new structure to represent static shared memory >> regions >> libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config >> files >> xsm: flask: change the dummy xsm policy and flask hook for >> map_gmfn_foregin >> libxl: support mapping static shared memory areas during domain >> creation >> libxl: support unmapping static shared memory areas during domain >> destruction >> >> tools/flask/policy/modules/xen.if | 2 + >> tools/libxc/include/xenctrl.h | 4 + >> tools/libxc/xc_domain.c | 11 + >> tools/libxl/Makefile| 4 +- >> tools/libxl/libxl.h | 4 + >> tools/libxl/libxl_arch.h| 6 + >> tools/libxl/libxl_arm.c | 15 ++ >> tools/libxl/libxl_create.c | 27 ++ >> tools/libxl/libxl_domain.c | 5 + >> tools/libxl/libxl_internal.h| 15 ++ >> tools/libxl/libxl_sshm.c| 480 >> >> tools/libxl/libxl_types.idl | 34 ++- >> tools/libxl/libxl_x86.c | 18 ++ >> tools/libxl/libxlu_sshm.c | 210 >> tools/libxl/libxlutil.h | 6 + >> tools/xl/xl_parse.c | 24 +- >> xen/arch/arm/mm.c | 2 +- >> xen/arch/x86/mm/p2m.c | 2 +- >> xen/include/xsm/dummy.h | 8 +- >> xen/include/xsm/xsm.h | 7 +- >> xen/xsm/flask/hooks.c | 10 +- >> xen/xsm/flask/policy/access_vectors | 4 + >> 22 files changed, 883 insertions(+), 15 deletions(-) >> create mode 100644 tools/libxl/libxl_sshm.c >> create mode 100644 tools/libxl/libxlu_sshm.c >> >> -- >> 2.14.0 >> ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
>>> On 10.10.17 at 16:37, wrote: >> From: Paul Durrant >> Sent: 10 October 2017 15:10 >> > From: Jan Beulich [mailto:jbeul...@suse.com] >> > Sent: 09 October 2017 15:23 >> > To: Paul Durrant >> > >>> On 06.10.17 at 14:25, wrote: >> > > --- a/xen/common/memory.c >> > > +++ b/xen/common/memory.c >> > > @@ -965,6 +965,67 @@ static long xatp_permission_check(struct domain >> > *d, unsigned int space) >> > > return xsm_add_to_physmap(XSM_TARGET, current->domain, d); >> > > } >> > > >> > > +#ifdef CONFIG_X86 >> > > +static int acquire_resource(const xen_mem_acquire_resource_t *xmar) >> > > +{ >> > > +struct domain *d, *currd = current->domain; >> > > +unsigned long mfn_list[2]; >> > > +int rc; >> > > + >> > > +if ( xmar->nr_frames == 0 || xmar->pad != 0 ) >> > > +return -EINVAL; >> > > + >> > > +if ( xmar->nr_frames > ARRAY_SIZE(mfn_list) ) >> > > +return -E2BIG; >> > > + >> > > +d = rcu_lock_domain_by_any_id(xmar->domid); >> > > +if ( d == NULL ) >> > > +return -ESRCH; >> > > + >> > > +rc = xsm_domain_memory_map(XSM_TARGET, d); >> > >> > Looking at the description of patch 6 - why is this XSM_TARGET >> > rather than XSM_DM_PRIV? >> >> Good point. I was using the priv mapping code as a guide, but XSM_DM_PRIV >> is probably the right thing to use in this case. >> > > Actually that's not possible. There is an assertion in > xsm_domain_memory_map() that the action is XSM_TARGET. Well, I was afraid of this being the case, but this only complicates your job, it doesn't make XSM_TARGET the right choice here. But wait, maybe it can be considered sufficient, but then this needs to be prominently pointed out by a comment added at a suitable place: For the ioreq pages, them being owned by the emulating domain, page ownership validations while trying to make use of the MFNs would prevent mis-use by the domain the emulation is being done for. And for grant table pages the guest is able to access them another way anyway. Which basically leaves the question of this being an information leak for ioreq pages, as the guest is not supposed to know the MFNs, but could obtain them here. I for one would consider such a leak a security issue, even if knowledge of the MFNs alone is not enough to exploit anything, but maybe others think differently. But the grant table aspect suggests anyway that perhaps the permission to be checked here needs to depend on resource type. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 06/11] x86/hvm/ioreq: add a new mappable resource type...
>>> On 10.10.17 at 16:45, wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 09 October 2017 16:21 >> >>> On 06.10.17 at 14:25, wrote: >> > @@ -784,6 +885,45 @@ int hvm_get_ioreq_server_info(struct domain *d, >> ioservid_t id, >> > return rc; >> > } >> > >> > +int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id, >> > + unsigned int idx, mfn_t *mfn) >> > +{ >> > +struct hvm_ioreq_server *s; >> > +int rc; >> > + >> > +spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock); >> > + >> > +if ( id == DEFAULT_IOSERVID ) >> > +return -EOPNOTSUPP; >> > + >> > +s = get_ioreq_server(d, id); >> > + >> > +ASSERT(!IS_DEFAULT(s)); >> > + >> > +rc = hvm_ioreq_server_alloc_pages(s); >> > +if ( rc ) >> > +goto out; >> > + >> > +if ( idx == 0 ) >> >> switch() ? > > Yes, but if idx can exceed 1 in future (which would need to be the case to > support greater numbers of vcpus) then I guess it may change back. If you anticipate that to not be expressable by case labels (not even by range ones), then perhaps indeed better stick to if/else. >> > @@ -3866,6 +3867,27 @@ int xenmem_add_to_physmap_one( >> > return rc; >> > } >> > >> > +int xenmem_acquire_ioreq_server(struct domain *d, unsigned int id, >> > +unsigned long frame, >> > +unsigned long nr_frames, >> > +unsigned long mfn_list[]) >> > +{ >> > +unsigned int i; >> > + >> > +for ( i = 0; i < nr_frames; i++ ) >> > +{ >> > +mfn_t mfn; >> > +int rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn); >> >> Coming back to the question of the size of the "frame" interface >> structure field, note how you silently truncate the upper 32 bits >> here. > > OK. For this resource type I can't see 64-bits being needed, but I'll carry > them through. Carrying the full value through is one option. The other is to bail early when finding the upper bits set. >> > --- a/xen/include/public/memory.h >> > +++ b/xen/include/public/memory.h >> > @@ -609,15 +609,26 @@ struct xen_mem_acquire_resource { >> > domid_t domid; >> > /* IN - the type of resource */ >> > uint16_t type; >> > + >> > +#define XENMEM_resource_ioreq_server 0 >> > + >> > /* >> > * IN - a type-specific resource identifier, which must be zero >> > * unless stated otherwise. >> > + * >> > + * type == XENMEM_resource_ioreq_server -> id == ioreq server id >> > */ >> > uint32_t id; >> > /* IN - number of (4K) frames of the resource to be mapped */ >> > uint32_t nr_frames; >> > uint32_t pad; >> > -/* IN - the index of the initial frame to be mapped */ >> > +/* IN - the index of the initial frame to be mapped >> > + * >> > + * type == XENMEM_resource_ioreq_server -> frame == 0 -> bufioreq >> > + * page >> > + * frame == 1 -> ioreq >> > + * page >> > + */ >> >> Long comment or not I think you want to introduce constants >> for these two numbers. >> > > Yes, that would probably be better although increasing the number of > supported vcpus may mean that >1 becomes valid in future. Sure, but that would then still better be expressed by a suitable macro (perhaps one long the lines of MSR_P6_PERFCTR()). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 11 October 2017 09:31 > To: Paul Durrant > Cc: Andrew Cooper ; George Dunlap > ; Ian Jackson ; Wei Liu > ; 'Stefano Stabellini' ; 'xen- > de...@lists.xenproject.org' ; > 'KonradRzeszutek Wilk' ; Tim (Xen.org) > > Subject: RE: [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to > acquire guest resources > > >>> On 10.10.17 at 16:37, wrote: > >> From: Paul Durrant > >> Sent: 10 October 2017 15:10 > >> > From: Jan Beulich [mailto:jbeul...@suse.com] > >> > Sent: 09 October 2017 15:23 > >> > To: Paul Durrant > >> > >>> On 06.10.17 at 14:25, wrote: > >> > > --- a/xen/common/memory.c > >> > > +++ b/xen/common/memory.c > >> > > @@ -965,6 +965,67 @@ static long xatp_permission_check(struct > domain > >> > *d, unsigned int space) > >> > > return xsm_add_to_physmap(XSM_TARGET, current->domain, d); > >> > > } > >> > > > >> > > +#ifdef CONFIG_X86 > >> > > +static int acquire_resource(const xen_mem_acquire_resource_t > *xmar) > >> > > +{ > >> > > +struct domain *d, *currd = current->domain; > >> > > +unsigned long mfn_list[2]; > >> > > +int rc; > >> > > + > >> > > +if ( xmar->nr_frames == 0 || xmar->pad != 0 ) > >> > > +return -EINVAL; > >> > > + > >> > > +if ( xmar->nr_frames > ARRAY_SIZE(mfn_list) ) > >> > > +return -E2BIG; > >> > > + > >> > > +d = rcu_lock_domain_by_any_id(xmar->domid); > >> > > +if ( d == NULL ) > >> > > +return -ESRCH; > >> > > + > >> > > +rc = xsm_domain_memory_map(XSM_TARGET, d); > >> > > >> > Looking at the description of patch 6 - why is this XSM_TARGET > >> > rather than XSM_DM_PRIV? > >> > >> Good point. I was using the priv mapping code as a guide, but > XSM_DM_PRIV > >> is probably the right thing to use in this case. > >> > > > > Actually that's not possible. There is an assertion in > > xsm_domain_memory_map() that the action is XSM_TARGET. > > Well, I was afraid of this being the case, but this only complicates > your job, it doesn't make XSM_TARGET the right choice here. But > wait, maybe it can be considered sufficient, but then this needs > to be prominently pointed out by a comment added at a suitable > place: For the ioreq pages, them being owned by the emulating > domain, page ownership validations while trying to make use of the > MFNs would prevent mis-use by the domain the emulation is being > done for. And for grant table pages the guest is able to access > them another way anyway. > > Which basically leaves the question of this being an information > leak for ioreq pages, as the guest is not supposed to know the > MFNs, but could obtain them here. I for one would consider such > a leak a security issue, even if knowledge of the MFNs alone is > not enough to exploit anything, but maybe others think differently. > I agree with you. Now you point it out, it does rather defeat the purpose of having the separate resource map hypercall. > But the grant table aspect suggests anyway that perhaps the > permission to be checked here needs to depend on resource type. Separate permissions could be an option, but maybe it would be better just to introduce a new resource mapping permission. I'll probably go with the latter. Paul > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 10.10.17 at 18:01, wrote: >> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d, >> unsigned long idx, gfn_t gfn, >> > return rc; >> > } >> > >> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn) >> >> const struct domain * (I realize now that the same should have >> been done for gnttab_map_frame() when it was introduced; >> perhaps you could change that at the same time). > > Again, the problem is that grow_table and functions it calls don't take a > const pointer. I tried cascading the const through to the underlying function > but the patch started to balloon so I think such work should be deferred. Right, I had overlooked that. And it won't work, as share_xen_page_with_guest() can't be passed a const pointer. >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const >> xen_mem_acquire_resource_t *xmar) >> > xmar->nr_frames, mfn_list); >> > break; >> > >> > +case XENMEM_resource_grant_table: >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame, >> > + xmar->nr_frames, mfn_list); >> > +break; >> >> Is this really generally useful with mfn_list[] having just two entries? >> > > Good point. I'll increase the size of the array in this patch (to the > default table size of 32... I think that's a reasonable value to choose). I suppose for the only current use you have for this (seeding the grant table from the tool stack) even the two entries you have right now would suffice. If, however, a full grant table is supposed to be accessible this way, I can't see how a static upper limit will do. Or if you intend the caller to do multiple invocations in such a case, there ought to be a way to find out the (implementation) limit. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
>>> On 11.10.17 at 10:38, wrote: > Separate permissions could be an option, but maybe it would be better just > to introduce a new resource mapping permission. I'll probably go with the > latter. Oh, I'm certainly fine with that, it just means more changes. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 11 October 2017 09:47 > To: Paul Durrant > Cc: Andrew Cooper ; George Dunlap > ; Ian Jackson ; Wei Liu > ; Stefano Stabellini ; xen- > de...@lists.xenproject.org; KonradRzeszutek Wilk > ; Tim (Xen.org) > Subject: RE: [Xen-devel] [PATCH v9 10/11] common: add a new mappable > resource type: XENMEM_resource_grant_table > > >>> On 10.10.17 at 18:01, wrote: > >> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d, > >> unsigned long idx, gfn_t gfn, > >> > return rc; > >> > } > >> > > >> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t > *mfn) > >> > >> const struct domain * (I realize now that the same should have > >> been done for gnttab_map_frame() when it was introduced; > >> perhaps you could change that at the same time). > > > > Again, the problem is that grow_table and functions it calls don't take a > > const pointer. I tried cascading the const through to the underlying > > function > > but the patch started to balloon so I think such work should be deferred. > > Right, I had overlooked that. And it won't work, as > share_xen_page_with_guest() can't be passed a const pointer. > > >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const > >> xen_mem_acquire_resource_t *xmar) > >> > xmar->nr_frames, mfn_list); > >> > break; > >> > > >> > +case XENMEM_resource_grant_table: > >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame, > >> > + xmar->nr_frames, mfn_list); > >> > +break; > >> > >> Is this really generally useful with mfn_list[] having just two entries? > >> > > > > Good point. I'll increase the size of the array in this patch (to the > > default table size of 32... I think that's a reasonable value to choose). > > I suppose for the only current use you have for this (seeding the > grant table from the tool stack) even the two entries you have > right now would suffice. If, however, a full grant table is supposed > to be accessible this way, I can't see how a static upper limit will do. > Or if you intend the caller to do multiple invocations in such a case, > there ought to be a way to find out the (implementation) limit. I'm open to ideas but there clearly needs to be some sort of upper limit, or we do away with being able to map multiple frames in a single invocation. The dm_op hypercalls currently have a similar upper limit on the size of the buffer array. I'd rather not have to introduce another hypercall just to find out such a thing. It's a tools-only hypercall so could I not just add a comment on what the limit currently is? Paul > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 04/11] public: xen.h: add definitions for UUID handling
>>> On 11.10.17 at 01:24, wrote: > On Tue, 10 Oct 2017, Volodymyr Babchuk wrote: >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -930,6 +930,39 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t); >> __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t); >> __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t); >> >> +typedef struct { >> +uint8_t a[16]; >> +} xen_uuid_t; >> + >> +/* >> + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, >> + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) >> + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as >> + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, >> + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; >> + * >> + * NB: This is compatible with Linux kernel and with libuuid, but it is not >> + * compatible with Microsoft, as they use mixed-endian encoding (some >> + * components are little-endian, some are big-endian). >> + */ >> +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ >> +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ >> + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ >> + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ >> + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ >> + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ >> +e1, e2, e3, e4, e5, e6}} >> + >> +/* Compound literals are supported in C99 and later. */ >> +#if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L >> +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ >> +((xen_uuid_t)XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)) >> +#else >> +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ >> +XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6) >> + >> +#endif /* defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L */ >> + >> #endif /* !__ASSEMBLY__ */ >> >> /* Default definitions for macros used by domctl/sysctl. */ > > This looks good to me, but I would like to get Jan's opinion on this. > Ideally we would commit the series tomorrow before the code freeze. While I can live with it being the way it is now, I've already indicated that I'd prefer __GNUC__ to also be checked for here. As that's a relaxation, it wouldn't be a problem to add later, I think (but I can't exclude I'm overlooking something, so it would feel better if it was done the intended final way from the beginning). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 01/12] fuzz/x86_emulate: Clear errors after each iteration
>>> On 10.10.17 at 18:47, wrote: > On 10/10/2017 05:20 PM, George Dunlap wrote: >> Once feof() returns true for a stream, it will continue to return true >> for that stream until clearerr() is called (or the stream is closed >> and re-opened). >> >> In llvm-clang-fast-mode, the same file descriptor is used for each >> iteration of the loop, meaning that the "Input too large" check was >> broken -- feof() would return true even if the fread() hadn't hit the >> end of the file. The result is that AFL generates testcases of >> arbitrary size. >> >> Fix this by fseek'ing to the beginning of the file on every iteration; >> this resets the EOF marker and other state. >> >> Signed-off-by: George Dunlap >> --- >> Changes in v3: >> - Fix the issue in the official sanctioned way > > Hmm, seems v2 of this patch was checked in; review had flagged up that > "clearerr()" was too big of a hammer. Oh, I'm sorry for having overlooked that feedback. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 01/12] fuzz/x86_emulate: Clear errors after each iteration
>>> On 10.10.17 at 19:22, wrote: > George Dunlap writes ("[PATCH v3 01/12] fuzz/x86_emulate: Clear errors after > each iteration"): >> Once feof() returns true for a stream, it will continue to return true >> for that stream until clearerr() is called (or the stream is closed >> and re-opened). >> >> In llvm-clang-fast-mode, the same file descriptor is used for each >> iteration of the loop, meaning that the "Input too large" check was >> broken -- feof() would return true even if the fread() hadn't hit the >> end of the file. The result is that AFL generates testcases of >> arbitrary size. >> >> Fix this by fseek'ing to the beginning of the file on every iteration; >> this resets the EOF marker and other state. > > Acked-by: Ian Jackson > >> This is a candidate for backport to 4.9. > > Please let me know when it is committed and I will add it to my > backport list. I have the original one on mine already, so I can easily add this one then as well; perhaps I would want to even fold the two into just a single (good) commit). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 04/12] fuzz/x86_emulate: Rename the file containing the wrapper code
>>> On 10.10.17 at 18:20, wrote: > When generating coverage output, by default gcov generates output > filenames based only on the coverage file and the "leaf" source file, > not the full path. As a result, it uses the same name for > x86_emulate.c and x86_emulate/x86_emulate.c, generally overwriting the > second (which we actually are about) with the first (which is just a > wrapper). > > Rename the user-space wrapper helpers to x86-emulate.[ch], so > that it generates separate files. > > There is actually an option to gcov, `--preserve-paths`, which will > cause the full path name to be included in the filename, properly > distinguishing between the two. However, given that the user-space > wrapper doesn't actually do any emulation (and the poor state of gcov > documentation making it difficult to find the option in the first > place), it seems to make more sense to rename the file anyway. > > Signed-off-by: George Dunlap > Acked-by: Wei Liu Acked-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH RFC] osstest: rename pvh tests to pvhv2
Due to the recent changes to the PVH tests, all of them are now failing because the current Linux kernel used by osstest doesn't support PVHv2, and osstest treats the failures as regressions because previously the PVH tests where actually testing classic PV. Rename the tests to 'pvhv2' in order to prevent osstest from classifying the failures as regressions. Signed-off-by: Roger Pau Monné --- Cc: Ian Jackson --- make-flight | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/make-flight b/make-flight index df8f7649..fc42a3a3 100755 --- a/make-flight +++ b/make-flight @@ -812,7 +812,7 @@ test_matrix_do_one () { for cpuvendor in amd intel; do - job_create_test test-$xenarch$kern-$dom0arch-xl-pvh-$cpuvendor \ + job_create_test test-$xenarch$kern-$dom0arch-xl-pvhv2-$cpuvendor \ test-debian xl $xenarch $dom0arch \ debian_pvh=1 $debian_runvars \ all_hostflags=$most_hostflags,hvm-$cpuvendor -- 2.13.5 (Apple Git-94) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/12] fuzz/x86_emulate: Move definitions into a header
>>> On 10.10.17 at 19:25, wrote: > George Dunlap writes ("[PATCH v3 08/12] fuzz/x86_emulate: Move definitions > into a header"): >> Move fuzz-emul.c function prototypes into a header. Also share the >> definition of the input size (rather than hard-coding it in >> fuzz-emul.c). >> >> Signed-off-by: George Dunlap > > Reviewed-by: Ian Jackson Acked-by: Jan Beulich (also if you add ... >> RFC: Worth trying to BUILD_BUG_ON(INPUT_SIZE < DATA_SIZE_FULL)? > > I don't mind. ... this) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 09/12] fuzz/x86_emulate: Make input more compact
>>> On 10.10.17 at 18:20, wrote: > At the moment, AFL reckons that for any given input, 87% of it is > completely irrelevant: that is, it can change it as much as it wants > but have no impact on the result of the test; and yet it can't remove > it. > > This is largely because we interpret the blob handed to us as a large > struct, including CR values, MSR values, segment registers, and a full > cpu_user_regs. > > Instead, modify our interpretation to have a "set state" stanza at the > front. Begin by reading a 16-bit value; if it is lower than a certain > threshold, set some state according to what byte it is, and repeat. > Continue until the byte is above a certain threshold. > > This allows AFL to compact any given test case much smaller; to the > point where now it reckons there is not a single byte of the test file > which becomes irrelevant. Testing have shown that this option both > allows AFL to reach coverage much faster, and to have a total coverage > higher than with the old format. > > Make this an option (rather than a unilateral change) to enable > side-by-side performance comparison of the old and new formats. > > Signed-off-by: George Dunlap Without meaning to override Andrew's objections, in case he can grudgingly accept this going in Acked-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 10/12] fuzz/x86_emulate: Add --rerun option to try to track down instability
>>> On 10.10.17 at 20:44, wrote: > On 10/10/17 17:20, George Dunlap wrote: >> +if ( state[0].elem != state[1].elem ) \ >> +printf(#elem " differ: %lx != %lx\n", \ >> + (unsigned long)state[0].elem, \ >> + (unsigned long)state[1].elem) >> +PINE(regs.r15); > > Hmm - this is going to cause problems for the 32bit build. I can't > think of an easy suggestion to fix it. The fuzzer isn't being built as a 32-bit binary: ifeq ($(CONFIG_X86_64),y) x86-insn-fuzz-all: x86-insn-fuzzer.a fuzz-emul.o afl else x86-insn-fuzz-all: endif Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 04/11] public: xen.h: add definitions for UUID handling
Hi Jan, On 11/10/17 09:54, Jan Beulich wrote: On 11.10.17 at 01:24, wrote: On Tue, 10 Oct 2017, Volodymyr Babchuk wrote: --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -930,6 +930,39 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t); __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t); __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t); +typedef struct { +uint8_t a[16]; +} xen_uuid_t; + +/* + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; + * + * NB: This is compatible with Linux kernel and with libuuid, but it is not + * compatible with Microsoft, as they use mixed-endian encoding (some + * components are little-endian, some are big-endian). + */ +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ +e1, e2, e3, e4, e5, e6}} + +/* Compound literals are supported in C99 and later. */ +#if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ +((xen_uuid_t)XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)) +#else +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ +XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6) + +#endif /* defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L */ + #endif /* !__ASSEMBLY__ */ /* Default definitions for macros used by domctl/sysctl. */ This looks good to me, but I would like to get Jan's opinion on this. Ideally we would commit the series tomorrow before the code freeze. While I can live with it being the way it is now, I've already indicated that I'd prefer __GNUC__ to also be checked for here. As that's a relaxation, it wouldn't be a problem to add later, I think (but I can't exclude I'm overlooking something, so it would feel better if it was done the intended final way from the beginning). To be clear, you ask to do: #if defined(__GNUC__) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) am I correct? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 11/12] fuzz/x86_emulate: Set and fuzz more CPU state
>>> On 10.10.17 at 18:20, wrote: > --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c > +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c > @@ -40,6 +40,8 @@ struct fuzz_state > uint64_t msr[MSR_INDEX_MAX]; > struct segment_register segments[SEG_NUM]; > struct cpu_user_regs regs; > +char fxsave[512] __attribute__((aligned(16))); > + > > /* Fuzzer's input data. */ No double blank lines please. > @@ -596,6 +598,54 @@ 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, &c) > + */ > +static void _set_fpu_state(char *fxsave, bool write) > +{ > +if ( cpu_has_fxsr ) > +{ > +static union __attribute__((__aligned__(16))) { > +char x[512]; > +struct { > +uint32_t other[6]; > +uint32_t mxcsr; > +uint32_t mxcsr_mask; > +/* ... */ > +}; > +} *fxs; > + > +fxs = (typeof(fxs)) fxsave; Stray blank after the cast. > +if ( write ) > +{ > +char null[512] __attribute__((aligned(16))) = { }; > + > +fxs->mxcsr &= mxcsr_mask; > + > +asm volatile( "fxrstor %0" :: "m" (*null) ); > +asm volatile( "fxrstor %0" :: "m" (*fxs) ); Without a comment I still don't really understand what good this double load is intended to do. In particular I don't think there are any state components that the first may alter but the second won't. Quite the opposite, on AMD I think you may end up not altering FIP/FDP/FOP despite this double load (iirc they're being loaded only when an exception is indicated to be pending in the status word; see fpu_fxrstor() in the hypervisor). > @@ -737,6 +791,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 < 128 ) sizeof(s->fxsave) / 4 Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/12] fuzz/x86_emulate: Add an option to limit the number of instructions executed
>>> On 10.10.17 at 18:20, wrote: > AFL considers a testcase to be a useful addition not only if there are > tuples exercised by that testcase which were not exercised otherwise, > but also if the *number* of times an individual tuple is exercised > changes significantly; in particular, if the number of the highest > non-zero bit changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c). > > One simple way to increase these stats it to execute the same (or > similar) instructions multiple times: If executing a given instruction > once hits a particular tuple 2 times, executing it twice will hit the > tuple 4 times, four times will hit the tuple 8 times, and so on. All > of these will look different to AFL, and so it is likely that many of > the "unique test cases" will simply be the same instruction repeated > powers of 2 times until the tuple counts max out (at 128). > > It is unlikely that executing a single instruction more than a handful > of times will generate any state we actually care about; but such long > testcases take exponentially longer to fuzz: the fuzzer spends more > time flipping bits looking for meaningful changes, and each execution > takes longer because it is doing more things. So long paths which add > nothing to the actual code coverage but effectively "distract" the > fuzzer, making it less effective. > > Experiments have shown that not allowing infinite number of > instruction retries for the old (non-compact) format does indeed speed > up and increase code coverage. However, it has also shown that on the > new, more compact format, having no instruction limit causes the highest > throughput in code coverage. > > So leave the option in, but have it default to 0 (no limit). > > Signed-off-by: George Dunlap Acked-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 04/11] public: xen.h: add definitions for UUID handling
>>> On 11.10.17 at 11:29, wrote: > Hi Jan, > > On 11/10/17 09:54, Jan Beulich wrote: > On 11.10.17 at 01:24, wrote: >>> On Tue, 10 Oct 2017, Volodymyr Babchuk wrote: --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -930,6 +930,39 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t); __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t); __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t); +typedef struct { +uint8_t a[16]; +} xen_uuid_t; + +/* + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; + * + * NB: This is compatible with Linux kernel and with libuuid, but it is not + * compatible with Microsoft, as they use mixed-endian encoding (some + * components are little-endian, some are big-endian). + */ +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ +e1, e2, e3, e4, e5, e6}} + +/* Compound literals are supported in C99 and later. */ +#if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ +((xen_uuid_t)XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)) +#else +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ +XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6) + +#endif /* defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L */ + #endif /* !__ASSEMBLY__ */ /* Default definitions for macros used by domctl/sysctl. */ >>> >>> This looks good to me, but I would like to get Jan's opinion on this. >>> Ideally we would commit the series tomorrow before the code freeze. >> >> While I can live with it being the way it is now, I've already >> indicated that I'd prefer __GNUC__ to also be checked for >> here. As that's a relaxation, it wouldn't be a problem to add >> later, I think (but I can't exclude I'm overlooking something, >> so it would feel better if it was done the intended final way >> from the beginning). > > To be clear, you ask to do: > > #if defined(__GNUC__) || (defined(__STDC_VERSION__) && __STDC_VERSION__ > >= 199901L) > > am I correct? Assuming this works for the range of gcc versions we support (which I assume it does), then yes. (Cosmetic remark: I'd put the compiler specific check last.) Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 11.10.17 at 10:54, wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 11 October 2017 09:47 >> >>> On 10.10.17 at 18:01, wrote: >> >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const >> >> xen_mem_acquire_resource_t *xmar) >> >> > xmar->nr_frames, mfn_list); >> >> > break; >> >> > >> >> > +case XENMEM_resource_grant_table: >> >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame, >> >> > + xmar->nr_frames, mfn_list); >> >> > +break; >> >> >> >> Is this really generally useful with mfn_list[] having just two entries? >> >> >> > >> > Good point. I'll increase the size of the array in this patch (to the >> > default table size of 32... I think that's a reasonable value to choose). >> >> I suppose for the only current use you have for this (seeding the >> grant table from the tool stack) even the two entries you have >> right now would suffice. If, however, a full grant table is supposed >> to be accessible this way, I can't see how a static upper limit will do. >> Or if you intend the caller to do multiple invocations in such a case, >> there ought to be a way to find out the (implementation) limit. > > I'm open to ideas but there clearly needs to be some sort of upper limit, or > we do away with being able to map multiple frames in a single invocation. The > dm_op hypercalls currently have a similar upper limit on the size of the > buffer array. I'd rather not have to introduce another hypercall just to find > out such a thing. It's a tools-only hypercall so could I not just add a > comment on what the limit currently is? Hmm, that would be an option, but I'd prefer if we could get away without. And no, I wasn't suggesting to introduce yet another hypercall. Instead how about the handle being a null one asking for the implementation limit to be returned in nr_frames (or, to keep that IN only, in the re-purposed pad field)? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On Tue, 2017-10-10 at 06:28 -0600, Jan Beulich wrote: > > > > +typedef struct xen_hvm_altp2m_set_mem_access_multi > > +xen_hvm_altp2m_set_mem_access_multi_t; > > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t); > > Are typedef and handle actually needed anywhere? Otherwise > please don't add them. Just like recently done for domctl and > sysctl we should even consider cleaning up the others here. > > Jan All xen_hvm_altp2m_* structs have also defined the typedef and the handle. I can remove them for xen_hvm_altp2m_set_mem_access_multi but this way it will not be in sync with the other xen_hvm_altp2m_* definitions. Also, regarding the typedef, I have encountered a possible usage when trying to generate the XLAT macro for xen_hvm_altp2m_op. Using the existing way of declaring the structure (union of structs) the enum corresponding to the union members was not generated. Replacing struct with the corresponding typedef fixed the issue. The cause is the condition the ./xen/tools/get-fields.sh uses to generate the enum (if [ "${kind%%;*}" = union ] ). In our case the kind variable is something like "struct;struct;struct;..;union" and the condition is valid only if kind starts with "union", hence the enum is not generated. Unfortunately my understanding of this script is quite scarce, so I cannot propose viable a solution. Personally I would prefer to keep the definition in sync with the other xen_hvm_altp2m_* structs for now and hanlde this issue in a separate patch for applicable for all definitions. Best regards, Petre > > > 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] tools/libxc: Fix domid parameter types
On 11/10/17 10:51, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH] tools/libxc: Fix domid parameter types"): >> Mixed throughout libxc are uint32_t, int, and domid_t for domid parameters. >> With a signed type, and an explicitly 16-bit type, it is exceedingly >> difficult >> to construct an INVALID_DOMID constant which works with all of them. (The >> main problem being that domid_t gets unconditionally zero extended when >> promoted to int for arithmatic.) >> >> Libxl uses uint32_t consistently everywhere, so alter libxc to match. > I like this plan. But I think this comes a bit late for 4.10 ? I'm sorry for it being late, but it is blocking my other series to fix gnttab construction in the domain builder, which really is 4.10 material. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"): > Actually, a numeric UID without group name or ID could be made to work > just fine as long as it maps to a user name. The use case may not be > worth the bother, though. In libxl's use case, it wouldn't map to a name. > Using '.' to separate user and group is suboptimal, because POSIX > portable user and group names may contain it: ... > Coreutils uses ':'. Let's follow its lead. I'm happy to change it to use `:'. Can you confirm that this command line interface is then OK ? We would like to stablise the corresponding code in libxl... Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
> -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 11 October 2017 10:43 > To: Paul Durrant > Cc: Andrew Cooper ; George Dunlap > ; Ian Jackson ; Wei Liu > ; Stefano Stabellini ; xen- > de...@lists.xenproject.org; KonradRzeszutek Wilk > ; Tim (Xen.org) > Subject: RE: [Xen-devel] [PATCH v9 10/11] common: add a new mappable > resource type: XENMEM_resource_grant_table > > >>> On 11.10.17 at 10:54, wrote: > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: 11 October 2017 09:47 > >> >>> On 10.10.17 at 18:01, wrote: > >> >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const > >> >> xen_mem_acquire_resource_t *xmar) > >> >> > xmar->nr_frames, mfn_list); > >> >> > break; > >> >> > > >> >> > +case XENMEM_resource_grant_table: > >> >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame, > >> >> > + xmar->nr_frames, mfn_list); > >> >> > +break; > >> >> > >> >> Is this really generally useful with mfn_list[] having just two entries? > >> >> > >> > > >> > Good point. I'll increase the size of the array in this patch (to the > >> > default table size of 32... I think that's a reasonable value to choose). > >> > >> I suppose for the only current use you have for this (seeding the > >> grant table from the tool stack) even the two entries you have > >> right now would suffice. If, however, a full grant table is supposed > >> to be accessible this way, I can't see how a static upper limit will do. > >> Or if you intend the caller to do multiple invocations in such a case, > >> there ought to be a way to find out the (implementation) limit. > > > > I'm open to ideas but there clearly needs to be some sort of upper limit, or > > we do away with being able to map multiple frames in a single invocation. > The > > dm_op hypercalls currently have a similar upper limit on the size of the > > buffer array. I'd rather not have to introduce another hypercall just to > > find > > out such a thing. It's a tools-only hypercall so could I not just add a > > comment on what the limit currently is? > > Hmm, that would be an option, but I'd prefer if we could get away > without. And no, I wasn't suggesting to introduce yet another > hypercall. Instead how about the handle being a null one asking > for the implementation limit to be returned in nr_frames (or, to > keep that IN only, in the re-purposed pad field)? > Ok, I'll make nr_frames IN/OUT. I guess I could define it to be set to the implementation limit if the hypercall returns -E2BIG. Paul > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] tools/libxc: Fix domid parameter types
Andrew Cooper writes ("[PATCH] tools/libxc: Fix domid parameter types"): > Mixed throughout libxc are uint32_t, int, and domid_t for domid parameters. > With a signed type, and an explicitly 16-bit type, it is exceedingly difficult > to construct an INVALID_DOMID constant which works with all of them. (The > main problem being that domid_t gets unconditionally zero extended when > promoted to int for arithmatic.) > > Libxl uses uint32_t consistently everywhere, so alter libxc to match. I like this plan. But I think this comes a bit late for 4.10 ? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] tools/libxc: Fix domid parameter types
On Wed, Oct 11, 2017 at 10:52:42AM +0100, Andrew Cooper wrote: > On 11/10/17 10:51, Ian Jackson wrote: > > Andrew Cooper writes ("[PATCH] tools/libxc: Fix domid parameter types"): > >> Mixed throughout libxc are uint32_t, int, and domid_t for domid parameters. > >> With a signed type, and an explicitly 16-bit type, it is exceedingly > >> difficult > >> to construct an INVALID_DOMID constant which works with all of them. (The > >> main problem being that domid_t gets unconditionally zero extended when > >> promoted to int for arithmatic.) > >> > >> Libxl uses uint32_t consistently everywhere, so alter libxc to match. > > I like this plan. But I think this comes a bit late for 4.10 ? > > I'm sorry for it being late, but it is blocking my other series to fix > gnttab construction in the domain builder, which really is 4.10 material. > OK. I don't want to block the more important work. Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10] common/gnttab: Improve logging message by including relevent domid
>>> On 10.10.17 at 22:24, wrote: > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -76,6 +76,9 @@ struct grant_table { > /* Mapping tracking table per vcpu. */ > struct grant_mapping **maptrack; > > +/* Domain to which this struct grant_table belongs. */ > +struct domain *domain; As you're after only the domain ID, why not just domid_t? Or otherwise at least const-qualify the pointer? > @@ -2027,7 +2034,7 @@ gnttab_transfer( > /* Read from caller address space. */ > if ( unlikely(__copy_from_guest(&gop, uop, 1)) ) > { > -gdprintk(XENLOG_INFO, "gnttab_transfer: error reading req > %d/%d\n", > +gdprintk(XENLOG_INFO, "error reading req %d/%d\n", > i, count); "i" (wrongly) is plain int, so %d is fine, but "count" wants %u. > @@ -2076,8 +2081,7 @@ gnttab_transfer( > /* Find the target domain. */ > if ( unlikely((e = rcu_lock_domain_by_id(gop.domid)) == NULL) ) > { > -gdprintk(XENLOG_INFO, "gnttab_transfer: can't find domain %d\n", > -gop.domid); > +gdprintk(XENLOG_INFO, "can't find domain %d\n", gop.domid); d%d here too? If you decide to take care of all of the above, then Acked-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 27/27 v10] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt
On Wed, Oct 11, 2017 at 01:28:44PM +0530, Bhupinder Thakur wrote: > Hi Dave, > > On 26 September 2017 at 20:08, Dave Martin wrote: > > On Fri, Sep 22, 2017 at 01:53:26PM +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 > >> --- > >> CC: Julien Grall > >> CC: Andre Przywara > >> CC: Stefano Stabellini > > > > (Taking a quick look at this because I remember fighthing with FIFO > > behaviour issues when hacking the Linux driver -- but beware, I'm not a > > Xen guy...) > > > > > > Should this patch be flattened into the patches is fixes? Keeping > > known-wrong code in the series does not help reviewers (but maybe it's > > the Xen way). > > > >> 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 > > > > What's sizeof(intf->in), sizeof(intf->out)? > > > > For correct operation, you assume that the total ring buffer size is >= > > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring > > buffer size is set elsewhere and can't be chosen by a driver, you may at > > least add a build-time assert to check that it's big enough. > > > I will add an assert to check this condition. > > > [...] > > > >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, > >> uint8_t data) > > > > [...] > > > >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark > >> which > >> + * is the trigger level for asserting/de-assterting the TX > >> interrupt. > >> */ > >> -vpl011->uartfr |= BUSY; > >> +fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; > >> + > >> +if ( fifo_level <= fifo_threshold ) > >> +vpl011->uartris |= TXI; > >> +else > >> +vpl011->uartris &= ~TXI; > >> } > >> +else > >> +gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); > >> > >> vpl011->uartfr &= ~TXFE; > >> > > > > [...] > > > >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) > >> > >> smp_rmb(); > >> > >> -in_ring_qsize = xencons_queued(in_prod, > >> +in_fifo_level = xencons_queued(in_prod, > >> in_cons, > >> sizeof(intf->in)); > >> > >> -out_ring_qsize = xencons_queued(out_prod, > >> +out_fifo_level = xencons_queued(out_prod, > >> out_cons, > >> sizeof(intf->out)); > >> > >> /* Update the uart rx state if the buffer is not empty. */ > >> -if ( in_ring_qsize != 0 ) > >> +if ( in_fifo_level != 0 ) > >> { > >> vpl011->uartfr &= ~RXFE; > >> -if ( in_ring_qsize == sizeof(intf->in) ) > >> + > >> +if ( in_fifo_level == sizeof(intf->in) ) > >> vpl011->uartfr |= RXFF; > >> + > >> vpl011->uartris |= RXI; > >> } > >> > >> /* Update the uart tx state if the buffer is not full. */ > >> -if ( out_ring_qsize != sizeof(intf->out) ) > >> +if ( out_fifo_level != sizeof(intf->out) ) > >> { > >> +unsigned int out_fifo_threshold; > >> + > >> vpl011->uartfr &= ~TXFF; > >> -vpl011->uartris |= TXI; > >> > >> /* > >> - * Clear the BUSY bit as soon as space becomes available > >> + * Clear the BUSY bit as soon as space becomes avaliable > >> * so that the SBSA UART driver can start writing more data > >> * without any further delay. > >
Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table
>>> On 11.10.17 at 11:54, wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 11 October 2017 10:43 >> Hmm, that would be an option, but I'd prefer if we could get away >> without. And no, I wasn't suggesting to introduce yet another >> hypercall. Instead how about the handle being a null one asking >> for the implementation limit to be returned in nr_frames (or, to >> keep that IN only, in the re-purposed pad field)? >> > > Ok, I'll make nr_frames IN/OUT. I guess I could define it to be set to the > implementation limit if the hypercall returns -E2BIG. Sure, but please don't make this the _only_ way to find out the limit. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
>>> On 10.10.17 at 23:00, wrote: > On Tue, 2017-10-10 at 06:28 -0600, Jan Beulich wrote: >> > > > +typedef struct xen_hvm_altp2m_set_mem_access_multi >> > +xen_hvm_altp2m_set_mem_access_multi_t; >> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t); >> >> Are typedef and handle actually needed anywhere? Otherwise >> please don't add them. Just like recently done for domctl and >> sysctl we should even consider cleaning up the others here. >> > All xen_hvm_altp2m_* structs have also defined the typedef and the > handle. I can remove them for xen_hvm_altp2m_set_mem_access_multi but > this way it will not be in sync with the other xen_hvm_altp2m_* > definitions. Please, as frequently asked for elsewhere elsewhere, let's not spread badness once it was recognized. > Also, regarding the typedef, I have encountered a possible usage when > trying to generate the XLAT macro for xen_hvm_altp2m_op. Using the > existing way of declaring the structure (union of structs) the enum > corresponding to the union members was not generated. Replacing struct > with the corresponding typedef fixed the issue. Now that's a valid argument, if that way less customization is necessary elsewhere in your patch. But that still wouldn't require the handle to be declared. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-coverity test] 114339: all pass - PUSHED
flight 114339 xen-unstable-coverity real [real] http://logs.test-lab.xenproject.org/osstest/logs/114339/ Perfect :-) All tests in this flight passed as required version targeted for testing: xen 3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a baseline version: xen dbc4b6e13a5d0dd8967cde7ff7000ab1ed88625e Last test of basis 113998 2017-10-04 09:23:45 Z7 days Testing same since 114339 2017-10-11 09:23:54 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Awais Masood Bhupinder Thakur Dario Faggioli Dario Faggioli George Dunlap Ian Jackson Jan Beulich Joao Martins Juergen Gross Julien Grall Julien Grall Jun Nakajima Kevin Tian Konrad Rzeszutek Wilk Manish Jaggi Meng Xu ? Roger Pau Monne Roger Pau Monné Ross Lagerwall Sergey Dyasli Stefano Stabellini Tamas K Lengyel Tim Deegan Wei Liu jobs: coverity-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : + branch=xen-unstable-coverity + revision=3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig export PERLLIB=.:. PERLLIB=.:. +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-coverity 3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a + branch=xen-unstable-coverity + revision=3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig export PERLLIB=.:.:. PERLLIB=.:.:. +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig +++ export PERLLIB=.:.:.:. +++ PERLLIB=.:.:.:. ++ umask 002 + select_xenbranch + case "$branch" in + tree=xen + xenbranch=xen-unstable-coverity + qemuubranch=qemu-upstream-unstable-coverity + qemuubranch=qemu-upstream-unstable + '[' xxen = xlinux ']' + linuxbranch= + '[' xqemu-upstream-unstable = x ']' + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable-coverity + prevxenbranch=xen-4.9-testing + '[' x3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
[Xen-devel] [xen-unstable-smoke test] 114335: regressions - FAIL
flight 114335 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114335/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl 7 xen-boot fail REGR. vs. 114299 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass version targeted for testing: xen f17d2cd2ffeda70aba8788910e9d088415562c8b baseline version: xen 3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a Last test of basis 114299 2017-10-10 21:02:54 Z0 days Failing since114308 2017-10-10 23:01:10 Z0 days4 attempts Testing same since 114318 2017-10-11 02:19:34 Z0 days3 attempts People who touched revisions under test: Andre Przywara Julien Grall Stefano Stabellini jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl fail 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 Not pushing. commit f17d2cd2ffeda70aba8788910e9d088415562c8b Author: Andre Przywara Date: Sat Oct 7 01:06:40 2017 +0100 ARM: sunxi: support more Allwinner SoCs So far we only supported the Allwinner A20 SoC. Add support for most of the other virtualization capable Allwinner SoCs by: - supporting the watchdog in newer (sun8i) SoCs - getting the watchdog address from DT - adding compatible strings for other 32-bit SoCs - adding compatible strings for 64-bit SoCs As all 64-bit SoCs support system reset via PSCI, we don't use the platform specific reset routine there. Should the 32-bit SoCs start to properly support the PSCI 0.2 SYSTEM_RESET call, we will use it for them automatically, as we try PSCI first, then fall back to platform reset. Signed-off-by: Andre Przywara Signed-off-by: Stefano Stabellini Reviewed-by: Stefano Stabellini commit e2dfe4a037b0c6ccfd2375e4b60668109a0118e5 Author: Julien Grall Date: Mon Oct 9 14:23:41 2017 +0100 xen/arm: mm: Use memory flags for modify_xen_mappings rather than custom one This will help to consolidate the page-table code and avoid different path depending on the action to perform. Signed-off-by: Julien Grall Reviewed-by: Andre Przywara Reviewed-by: Stefano Stabellini Reviewed-by: Konrad Rzeszutek Wilk commit 6b88beed40c756aaff018d286f4de31351240a93 Author: Julien Grall Date: Mon Oct 9 14:23:40 2017 +0100 xen/arm: mm: Handle permission flags when adding a new mapping Currently, all the new mappings will be read-write non-executable. Allow the caller to use other permissions. Signed-off-by: Julien Grall Reviewed-by: Stefano Stabellini commit 28f2ad440a08908010abec43b7ccc3283051e943 Author: Julien Grall Date: Mon Oct 9 14:23:39 2017 +0100 xen/arm: mm: Embed permission in the flags Currently, it is not possible to specify the permission of a new mapping. It would be necessary to use the function modify_xen_mappings with a different set of flags. Introduce a couple of new flags for the permissions (Non-eXecutable, Read-Only) and also provides definition that combine the memory attribute and permission for common combinations. PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write, non-executable mappings). This does not affect the current mapping using PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be non-executable by default (see mfn_to_xen_entry). A follow-up patch will change modify_xen_mappings to use the new flags. Signed-off-by: Julien Grall Reviewed-by: Stefano Stabellini Signed-off-by: Stefano Stabellini commit 5f3edb5f32e511b915d173403d0b7b5ea38e00ad Author: Julien Grall Date: Mon Oct 9 14:23:38 2017 +0100 xen/arm: page: Describe the layout of flags used to update page tables
Re: [Xen-devel] [PATCH v3 5/5] docs: enable per-VCPU extratime flag for RTDS
On Tue, Oct 10, 2017 at 07:17:45PM -0400, Meng Xu wrote: > Revise xl tool use case by adding -e option > Remove work-conserving from TODO list > > Signed-off-by: Meng Xu Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/5] libxl: enable per-VCPU extratime flag for RTDS
On Tue, Oct 10, 2017 at 07:17:42PM -0400, Meng Xu wrote: > Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU extratime flag > > Signed-off-by: Meng Xu Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/5] xl: enable per-VCPU extratime flag for RTDS
On Tue, Oct 10, 2017 at 07:17:43PM -0400, Meng Xu wrote: > Change main_sched_rtds and related output functions to support > per-VCPU extratime flag. > > Signed-off-by: Meng Xu Acked-by: Wei Liu ___ 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
>>> On 10.10.17 at 13:35, wrote: > On Wed, Oct 04, 2017 at 08:34:13AM +, Jan Beulich wrote: >> >>> On 19.09.17 at 17:29, wrote: >> > --- a/xen/include/xen/vpci.h >> > +++ b/xen/include/xen/vpci.h >> > @@ -72,6 +72,30 @@ struct vpci { >> > } bars[7]; /* At most 6 BARS + 1 expansion ROM BAR. */ >> > /* FIXME: currently there's no support for SR-IOV. */ >> > } header; >> > + >> > +/* MSI data. */ >> > +struct vpci_msi { >> > +/* Arch-specific data. */ >> > +struct vpci_arch_msi arch; >> > +/* Address. */ >> > +uint64_t address; >> > +/* Offset of the capability in the config space. */ >> > +unsigned int pos; >> > +/* Maximum number of vectors supported by the device. */ >> > +unsigned int max_vectors; >> > +/* Number of vectors configured. */ >> > +unsigned int vectors; >> > +/* Mask bitfield. */ >> > +uint32_t mask; >> > +/* Data. */ >> > +uint16_t data; >> > +/* Enabled? */ >> > +bool enabled; >> > +/* Supports per-vector masking? */ >> > +bool masking; >> > +/* 64-bit address capable? */ >> > +bool address64; >> > +} *msi; >> > #endif >> > }; >> >> As there may be quite a few instance of this structure, please strive to >> keep its size down. Many of the fields above have a pretty limited valid >> value range and hence would benefit from using more narrow types and/or >> bitfields. > > max_vectors/vectors can be uint8_t, the rest I'm not sure how to > reduce. "pos" can be uint8_t too afaict. > I could turn the bools into a bitfield, but isn't a bool > already limited to 1 bit? Plus 7 bits of unused data. The minimum addressable unit is a byte after all, and non-bitfield members need to allow their address being taken. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC] osstest: rename pvh tests to pvhv2
Roger Pau Monne writes ("[PATCH RFC] osstest: rename pvh tests to pvhv2"): > Due to the recent changes to the PVH tests, all of them are now > failing because the current Linux kernel used by osstest doesn't > support PVHv2, and osstest treats the failures as regressions because > previously the PVH tests where actually testing classic PV. > > Rename the tests to 'pvhv2' in order to prevent osstest from > classifying the failures as regressions. Acked and applied. Should pass the push gate without trouble I think. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] tools/libxc: Fix domid parameter types
Wei Liu writes ("Re: [PATCH] tools/libxc: Fix domid parameter types"): > On Wed, Oct 11, 2017 at 10:52:42AM +0100, Andrew Cooper wrote: > > I'm sorry for it being late, but it is blocking my other series to fix > > gnttab construction in the domain builder, which really is 4.10 material. Ah. OK. > OK. I don't want to block the more important work. > > Acked-by: Wei Liu Do we need a release ack ? Julien ? I am happy for this to go in. (Also, my qemu depriv series is outstanding and it would probably be easier for me to fix that up after Andrew's changes than vice versa.) Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 11/11] vpci/msix: add MSI-X handlers
>>> On 10.10.17 at 17:04, wrote: > On Wed, Oct 04, 2017 at 08:34:43AM +, Jan Beulich wrote: >> >>> On 19.09.17 at 17:29, wrote: >> > --- a/xen/include/xen/vpci.h >> > +++ b/xen/include/xen/vpci.h >> > @@ -100,6 +100,40 @@ struct vpci { >> > /* 64-bit address capable? */ >> > bool address64; >> > } *msi; >> > + >> > +/* MSI-X data. */ >> > +struct vpci_msix { >> > +struct pci_dev *pdev; >> > +/* List link. */ >> > +struct list_head next; >> > +/* Table information. */ >> > +struct vpci_msix_mem { >> > +/* MSI-X table offset. */ >> > +unsigned int offset; >> > +/* MSI-X table BIR. */ >> > +unsigned int bir; >> > +/* Table size. */ >> > +unsigned int size; >> > +#define VPCI_MSIX_TABLE 0 >> > +#define VPCI_MSIX_PBA 1 >> > +#define VPCI_MSIX_MEM_NUM 2 >> > +} mem[VPCI_MSIX_MEM_NUM]; >> > +/* Maximum number of vectors supported by the device. */ >> > +unsigned int max_entries; >> > +/* MSI-X enabled? */ >> > +bool enabled; >> > +/* Masked? */ >> > +bool masked; >> > +/* Entries. */ >> > +struct vpci_msix_entry { >> > +uint64_t addr; >> > +uint32_t data; >> > +unsigned int nr; >> > +struct vpci_arch_msix_entry arch; >> > +bool masked; >> > +bool updated; >> > +} entries[]; >> > +} *msix; >> >> Same remark as for MSI regarding optimizing structure size. > > Going over the fields, bir can be turned into a uint8_t, and size into > a uint16_t. max_entries can also be converted to a uint16_t together > with nr. > > Apart from that I don't see much more optimization, unless we start > packaging fields (ie: offset and bir could reside in a uint32_t), but > IMHO that's going to make the code harder to parse for little gain, > and will involve more calculations in the handlers. The more instances of a structure there may be, the more relevant it is to pack them tightly. I.e. primary focus needs to be on struct vpci_msix_entry, but since - as indicated - there may be many devices supporting MSI-X, struct vpci_msix as a whole should be reasonably well packed as well. I don't think more calculation in the handlers is an argument - the compiler will do it for you, and the affected code shouldn't really be performance critical (it's involved in setting up interrupts, not delivering them). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] tools/libxc: Fix domid parameter types
On 11 October 2017 at 11:28, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH] tools/libxc: Fix domid parameter types"): >> On Wed, Oct 11, 2017 at 10:52:42AM +0100, Andrew Cooper wrote: >> > I'm sorry for it being late, but it is blocking my other series to fix >> > gnttab construction in the domain builder, which really is 4.10 material. > > Ah. OK. > >> OK. I don't want to block the more important work. >> >> Acked-by: Wei Liu > > Do we need a release ack ? Julien ? I am happy for this to go in. Release-acked-by: Julien Grall Cheers, > > (Also, my qemu depriv series is outstanding and it would probably be > easier for me to fix that up after Andrew's changes than vice versa.) > > Ian. > > ___ > 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
Re: [Xen-devel] [PATCH v3 3/5] xl: enable per-VCPU extratime flag for RTDS
On Tue, 2017-10-10 at 19:17 -0400, Meng Xu wrote: > Change main_sched_rtds and related output functions to support > per-VCPU extratime flag. > > Signed-off-by: Meng Xu > Reviewed-by: Dario Faggioli Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 5/5] docs: enable per-VCPU extratime flag for RTDS
On Tue, 2017-10-10 at 19:17 -0400, Meng Xu wrote: > Revise xl tool use case by adding -e option > Remove work-conserving from TODO list > > Signed-off-by: Meng Xu > Reviewed-by: Dario Faggioli Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/5] xen:rtds: towards work conserving RTDS
On Tue, 2017-10-10 at 19:17 -0400, Meng Xu wrote: > Make RTDS scheduler work conserving without breaking the real-time > guarantees. > > VCPU model: > Each real-time VCPU is extended to have an extratime flag > and a priority_level field. > When a VCPU's budget is depleted in the current period, > if it has extratime flag set, > its priority_level will increase by 1 and its budget will be > refilled; > othewrise, the VCPU will be moved to the depletedq. > > Scheduling policy is modified global EDF: > A VCPU v1 has higher priority than another VCPU v2 if > (i) v1 has smaller priority_leve; or > (ii) v1 has the same priority_level but has a smaller deadline > > Queue management: > Run queue holds VCPUs with extratime flag set and VCPUs with > remaining budget. Run queue is sorted in increasing order of VCPUs > priorities. > Depleted queue holds VCPUs which have extratime flag cleared and > depleted budget. > Replenished queue is not modified. > > Distribution of spare bandwidth > Spare bandwidth is distributed among all VCPUs with extratime flag > set, > proportional to these VCPUs utilizations > > Signed-off-by: Meng Xu > Reviewed-by: Dario Faggioli Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/5] libxl: enable per-VCPU extratime flag for RTDS
On Tue, 2017-10-10 at 19:17 -0400, Meng Xu wrote: > Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU extratime flag > > Signed-off-by: Meng Xu > Reviewed-by: Dario Faggioli Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/5] xentrace: enable per-VCPU extratime flag for RTDS
On Tue, 2017-10-10 at 19:17 -0400, Meng Xu wrote: > --- a/tools/xentrace/formats > +++ b/tools/xentrace/formats > @@ -75,7 +75,7 @@ > 0x00022801 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:tickle[ > cpu = %(1)d ] > 0x00022802 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:runq_pick [ > dom:vcpu = 0x%(1)08x, cur_deadline = 0x%(3)08x%(2)08x, cur_budget = > 0x%(5)08x%(4)08x ] > 0x00022803 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:burn_budget [ > dom:vcpu = 0x%(1)08x, cur_budget = 0x%(3)08x%(2)08x, delta = %(4)d ] > -0x00022804 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:repl_budget [ > dom:vcpu = 0x%(1)08x, cur_deadline = 0x%(3)08x%(2)08x, cur_budget = > 0x%(5)08x%(4)08x ] > +0x00022804 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:repl_budget [ > dom:vcpu = 0x%(1)08x, priority_level = 0x%(2)08d cur_deadline = > 0x%(4)08x%(3)08x, cur_budget = 0x%(6)08x%(5)08x ] > 0x00022805 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:sched_tasklet > 0x00022806 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:schedule [ > cpu[16]:tasklet[8]:idle[4]:tickled[4] = %(1)08x ] > But, both in case of this file and below in xenalyze.c, you update 1 record (the one of REPL_BUDGET). However, in patch 1, you added the priority_level field to two records: REPL_BUDGET and BURN_BUDGET. Or am I missing something? Regards, Dario > diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c > index 79bdba7..2783204 100644 > --- a/tools/xentrace/xenalyze.c > +++ b/tools/xentrace/xenalyze.c > @@ -7946,12 +7946,14 @@ void sched_process(struct pcpu_info *p) > if(opt.dump_all) { > struct { > unsigned int vcpuid:16, domid:16; > +unsigned int priority_level; > uint64_t cur_dl, cur_bg; > } __attribute__((packed)) *r = (typeof(r))ri->d; > > -printf(" %s rtds:repl_budget d%uv%u, deadline = > %"PRIu64", " > - "budget = %"PRIu64"\n", ri->dump_header, > - r->domid, r->vcpuid, r->cur_dl, r->cur_bg); > +printf(" %s rtds:repl_budget d%uv%u, priority_level > = %u," > + "deadline = %"PRIu64", budget = %"PRIu64"\n", > + ri->dump_header, r->domid, r->vcpuid, > + r->priority_level, r->cur_dl, r->cur_bg); > } > break; > case TRC_SCHED_CLASS_EVT(RTDS, 5): /* SCHED_TASKLET*/ -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-4.5-testing baseline-only test] 72227: regressions - FAIL
This run is configured for baseline tests only. flight 72227 xen-4.5-testing real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/72227/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-amd64-pvgrub 17 guest-localmigrate/x10 fail REGR. vs. 72109 test-amd64-i386-xl-raw 22 leak-check/check fail REGR. vs. 72109 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 16 guest-localmigrate/x10 fail REGR. vs. 72109 test-amd64-amd64-xl-qemuu-winxpsp3 16 guest-localmigrate/x10 fail REGR. vs. 72109 Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail REGR. vs. 72109 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-vhd 10 debian-di-installfail like 72109 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 72109 test-amd64-amd64-xl-rtds 7 xen-boot fail like 72109 test-xtf-amd64-amd64-4 59 leak-check/check fail like 72109 test-xtf-amd64-amd64-3 59 leak-check/check fail like 72109 test-xtf-amd64-amd64-5 59 leak-check/check fail like 72109 test-xtf-amd64-amd64-1 59 leak-check/check fail like 72109 test-xtf-amd64-amd64-2 59 leak-check/check fail like 72109 test-amd64-amd64-qemuu-nested-intel 14 xen-boot/l1 fail like 72109 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail like 72109 test-amd64-amd64-xl-qemut-winxpsp3 10 windows-install fail like 72109 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-i386-xl-qemut-ws16-amd64 10 windows-install fail never pass test-amd64-i386-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 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass test-armhf-armhf-xl-midway 13 migrate-support-checkfail never pass test-armhf-armhf-xl-midway 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-xl-pvh-intel 15 guest-saverestorefail never pass test-amd64-amd64-xl-pvh-amd 12 guest-start fail never pass test-xtf-amd64-amd64-4 58 xtf/test-hvm64-xsa-195 fail never pass test-xtf-amd64-amd64-3 58 xtf/test-hvm64-xsa-195 fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-xtf-amd64-amd64-5 58 xtf/test-hvm64-xsa-195 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-xtf-amd64-amd64-1 58 xtf/test-hvm64-xsa-195 fail never pass test-armhf-armhf-libvirt-raw 11 guest-start fail never pass test-xtf-amd64-amd64-2 58 xtf/test-hvm64-xsa-195 fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-xl-qemut-ws16-amd64 13 guest-saverestore fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail never pass test-amd64-i386-xl-qemut-win10-i386 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win10-i386 17 guest-stop fail never pass version targeted for testing: xen db487a6678521c213f7bfe3bab4a3170d46d2b41 baseline version: xen 83724d9f3ae21a3b96362742e2f052b19d9f559a Last test of basis72109 2017-09-15 07:13:48 Z 26 days Testing same since72227 2017-10-11 04:46:41 Z0 days1 attempts People who touched revisions under test: Boris Ostrovsky Julien Grall Stefano Stabellini jobs: build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-li
Re: [Xen-devel] [PATCH] tools/libxc: Fix domid parameter types
On Fri, Oct 06, 2017 at 08:00:00PM +0100, Andrew Cooper wrote: > Mixed throughout libxc are uint32_t, int, and domid_t for domid parameters. > With a signed type, and an explicitly 16-bit type, it is exceedingly difficult > to construct an INVALID_DOMID constant which works with all of them. (The > main problem being that domid_t gets unconditionally zero extended when > promoted to int for arithmatic.) > > Libxl uses uint32_t consistently everywhere, so alter libxc to match. > The following diff is required: diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c index 194bbdbc5d..6487672277 100644 --- a/tools/libxl/libxl_dom_save.c +++ b/tools/libxl/libxl_dom_save.c @@ -149,7 +149,7 @@ static void domain_suspend_switch_qemu_logdirty_done } void libxl__domain_suspend_common_switch_qemu_logdirty - (int domid, unsigned enable, void *user) + (uint32_t domid, unsigned enable, void *user) { libxl__save_helper_state *shs = user; libxl__egc *egc = shs->egc; diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index bcb6b0ae95..70a1e6e915 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -3793,7 +3793,7 @@ void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc, _hidden void libxl__domain_suspend_common_switch_qemu_logdirty - (int domid, unsigned int enable, void *data); + (uint32_t domid, unsigned int enable, void *data); _hidden void libxl__domain_common_switch_qemu_logdirty(libxl__egc *egc, int domid, unsigned enable, libxl__logdirty_switch *lds); diff --git a/tools/libxl/libxl_save_msgs_gen.pl b/tools/libxl/libxl_save_msgs_gen.pl index 3ae7373afc..cba7a30e4c 100755 --- a/tools/libxl/libxl_save_msgs_gen.pl +++ b/tools/libxl/libxl_save_msgs_gen.pl @@ -27,7 +27,7 @@ our @msgs = ( [ 4, 'srcxA', "postcopy", [] ], [ 5, 'srcxA', "checkpoint", [] ], [ 6, 'srcxA', "wait_checkpoint", [] ], -[ 7, 'scxA', "switch_qemu_logdirty", [qw(int domid +[ 7, 'scxA', "switch_qemu_logdirty", [qw(uint32_t domid unsigned enable)] ], [ 8, 'rcx',"restore_results", ['xen_pfn_t', 'store_gfn', 'xen_pfn_t', 'console_gfn'] ], ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [distros-debian-squeeze test] 72228: tolerable trouble: broken/fail/pass
flight 72228 distros-debian-squeeze real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/72228/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: build-arm64 2 hosts-allocate broken like 72196 build-arm64-pvops 2 hosts-allocate broken like 72196 build-arm64 3 capture-logs broken like 72196 build-arm64-pvops 3 capture-logs broken like 72196 test-amd64-i386-amd64-squeeze-netboot-pygrub 10 debian-di-install fail like 72196 test-amd64-i386-i386-squeeze-netboot-pygrub 10 debian-di-install fail like 72196 test-amd64-amd64-i386-squeeze-netboot-pygrub 10 debian-di-install fail like 72196 test-amd64-amd64-amd64-squeeze-netboot-pygrub 10 debian-di-install fail like 72196 baseline version: flight 72196 jobs: build-amd64 pass build-arm64 broken build-armhf pass build-i386 pass build-amd64-pvopspass build-arm64-pvopsbroken build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-amd64-squeeze-netboot-pygrubfail test-amd64-i386-amd64-squeeze-netboot-pygrub fail test-amd64-amd64-i386-squeeze-netboot-pygrub fail test-amd64-i386-i386-squeeze-netboot-pygrub fail sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: handle address wrapping for VMASKMOVP{S, D}
On 11/10/17 08:44, Jan Beulich wrote: On 10.10.17 at 14:43, wrote: >> On 10/10/17 11:43, Jan Beulich wrote: >>> There's another issue here, but I'll first have to think about possible >>> (preferably non-intrusive) solutions: An access crossing a page >>> boundary and having >>> - a set mask bit corresponding to an element fully living in the first >>> page, >>> - one or more clear mask bits corresponding to the initial elements on >>> the second page, >>> - another higher mask bit being set >>> would result in a wrong CR2 value to be reported in case the access to >>> the second page would cause a fault (it would point to the start of the >>> page instead of the element being accessed). Neither splitting the >>> access here into multiple ones nor uniformly passing a byte or word >>> enable mask into ->write() seem very desirable. >> Is this just supposition, or have you confirmed what really happens on >> hardware? > I had done some experiments already at the time I wrote this. > I've now done more. If the fault occurs on the high page, the CR2 > value (on Haswell) depends on whether there was an enabled > access to the low page. If so, CR2 holds the address of the last > byte of the overall range (even if the highest mask bit is clear). If > not, CR2 holds the address of the lowest byte actually being > accessed. The answer appears to be "its complicated". >> I expect that the mask operations turn into multi-part operations, which >> means their behaviour on a straddled fault is implementation defined >> (and behaves differently between Atoms and Xeons). >> >> One option we could do is to have a variation of the "Implement >> hvmemul_write() using real mappings" logic where we pull mappings into >> the vmap individually, but that would require some part of the code to >> convert ea + mask => linear address of each unit, so the eventual >> mapping can be constructed piece-wise. > I certainly have no Atom to play with - on the Haswell, the write > to the first page does not take effect when the access to the > second page faults. Hence splitting the accesses (as suggested > as an option above) clearly would not be valid. Not so. If the behaviour is implementation defined, splitting the accesses in the emulator would be valid, even if it differs from hardware behaviour. The problem here is working out whether the behaviour here is implementation defined, or whether it defined as having atomic properties. I am not aware of any statement in any of the vendor manuals which confirms the atomic properties, or declares the behaviour as implementation defined. > > On my Dinar (AMD Fam15), otoh, CR2 always points at the > lowest byte of the actually accessed field(s) on the page causing > the fault (i.e. the behavior I had implied in my original remark to > the patch). This is certainly more logical behaviour. > > Perhaps to avoid passing byte/word enables into ->read and > ->write() (not sure why originally I had thought of the latter > only) we could extend struct pagefault_info to include an > "offset from start of accessed range" field, to be passed into > x86_emul_pagefault() in addition to the current arguments? An interesting question would be whether an access with no mask bits set in the first page faults if the first page is not present. This would help identify whether a pagewalk is done for start of the access, or whether the first page is omitted entirely when the mask permits. (I can see the latter behaviour being faster, but perhaps more complicated in silicon, so I'm having a hard time judging is likely to happen). At the end of the day, this is clearly a corner case which behaves differently in different circumstances, which gives us some flexibility in how we fix it. I'd opt for atomic behaviour wherever possible, because it is clearly how at least one piece of hardware actually behaves, and it is the safer option to take e.g. for emulation caused by introspection. So long as CR2 points to the correct 4k frame, demand paging will work inside the guest, so I don't think we should worry too much about exactly where it points. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] VT-d: use two 32-bit writes to update DMAR fault address registers
>>> On 11.10.17 at 05:03, wrote: > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1105,7 +1105,13 @@ static void dma_msi_set_affinity(struct irq_desc > *desc, const cpumask_t *mask) > > spin_lock_irqsave(&iommu->register_lock, flags); > dmar_writel(iommu->reg, DMAR_FEDATA_REG, msg.data); > -dmar_writeq(iommu->reg, DMAR_FEADDR_REG, msg.address); > +dmar_writel(iommu->reg, DMAR_FEADDR_REG, msg.address_lo); > +/* > + * When x2APIC is not enabled, DMAR_FEUADDR_REG is reserved and > + * it's not necessary to update it. > + */ > +if (x2apic_enabled) I'm pretty sure it was pointed out before that the style here is wrong (missing spaces). That's easy to fix while committing, but anyway. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 07/12] fuzz/x86_emulate: Move all state into fuzz_state
On 10/10/2017 07:20 PM, Andrew Cooper wrote: > On 10/10/17 17:20, George Dunlap wrote: >> This is in preparation for adding the option for a more "compact" >> interpretation of the fuzzing data, in which we only change select >> bits of the state. >> >> Signed-off-by: George Dunlap >> Acked-by: Jan Beulich >> --- >> v3: >> - Move DATA_OFFSET inside the structure >> - Remove a stray blank line >> v2: Port over previous changes >> >> CC: Ian Jackson >> CC: Wei Liu >> CC: Andrew Cooper >> CC: Jan Beulich >> --- >> tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 89 >> + >> 1 file changed, 45 insertions(+), 44 deletions(-) >> >> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> index 8998f21fe1..20d52b33f8 100644 >> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> @@ -24,14 +24,8 @@ >> /* Layout of data expected as fuzzing input. */ >> struct fuzz_corpus >> { >> -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[4096]; >> } input; >> -#define DATA_OFFSET offsetof(struct fuzz_corpus, data) >> >> /* >> * Internal state of the fuzzing harness. Calculated initially from the >> input >> @@ -39,7 +33,14 @@ struct fuzz_corpus >> */ > > You've invalidated a number of the comments describing behaviours, > including the description of the difference between fuzz_state and > fuzz_corpus. Well completely apart from the 'compact' format, I think this move makes sense. The state moved is actually the state of the "emulated cpu" -- the emulator actually modifies this state as instructions are executed. I think it makes sense to keep the "current state of the virtual processor" separate from "input we get from a file". In fact, the comment above this says: "Internal state of the fuzzing harness. Calculated initially from the input corpus, and later [mutated] by the emulation callbacks." This actually makes that comment *more* true, since before this patch almost the only state modified by the emulation callbacks was actually in fuzz_corpus, not fuzz_state. > Why do you need to move any of this in the first place? If you insist > on keeping the compact mode, what's wrong with conditionally reading the > AFL input into either fuzz_copus entirely, or into fuzz_corpus.data[] > and then conditionally deriving this state? I don't see any advantage of that. In fact it would mean that the name "input" and "fuzz_corpus" even more misleading than they are before this patch. > That way, you don't block work to fix the root cause, which needs to end > up with architectural values in fuzz_state, derived from a bitfields in > fuzz_corpus. x86_emulate() needs a cpu_user_regs structure; so if you want the data in fuzz_corpus not to look exactly like cpu_user_regs, then you'll need to have another place to put cpu_user_regs, and "populate" it based on the data (whatever that looks like). The most obvious thing to do is to is to do exactly what I've done -- place cpu_user_regs inside fuzz_state. The same thing is true for the other bits of data -- the read_* and write* callbacks need "unpacked" state which they can read and modify. The most obvious thing to do is to have arrays in fuzz_state which they can simply read and write, and to populate them based on whatever structure "compact" structure you end up with. If you want to re-introduce a more compact structure format for the input file, there are lots of options. We can make fuzz_corpus into a union. We can have 'input' be a pure char array, and cast it into several different structures depending on how we want to interpret it. Or, we can define a local structure on a stack and "read" from the main input file via input_read() and friends. This patch makes all of those options easier, not harder. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [qemu-upstream-unstable test] 114273: regressions - FAIL
On Wed, Oct 11, 2017 at 04:33:26AM +, osstest service owner wrote: > flight 114273 qemu-upstream-unstable real [real] > http://logs.test-lab.xenproject.org/osstest/logs/114273/ > > Regressions :-( > > Tests which did not succeed and are blocking, > including tests which could not be run: > test-amd64-amd64-xl-pvh-intel 12 guest-start fail REGR. vs. > 114029 > test-amd64-amd64-xl-pvh-amd 12 guest-start fail REGR. vs. > 114029 Hi, Is this an expected failure due to recent change about pvh (in libxl)? In the logs, `xl create` fails with this: xc: error: panic: xc_dom_x86.c:1587: bootlate_pv: pin_table failed (pfn 0x2374, rc=1): Internal error libxl: error: libxl_dom.c:745:libxl__build_dom: xc_dom_boot_image failed: Invalid argument domainbuilder: detail: xc_dom_release: called Can we force push? as this error does not seems to be related to QEMU. Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86emul: handle address wrapping for VMASKMOVP{S, D}
>>> On 11.10.17 at 13:23, wrote: > On 11/10/17 08:44, Jan Beulich wrote: > On 10.10.17 at 14:43, wrote: >>> On 10/10/17 11:43, Jan Beulich wrote: There's another issue here, but I'll first have to think about possible (preferably non-intrusive) solutions: An access crossing a page boundary and having - a set mask bit corresponding to an element fully living in the first page, - one or more clear mask bits corresponding to the initial elements on the second page, - another higher mask bit being set would result in a wrong CR2 value to be reported in case the access to the second page would cause a fault (it would point to the start of the page instead of the element being accessed). Neither splitting the access here into multiple ones nor uniformly passing a byte or word enable mask into ->write() seem very desirable. >>> Is this just supposition, or have you confirmed what really happens on >>> hardware? >> I had done some experiments already at the time I wrote this. >> I've now done more. If the fault occurs on the high page, the CR2 >> value (on Haswell) depends on whether there was an enabled >> access to the low page. If so, CR2 holds the address of the last >> byte of the overall range (even if the highest mask bit is clear). If >> not, CR2 holds the address of the lowest byte actually being >> accessed. > > The answer appears to be "its complicated". > >>> I expect that the mask operations turn into multi-part operations, which >>> means their behaviour on a straddled fault is implementation defined >>> (and behaves differently between Atoms and Xeons). >>> >>> One option we could do is to have a variation of the "Implement >>> hvmemul_write() using real mappings" logic where we pull mappings into >>> the vmap individually, but that would require some part of the code to >>> convert ea + mask => linear address of each unit, so the eventual >>> mapping can be constructed piece-wise. >> I certainly have no Atom to play with - on the Haswell, the write >> to the first page does not take effect when the access to the >> second page faults. Hence splitting the accesses (as suggested >> as an option above) clearly would not be valid. > > Not so. If the behaviour is implementation defined, splitting the > accesses in the emulator would be valid, even if it differs from > hardware behaviour. > > The problem here is working out whether the behaviour here is > implementation defined, or whether it defined as having atomic > properties. I am not aware of any statement in any of the vendor > manuals which confirms the atomic properties, or declares the behaviour > as implementation defined. As much as other instructions rarely have any such information. In fact, the absence of an "implementation defined" statement normally kind of implies there's nothing implementation defined there. In the case here, vendors agreeing that no partial writes are being done, I would consider it wrong for the emulator to do so unless there was an explicit statement allowing for this. The text, however, only says that the order of accesses is implementation defined. >> Perhaps to avoid passing byte/word enables into ->read and >> ->write() (not sure why originally I had thought of the latter >> only) we could extend struct pagefault_info to include an >> "offset from start of accessed range" field, to be passed into >> x86_emul_pagefault() in addition to the current arguments? > > An interesting question would be whether an access with no mask bits set > in the first page faults if the first page is not present. It doesn't - that's what I had tried in my first round of experiments. > This would > help identify whether a pagewalk is done for start of the access, or > whether the first page is omitted entirely when the mask permits. Equally well the page walk may be done, but its results discarded. > At the end of the day, this is clearly a corner case which behaves > differently in different circumstances, which gives us some flexibility > in how we fix it. I'd opt for atomic behaviour wherever possible, > because it is clearly how at least one piece of hardware actually > behaves, and it is the safer option to take e.g. for emulation caused by > introspection. So long as CR2 points to the correct 4k frame, demand > paging will work inside the guest, so I don't think we should worry too > much about exactly where it points. Well, for the moment (i.e. 4.10) I'm certainly not meaning to fix this beyond the patch already submitted, but since AVX-512 allows such masked accesses all the time I will want to get this right before adding AVX-512 support (or else we'd spread the problem). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [qemu-upstream-unstable test] 114273: regressions - FAIL
On Wed, Oct 11, 2017 at 11:34:23AM +, Anthony PERARD wrote: > On Wed, Oct 11, 2017 at 04:33:26AM +, osstest service owner wrote: > > flight 114273 qemu-upstream-unstable real [real] > > http://logs.test-lab.xenproject.org/osstest/logs/114273/ > > > > Regressions :-( > > > > Tests which did not succeed and are blocking, > > including tests which could not be run: > > test-amd64-amd64-xl-pvh-intel 12 guest-start fail REGR. vs. > > 114029 > > test-amd64-amd64-xl-pvh-amd 12 guest-start fail REGR. vs. > > 114029 > > Hi, > > Is this an expected failure due to recent change about pvh (in libxl)? > > In the logs, `xl create` fails with this: > xc: error: panic: xc_dom_x86.c:1587: bootlate_pv: pin_table failed (pfn > 0x2374, rc=1): Internal error > libxl: error: libxl_dom.c:745:libxl__build_dom: xc_dom_boot_image failed: > Invalid argument > domainbuilder: detail: xc_dom_release: called > > > Can we force push? as this error does not seems to be related to QEMU. I've sent a patch for osstest this morning that should solve this: https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg01328.html Ian has already pushed it, so it shouldn't take long before this is fixed, sorry for the breakage. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list
>>> On 10.10.17 at 18:31, wrote: > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -33,6 +33,37 @@ > > #include > > +static void set_ioreq_server(struct domain *d, unsigned int id, > + struct hvm_ioreq_server *s) > +{ > +ASSERT(id < MAX_NR_IOREQ_SERVERS); > +ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]); > + > +d->arch.hvm_domain.ioreq_server.server[id] = s; > +} > + > +#define GET_IOREQ_SERVER(d, id) \ > +(d)->arch.hvm_domain.ioreq_server.server[id] > + > +static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d, > + unsigned int id) > +{ > +if ( id >= MAX_NR_IOREQ_SERVERS ) > +return NULL; > + > +return GET_IOREQ_SERVER(d, id); > +} > + > +#define IS_DEFAULT(s) \ > +((s) && (s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID)) Any reason not to use GET_IOREQ_SERVER() here? > +/* Iterate over all possible ioreq servers */ > +#define FOR_EACH_IOREQ_SERVER(d, id, s) \ > +for ( (id) = 0; (id) < MAX_NR_IOREQ_SERVERS; (id)++ ) \ > +if ( !(s = GET_IOREQ_SERVER((d), (id))) ) \ The two pairs of innermost parentheses are pointless here. With the first one at least explained and the second one taken care of (easily doable at commit time) Reviewed-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v10 04/11] public: xen.h: add definitions for UUID handling
Added type xen_uuid_t. This type represents UUID as an array of 16 bytes in big endian format. Added macro XEN_DEFINE_UUID that constructs UUID in the usual way: XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff} NB: We define a new structure here rather than re-using EFI_GUID. EFI_GUID uses a Microsoft-style encoding which, among other things, mixes little-endian and big-endian. The structure defined in this patch, unlike EFI_GUID, is compatible with the Linux kernel and libuuid. Signed-off-by: Volodymyr Babchuk --- * Added check `|| defined(__GNUC__) to #if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) --- xen/include/public/xen.h | 33 + 1 file changed, 33 insertions(+) diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index 2ac6b1e..e7129fd 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -930,6 +930,39 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t); __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t); __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t); +typedef struct { +uint8_t a[16]; +} xen_uuid_t; + +/* + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; + * + * NB: This is compatible with Linux kernel and with libuuid, but it is not + * compatible with Microsoft, as they use mixed-endian encoding (some + * components are little-endian, some are big-endian). + */ +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ +e1, e2, e3, e4, e5, e6}} + +/* Compound literals are supported in C99 and later. */ +#if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \ +defined (__GNUC__) +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ +((xen_uuid_t)XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)) +#else +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ +XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6) +#endif /* defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L */ + #endif /* !__ASSEMBLY__ */ /* Default definitions for macros used by domctl/sysctl. */ -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] libelf: allow having HYPERCALL_PAGE entry before VIRT_BASE in __xen_guest section.
From: Gregory Herrero When filling __xen_guest section of a guest, user may define HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an incorrect hypercall page address since an undefined virt_base could be used to compute hypercall page address. If there is no VIRT_BASE entry in __xen_guest section, default value of 0 is used for virt_base. Thus, setting hypercall page address to HYPERCALL_PAGE value is correct in this case too. Signed-off-by: Gregory Herrero --- xen/common/libelf/libelf-dominfo.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index a52900c00cd..b1255acc059 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -330,14 +330,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, /* longs */ if ( !strcmp(name, "VIRT_BASE") ) +{ parms->virt_base = strtoull(value, NULL, 0); +if ( parms->virt_hypercall != UNSET_ADDR ) +params->virt_hypercall += params->virt_base; +} if ( !strcmp(name, "VIRT_ENTRY") ) parms->virt_entry = strtoull(value, NULL, 0); if ( !strcmp(name, "ELF_PADDR_OFFSET") ) parms->elf_paddr_offset = strtoull(value, NULL, 0); if ( !strcmp(name, "HYPERCALL_PAGE") ) -parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + -parms->virt_base; +{ +parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); +if ( parms->virt_base != UNSET_ADDR ) +params->virt_hypercall += params->virt_base; +} /* other */ if ( !strcmp(name, "FEATURES") ) -- 2.14.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5] x86: psr: support co-exist features' values setting
>>> On 11.10.17 at 09:20, wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -,25 +,43 @@ static unsigned int get_socket_cpu(unsigned int > socket) > struct cos_write_info > { > unsigned int cos; > -struct feat_node *feature; > const uint32_t *val; > -const struct feat_props *props; > +unsigned int array_len; > }; The addition wants to go into the hole after "cos". > static void do_write_psr_msrs(void *data) > { > -const struct cos_write_info *info = data; > -struct feat_node *feat = info->feature; > -const struct feat_props *props = info->props; > -unsigned int i, cos = info->cos, cos_num = props->cos_num; > +struct cos_write_info *info = data; const > +unsigned int i, index = 0, cos = info->cos; > +struct psr_socket_info *socket_info = const > + > get_socket_info(cpu_to_socket(smp_processor_id())); > > -for ( i = 0; i < cos_num; i++ ) > +/* > + * Iterate all featuers to write different value (not same as MSR) for > + * each feature. > + */ > +for ( i = 0; i < ARRAY_SIZE(feat_props); i++ ) > { > -if ( feat->cos_reg_val[cos * cos_num + i] != info->val[i] ) > +struct feat_node *feat = socket_info->features[i]; > +const struct feat_props *props = feat_props[i]; > +unsigned int cos_num, j; > + > +if ( !feat || !props ) > +continue; > + > +cos_num = props->cos_num; > +ASSERT(info->array_len >= index + cos_num); While this transformation from the original -ENOSPC return looks to be correct, but not obviously so, it would have been a good idea to mention this in the commit message. After all the above can be correct only if the original -ENOSPC return path could have been an ASSERT() as well. > +for ( j = 0; j < cos_num; j++ ) > { > -feat->cos_reg_val[cos * cos_num + i] = info->val[i]; > -props->write_msr(cos, info->val[i], props->type[i]); > +if ( feat->cos_reg_val[cos * cos_num + j] != info->val[index + > j] ) > +{ > +feat->cos_reg_val[cos * cos_num + j] = info->val[index + j]; > +props->write_msr(cos, info->val[index + j], props->type[j]); > +} > } > + > +index += cos_num; Looks like I only meant to comment on the uses of index above: If you incremented it alongside j, you could use just index in the respective array accesses, and you'd avoid the last statement above altogether. In the interest of getting the patch in I'll see to make the adjustments myself. Please double check the result in case I end up committing what I've come up with. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 04/11] public: xen.h: add definitions for UUID handling
Hi jan On 11.10.17 11:07, Jan Beulich wrote: On 10.10.17 at 19:03, wrote: On 10.10.17 19:12, Jan Beulich wrote: On 10.10.17 at 17:52, wrote: +uint8_t a[16]; +} xen_uuid_t; + +/* + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; + * + * NB: This is compatible with Linux kernel and with libuuid, but it is not + * compatible with Microsoft, as they use mixed-endian encoding (some + * components are little-endian, some are big-endian). + */ +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ +e1, e2, e3, e4, e5, e6}} + +/* Compound literals are supported in C99 and later. */ +#if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L I didn't notice this earlier - why no check for __GNUC__? I have seen pattern "#if defined (__STDC_VERSION__) && __STDC_VERSION__ But if you check, all of the existing ones have "#elif defined(__GNUC__)". Yes. But there was a reason do to so. For example: #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L uint32_t optarr[]; #elif defined(__GNUC__) uint32_t optarr[0]; #endif (xen/include/public/physdev.h:303) If compiler is C99 then we use flexible length array, else if compiller is GCC, we use zero-length array, which is GCC extension (correct me). Other compilers (non-gcc C90 compatible) are not supported. Probably this is a bug. Another case is even worse: #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L unsigned char buf[]; #elif defined(__GNUC__) unsigned char buf[1]; /* OUT: Variable length buffer with build_id. */ (xen/include/public/version.h:49) Array of length one is perfectly fine in any dialect of C. There are no need to check for GCC. Also, again, this code will not define `buf` on non-gcc, C90 compatible compilers. My code does not use gcc-only extensions like zero-length arrays, so I don't see how #elif defined (__GNUC__) can fit in the my case. >= 199901" in various places in XEN, so I did it alike. Also, I'm using C99 feature, not gcc-only one. I didn't ask you to replace the conditional, but to (possibly) extend it. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 04/11] public: xen.h: add definitions for UUID handling
>>> On 11.10.17 at 14:12, wrote: > On 11.10.17 11:07, Jan Beulich wrote: > On 10.10.17 at 19:03, wrote: >>> On 10.10.17 19:12, Jan Beulich wrote: >>> On 10.10.17 at 17:52, wrote: > +uint8_t a[16]; > +} xen_uuid_t; > + > +/* > + * XEN_DEFINE_UUID(0x00112233, 0x4455, 0x6677, 0x8899, > + * 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff) > + * will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as > + * {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, > + * 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; > + * > + * NB: This is compatible with Linux kernel and with libuuid, but it is > not > + * compatible with Microsoft, as they use mixed-endian encoding (some > + * components are little-endian, some are big-endian). > + */ > +#define XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)\ > +{{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \ > + ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \ > + ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \ > + ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \ > + ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \ > +e1, e2, e3, e4, e5, e6}} > + > +/* Compound literals are supported in C99 and later. */ > +#if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L I didn't notice this earlier - why no check for __GNUC__? >>> I have seen pattern "#if defined (__STDC_VERSION__) && __STDC_VERSION__ >> >> But if you check, all of the existing ones have >> "#elif defined(__GNUC__)". > Yes. But there was a reason do to so. For example: > > #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L > uint32_t optarr[]; > #elif defined(__GNUC__) > uint32_t optarr[0]; > #endif > > (xen/include/public/physdev.h:303) > > If compiler is C99 then we use flexible length array, else if compiller > is GCC, we use zero-length array, which is GCC extension (correct me). > Other compilers (non-gcc C90 compatible) are not supported. Probably > this is a bug. Why would that be a bug? In C89 you simply have no way to express what we want, so people using plain C89 compilers will have to code differently (i.e. without using the optarr field) anyway. > Another case is even worse: > > #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L > unsigned char buf[]; > #elif defined(__GNUC__) > unsigned char buf[1]; /* OUT: Variable length buffer with > build_id. */ > > (xen/include/public/version.h:49) That's not very reasonable indeed. > My code does not use gcc-only extensions like zero-length arrays, so I > don't see how #elif defined (__GNUC__) can fit in the my case. Did you check under what conditions the various gcc versions define __STDC_VERSION__? Trying to compile int test(void) { return __STDC_VERSION__; } with e.g. gcc 4.3.4 fails (with no other options specified). Otoh even using -ansi compound literals compile fine. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 04/11] public: xen.h: add definitions for UUID handling
>>> On 11.10.17 at 13:57, wrote: > +/* Compound literals are supported in C99 and later. */ > +#if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \ > +defined (__GNUC__) Strictly speaking the comment above is now stale. Also please don't put a space between "defined" and the opening parenthesis. If you check, the only two other such instances throughout the public headers are two ARM conditionals. And then the whole thing can be made fit on one line: #if defined(__STDC_VERSION__) ? __STDC_VERSION__ >= 199901L : defined(__GNUC__) > +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ > +((xen_uuid_t)XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6)) > +#else > +#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \ > +XEN_DEFINE_UUID_(a, b, c, d, e1, e2, e3, e4, e5, e6) > +#endif /* defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L */ Same for the comment here. I'd suggest either dropping it altogether or shortening it (e.g. /* __STDC_VERSION__ / __GNUC__ */) All of these adjustments ought to be doable while committing. With them done Acked-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libelf: allow having HYPERCALL_PAGE entry before VIRT_BASE in __xen_guest section.
>>> On 11.10.17 at 14:03, wrote: > When filling __xen_guest section of a guest, user may define > HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an > incorrect hypercall page address since an undefined virt_base could be > used to compute hypercall page address. > If there is no VIRT_BASE entry in __xen_guest section, default value of > 0 is used for virt_base. Thus, setting hypercall page address to > HYPERCALL_PAGE value is correct in this case too. Do you have an example of a guest kernel that is doing this? I ask because the __xen_guest section has been deprecated for many years - everyone should be using the notes section instead nowadays. > --- a/xen/common/libelf/libelf-dominfo.c > +++ b/xen/common/libelf/libelf-dominfo.c > @@ -330,14 +330,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct > elf_binary *elf, > > /* longs */ > if ( !strcmp(name, "VIRT_BASE") ) > +{ > parms->virt_base = strtoull(value, NULL, 0); > +if ( parms->virt_hypercall != UNSET_ADDR ) > +params->virt_hypercall += params->virt_base; > +} > if ( !strcmp(name, "VIRT_ENTRY") ) > parms->virt_entry = strtoull(value, NULL, 0); > if ( !strcmp(name, "ELF_PADDR_OFFSET") ) > parms->elf_paddr_offset = strtoull(value, NULL, 0); > if ( !strcmp(name, "HYPERCALL_PAGE") ) > -parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + > -parms->virt_base; > +{ > +parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); > +if ( parms->virt_base != UNSET_ADDR ) > +params->virt_hypercall += params->virt_base; > +} I think you rather want to do this once after the loop. That'll then also take care of there being multiple VIRT_BASE entries. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/hvm: Remove unnecessary is_hvm_domain() test in construct_vmcs()
>>> On 20.09.17 at 21:50, wrote: > It's a leftover from PVHv1 days. > > Signed-off-by: Boris Ostrovsky I've applied this despite the still missing VMX maintainer ack, for it being simple enough. But in general it should be you to chase missing acks. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libelf: allow having HYPERCALL_PAGE entry before VIRT_BASE in __xen_guest section.
On Wed, Oct 11, 2017 at 06:40:14AM -0600, Jan Beulich wrote: > >>> On 11.10.17 at 14:03, wrote: > > When filling __xen_guest section of a guest, user may define > > HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an > > incorrect hypercall page address since an undefined virt_base could be > > used to compute hypercall page address. > > If there is no VIRT_BASE entry in __xen_guest section, default value of > > 0 is used for virt_base. Thus, setting hypercall page address to > > HYPERCALL_PAGE value is correct in this case too. > > Do you have an example of a guest kernel that is doing this? I > ask because the __xen_guest section has been deprecated for > many years - everyone should be using the notes section > instead nowadays. > There is no public example AFAIK, I faced this issue while creating a guest locally for testing purpose. > > --- a/xen/common/libelf/libelf-dominfo.c > > +++ b/xen/common/libelf/libelf-dominfo.c > > @@ -330,14 +330,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct > > elf_binary *elf, > > > > /* longs */ > > if ( !strcmp(name, "VIRT_BASE") ) > > +{ > > parms->virt_base = strtoull(value, NULL, 0); > > +if ( parms->virt_hypercall != UNSET_ADDR ) > > +params->virt_hypercall += params->virt_base; > > +} > > if ( !strcmp(name, "VIRT_ENTRY") ) > > parms->virt_entry = strtoull(value, NULL, 0); > > if ( !strcmp(name, "ELF_PADDR_OFFSET") ) > > parms->elf_paddr_offset = strtoull(value, NULL, 0); > > if ( !strcmp(name, "HYPERCALL_PAGE") ) > > -parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + > > -parms->virt_base; > > +{ > > +parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); > > +if ( parms->virt_base != UNSET_ADDR ) > > +params->virt_hypercall += params->virt_base; > > +} > > I think you rather want to do this once after the loop. That'll > then also take care of there being multiple VIRT_BASE entries. > I will send an updated patch with your suggestion. Feel free to ignore it if usage of __xen_guest section is deprecated. Thanks, Gregory ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH xtf] libc: Fix strcpy() assignment mistake
From: Paul Semel the strcpy function was doing a comparison instead of doing an assignment. Signed-off-by: Paul Semel Reviewed-by: Pawel Wieczorkiewicz Reviewed-by: Bjoern Doebel Reviewed-by: Martin Pohlack --- common/libc/string.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/libc/string.c b/common/libc/string.c index 94acc7e..967f2fa 100644 --- a/common/libc/string.c +++ b/common/libc/string.c @@ -24,7 +24,7 @@ size_t strnlen(const char *str, size_t max) { char *p = dst; -while ( *p++ == *src++ ) +while ( (*p++ = *src++) ) ; return dst; -- 1.8.3.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 06/16] x86: implement get hw info flow for MBA
>>> On 08.10.17 at 09:23, wrote: > 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 with one further adjustment: > --- 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_FLAG 2 > +#define PSR_INFO_IDX_MBA_THRTL_MAX 1 > +#define PSR_INFO_IDX_MBA_FLAG 2 PSR_INFO_IDX_MBA_FLAGS please, even if right now there's only one flag. The CAT equivalent wants changing too (perhaps in an earlier patch). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH xtf] libc: Fix strcpy() assignment mistake
On 11/10/17 14:07, Paul Semel wrote: > From: Paul Semel > > the strcpy function was doing a comparison instead of doing an > assignment. > > Signed-off-by: Paul Semel > > Reviewed-by: Pawel Wieczorkiewicz > Reviewed-by: Bjoern Doebel > Reviewed-by: Martin Pohlack Oops. This issue is hidden due to __builtin_strcpy() optimising all in-tree callsites. Reviewed-and-tested-by: Andrew Cooper , and pushed. > --- > common/libc/string.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/common/libc/string.c b/common/libc/string.c > index 94acc7e..967f2fa 100644 > --- a/common/libc/string.c > +++ b/common/libc/string.c > @@ -24,7 +24,7 @@ size_t strnlen(const char *str, size_t max) It looks like git isn't terribly happy with the (strcpy) preprocessor trick. I did a double-take when I first read the patch. ~Andrew > { > char *p = dst; > > -while ( *p++ == *src++ ) > +while ( (*p++ = *src++) ) > ; > > return dst; ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2] libelf: allow having HYPERCALL_PAGE entry before VIRT_BASE in __xen_guest section.
From: Gregory Herrero When filling __xen_guest section of a guest, user may define HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an incorrect hypercall page address since an undefined virt_base could be used to compute hypercall page address. If there is no VIRT_BASE entry in __xen_guest section, default value of 0 is used for virt_base. Thus, setting hypercall page address to HYPERCALL_PAGE value is correct in this case too. Signed-off-by: Gregory Herrero --- Changed since v1: * set virt_hypercall once after the while loop. --- xen/common/libelf/libelf-dominfo.c | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index a52900c00cd..eaeb774ff3d 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -269,6 +269,7 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, elf_ptrval h; unsigned char name[32], value[128]; unsigned len; +elf_errorstatus ret = 0; h = parms->guest_info; #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1)) @@ -336,16 +337,22 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, if ( !strcmp(name, "ELF_PADDR_OFFSET") ) parms->elf_paddr_offset = strtoull(value, NULL, 0); if ( !strcmp(name, "HYPERCALL_PAGE") ) -parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + -parms->virt_base; +parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); /* other */ if ( !strcmp(name, "FEATURES") ) if ( elf_xen_parse_features(value, parms->f_supported, parms->f_required) ) -return -1; +{ +ret = -1; +break; +} } -return 0; +if ( (parms->virt_base != UNSET_ADDR) && + (parms->virt_hypercall != UNSET_ADDR) ) +parms->virt_hypercall += parms->virt_base; + +return ret; } /* */ -- 2.14.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 08/16] x86: implement set value flow for MBA
>>> On 08.10.17 at 09:23, wrote: > --- 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); > + > +/* > + * check_val is used to check if input val fulfills SDM requirement. > + * Change it to valid value if SDM allows. > + */ > +bool (*check_val)(const struct feat_node *feat, unsigned long *val); I'm pretty sure I've said so before - "check" to me implies all r/o inputs. Perhaps sanitize_val() or even just sanitize()? And why unsigned long when the only caller has a uint32_t in its hands? > @@ -274,30 +280,30 @@ 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, unsigned long *cbm) > { > unsigned int first_bit, zero_bit; > +unsigned int cbm_len = feat->cat.cbm_len; > > -/* 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 only in the range of [0, cbm_len]. As you alter the comment anyway, please also add the missing "be". Also - isn't the upper bound of the range exclusive, i.e. shouldn't this be [0, cbm_len)? > + * And, at least one bit need to be set. > + */ > +if ( *cbm & (~0ul << cbm_len) || *cbm == 0 ) Parentheses missing for the left side operand of || and if you omit != 0 on the left part (which I appreciate) please also use ! instead of == 0 on the right side. > +static bool mba_check_thrtl(const struct feat_node *feat, unsigned long > *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 ) > +{ > +unsigned int mod; > + > +if ( feat->mba.thrtl_max >= 100 ) > +return false; Don't you check this right after collecting CPUID output? If so, this should be at most an ASSERT(). > +mod = *thrtl % (100 - feat->mba.thrtl_max); > +*thrtl -= mod; Do you really need the intermediate variable? > +} > +else > +{ > +/* Not power of 2. */ > +if ( *thrtl & (*thrtl - 1) ) > +*thrtl = *thrtl & (1 << (flsl(*thrtl) - 1)); &= or even simply =. > @@ -950,6 +997,7 @@ static int insert_val_into_array(uint32_t val[], > const struct feat_node *feat; > const struct feat_props *props; > unsigned int i; > +unsigned long check_val = new_val; > int ret; > > ASSERT(feat_type < FEAT_TYPE_NUM); > @@ -974,9 +1022,11 @@ static int insert_val_into_array(uint32_t val[], > if ( array_len < props->cos_num ) > return -ENOSPC; > > -if ( !psr_check_cbm(feat->cat.cbm_len, new_val) ) > +if ( !props->check_val(feat, &check_val) ) > return -EINVAL; > > +new_val = check_val; When the function pointer's parameter changes to uint32_t * you won't need the intermediate variable anymore afaict. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] libelf: allow having HYPERCALL_PAGE entry before VIRT_BASE in __xen_guest section.
>>> On 11.10.17 at 15:35, wrote: > @@ -336,16 +337,22 @@ elf_errorstatus elf_xen_parse_guest_info(struct > elf_binary *elf, > if ( !strcmp(name, "ELF_PADDR_OFFSET") ) > parms->elf_paddr_offset = strtoull(value, NULL, 0); > if ( !strcmp(name, "HYPERCALL_PAGE") ) > -parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + > -parms->virt_base; > +parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); Please also drop the now pointless parentheses. > /* other */ > if ( !strcmp(name, "FEATURES") ) > if ( elf_xen_parse_features(value, parms->f_supported, > parms->f_required) ) > -return -1; > +{ > +ret = -1; > +break; > +} > } > -return 0; > +if ( (parms->virt_base != UNSET_ADDR) && Please add a blank line ahead of this addition. With those taken care of Reviewed-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 114346: regressions - FAIL
flight 114346 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114346/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl 7 xen-boot fail REGR. vs. 114299 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass version targeted for testing: xen f17d2cd2ffeda70aba8788910e9d088415562c8b baseline version: xen 3b40cfcd1a1912c2e4c4eb353dc77cbf35c63c3a Last test of basis 114299 2017-10-10 21:02:54 Z0 days Failing since114308 2017-10-10 23:01:10 Z0 days5 attempts Testing same since 114318 2017-10-11 02:19:34 Z0 days4 attempts People who touched revisions under test: Andre Przywara Julien Grall Stefano Stabellini jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl fail 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 Not pushing. commit f17d2cd2ffeda70aba8788910e9d088415562c8b Author: Andre Przywara Date: Sat Oct 7 01:06:40 2017 +0100 ARM: sunxi: support more Allwinner SoCs So far we only supported the Allwinner A20 SoC. Add support for most of the other virtualization capable Allwinner SoCs by: - supporting the watchdog in newer (sun8i) SoCs - getting the watchdog address from DT - adding compatible strings for other 32-bit SoCs - adding compatible strings for 64-bit SoCs As all 64-bit SoCs support system reset via PSCI, we don't use the platform specific reset routine there. Should the 32-bit SoCs start to properly support the PSCI 0.2 SYSTEM_RESET call, we will use it for them automatically, as we try PSCI first, then fall back to platform reset. Signed-off-by: Andre Przywara Signed-off-by: Stefano Stabellini Reviewed-by: Stefano Stabellini commit e2dfe4a037b0c6ccfd2375e4b60668109a0118e5 Author: Julien Grall Date: Mon Oct 9 14:23:41 2017 +0100 xen/arm: mm: Use memory flags for modify_xen_mappings rather than custom one This will help to consolidate the page-table code and avoid different path depending on the action to perform. Signed-off-by: Julien Grall Reviewed-by: Andre Przywara Reviewed-by: Stefano Stabellini Reviewed-by: Konrad Rzeszutek Wilk commit 6b88beed40c756aaff018d286f4de31351240a93 Author: Julien Grall Date: Mon Oct 9 14:23:40 2017 +0100 xen/arm: mm: Handle permission flags when adding a new mapping Currently, all the new mappings will be read-write non-executable. Allow the caller to use other permissions. Signed-off-by: Julien Grall Reviewed-by: Stefano Stabellini commit 28f2ad440a08908010abec43b7ccc3283051e943 Author: Julien Grall Date: Mon Oct 9 14:23:39 2017 +0100 xen/arm: mm: Embed permission in the flags Currently, it is not possible to specify the permission of a new mapping. It would be necessary to use the function modify_xen_mappings with a different set of flags. Introduce a couple of new flags for the permissions (Non-eXecutable, Read-Only) and also provides definition that combine the memory attribute and permission for common combinations. PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write, non-executable mappings). This does not affect the current mapping using PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be non-executable by default (see mfn_to_xen_entry). A follow-up patch will change modify_xen_mappings to use the new flags. Signed-off-by: Julien Grall Reviewed-by: Stefano Stabellini Signed-off-by: Stefano Stabellini commit 5f3edb5f32e511b915d173403d0b7b5ea38e00ad Author: Julien Grall Date: Mon Oct 9 14:23:38 2017 +0100 xen/arm: page: Describe the layout of flags used to update page tables
Re: [Xen-devel] [PATCH 27/27 v10] xen/arm: vpl011: Correct the logic for asserting/de-asserting SBSA UART TX interrupt
On 11 October 2017 at 15:38, Dave Martin wrote: > On Wed, Oct 11, 2017 at 01:28:44PM +0530, Bhupinder Thakur wrote: >> Hi Dave, >> >> On 26 September 2017 at 20:08, Dave Martin wrote: >> > On Fri, Sep 22, 2017 at 01:53:26PM +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 >> >> --- >> >> CC: Julien Grall >> >> CC: Andre Przywara >> >> CC: Stefano Stabellini >> > >> > (Taking a quick look at this because I remember fighthing with FIFO >> > behaviour issues when hacking the Linux driver -- but beware, I'm not a >> > Xen guy...) >> > >> > >> > Should this patch be flattened into the patches is fixes? Keeping >> > known-wrong code in the series does not help reviewers (but maybe it's >> > the Xen way). >> > >> >> 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 >> > >> > What's sizeof(intf->in), sizeof(intf->out)? >> > >> > For correct operation, you assume that the total ring buffer size is >= >> > SBSA_UART_FIFO_SIZE, but nothing enforces this. If the xencons ring >> > buffer size is set elsewhere and can't be chosen by a driver, you may at >> > least add a build-time assert to check that it's big enough. >> > >> I will add an assert to check this condition. >> >> > [...] >> > >> >> @@ -144,28 +148,41 @@ static void vpl011_write_data(struct domain *d, >> >> uint8_t data) >> > >> > [...] >> > >> >> + * Clear the TXI bit if the fifo level exceeds fifo_size/2 mark >> >> which >> >> + * is the trigger level for asserting/de-assterting the TX >> >> interrupt. >> >> */ >> >> -vpl011->uartfr |= BUSY; >> >> +fifo_threshold = sizeof (intf->out) - SBSA_UART_FIFO_SIZE/2; >> >> + >> >> +if ( fifo_level <= fifo_threshold ) >> >> +vpl011->uartris |= TXI; >> >> +else >> >> +vpl011->uartris &= ~TXI; >> >> } >> >> +else >> >> +gprintk(XENLOG_ERR, "vpl011: Unexpected OUT ring buffer full\n"); >> >> >> >> vpl011->uartfr &= ~TXFE; >> >> >> > >> > [...] >> > >> >> @@ -353,37 +370,51 @@ static void vpl011_data_avail(struct domain *d) >> >> >> >> smp_rmb(); >> >> >> >> -in_ring_qsize = xencons_queued(in_prod, >> >> +in_fifo_level = xencons_queued(in_prod, >> >> in_cons, >> >> sizeof(intf->in)); >> >> >> >> -out_ring_qsize = xencons_queued(out_prod, >> >> +out_fifo_level = xencons_queued(out_prod, >> >> out_cons, >> >> sizeof(intf->out)); >> >> >> >> /* Update the uart rx state if the buffer is not empty. */ >> >> -if ( in_ring_qsize != 0 ) >> >> +if ( in_fifo_level != 0 ) >> >> { >> >> vpl011->uartfr &= ~RXFE; >> >> -if ( in_ring_qsize == sizeof(intf->in) ) >> >> + >> >> +if ( in_fifo_level == sizeof(intf->in) ) >> >> vpl011->uartfr |= RXFF; >> >> + >> >> vpl011->uartris |= RXI; >> >> } >> >> >> >> /* Update the uart tx state if the buffer is not full. */ >> >> -if ( out_ring_qsize != sizeof(intf->out) ) >> >> +if ( out_fifo_level != sizeof(intf->out) ) >> >> { >> >> +unsigned int out_fifo_threshold; >> >> + >> >> vpl011->uartfr &= ~TXFF; >> >> -vpl011->uartris |= TXI; >> >> >> >> /* >> >> - * Clear the BUSY bit as soon as space becomes available >> >>
[Xen-devel] [PATCH v3] libelf: allow having HYPERCALL_PAGE entry before VIRT_BASE in __xen_guest section.
From: Gregory Herrero When filling __xen_guest section of a guest, user may define HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an incorrect hypercall page address since an undefined virt_base could be used to compute hypercall page address. If there is no VIRT_BASE entry in __xen_guest section, default value of 0 is used for virt_base. Thus, setting hypercall page address to HYPERCALL_PAGE value is correct in this case too. Signed-off-by: Gregory Herrero Reviewed-by: Jan Beulich --- Changed since v1: * set virt_hypercall once after the while loop. * Correct coding style. --- xen/common/libelf/libelf-dominfo.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index a52900c00cd..829d5176a91 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -269,6 +269,7 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, elf_ptrval h; unsigned char name[32], value[128]; unsigned len; +elf_errorstatus ret = 0; h = parms->guest_info; #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1)) @@ -336,16 +337,23 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, if ( !strcmp(name, "ELF_PADDR_OFFSET") ) parms->elf_paddr_offset = strtoull(value, NULL, 0); if ( !strcmp(name, "HYPERCALL_PAGE") ) -parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + -parms->virt_base; +parms->virt_hypercall = strtoull(value, NULL, 0) << 12; /* other */ if ( !strcmp(name, "FEATURES") ) if ( elf_xen_parse_features(value, parms->f_supported, parms->f_required) ) -return -1; +{ +ret = -1; +break; +} } -return 0; + +if ( (parms->virt_base != UNSET_ADDR) && + (parms->virt_hypercall != UNSET_ADDR) ) +parms->virt_hypercall += parms->virt_base; + +return ret; } /* */ -- 2.14.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] X86EMUL_CMPXCHG_FAILED
Hello, Now that "x86/hvm: implement hvmemul_write() using real mappings" is in, we can start working again on implementing hvmemul_cmpxchg() with a real CMPXCHG, and finally fix the SMP emulation race upstream. However, in order to do that we would need X86EMUL_CMPXCHG_FAILED which has been dropped by Andrew here: https://patchwork.kernel.org/patch/9449339/ and re-contributed by Jan here: https://patchwork.kernel.org/patch/9651613/ (the patch is a combination of Jan's patch and my fumbling with CMPXCHG). However, I remember Jan saying that his patch is no longer the way to go here. How should we proceed? Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] VPMU interrupt unreliability
On 10/10/2017 12:54 PM, Kyle Huey wrote: > On Mon, Jul 24, 2017 at 9:54 AM, Kyle Huey wrote: >> On Mon, Jul 24, 2017 at 8:07 AM, Boris Ostrovsky >> wrote: > One thing I noticed is that the workaround doesn't appear to be > complete: it is only checking PMC0 status and not other counters (fixed > or architectural). Of course, without knowing what the actual problem > was it's hard to say whether this was intentional. handle_pmc_quirk appears to loop through all the counters ... >>> Right, I didn't notice that it is shifting MSR_CORE_PERF_GLOBAL_STATUS >>> value one by one and so it is looking at all bits. >>> >> 2. Intercepting MSR loads for counters that have the workaround >> applied and giving the guest the correct counter value. > We'd have to keep track of whether the counter has been reset (by the > quirk) since the last MSR write. Yes. >> 3. Or perhaps even changing the workaround to disable the PMI on that >> counter until the guest acks via GLOBAL_OVF_CTRL, assuming that works >> on the relevant hardware. > MSR_CORE_PERF_GLOBAL_OVF_CTRL is written immediately after the quirk > runs (in core2_vpmu_do_interrupt()) so we already do this, don't we? I'm suggesting waiting until the *guest* writes to the (virtualized) GLOBAL_OVF_CTRL. >>> Wouldn't it be better to wait until the counter is reloaded? >> Maybe! I haven't thought through it a lot. It's still not clear to >> me whether MSR_CORE_PERF_GLOBAL_OVF_CTRL actually controls the >> interrupt in any way or whether it just resets the bits in >> MSR_CORE_PERF_GLOBAL_STATUS and acking the interrupt on the APIC is >> all that's required to reenable it. >> >> - Kyle > I wonder if it would be reasonable to just remove the workaround > entirely at some point. The set of people using 1) several year old > hardware, 2) an up to date Xen, and 3) the off-by-default performance > counters is probably rather small. We'd probably want to only enable this for affected processors, not remove it outright. But the problem is that we still don't know for sure whether this issue affects NHM only, do we? (https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg02242.html is the original message) -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] X86EMUL_CMPXCHG_FAILED
>>> On 11.10.17 at 15:59, wrote: > Now that "x86/hvm: implement hvmemul_write() using real mappings" is in, > we can start working again on implementing hvmemul_cmpxchg() with a real > CMPXCHG, and finally fix the SMP emulation race upstream. That's my plan, but this will end up being complete only with the RMW patch I had also posted, and which now can be made use of the hvm/emulate.c. I'm intending to get to this as soon as time permits. > However, in order to do that we would need X86EMUL_CMPXCHG_FAILED which > has been dropped by Andrew here: > > https://patchwork.kernel.org/patch/9449339/ > > and re-contributed by Jan here: > > https://patchwork.kernel.org/patch/9651613/ > > (the patch is a combination of Jan's patch and my fumbling with CMPXCHG). > > However, I remember Jan saying that his patch is no longer the way to go > here. How should we proceed? Well, the patch will need some re-working, so that the end result will fit with the RMW one (in particular I also want to eliminate the multiple reads issue we have), but right now I don't recall any reason why it wouldn't be suitable (anymore) at all. But this is all work targeted at 4.11, so not really a need to rush. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH for-4.10] xen/arm: mm: Rework MAIR* definitions to handle 32-bit compilation environment
Commit a0543df403 "xen/arm: page: Clean-up the definition of MAIRVAL" combined the definition of MAIR0VAL and MAIR1VAL in MAIRVAL. Sadly, when building in 32-bit environment, the assembler is unable to compute 64-bit constant and will ignore the 32-bit most-significants bits. This will result of MAIR1 set 0. Rather than fully reverting the offending commit, the code is reworked to still avoid hardcoded values but split the definition in 2. Lastly, a comment is added to avoid trying to blindly combine the both definition again in the future. Signed-off-by: Julien Grall --- xen/include/asm-arm/page.h | 23 ++- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index f558184e10..d948250a4a 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -52,18 +52,23 @@ * ?? 101 * reserved 110 * MT_NORMAL111 -- Write-back write-allocate + * + * /!\ It is not possible to combine the definition in MAIRVAL and then + * split because it would result to a 64-bit value that some assembler + * doesn't understand. */ -#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8)) +#define _MAIR0(attr, mt) (_AC(attr, ULL) << ((mt) * 8)) +#define _MAIR1(attr, mt) (_AC(attr, ULL) << (((mt) * 8) - 32)) + +#define MAIR0VAL (_MAIR0(0x00, MT_DEVICE_nGnRnE)| \ + _MAIR0(0x44, MT_NORMAL_NC)| \ + _MAIR0(0xaa, MT_NORMAL_WT)| \ + _MAIR0(0xee, MT_NORMAL_WB)) -#define MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \ - MAIR(0x44, MT_NORMAL_NC)| \ - MAIR(0xaa, MT_NORMAL_WT)| \ - MAIR(0xee, MT_NORMAL_WB)| \ - MAIR(0x04, MT_DEVICE_nGnRE) | \ - MAIR(0xff, MT_NORMAL)) +#define MAIR1VAL (_MAIR1(0x04, MT_DEVICE_nGnRE) | \ + _MAIR1(0xff, MT_NORMAL)) -#define MAIR0VAL (MAIRVAL & 0x) -#define MAIR1VAL (MAIRVAL >> 32) +#define MAIRVAL (MAIR1VAL << 32 | MAIR0VAL) /* * Layout of the flags used for updating the hypervisor page tables -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v9 3/3] tools/libxc: use superpages during restore of HVM guest
On Fri, Sep 08, Olaf Hering wrote: > A related question: is it save to increase MAX_BATCH_SIZE from 1024 to > (256*1024) to transfer a whole gigabyte at a time? That way it will be > easier to handle holes within a 1GB superpage. To answer my own question: This change leads to this error: -#define MAX_BATCH_SIZE 1024 /* up to 1024 pages (4MB) at a time */ +#define MAX_BATCH_SIZE SUPERPAGE_1GB_NR_PFNS /* up to 1GB at a time */ ... xc: info: Found x86 HVM domain from Xen 4.10 xc: detail: dom 9 p2m_size fee01 max_pages 100100 xc: info: Restoring domain xc: error: Failed to read Record Header from stream (0 = Success): Internal error xc: error: Restore failed (0 = Success): Internal error ... Olaf signature.asc Description: PGP signature ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v10 1/3] tools/libxc: move SUPERPAGE macros to common header
The macros SUPERPAGE_2MB_SHIFT and SUPERPAGE_1GB_SHIFT will be used by other code in libxc. Move the macros to a header file. Signed-off-by: Olaf Hering Acked-by: Wei Liu --- tools/libxc/xc_dom_x86.c | 5 - tools/libxc/xc_private.h | 5 + 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index cb68efcbd3..5aff5cad58 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -43,11 +43,6 @@ #define SUPERPAGE_BATCH_SIZE 512 -#define SUPERPAGE_2MB_SHIFT 9 -#define SUPERPAGE_2MB_NR_PFNS (1UL << SUPERPAGE_2MB_SHIFT) -#define SUPERPAGE_1GB_SHIFT 18 -#define SUPERPAGE_1GB_NR_PFNS (1UL << SUPERPAGE_1GB_SHIFT) - #define X86_CR0_PE 0x01 #define X86_CR0_ET 0x10 diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h index 1c27b0fded..d581f850b0 100644 --- a/tools/libxc/xc_private.h +++ b/tools/libxc/xc_private.h @@ -66,6 +66,11 @@ struct iovec { #define DECLARE_FLASK_OP struct xen_flask_op op #define DECLARE_PLATFORM_OP struct xen_platform_op platform_op +#define SUPERPAGE_2MB_SHIFT 9 +#define SUPERPAGE_2MB_NR_PFNS (1UL << SUPERPAGE_2MB_SHIFT) +#define SUPERPAGE_1GB_SHIFT 18 +#define SUPERPAGE_1GB_NR_PFNS (1UL << SUPERPAGE_1GB_SHIFT) + #undef PAGE_SHIFT #undef PAGE_SIZE #undef PAGE_MASK ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v10 2/3] tools/libxc: add API for bitmap access for restore
Extend API for managing bitmaps. Each bitmap is now represented by a generic struct xc_sr_bitmap. Switch the existing populated_pfns to this API. Signed-off-by: Olaf Hering Acked-by: Wei Liu --- tools/libxc/xc_sr_common.c | 41 + tools/libxc/xc_sr_common.h | 73 +++-- tools/libxc/xc_sr_restore.c | 66 ++-- 3 files changed, 115 insertions(+), 65 deletions(-) diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c index 79b9c3e940..28c7be2b15 100644 --- a/tools/libxc/xc_sr_common.c +++ b/tools/libxc/xc_sr_common.c @@ -155,6 +155,47 @@ static void __attribute__((unused)) build_assertions(void) BUILD_BUG_ON(sizeof(struct xc_sr_rec_hvm_params)!= 8); } +/* + * Expand the tracking structures as needed. + * To avoid realloc()ing too excessively, the size increased to the nearest power + * of two large enough to contain the required number of bits. + */ +bool _xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits) +{ +if ( bits > bm->bits ) +{ +size_t new_max; +size_t old_sz, new_sz; +void *p; + +/* Round up to the nearest power of two larger than bit, less 1. */ +new_max = bits; +new_max |= new_max >> 1; +new_max |= new_max >> 2; +new_max |= new_max >> 4; +new_max |= new_max >> 8; +new_max |= new_max >> 16; +#ifdef __x86_64__ +new_max |= new_max >> 32; +#endif + +old_sz = bitmap_size(bm->bits + 1); +new_sz = bitmap_size(new_max + 1); +p = realloc(bm->p, new_sz); +if ( !p ) +return false; + +if (bm->p) +memset(p + old_sz, 0, new_sz - old_sz); +else +memset(p, 0, new_sz); + +bm->p = p; +bm->bits = new_max; +} +return true; +} + /* * Local variables: * mode: C diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h index a83f22af4e..a728c93e53 100644 --- a/tools/libxc/xc_sr_common.h +++ b/tools/libxc/xc_sr_common.h @@ -172,6 +172,12 @@ struct xc_sr_x86_pv_restore_vcpu size_t basicsz, extdsz, xsavesz, msrsz; }; +struct xc_sr_bitmap +{ +void *p; +unsigned long bits; +}; + struct xc_sr_context { xc_interface *xch; @@ -255,8 +261,7 @@ struct xc_sr_context domid_t xenstore_domid, console_domid; /* Bitmap of currently populated PFNs during restore. */ -unsigned long *populated_pfns; -xen_pfn_t max_populated_pfn; +struct xc_sr_bitmap populated_pfns; /* Sender has invoked verify mode on the stream. */ bool verify; @@ -343,6 +348,70 @@ extern struct xc_sr_save_ops save_ops_x86_hvm; extern struct xc_sr_restore_ops restore_ops_x86_pv; extern struct xc_sr_restore_ops restore_ops_x86_hvm; +bool _xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits); + +static inline bool xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits) +{ +if ( bits > bm->bits ) +return _xc_sr_bitmap_resize(bm, bits); +return true; +} + +static inline void xc_sr_bitmap_free(struct xc_sr_bitmap *bm) +{ +free( bm->p ); +bm->bits = 0; +bm->p = NULL; +} + +static inline bool xc_sr_set_bit(unsigned long bit, struct xc_sr_bitmap *bm) +{ +if ( !xc_sr_bitmap_resize(bm, bit) ) +return false; + +set_bit(bit, bm->p); +return true; +} + +static inline bool xc_sr_test_bit(unsigned long bit, struct xc_sr_bitmap *bm) +{ +if ( bit > bm->bits || !bm->bits ) +return false; +return !!test_bit(bit, bm->p); +} + +static inline bool xc_sr_test_and_clear_bit(unsigned long bit, struct xc_sr_bitmap *bm) +{ +if ( bit > bm->bits || !bm->bits ) +return false; +return !!test_and_clear_bit(bit, bm->p); +} + +static inline bool xc_sr_test_and_set_bit(unsigned long bit, struct xc_sr_bitmap *bm) +{ +if ( bit > bm->bits || !bm->bits ) +return false; +return !!test_and_set_bit(bit, bm->p); +} + +static inline bool pfn_is_populated(struct xc_sr_context *ctx, xen_pfn_t pfn) +{ +return xc_sr_test_bit(pfn, &ctx->restore.populated_pfns); +} + +static inline int pfn_set_populated(struct xc_sr_context *ctx, xen_pfn_t pfn) +{ +xc_interface *xch = ctx->xch; + +if ( !xc_sr_set_bit(pfn, &ctx->restore.populated_pfns) ) +{ +ERROR("Failed to realloc populated_pfns bitmap"); +errno = ENOMEM; +return -1; +} +return 0; +} + struct xc_sr_record { uint32_t type; diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c index a016678332..d53948e1a6 100644 --- a/tools/libxc/xc_sr_restore.c +++ b/tools/libxc/xc_sr_restore.c @@ -68,64 +68,6 @@ static int read_headers(struct xc_sr_context *ctx) return 0; } -/* - * Is a pfn populated? - */ -static bool pfn_is_populated(const struct xc_sr_context *ctx, xen_pfn_t pfn) -{ -if ( pfn > ctx-
[Xen-devel] [PATCH v10 3/3] tools/libxc: use superpages during restore of HVM guest
During creating of a HVM domU meminit_hvm() tries to map superpages. After save/restore or migration this mapping is lost, everything is allocated in single pages. This causes a performance degradition after migration. Add neccessary code to preallocate a superpage for the chunk of pfns that is received. In case a pfn was not populated on the sending side it must be freed on the receiving side to avoid over-allocation. The existing code for x86_pv is moved unmodified into its own file. Signed-off-by: Olaf Hering --- tools/libxc/xc_sr_common.h | 30 +- tools/libxc/xc_sr_restore.c | 75 + tools/libxc/xc_sr_restore_x86_hvm.c | 536 tools/libxc/xc_sr_restore_x86_pv.c | 72 - 4 files changed, 635 insertions(+), 78 deletions(-) diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h index a728c93e53..0477c20617 100644 --- a/tools/libxc/xc_sr_common.h +++ b/tools/libxc/xc_sr_common.h @@ -139,6 +139,15 @@ struct xc_sr_restore_ops */ int (*setup)(struct xc_sr_context *ctx); +/** + * Populate PFNs + * + * Given a set of pfns, obtain memory from Xen to fill the physmap for the + * unpopulated subset. + */ +int (*populate_pfns)(struct xc_sr_context *ctx, unsigned count, + const xen_pfn_t *original_pfns, const uint32_t *types); + /** * Process an individual record from the stream. The caller shall take * care of processing common records (e.g. END, PAGE_DATA). @@ -224,6 +233,8 @@ struct xc_sr_context int send_back_fd; unsigned long p2m_size; +unsigned long max_pages; +unsigned long tot_pages; xc_hypercall_buffer_t dirty_bitmap_hbuf; /* From Image Header. */ @@ -336,6 +347,17 @@ struct xc_sr_context /* HVM context blob. */ void *context; size_t contextsz; + +/* Bitmap of currently allocated PFNs during restore. */ +struct xc_sr_bitmap attempted_1g; +struct xc_sr_bitmap attempted_2m; +struct xc_sr_bitmap allocated_pfns; +xen_pfn_t idx1G_prev, idx2M_prev; + +/* List of PFNs for decrease_reservation */ +xen_pfn_t *extents; +unsigned long max_extents; +unsigned long nr_extents; } restore; }; } x86_hvm; @@ -460,14 +482,6 @@ static inline int write_record(struct xc_sr_context *ctx, */ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec); -/* - * This would ideally be private in restore.c, but is needed by - * x86_pv_localise_page() if we receive pagetables frames ahead of the - * contents of the frames they point at. - */ -int populate_pfns(struct xc_sr_context *ctx, unsigned count, - const xen_pfn_t *original_pfns, const uint32_t *types); - #endif /* * Local variables: diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c index d53948e1a6..8cd9289d1a 100644 --- a/tools/libxc/xc_sr_restore.c +++ b/tools/libxc/xc_sr_restore.c @@ -68,74 +68,6 @@ static int read_headers(struct xc_sr_context *ctx) return 0; } -/* - * Given a set of pfns, obtain memory from Xen to fill the physmap for the - * unpopulated subset. If types is NULL, no page type checking is performed - * and all unpopulated pfns are populated. - */ -int populate_pfns(struct xc_sr_context *ctx, unsigned count, - const xen_pfn_t *original_pfns, const uint32_t *types) -{ -xc_interface *xch = ctx->xch; -xen_pfn_t *mfns = malloc(count * sizeof(*mfns)), -*pfns = malloc(count * sizeof(*pfns)); -unsigned i, nr_pfns = 0; -int rc = -1; - -if ( !mfns || !pfns ) -{ -ERROR("Failed to allocate %zu bytes for populating the physmap", - 2 * count * sizeof(*mfns)); -goto err; -} - -for ( i = 0; i < count; ++i ) -{ -if ( (!types || (types && - (types[i] != XEN_DOMCTL_PFINFO_XTAB && - types[i] != XEN_DOMCTL_PFINFO_BROKEN))) && - !pfn_is_populated(ctx, original_pfns[i]) ) -{ -rc = pfn_set_populated(ctx, original_pfns[i]); -if ( rc ) -goto err; -pfns[nr_pfns] = mfns[nr_pfns] = original_pfns[i]; -++nr_pfns; -} -} - -if ( nr_pfns ) -{ -rc = xc_domain_populate_physmap_exact( -xch, ctx->domid, nr_pfns, 0, 0, mfns); -if ( rc ) -{ -PERROR("Failed to populate physmap"); -goto err; -} - -for ( i = 0; i < nr_pfns; ++i ) -{ -if ( mfns[i] == INVALID_MFN ) -{ -ERROR("Populate physmap failed for pfn %u", i); -rc = -1; -
[Xen-devel] [PATCH v10 0/3] tools/libxc: use superpages
Using superpages on the receiving dom0 will avoid performance regressions. Olaf v10: coding style in xc_sr_bitmap API reset bitmap size on free check for empty bitmap in xc_sr_bitmap API add comment to struct x86_hvm_sp, keep the short name style and type changes in x86_hvm_punch_hole do not mark VGA hole as busy in x86_hvm_setup call decrease_reservation once for all pfns rename variable in x86_hvm_populate_pfns call decrease_reservation in 2MB chucks if possible v9: update hole checking in x86_hvm_populate_pfns add out of bounds check to xc_sr_test_and_set/clear_bit v8: remove double check of 1G/2M idx in x86_hvm_populate_pfns v7: cover holes that span multiple superpages v6: handle freeing of partly populated superpages correctly more DPRINTFs v5: send correct version, rebase was not fully finished v4: restore trailing "_bit" in bitmap function names keep track of gaps between previous and current batch split alloc functionality in x86_hvm_allocate_pfn v3: clear pointer in xc_sr_bitmap_free some coding style changes use getdomaininfo.max_pages to avoid Over-allocation check trim bitmap function names, drop trailing "_bit" add some comments v2: split into individual commits based on staging c39cf093fc ("x86/asm: add .file directives") Olaf Hering (3): tools/libxc: move SUPERPAGE macros to common header tools/libxc: add API for bitmap access for restore tools/libxc: use superpages during restore of HVM guest tools/libxc/xc_dom_x86.c| 5 - tools/libxc/xc_private.h| 5 + tools/libxc/xc_sr_common.c | 41 +++ tools/libxc/xc_sr_common.h | 103 ++- tools/libxc/xc_sr_restore.c | 141 +- tools/libxc/xc_sr_restore_x86_hvm.c | 536 tools/libxc/xc_sr_restore_x86_pv.c | 72 - 7 files changed, 755 insertions(+), 148 deletions(-) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 114288: regressions - FAIL
flight 114288 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/114288/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-rumprun8 xen-buildfail REGR. vs. 114204 Tests which did not succeed, but are not blocking: test-amd64-i386-rumprun-i386 1 build-check(1) blocked n/a test-amd64-amd64-xl-pvh-intel 12 guest-start fail like 114204 test-amd64-amd64-xl-pvh-amd 12 guest-start fail like 114204 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail like 114204 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 114204 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 114204 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 114204 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 114204 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 114204 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 114204 test-amd64-amd64-xl-rtds 10 debian-install fail like 114204 test-amd64-amd64-xl-qemut-ws16-amd64 10 windows-installfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-installfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qcow2 12 migrate-support-checkfail never pass test-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-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 13 guest-saverestore fail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-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-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-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-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 version targeted for testing: xen 3c1ca29bd53570ffce049a297d18956f5d93ec8a baseline version: xen 572a78190403e5f2acbd01fa72c35fafe9700169 Last test of basis 114204 2017-10-09 19:19:08 Z1 days Testing same since 114288 2017-10-10 17:02:59 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Kevin Tian Tim Deegan jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf
[Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
The function get_ipa_output_size is check whether the input size configured by the guest is valid and will return it. The check is done with the IPS already shifted against TCR_EL1_IPS_48_BIT. However the constant has been defined with the shift included, resulting the check always been false. Fix it by doing the check on the non-shifted value. This was introduced by commit 7d623b358a "arm/mem_access: Add long-descriptor based gpt" introduced software page-table walk for stage-1. Note that the IPS code is now surrounded with #ifdef CONFIG_ARM_64 because the Arm32 compiler will complain of shift bigger than the width of the variable. This is fine as the code is executed for 64-bit domain only. Coverity-ID: 1457707 Signed-off-by: Julien Grall --- Cc: Sergej Proskurin Changes in v2: - Fix compilation on Arm32 --- xen/arch/arm/guest_walk.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c index c38bedcf65..4d1ea0cdc1 100644 --- a/xen/arch/arm/guest_walk.c +++ b/xen/arch/arm/guest_walk.c @@ -185,7 +185,8 @@ static int guest_walk_sd(const struct vcpu *v, static int get_ipa_output_size(struct domain *d, register_t tcr, unsigned int *output_size) { -unsigned int ips; +#ifdef CONFIG_ARM_64 +register_t ips; static const unsigned int ipa_sizes[7] = { TCR_EL1_IPS_32_BIT_VAL, @@ -200,7 +201,7 @@ static int get_ipa_output_size(struct domain *d, register_t tcr, if ( is_64bit_domain(d) ) { /* Get the intermediate physical address size. */ -ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT; +ips = tcr & TCR_EL1_IPS_MASK; /* * Return an error on reserved IPA output-sizes and if the IPA @@ -211,9 +212,10 @@ static int get_ipa_output_size(struct domain *d, register_t tcr, if ( ips > TCR_EL1_IPS_48_BIT ) return -EFAULT; -*output_size = ipa_sizes[ips]; +*output_size = ipa_sizes[ips >> TCR_EL1_IPS_SHIFT]; } else +#endif *output_size = TCR_EL1_IPS_40_BIT_VAL; return 0; -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [xen-unstable-smoke test] 114332: regressions - FAIL
Hi, On 11/10/17 08:30, Roger Pau Monné wrote: Adding Julien and Stefano. On Wed, Oct 11, 2017 at 07:17:59AM +, osstest service owner wrote: flight 114332 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/114332/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl 7 xen-boot fail REGR. vs. 114299 Seems like Xen is not able to start on the Exynos or Cubietrucks anymore: I have posted a fix few minutes ago (see [1]). It is because on 32 environment, the assembler does seem to support 64-bit constant and will ignore the top 32-bit. Cheers, [1] https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg01394.html -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel